diff --git a/packages/webui/webapp/components/composer.tsx b/packages/webui/webapp/components/composer.tsx index 97aa3ed7..144a79e0 100644 --- a/packages/webui/webapp/components/composer.tsx +++ b/packages/webui/webapp/components/composer.tsx @@ -14,12 +14,18 @@ import { import { createPortal } from "react-dom"; import * as api from "@/lib/api"; +import { clientId } from "@/lib/cid"; import { getComposerDraft, setComposerDraft, subscribeComposerDraft, } from "@/lib/composer-draft"; -import { useSessionContext } from "@/lib/store"; +import { + completeComposerSent, + failComposerSent, + startComposerSent, +} from "@/lib/composer-sent"; +import { getActiveSessionId, useSessionContext } from "@/lib/store"; import { decodeTranscript } from "@/lib/transcript"; import { translate, type Locale, type MessageKey } from "@/lib/i18n"; import { ContextMeter } from "./context-meter"; @@ -146,6 +152,19 @@ export function Composer({ const [slashIndex, setSlashIndex] = useState(0); const editorRef = useRef(null); const fileRef = useRef(null); + // The live session id used to live on a per-instance ref here. + // That was correct for chat→chat switching (the instance survives) + // but wrong for the home↔chat boundary: page.tsx swaps the + // composer between two tree positions when `hasConversation` + // flips, and creating a fresh session clears `chat`, which can + // unmount the composer mid-flight. The in-flight closure keeps a + // ref frozen at the dispatch-time session id and never sees the + // rotation. Reading from `getActiveSessionId()` at catch time + // resolves the live session id from the module-scope snapshot + // the SSE handler writes — the same store `composer-draft.ts` + // and `composer-sent.ts` already use to survive remounts. + // See lib/store.tsx#getActiveSessionId and the tripwire test + // `composer-submit-tripwire.test.ts` for the wiring pin. // Drag-and-drop overlay state. The counter lives in a ref so that the // dragenter/dragleave sequence can update it without scheduling a // re-render on every event — only the visible overlay (driven by @@ -316,20 +335,101 @@ export function Composer({ const submit = useCallback(async () => { const content = value.trim(); if ((!content && attachments.length === 0) || readOnly || sending) return; + // Capture the dispatch context — what session this send was FOR. + // The outbox record stores these, so a later failure can identify + // its owner. They are NOT the values the catch branch compares + // against; the catch branch reads the LIVE context (see below). + const dispatchCid = clientId(); + const dispatchSessionId = state?.sessionId ?? null; setSending(true); setComposerDraft({ error: null }); + // Ticket 13 — optimistic clear. The backend does session + // switching and transcript backfill before its ack, so waiting + // for the await leaves the text sitting in the box for the whole + // in-flight window. Park the message in the outbox (a sibling + // module-scope store to `composer-draft.ts`, see `lib/composer- + // sent.ts`) and clear the composer immediately. On success the + // outbox flips to `delivered` and the SSE stream renders the user + // bubble; on failure the catch branch reads the stashed text back + // into the composer — see the design note at the top of + // `lib/composer-sent.ts`. + startComposerSent({ + cid: dispatchCid, + sessionId: dispatchSessionId, + content, + attachments, + }); + setComposerDraft({ value: "", attachments: [] }); try { // A leading slash is a command, not a message: mcode parses those, and the - // webui's own slash commands are handled server-side too. + // webui's own slash commands are handled server-side too. The same + // record/clear/restore semantics apply to both branches. if (content.startsWith("/")) await api.sendCommand(content); else await api.sendMessage({ content, attachments }); - setComposerDraft({ value: "", attachments: [] }); + completeComposerSent(); } catch (cause) { - setComposerDraft({ error: cause instanceof Error ? cause.message : String(cause) }); + const errorMessage = cause instanceof Error ? cause.message : String(cause); + // Read the LIVE context at catch time. The dispatch-side + // closure has the session id from when the user pressed + // Enter; if the user has since switched sessions (e.g. via the + // sidebar), the active session id is now different and the + // failure belongs to the old session, not the one currently + // rendered. Comparing against the captured `dispatchSessionId` + // would always succeed (dispatch vs dispatch) — that was the + // first wiring bug acceptance caught. The live context is + // resolved from the MODULE-scope store snapshot (lib/store.tsx + // #getActiveSessionId), not from a per-instance ref. The + // composer's `submit` can outlive its own React tree — + // page.tsx swaps the composer between two positions when + // `hasConversation` flips, and creating a fresh session + // clears it. An instance-scoped ref frozen at dispatch time + // never sees the rotation; the module snapshot is the same + // store the SSE handler writes, so it always reflects the + // current session. `clientId()` is module-scope too + // (lib/cid.ts), so the cid side has always been correct. + const liveCid = clientId(); + const liveSessionId = getActiveSessionId(); + // failComposerSent returns the restore payload only when the + // LIVE context still matches the dispatch context — a session + // switch mid-flight must never paste the old session's text + // into the new session's composer. + const restored = failComposerSent({ + cid: liveCid, + sessionId: liveSessionId, + error: errorMessage, + }); + // Always set the error banner — the failure is real even when + // the active session no longer matches the record (the banner + // is in the module-scope draft store too, so it outlives a + // session switch). + if (restored) { + // The user may have typed INTERIM text during the in-flight + // window. We must not clobber it — "Nothing may vanish" + // applies to both the failed message and whatever the user + // typed since. Merge: put the restored text after the + // current draft with a blank-line separator. The error + // banner explains why the original bounced; both messages + // remain editable. + const current = getComposerDraft(); + const interim = current.value.trim(); + const mergedValue = + interim.length > 0 + ? `${current.value}\n\n${restored.content}` + : restored.content; + // Restored attachments come first so the chip list reads in + // the order the user assembled it (the failed message's + // attachments, then any new attachments added meanwhile). + const mergedAttachments = [ + ...restored.attachments, + ...current.attachments, + ]; + setComposerDraft({ value: mergedValue, attachments: mergedAttachments }); + } + setComposerDraft({ error: errorMessage }); } finally { setSending(false); } - }, [value, attachments, readOnly, sending]); + }, [value, attachments, readOnly, sending, state?.sessionId]); const onPickFiles = useCallback(async (files: FileList | null) => { if (!files?.length) return; diff --git a/packages/webui/webapp/lib/composer-sent.ts b/packages/webui/webapp/lib/composer-sent.ts new file mode 100644 index 00000000..ee060ae0 --- /dev/null +++ b/packages/webui/webapp/lib/composer-sent.ts @@ -0,0 +1,221 @@ +/** + * Composer outbox — the in-flight send record. + * + * Shaped like the draft store in `composer-draft.ts`: a single module-scope + * object, a `Set` of listeners, and a `subscribe`/`get` pair that + * `useSyncExternalStore` consumes. The store survives the composer + * remount that `page.tsx` triggers when the first user line arrives (a + * `useState`-held record would die with the unmounted instance), so the + * outbox is reachable both during a long in-flight window and after the + * user navigates back into the composer. + * + * Why a separate store instead of extending `composer-draft.ts`: the + * draft store is *what the user is typing right now*; the outbox is + * *what was just sent*. Mixing them would re-introduce the very + * vanishing-on-remount bug the draft store exists to prevent — a + * remount mid-send that re-uses the same store would lose either the + * draft or the record depending on which write landed last. Two + * independent module-scope stores keep the two concerns apart. + * + * Per-cid / per-session scoping: each record carries the `cid` and + * `sessionId` it was dispatched under. `failSend` only restores the + * text into the composer when both still match — switching sessions + * while a send is in flight MUST NOT paste another session's text into + * the new session's composer. + */ + +export type ComposerSentStatus = "in-flight" | "delivered" | "failed"; + +export interface ComposerSentRecord { + /** Browser-stable client id (`lib/cid.ts`). The per-cid server keeps + * one engine subprocess; the outbox is keyed by it so two browsers + * on the same machine never share a record. */ + cid: string; + /** Active session at dispatch time (`state.sessionId`, possibly + * null before the first snapshot). Session switches are caught by + * matching on this field — a failure in session A must not be + * restored into session B. */ + sessionId: string | null; + /** Trimmed text the user submitted. Preserved verbatim for restore. */ + content: string; + /** `@path` references captured at dispatch. Restored verbatim too. */ + attachments: string[]; + status: ComposerSentStatus; + /** Failure message from the rejected request; `null` while in-flight + * or after a successful delivery. */ + error: string | null; + /** `Date.now()` at the moment of the most recent state transition. + * Test-only determinism: the reducer accepts an injected timestamp + * so the same scenario is reproducible without a fake clock. */ + timestamp: number; +} + +// --- Pure reducers -------------------------------------------------------- +// +// All branching in the composer is concentrated here so the rest of the +// codebase (and the unit tests) can exercise the decision tree without a +// browser, a fetch, or a React tree. The composer's `submit` callback +// reads three transitions: +// 1. dispatch — start a new send (status -> in-flight). +// 2. complete — mark an in-flight record delivered. +// 3. fail — mark an in-flight record failed and (when cid + +// sessionId still match) hand back the text the +// composer should restore. + +/** + * Dispatch a fresh send. + * + * Attachments are deep-copied so a later mutation of the caller's + * array cannot leak into the record. The `timestamp` is overridable + * for tests; the store wrapper injects `Date.now()`. + */ +export function dispatchSend(args: { + cid: string; + sessionId: string | null; + content: string; + attachments: string[]; + timestamp?: number; +}): ComposerSentRecord { + return { + cid: args.cid, + sessionId: args.sessionId, + content: args.content, + attachments: [...args.attachments], + status: "in-flight", + error: null, + timestamp: args.timestamp ?? Date.now(), + }; +} + +/** + * Mark a record delivered. + * + * Idempotent only on `in-flight`: a `delivered` write is a no-op + * (already delivered), and a `failed` write is also a no-op so a + * late-arriving success signal after a failure cannot silently + * overwrite the failure banner. The composer calls this from the + * success branch of its try/catch; the test suite pins the no-op + * behaviour for the late-success case. + */ +export function completeSend(record: ComposerSentRecord): ComposerSentRecord { + if (record.status !== "in-flight") return record; + return { ...record, status: "delivered", error: null }; +} + +/** Restore payload returned by `failSend` when cid + sessionId match. */ +export interface RestorePayload { + content: string; + attachments: string[]; +} + +/** + * Mark an in-flight send failed and (when cid + sessionId match the + * record's) return the text + attachments to put back into the composer. + * + * Returns `null` when the dispatch context no longer matches — the + * user switched sessions, the cid rotated, etc. A `null` return means + * the record is left untouched; the failure is logged nowhere on the + * client because the wrong-session composer would have nothing to + * show it on. + * + * `null` is also returned when there is no record at all (defensive: + * the store wrapper already short-circuits, but the pure reducer has + * to behave identically for direct callers). + */ +export function failSend( + record: ComposerSentRecord | null, + args: { + cid: string; + sessionId: string | null; + error: string; + timestamp?: number; + }, +): { record: ComposerSentRecord; payload: RestorePayload } | null { + if (!record) return null; + if (record.cid !== args.cid) return null; + if (record.sessionId !== args.sessionId) return null; + const next: ComposerSentRecord = { + ...record, + status: "failed", + error: args.error, + timestamp: args.timestamp ?? Date.now(), + }; + return { + record: next, + payload: { content: record.content, attachments: [...record.attachments] }, + }; +} + +/** True iff the record's cid + sessionId match the active context. */ +export function recordMatches( + record: ComposerSentRecord | null, + cid: string, + sessionId: string | null, +): boolean { + if (!record) return false; + return record.cid === cid && record.sessionId === sessionId; +} + +// --- Store ----------------------------------------------------------------- +// +// One record, last write wins. Only one send is ever in flight per +// composer instance (the `sending` flag is the gate); a second submit +// while the first is unresolved overwrites the previous record, which +// is the right behaviour — the second message is the one the user is +// now committed to sending. + +const listeners = new Set<() => void>(); +let record: ComposerSentRecord | null = null; + +export function startComposerSent(args: { + cid: string; + sessionId: string | null; + content: string; + attachments: string[]; +}): ComposerSentRecord { + record = dispatchSend(args); + for (const listener of listeners) listener(); + return record; +} + +export function completeComposerSent(): void { + if (!record || record.status !== "in-flight") return; + record = completeSend(record); + for (const listener of listeners) listener(); +} + +/** + * Mark the in-flight record failed. Returns the restore payload when + * the dispatch context still matches; otherwise `null` and the record + * is left as-is (the failure belongs to a session the user is no + * longer looking at). + */ +export function failComposerSent(args: { + cid: string; + sessionId: string | null; + error: string; +}): RestorePayload | null { + const result = failSend(record, args); + if (!result) return null; + record = result.record; + for (const listener of listeners) listener(); + return result.payload; +} + +export function getComposerSent(): ComposerSentRecord | null { + return record; +} + +/** `useSyncExternalStore` subscription. Returns the unsubscribe thunk. */ +export function subscribeComposerSent(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +/** Test-only: clear the store and its listeners between cases. */ +export function resetComposerSentForTests(): void { + record = null; + listeners.clear(); +} \ No newline at end of file diff --git a/packages/webui/webapp/lib/store.tsx b/packages/webui/webapp/lib/store.tsx index c8c20a9a..1316f4df 100644 --- a/packages/webui/webapp/lib/store.tsx +++ b/packages/webui/webapp/lib/store.tsx @@ -110,6 +110,29 @@ export function __testSnapshot(): StoreSnapshot { return snapshot; } +/** + * Active session id from the module-scope snapshot. + * + * Why a separate accessor instead of reading from a React component's + * closure: the SSE-driven store lives at module scope (it must — the + * SSE connection has to outlive every component). A composer's + * in-flight submit can be closed over a ref that was current at + * dispatch time; if the user switches sessions, that ref is frozen + * at the OLD session id because the component may have remounted + * (page.tsx swaps the composer between the inline and chat-tree + * positions when `hasConversation` flips). Reading from this + * accessor at catch time resolves the live session id from the + * SAME module-scope state the SSE handler writes — it survives + * remounts and is updated by every state push. + * + * The composer's `submit` reads this at catch time so the cid + + * sessionId restore-gate compares against the active context, not + * a stale closure snapshot. + */ +export function getActiveSessionId(): string | null { + return snapshot.state ? snapshot.state.sessionId : null; +} + /** * Test-only handle: simulate an SSE frame dispatch. Production code * NEVER calls this — it exists so the revision-guard reducer can be diff --git a/packages/webui/webapp/test/composer-sent.test.ts b/packages/webui/webapp/test/composer-sent.test.ts new file mode 100644 index 00000000..dd260675 --- /dev/null +++ b/packages/webui/webapp/test/composer-sent.test.ts @@ -0,0 +1,453 @@ +// webapp/test/composer-sent.test.ts +// +// Contract tests for `lib/composer-sent.ts` — the composer's outbox. +// +// Why this exists: ticket 13 (composer-send-echo). The bug was that the +// composer's success path cleared the draft AFTER the awaited request +// resolved, so the entire in-flight window left the text sitting in the +// box (the user's screenshot showed exactly that). The fix moves the +// clear BEFORE the await and parks the message in a module-scope +// outbox; on failure, the outbox is the only place the message lives +// and the catch branch reads it back into the composer. These tests +// pin the three transitions the composer relies on so a regression in +// the pure logic surfaces without a browser: +// +// 1. dispatch — start a new send (status -> in-flight, outbox +// carries the text + attachments). +// 2. complete — mark an in-flight record delivered. A late write +// after a failure must NOT silently flip the record +// back to delivered (that would hide the failure). +// 3. fail — mark failed and (when cid + sessionId still +// match) hand back the restore payload. A cid or +// sessionId mismatch returns `null` and leaves the +// record untouched, so switching sessions while a +// send is in flight never pastes another session's +// text into the new session's composer. +// +// The module-scope store wrapper is also tested for its `subscribe` +// notification contract, which mirrors `composer-draft.ts`'s so the +// two stores share the same React-free shape. + +import { test, describe, beforeEach } from "node:test"; +import assert from "node:assert/strict"; + +import { + completeComposerSent, + completeSend, + dispatchSend, + failComposerSent, + failSend, + getComposerSent, + recordMatches, + resetComposerSentForTests, + startComposerSent, + subscribeComposerSent, +} from "../lib/composer-sent"; + +const CID_A = "cid-aaaa"; +const CID_B = "cid-bbbb"; +const SESSION_1 = "session-1"; +const SESSION_2 = "session-2"; + +beforeEach(() => { + resetComposerSentForTests(); +}); + +describe("dispatchSend — optimistic clear records the in-flight message", () => { + test("creates an in-flight record with the dispatch context", () => { + const record = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "继续这个任务", + attachments: ["@uploads/a.txt"], + timestamp: 1_700_000_000_000, + }); + assert.equal(record.status, "in-flight"); + assert.equal(record.error, null); + assert.equal(record.cid, CID_A); + assert.equal(record.sessionId, SESSION_1); + assert.equal(record.content, "继续这个任务"); + assert.deepEqual(record.attachments, ["@uploads/a.txt"]); + assert.equal(record.timestamp, 1_700_000_000_000); + }); + + test("attachments array is copied, not aliased", () => { + const attachments = ["@uploads/a.txt"]; + const record = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments, + timestamp: 1, + }); + // Mutating the caller's array must not leak into the record. This + // is what the composer relies on: the draft store and the outbox + // share the attachments array via reference, and a clear-then- + // reassign in the draft store would otherwise also rewrite the + // outbox. + attachments.push("@uploads/b.txt"); + assert.deepEqual(record.attachments, ["@uploads/a.txt"]); + }); + + test("sessionId null is allowed — covers the pre-snapshot window", () => { + const record = dispatchSend({ + cid: CID_A, + sessionId: null, + content: "first message before session id lands", + attachments: [], + timestamp: 1, + }); + assert.equal(record.sessionId, null); + }); +}); + +describe("completeSend — success path", () => { + test("in-flight -> delivered, error cleared", () => { + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + timestamp: 1, + }); + const completed = completeSend(dispatched); + assert.equal(completed.status, "delivered"); + assert.equal(completed.error, null); + // content / attachments / cid / sessionId preserved through the + // transition — the delivered marker is the only thing that + // changes. + assert.equal(completed.content, "msg"); + assert.equal(completed.cid, CID_A); + assert.equal(completed.sessionId, SESSION_1); + }); + + test("already-delivered write is idempotent", () => { + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + timestamp: 1, + }); + const first = completeSend(dispatched); + const second = completeSend(first); + assert.equal(second.status, "delivered"); + // Same identity (no spread) — a no-op reducer should not allocate. + assert.equal(second, first); + }); + + test("late success after a failure does NOT mark the record delivered", () => { + // The composer's success branch runs in the try, the failure + // branch runs in the catch. If the server eventually acks AFTER + // the catch already ran (very late retry, double-tap on Enter, + // etc.) a completeSend against the failed record must not flip + // it back to delivered — that would silently hide the failure + // banner the user is looking at. + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + timestamp: 1, + }); + const failed = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_1, + error: "boom", + timestamp: 2, + })!.record; + const lateSuccess = completeSend(failed); + assert.equal(lateSuccess.status, "failed"); + assert.equal(lateSuccess.error, "boom"); + }); +}); + +describe("failSend — failure path with cid + sessionId scoping", () => { + test("matching cid + sessionId returns the restore payload and marks failed", () => { + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "继续这个任务", + attachments: ["@uploads/a.txt"], + timestamp: 1, + }); + const result = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_1, + error: "no response within 30000ms", + timestamp: 2, + }); + assert.ok(result, "failSend must return a result on a matching context"); + assert.equal(result!.record.status, "failed"); + assert.equal(result!.record.error, "no response within 30000ms"); + assert.equal(result!.record.timestamp, 2); + // The payload the composer restores into the textarea + chip list. + assert.deepEqual(result!.payload, { + content: "继续这个任务", + attachments: ["@uploads/a.txt"], + }); + }); + + test("payload attachments are copied, not aliased to the record", () => { + // Same isolation guarantee as dispatchSend: a later draft-store + // clear must not also wipe the payload the catch branch is about + // to write back. + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: ["@a", "@b"], + timestamp: 1, + }); + const result = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_1, + error: "x", + timestamp: 2, + })!; + const payload = result.payload; + // A new dispatch from the store wrapper would replace the record, + // not mutate the payload's attachments array — the test below + // pins this by re-dispatching and re-reading the payload array + // identity would change anyway, so we instead check that the + // payload array is not the SAME reference as the record's + // attachments array. + assert.notEqual(payload.attachments, dispatched.attachments); + }); + + test("cid mismatch -> null, record untouched", () => { + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + timestamp: 1, + }); + const result = failSend(dispatched, { + cid: CID_B, // wrong cid + sessionId: SESSION_1, + error: "x", + timestamp: 2, + }); + assert.equal(result, null); + // Caller is expected to leave the record alone on a null return. + assert.equal(dispatched.status, "in-flight"); + }); + + test("sessionId mismatch -> null, record untouched (the session-switch case)", () => { + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg in session 1", + attachments: [], + timestamp: 1, + }); + // User has now switched to session 2; the cid is the same (same + // browser) but the session is different. Restoring here would + // paste session-1's text into session-2's composer — the bug + // the scoping exists to prevent. + // + // This is the test that would have caught acceptance's first- + // pass regression: if the caller wired `failComposerSent` to + // receive the DISPATCH-time sessionId (the same value already + // on the record), this assertion would still hold because the + // reducer is correct — the bug was in the CALLER, not the + // reducer. The composer's tripwire test pins the wiring; this + // test pins the reducer's behaviour when given the right input. + const result = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_2, + error: "x", + timestamp: 2, + }); + assert.equal(result, null); + assert.equal(dispatched.status, "in-flight"); + }); + + test("live-context semantics: args are the LIVE catch-time context", () => { + // The contract the composer's catch branch relies on: the + // second argument to failSend is the LIVE context — what the + // user is currently looking at — and is checked against the + // DISPATCH context stored in the record. A rotation between + // dispatch and catch (session switch, cid rotation) is exactly + // what the gate exists to catch. The composer's tripwire pins + // the wiring (the caller reads the live context at catch + // time); this test pins the reducer's contract. + const dispatched = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg in session 1", + attachments: ["@uploads/a.txt"], + timestamp: 1, + }); + // Catch time: user has switched to session 2 (same cid). + const liveResult = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_2, + error: "x", + timestamp: 2, + }); + assert.equal(liveResult, null, "session rotation must abort the restore"); + // And when the active session matches the dispatch session, + // the same record IS restored — the gate is symmetric. + const sameSessionResult = failSend(dispatched, { + cid: CID_A, + sessionId: SESSION_1, + error: "x", + timestamp: 3, + }); + assert.ok(sameSessionResult); + assert.equal(sameSessionResult!.record.status, "failed"); + assert.deepEqual(sameSessionResult!.payload.attachments, ["@uploads/a.txt"]); + }); + + test("null record -> null (defensive: store wrapper already guards this)", () => { + const result = failSend(null, { + cid: CID_A, + sessionId: SESSION_1, + error: "x", + timestamp: 1, + }); + assert.equal(result, null); + }); +}); + +describe("recordMatches — pure cid + sessionId check", () => { + const record = dispatchSend({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + timestamp: 1, + }); + + test("matching context -> true", () => { + assert.equal(recordMatches(record, CID_A, SESSION_1), true); + }); + test("cid mismatch -> false", () => { + assert.equal(recordMatches(record, CID_B, SESSION_1), false); + }); + test("sessionId mismatch -> false", () => { + assert.equal(recordMatches(record, CID_A, SESSION_2), false); + }); + test("null record -> false", () => { + assert.equal(recordMatches(null, CID_A, SESSION_1), false); + }); +}); + +describe("store wrapper — module-scope subscribe/get, mirrors composer-draft.ts", () => { + test("startComposerSent writes a record visible to getComposerSent", () => { + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: ["@uploads/a.txt"], + }); + const record = getComposerSent(); + assert.ok(record); + assert.equal(record!.status, "in-flight"); + assert.equal(record!.content, "msg"); + assert.deepEqual(record!.attachments, ["@uploads/a.txt"]); + }); + + test("completeComposerSent flips an in-flight record to delivered", () => { + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + }); + completeComposerSent(); + assert.equal(getComposerSent()!.status, "delivered"); + }); + + test("completeComposerSent is a no-op when there is no record", () => { + // Defensive: the catch branch of the composer should never call + // completeComposerSent, but if a future refactor wires it + // incorrectly the guard prevents an exception. + completeComposerSent(); + assert.equal(getComposerSent(), null); + }); + + test("failComposerSent returns the restore payload on matching context", () => { + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: ["@a"], + }); + const payload = failComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + error: "boom", + }); + assert.deepEqual(payload, { content: "msg", attachments: ["@a"] }); + assert.equal(getComposerSent()!.status, "failed"); + assert.equal(getComposerSent()!.error, "boom"); + }); + + test("failComposerSent on session-switch returns null and leaves record untouched", () => { + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "msg", + attachments: [], + }); + const payload = failComposerSent({ + cid: CID_A, + sessionId: SESSION_2, + error: "x", + }); + assert.equal(payload, null); + // The record is the original dispatch — switching sessions must + // never leak the text, but the record is also not rewritten, so a + // later dispatcher (the next send in session 1, if the user + // navigates back) sees the unchanged record it overwrites. + assert.equal(getComposerSent()!.status, "in-flight"); + }); + + test("a second dispatch overwrites the previous record", () => { + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "first", + attachments: [], + }); + startComposerSent({ + cid: CID_A, + sessionId: SESSION_1, + content: "second", + attachments: [], + }); + assert.equal(getComposerSent()!.content, "second"); + assert.equal(getComposerSent()!.status, "in-flight"); + }); + + test("subscribers are notified on every write", () => { + const seen: string[] = []; + const unsubscribe = subscribeComposerSent(() => { + seen.push(getComposerSent()?.content ?? "(none)"); + }); + startComposerSent({ cid: CID_A, sessionId: SESSION_1, content: "a", attachments: [] }); + startComposerSent({ cid: CID_A, sessionId: SESSION_1, content: "b", attachments: [] }); + unsubscribe(); + startComposerSent({ cid: CID_A, sessionId: SESSION_1, content: "c", attachments: [] }); + assert.deepEqual(seen, ["a", "b"]); + }); + + test("completeComposerSent and failComposerSent also notify", () => { + let calls = 0; + const unsubscribe = subscribeComposerSent(() => { + calls += 1; + }); + startComposerSent({ cid: CID_A, sessionId: SESSION_1, content: "a", attachments: [] }); + assert.equal(calls, 1); + completeComposerSent(); + assert.equal(calls, 2); + startComposerSent({ cid: CID_A, sessionId: SESSION_1, content: "b", attachments: [] }); + assert.equal(calls, 3); + failComposerSent({ cid: CID_A, sessionId: SESSION_1, error: "x" }); + assert.equal(calls, 4); + unsubscribe(); + }); +}); \ No newline at end of file diff --git a/packages/webui/webapp/test/composer-submit-tripwire.test.ts b/packages/webui/webapp/test/composer-submit-tripwire.test.ts new file mode 100644 index 00000000..c41b871d --- /dev/null +++ b/packages/webui/webapp/test/composer-submit-tripwire.test.ts @@ -0,0 +1,248 @@ +// webapp/test/composer-submit-tripwire.test.ts +// +// Static-source tripwire for the composer submit wiring. +// +// Why this exists: ticket 13. The pure-reducer tests in +// `composer-sent.test.ts` cover the decision tree (dispatch / +// complete / fail, cid + sessionId scoping), but the actual bug was +// the ORDER of operations in `submit` — clearing the draft after +// `await api.sendMessage(...)` instead of before. A regression to +// the clear-after-await ordering would keep all 546 webapp tests +// green (the reducers are pure and order-agnostic) and silently +// re-introduce the user-reported bug ("text sitting in the box until +// the reply arrives"). This tripwire is the only thing that pins the +// WIRING. +// +// The same problem applied to the cid + sessionId restore-gate: the +// reducer compares `record.cid` vs `args.cid` and `record.sessionId` +// vs `args.sessionId`, so the gate is only meaningful when the args +// are the LIVE context (catch-time state) and the record is the +// DISPATCH context. The first version of the fix passed the +// closure-captured dispatch values to BOTH calls and acceptance +// caught the resulting leak: a session-A failure pasted A's text +// into session-B's composer. v2 moved to a per-instance ref +// (`liveStateRef.current`); that worked for chat→chat switches but +// not for the 新建会话 → fresh empty B flow, because page.tsx swaps +// the composer between the inline and chat-tree positions when +// `hasConversation` flips. The current fix reads from the +// MODULE-scope store snapshot (lib/store.tsx#getActiveSessionId) +// so the live session id outlives any composer remount. +// +// The tripwire is intentionally tight: every assertion checks the +// call site (that an identifier appears AT THE CALL with the right +// meaning), not just that the identifier exists somewhere in the +// file. A plausible revert that passes `dispatchSessionId` to +// `failComposerSent` while still importing `getActiveSessionId` +// fails the live-context assertion; a refactor that moves the +// `await api.sendMessage` ahead of `setComposerDraft({ value: "", +// attachments: [] })` fails the ordering assertion. A tripwire +// that cannot fail on a plausible revert is decoration. + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { resolve, dirname } from "node:path"; + +const here = dirname(fileURLToPath(import.meta.url)); +const composerSource = readFileSync( + resolve(here, "../components/composer.tsx"), + "utf8", +); + +function indexOfOrThrow(haystack: string, needle: string, label: string): number { + const idx = haystack.indexOf(needle); + if (idx < 0) { + throw new Error(`tripwire missing in composer.tsx: ${label}`); + } + return idx; +} + +/** + * Slice the source from a marker to the next `};` or end-of-file. + * Returns just enough context for the call-site assertion below. + */ +function sliceAfter(haystack: string, marker: string, label: string, maxLen = 1200): string { + const idx = indexOfOrThrow(haystack, marker, label); + return haystack.slice(idx, idx + maxLen); +} + +describe("composer submit ordering — ticket 13 wiring tripwire", () => { + test("the outbox record is parked BEFORE any awaited send", () => { + // The optimistic-clear ordering: startComposerSent + the draft + // clear must precede `await api.sendMessage` / `await api.sendCommand`. + // If a refactor regresses to clear-after-await, the user's bug + // returns ("text sitting in the box until the reply arrives") + // and all 21 reducer tests still pass — this tripwire is what + // pins the wiring. + const startSendIdx = indexOfOrThrow( + composerSource, + "startComposerSent({", + "startComposerSent({", + ); + const clearDraftIdx = indexOfOrThrow( + composerSource, + 'setComposerDraft({ value: "", attachments: [] })', + 'setComposerDraft({ value: "", attachments: [] })', + ); + const awaitSendIdx = indexOfOrThrow( + composerSource, + "await api.sendMessage", + "await api.sendMessage", + ); + const awaitCmdIdx = indexOfOrThrow( + composerSource, + "await api.sendCommand", + "await api.sendCommand", + ); + const earliestAwait = Math.min(awaitSendIdx, awaitCmdIdx); + + assert.ok( + startSendIdx < earliestAwait, + `startComposerSent must be called before any await ` + + `(start=${startSendIdx}, earliestAwait=${earliestAwait})`, + ); + assert.ok( + clearDraftIdx < earliestAwait, + `setComposerDraft({value:"", attachments:[]}) must be called before any await ` + + `(clear=${clearDraftIdx}, earliestAwait=${earliestAwait})`, + ); + assert.ok( + clearDraftIdx > startSendIdx, + `the draft clear must come AFTER startComposerSent so the record is ` + + `parked first (start=${startSendIdx}, clear=${clearDraftIdx})`, + ); + }); + + test("the catch branch passes LIVE module-scope values to failComposerSent", () => { + // The cid + sessionId restore-gate compares the LIVE context + // against the record. If `submit` passes the closure-captured + // `dispatchSessionId` to `failComposerSent`, the gate compares + // dispatch-time values against themselves and always passes — + // session-A failures leak into session-B's composer (the bug + // both rounds of acceptance caught). + // + // The fix reads the live session id from the MODULE-scope + // store snapshot (`getActiveSessionId()` from lib/store.tsx). + // The cid side has always been module-scope (`clientId()` from + // lib/cid.ts). + // + // The assertions below are on the CATCH BLOCK as a whole, not + // the identifier presence. A plausible revert that keeps the + // live-context identifier somewhere in the file but passes + // closure constants to `failComposerSent` will trip this — + // the catch branch must call `clientId()` AND + // `getActiveSessionId()`, and the dispatch-time constants must + // NOT appear at the call site. + + // Slice the catch block: from `} catch (cause) {` to the end of + // file, then take just the call-site argument window. Use a + // generous window so the surrounding `liveCid = clientId()` + // assignments are still in the slice (a refactor that reads + // `clientId()` inline at the call site would also pass; the + // important property is that the call uses the module-scope + // accessor, not a closure constant). + const catchStartIdx = indexOfOrThrow( + composerSource, + "} catch (cause) {", + "} catch (cause) {", + ); + const callIdx = indexOfOrThrow( + composerSource, + "failComposerSent({", + "failComposerSent({", + ); + assert.ok( + callIdx > catchStartIdx, + "failComposerSent must be called inside the catch branch", + ); + // Slice the catch block from `} catch (cause) {` through the + // failComposerSent call (and a little beyond). This captures + // both the live-value definitions and the call site. + const catchSlice = composerSource.slice(catchStartIdx, callIdx + 600); + + // The live module-scope accessors must appear in the catch + // block. Both `clientId()` (lib/cid.ts) and + // `getActiveSessionId()` (lib/store.tsx) are module-scope — + // they survive any composer remount and reflect the SSE-driven + // state pushes. The cid side has always been module-scope; the + // sessionId side moved from a per-instance ref to this accessor + // because the per-instance ref froze at dispatch time when + // page.tsx swapped the composer between the inline and chat-tree + // positions on a fresh-session switch. + assert.ok( + /clientId\s*\(\s*\)/.test(catchSlice), + "the catch block must call clientId() — cid is module-scope and a " + + "closure constant cid breaks cross-tab isolation", + ); + assert.ok( + /getActiveSessionId\s*\(\s*\)/.test(catchSlice), + "the catch block must call getActiveSessionId() — the live session id " + + "must come from the module-scope store snapshot, not from a per-" + + "instance ref or closure constant. Per-instance refs die with the " + + "composer (page.tsx swaps the composer between two tree positions " + + "when hasConversation flips), which is exactly what the 新建会话 → " + + "fresh empty B scenario does.", + ); + + // The dispatch-time constants must NOT appear inside the + // failComposerSent argument list. They MAY appear elsewhere in + // the catch block (the cid might still be re-read for logging), + // but they must not be the values fed to the gate. + const callArgs = composerSource.slice( + callIdx, + composerSource.indexOf("});", callIdx) + 3, + ); + assert.ok( + !/\bdispatchSessionId\b/.test(callArgs), + "failComposerSent must not be passed dispatchSessionId — that was " + + "the v1 bug. The arg must be the LIVE value (getActiveSessionId()).", + ); + assert.ok( + !/\bdispatchCid\b/.test(callArgs), + "failComposerSent must not be passed dispatchCid — cid is module-scope " + + "via clientId() and must be read at catch time, not reused from " + + "dispatch.", + ); + }); + + test("the live accessor is imported into the composer module", () => { + // A plausible revert could remove the import and pass closure + // constants — the call site check above would still catch it + // (the call site would lack `getActiveSessionId()`), but + // belt-and-braces: the import must be present at the top of + // the module. + assert.ok( + /import\s+\{[^}]*\bgetActiveSessionId\b[^}]*\}\s+from\s+["']@\/lib\/store["']/.test( + composerSource, + ), + "getActiveSessionId must be imported from @/lib/store at the top of " + + "the composer module — the catch branch needs the module-scope " + + "accessor.", + ); + }); + + test("the success branch flips the record to delivered", () => { + // Pairs with the optimistic-clear ordering: a record parked and + // never closed would leave stale 'in-flight' records accumulating + // on every send. completeComposerSent must run on the success + // path so a later 'failSend' against the same cid + sessionId + // would no-op (the reducer returns the record unchanged when it + // is not 'in-flight'). + const successIdx = indexOfOrThrow( + composerSource, + "completeComposerSent();", + "completeComposerSent();", + ); + const catchIdx = indexOfOrThrow( + composerSource, + "} catch (cause) {", + "} catch (cause) {", + ); + assert.ok( + successIdx < catchIdx, + `completeComposerSent must be called BEFORE the catch branch ` + + `(complete=${successIdx}, catch=${catchIdx})`, + ); + }); +}); \ No newline at end of file diff --git a/release/public-source.json b/release/public-source.json index 229f4552..cf1a5822 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3632,6 +3632,7 @@ "packages/webui/webapp/lib/browser-nav.ts", "packages/webui/webapp/lib/cid.ts", "packages/webui/webapp/lib/composer-draft.ts", + "packages/webui/webapp/lib/composer-sent.ts", "packages/webui/webapp/lib/file-open-reason.ts", "packages/webui/webapp/lib/file-preview.ts", "packages/webui/webapp/lib/files-tree.ts", @@ -3673,6 +3674,8 @@ "packages/webui/webapp/test/cid.test.ts", "packages/webui/webapp/test/composer-draft.test.ts", "packages/webui/webapp/test/composer-models.test.ts", + "packages/webui/webapp/test/composer-sent.test.ts", + "packages/webui/webapp/test/composer-submit-tripwire.test.ts", "packages/webui/webapp/test/context-meter-format.test.ts", "packages/webui/webapp/test/file-open-reason.test.ts", "packages/webui/webapp/test/file-preview.test.ts",