From 5f0bd9fe74a0c0f40ce97f5e88fa4f5c24dd6d02 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:04:09 +0800 Subject: [PATCH] fix(chat): address review blockers on chat reply attachments (MUL-4287) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two final-review blockers on PR #5164: 1. Mobile inline dedup only checked raw `url`, so an attachment referenced inline via `markdown_url` (exactly what the CLI snippet emits) rendered twice — once inline, once as a standalone card. Reuse the core `contentReferencesAttachment` helper so dedup covers every real reference form (stable /api/attachments//download path, url, download_url, markdown_url), matching web's AttachmentList. Extracted the filter into a pure `lib/attachment-dedup.ts` so it is unit-testable, and added a regression test covering `content` containing `attachment.markdown_url` (plus the other URL forms and same-identity sibling dedup). 2. CLI `attachment upload` emitted `![...]` image markdown for every file, producing a broken-image snippet for non-images. Emit image markdown only for image/* content types and a plain link otherwise, with a CLI contract test for both. Approved scope otherwise unchanged. Co-Authored-By: Claude Opus 4.8 Co-authored-by: multica-agent --- .../issue/comment-attachment-list.tsx | 33 +++------ apps/mobile/lib/attachment-dedup.test.ts | 67 +++++++++++++++++++ apps/mobile/lib/attachment-dedup.ts | 42 ++++++++++++ server/cmd/multica/cmd_attachment.go | 9 ++- server/cmd/multica/cmd_attachment_test.go | 45 ++++++++++++- 5 files changed, 170 insertions(+), 26 deletions(-) create mode 100644 apps/mobile/lib/attachment-dedup.test.ts create mode 100644 apps/mobile/lib/attachment-dedup.ts diff --git a/apps/mobile/components/issue/comment-attachment-list.tsx b/apps/mobile/components/issue/comment-attachment-list.tsx index f48b4b301c..2870ca7bdd 100644 --- a/apps/mobile/components/issue/comment-attachment-list.tsx +++ b/apps/mobile/components/issue/comment-attachment-list.tsx @@ -25,6 +25,7 @@ import { useMemo } from "react"; import { Linking, Pressable, View } from "react-native"; import { Ionicons } from "@expo/vector-icons"; import type { Attachment } from "@multica/core/types"; +import { standaloneAttachments } from "@/lib/attachment-dedup"; import { MarkdownImage } from "@/lib/markdown/markdown-image"; import { resolveAttachmentUrl } from "@/lib/attachment-url"; import { useColorScheme } from "@/lib/use-color-scheme"; @@ -45,30 +46,14 @@ export function CommentAttachmentList({ attachments, content }: Props) { const { colorScheme } = useColorScheme(); const theme = THEME[colorScheme]; - const standalone = useMemo(() => { - if (!attachments || attachments.length === 0) return []; - if (!content) return attachments; - return attachments.filter((a) => { - // Skip attachments whose URL is already referenced inline in the - // markdown — they'll render via MarkdownImage (images) or a markdown - // link (files), and we'd otherwise show them twice. - if (content.includes(a.url)) return false; - // Dedup: if another attachment with the same file identity (name, - // type, size) is already inline in the content, this one is a - // duplicate upload — skip it. Mirrors web's - // `comment-card.tsx:132-140` defense. - const hasSiblingInContent = attachments.some( - (other) => - other.id !== a.id && - other.filename === a.filename && - other.content_type === a.content_type && - other.size_bytes === a.size_bytes && - content.includes(other.url), - ); - if (hasSiblingInContent) return false; - return true; - }); - }, [attachments, content]); + // Only render attachments not already referenced inline in the body. The + // dedup lives in a pure helper (lib/attachment-dedup) so it can be unit + // tested; it matches every real URL form the server emits (stable path / + // url / download_url / markdown_url), mirroring web's AttachmentList. + const standalone = useMemo( + () => standaloneAttachments(attachments, content), + [attachments, content], + ); if (standalone.length === 0) return null; diff --git a/apps/mobile/lib/attachment-dedup.test.ts b/apps/mobile/lib/attachment-dedup.test.ts new file mode 100644 index 0000000000..54eb2391a2 --- /dev/null +++ b/apps/mobile/lib/attachment-dedup.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import type { Attachment } from "@multica/core/types"; +import { standaloneAttachments } from "./attachment-dedup"; + +function att(over: 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: "agent", + uploader_id: "agent-1", + filename: "chart.png", + url: "https://cdn.example/chart.png", + download_url: "https://signed.example/chart.png?sig=x", + markdown_url: "https://public.example/api/attachments/att-1/download", + content_type: "image/png", + size_bytes: 123, + created_at: "2026-07-09T00:00:00Z", + ...over, + }; +} + +describe("standaloneAttachments", () => { + it("excludes an attachment referenced inline via markdown_url (the CLI snippet form)", () => { + const a = att(); + const content = `see the result\n\n![chart.png](${a.markdown_url})`; + expect(standaloneAttachments([a], content)).toEqual([]); + }); + + it("excludes an attachment referenced inline via the stable download path", () => { + const a = att(); + const content = `![chart.png](/api/attachments/${a.id}/download)`; + expect(standaloneAttachments([a], content)).toEqual([]); + }); + + it("excludes an attachment referenced inline via raw url", () => { + const a = att(); + expect(standaloneAttachments([a], `![x](${a.url})`)).toEqual([]); + }); + + it("excludes an attachment referenced inline via signed download_url", () => { + const a = att(); + expect(standaloneAttachments([a], `![x](${a.download_url})`)).toEqual([]); + }); + + it("keeps an attachment that is not referenced anywhere in the body", () => { + const a = att(); + expect(standaloneAttachments([a], "just some text, no image")).toEqual([a]); + }); + + it("renders every attachment when content is undefined (no body to reference them)", () => { + const a = att(); + expect(standaloneAttachments([a], undefined)).toEqual([a]); + }); + + it("drops a duplicate upload whose same-identity sibling is inline via markdown_url", () => { + const inline = att({ id: "att-inline" }); + const dup = att({ id: "att-dup", url: "https://cdn.example/other.png" }); + // Only the sibling's markdown_url appears in the body; the dup shares + // filename/type/size, so it must be treated as the same file and dropped. + const content = `![chart.png](${inline.markdown_url})`; + expect(standaloneAttachments([inline, dup], content)).toEqual([]); + }); +}); diff --git a/apps/mobile/lib/attachment-dedup.ts b/apps/mobile/lib/attachment-dedup.ts new file mode 100644 index 0000000000..81bfa700df --- /dev/null +++ b/apps/mobile/lib/attachment-dedup.ts @@ -0,0 +1,42 @@ +import type { Attachment } from "@multica/core/types"; +import { contentReferencesAttachment } from "@multica/core/types"; + +/** + * The attachments to render as standalone cards below a message / comment + * body: the ones NOT already referenced inline in `content`, minus duplicate + * uploads whose same-identity sibling is already inline. + * + * "Referenced inline" is decided by the core `contentReferencesAttachment` + * helper, so it matches every real URL form the server emits — the stable + * `/api/attachments//download` path, the raw storage `url`, the signed + * `download_url`, and the durable `markdown_url` that agents actually paste + * into a reply. Checking only raw `url` (the old behaviour) missed the + * `markdown_url` case and rendered the same image twice. + * + * Pass `content === undefined` (not "") to render every attachment — used when + * the body has no markdown that could reference them. + * + * Mirrors web's `AttachmentList` filter in + * `packages/views/issues/components/comment-card.tsx`. + */ +export function standaloneAttachments( + attachments: Attachment[] | undefined, + content: string | undefined, +): Attachment[] { + if (!attachments || attachments.length === 0) return []; + if (!content) return attachments; + return attachments.filter((a) => { + if (contentReferencesAttachment(content, a)) return false; + // Dedup: if another attachment with the same file identity (name, type, + // size) is already inline, this one is a duplicate upload — skip it. + const hasSiblingInContent = attachments.some( + (other) => + other.id !== a.id && + other.filename === a.filename && + other.content_type === a.content_type && + other.size_bytes === a.size_bytes && + contentReferencesAttachment(content, other), + ); + return !hasSiblingInContent; + }); +} diff --git a/server/cmd/multica/cmd_attachment.go b/server/cmd/multica/cmd_attachment.go index dbb64bcbd7..2df822abb7 100644 --- a/server/cmd/multica/cmd_attachment.go +++ b/server/cmd/multica/cmd_attachment.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/spf13/cobra" @@ -90,7 +91,13 @@ func runAttachmentUpload(cmd *cobra.Command, args []string) error { } filename := filepath.Base(path) - markdown := fmt.Sprintf("![%s](%s)", filename, att.MarkdownURL) + // Image content types get image markdown so they render inline in the + // reply; every other file gets a normal link so it shows as a proper file + // reference instead of a broken-image icon. + markdown := fmt.Sprintf("[%s](%s)", filename, att.MarkdownURL) + if strings.HasPrefix(att.ContentType, "image/") { + markdown = fmt.Sprintf("![%s](%s)", filename, att.MarkdownURL) + } fmt.Fprintln(os.Stderr, "Uploaded:", filename) return cli.PrintJSON(os.Stdout, map[string]any{ diff --git a/server/cmd/multica/cmd_attachment_test.go b/server/cmd/multica/cmd_attachment_test.go index 755a581643..f8d98b0617 100644 --- a/server/cmd/multica/cmd_attachment_test.go +++ b/server/cmd/multica/cmd_attachment_test.go @@ -99,6 +99,7 @@ func TestRunAttachmentUploadSendsTaskIDAndPrintsContract(t *testing.T) { _ = json.NewEncoder(w).Encode(map[string]any{ "id": "att-999", "filename": "chart.png", + "content_type": "image/png", "url": "https://cdn.example/chart.png", "download_url": "https://signed.example/chart.png?sig=x", "markdown_url": "https://public.example/api/attachments/att-999/download", @@ -136,8 +137,50 @@ func TestRunAttachmentUploadSendsTaskIDAndPrintsContract(t *testing.T) { if !strings.Contains(out, `"markdown_url": "https://public.example/api/attachments/att-999/download"`) { t.Fatalf("stdout missing markdown_url: %q", out) } + // Image content type → image markdown so it renders inline. if !strings.Contains(out, `![chart.png](https://public.example/api/attachments/att-999/download)`) { - t.Fatalf("stdout missing markdown snippet: %q", out) + t.Fatalf("stdout missing image markdown snippet: %q", out) + } +} + +// TestRunAttachmentUploadNonImageUsesLinkMarkdown covers the content-type +// branch: a non-image file must emit a plain link, not `![...]`, which would +// render as a broken image in the reply. +func TestRunAttachmentUploadNonImageUsesLinkMarkdown(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseMultipartForm(1 << 20); err != nil { + t.Fatalf("parse multipart: %v", err) + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": "att-doc", + "filename": "report.pdf", + "content_type": "application/pdf", + "url": "https://cdn.example/report.pdf", + "markdown_url": "https://public.example/api/attachments/att-doc/download", + }) + })) + defer srv.Close() + setCLITestServerEnv(t, srv.URL) + t.Setenv("MULTICA_TOKEN", "mat_test-token") + + dir := t.TempDir() + docPath := filepath.Join(dir, "report.pdf") + if err := os.WriteFile(docPath, []byte("%PDF-1.4 bytes"), 0o644); err != nil { + t.Fatalf("write temp doc: %v", err) + } + + cmd := newAttachmentUploadTestCmd() + _ = cmd.Flags().Set("task", "task-abc") + + out, err := captureStdout(t, func() error { return runAttachmentUpload(cmd, []string{docPath}) }) + if err != nil { + t.Fatalf("runAttachmentUpload: %v", err) + } + if !strings.Contains(out, `[report.pdf](https://public.example/api/attachments/att-doc/download)`) { + t.Fatalf("stdout missing link markdown: %q", out) + } + if strings.Contains(out, `![report.pdf]`) { + t.Fatalf("non-image must not use image markdown: %q", out) } }