From 2a9666e2f330c3be356a4a5a7eb8318c1e0533c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Sat, 23 May 2026 01:51:04 +0100 Subject: [PATCH] fix(web): preview chat images in modal (#1030) --- .../task/chat/context-items/image-item.tsx | 42 ++++++------ .../task/chat/file-attachment.test.ts | 36 +++++----- .../components/task/chat/file-attachment.ts | 16 ----- .../task/chat/image-preview-dialog.tsx | 67 +++++++++++++++++++ .../task/chat/messages/chat-message.test.tsx | 32 ++++++++- .../task/chat/messages/chat-message.tsx | 8 +-- .../task/chat/messages/rich-blocks.tsx | 13 ++-- .../task/chat/queued-ghost-message.test.tsx | 57 ++++++++-------- .../task/chat/queued-ghost-message.tsx | 21 ++---- 9 files changed, 180 insertions(+), 112 deletions(-) create mode 100644 apps/web/components/task/chat/image-preview-dialog.tsx diff --git a/apps/web/components/task/chat/context-items/image-item.tsx b/apps/web/components/task/chat/context-items/image-item.tsx index 68a62ba716..de9aa1ef9a 100644 --- a/apps/web/components/task/chat/context-items/image-item.tsx +++ b/apps/web/components/task/chat/context-items/image-item.tsx @@ -1,49 +1,49 @@ "use client"; import { memo, useState, useCallback } from "react"; -import { Dialog, DialogContent, DialogTitle } from "@kandev/ui/dialog"; +import { Dialog, DialogContent } from "@kandev/ui/dialog"; import type { ImageContextItem } from "@/lib/types/context"; +import { + IMAGE_PREVIEW_DIALOG_CONTENT_CLASSNAME, + ImagePreviewContent, +} from "@/components/task/chat/image-preview-dialog"; import { ContextChip } from "./context-chip"; export const ImageItem = memo(function ImageItem({ item }: { item: ImageContextItem }) { const [dialogOpen, setDialogOpen] = useState(false); + const previewSrc = item.attachment.preview; const handleClick = useCallback(() => { setDialogOpen(true); }, []); - const preview = ( + const preview = previewSrc ? (
{/* eslint-disable-next-line @next/next/no-img-element -- base64 preview */} - Preview + Preview
- ); + ) : undefined; return ( <> - - - Image preview - {/* eslint-disable-next-line @next/next/no-img-element -- base64 preview */} - Full size preview - - + {previewSrc && ( + + + + + + )} ); }); diff --git a/apps/web/components/task/chat/file-attachment.test.ts b/apps/web/components/task/chat/file-attachment.test.ts index 9afa5b872d..7d7371b00d 100644 --- a/apps/web/components/task/chat/file-attachment.test.ts +++ b/apps/web/components/task/chat/file-attachment.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { openImageInWindow, processFile } from "./file-attachment"; +import { processFile } from "./file-attachment"; const UUID_V4_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; @@ -58,26 +58,22 @@ describe("processFile in insecure context (HTTP, no crypto.randomUUID)", () => { expect(attachment!.isImage).toBe(true); expect(attachment!.preview).toMatch(/^data:image\/png;base64,/); }); -}); -describe("openImageInWindow", () => { - it("falls back to image/png when the MIME type is not in the safe allowlist", () => { - let written = ""; - const fakeWin = { document: { write: (s: string) => (written = s) } }; - const openSpy = vi.spyOn(window, "open").mockReturnValue(fakeWin as unknown as Window); - openImageInWindow('image/png" onerror="alert(1)', "AAAA"); - expect(openSpy).toHaveBeenCalledWith("", "_blank", "noopener,noreferrer"); - expect(written).toContain("data:image/png;base64,AAAA"); - expect(written).not.toContain("onerror"); - openSpy.mockRestore(); - }); + it("treats non-previewable image MIME types as regular file attachments", async () => { + const ImageSpy = vi.fn(); + vi.stubGlobal("crypto", {}); + vi.stubGlobal("FileReader", FakeFileReader); + vi.stubGlobal("Image", ImageSpy); - it("passes through a safe MIME type unchanged", () => { - let written = ""; - const fakeWin = { document: { write: (s: string) => (written = s) } }; - const openSpy = vi.spyOn(window, "open").mockReturnValue(fakeWin as unknown as Window); - openImageInWindow("image/jpeg", "ZZZ"); - expect(written).toContain("data:image/jpeg;base64,ZZZ"); - openSpy.mockRestore(); + const file = new File(["svg"], "icon.svg", { type: "image/svg+xml" }); + Object.defineProperty(file, "size", { value: 3 }); + + const attachment = await processFile(file); + expect(attachment).not.toBeNull(); + expect(attachment!.id).toMatch(UUID_V4_REGEX); + expect(attachment!.isImage).toBe(false); + expect(attachment!.mimeType).toBe("image/svg+xml"); + expect(attachment!.preview).toBeUndefined(); + expect(ImageSpy).not.toHaveBeenCalled(); }); }); diff --git a/apps/web/components/task/chat/file-attachment.ts b/apps/web/components/task/chat/file-attachment.ts index 6245b2453a..1c835378cc 100644 --- a/apps/web/components/task/chat/file-attachment.ts +++ b/apps/web/components/task/chat/file-attachment.ts @@ -29,22 +29,6 @@ export function formatBytes(bytes: number): string { return `${parseFloat((bytes / Math.pow(k, i)).toFixed(1))} ${sizes[i]}`; } -/** - * Open a base64-encoded image in a new browser window for full-size viewing. - * MIME type is constrained to PREVIEWABLE_IMAGE_TYPES so a crafted value can't - * break out of the `src` attribute in document.write. `noopener` keeps the - * opened window from accessing window.opener. - */ -export function openImageInWindow(mimeType: string, data: string): void { - const safeMime = PREVIEWABLE_IMAGE_TYPES.includes(mimeType) ? mimeType : "image/png"; - const win = window.open("", "_blank", "noopener,noreferrer"); - if (win) { - win.document.write( - ``, - ); - } -} - function isPreviewableImage(mimeType: string): boolean { return PREVIEWABLE_IMAGE_TYPES.includes(mimeType); } diff --git a/apps/web/components/task/chat/image-preview-dialog.tsx b/apps/web/components/task/chat/image-preview-dialog.tsx new file mode 100644 index 0000000000..67cbf5a427 --- /dev/null +++ b/apps/web/components/task/chat/image-preview-dialog.tsx @@ -0,0 +1,67 @@ +"use client"; + +import { Dialog, DialogContent, DialogTitle, DialogTrigger } from "@kandev/ui/dialog"; +import { cn } from "@/lib/utils"; + +type ImagePreviewDialogProps = { + src: string; + alt: string; + thumbnailClassName?: string; + interactive?: boolean; +}; + +export const IMAGE_PREVIEW_DIALOG_CONTENT_CLASSNAME = + "flex w-fit max-w-[calc(100vw-1rem)] items-center justify-center overflow-hidden p-2 sm:max-w-[calc(100vw-2rem)] sm:p-3"; + +const IMAGE_PREVIEW_IMAGE_CLASSNAME = + "block h-auto max-h-[calc(100dvh-5rem)] w-[min(92vw,1100px)] max-w-full rounded object-contain"; + +type ImagePreviewContentProps = { + src: string; + alt: string; +}; + +export function ImagePreviewContent({ src, alt }: ImagePreviewContentProps) { + return ( + <> + Image preview + {/* eslint-disable-next-line @next/next/no-img-element -- base64 preview URL */} + {alt} + + ); +} + +export function ImagePreviewDialog({ + src, + alt, + thumbnailClassName, + interactive = true, +}: ImagePreviewDialogProps) { + if (!interactive) { + return ( + // eslint-disable-next-line @next/next/no-img-element -- base64 preview URL + {alt} + ); + } + + return ( + + + + + + + + + ); +} diff --git a/apps/web/components/task/chat/messages/chat-message.test.tsx b/apps/web/components/task/chat/messages/chat-message.test.tsx index bf0a6f05be..8ed50b98eb 100644 --- a/apps/web/components/task/chat/messages/chat-message.test.tsx +++ b/apps/web/components/task/chat/messages/chat-message.test.tsx @@ -1,6 +1,6 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import type { ReactNode } from "react"; -import { render } from "@testing-library/react"; +import { fireEvent, render, screen } from "@testing-library/react"; import { StateProvider } from "@/components/state-provider"; import { ChatMessage } from "./chat-message"; import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; @@ -8,6 +8,14 @@ import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/li const SENDER_TASK_ID = "task-sender"; const SENDER_TITLE = "Fix login bug"; const SENDER_BADGE_SELECTOR = "[data-testid='sender-task-badge']"; +const PNG_BASE64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII="; +const OPEN_ATTACHMENT_1_LABEL = "Open Attachment 1"; +const FULL_SIZE_ATTACHMENT_1_ALT = "Full size Attachment 1"; + +afterEach(() => { + vi.restoreAllMocks(); +}); function userMessage(overrides: Partial): Message { return { @@ -131,3 +139,23 @@ describe("ChatMessage sender badge", () => { expect(container.querySelector(SENDER_BADGE_SELECTOR)).toBeNull(); }); }); + +describe("ChatMessage image attachments", () => { + it("opens image attachments in an in-app preview dialog", () => { + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + renderWithSender([], { + attachments: [{ type: "image", data: PNG_BASE64, mime_type: "image/png" }], + }); + + fireEvent.click(screen.getByRole("button", { name: OPEN_ATTACHMENT_1_LABEL })); + + expect(openSpy).not.toHaveBeenCalled(); + const dialog = screen.getByRole("dialog"); + const preview = screen.getByAltText(FULL_SIZE_ATTACHMENT_1_ALT); + expect(dialog.className).toContain("w-fit"); + expect(dialog.className).toContain("max-w-[calc(100vw-1rem)]"); + expect(preview.className).toContain("w-[min(92vw,1100px)]"); + expect(preview.className).toContain("max-h-[calc(100dvh-5rem)]"); + expect(preview.getAttribute("src")).toBe(`data:image/png;base64,${PNG_BASE64}`); + }); +}); diff --git a/apps/web/components/task/chat/messages/chat-message.tsx b/apps/web/components/task/chat/messages/chat-message.tsx index 93d7cf2479..ddd5270d06 100644 --- a/apps/web/components/task/chat/messages/chat-message.tsx +++ b/apps/web/components/task/chat/messages/chat-message.tsx @@ -14,7 +14,7 @@ import { normalizeMarkdown, remarkPlugins, } from "@/components/shared/markdown-components"; -import { openImageInWindow } from "@/components/task/chat/file-attachment"; +import { ImagePreviewDialog } from "@/components/task/chat/image-preview-dialog"; type ChatMessageProps = { comment: Message; @@ -211,13 +211,11 @@ function UserMessageContent({ {hasAttachments && (
{imageAttachments.map((att, index) => ( - /* eslint-disable-next-line @next/next/no-img-element -- base64 data URLs are not compatible with next/image */ - {`Attachment openImageInWindow(att.mime_type, att.data)} + thumbnailClassName="max-h-48 max-w-full rounded-lg object-contain transition-opacity hover:opacity-90" /> ))} {fileAttachments.map((att, index) => ( diff --git a/apps/web/components/task/chat/messages/rich-blocks.tsx b/apps/web/components/task/chat/messages/rich-blocks.tsx index e4ef3aab18..9dab3e04f1 100644 --- a/apps/web/components/task/chat/messages/rich-blocks.tsx +++ b/apps/web/components/task/chat/messages/rich-blocks.tsx @@ -5,6 +5,7 @@ import type { Message } from "@/lib/types/http"; import type { ContentBlock, RichMetadata } from "@/components/task/chat/types"; import { DiffViewBlock } from "@/components/task/chat/messages/diff-view-block"; import { TodoMessage } from "@/components/task/chat/messages/todo-message"; +import { ImagePreviewDialog } from "@/components/task/chat/image-preview-dialog"; import { normalizeDiffString } from "@/lib/diff"; import type { FileDiffData } from "@/lib/diff/types"; @@ -120,16 +121,18 @@ function ContentBlockView({ block }: { block: ContentBlock }) { Image
{block.data && ( - // eslint-disable-next-line @next/next/no-img-element -- base64 data URIs are not optimizable by next/image - Agent image )} {block.uri && !block.data && ( - // eslint-disable-next-line @next/next/no-img-element -- external agent image URIs - Agent image + )} ); diff --git a/apps/web/components/task/chat/queued-ghost-message.test.tsx b/apps/web/components/task/chat/queued-ghost-message.test.tsx index fb36f9d374..eb02239fcd 100644 --- a/apps/web/components/task/chat/queued-ghost-message.test.tsx +++ b/apps/web/components/task/chat/queued-ghost-message.test.tsx @@ -5,6 +5,7 @@ import type { QueuedMessage } from "@/lib/state/slices/session/types"; afterEach(() => { cleanup(); + vi.restoreAllMocks(); }); vi.mock("@kandev/ui/tooltip", () => ({ @@ -18,6 +19,8 @@ const PNG_BASE64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII="; const ATTACHMENT_1_ALT = "Attachment 1"; +const OPEN_ATTACHMENT_1_LABEL = "Open Attachment 1"; +const FULL_SIZE_ATTACHMENT_1_ALT = "Full size Attachment 1"; function entry(overrides: Partial = {}): QueuedMessage { return { @@ -44,9 +47,10 @@ describe("QueuedGhostMessage attachment thumbnails", () => { onRemove={() => {}} />, ); - const img = screen.getByAltText(ATTACHMENT_1_ALT) as HTMLImageElement; + const trigger = screen.getByRole("button", { name: OPEN_ATTACHMENT_1_LABEL }); + const img = trigger.querySelector("img") as HTMLImageElement; expect(img.src).toBe(`data:image/png;base64,${PNG_BASE64}`); - expect(img.className).toContain("cursor-pointer"); + expect(trigger.className).toContain("cursor-pointer"); }); it("renders a file chip for non-image (resource) attachments", () => { @@ -64,7 +68,23 @@ describe("QueuedGhostMessage attachment thumbnails", () => { expect(screen.getByText("Attachment")).toBeTruthy(); }); - it("opens the image via keyboard (Enter) when focused in display mode", () => { + it("renders image thumbnails as accessible dialog triggers in display mode", () => { + render( + {}} + onRemove={() => {}} + />, + ); + const trigger = screen.getByRole("button", { name: OPEN_ATTACHMENT_1_LABEL }); + expect(trigger.getAttribute("type")).toBe("button"); + expect(trigger.querySelector("img")).toBeTruthy(); + }); + + it("opens the image in a preview dialog when clicked in display mode", () => { const openSpy = vi.spyOn(window, "open").mockReturnValue(null); render( { onRemove={() => {}} />, ); - const img = screen.getByAltText(ATTACHMENT_1_ALT) as HTMLImageElement; - expect(img.getAttribute("role")).toBe("button"); - expect(img.getAttribute("tabindex")).toBe("0"); - fireEvent.keyDown(img, { key: "Enter" }); - expect(openSpy).toHaveBeenCalledOnce(); - openSpy.mockRestore(); - }); - - it("opens the image in a new window when clicked in display mode", () => { - const openSpy = vi.spyOn(window, "open").mockReturnValue(null); - render( - {}} - onRemove={() => {}} - />, + fireEvent.click(screen.getByRole("button", { name: OPEN_ATTACHMENT_1_LABEL })); + expect(openSpy).not.toHaveBeenCalled(); + expect(screen.getByRole("dialog")).toBeTruthy(); + expect(screen.getByAltText(FULL_SIZE_ATTACHMENT_1_ALT).getAttribute("src")).toBe( + `data:image/png;base64,${PNG_BASE64}`, ); - fireEvent.click(screen.getByAltText(ATTACHMENT_1_ALT)); - expect(openSpy).toHaveBeenCalledOnce(); - openSpy.mockRestore(); }); - it("renders thumbnails read-only in edit mode (no click handler, no cursor-pointer)", () => { + it("renders thumbnails read-only in edit mode (no dialog trigger, no cursor-pointer)", () => { const openSpy = vi.spyOn(window, "open").mockReturnValue(null); render( { fireEvent.click(screen.getByTitle("Edit queued message")); const img = screen.getByAltText(ATTACHMENT_1_ALT) as HTMLImageElement; expect(img.className).not.toContain("cursor-pointer"); + expect(screen.queryByRole("button", { name: OPEN_ATTACHMENT_1_LABEL })).toBeNull(); fireEvent.click(img); expect(openSpy).not.toHaveBeenCalled(); - openSpy.mockRestore(); }); it("renders no thumbnail row when there are no attachments", () => { diff --git a/apps/web/components/task/chat/queued-ghost-message.tsx b/apps/web/components/task/chat/queued-ghost-message.tsx index 7452e54b8c..12f5d14136 100644 --- a/apps/web/components/task/chat/queued-ghost-message.tsx +++ b/apps/web/components/task/chat/queued-ghost-message.tsx @@ -19,7 +19,7 @@ import { Textarea } from "@kandev/ui/textarea"; import { cn } from "@/lib/utils"; import { QueueEntryNotFoundError } from "@/lib/api/domains/queue-api"; import { stripSystemTags } from "@/lib/utils/system-tags"; -import { openImageInWindow } from "@/components/task/chat/file-attachment"; +import { ImagePreviewDialog } from "@/components/task/chat/image-preview-dialog"; import { SenderTaskBadge, type SenderTaskInfo, @@ -44,29 +44,18 @@ function AttachmentRow({ attachments, interactive }: AttachmentRowProps) { if (attachments.length === 0) return null; const images = attachments.filter((a) => a.type === "image"); const files = attachments.filter((a) => a.type !== "image"); - const onImageKeyDown = (att: QueuedAttachment) => (e: React.KeyboardEvent) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - openImageInWindow(att.mime_type, att.data); - } - }; return (
{images.map((att, i) => ( - /* eslint-disable-next-line @next/next/no-img-element -- base64 data URLs are not compatible with next/image */ - {`Attachment openImageInWindow(att.mime_type, att.data) : undefined} - onKeyDown={interactive ? onImageKeyDown(att) : undefined} /> ))} {files.map((_, i) => (