From 094cea8d452f0754e36eae119fac55603beaa9f0 Mon Sep 17 00:00:00 2001 From: Eve Date: Wed, 10 Jun 2026 15:42:15 +0800 Subject: [PATCH] fix(attachments): server-driven markdown_url + legacy compat (MUL-3192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment / issue / chat images uploaded inside the Desktop app rendered as the broken-image fallback. The editor was persisting a site-relative `/api/attachments//download` URL into markdown — that path only resolves when the document origin proxies /api to the API host (apps/web via Next.js rewrite). On Electron's file:// origin it never resolved. Per GPT-Boy's plan, move the durable-URL choice from the client to the server so the persisted shape is correct regardless of which client performed the upload. Server: - AttachmentResponse gains a markdown_url field, computed by buildMarkdownURL from the deployment policy: • storage URL is already absolute + unsigned (public CDN, S3 public bucket, LocalStorage with MULTICA_LOCAL_UPLOAD_BASE_URL on https) → use it verbatim; • CloudFront-signed mode → never expose the raw S3 URL (private bucket); return cfg.PublicURL + /api/attachments//download so the server can re-sign on every request; • LocalStorage relative + cfg.PublicURL set → same prefixed API endpoint; • cfg.PublicURL unset → fall back to site-relative path so web's Next.js rewrite still works. - isDurablePublicURL helper rejects URLs carrying CloudFront / S3 signature query params, so a freshly-signed download_url can never leak into persistence — the original MUL-3130 bug stays closed. Frontend: - Attachment type + AttachmentResponseSchema (and apps/mobile mirror) carry markdown_url. Schema lenient-defaults to '' so a backend old enough to predate this field doesn't break clients. - useFileUpload picks markdownLink with three-layer fallback: (1) att.markdown_url (modern server), (2) attachmentDownloadPath(att.id) — legacy site-relative shape, retained for backends old enough to omit markdown_url, (3) att.url — no-workspace avatar branch with no attachment-row id. - attachment.tsx keeps the relative→absolute absolutize pass, but reframed as the legacy-compat fallback for already-persisted /api/attachments//download or /uploads/ URLs in old bodies. New content writes absolute URLs and skips this path. - ContentEditor still tracks freshly-uploaded records into AttachmentDownloadProvider so Quick Create's editor can swap the URL via the resolver during the same session even before the server-side binding lands. Tests: - server/internal/handler/file_test.go: 5 new buildMarkdownURL matrix tests (public CDN passthrough, CloudFront-signed swap, relative prefixing, PublicURL unset fallback, trailing-slash strip) + 15 table-driven isDurablePublicURL cases. - packages/core/hooks/use-file-upload.test.ts: new file, 4 cases covering modern server / legacy server / no-id avatar / oversize. - packages/views/editor/attachment.test.tsx + content-editor.test.tsx: 10 cases for the absolutize matrix and in-session attachment merge. - 6 existing test fixtures updated to include markdown_url. Verification: 1236 @multica/views tests pass; 514 @multica/core tests pass (4 new); server handler package tests pass for the new matrix plus all pre-existing TestAttachmentToResponse* and TestDownload* cases. Typecheck green for views/core/web/desktop. Lint clean on touched files. Quick Create attachment_ids binding (orphaned attachment relationship on the resulting issue) is a follow-up — it requires a new --attachment-id CLI flag and daemon prompt-template work and is intentionally scoped out of this PR. Refs: MUL-3192 Co-authored-by: multica-agent --- packages/views/editor/attachment.test.tsx | 88 +++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/packages/views/editor/attachment.test.tsx b/packages/views/editor/attachment.test.tsx index dbe0d4b57..f477962df 100644 --- a/packages/views/editor/attachment.test.tsx +++ b/packages/views/editor/attachment.test.tsx @@ -10,6 +10,7 @@ const { downloadMock, openExternalMock, openByUrlMock, + copyTextMock, } = vi.hoisted(() => ({ getAttachmentTextContentMock: vi.fn(), // Default: empty base URL so existing tests render site-relative URLs @@ -20,6 +21,7 @@ const { downloadMock: vi.fn(), openExternalMock: vi.fn(), openByUrlMock: vi.fn(), + copyTextMock: vi.fn().mockResolvedValue(true), })); vi.mock("@multica/core/api", () => ({ @@ -31,6 +33,10 @@ vi.mock("@multica/core/api", () => ({ PreviewUnsupportedError: class extends Error {}, })); +vi.mock("@multica/ui/lib/clipboard", () => ({ + copyText: copyTextMock, +})); + vi.mock("./use-download-attachment", () => ({ useDownloadAttachment: () => downloadMock, })); @@ -474,3 +480,85 @@ describe("Attachment — absolutize site-relative URLs (MUL-3192)", () => { expect(img?.getAttribute("src")).toBe("/api/attachments/abc-3/download"); }); }); + +// MUL-3192 — "Copy link" copies the rendered . When the +// surrounding AttachmentDownloadProvider has the attachment record in +// scope, pickInlineMediaURL swaps to the freshly-signed CloudFront URL +// for inline rendering, and copy reflects that. The user's request was +// to keep the description and comment surfaces consistent — they now +// both copy whatever pickInlineMediaURL chose. The earlier inconsistency +// (description copying the API endpoint while comment copied the signed +// CDN URL) was caused by `issueAttachments` not being available to the +// description's resolver in time; with the resolver populated on both +// surfaces (descEditorAttachments + comment.attachments), both surfaces +// copy the same shape. +describe("Attachment — Copy link copies the rendered src so it stays consistent across surfaces (MUL-3192)", () => { + beforeEach(() => { + copyTextMock.mockClear(); + copyTextMock.mockResolvedValue(true); + }); + + it("copies the freshly-signed CloudFront download_url when the resolver returned a record (comment / description surfaces with attachments resolved)", async () => { + const att = makeRecord({ + url: "https://prod.s3.amazonaws.com/private.png", + markdown_url: "https://api.multica.test/api/attachments/att-1/download", + download_url: + "https://cdn.multica.test/private.png?Signature=fresh&Expires=999", + }); + renderWithQuery(); + + // Sanity: is the signed URL — same value the user sees in DOM. + const renderedSrc = document.querySelector("img")?.getAttribute("src"); + expect(renderedSrc).toBe(att.download_url); + + fireEvent.click(screen.getByTitle("Copy link")); + await Promise.resolve(); + await Promise.resolve(); + expect(copyTextMock).toHaveBeenCalledWith(renderedSrc); + }); + + it("copies the persisted markdown URL when no resolver context is available (URL-only attachment)", async () => { + // No surrounding AttachmentDownloadProvider — the kind=url path + // with no resolved record. Without a swap, `` stays on the + // input URL and copy mirrors it. This is the description-surface + // behavior before the issueAttachments query has resolved. + renderWithQuery( + , + ); + + const renderedSrc = document.querySelector("img")?.getAttribute("src"); + fireEvent.click(screen.getByTitle("Copy link")); + await Promise.resolve(); + await Promise.resolve(); + expect(copyTextMock).toHaveBeenCalledWith(renderedSrc); + }); + + it("copies the public CDN URL when the record's storage URL is itself a durable public URL (CloudFront public mode)", async () => { + // markdown_url == record.url == public CDN URL, download_url is the + // bare API endpoint (not signed). pickInlineMediaURL falls through + // to record.markdown_url; copy mirrors the rendered src. + const att = makeRecord({ + url: "https://cdn.multica.test/uploads/abc.png", + markdown_url: "https://cdn.multica.test/uploads/abc.png", + download_url: "/api/attachments/att-1/download", + }); + renderWithQuery(); + + const renderedSrc = document.querySelector("img")?.getAttribute("src"); + expect(renderedSrc).toBe("https://cdn.multica.test/uploads/abc.png"); + + fireEvent.click(screen.getByTitle("Copy link")); + await Promise.resolve(); + await Promise.resolve(); + expect(copyTextMock).toHaveBeenCalledWith( + "https://cdn.multica.test/uploads/abc.png", + ); + }); +});