From f66a522d0223d53d21cb818a40766f2498368cfa Mon Sep 17 00:00:00 2001
From: Eve <eve@multica-ai.local>
Date: Tue, 9 Jun 2026 14:04:18 +0800
Subject: [PATCH] =?UTF-8?q?MUL-3130:=20address=20PR=20review=20=E2=80=94?=
 =?UTF-8?q?=20split=20markdown=20link=20from=20upload=20link,=20swap=20ren?=
 =?UTF-8?q?der=20src?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two follow-ups from GPT-Boy's review on PR #3937.

(1) Don't reroute every upload consumer through the workspace-gated
    download endpoint.

    The previous change made `useFileUpload`'s `link` field unconditionally
    return `/api/attachments/<id>/download` whenever the upload had an id.
    But `useFileUpload` is also used by avatar / logo pickers
    (account-tab, workspace-tab, agents/avatar-picker, squads/squad-detail-page)
    that persist `result.link` directly into `avatar_url`. Avatars are
    referenced cross-workspace (mention chips, member lists, inbox
    items), so binding their URL to a workspace-membership-gated
    endpoint would silently break cross-workspace avatar visibility.

    The fix splits the URL into two semantically distinct fields:

      - `link`         — same as `att.url` (legacy contract). Avatar /
                          logo callers continue to use this and remain
                          on whatever URL semantics the storage backend
                          dictates.
      - `markdownLink` — the stable per-attachment URL
                          `/api/attachments/<id>/download`. Only the
                          editor's markdown-persisting flow consumes
                          this. Falls back to `link` for the
                          no-workspace upload branch (where there is
                          no attachment-row id to address).

    `editor/extensions/file-upload.ts` switches `image.src` and
    `fileCard.href` to `markdownLink ?? link` so comment markdown gets
    the stable shape while avatar callers stay on `link` unchanged.

(2) Make the render-time img src loadable for token-mode clients.

    Persisting the stable `/api/attachments/<id>/download` URL fixes the
    expiry problem but the path itself sits behind `middleware.Auth`,
    which expects either a `multica_auth` cookie or a Bearer token in
    `Authorization`. Native `<img>`/`<video>` resource loads from
    token-mode clients (Electron's default mode, the mobile app,
    legacy-token web sessions) cannot attach the Authorization header,
    so the bare URL would 401 immediately rather than 30 minutes later.

    `Attachment.normalize` now runs the resolved record through a new
    `pickInlineMediaURL` helper that returns:

      - `record.download_url` when it's an absolute URL with a
         recognised CDN signature query (CloudFront-signed
         `Signature` / `Expires` / `Key-Pair-Id`, or
         `X-Amz-Signature` for raw S3 presigns) — these load as
         native resource src in any client.
      - else `record.url`, which on the LocalStorage backend carries
         a freshly-minted `/uploads/<key>?exp&sig` query whose
         signature IS the auth (token-mode-loadable). On non-CF S3
         backends this is the raw stored URL — same behaviour as
         today.
      - else the original input URL (legacy / unresolved markdown
         keeps its existing path).

    This gives the same effect for both `kind: "record"` and
    `kind: "url"` attachment inputs: once a record is in hand, the
    rendered media src is whichever URL the current backend exposes
    a working signature on.

Tests:

  - New `file-upload.test.ts` regression pinning that `markdownLink`
    is what lands in the markdown body when the upload result returns
    both a short-lived storage URL and a stable download path.
  - Updated `attachment.test.tsx` to reflect the new render-time
    swap (the rendered img src now follows the freshly signed URL,
    not the raw storage URL) and added a record-mode regression
    pinning the LocalStorage default — when `download_url` is the
    bare /api/attachments/<id>/download path, the renderer must fall
    through to the signed `record.url`.
  - Updated `chat-input.test.tsx` makeUpload helper for the new
    `markdownLink` UploadResult field.
  - 1222 frontend views tests + 507 core tests + typecheck across
    @multica/{core,ui,views} all pass.

