Add image selection and swapping in the visual editor#2717
Add image selection and swapping in the visual editor#2717azizmejri1 wants to merge 11 commits intodyad-sh:mainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello @azizmejri1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the visual editing experience by introducing comprehensive image management capabilities. It allows users to intuitively change image sources within their components, supporting both URL-based and file upload methods. The changes span across the frontend for UI interaction, the backend for file persistence and Git integration, and the core AST analysis and IPC layers to ensure seamless real-time updates and accurate code transformation. Highlights
Changelog
ActivityUsing Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryFound 4 new issue(s) flagged by 3 independent reviewers (consensus voting: only issues identified by 2+ reviewers are reported). Summary
Issues to Address
See inline comments for details. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
|
All contributors have signed the CLA ✍️ ✅ |
azizmejri1
removed
the
needs-human:review-issue
🔍 Dyadbot Code Review Summary (Round 2)Found 7 new issue(s) flagged by 3 independent reviewers (consensus voting: only issues identified by 2+ reviewers are reported). Summary
Issues to Address
⚪ Low Priority Issues (1 item)
See inline comments for details on HIGH/MEDIUM issues. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
azizmejri1
removed
the
needs-human:review-issue
|
@BugBot run |
- Fix stale event listeners on rapid image swaps using AbortController - Add dynamic image source detection with warning in ImageSwapPopover - Fix handleTextUpdated dropping imageSrc/imageUpload from pending changes - Extract mergePendingChange helper to prevent data loss across edit types - Add orphaned image file cleanup on transform failure - Fix client/server image size limit mismatch (derive server limit from client) - Update "Current source" display to reflect applied swaps - Add dark mode variants to ImageSwapPopover - Remove broken image URL from pending changes on load failure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All 12 unresolved review threads have been addressed with code changes and resolved. The core data-integrity and race-condition issues are fixed, but manual testing of the full image swap flow would be valuable before merge. Unresolved Threads
No unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — product principles were clear enough for all decisions made in this run. 🤖 Generated by Claude Code |
| findImg(child); | ||
| if (hasImage) return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | logic (2/3 reviewers)
findImg does not traverse JSXExpressionContainer children
The recursive findImg function only walks node.children, which contains direct JSX children. However, <img> elements inside JSXExpressionContainer nodes (e.g., {condition && <img src="..." />} or {items.map(i => <img />)}) are nested under node.expression, not node.children. These images will not be detected by analyzeComponent, so the Swap Image button will not appear for components that contain images inside conditional expressions.
Meanwhile, transformContent uses Babel's path.traverse which does handle these cases — so there's a detection/transformation mismatch.
💡 Suggestion: When findImg encounters a node with type === 'JSXExpressionContainer', recursively traverse its expression property. Also handle ConditionalExpression (traverse consequent/alternate) and LogicalExpression (traverse right).
| ...existing, | ||
| imageSrc: undefined, | ||
| imageUpload: undefined, | ||
| }); |
There was a problem hiding this comment.
🟡 MEDIUM | duplication (2/3 reviewers)
Redundant identical branches in image-load-error handler
The first branch (line 568: if styles has keys) and the third branch (line 578: else) execute the exact same code — both spread existing and set imageSrc: undefined, imageUpload: undefined. Only the middle branch (else if !textContent → delete) is distinct.
💡 Suggestion: Simplify to two branches:
const hasOtherChanges =
(existing.styles && Object.keys(existing.styles).length > 0) ||
existing.textContent;
if (hasOtherChanges) {
updated.set(elementId, { ...existing, imageSrc: undefined, imageUpload: undefined });
} else {
updated.delete(elementId);
}
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. All previously flagged HIGH issues from prior review rounds have been addressed. The remaining issues are minor and non-blocking. Issues Summary
🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. All previously flagged HIGH issues from prior review rounds have been addressed. The remaining issues are minor and non-blocking. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (11 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
needs-human:final-check
| } | ||
|
|
||
| if (event.data?.type === "dyad-image-load-error") { | ||
| showError("Image failed to load. Please check the URL and try again."); |
There was a problem hiding this comment.
🟡 MEDIUM | error-recovery (2/3 reviewers)
Broken image remains visible in preview after load error
When a URL is entered and the image fails to load, the dyad-image-load-error handler correctly removes the broken entry from pending changes and shows a toast error. However, the iframe's <img> element still has the broken src set — the user sees a broken image icon in the preview with no way to revert it (short of re-selecting the component). The preview and the pending changes state become out of sync.
💡 Suggestion: When a dyad-image-load-error is received, send a modify-dyad-image-src message back to the iframe with the original/previous src to restore the image in the preview. The original src could be passed in the error event data or tracked in currentImageSrc state.
🎭 Playwright Test Results✅ All tests passed!
Total: 237 tests passed (9 flaky) (6 skipped)
|
|
@BugBot run |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| imageSrc: newSrc, | ||
| ...(uploadData && { | ||
| imageUpload: uploadData, | ||
| }), | ||
| }), |
There was a problem hiding this comment.
🔴 Stale imageUpload persists when switching from upload to URL mode, causing URL to be overwritten
When a user first uploads an image and then switches to URL mode to enter a URL instead, the stale imageUpload data from the first action is never cleared from pending changes. This causes the saved changes to write the old uploaded file and overwrite the user's URL.
Root Cause
In handleImageSwap (src/components/preview_panel/VisualEditingToolbar.tsx:348-350), when uploadData is undefined (URL mode), the conditional spread ...(uploadData && { imageUpload: uploadData }) evaluates to ...false, which is a no-op. This means imageUpload is not a key in the partial object passed to mergePendingChange.
In mergePendingChange (src/ipc/types/visual-editing.ts:124-125), the check "imageUpload" in partial returns false, so it falls back to existing?.imageUpload — preserving the stale upload data from the previous action.
When changes are saved, the handler at src/pro/main/ipc/handlers/visual_editing_handlers.ts:84 sees change.imageUpload is truthy, writes the old uploaded file to disk, and at line 113 overwrites change.imageSrc with the uploaded file path (/images/${finalFileName}), discarding the URL the user actually intended.
Impact: The user's intended URL is silently replaced with the previously uploaded image. An unnecessary file is written to disk.
| imageSrc: newSrc, | |
| ...(uploadData && { | |
| imageUpload: uploadData, | |
| }), | |
| }), | |
| imageSrc: newSrc, | |
| imageUpload: uploadData, |
Was this helpful? React with 👍 or 👎 to provide feedback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Confidence score: 3/5
- The broken image
srcremains insrc/components/preview_panel/PreviewIframe.tsx, so failed loads can still render a bad image even after error handling—this is user-visible behavior. - Orphaned staging files are possible in
src/pro/main/ipc/handlers/visual_editing_handlers.tsif a write fails before tracking, which could leak temp media on disk. - Pay close attention to
src/components/preview_panel/PreviewIframe.tsx,src/pro/main/ipc/handlers/visual_editing_handlers.ts,src/components/preview_panel/ImageSwapPopover.tsx- image error handling, cleanup tracking, and file re-selection UX.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/preview_panel/PreviewIframe.tsx">
<violation number="1" location="src/components/preview_panel/PreviewIframe.tsx:559">
P2: When an image fails to load, the `dyad-image-load-error` handler correctly removes the broken entry from pending changes and shows an error toast. However, the iframe's `<img>` element still has the broken `src` set — the user sees a broken image icon in the preview with no way to revert it. The preview and pending changes state become out of sync. After removing the pending change, send a `modify-dyad-image-src` message back to the iframe with the original `src` (available via `currentImageSrc` state) to restore the image in the preview.</violation>
</file>
<file name="src/components/preview_panel/ImageSwapPopover.tsx">
<violation number="1" location="src/components/preview_panel/ImageSwapPopover.tsx:48">
P2: Clear the file input in the validation-error branches so users can reselect the same file after an error. Right now the early returns skip `e.target.value = ""`, preventing re-selection of the same invalid file.</violation>
</file>
<file name="src/pro/main/ipc/handlers/visual_editing_handlers.ts">
<violation number="1" location="src/pro/main/ipc/handlers/visual_editing_handlers.ts:99">
P2: The staging copy in .dyad/media isn’t tracked for cleanup until after the public write succeeds, so failures between the media write and the later push can leave orphaned files. Track the media path immediately after writing it so the catch cleanup can remove it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return; | ||
| } | ||
|
|
||
| if (event.data?.type === "dyad-image-load-error") { |
There was a problem hiding this comment.
P2: When an image fails to load, the dyad-image-load-error handler correctly removes the broken entry from pending changes and shows an error toast. However, the iframe's <img> element still has the broken src set — the user sees a broken image icon in the preview with no way to revert it. The preview and pending changes state become out of sync. After removing the pending change, send a modify-dyad-image-src message back to the iframe with the original src (available via currentImageSrc state) to restore the image in the preview.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/PreviewIframe.tsx, line 559:
<comment>When an image fails to load, the `dyad-image-load-error` handler correctly removes the broken entry from pending changes and shows an error toast. However, the iframe's `<img>` element still has the broken `src` set — the user sees a broken image icon in the preview with no way to revert it. The preview and pending changes state become out of sync. After removing the pending change, send a `modify-dyad-image-src` message back to the iframe with the original `src` (available via `currentImageSrc` state) to restore the image in the preview.</comment>
<file context>
@@ -542,6 +556,35 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
return;
}
+ if (event.data?.type === "dyad-image-load-error") {
+ showError("Image failed to load. Please check the URL and try again.");
+ // Remove the broken image from pending changes
</file context>
| const trimmed = urlValue.trim(); | ||
| if (!trimmed) { | ||
| setUrlError("Please enter a URL."); | ||
| return; |
There was a problem hiding this comment.
P2: Clear the file input in the validation-error branches so users can reselect the same file after an error. Right now the early returns skip e.target.value = "", preventing re-selection of the same invalid file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/ImageSwapPopover.tsx, line 48:
<comment>Clear the file input in the validation-error branches so users can reselect the same file after an error. Right now the early returns skip `e.target.value = ""`, preventing re-selection of the same invalid file.</comment>
<file context>
@@ -0,0 +1,222 @@
+ const trimmed = urlValue.trim();
+ if (!trimmed) {
+ setUrlError("Please enter a URL.");
+ return;
+ }
+ // Accept absolute URLs (http/https/protocol-relative) and root-relative paths
</file context>
| await fsPromises.writeFile( | ||
| path.join(mediaDir, finalFileName), | ||
| buffer, | ||
| ); | ||
| await ensureDyadGitignored(appPath); |
There was a problem hiding this comment.
P2: The staging copy in .dyad/media isn’t tracked for cleanup until after the public write succeeds, so failures between the media write and the later push can leave orphaned files. Track the media path immediately after writing it so the catch cleanup can remove it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pro/main/ipc/handlers/visual_editing_handlers.ts, line 99:
<comment>The staging copy in .dyad/media isn’t tracked for cleanup until after the public write succeeds, so failures between the media write and the later push can leave orphaned files. Track the media path immediately after writing it so the catch cleanup can remove it.</comment>
<file context>
@@ -40,11 +48,91 @@ export function registerVisualEditingHandlers() {
+ // Save to .dyad/media as a staging copy
+ const mediaDir = path.join(appPath, DYAD_MEDIA_DIR_NAME);
+ await fsPromises.mkdir(mediaDir, { recursive: true });
+ await fsPromises.writeFile(
+ path.join(mediaDir, finalFileName),
+ buffer,
</file context>
| await fsPromises.writeFile( | |
| path.join(mediaDir, finalFileName), | |
| buffer, | |
| ); | |
| await ensureDyadGitignored(appPath); | |
| await fsPromises.writeFile( | |
| path.join(mediaDir, finalFileName), | |
| buffer, | |
| ); | |
| writtenImagePaths.push(path.join(mediaDir, finalFileName)); | |
| await ensureDyadGitignored(appPath); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab35747a4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| imageSrc: newSrc, | ||
| ...(uploadData && { | ||
| imageUpload: uploadData, | ||
| }), |
There was a problem hiding this comment.
handleImageSwap only sets imageUpload when uploadData is provided, so mergePendingChange keeps any prior upload metadata on URL-only updates. If a user uploads an image and then switches to a URL before saving, the pending change still contains imageUpload; the main apply-visual-editing-changes handler then treats that upload as authoritative and rewrites imageSrc to /images/<generated>, so the saved source no longer matches the URL the user just applied.
Useful? React with 👍 / 👎.
| for (const filePath of writtenImagePaths) { | ||
| try { | ||
| await fsPromises.unlink(filePath); | ||
| } catch { |
There was a problem hiding this comment.
The failure cleanup only unlinks files from disk, but uploaded images were already staged earlier with gitAdd. If any later step in this handler throws (for example, while transforming or writing component files), the repo can be left with staged additions pointing to deleted files (AD state), which pollutes subsequent commits and can break later git operations; cleanup needs to unstage/reset those paths as well.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds image selection and swapping to the visual editor, allowing users to replace Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ImageSwapPopover
participant VisualEditingToolbar
participant PreviewIframe
participant IframeWorker as dyad-visual-editor-client.js
participant IPC as visual_editing_handlers.ts
User->>PreviewIframe: Click image element
PreviewIframe->>IPC: analyze-component (appId, componentId)
IPC-->>PreviewIframe: {hasImage, imageSrc, isDynamicImage}
PreviewIframe->>VisualEditingToolbar: hasImage=true, currentImageSrc, isDynamicImage
User->>ImageSwapPopover: Open "Swap Image" popover
Note over ImageSwapPopover: URL tab or Upload tab
alt URL mode
User->>ImageSwapPopover: Enter URL, click Apply
ImageSwapPopover->>VisualEditingToolbar: onSwap(newSrc, undefined)
VisualEditingToolbar->>IframeWorker: postMessage(modify-dyad-image-src, src=URL)
IframeWorker->>IframeWorker: Set imgEl.src = URL
alt Image loads
IframeWorker-->>PreviewIframe: dyad-component-coordinates-updated
else Image fails
IframeWorker-->>PreviewIframe: dyad-image-load-error
PreviewIframe->>PreviewIframe: Remove imageSrc from pendingChanges
end
VisualEditingToolbar->>PreviewIframe: setPendingChanges (imageSrc=URL)
else Upload mode
User->>ImageSwapPopover: Choose file
ImageSwapPopover->>ImageSwapPopover: FileReader.readAsDataURL
ImageSwapPopover->>VisualEditingToolbar: onSwap(/images/name, uploadData{base64})
VisualEditingToolbar->>IframeWorker: postMessage(modify-dyad-image-src, src=base64DataURL)
IframeWorker->>IframeWorker: Set imgEl.src = base64DataURL
VisualEditingToolbar->>PreviewIframe: setPendingChanges (imageSrc+imageUpload)
end
User->>PreviewIframe: Click "Save Changes"
PreviewIframe->>IPC: apply-visual-editing-changes(appId, changes)
opt change.imageUpload present
IPC->>IPC: Validate MIME type + size
IPC->>IPC: Write buffer to public/images/ and .dyad/media/
IPC->>IPC: git add public/images/finalFileName
IPC->>IPC: change.imageSrc = /images/timestamp-name
end
IPC->>IPC: transformContent (update <img src>)
IPC->>IPC: git add + git commit source file
IPC-->>PreviewIframe: success
PreviewIframe-->>User: Toast "Visual changes saved to source files"
Last reviewed commit: ab35747 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| updated.set( | ||
| selectedComponent.id, | ||
| mergePendingChange(existing, { | ||
| componentId: selectedComponent.id, | ||
| componentName: selectedComponent.name, | ||
| relativePath: selectedComponent.relativePath, | ||
| lineNumber: selectedComponent.lineNumber, | ||
| imageSrc: newSrc, | ||
| ...(uploadData && { | ||
| imageUpload: uploadData, | ||
| }), | ||
| }), | ||
| ); | ||
| return updated; | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Stale imageUpload when switching from upload → URL mode
When a user uploads a file first and then switches to the URL tab and clicks Apply, the old imageUpload data is silently preserved in the pending change. This is because ...(uploadData && { imageUpload: uploadData }) spreads nothing when uploadData is undefined, so "imageUpload" in partial is false in mergePendingChange — and it falls back to existing?.imageUpload.
When the user saves, the server handler detects change.imageUpload, writes the previously-uploaded file, and overwrites change.imageSrc with that file's path — discarding the URL the user just entered.
Fix: always include the imageUpload key in the partial (even as undefined) so mergePendingChange clears the old value:
| updated.set( | |
| selectedComponent.id, | |
| mergePendingChange(existing, { | |
| componentId: selectedComponent.id, | |
| componentName: selectedComponent.name, | |
| relativePath: selectedComponent.relativePath, | |
| lineNumber: selectedComponent.lineNumber, | |
| imageSrc: newSrc, | |
| ...(uploadData && { | |
| imageUpload: uploadData, | |
| }), | |
| }), | |
| ); | |
| return updated; | |
| }); | |
| } | |
| }; | |
| updated.set( | |
| selectedComponent.id, | |
| mergePendingChange(existing, { | |
| componentId: selectedComponent.id, | |
| componentName: selectedComponent.name, | |
| relativePath: selectedComponent.relativePath, | |
| lineNumber: selectedComponent.lineNumber, | |
| imageSrc: newSrc, | |
| imageUpload: uploadData, | |
| }), | |
| ); |
With imageUpload: uploadData always present as a key in the partial, "imageUpload" in partial is always true, so mergePendingChange uses partial.imageUpload (either the new upload data or undefined).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/preview_panel/VisualEditingToolbar.tsx
Line: 340-356
Comment:
**Stale `imageUpload` when switching from upload → URL mode**
When a user uploads a file first and then switches to the URL tab and clicks Apply, the old `imageUpload` data is silently preserved in the pending change. This is because `...(uploadData && { imageUpload: uploadData })` spreads **nothing** when `uploadData` is `undefined`, so `"imageUpload" in partial` is `false` in `mergePendingChange` — and it falls back to `existing?.imageUpload`.
When the user saves, the server handler detects `change.imageUpload`, writes the previously-uploaded file, and **overwrites** `change.imageSrc` with that file's path — discarding the URL the user just entered.
Fix: always include the `imageUpload` key in the partial (even as `undefined`) so `mergePendingChange` clears the old value:
```suggestion
updated.set(
selectedComponent.id,
mergePendingChange(existing, {
componentId: selectedComponent.id,
componentName: selectedComponent.name,
relativePath: selectedComponent.relativePath,
lineNumber: selectedComponent.lineNumber,
imageSrc: newSrc,
imageUpload: uploadData,
}),
);
```
With `imageUpload: uploadData` always present as a key in the partial, `"imageUpload" in partial` is always `true`, so `mergePendingChange` uses `partial.imageUpload` (either the new upload data or `undefined`).
How can I resolve this? If you propose a fix, please make it concise.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (8 items)
Generated by Dyadbot multi-agent code review |
| lineNumber: selectedComponent.lineNumber, | ||
| imageSrc: newSrc, | ||
| ...(uploadData && { | ||
| imageUpload: uploadData, |
There was a problem hiding this comment.
🔴 HIGH | data-integrity
Stale imageUpload persists when switching from file upload to URL mode
When handleImageSwap is called without uploadData (URL mode), the spread ...(uploadData && { imageUpload: uploadData }) produces no keys, so imageUpload is never set in the partial object. In mergePendingChange, since "imageUpload" in partial is false, it falls back to existing?.imageUpload, preserving stale upload data from a previous file upload.
If a user uploads a file, then switches to URL mode and enters a URL, the old imageUpload data remains. When saved, the server sees imageUpload, writes the old file to disk, and overwrites change.imageSrc with the file path, silently discarding the URL the user entered.
💡 Suggestion: Always set imageUpload explicitly:
| imageUpload: uploadData, | |
| imageSrc: newSrc, | |
| imageUpload: uploadData, |
| // Check if the element itself is an <img> | ||
| if (tagName.type === "JSXIdentifier" && tagName.name === "img") { | ||
| hasImage = true; | ||
| imageSrc = extractStaticSrc(foundElement.openingElement); | ||
| // If there's a src attribute but extractStaticSrc returned undefined, it's dynamic | ||
| const hasSrcAttr = foundElement.openingElement.attributes.some( | ||
| (attr: any) => attr.type === "JSXAttribute" && attr.name?.name === "src", | ||
| ); | ||
| if (hasSrcAttr && !imageSrc) { | ||
| isDynamicImage = true; | ||
| } | ||
| } | ||
|
|
||
| // Recursively check descendants for <img> elements | ||
| if (!hasImage && foundElement) { | ||
| const findImg = (node: any): void => { | ||
| if (!node || hasImage) return; | ||
|
|
||
| if ( | ||
| node.type === "JSXElement" && | ||
| node.openingElement.name.type === "JSXIdentifier" && | ||
| node.openingElement.name.name === "img" | ||
| ) { | ||
| hasImage = true; | ||
| imageSrc = extractStaticSrc(node.openingElement); | ||
| const hasSrcAttr = node.openingElement.attributes.some( | ||
| (attr: any) => | ||
| attr.type === "JSXAttribute" && attr.name?.name === "src", | ||
| ); | ||
| if (hasSrcAttr && !imageSrc) { | ||
| isDynamicImage = true; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | duplication
Image detection logic duplicated for self vs. descendant <img>
The block at lines 441-451 (checking if the element itself is an <img>) and the block inside findImg (lines 459-472) contain identical logic: call extractStaticSrc, check for hasSrcAttr, set isDynamicImage. This is a ~10-line copy-paste.
💡 Suggestion: Extract into a helper, e.g.:
function analyzeImgElement(openingElement: any): { imageSrc?: string; isDynamicImage: boolean } {
const imageSrc = extractStaticSrc(openingElement);
const hasSrcAttr = openingElement.attributes.some(
(attr: any) => attr.type === "JSXAttribute" && attr.name?.name === "src",
);
return { imageSrc, isDynamicImage: hasSrcAttr && !imageSrc };
}| export interface ImageUploadData { | ||
| fileName: string; | ||
| base64Data: string; | ||
| mimeType: string; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | duplication
ImageUploadData interface duplicates the Zod schema shape
This manually declares { fileName, base64Data, mimeType } which is the exact same shape as the imageUpload field in VisualEditingChangeSchema. If the schema changes, this interface won't automatically stay in sync.
💡 Suggestion: Derive from the Zod schema:
| export interface ImageUploadData { | |
| fileName: string; | |
| base64Data: string; | |
| mimeType: string; | |
| } | |
| export type ImageUploadData = NonNullable<VisualEditingChange['imageUpload']>; |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
@azizmejri1 overall LGTM (good feature!)
let's first resolve the review comments (I commented on a couple specific issues) before merging
3 participants
closes #2634