fix(issues): persist draft attachment when the content draft is created, not only on pendingAttachments change (MUL-4475)

Real timing: an upload result sets pendingAttachments BEFORE the debounced
content flush creates the draft. The persist effect keyed only on
pendingAttachments therefore ran once (while no draft existed → skipped) and
never re-ran when the draft was later created, so the attachment was lost.

All three composers (comment-input / reply-input / comment-card edit) now add
`content` — the draft-creation signal — as a dependency and gate on it
(`content.trim()` → an honest, used dep) plus the draft-existence guard (still
never fabricates a content-less draft). When the debounced content flush lands,
the effect re-runs and persists the already-uploaded attachment.

New regression coverage per composer: upload result lands first (no draft yet),
then the content flush arrives → the attachment ends up in the draft.

Verify: views typecheck clean; full views suite 1865 pass; eslint clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Naiyuan Qing
2026-07-14 01:43:06 +08:00
parent 817296dc4e
commit 7ad0da13bb
4 changed files with 154 additions and 6 deletions

View File

@@ -394,11 +394,19 @@ export function useEditAttachmentState(
// remount (desktop tab switch / virtualization) can restore them via
// startEdit — only into an existing content draft, always writing the
// current value (incl. `[]`). See CommentInput for the rationale.
//
// `content` is a dependency so this re-runs when the (debounced) content
// draft is first created: an upload result sets `pendingAttachments` before
// that flush, so `pendingAttachments` alone would evaluate the guard while no
// draft exists yet and never persist the attachment. The draft-existence
// guard also keeps startEdit (content = the comment body, no draft yet) from
// fabricating a content-less draft.
useEffect(() => {
if (!editing) return;
if (content.trim().length === 0) return;
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
setDraft(draftKey, { attachments: pendingAttachments });
}, [editing, pendingAttachments, draftKey, setDraft]);
}, [editing, content, pendingAttachments, draftKey, setDraft]);
useEffect(() => {
if (!editing) return;

View File

@@ -10,12 +10,14 @@ import { fireEvent, render, screen, waitFor } from "@testing-library/react";
import type { Attachment, TimelineEntry } from "@multica/core/types";
import { useCommentDraftStore } from "@multica/core/issues/stores";
import { CommentInput } from "./comment-input";
import { ReplyInput } from "./reply-input";
import { useEditAttachmentState } from "./comment-card";
// One controllable trigger agent so a suppression chip renders. Hoisted +
// stable so the composer's "reconcile suppressed against visible agents" effect
// runs once and toggling doesn't churn it.
const mockAgents = vi.hoisted(() => [{ id: "ag-1", name: "Walt", source: "mention_agent" }]);
const uploadWithToast = vi.hoisted(() => vi.fn());
vi.mock("../hooks/use-comment-trigger-preview", () => ({
useCommentTriggerPreview: () => ({ agents: mockAgents }),
@@ -50,7 +52,24 @@ vi.mock("./comment-trigger-chips", () => ({
vi.mock("@multica/core/api", () => ({ api: {} }));
vi.mock("@multica/core/hooks/use-file-upload", () => ({
useFileUpload: () => ({ uploadWithToast: vi.fn() }),
useFileUpload: () => ({ uploadWithToast }),
}));
vi.mock("@multica/ui/components/common/file-upload-button", () => ({
FileUploadButton: ({ onSelect }: { onSelect: (f: File) => void }) => (
<button
type="button"
data-testid="upload-btn"
onClick={() => onSelect(new File(["x"], "a.png"))}
>
attach
</button>
),
}));
vi.mock("../../common/actor-avatar", () => ({
ActorAvatar: () => null,
AgentStatusDot: () => null,
}));
vi.mock("../../i18n", () => ({
@@ -68,9 +87,11 @@ vi.mock("../../editor", () => ({
{
defaultValue,
onUpdate,
onUploadFile,
}: {
defaultValue?: string;
onUpdate?: (markdown: string) => void;
onUploadFile?: (file: File) => Promise<unknown>;
},
ref: Ref<unknown>,
) {
@@ -82,7 +103,12 @@ vi.mock("../../editor", () => ({
},
focus: () => {},
blur: () => {},
uploadFile: async () => {},
// The upload result lands, but the content flush is DEBOUNCED — we
// deliberately do NOT call onUpdate here, so tests drive the
// "upload result first, content draft after" ordering explicitly.
uploadFile: async (file: File) => {
await onUploadFile?.(file);
},
hasActiveUploads: () => false,
}));
return (
@@ -121,6 +147,7 @@ function makeAttachment(id: string, url: string): Attachment {
beforeEach(() => {
localStorage.clear();
useCommentDraftStore.setState({ drafts: {} });
uploadWithToast.mockReset();
});
describe("comment composer suppression persistence", () => {
@@ -254,3 +281,107 @@ describe("edit composer attachment persistence", () => {
);
});
});
// The real timing: an upload result lands (pendingAttachments set) BEFORE the
// debounced content flush creates the draft. The persist effect must re-run
// when the draft is created — not only when pendingAttachments changes — or the
// attachment is never persisted.
describe("attachment persist survives the upload-before-content-draft ordering", () => {
it("comment-input: an upload landing before the debounced content draft is still persisted", async () => {
const att = makeAttachment("att-1", "http://x/att-1.png");
uploadWithToast.mockResolvedValue(att);
render(
<CommentInput issueId="issue-1" onSubmit={vi.fn().mockResolvedValue(true)} />,
);
// Upload result lands first — pendingAttachments set, no content draft yet.
await act(async () => {
fireEvent.click(screen.getByTestId("upload-btn"));
});
expect(useCommentDraftStore.getState().getDraft("new:issue-1")).toBeUndefined();
// Debounced content flush arrives → draft created → attachment persisted.
fireEvent.change(screen.getByTestId("editor"), {
target: { value: "body http://x/att-1.png" },
});
await waitFor(() =>
expect(
useCommentDraftStore.getState().getDraft("new:issue-1")?.attachments,
).toEqual([att]),
);
});
it("reply-input: same ordering, persisted into the reply draft", async () => {
const att = makeAttachment("att-2", "http://x/att-2.png");
uploadWithToast.mockResolvedValue(att);
const draftKey = "reply:issue-1:c-root" as const;
render(
<ReplyInput
issueId="issue-1"
parentId="c-root"
avatarType="member"
avatarId="u1"
onSubmit={vi.fn().mockResolvedValue(true)}
draftKey={draftKey}
/>,
);
await act(async () => {
fireEvent.click(screen.getByTestId("upload-btn"));
});
expect(useCommentDraftStore.getState().getDraft(draftKey)).toBeUndefined();
fireEvent.change(screen.getByTestId("editor"), {
target: { value: "reply http://x/att-2.png" },
});
await waitFor(() =>
expect(useCommentDraftStore.getState().getDraft(draftKey)?.attachments).toEqual([
att,
]),
);
});
it("comment-card edit: an upload during edit before the content flush is persisted", async () => {
const att = makeAttachment("att-3", "http://x/att-3.png");
uploadWithToast.mockResolvedValue(att);
const entry = {
id: "c-1",
content: "original",
attachments: [],
parent_id: null,
} as unknown as TimelineEntry;
let api: EditApi | null = null;
render(
<EditHarness
entry={entry}
onEdit={vi.fn().mockResolvedValue(undefined)}
capture={(a) => {
api = a;
}}
/>,
);
act(() => {
api!.startEdit();
});
// Upload result lands during edit — no content flush yet, so no edit draft.
await act(async () => {
await api!.handleUpload(new File(["x"], "a.png"));
});
expect(
useCommentDraftStore.getState().getDraft("edit:issue-1:c-1"),
).toBeUndefined();
// The debounced onUpdate: setContent + setDraft(content) → draft created.
act(() => {
api!.setContent("edited http://x/att-3.png");
api!.setDraft(api!.draftKey, { content: "edited http://x/att-3.png" });
});
await waitFor(() =>
expect(
useCommentDraftStore.getState().getDraft("edit:issue-1:c-1")?.attachments,
).toEqual([att]),
);
});
});

View File

@@ -97,10 +97,16 @@ function CommentInput({ issueId, onSubmit }: CommentInputProps) {
// write INTO an existing content draft — never create a content-less draft
// just to record these — and always write the current value (incl. `[]`) so
// clearing a suppression leaves no stale array to re-apply on remount.
//
// `content` is a dependency so this also re-runs when the (debounced) content
// draft is first created: an upload result sets `pendingAttachments` before
// that flush, so depending on `pendingAttachments` alone would evaluate the
// guard while no draft exists yet and never persist the attachment.
useEffect(() => {
if (content.trim().length === 0) return;
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
setDraft(draftKey, { attachments: pendingAttachments });
}, [pendingAttachments, draftKey, setDraft]);
}, [content, pendingAttachments, draftKey, setDraft]);
useEffect(() => {
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;

View File

@@ -115,12 +115,15 @@ function ReplyInput({
// Persist attachments + suppressed choices — only when a draftKey is present
// AND a content draft already exists (never create a content-less draft).
// Always write the current value (incl. `[]`) so clearing a suppression
// leaves no stale array to re-apply on remount.
// leaves no stale array to re-apply on remount. `content` is a dependency so
// this also re-runs when the (debounced) content draft is first created —
// an upload result can land before that flush (see CommentInput).
useEffect(() => {
if (!draftKey) return;
if (content.trim().length === 0) return;
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
setDraft(draftKey, { attachments: pendingAttachments });
}, [draftKey, pendingAttachments, setDraft]);
}, [draftKey, content, pendingAttachments, setDraft]);
useEffect(() => {
if (!draftKey) return;