Refs: MUL-3130, GitHub issue #3891. Builds on a740f7a35.
Co-authored-by: multica-agent <github@multica.ai>
---
 packages/core/hooks/use-file-upload.ts        | 61 +++++++++++++------
 .../views/chat/components/chat-input.test.tsx |  5 ++
 packages/views/editor/attachment.test.tsx     | 38 +++++++++++-
 packages/views/editor/attachment.tsx          | 57 ++++++++++++++++-
 .../editor/extensions/file-upload.test.ts     | 36 +++++++++++
 .../views/editor/extensions/file-upload.ts    | 11 +++-
 6 files changed, 184 insertions(+), 24 deletions(-)

diff --git a/packages/core/hooks/use-file-upload.ts b/packages/core/hooks/use-file-upload.ts
index 8e5273518..baaa31291 100644
--- a/packages/core/hooks/use-file-upload.ts
+++ b/packages/core/hooks/use-file-upload.ts
@@ -7,18 +7,41 @@ import { attachmentDownloadPath } from "../types/attachment-url";
 import { MAX_FILE_SIZE } from "../constants/upload";
 
 // Carries the full Attachment so editors that need preview metadata
-// (`content_type`, `download_url`) get it directly; `link` is the
-// stable persisted URL — `/api/attachments/<id>/download` — that the
-// editor writes into the markdown body. The previous design used
-// `att.url`, but on the LocalStorage backend that value became a
-// 30-min HMAC-signed `/uploads/<key>?exp&sig` URL after MUL-3132.
-// Persisting a short-lived signature into a permanent comment body
-// broke every image the moment the signature expired (MUL-3130).
-// `attachmentDownloadPath(att.id)` re-signs at request time, never
-// stops working until the attachment row is deleted, and resolves
-// the workspace from the row itself so it loads as a native <img>
-// src without any X-Workspace-* headers.
-export type UploadResult = Attachment & { link: string };
+// (`content_type`, `download_url`) get it directly. Two URL fields are
+// surfaced because they have different lifetimes:
+//
+//   `link`         — the same value as `att.url`. Short-lived for the
+//                    LocalStorage backend (HMAC-signed `/uploads/<key>`)
+//                    and a long-lived CDN URL on S3 / CloudFront. This
+//                    is what avatar / logo callers persist into
+//                    `avatar_url` style fields, and what URL-only
+//                    consumers (Markdown renderers without a record
+//                    in hand) get to load directly. Keeping it
+//                    semantically equal to `att.url` preserves the
+//                    pre-MUL-3130 contract for non-markdown callers
+//                    so avatar uploads do not get rerouted through
+//                    the workspace-membership-gated download endpoint.
+//
+//   `markdownLink` — the stable per-attachment URL
+//                    `/api/attachments/<id>/download`. Safe to embed
+//                    in markdown bodies that outlive the current
+//                    session because the server self-resolves the
+//                    workspace from the attachment row and re-signs /
+//                    proxies on every request. Empty when the upload
+//                    has no attachment-row id (the no-workspace
+//                    avatar branch of UploadFile, which falls back
+//                    to `link`). Only the editor's markdown-persisting
+//                    flow should use it; everything else uses `link`.
+//
+// MUL-3130 introduced the persisted-image regression by collapsing
+// these two semantics into a single `link` field. Keeping them
+// separate is what lets new comments embed a stable URL while
+// avatar / logo callers continue to write `att.url` into long-lived
+// fields without picking up workspace-membership gating.
+export type UploadResult = Attachment & {
+  link: string;
+  markdownLink: string;
+};
 
 export interface UploadContext {
   issueId?: string;
@@ -45,12 +68,14 @@ export function useFileUpload(
           commentId: ctx?.commentId,
           chatSessionId: ctx?.chatSessionId,
         });
-        // Avatar uploads (no workspace context) come back without an id;
-        // fall back to the storage URL so legacy avatar pickers that
-        // persist `att.url` directly keep working. Comment / issue
-        // uploads always carry an id.
-        const link = att.id ? attachmentDownloadPath(att.id) : att.url;
-        return { ...att, link };
+        // Avatar / no-workspace uploads come back without an
+        // attachment-row id (UploadFile's no-workspace branch returns
+        // {id, url, filename} that fails the AttachmentResponseSchema
+        // and degrades to the empty record). In that case the stable
+        // markdown URL doesn't exist — markdown callers fall back to
+        // `link` which mirrors `att.url`.
+        const markdownLink = att.id ? attachmentDownloadPath(att.id) : att.url;
+        return { ...att, link: att.url, markdownLink };
       } finally {
         setUploading(false);
       }
