Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions e2e-tests/fixtures/engine/image-basic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
OK, I'm going to write an app with an image now...

<dyad-write path="src/pages/Index.tsx" description="write-description">
import { MadeWithDyad } from "@/components/made-with-dyad";

const Index = () => {
return (
<div className="min-h-screen flex items-center justify-center bg-gray-100">
<div className="text-center">
<h1 className="text-4xl font-bold mb-4">Welcome to Your Blank App</h1>
<img src="/placeholder.svg" alt="Hero image" className="mx-auto mb-4 w-64 h-64" />
<p className="text-xl text-gray-600">
Start building your amazing project here!
</p>
</div>
<MadeWithDyad />
</div>
);
};

export default Index;
</dyad-write>

And it's done!
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
=== src/pages/Index.tsx ===
import { MadeWithDyad } from "@/components/made-with-dyad";

const Index = () => {
return (
<div className="min-h-screen flex items-center justify-center bg-gray-100">
<div className="text-center">
<h1 className="text-4xl font-bold mb-4">Welcome to Your Blank App</h1>
<img
src="https://example.com/new-hero.png"
alt="Hero image"
className="mx-auto mb-4 w-64 h-64" />
<p className="text-xl text-gray-600">Start building your amazing project here!
</p>
</div>
<MadeWithDyad />
</div>
);
};

export default Index;
74 changes: 74 additions & 0 deletions e2e-tests/visual_editing.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,80 @@ testSkipIfWindows("edit text of the selected component", async ({ po }) => {
});
});

testSkipIfWindows("swap image via URL", async ({ po }) => {
await po.setUpDyadPro();
await po.sendPrompt("tc=image-basic");
await po.approveProposal();

// Wait for the app to rebuild with the new code
await po.previewPanel.clickPreviewPickElement();

// Wait for the image element to appear in the iframe after rebuild
const heroImage = po.previewPanel
.getPreviewIframeElement()
.contentFrame()
.getByRole("img", { name: "Hero image" });
await expect(heroImage).toBeVisible({ timeout: Timeout.LONG });

// Select the image element in the preview
await heroImage.click();

// Wait for the toolbar to appear (check for the Margin button which is always visible)
const marginButton = po.page.getByRole("button", { name: "Margin" });
await expect(marginButton).toBeVisible({
timeout: Timeout.MEDIUM,
});

// Ensure the toolbar has proper coordinates before clicking
await expect(async () => {
const box = await marginButton.boundingBox();
expect(box).not.toBeNull();
expect(box!.y).toBeGreaterThan(0);
}).toPass({ timeout: Timeout.MEDIUM });

// Click the Swap Image button to open the image popover
const swapImageButton = po.page.getByRole("button", { name: "Swap Image" });
await expect(swapImageButton).toBeVisible({ timeout: Timeout.LONG });
await swapImageButton.click();

// Wait for the Image Source popover to appear
const imagePopover = po.page
.locator('[role="dialog"]')
.filter({ hasText: "Image Source" });
await expect(imagePopover).toBeVisible({
timeout: Timeout.LONG,
});

// Enter a new image URL
const urlInput = po.page.getByLabel("Image URL");
await urlInput.fill("https://example.com/new-hero.png");

// Click Apply to submit the new URL
await po.page.getByRole("button", { name: "Apply" }).click();

// Close the popover
await po.page.keyboard.press("Escape");

// Verify the visual changes dialog appears
await expect(po.page.getByText(/\d+ component[s]? modified/)).toBeVisible({
timeout: Timeout.MEDIUM,
});

// Save the changes
await po.page.getByRole("button", { name: "Save Changes" }).click();

// Wait for the success toast
await po.toastNotifications.waitForToastWithText(
"Visual changes saved to source files",
);

// Verify that the changes are applied to the codebase
await po.snapshotAppFiles({
name: "visual-editing-swap-image",
files: ["src/pages/Index.tsx"],
});
});

