mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-28 05:46:58 +02:00
fix(issues): close out Howard review on blocker 3 draft externalization (MUL-4475)
- edit composer (comment-card `useEditAttachmentState`): externalize pendingAttachments + suppressedAgentIds into the edit draft and re-hydrate them in startEdit, so uploading an attachment mid-edit → tab-switch remount → save keeps the attachment id (was dropped). Hook exported for testing. - top-level + reply composers: persist attachments/suppressed only INTO an existing content draft (never fabricate a content-only empty draft), and always write the current value — incl. `[]` — so un-suppressing an agent no longer leaves a stale array that re-suppresses it on remount. - issue-surface: import `getIssueSurfaceViewStore` from the public `@multica/core/issues/stores` barrel instead of the deep private path. New regression coverage: edit attachment survives remount → save keeps id; suppress → un-suppress persists `[]` and a remount stays un-suppressed; toggling suppression with no content never creates an empty draft. Verify: views typecheck clean; full views suite 1862 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:
@@ -309,7 +309,7 @@ function TaskCommentRetryButton({
|
||||
// Shared edit-attachment state hook
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
function useEditAttachmentState(
|
||||
export function useEditAttachmentState(
|
||||
issueId: string,
|
||||
entry: TimelineEntry,
|
||||
onEdit: (commentId: string, content: string, attachmentIds: string[], suppressAgentIds?: string[]) => Promise<void>,
|
||||
@@ -390,9 +390,28 @@ function useEditAttachmentState(
|
||||
clearDraft(draftKey);
|
||||
};
|
||||
|
||||
// Persist edit attachments + suppressed choices into the edit draft so a
|
||||
// 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.
|
||||
useEffect(() => {
|
||||
if (!editing) return;
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { attachments: pendingAttachments });
|
||||
}, [editing, pendingAttachments, draftKey, setDraft]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!editing) return;
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { suppressedAgentIds: [...suppressedAgentIds] });
|
||||
}, [editing, suppressedAgentIds, draftKey, setDraft]);
|
||||
|
||||
const startEdit = () => {
|
||||
cancelledRef.current = false;
|
||||
setContent(getDraft(draftKey)?.content ?? entry.content ?? "");
|
||||
const draft = getDraft(draftKey);
|
||||
setContent(draft?.content ?? entry.content ?? "");
|
||||
setPendingAttachments(draft?.attachments ?? []);
|
||||
setSuppressedAgentIds(new Set(draft?.suppressedAgentIds ?? []));
|
||||
setRetainedStandaloneIds(initialStandaloneAttachmentIds(entry));
|
||||
setEditing(true);
|
||||
};
|
||||
|
||||
@@ -0,0 +1,256 @@
|
||||
import {
|
||||
forwardRef,
|
||||
useImperativeHandle,
|
||||
useRef,
|
||||
act,
|
||||
type Ref,
|
||||
} from "react";
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
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 { 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" }]);
|
||||
|
||||
vi.mock("../hooks/use-comment-trigger-preview", () => ({
|
||||
useCommentTriggerPreview: () => ({ agents: mockAgents }),
|
||||
}));
|
||||
|
||||
// Test double for the chip strip: one toggle button per agent.
|
||||
vi.mock("./comment-trigger-chips", () => ({
|
||||
CommentTriggerChips: ({
|
||||
agents,
|
||||
suppressedAgentIds,
|
||||
onToggle,
|
||||
}: {
|
||||
agents: { id: string; name: string }[];
|
||||
suppressedAgentIds: Set<string>;
|
||||
onToggle: (id: string) => void;
|
||||
}) => (
|
||||
<div>
|
||||
{agents.map((a) => (
|
||||
<button
|
||||
key={a.id}
|
||||
data-testid={`chip-${a.id}`}
|
||||
aria-pressed={suppressedAgentIds.has(a.id)}
|
||||
onClick={() => onToggle(a.id)}
|
||||
>
|
||||
{a.name}
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
),
|
||||
}));
|
||||
|
||||
vi.mock("@multica/core/api", () => ({ api: {} }));
|
||||
|
||||
vi.mock("@multica/core/hooks/use-file-upload", () => ({
|
||||
useFileUpload: () => ({ uploadWithToast: vi.fn() }),
|
||||
}));
|
||||
|
||||
vi.mock("../../i18n", () => ({
|
||||
useT: () => ({ t: () => "translated" }),
|
||||
useTimeAgo: () => () => "now",
|
||||
}));
|
||||
|
||||
vi.mock("../../editor", () => ({
|
||||
useFileDropZone: () => ({
|
||||
isDragOver: false,
|
||||
dropZoneProps: { "data-testid": "drop-zone" },
|
||||
}),
|
||||
FileDropOverlay: () => null,
|
||||
ContentEditor: forwardRef(function MockContentEditor(
|
||||
{
|
||||
defaultValue,
|
||||
onUpdate,
|
||||
}: {
|
||||
defaultValue?: string;
|
||||
onUpdate?: (markdown: string) => void;
|
||||
},
|
||||
ref: Ref<unknown>,
|
||||
) {
|
||||
const valueRef = useRef(defaultValue ?? "");
|
||||
useImperativeHandle(ref, () => ({
|
||||
getMarkdown: () => valueRef.current,
|
||||
clearContent: () => {
|
||||
valueRef.current = "";
|
||||
},
|
||||
focus: () => {},
|
||||
blur: () => {},
|
||||
uploadFile: async () => {},
|
||||
hasActiveUploads: () => false,
|
||||
}));
|
||||
return (
|
||||
<textarea
|
||||
data-testid="editor"
|
||||
defaultValue={defaultValue}
|
||||
onChange={(e) => {
|
||||
valueRef.current = e.target.value;
|
||||
onUpdate?.(e.target.value);
|
||||
}}
|
||||
/>
|
||||
);
|
||||
}),
|
||||
}));
|
||||
|
||||
function makeAttachment(id: string, url: string): Attachment {
|
||||
return {
|
||||
id,
|
||||
workspace_id: "ws-1",
|
||||
issue_id: "issue-1",
|
||||
comment_id: null,
|
||||
chat_session_id: null,
|
||||
chat_message_id: null,
|
||||
uploader_type: "member",
|
||||
uploader_id: "u1",
|
||||
filename: `${id}.png`,
|
||||
url,
|
||||
download_url: url,
|
||||
markdown_url: url,
|
||||
content_type: "image/png",
|
||||
size_bytes: 1,
|
||||
created_at: "2026-01-01T00:00:00Z",
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
localStorage.clear();
|
||||
useCommentDraftStore.setState({ drafts: {} });
|
||||
});
|
||||
|
||||
describe("comment composer suppression persistence", () => {
|
||||
it("writes suppressedAgentIds:[] on un-suppress (no stale array, no empty draft) and a remount stays un-suppressed", async () => {
|
||||
const view = render(<CommentInput issueId="issue-1" onSubmit={vi.fn().mockResolvedValue(true)} />);
|
||||
|
||||
// A content draft must exist for suppression to persist.
|
||||
fireEvent.change(screen.getByTestId("editor"), { target: { value: "ping @walt" } });
|
||||
|
||||
fireEvent.click(screen.getByTestId("chip-ag-1")); // suppress
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
useCommentDraftStore.getState().getDraft("new:issue-1")?.suppressedAgentIds,
|
||||
).toEqual(["ag-1"]),
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByTestId("chip-ag-1")); // un-suppress
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
useCommentDraftStore.getState().getDraft("new:issue-1")?.suppressedAgentIds,
|
||||
).toEqual([]),
|
||||
);
|
||||
|
||||
// The content draft is preserved (not wiped, not turned into an empty draft).
|
||||
expect(useCommentDraftStore.getState().getDraft("new:issue-1")?.content).toBe(
|
||||
"ping @walt",
|
||||
);
|
||||
|
||||
// Remount: the composer hydrates from the draft — the agent must NOT come
|
||||
// back suppressed off a stale array.
|
||||
view.unmount();
|
||||
render(<CommentInput issueId="issue-1" onSubmit={vi.fn().mockResolvedValue(true)} />);
|
||||
expect(screen.getByTestId("chip-ag-1")).toHaveAttribute("aria-pressed", "false");
|
||||
});
|
||||
|
||||
it("never creates a content-only empty draft just from toggling suppression", () => {
|
||||
render(<CommentInput issueId="issue-1" onSubmit={vi.fn().mockResolvedValue(true)} />);
|
||||
// No content typed → toggling suppression must not persist anything.
|
||||
fireEvent.click(screen.getByTestId("chip-ag-1"));
|
||||
fireEvent.click(screen.getByTestId("chip-ag-1"));
|
||||
expect(useCommentDraftStore.getState().getDraft("new:issue-1")).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Edit composer: attachments survive a remount (blocker 3, edit path)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
type EditApi = ReturnType<typeof useEditAttachmentState>;
|
||||
|
||||
const MockEditEditor = forwardRef(function MockEditEditor(
|
||||
{ defaultValue }: { defaultValue?: string },
|
||||
ref: Ref<unknown>,
|
||||
) {
|
||||
const valueRef = useRef(defaultValue ?? "");
|
||||
useImperativeHandle(ref, () => ({
|
||||
getMarkdown: () => valueRef.current,
|
||||
clearContent: () => {
|
||||
valueRef.current = "";
|
||||
},
|
||||
focus: () => {},
|
||||
blur: () => {},
|
||||
uploadFile: async () => {},
|
||||
hasActiveUploads: () => false,
|
||||
}));
|
||||
return <textarea data-testid="edit-editor" defaultValue={defaultValue} />;
|
||||
});
|
||||
|
||||
function EditHarness({
|
||||
entry,
|
||||
onEdit,
|
||||
capture,
|
||||
}: {
|
||||
entry: TimelineEntry;
|
||||
onEdit: (
|
||||
commentId: string,
|
||||
content: string,
|
||||
attachmentIds: string[],
|
||||
suppressAgentIds?: string[],
|
||||
) => Promise<void>;
|
||||
capture: (api: EditApi) => void;
|
||||
}) {
|
||||
const edit = useEditAttachmentState("issue-1", entry, onEdit);
|
||||
capture(edit);
|
||||
return edit.editing ? (
|
||||
<MockEditEditor ref={edit.editorRef} defaultValue={edit.initialValue} />
|
||||
) : null;
|
||||
}
|
||||
|
||||
describe("edit composer attachment persistence", () => {
|
||||
it("re-hydrates an uploaded attachment on startEdit so save keeps its attachment id", async () => {
|
||||
const att = makeAttachment("att-1", "http://x/att-1.png");
|
||||
// A prior edit session uploaded att-1 (its URL is in the body) then the tab
|
||||
// switched — the edit draft persisted content + attachments.
|
||||
useCommentDraftStore.getState().setDraft("edit:issue-1:c-1", {
|
||||
content: "edited body http://x/att-1.png",
|
||||
attachments: [att],
|
||||
});
|
||||
|
||||
const entry = {
|
||||
id: "c-1",
|
||||
content: "original body",
|
||||
attachments: [],
|
||||
parent_id: null,
|
||||
} as unknown as TimelineEntry;
|
||||
const onEdit = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
let api: EditApi | null = null;
|
||||
render(
|
||||
<EditHarness
|
||||
entry={entry}
|
||||
onEdit={onEdit}
|
||||
capture={(a) => {
|
||||
api = a;
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
act(() => {
|
||||
api!.startEdit();
|
||||
});
|
||||
await act(async () => {
|
||||
await api!.saveEdit();
|
||||
});
|
||||
|
||||
expect(onEdit).toHaveBeenCalledWith(
|
||||
"c-1",
|
||||
"edited body http://x/att-1.png",
|
||||
["att-1"],
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -93,17 +93,18 @@ function CommentInput({ issueId, onSubmit }: CommentInputProps) {
|
||||
}, [draftKey]);
|
||||
|
||||
// Persist attachments + suppressed choices so the composer's full state
|
||||
// survives an unmount (desktop tab switch / virtualization scroll-out).
|
||||
// survives an unmount (desktop tab switch / virtualization scroll-out). Only
|
||||
// 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.
|
||||
useEffect(() => {
|
||||
if (pendingAttachments.length > 0) {
|
||||
setDraft(draftKey, { attachments: pendingAttachments });
|
||||
}
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { attachments: pendingAttachments });
|
||||
}, [pendingAttachments, draftKey, setDraft]);
|
||||
|
||||
useEffect(() => {
|
||||
if (suppressedAgentIds.size > 0) {
|
||||
setDraft(draftKey, { suppressedAgentIds: [...suppressedAgentIds] });
|
||||
}
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { suppressedAgentIds: [...suppressedAgentIds] });
|
||||
}, [suppressedAgentIds, draftKey, setDraft]);
|
||||
|
||||
useEffect(() => {
|
||||
|
||||
@@ -112,17 +112,20 @@ function ReplyInput({
|
||||
setSuppressedAgentIds(new Set(draft?.suppressedAgentIds ?? []));
|
||||
}, [draftKey, issueId, parentId]);
|
||||
|
||||
// Persist attachments + suppressed choices (only when a draftKey is present).
|
||||
// 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.
|
||||
useEffect(() => {
|
||||
if (draftKey && pendingAttachments.length > 0) {
|
||||
setDraft(draftKey, { attachments: pendingAttachments });
|
||||
}
|
||||
if (!draftKey) return;
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { attachments: pendingAttachments });
|
||||
}, [draftKey, pendingAttachments, setDraft]);
|
||||
|
||||
useEffect(() => {
|
||||
if (draftKey && suppressedAgentIds.size > 0) {
|
||||
setDraft(draftKey, { suppressedAgentIds: [...suppressedAgentIds] });
|
||||
}
|
||||
if (!draftKey) return;
|
||||
if (useCommentDraftStore.getState().getDraft(draftKey) === undefined) return;
|
||||
setDraft(draftKey, { suppressedAgentIds: [...suppressedAgentIds] });
|
||||
}, [draftKey, suppressedAgentIds, setDraft]);
|
||||
|
||||
useEffect(() => {
|
||||
|
||||
@@ -6,9 +6,11 @@ import { Button } from "@multica/ui/components/ui/button";
|
||||
import { Skeleton } from "@multica/ui/components/ui/skeleton";
|
||||
import { cn } from "@multica/ui/lib/utils";
|
||||
import { useWorkspaceId } from "@multica/core/hooks";
|
||||
import { ViewStoreProvider } from "@multica/core/issues/stores/view-store-context";
|
||||
import { useIssueViewStoreFactory } from "@multica/core/issues/stores";
|
||||
import { getIssueSurfaceViewStore } from "@multica/core/issues/stores/surface-view-store";
|
||||
import {
|
||||
ViewStoreProvider,
|
||||
useIssueViewStoreFactory,
|
||||
getIssueSurfaceViewStore,
|
||||
} from "@multica/core/issues/stores";
|
||||
import { issueScopeKey } from "@multica/core/issues/surface/scope";
|
||||
import type { Issue } from "@multica/core/types";
|
||||
import { BoardView } from "../components/board-view";
|
||||
|
||||
Reference in New Issue
Block a user