diff --git a/packages/views/chat/components/chat-input.test.tsx b/packages/views/chat/components/chat-input.test.tsx
index 580c09934..2845b3095 100644
--- a/packages/views/chat/components/chat-input.test.tsx
+++ b/packages/views/chat/components/chat-input.test.tsx
@@ -20,6 +20,11 @@ function makeUpload(overrides: Partial<UploadResult> & { id: string; link: strin
     content_type: "image/png",
     size_bytes: 1,
     created_at: new Date(0).toISOString(),
+    // markdownLink defaults to the same value as `link` so legacy
+    // tests assert the previous URL shape unless they pass an
+    // explicit override. Real callers always set it to the stable
+    // /api/attachments/<id>/download path via useFileUpload.
+    markdownLink: overrides.link,
     ...overrides,
   };
 }
diff --git a/packages/views/editor/attachment.test.tsx b/packages/views/editor/attachment.test.tsx
index 6eb543962..6215b6b26 100644
--- a/packages/views/editor/attachment.test.tsx
+++ b/packages/views/editor/attachment.test.tsx
@@ -136,7 +136,12 @@ describe("Attachment — image dispatch", () => {
     renderWithQuery(<Attachment attachment={{ kind: "record", attachment: att }} />);
     const img = document.querySelector("img");
     expect(img).toBeTruthy();
-    expect(img?.getAttribute("src")).toBe(att.url);
+    // The rendered src is the freshly-loadable URL — when the record
+    // carries a signed download_url (CloudFront / S3 presign style) it
+    // wins over the raw stored url so token-mode <img> loads work
+    // without an Authorization header. See pickInlineMediaURL in
+    // attachment.tsx for the rationale (MUL-3130 review follow-up).
+    expect(img?.getAttribute("src")).toBe(att.download_url);
     expect(img?.getAttribute("alt")).toBe("shot.png");
     expect(screen.getByTitle("View")).toBeTruthy();
     expect(screen.getByTitle("Download")).toBeTruthy();
@@ -176,7 +181,12 @@ describe("Attachment — image dispatch", () => {
       />,
     );
     const img = document.querySelector("img");
-    expect(img?.getAttribute("src")).toBe(att.url);
+    // Once the URL resolves to a record, the rendered src swaps to
+    // the record's signed download_url so the image is loadable in
+    // token-mode clients that can't attach Authorization headers
+    // (MUL-3130 review). The raw stored url is the fallback for
+    // unresolved markdown only.
+    expect(img?.getAttribute("src")).toBe(att.download_url);
     fireEvent.click(screen.getByTitle("Download"));
     expect(downloadMock).toHaveBeenCalledWith("att-1");
   });
@@ -229,6 +239,30 @@ describe("Attachment — image dispatch", () => {
     expect(screen.queryByTitle("View")).toBeNull();
     expect(screen.queryByTitle("Download")).toBeNull();
   });
+
+  it("stable /api/attachments/<id>/download download_url falls through to the freshly-signed record.url for token-mode loadability (MUL-3130)", () => {
+    // Pinned behavior: when an attachment record carries the default
+    // proxy-mode download_url (a bare /api/attachments/<id>/download
+    // path, which does NOT load as a native <img>/<video> src in
+    // token-mode clients because they can't attach Authorization
+    // headers), the renderer must fall through to record.url. On the
+    // LocalStorage backend record.url carries a freshly-minted
+    // /uploads/<key>?exp&sig query whose signature IS the auth, so
+    // the image loads regardless of how the client is authenticated.
+    // pickInlineMediaURL implements this fallback (attachment.tsx).
+    const signedStorageURL =
+      "/uploads/workspaces/ws-1/freshly-signed.png?exp=99&sig=fresh";
+    const att = makeRecord({
+      url: signedStorageURL,
+      // bare API path — no signature query, so pickInlineMediaURL
+      // skips download_url and uses record.url.
+      download_url: "/api/attachments/att-1/download",
+    });
+    renderWithQuery(<Attachment attachment={{ kind: "record", attachment: att }} />);
+    const img = document.querySelector("img");
+    expect(img?.getAttribute("src")).toBe(signedStorageURL);
+    expect(img?.getAttribute("src")).not.toContain("/api/attachments/");
+  });
 });
 
 describe("Attachment — html dispatch", () => {
diff --git a/packages/views/editor/attachment.tsx b/packages/views/editor/attachment.tsx
index a1c7c6e43..dc43dbd04 100644
--- a/packages/views/editor/attachment.tsx
+++ b/packages/views/editor/attachment.tsx
@@ -110,7 +110,7 @@ function normalize(
     return {
       filename: input.attachment.filename,
       contentType: input.attachment.content_type,
-      url: input.attachment.url,
+      url: pickInlineMediaURL(input.attachment, input.attachment.url),
       attachmentId: input.attachment.id,
       record: input.attachment,
       uploading: false,
@@ -120,7 +120,18 @@ function normalize(
   return {
     filename: input.filename || record?.filename || "",
     contentType: input.contentType || record?.content_type || "",
-    url: input.url,
+    // When the markdown URL resolved to an attachment record, swap to
+    // the record's freshly-loadable URL. The persisted markdown URL
+    // (`/api/attachments/<id>/download` for new content; raw stored URL
+    // for legacy) is correct as a stable reference but doesn't
+    // necessarily load as a native <img>/<video> resource for every
+    // client — token-mode clients can't attach an Authorization header
+    // to bare /api/* fetches, and a CloudFront-signed `download_url`
+    // is the only working media src in that mode. `pickInlineMediaURL`
+    // picks the URL with embedded credentials when one exists and
+    // falls back to the input URL otherwise so legacy / unresolved
+    // markdown stays on its existing path. See MUL-3130 review.
+    url: record ? pickInlineMediaURL(record, input.url) : input.url,
     attachmentId: record?.id,
     record,
     uploading: !!input.uploading,
@@ -129,6 +140,48 @@ function normalize(
   };
 }
 
+// pickInlineMediaURL returns the URL most likely to load successfully
+// inside a native <img>/<video>/<iframe> resource fetch — i.e. without
+// the calling client attaching an Authorization header.
+//
+// The metadata response from the backend offers two URL fields per
+// attachment row:
+//
+//   - `record.url`         — for LocalStorage this is a freshly-signed
+//                             `/uploads/<key>?exp=<unix>&sig=<HMAC>`
+//                             URL whose query string IS the auth (works
+//                             for token-mode <img> loads). For S3/
+//                             CloudFront it's the raw stored URL with
+//                             no signature.
+//   - `record.download_url` — `/api/attachments/<id>/download` in the
+//                             default proxy/presign mode (requires
+//                             cookie or Authorization header — does
+//                             NOT work as a native resource load for
+//                             token-mode clients). In CloudFront mode
+//                             this is replaced server-side with a
+//                             CloudFront-signed URL that DOES work as
+//                             a native <img> src.
+//
+// Heuristic: when `download_url` is an absolute URL with a recognised
+// CDN signature query (`Signature` / `Expires` / `Key-Pair-Id` for
+// CloudFront, `X-Amz-Signature` / `X-Amz-Expires` for raw S3 presigns
+// that may surface here in future modes), use it. Otherwise use
+// `record.url`, which carries the LocalStorage `?exp&sig` token and is
+// the only inline-loadable URL in that backend. Falls back to the
+// input URL when neither is usable so legacy markdown links keep their
+// pre-fix behaviour.
+function pickInlineMediaURL(record: AttachmentRecord, fallback: string): string {
+  const dl = record.download_url ?? "";
+  if (
+    /^https?:\/\//i.test(dl) &&
+    /[?&](Signature|X-Amz-Signature|Key-Pair-Id|Expires|X-Amz-Expires)=/i.test(dl)
+  ) {
+    return dl;
+  }
+  if (record.url) return record.url;
+  return fallback;
+}
+
 // ---------------------------------------------------------------------------
 // Dispatcher
 // ---------------------------------------------------------------------------
diff --git a/packages/views/editor/extensions/file-upload.test.ts b/packages/views/editor/extensions/file-upload.test.ts
index b27998ea8..9cd520ef9 100644
--- a/packages/views/editor/extensions/file-upload.test.ts
+++ b/packages/views/editor/extensions/file-upload.test.ts
@@ -58,6 +58,11 @@ function makeUpload(
     content_type: "image/png",
     size_bytes: 1,
     created_at: new Date(0).toISOString(),
+    // markdownLink defaults to the same value as `link` so legacy tests
+    // continue to assert the previous URL shape unless they pass an
+    // explicit override. Real callers always set it to the stable
+    // `/api/attachments/<id>/download` path via useFileUpload.
+    markdownLink: overrides.link,
     ...overrides,
   };
 }
@@ -186,4 +191,35 @@ describe("uploadAndInsertFile", () => {
       vi.unstubAllGlobals();
     }
   });
+
+  it("persists markdownLink (the stable per-attachment URL) into the markdown body, not the short-lived storage URL", async () => {
+    // Regression pin for MUL-3130 review feedback. useFileUpload returns
+    // both `link` (= att.url, short-lived signed `/uploads/<key>?exp&sig`
+    // on LocalStorage) and `markdownLink` (= /api/attachments/<id>/download).
+    // The editor must persist `markdownLink` so the comment doesn't
+    // capture a 30-min signature, while non-markdown callers (avatar
+    // pickers, logo upload) keep using `link` for backward compatibility.
+    const editor = makeEditor();
+    const SIGNED_URL = "/uploads/workspaces/ws-1/photo.png?exp=42&sig=fake";
+    const STABLE_URL = "/api/attachments/attachment-7/download";
+    const handler = vi.fn(async () =>
+      makeUpload({
+        id: "attachment-7",
+        link: SIGNED_URL,
+        markdownLink: STABLE_URL,
+        filename: "photo.png",
+      }),
+    );
+    const file = new File(["image"], "photo.png", { type: "image/png" });
+
+    await uploadAndInsertFile(editor, file, handler);
+
+    // The img node ends up with the stable URL as its src — the
+    // expiring signed URL never makes it into the persisted markdown.
+    const attrs = firstImageAttrs(editor);
+    expect(attrs?.src).toBe(STABLE_URL);
+    expect(editor.getMarkdown().trimEnd()).toBe(`![photo.png](${STABLE_URL})`);
+    expect(editor.getMarkdown()).not.toContain("?exp=");
+    expect(editor.getMarkdown()).not.toContain("?sig=");
+  });
 });
diff --git a/packages/views/editor/extensions/file-upload.ts b/packages/views/editor/extensions/file-upload.ts
index f6cc25174..6fd1df43d 100644
--- a/packages/views/editor/extensions/file-upload.ts
+++ b/packages/views/editor/extensions/file-upload.ts
@@ -164,7 +164,14 @@ export async function uploadAndInsertFile(
         if (imagePos !== null && imageNode) {
           const tr = editor.state.tr.setNodeMarkup(imagePos, undefined, {
             ...imageNode.attrs,
-            src: result.link,
+            // Persist the stable per-attachment URL into markdown so
+            // the comment doesn't capture a short-lived signed URL
+            // (MUL-3130). Falls back to `link` for the no-workspace
+            // avatar branch where there's no attachment-row id; that
+            // path is unreachable from comment/issue editors but the
+            // fallback keeps the contract consistent for any caller
+            // that drops in without an issue context.
+            src: result.markdownLink || result.link,
             alt: result.filename,
             uploading: false,
           });
@@ -192,7 +199,7 @@ export async function uploadAndInsertFile(
     try {
       const result = await handler(file);
       if (result) {
-        finalizeFileCard(editor, uploadId, result.link);
+        finalizeFileCard(editor, uploadId, result.markdownLink || result.link);
       } else {
         removeUploadingFileCard(editor, uploadId);
       }