From 94e375ee20772f254c4ef8b1fa132c0cd91bd539 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Thu, 16 Jul 2026 15:27:48 +0800 Subject: [PATCH] fix(chat): keep an in-flight upload bound to the draft it started in (MUL-4864) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review blocker. An upload PINS the editor to its source document: Guard 0 refuses to setContent over an `uploading` node, because wiping it strands the upload's finalize and the file silently vanishes. So while an upload runs, the instance still holds A's document even though the user has navigated to B — and the upload's own completion dispatch fires onUpdate on that instance, which resolves to the latest render's closure. Result: A's body and its attachment URL were written into B's draft, B's own draft was destroyed, and A lost the upload it was waiting for. Nothing stopped this: useUploadGate gates submit, not session/agent navigation. Reproduced as a test before fixing — B's "B's own words" was replaced by A's body + A's CDN URL. Root cause is the same shape as the debounce bug, one level up: the editor's writes were keyed off what is SELECTED, when they belong to what is LOADED. Those are the same key except while an upload pins the document. - chat-input models that explicitly with `editorDraftKeyRef` — the draft whose document the instance holds. Every editor-driven write (onUpdate, the upload's attachment binding) uses it, not `draftKey`. - The draft switch defers while `hasActiveUploads()`, leaving both the document and its writes on the source key, then completes when the gate clears. Blocking navigation instead would make the user wait on a network round-trip to change tabs. - The deferred case must force the adopt: ContentEditor's sync effect CONSUMED the defaultValue change while Guard 0 was up (lastDefaultValueRef advances before the guard), so it never re-runs and B's draft would never load. New `adoptContent()` ref method lands it; its body is the sync effect's own apply path, extracted so both agree. - handleSend refuses while loaded !== selected. The upload gate covers most of that window but not the sliver between the upload's final dispatch and the adopt re-render — React flushes that on a scheduler task, so Mod+Enter can land first. Tests (real ContentEditor, real Guard 0, real debounce, real upload lifecycle incl. the transaction that publishes the queue flip): completing upload's markdown stays in the source draft and never reaches the target; an attachment dropped while pinned binds to the source; the target draft loads once the guard clears; send is refused mid-divergence. Each verified to fail without its fix — the send guard initially passed vacuously (the disabled SubmitButton swallowed the click) and now drives the real Mod+Enter path. Co-Authored-By: Claude Opus 4.8 Co-authored-by: multica-agent --- .../chat-input-draft-isolation.test.tsx | 207 +++++++++++++++++- .../views/chat/components/chat-input.test.tsx | 5 + packages/views/chat/components/chat-input.tsx | 135 ++++++++---- packages/views/editor/content-editor.tsx | 118 ++++++---- 4 files changed, 379 insertions(+), 86 deletions(-) diff --git a/packages/views/chat/components/chat-input-draft-isolation.test.tsx b/packages/views/chat/components/chat-input-draft-isolation.test.tsx index aa0d223ac0..93f47eabc4 100644 --- a/packages/views/chat/components/chat-input-draft-isolation.test.tsx +++ b/packages/views/chat/components/chat-input-draft-isolation.test.tsx @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { act, render } from "@testing-library/react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; import { I18nProvider } from "@multica/core/i18n/react"; +import type { UploadResult } from "@multica/core/hooks/use-file-upload"; import enCommon from "../../locales/en/common.json"; import enChat from "../../locales/en/chat.json"; @@ -39,11 +40,32 @@ const mockSetContent = vi.hoisted(() => vi.fn()); vi.mock("@tanstack/react-query", () => ({ useQueryClient: () => ({}), })); +// Captures what ContentEditor wires into its extensions. `onSubmitRef` is the +// Mod+Enter path — it bypasses the SubmitButton entirely, which is why the +// send guards live inside the handler and not only in the button's disabled +// state. +const capturedExtOptions = vi.hoisted<{ + current?: { onSubmitRef?: { current?: () => void } }; +}>(() => ({})); vi.mock("../../editor/extensions", () => ({ - createEditorExtensions: () => [], + createEditorExtensions: (options: { onSubmitRef?: { current?: () => void } }) => { + capturedExtOptions.current = options; + return []; + }, })); +// Mirrors the real contract that matters here: `handler` is captured at CALL +// time (content-editor passes `onUploadFileRef.current` when the upload +// starts), and on completion the real implementation dispatches into the SAME +// editor instance — which is what fires onUpdate. Tests drive that dispatch +// explicitly via `finishUpload()`. vi.mock("../../editor/extensions/file-upload", () => ({ - uploadAndInsertFile: vi.fn(), + uploadAndInsertFile: async ( + _editor: unknown, + file: File, + handler: (f: File) => Promise, + ) => { + await handler(file); + }, })); vi.mock("../../editor/utils/preprocess", () => ({ preprocessMarkdown: (value: string) => value, @@ -144,6 +166,29 @@ import { useChatStore } from "@multica/core/chat"; const TEST_RESOURCES = { en: { common: enCommon, chat: enChat } }; +function makeUpload(id: string, filename: string): UploadResult { + const link = `/api/attachments/${id}/download`; + return { + id, + filename, + workspace_id: "ws-1", + issue_id: null, + comment_id: null, + chat_session_id: null, + chat_message_id: null, + uploader_type: "member", + uploader_id: "user-1", + url: link, + download_url: link, + markdown_url: link, + content_type: "image/png", + size_bytes: 1, + created_at: new Date(0).toISOString(), + markdownLink: link, + link, + }; +} + function store() { return useChatStore.getState() as unknown as { activeSessionId: string | null; @@ -153,14 +198,43 @@ function store() { }; } -function element() { +function element(props: Partial> = {}) { return ( - + ); } +/** ContentEditor publishes upload-queue flips off `editor.on("transaction")`, + * not off onUpdate — every insert/dispatch below is a transaction in the real + * editor, so the tests emit one too. */ +function emitTransaction() { + act(() => { + for (const listener of [...transactionListeners.current]) listener(); + }); +} + +/** Put the editor in the state a live upload leaves it in: file-upload.ts + * inserts a blob placeholder node (attrs.uploading = true) via a transaction, + * so `hasUploadingNode` — and therefore Guard 0, `hasActiveUploads`, and the + * host's upload gate — all report an upload in flight. */ +function beginUpload(markdown: string) { + editorState.uploadingNodes = [{ attrs: { uploading: true } }]; + type(markdown); + emitTransaction(); +} + +/** What file-upload.ts does on completion: swap the blob for the CDN URL and + * clear the uploading flag, then dispatch into the SAME editor instance. That + * dispatch both fires onUpdate and, as a transaction, is how ContentEditor + * publishes the flip that un-gates the host. */ +function finishUpload(markdown: string) { + editorState.uploadingNodes = []; + type(markdown); + emitTransaction(); +} + /** Simulate real typing: move the document, then fire the editor's own * (debounced) onUpdate exactly as Tiptap would. */ function type(markdown: string) { @@ -249,6 +323,129 @@ describe("ChatInput draft isolation across a composer switch (real debounce)", ( expect(store().inputDrafts).toEqual({ __new__: "half a thought" }); }); + // An in-flight upload PINS the editor to the source document: Guard 0 in + // ContentEditor refuses to setContent over an `uploading` node (doing so + // strands the upload's finalize and the file vanishes). So while the upload + // runs, the instance still holds A's document even though the user is on B — + // and the upload's own completion dispatch fires onUpdate on that instance. + // Every write it produces belongs to A, not to wherever the user navigated. + // + // `useUploadGate` does NOT protect this: it gates submit, not session/agent + // navigation (use-chat-controller.ts handleSelectSession has no upload check). + describe("with an upload in flight", () => { + it("keeps the completing upload's markdown in the source draft, never the one switched to", () => { + store().activeSessionId = "session-a"; + store().inputDrafts["session-b"] = "B's own words"; + const { rerender } = render(element()); + + beginUpload("look at this ![](blob:local-preview)"); + + // User clicks session B while the upload is still running. + store().activeSessionId = "session-b"; + rerender(element()); + + // Upload lands: blob → CDN URL, uploading flag cleared, dispatched into + // the same instance. + finishUpload("look at this ![](/api/attachments/att-1/download)"); + act(() => { + vi.advanceTimersByTime(1000); + }); + + // B must never receive A's body or its attachment URL. + expect(store().inputDrafts["session-b"]).toBe("B's own words"); + // The upload result belongs to the draft it was started in. + expect(store().inputDrafts["session-a"]).toBe( + "look at this ![](/api/attachments/att-1/download)", + ); + }); + + it("binds an attachment dropped while the editor is still pinned to the source draft", async () => { + store().activeSessionId = "session-a"; + const onUploadFile = vi.fn(async (_file: File) => makeUpload("att-2", "second.png")); + const { rerender } = render(element({ onUploadFile })); + + // An upload is already in flight, so Guard 0 pins the editor to A's + // document. + beginUpload("first ![](blob:one)"); + store().activeSessionId = "session-b"; + rerender(element({ onUploadFile })); + + // The composer still shows A's document, so a file dropped now lands in + // A's body — its attachment row has to bind to A, or the body and the + // staged attachment end up in different drafts. + await act(async () => { + fireEvent.drop(screen.getByTestId("editor-content"), { + dataTransfer: { files: [new File(["x"], "second.png", { type: "image/png" })] }, + }); + await Promise.resolve(); + }); + + const addAttachment = ( + useChatStore.getState() as unknown as { + addInputDraftAttachment: ReturnType; + } + ).addInputDraftAttachment; + expect(addAttachment).toHaveBeenCalledWith( + "session-a", + expect.objectContaining({ id: "att-2" }), + ); + expect(addAttachment).not.toHaveBeenCalledWith("session-b", expect.anything()); + }); + + it("refuses to send while the composer still holds the source draft's document", () => { + // The upload gate covers most of the pinned window, but not the sliver + // between the upload's final dispatch (uploading node gone, so + // `hasActiveUploads()` is already false) and the re-render that adopts — + // React flushes that update on a scheduler task, so a click can land + // first. Sending then would post A's text into B's session. + store().activeSessionId = "session-a"; + const onSend = vi.fn(); + const { rerender } = render(element({ onSend })); + + beginUpload("A's words ![](blob:one)"); + store().activeSessionId = "session-b"; + rerender(element({ onSend })); + + // Upload's node is gone, but no transaction has re-rendered us yet, so + // the adopt has not run: the editor still holds A. + editorState.uploadingNodes = []; + act(() => { + latestEditorOptions.current?.onUpdate?.({ editor: editorInstance.current }); + }); + + // Mod+Enter, which skips the SubmitButton (and its disabled state) + // entirely — the only way to actually reach handleSend in this window. + act(() => { + capturedExtOptions.current?.onSubmitRef?.current?.(); + }); + + expect(onSend).not.toHaveBeenCalled(); + }); + + it("loads the target draft once the upload guard clears", () => { + store().activeSessionId = "session-a"; + store().inputDrafts["session-b"] = "B's own words"; + const { rerender } = render(element()); + + beginUpload("uploading ![](blob:local-preview)"); + store().activeSessionId = "session-b"; + rerender(element()); + + // Guard 0 blocks the sync while the upload runs — and it CONSUMES the + // defaultValue change (lastDefaultValueRef advances before the guard), so + // nothing re-applies it later on its own. + mockSetContent.mockClear(); + + finishUpload("uploaded ![](/api/attachments/att-1/download)"); + act(() => { + vi.advanceTimersByTime(1000); + }); + + // The composer must end up showing B's draft, not A's document. + expect(mockSetContent).toHaveBeenCalled(); + }); + }); + it("does not strand the last keystrokes when a lazy session create re-keys the draft mid-compose", () => { // First send / first upload in a New Chat flips activeSessionId null → uuid // under a live editor. Those bytes were typed in the New Chat slot, so they diff --git a/packages/views/chat/components/chat-input.test.tsx b/packages/views/chat/components/chat-input.test.tsx index 9f92a0d049..b97074378e 100644 --- a/packages/views/chat/components/chat-input.test.tsx +++ b/packages/views/chat/components/chat-input.test.tsx @@ -119,6 +119,11 @@ vi.mock("../../editor", async () => ({ // the draft-switch flush that depends on it) is covered against the real // ContentEditor in chat-input-draft-isolation.test.tsx. flushPendingUpdate: () => null, + // Same file: the upload-pinned adopt path needs the real Guard 0, which + // this mock has no concept of. Kept so the ref honours the full contract. + adoptContent: (markdown: string) => { + valueRef.current = markdown; + }, })); return (