testSkipIfWindows("discard changes", async ({ po }) => {
await po.setUpDyadPro();
await po.sendPrompt("tc=basic");
Expand Down
207 changes: 207 additions & 0 deletions src/components/preview_panel/ImageSwapPopover.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
import { useState, useRef, useEffect } from "react";
import { ImageIcon, Upload, Link, Check } from "lucide-react";
import { Label } from "@/components/ui/label";
import { Input } from "@/components/ui/input";
import { Button } from "@/components/ui/button";
import { Tabs, TabsList, TabsTrigger } from "@/components/ui/tabs";
import { StylePopover } from "./StylePopover";
import { VALID_IMAGE_MIME_TYPES } from "@/ipc/types/visual-editing";

export interface ImageUploadData {
fileName: string;
base64Data: string;
mimeType: string;
}

interface ImageSwapPopoverProps {
currentSrc: string;
onSwap: (newSrc: string, uploadData?: ImageUploadData) => void;
}

export function ImageSwapPopover({
currentSrc,
onSwap,
}: ImageSwapPopoverProps) {
const [mode, setMode] = useState<"url" | "upload">("url");
const [urlValue, setUrlValue] = useState(currentSrc);
const [selectedFileName, setSelectedFileName] = useState<string | null>(null);
const [fileError, setFileError] = useState<string | null>(null);
const [urlError, setUrlError] = useState<string | null>(null);
const fileInputRef = useRef<HTMLInputElement>(null);

// Sync urlValue when a different component is selected
useEffect(() => {
setUrlValue(currentSrc);
setSelectedFileName(null);
setFileError(null);
setUrlError(null);
}, [currentSrc]);

const handleUrlSubmit = () => {
const trimmed = urlValue.trim();
if (!trimmed) {
setUrlError("Please enter a URL.");
return;
}
// Accept absolute URLs (http/https/protocol-relative) and root-relative paths
if (
!/^https?:\/\//i.test(trimmed) &&
!trimmed.startsWith("//") &&
!trimmed.startsWith("/")
) {
setUrlError(
"Please enter a valid URL (https://...) or an absolute path (/...).",
);
return;
}
setUrlError(null);
onSwap(trimmed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

};

const handleFileSelect = (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0];
if (!file) return;

if (!(VALID_IMAGE_MIME_TYPES as readonly string[]).includes(file.type)) {
setFileError("Unsupported file type. Please use JPG, PNG, GIF, or WebP.");
return;
}
if (file.size > 7.5 * 1024 * 1024) {
setFileError(
"Image is too large (max 7.5 MB). Please choose a smaller file.",
);
return;
}
setFileError(null);

setSelectedFileName(file.name);

const reader = new FileReader();
Comment on lines +77 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM | edge-case (3/3 reviewers)

No client-side file size limit before reading into memory

handleFileSelect validates the file type but not the file size before calling reader.readAsDataURL(file). A user could select a very large image (e.g. 100MB), which gets fully read into memory as a ~133MB base64 string, sent via postMessage to the iframe for preview, and stored in React state — all before the server's 10MB base64 limit rejects it much later at apply time. For very large files this could cause the renderer process to hang or crash.

💡 Suggestion: Add a file.size check right after the type check:

Suggested change
setSelectedFileName(file.name);
const reader = new FileReader();
if (!VALID_IMAGE_TYPES.includes(file.type)) {
setFileError("Unsupported file type. Please use JPG, PNG, GIF, or WebP.");
return;
}
if (file.size > 7.5 * 1024 * 1024) {
setFileError("Image is too large (max 7.5 MB). Please choose a smaller file.");
return;
}

reader.onload = () => {
const base64DataUrl = reader.result as string;

// The backend will generate the final unique filename.
// We just need a placeholder path for the pending change.
const sanitizedName = file.name.replace(/[^a-zA-Z0-9._-]/g, "_");
const newSrc = `/images/${sanitizedName}`;

onSwap(newSrc, {
fileName: file.name,
base64Data: base64DataUrl,
mimeType: file.type,
});
};
reader.onerror = () => {
setFileError(
"Failed to read the file. Please try again or choose a different file.",
);
setSelectedFileName(null);
};
reader.readAsDataURL(file);

// Clear input so same file can be selected again
e.target.value = "";
};

return (
<StylePopover
icon={<ImageIcon size={16} />}
title="Image Source"
tooltip="Swap Image"
>
<div className="space-y-3">
{/* Mode toggle tabs */}
<Tabs
value={mode}
onValueChange={(val) => setMode(val as "url" | "upload")}
>
<TabsList className="w-full h-8">
<TabsTrigger value="url" className="flex-1 gap-1 text-xs">
<Link size={12} />
URL
</TabsTrigger>
<TabsTrigger value="upload" className="flex-1 gap-1 text-xs">
<Upload size={12} />
Upload
</TabsTrigger>
</TabsList>
</Tabs>

{mode === "url" ? (
<div className="space-y-2">
<Label htmlFor="image-url" className="text-xs">
Image URL
</Label>
<Input
id="image-url"
type="text"
placeholder="https://example.com/image.png"
className="h-8 text-xs"
value={urlValue}
aria-invalid={!!urlError}
aria-describedby={urlError ? "image-url-error" : undefined}
onChange={(e) => {
setUrlValue(e.target.value);
setUrlError(null);
}}
onKeyDown={(e) => {
if (e.key === "Enter") handleUrlSubmit();
}}
/>
{urlError && (
<p
id="image-url-error"
role="alert"
className="text-xs text-red-500"
>
{urlError}
</p>
)}
<Button size="sm" onClick={handleUrlSubmit} className="w-full">
<Check size={14} className="mr-1" />
Apply
</Button>
</div>
) : (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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}

<div className="space-y-2">
<Label className="text-xs">Upload Image</Label>
<input
ref={fileInputRef}
type="file"
accept="image/jpeg,image/png,image/gif,image/webp"
className="hidden"
onChange={handleFileSelect}
/>
<Button
size="sm"
variant="outline"
onClick={() => fileInputRef.current?.click()}
className="w-full"
>
<Upload size={14} className="mr-1 shrink-0" />
<span className="truncate">
{selectedFileName || "Choose File"}
</span>
</Button>
{fileError && (
<p role="alert" className="text-xs text-red-500">
{fileError}
</p>
)}
<p className="text-xs text-gray-500">
Supports: JPG, PNG, GIF, WebP
</p>
</div>
)}

{/* Current source display */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

<div className="pt-2 border-t">
<Label className="text-xs text-gray-500">Current source</Label>
<p className="text-xs font-mono truncate mt-1" title={currentSrc}>
{currentSrc || "none"}
</p>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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(...);

</div>
</StylePopover>
);
}
13 changes: 13 additions & 0 deletions src/components/preview_panel/PreviewIframe.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
// AST Analysis State
const [isDynamicComponent, setIsDynamicComponent] = useState(false);
const [hasStaticText, setHasStaticText] = useState(false);
const [hasImage, setHasImage] = useState(false);
const [currentImageSrc, setCurrentImageSrc] = useState("");

// Device mode state
const deviceMode: DeviceMode = settings?.previewDeviceMode ?? "desktop";
Expand All @@ -262,6 +264,8 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
});
setIsDynamicComponent(result.isDynamic);
setHasStaticText(result.hasStaticText);
setHasImage(result.hasImage);
setCurrentImageSrc(result.imageSrc || "");

// Automatically enable text editing if component has static text
if (result.hasStaticText && iframeRef.current?.contentWindow) {
Expand All @@ -280,6 +284,8 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
console.error("Failed to analyze component", err);
setIsDynamicComponent(false);
setHasStaticText(false);
setHasImage(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

setCurrentImageSrc("");
}
};

Expand Down Expand Up @@ -542,6 +548,11 @@ 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.");
return;
}

if (event.data?.type === "dyad-component-coordinates-updated") {
if (event.data.coordinates) {
setCurrentComponentCoordinates(event.data.coordinates);
Expand Down Expand Up @@ -1368,6 +1379,8 @@ export const PreviewIframe = ({ loading }: { loading: boolean }) => {
iframeRef={iframeRef}
isDynamic={isDynamicComponent}
hasStaticText={hasStaticText}
hasImage={hasImage}
currentImageSrc={currentImageSrc}
/>
)}
</>
Expand Down
Loading
Loading