Add image selection and swapping in the visual editor#2717
Add image selection and swapping in the visual editor#2717azizmejri1 wants to merge 8 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}
There was a problem hiding this comment.
Code Review
This pull request adds a new feature to the visual editor for selecting and swapping images, either by providing a URL or by uploading a new file. However, my security audit identified two significant issues: an unrestricted file upload vulnerability in the IPC handler that could lead to stored XSS, and a sensitive data leak via postMessage due to the use of a wildcard target origin, which is not permissible for sensitive data as per repository guidelines. Additionally, there is some redundant logic for filename generation between the frontend and backend, and the AST analysis and transformation logic for finding <img> tags could be more robust by performing a recursive search. Addressing these vulnerabilities and improvements is crucial for maintaining the security and robustness of the application.
🔍 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}
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) |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
azizmejri1
removed
the
needs-human:review-issue
|
@BugBot run |
| const VALID_IMAGE_TYPES = [ | ||
| "image/jpeg", | ||
| "image/png", | ||
| "image/gif", | ||
| "image/webp", | ||
| ]; |
There was a problem hiding this comment.
🟡 MEDIUM | duplication (1/3 reviewers, validated)
VALID_IMAGE_TYPES duplicated between client and server
This same list is defined independently here (as VALID_IMAGE_TYPES) and in visual_editing_handlers.ts (as VALID_IMAGE_MIME_TYPES). If one is updated without the other, the client and server validations diverge — the client might accept a type the server rejects, giving the user a confusing delayed error at apply time.
💡 Suggestion: Extract the list into a shared constant in src/ipc/types/visual-editing.ts and import it from both locations.
| Apply | ||
| </Button> | ||
| </div> | ||
| ) : ( |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility (1/3 reviewers, validated)
Error messages not announced to screen readers
The URL and file error messages are rendered as plain <p> elements. Screen reader users won't be informed when these appear because there's no role="alert" or aria-live region. Additionally, the URL input lacks aria-invalid and aria-describedby to associate it with the error.
💡 Suggestion: Add role="alert" to the error paragraphs and connect them to their inputs:
{urlError && <p id="url-error" role="alert" className="text-xs text-red-500">{urlError}</p>}
// and on the Input:
aria-invalid={!!urlError}
aria-describedby={urlError ? "url-error" : undefined}
azizmejri1
force-pushed
the
feat/image-visual-editor
branch
from
8efaa5e to
b56158a
Compare
|
@BugBot run |
🔍 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 (4 items)
🚫 Dropped False Positives (9 items)
Generated by Dyadbot multi-agent code review |
| <p className="text-xs font-mono truncate mt-1" title={currentSrc}> | ||
| {currentSrc || "none"} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
🟡 MEDIUM | stale-ui-state
"Current source" display does not update after swapping
currentSrc comes from the initial analyzeComponent AST analysis and only updates when a different component is selected. After the user swaps an image (via URL or upload), the footer still shows the old source value, creating a confusing disconnect — the user just changed the image but the UI says the source is still the old one.
💡 Suggestion: Track a local appliedSrc state that updates when onSwap is called, and display that in the footer. For example:
const [appliedSrc, setAppliedSrc] = useState(currentSrc);
// In useEffect: setAppliedSrc(currentSrc);
// In handlers: setAppliedSrc(newSrc); before calling onSwap(...);
| let hasImage = false; | ||
| let imageSrc: string | undefined; | ||
|
|
||
| const tagName = foundElement.openingElement.name; |
There was a problem hiding this comment.
🟡 MEDIUM | edge-case
Dynamic image sources silently overwritten with static value
When an <img> has a dynamic src (e.g., src={myImgUrl} or src={props.imageUrl}), extractStaticSrc returns undefined but hasImage is still set to true. The user sees the image swap popover with "Current source: none" — misleading, since there IS a source, it's just dynamic.
More critically, if the user swaps the image, transformContent will replace the dynamic expression (e.g., {myImgUrl}) with a static string literal, silently breaking the dynamic behavior with no warning.
💡 Suggestion: Add a check for dynamic image sources analogous to the existing isDynamic detection. When the <img> has a src attribute but extractStaticSrc returns undefined, set a flag like isDynamicImage: true and either disable the swap popover or show a warning in the UI.
| console.error("Failed to analyze component", err); | ||
| setIsDynamicComponent(false); | ||
| setHasStaticText(false); | ||
| setHasImage(false); |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
handleTextUpdated (line 310) silently drops imageSrc and imageUpload from pending changes
The handleTextUpdated function (line 292-322, outside this diff hunk) builds a new pending change entry that preserves styles from the existing entry but does not preserve imageSrc or imageUpload. If a user first swaps an image (which stores imageSrc + imageUpload in pending changes via handleImageSwap), and then edits text on the same component, the image change data is silently lost.
This is the same class of bug that was previously fixed in sendStyleModification (which now correctly preserves these fields on lines 176-177 of VisualEditingToolbar.tsx).
💡 Suggestion: Add imageSrc: existing?.imageSrc and imageUpload: existing?.imageUpload to the pending change object in handleTextUpdated at line 310-318.
|
@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 have been resolved. Good test coverage with both unit tests and E2E. The remaining issues are code health/polish concerns suitable for follow-up. Issues Summary
🚫 Dropped False Positives (7 items)
Generated by Dyadbot multi-agent code review |
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity (2/3 reviewers)
Orphaned image files on source code transform failure
Image files are written to public/images/ and git add-ed (lines 78-107) before the source code transformation runs (lines 142-161). If transformContent throws an error or the source file write fails, the image files remain on disk and staged in git with no corresponding source code reference. There is no cleanup in the catch block.
💡 Suggestion: Track the written file paths and delete them in the catch block, or defer the file writes until after all source code transforms succeed.
| return updated; | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 MEDIUM | duplication (2/3 reviewers)
Pending change construction logic duplicated across 3 locations
The pattern of building a full VisualEditingChange object (with componentId, componentName, relativePath, lineNumber, styles, textContent, imageSrc, imageUpload) is repeated in:
sendStyleModification(this file, line ~169)handleImageSwap(this file, line ~337)handleTextUpdated(PreviewIframe.tsx, line ~310)
Each location must remember to preserve all fields from other edit types. This fragile pattern has already caused bugs (existing comments #14 and #24 about dropping imageSrc/imageUpload and textContent).
💡 Suggestion: Extract a shared mergePendingChange(existing, partial) helper that automatically preserves all unrelated fields when applying a partial update.
| </div> | ||
| )} | ||
|
|
||
| {/* Current source display */} |
There was a problem hiding this comment.
🟡 MEDIUM | dark-mode (1/3 reviewers, validated)
No dark mode support in ImageSwapPopover
This component uses hardcoded light-mode colors (text-gray-500, text-red-500, border-t) with no dark: variants. Other components in the codebase consistently provide dark mode variants (e.g., text-gray-600 dark:text-gray-400). In dark mode, helper text and error messages will have poor contrast against the dark background.
💡 Suggestion: Add dark: variants: text-gray-500 dark:text-gray-400 for helper text, text-red-500 dark:text-red-400 for errors, and use a theme-aware border variable for the separator.
| return; | ||
| } | ||
| setUrlError(null); | ||
| onSwap(trimmed); |
There was a problem hiding this comment.
🟡 MEDIUM | error-recovery (1/3 reviewers, validated)
Failed image URL is saved as a pending change with no undo path
When a user enters a URL and clicks Apply, onSwap immediately stores the URL in pending changes. If the image fails to load, a toast error appears ("Image failed to load"), but the broken URL remains in pending changes. If the user then clicks "Save Changes", the broken image URL is written to the source file. There is no indication in the popover that the URL failed, and no way to revert other than manually re-entering the old URL.
💡 Suggestion: Consider removing or marking the pending image change when dyad-image-load-error is received, or show the error inline in the popover so the user has context.
Enable users to select <img> elements in the visual editor and swap their source by entering a URL or uploading a new file. This extends the existing visual editing pipeline with image-specific detection, transformation, and UI. Changes: - Extend IPC schemas with imageSrc and imageUpload fields - Add image detection to AST analyzeComponent (self and child <img>) - Add image src AST transformation in transformContent - Handle image file uploads to public/images/ in save handler - Add modify-dyad-image-src iframe message handler for live preview - Create ImageSwapPopover component with URL and Upload modes - Wire up hasImage/currentImageSrc state through PreviewIframe to toolbar - Add comprehensive tests for image analysis and transformation https://claude.ai/code/session_01BtuNFxQdcBRQGoy78fDR9r
- Add useEffect to sync urlValue when switching between image components - Add error message when user selects unsupported file type - Remove duplicate filename generation from client (server is sole authority) - Add server-side MIME type validation and file size limit for image uploads - Use recursive search for nested <img> elements in transformContent - Use recursive search for nested <img> elements in analyzeComponent Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add URL validation with error feedback in ImageSwapPopover - Add FileReader onerror handler for failed file reads - Add image onerror handler to update overlay coordinates on load failure - Fix double-wrapped error message in visual editing handler - Extract inline onSwap handler into named handleImageSwap function - Remove dead handleGetImageSrc code and get-dyad-image-src handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix data-loss bug: sendStyleModification now preserves imageSrc and imageUpload in pending changes - Extract duplicated image src extraction logic into extractStaticSrc helper - Add image load error feedback: preview iframe now shows toast when image fails to load Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
azizmejri1
force-pushed
the
feat/image-visual-editor
branch
from
875df08 to
955cd9c
Compare
|
@BugBot run |
🎭 Playwright Test Results❌ Some tests failed
Summary: 235 passed, 2 failed, 6 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/auto_approve.spec.ts \
e2e-tests/template-create-nextjs.spec.ts
|
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🚫 Dropped False Positives (20 items)
Generated by Dyadbot multi-agent code review |
| } from "../../utils/visual_editing_utils"; | ||
| import { normalizePath } from "../../../../../shared/normalizePath"; | ||
|
|
||
| const MAX_IMAGE_SIZE = 10_000_000; // ~7.5MB decoded |
There was a problem hiding this comment.
🟡 MEDIUM | size-limit-mismatch (2/3 reviewers)
Client/server image size limits are mismatched
The client-side check in ImageSwapPopover.tsx allows files up to 7.5 * 1024 * 1024 = 7,864,320 bytes. When base64-encoded (with data URL prefix), this produces ~10,485,760 characters — which exceeds the server-side MAX_IMAGE_SIZE of 10,000,000.
A user could upload a file that passes client validation (e.g., a 7.4 MB image) but gets rejected by the server with a confusing "image is too large" error after they've already seen it previewed successfully.
💡 Suggestion: Either derive the server limit from the client limit (accounting for base64 overhead: Math.ceil(7.5 * 1024 * 1024 / 3) * 4 + 100), or define a shared raw-byte-size constant and derive both checks from it. Also fix the comment which says ~7.5MB decoded but the actual decoded size is ~7.3MB.
3 participants
closes #2634