diff --git a/apps/mobile/data/schemas.ts b/apps/mobile/data/schemas.ts index 152dc29b7..b20ee1255 100644 --- a/apps/mobile/data/schemas.ts +++ b/apps/mobile/data/schemas.ts @@ -58,6 +58,7 @@ export const AttachmentSchema: z.ZodType = z.object({ filename: z.string(), url: z.string(), download_url: z.string().default(""), + markdown_url: z.string().default(""), content_type: z.string().default(""), size_bytes: z.number().default(0), created_at: z.string().default(""), diff --git a/packages/core/api/schemas.ts b/packages/core/api/schemas.ts index ea3c7dda9..33953a917 100644 --- a/packages/core/api/schemas.ts +++ b/packages/core/api/schemas.ts @@ -84,10 +84,18 @@ const AttachmentSchema = z.object({ // in a new tab — `download_url` and `url` — must be strings, otherwise we'd // happily `window.open(undefined)`. `filename` gates the toast/title and is // also enforced so a missing value falls back to the empty record below. +// +// `markdown_url` is parsed lenient: a server old enough to predate +// MUL-3192 omits the field, in which case the schema defaults it to "". +// Callers that need to persist a URL into markdown should go through the +// `useFileUpload` helper (which falls back to the legacy +// `attachmentDownloadPath` shape when `markdown_url` is empty), so the +// empty-string default does not silently break any persistence path. export const AttachmentResponseSchema = z.object({ id: z.string(), url: z.string(), download_url: z.string(), + markdown_url: z.string().optional().default(""), filename: z.string(), chat_session_id: z.string().nullable().optional(), chat_message_id: z.string().nullable().optional(), @@ -105,6 +113,7 @@ export const EMPTY_ATTACHMENT: Attachment = { filename: "", url: "", download_url: "", + markdown_url: "", content_type: "", size_bytes: 0, created_at: "", diff --git a/packages/core/hooks/use-file-upload.test.ts b/packages/core/hooks/use-file-upload.test.ts new file mode 100644 index 000000000..4f9432caf --- /dev/null +++ b/packages/core/hooks/use-file-upload.test.ts @@ -0,0 +1,102 @@ +/** + * @vitest-environment jsdom + */ +import { describe, expect, it, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import type { ApiClient } from "../api/client"; +import type { Attachment } from "../types"; +import { useFileUpload, type UploadResult } from "./use-file-upload"; + +// MUL-3192 — verifies that the URL chosen for markdown persistence is +// a durable, server-supplied absolute URL when available, and falls +// through to the legacy site-relative shape only when the server didn't +// populate `markdown_url` (older deployments) or when there's no +// attachment-row id at all (no-workspace avatar branch). + +function makeAttachment(overrides: Partial = {}): Attachment { + return { + id: "att-1", + workspace_id: "ws-1", + issue_id: null, + comment_id: null, + chat_session_id: null, + chat_message_id: null, + uploader_type: "member", + uploader_id: "u-1", + filename: "shot.png", + url: "/uploads/ws-1/shot.png", + download_url: "/api/attachments/att-1/download", + markdown_url: "https://api.multica.test/api/attachments/att-1/download", + content_type: "image/png", + size_bytes: 1, + created_at: "2026-06-10T00:00:00Z", + ...overrides, + }; +} + +function makeApi(att: Attachment): ApiClient { + return { + uploadFile: vi.fn().mockResolvedValue(att), + } as unknown as ApiClient; +} + +async function runUpload(api: ApiClient): Promise { + const { result } = renderHook(() => useFileUpload(api)); + let upload: UploadResult | null = null; + await act(async () => { + upload = await result.current.upload( + new File(["data"], "shot.png", { type: "image/png" }), + ); + }); + return upload; +} + +describe("useFileUpload — markdownLink picks the durable URL with three-layer fallback", () => { + it("prefers att.markdown_url when the server populates it (modern deployment)", async () => { + const att = makeAttachment({ + markdown_url: "https://cdn.multica.test/uploads/abc.png", + }); + const upload = await runUpload(makeApi(att)); + expect(upload?.markdownLink).toBe("https://cdn.multica.test/uploads/abc.png"); + // `link` keeps its legacy semantics — same as att.url, used by avatar / + // logo callers that persist into long-lived fields. + expect(upload?.link).toBe(att.url); + }); + + it("falls back to the site-relative download path when the server omitted markdown_url (legacy server)", async () => { + const att = makeAttachment({ markdown_url: "" }); + const upload = await runUpload(makeApi(att)); + // On web this is fine — Next.js rewrite proxies /api/* to the API + // host server-side. Non-web surfaces hit attachment.tsx's legacy + // absolutize fallback at render time. + expect(upload?.markdownLink).toBe("/api/attachments/att-1/download"); + }); + + it("falls back to att.url when there's no attachment-row id (no-workspace avatar branch)", async () => { + const att = makeAttachment({ + id: "", + markdown_url: "", + url: "https://cdn.multica.test/avatars/u-1.png", + }); + const upload = await runUpload(makeApi(att)); + expect(upload?.markdownLink).toBe("https://cdn.multica.test/avatars/u-1.png"); + expect(upload?.link).toBe("https://cdn.multica.test/avatars/u-1.png"); + }); + + it("rejects oversize files before hitting the network", async () => { + const att = makeAttachment(); + const api = makeApi(att); + const huge = new File([new ArrayBuffer(1)], "big.bin", { + type: "application/octet-stream", + }); + Object.defineProperty(huge, "size", { value: 200 * 1024 * 1024 }); + + const { result } = renderHook(() => useFileUpload(api)); + await expect( + act(async () => { + await result.current.upload(huge); + }), + ).rejects.toThrow(/100 MB/); + expect(api.uploadFile as ReturnType).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/hooks/use-file-upload.ts b/packages/core/hooks/use-file-upload.ts index baaa31291..1534426e5 100644 --- a/packages/core/hooks/use-file-upload.ts +++ b/packages/core/hooks/use-file-upload.ts @@ -22,22 +22,33 @@ import { MAX_FILE_SIZE } from "../constants/upload"; // so avatar uploads do not get rerouted through // the workspace-membership-gated download endpoint. // -// `markdownLink` — the stable per-attachment URL -// `/api/attachments//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`. +// `markdownLink` — the URL the editor writes into markdown bodies. +// Source: `att.markdown_url` from the server, which +// `buildMarkdownURL` picks per deployment policy +// (public CDN durable URL, or +// `/api/attachments//download`). +// The contract is "absolute, no TTL, loads natively +// on every client" — that's what fixes the Desktop / +// mobile-webview regression where a site-relative +// `/api/attachments//download` couldn't resolve +// against `file://` (MUL-3192). +// +// Falls back through two layers when the server +// didn't populate `markdown_url`: +// 1. `attachmentDownloadPath(att.id)` — the legacy +// site-relative shape. Works on web (Next +// rewrite proxies /api/* to the API host) and +// is what older comments persist; render +// surfaces handle the absolutize for non-web +// clients via attachment.tsx's legacy compat. +// 2. `att.url` — the no-workspace avatar branch +// where there's no attachment-row id at all. // // 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. +// these two semantics into a single `link` field; MUL-3192 followed up +// by moving the durable-URL choice from the client to the server so +// the persisted shape is correct for the deployment without the client +// having to know whether it's running on web / desktop / mobile. export type UploadResult = Attachment & { link: string; markdownLink: string; @@ -49,6 +60,26 @@ export interface UploadContext { chatSessionId?: string; } +// pickMarkdownLink chooses the URL the editor will write into markdown. +// +// Order: +// 1. `att.markdown_url` — server-provided durable URL. This is the +// modern contract introduced in MUL-3192; the server (`buildMarkdownURL`) +// decides whether to emit a public CDN URL or an absolute API +// endpoint pinned to `MULTICA_PUBLIC_URL` based on the deployment. +// 2. `attachmentDownloadPath(att.id)` — site-relative legacy shape, +// retained for compatibility with backends old enough to predate +// MUL-3192. Web's Next rewrite makes this load; desktop / mobile +// surfaces hit the attachment.tsx legacy-absolutize fallback. +// 3. `att.url` — no attachment-row id (the no-workspace avatar branch +// of UploadFile). Markdown callers fall back to whatever storage +// backend produced for the upload; persistence is on the caller. +function pickMarkdownLink(att: Attachment): string { + if (att.markdown_url) return att.markdown_url; + if (att.id) return attachmentDownloadPath(att.id); + return att.url; +} + export function useFileUpload( api: ApiClient, onError?: (error: Error) => void, @@ -68,14 +99,7 @@ export function useFileUpload( commentId: ctx?.commentId, chatSessionId: ctx?.chatSessionId, }); - // 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 }; + return { ...att, link: att.url, markdownLink: pickMarkdownLink(att) }; } finally { setUploading(false); } diff --git a/packages/core/issues/ws-updaters.test.ts b/packages/core/issues/ws-updaters.test.ts index 817f2567e..9f44a1726 100644 --- a/packages/core/issues/ws-updaters.test.ts +++ b/packages/core/issues/ws-updaters.test.ts @@ -320,6 +320,7 @@ describe("onIssueDeleted", () => { filename: "evidence.png", url: "s3://bucket/evidence.png", download_url: "https://example.test/evidence.png", + markdown_url: "https://example.test/api/attachments/att-1/download", content_type: "image/png", size_bytes: 1, created_at: "2025-01-01T00:00:00Z", diff --git a/packages/core/types/attachment.ts b/packages/core/types/attachment.ts index feea5f356..139bf7744 100644 --- a/packages/core/types/attachment.ts +++ b/packages/core/types/attachment.ts @@ -10,6 +10,31 @@ export interface Attachment { filename: string; url: string; download_url: string; + /** + * Durable URL the client persists into markdown bodies. + * + * The server (`buildMarkdownURL` in server/internal/handler/file.go) + * computes this per deployment policy: + * - public CDN path when storage URL is itself absolute and unsigned; + * - otherwise `/api/attachments//download`, + * which the server self-resigns / proxies on every request. + * + * Distinct from `url` (raw storage URL — may be private / site-relative) + * and `download_url` (this-response click-time URL — may be a short-lived + * CloudFront / S3 signed URL with a TTL). `markdown_url` is contracted + * to be safe to embed in markdown bodies that outlive the current + * session and to load as a native browser resource fetch on every + * supported client (web / desktop / mobile webview). MUL-3192. + * + * Empty when the response was produced by a server old enough to + * predate this field, or by an upload path that did not produce a + * persisted attachment row (e.g. the no-workspace avatar branch). + * Frontend callers that need to embed a URL into markdown should use + * the helper in `useFileUpload` rather than reading this field + * directly so the legacy fallbacks (download path / `att.url`) stay + * centralized. + */ + markdown_url: string; content_type: string; size_bytes: number; created_at: string; diff --git a/packages/views/chat/components/chat-input.test.tsx b/packages/views/chat/components/chat-input.test.tsx index b6fdffa30..8a0ed33c1 100644 --- a/packages/views/chat/components/chat-input.test.tsx +++ b/packages/views/chat/components/chat-input.test.tsx @@ -17,6 +17,7 @@ function makeUpload(overrides: Partial & { id: string; link: strin uploader_id: "user-1", url: overrides.link, download_url: overrides.link, + markdown_url: overrides.link, content_type: "image/png", size_bytes: 1, created_at: new Date(0).toISOString(), diff --git a/packages/views/editor/attachment-preview-modal.test.tsx b/packages/views/editor/attachment-preview-modal.test.tsx index 4a87ec692..9e628a273 100644 --- a/packages/views/editor/attachment-preview-modal.test.tsx +++ b/packages/views/editor/attachment-preview-modal.test.tsx @@ -143,6 +143,7 @@ function makeAttachment(overrides: Partial = {}): Attachment { filename: "test.bin", url: "https://cdn.example.test/att-1.bin", download_url: "https://cdn.example.test/att-1.bin?Signature=s", + markdown_url: "https://cdn.example.test/api/attachments/att-1/download", content_type: "application/octet-stream", size_bytes: 0, created_at: "2026-05-13T00:00:00Z", diff --git a/packages/views/editor/attachment.test.tsx b/packages/views/editor/attachment.test.tsx index 6215b6b26..dbe0d4b57 100644 --- a/packages/views/editor/attachment.test.tsx +++ b/packages/views/editor/attachment.test.tsx @@ -6,18 +6,27 @@ import type { Attachment as AttachmentRecord } from "@multica/core/types"; const { getAttachmentTextContentMock, + getBaseUrlMock, downloadMock, openExternalMock, openByUrlMock, } = vi.hoisted(() => ({ getAttachmentTextContentMock: vi.fn(), + // Default: empty base URL so existing tests render site-relative URLs + // through the proxy (i.e. exactly the way the web app behaves). The + // absolutize-specific suite below overrides this to simulate Desktop / + // mobile webview, where the renderer's origin does NOT proxy /api. + getBaseUrlMock: vi.fn(() => ""), downloadMock: vi.fn(), openExternalMock: vi.fn(), openByUrlMock: vi.fn(), })); vi.mock("@multica/core/api", () => ({ - api: { getAttachmentTextContent: getAttachmentTextContentMock }, + api: { + getAttachmentTextContent: getAttachmentTextContentMock, + getBaseUrl: getBaseUrlMock, + }, PreviewTooLargeError: class extends Error {}, PreviewUnsupportedError: class extends Error {}, })); @@ -107,6 +116,7 @@ function makeRecord(overrides: Partial = {}): AttachmentRecord filename: "shot.png", url: "https://cdn.example.test/att-1.png", download_url: "https://cdn.example.test/att-1.png?Signature=s", + markdown_url: "https://cdn.example.test/api/attachments/att-1/download", content_type: "image/png", size_bytes: 1024, created_at: "2026-05-13T00:00:00Z", @@ -124,6 +134,10 @@ function renderWithQuery(ui: ReactElement) { beforeEach(() => { vi.clearAllMocks(); resolverState.attachments = []; + // Default to "no proxy override" — site-relative URLs stay as-is, mirroring + // the web app's same-origin proxy. Tests that simulate Desktop / mobile + // webview override per-case via getBaseUrlMock.mockReturnValue(...). + getBaseUrlMock.mockReturnValue(""); }); afterEach(() => { @@ -240,28 +254,41 @@ describe("Attachment — image dispatch", () => { expect(screen.queryByTitle("Download")).toBeNull(); }); - it("stable /api/attachments//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//download - // path, which does NOT load as a native /