Added image upload handling to CTA Card #1421
Conversation
ref https://linear.app/ghost/issue/PLG-323/add-image-upload-handling-to-cta-card - Wired up the ability to upload an image to the CTA Card.
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces enhanced media handling capabilities for the Call To Action (CTA) card in the Koenig-Lexical component. The changes focus on adding new props and methods to support image upload and removal functionality within the Changes
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
93-95: Add file validation before upload.Consider adding file type and size validation before proceeding with the upload.
const onFileChange = async (e) => { + const files = e.target.files; + const file = files[0]; + + if (!file.type.startsWith('image/')) { + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.uploadError = 'Please select an image file'; + }); + return; + } + + if (file.size > 5 * 1024 * 1024) { // 5MB limit + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.uploadError = 'Image size should be less than 5MB'; + }); + return; + } + handleImageChange(e.target.files); };
97-103: Clear error states when removing media.Consider clearing any error states when removing media to ensure a clean slate.
const onRemoveMedia = () => { editor.update(() => { const node = $getNodeByKey(nodeKey); node.imageUrl = ''; node.hasImage = false; + node.uploadError = null; + node.isLoading = false; }); };packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
213-231: LGTM! Comprehensive test coverage for image functionality.The test effectively covers:
- Image upload workflow
- Blob URL verification
- Image removal
However, consider adding tests for:
- Error scenarios (invalid file types, upload failures)
- Loading states
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(4 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
[error] 125-125: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (4)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
29-40: LGTM! Context and ref setup is well implemented.The implementation correctly uses React hooks and context for file upload functionality.
120-134: LGTM! Props are correctly passed to CtaCard.The implementation properly integrates the new image handling functionality with the CtaCard component.
🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
82-85: LGTM! Props are well-defined with proper types and defaults.The implementation:
- Correctly adds new image handling props
- Includes proper PropTypes definitions
- Provides appropriate default values
Also applies to: 337-340, 359-362
143-147: LGTM! MediaUploadSetting integration is correct.The component properly utilizes the new image handling props.
| const handleImageChange = async (files) => { | ||
| const result = await imageUploader.upload(files); | ||
| // reset original src so it can be replaced with preview and upload progress | ||
| editor.update(() => { | ||
| const node = $getNodeByKey(nodeKey); | ||
| node.imageUrl = result?.[0].url; | ||
| node.hasImage = true; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state management.
The image upload implementation should include:
- Error handling for failed uploads
- Loading state management for better UX
- File validation before upload
Consider this implementation:
const handleImageChange = async (files) => {
+ try {
+ editor.update(() => {
+ const node = $getNodeByKey(nodeKey);
+ node.isLoading = true;
+ });
+
const result = await imageUploader.upload(files);
- // reset original src so it can be replaced with preview and upload progress
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.imageUrl = result?.[0].url;
node.hasImage = true;
+ node.isLoading = false;
});
+ } catch (error) {
+ editor.update(() => {
+ const node = $getNodeByKey(nodeKey);
+ node.isLoading = false;
+ node.uploadError = error.message;
+ });
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleImageChange = async (files) => { | |
| const result = await imageUploader.upload(files); | |
| // reset original src so it can be replaced with preview and upload progress | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.imageUrl = result?.[0].url; | |
| node.hasImage = true; | |
| }); | |
| }; | |
| const handleImageChange = async (files) => { | |
| try { | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.isLoading = true; | |
| }); | |
| const result = await imageUploader.upload(files); | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.imageUrl = result?.[0].url; | |
| node.hasImage = true; | |
| node.isLoading = false; | |
| }); | |
| } catch (error) { | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.isLoading = false; | |
| node.uploadError = error.message; | |
| }); | |
| } | |
| }; |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1421 +/- ##
==========================================
+ Coverage 90.81% 90.83% +0.02%
==========================================
Files 184 185 +1
Lines 17164 17224 +60
Branches 1899 1913 +14
==========================================
+ Hits 15587 15645 +58
- Misses 1567 1569 +2
Partials 10 10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
94-104: Consider adding type checking for files.While the handlers work correctly, consider adding type checking for the files to ensure only valid image files are processed:
const onFileChange = async (e) => { + const files = e.target.files; + if (!files || !files.length) return; + + const file = files[0]; + if (!file.type.startsWith('image/')) { + console.error('Invalid file type. Please upload an image.'); + return; + } + handleImageChange(e.target.files); };
131-131: Avoid assignment in expression.The assignment in the ref callback makes the code less readable and was flagged by static analysis.
-setFileInputRef={ref => fileInputRef.current = ref} +setFileInputRef={(ref) => { + fileInputRef.current = ref; +}}🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
213-231: Consider adding more test cases for robustness.While the current test covers the basic flow, consider adding tests for:
- Invalid file types
- Upload failures
- Multiple file selection
Example test case for invalid file type:
test('rejects invalid file types', async function () { const filePath = path.relative(process.cwd(), __dirname + '/../fixtures/invalid.txt'); await focusEditor(page); await insertCard(page, {cardName: 'call-to-action'}); const fileChooserPromise = page.waitForEvent('filechooser'); await page.click('[data-testid="media-upload-placeholder"]'); const fileChooser = await fileChooserPromise; await fileChooser.setFiles([filePath]); const imgLocator = page.locator('[data-kg-card="call-to-action"] img[src^="blob:"]'); await expect(imgLocator).not.toBeVisible(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(5 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
[error] 131-131: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (5)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
3-3: LGTM! Required imports and context for file upload functionality.The additions of
useRefandfileUploaderare appropriate for implementing file upload handling.Also applies to: 29-29
37-40: Add error handling and loading state management.The image upload implementation should include error handling and loading states for better user experience.
Also applies to: 84-92
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (3)
83-85: LGTM! Props follow consistent naming convention.The new props for file handling are well-named and consistent with the existing prop naming pattern.
143-143: LGTM! Proper prop forwarding to MediaUploadSetting.The file handling props are correctly passed to the MediaUploadSetting component.
Also applies to: 146-147
339-341: LGTM! Complete PropTypes and defaultProps definitions.The new props are properly typed with PropTypes and have appropriate default values.
Also applies to: 361-363
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
84-92:⚠️ Potential issueAdd error handling and loading state management.
The image upload implementation should include error handling and loading state management for better UX.
const handleImageChange = async (files) => { + try { + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.isLoading = true; + }); + const result = await imageUploader.upload(files); // reset original src so it can be replaced with preview and upload progress editor.update(() => { const node = $getNodeByKey(nodeKey); node.imageUrl = result?.[0].url; node.hasImage = true; + node.isLoading = false; }); + } catch (error) { + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.isLoading = false; + node.uploadError = error.message; + }); + } };
🧹 Nitpick comments (2)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
132-132: Avoid assignment in expression.The file input ref assignment should be moved out of the JSX for better readability.
- setFileInputRef={ref => fileInputRef.current = ref} + setFileInputRef={ref => { + fileInputRef.current = ref; + }}🧰 Tools
🪛 Biome (1.9.4)
[error] 132-132: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
267-283: Add assertions for image dimensions and loading state.The image preview test should verify:
- Loading state during upload
- Final image dimensions
- Error state visibility
test('has image preview', async function () { const filePath = path.relative(process.cwd(), __dirname + `/../fixtures/large-image.jpeg`); await focusEditor(page); await insertCard(page, {cardName: 'call-to-action'}); const fileChooserPromise = page.waitForEvent('filechooser'); await page.click('[data-testid="media-upload-placeholder"]'); const fileChooser = await fileChooserPromise; await fileChooser.setFiles([filePath]); + // Verify loading state + await expect(page.locator('[data-testid="media-upload-loading"]')).toBeVisible(); + await page.waitForSelector('[data-testid="media-upload-filled"]'); const previewImage = await page.locator('[data-testid="media-upload-filled"] img'); await expect(previewImage).toBeVisible(); + + // Verify image dimensions + const dimensions = await previewImage.evaluate((img) => ({ + width: img.naturalWidth, + height: img.naturalHeight + })); + expect(dimensions.width).toBeGreaterThan(0); + expect(dimensions.height).toBeGreaterThan(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
[error] 132-132: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
| const onFileChange = async (e) => { | ||
| handleImageChange(e.target.files); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file validation before upload.
The file input handler should validate the files before uploading.
const onFileChange = async (e) => {
+ const files = e.target.files;
+ if (!files || files.length === 0) {
+ return;
+ }
+
+ const file = files[0];
+ const validTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'];
+ if (!validTypes.includes(file.type)) {
+ editor.update(() => {
+ const node = $getNodeByKey(nodeKey);
+ node.uploadError = 'Invalid file type. Please upload an image.';
+ });
+ return;
+ }
+
handleImageChange(e.target.files);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onFileChange = async (e) => { | |
| handleImageChange(e.target.files); | |
| }; | |
| const onFileChange = async (e) => { | |
| const files = e.target.files; | |
| if (!files || files.length === 0) { | |
| return; | |
| } | |
| const file = files[0]; | |
| const validTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp']; | |
| if (!validTypes.includes(file.type)) { | |
| editor.update(() => { | |
| const node = $getNodeByKey(nodeKey); | |
| node.uploadError = 'Invalid file type. Please upload an image.'; | |
| }); | |
| return; | |
| } | |
| handleImageChange(e.target.files); | |
| }; |
| test('can add and remove CTA Card image', async function () { | ||
| const filePath = path.relative(process.cwd(), __dirname + `/../fixtures/large-image.jpeg`); | ||
|
|
||
| await focusEditor(page); | ||
| await insertCard(page, {cardName: 'call-to-action'}); | ||
|
|
||
| const fileChooserPromise = page.waitForEvent('filechooser'); | ||
|
|
||
| await page.click('[data-testid="media-upload-placeholder"]'); | ||
|
|
||
| const fileChooser = await fileChooserPromise; | ||
| await fileChooser.setFiles([filePath]); | ||
|
|
||
| const imgLocator = page.locator('[data-kg-card="call-to-action"] img[src^="blob:"]'); | ||
| const imgElement = await imgLocator.first(); | ||
| await expect(imgElement).toHaveAttribute('src', /blob:/); | ||
| await page.click('[data-testid="media-upload-remove"]'); | ||
| await expect(imgLocator).not.toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error scenarios.
The image upload tests should cover error scenarios such as:
- Invalid file types
- Upload failures
- Network errors
Here's an example test case to add:
test('shows error for invalid file type', async function () {
const filePath = path.relative(process.cwd(), __dirname + '/../fixtures/invalid.txt');
await focusEditor(page);
await insertCard(page, {cardName: 'call-to-action'});
const fileChooserPromise = page.waitForEvent('filechooser');
await page.click('[data-testid="media-upload-placeholder"]');
const fileChooser = await fileChooserPromise;
await fileChooser.setFiles([filePath]);
await expect(page.locator('[data-testid="upload-error"]')).toBeVisible();
await expect(page.locator('[data-testid="upload-error"]')).toHaveText('Invalid file type. Please upload an image.');
});
ref https://linear.app/ghost/issue/PLG-323/add-image-upload-handling-to-cta-card
Summary by CodeRabbit
New Features
Tests