From ca19a2187a0e2bb85fee90e77852da94bd01a288 Mon Sep 17 00:00:00 2001 From: Yuqing Yang Date: Wed, 14 Oct 2026 09:55:42 +0000 Subject: [PATCH] fix(web): reconcile first-turn question writes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../canvasStore.agentDeltaConflict.test.ts | 80 ++++++- .../store/canvasStore.questionRestore.test.ts | 64 +++-- apps/web/src/store/canvasStore.ts | 62 +++-- .../nodeContentQueue.baseline.test.ts | 156 +++++++++++- .../canvasStore/save/nodeContentQueue.ts | 222 ++++++++++++------ docs/architecture/canvas-realtime-sync.md | 14 +- docs/architecture/canvas-storage.md | 9 +- docs/architecture/question-node.md | 7 +- 8 files changed, 484 insertions(+), 130 deletions(-) diff --git a/apps/web/src/store/canvasStore.agentDeltaConflict.test.ts b/apps/web/src/store/canvasStore.agentDeltaConflict.test.ts index 05b5b9c8d..f3f9b3aad 100644 --- a/apps/web/src/store/canvasStore.agentDeltaConflict.test.ts +++ b/apps/web/src/store/canvasStore.agentDeltaConflict.test.ts @@ -63,6 +63,7 @@ beforeEach(() => { }); afterEach(() => { + useCanvasStore.setState({ nodes: [] }); vi.clearAllTimers(); vi.useRealTimers(); }); @@ -211,7 +212,7 @@ describe('agent delta conflict protection', () => { nextMetadata: {}, }, ])( - 'still skips $name after JSON transport while local content is pending', + 'applies independent $name while local content is pending', ({ prevMetadata, nextMetadata }) => { useCanvasStore .getState() @@ -223,15 +224,88 @@ describe('agent delta conflict protection', () => { .getState() .applyDeltasFromAgent([delta], 2, pendingEffects); - expect(skipped).toEqual(['question-1']); + expect(skipped).toEqual([]); expect(useCanvasStore.getState().nodes[0]).toEqual({ ...before, - data: { ...before.data, status: 'running' }, + data: { + ...before.data, + ...nextMetadata, + status: 'running', + }, }); expect(useCanvasStore.getState().version).toBe(2); }, ); + it('applies an automatic title while a local body edit is pending', () => { + useCanvasStore + .getState() + .patchNodeSilent('question-1', { content: 'Pending prompt' }); + const prev = question('', 'idle'); + const next = { + ...question('', 'running'), + data: { + ...question('', 'running').data, + label: 'Generated title', + labelSource: 'auto', + conversationTitleSource: 'generated', + }, + }; + + expect( + useCanvasStore + .getState() + .applyDeltasFromAgent( + [{ type: 'REPLACE_NODE', prev, next }], + 2, + pendingEffects, + true, + ), + ).toEqual([]); + expect(useCanvasStore.getState().nodes[0]?.data).toMatchObject({ + content: 'Pending prompt', + label: 'Generated title', + labelSource: 'auto', + conversationTitleSource: 'generated', + status: 'running', + }); + }); + + it('applies first-turn content while preserving a pending manual title', () => { + useCanvasStore.getState().patchNodeSilent('question-1', { + label: 'Manual title', + labelSource: 'user', + }); + const prev = question('', 'idle'); + const next = { + ...question('First prompt', 'running'), + data: { + ...question('First prompt', 'running').data, + label: 'Generated title', + labelSource: 'auto', + conversationTitleSource: 'generated', + }, + }; + + expect( + useCanvasStore + .getState() + .applyDeltasFromAgent( + [{ type: 'REPLACE_NODE', prev, next }], + 2, + pendingEffects, + true, + ), + ).toEqual(['question-1']); + expect(useCanvasStore.getState().nodes[0]?.data).toMatchObject({ + content: 'First prompt', + label: 'Manual title', + labelSource: 'user', + conversationTitleSource: 'generated', + status: 'running', + }); + }); + it('applies lifecycle-only changes without overwriting pending local content', () => { useCanvasStore .getState() diff --git a/apps/web/src/store/canvasStore.questionRestore.test.ts b/apps/web/src/store/canvasStore.questionRestore.test.ts index 2ed5138d1..2452c5e3e 100644 --- a/apps/web/src/store/canvasStore.questionRestore.test.ts +++ b/apps/web/src/store/canvasStore.questionRestore.test.ts @@ -5,14 +5,21 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { nodeRevisionOf } from '@huabu/shared/canvas-engine'; -const { associateNode, deleteNode, putCanvas, postCanvasExecute, forkThread } = - vi.hoisted(() => ({ - associateNode: vi.fn(), - deleteNode: vi.fn().mockResolvedValue({ success: true }), - putCanvas: vi.fn(), - postCanvasExecute: vi.fn(), - forkThread: vi.fn(), - })); +const { + associateNode, + deleteNode, + putCanvas, + putNodeContent, + postCanvasExecute, + forkThread, +} = vi.hoisted(() => ({ + associateNode: vi.fn(), + deleteNode: vi.fn().mockResolvedValue({ success: true }), + putCanvas: vi.fn(), + putNodeContent: vi.fn(), + postCanvasExecute: vi.fn(), + forkThread: vi.fn(), +})); vi.mock('../api', async (importOriginal) => ({ ...(await importOriginal()), associateAgentNode: associateNode, @@ -23,17 +30,7 @@ vi.mock('../api', async (importOriginal) => ({ })); vi.mock('../api/canvas', async (importOriginal) => ({ ...(await importOriginal()), - putNodeContent: vi.fn( - async ( - _canvasId: string, - nodeId: string, - request: PutNodeContentRequest, - ) => ({ - nodeId, - label: null, - rev: nodeRevisionOf({ content: request.content }), - }), - ), + putNodeContent, preprocessNode: vi.fn().mockResolvedValue({ success: true }), })); vi.mock('../api/agent', () => ({ agentApi: { forkThread } })); @@ -66,6 +63,18 @@ beforeEach(() => { deleteNode.mockClear(); putCanvas.mockReset(); postCanvasExecute.mockReset(); + putNodeContent.mockReset(); + putNodeContent.mockImplementation( + async ( + _canvasId: string, + nodeId: string, + request: PutNodeContentRequest, + ) => ({ + nodeId, + label: null, + rev: nodeRevisionOf({ content: request.content }), + }), + ); forkThread.mockReset(); canvasHistoryManager.activate('canvas-restore', true); useCanvasStore.getState()._setStateNoAutosave({ @@ -112,6 +121,23 @@ function createQuestion(id: `node-${string}`): Node { } describe('Question creation settlement', () => { + it('does not issue a redundant sidecar PUT for the optimistic Question', async () => { + postCanvasExecute.mockResolvedValue({ + canvasId: 'canvas-restore', + fromVersion: 1, + toVersion: 2, + results: [{ applied: true }], + deltas: [], + pendingEffects, + }); + + const node = createQuestion('node-created-on-server'); + await awaitQuestionCreation('canvas-restore', node.id); + await vi.advanceTimersByTimeAsync(500); + + expect(putNodeContent).not.toHaveBeenCalled(); + }); + it('rolls back an unsupported conversation fork without poisoning unrelated saves or undo', async () => { const error = new Error('Fork not supported (501)'); forkThread.mockRejectedValue(error); diff --git a/apps/web/src/store/canvasStore.ts b/apps/web/src/store/canvasStore.ts index 39342e53e..cd4bef76f 100644 --- a/apps/web/src/store/canvasStore.ts +++ b/apps/web/src/store/canvasStore.ts @@ -967,6 +967,7 @@ const preprocessQueue = createPreprocessQueue({ const nodeContentQueue = createNodeContentQueue({ delayMs: NODE_CONTENT_DEBOUNCE_MS, getState: () => useCanvasStore.getState(), + waitForCreation: awaitQuestionCreation, retryStructure: () => useCanvasStore.getState().saveCanvas(), onRestoredContentPersisted: (canvasId, nodeId) => preprocessQueue.resumeRestored(canvasId, nodeId), @@ -1566,6 +1567,7 @@ const useCanvasStore = create()( !state.nodes.some((old) => old.id === node.id), ); if (createdQuestions.length > 0 && resolvedSource === 'ui') { + nodeContentQueue.prepareServerCreatedNodes(createdQuestions); const creation = Promise.all( createdQuestions.map((node) => questionForks.get(`${state.canvasId}\0${node.data.threadId}`), @@ -1766,6 +1768,7 @@ const useCanvasStore = create()( const dirty = new Set(nodeContentQueue.pendingNodeIds()); const skippedNodeIds: string[] = []; const skippedRemoteNodes: Node[] = []; + const advancedRemoteBaselines: Node[] = []; const preservedPendingNodeIds = new Set(); const localNodesById = new Map( get().nodes.map((node) => [node.id, node]), @@ -1777,18 +1780,51 @@ const useCanvasStore = create()( if (d.type === 'REPLACE_NODE' && dirty.has(d.next.id)) { const prevData = (d.prev.data ?? {}) as Record; const nextData = (d.next.data ?? {}) as Record; + const pendingFields = nodeContentQueue.pendingFields(d.next.id); // JSON transport gives unchanged keywords/provenance separate // object identities; compare their values, not references. - const changesPendingContent = [...NODE_CONTENT_KEYS].some( + const changedContentFields = [...NODE_CONTENT_KEYS].filter( (key) => !deepEqual(prevData[key], nextData[key]), ); + const changesPendingContent = changedContentFields.some((key) => + pendingFields.has(key), + ); + const local = localNodesById.get(d.next.id); + if (local?.type === 'question') { + const localData = (local.data ?? {}) as Record< + string, + unknown + >; + const mergedData = { ...nextData }; + for (const key of pendingFields) { + if (key in localData) mergedData[key] = localData[key]; + else delete mergedData[key]; + } + preservedPendingNodeIds.add(d.next.id); + if (changesPendingContent) { + skippedNodeIds.push(d.next.id); + skippedRemoteNodes.push(d.next as unknown as Node); + } else if ( + changedContentFields.some( + (key) => key === 'content' || key === 'src', + ) + ) { + advancedRemoteBaselines.push(d.next as unknown as Node); + } + return [ + { + ...d, + prev: local, + next: { ...local, data: mergedData }, + }, + ]; + } if (!changesPendingContent) { // Coarse REPLACE_NODE deltas carry the server's full node, // including stale content fields. A lifecycle-only update // (for example Question running/done/viewed) is safe to // apply while preserving every locally pending content // field; otherwise applying `next` would erase the edit. - const local = localNodesById.get(d.next.id); if (local) { const localData = (local.data ?? {}) as Record< string, @@ -1811,20 +1847,6 @@ const useCanvasStore = create()( } skippedNodeIds.push(d.next.id); skippedRemoteNodes.push(d.next as unknown as Node); - const local = localNodesById.get(d.next.id); - if (local?.type === 'question') { - preservedPendingNodeIds.add(d.next.id); - return [ - { - ...d, - prev: local, - next: { - ...local, - data: preserveAgentNodeOwnedData(local.data, nextData), - }, - }, - ]; - } return []; } if (d.type === 'DELETE_NODE' && dirty.has(d.node.id)) { @@ -1842,8 +1864,12 @@ const useCanvasStore = create()( // change the local-first policy already decided the user wins. The // "your version was kept" notice tells the user what happened; the // change-review card lets them inspect the agent's dropped edit. - if (skippedRemoteNodes.length > 0) { - nodeContentQueue.seedBaselines(skippedRemoteNodes); + const rebasedRemoteNodes = [ + ...skippedRemoteNodes, + ...advancedRemoteBaselines, + ]; + if (rebasedRemoteNodes.length > 0) { + nodeContentQueue.seedBaselines(rebasedRemoteNodes); } if (safeDeltas.length === 0) { diff --git a/apps/web/src/store/canvasStore/save/__tests__/nodeContentQueue.baseline.test.ts b/apps/web/src/store/canvasStore/save/__tests__/nodeContentQueue.baseline.test.ts index ccac0ae16..0dbdfe3d2 100644 --- a/apps/web/src/store/canvasStore/save/__tests__/nodeContentQueue.baseline.test.ts +++ b/apps/web/src/store/canvasStore/save/__tests__/nodeContentQueue.baseline.test.ts @@ -49,7 +49,19 @@ function noteNode(content: string, label = 'Note'): Node { } as Node; } -function makeQueue(node: Node) { +function questionNode(content: string, label = 'Question 1'): Node { + return { + id: 'q1', + type: 'question', + position: { x: 0, y: 0 }, + data: { content, label, threadId: 'thread-1' }, + } as Node; +} + +function makeQueue( + node: Node, + options?: { waitForCreation?: () => Promise }, +) { const state = { canvasId: 'c1', nodes: [node] as Node[], @@ -59,6 +71,9 @@ function makeQueue(node: Node) { const queue = createNodeContentQueue({ delayMs: 0, getState: () => state, + waitForCreation: options?.waitForCreation + ? () => options.waitForCreation!() + : undefined, }); return { queue, state }; } @@ -70,6 +85,141 @@ beforeEach(() => { }); describe('nodeContentQueue baseline lifecycle', () => { + it('suppresses the redundant Question creation PUT', async () => { + const node = questionNode(''); + const { queue } = makeQueue(node); + + queue.prepareServerCreatedNodes([node]); + queue.scheduleChanges('c1', [], [node]); + await queue.flushAll(); + + expect(putMock).not.toHaveBeenCalled(); + expect(queue.pendingNodeIds()).toEqual([]); + }); + + it('waits for Question creation before persisting a real pre-acknowledgement edit', async () => { + let acknowledge!: () => void; + const creation = new Promise((resolve) => { + acknowledge = resolve; + }); + const node = questionNode(''); + const { queue, state } = makeQueue(node, { + waitForCreation: () => creation, + }); + queue.prepareServerCreatedNodes([node]); + queue.scheduleChanges('c1', [], [node]); + const edited = questionNode('First prompt'); + state.nodes = [edited]; + queue.scheduleChanges('c1', [node], [edited]); + putMock.mockResolvedValue({ + nodeId: 'q1', + label: 'Question 1', + rev: 'FIRST', + }); + + const flushing = queue.flushAll(); + await Promise.resolve(); + expect(putMock).not.toHaveBeenCalled(); + acknowledge(); + await flushing; + + expect(putMock).toHaveBeenCalledOnce(); + expect(putMock.mock.calls[0][2]).toMatchObject({ + content: 'First prompt', + expectRev: nodeRevisionOf({ content: '' }), + }); + }); + + it('sends only locally dirty Question fields', async () => { + const node = questionNode(''); + const { queue, state } = makeQueue(node); + queue.seedBaselines([node]); + const edited = questionNode('Pending prompt'); + state.nodes = [edited]; + queue.scheduleChanges('c1', [node], [edited]); + putMock.mockResolvedValue({ + nodeId: 'q1', + label: 'Generated remotely', + rev: 'BODY', + }); + + await queue.flushAll(); + + expect(putMock.mock.calls[0][2]).toEqual({ + nodeType: 'question', + content: 'Pending prompt', + expectRev: nodeRevisionOf({ content: '' }), + }); + }); + + it('ignores an obsolete success after a newer authoritative baseline and retries', async () => { + let finishFirst!: (value: unknown) => void; + const first = new Promise((resolve) => { + finishFirst = resolve; + }); + const node = questionNode(''); + const { queue, state } = makeQueue(node); + queue.seedBaselines([node]); + const edited = questionNode('Mine'); + state.nodes = [edited]; + queue.scheduleChanges('c1', [node], [edited]); + putMock + .mockReturnValueOnce(first) + .mockResolvedValueOnce({ nodeId: 'q1', label: 'Remote', rev: 'MINE' }); + + const flushing = queue.flushAll(); + await vi.waitFor(() => expect(putMock).toHaveBeenCalledOnce()); + queue.seedBaselines([questionNode('Remote prompt', 'Remote')]); + finishFirst({ nodeId: 'q1', label: 'Question 1', rev: 'OLD' }); + await flushing; + await queue.flushAll(); + + expect(putMock).toHaveBeenCalledTimes(2); + expect(putMock.mock.calls[1][2].expectRev).toBe( + nodeRevisionOf({ content: 'Remote prompt' }), + ); + expect(state._setStateNoAutosave).toHaveBeenCalledWith({ + nodes: [ + expect.objectContaining({ + data: expect.objectContaining({ label: 'Remote' }), + }), + ], + }); + }); + + it('does not surface an obsolete conflict after a newer authoritative baseline', async () => { + let failFirst!: (error: unknown) => void; + const first = new Promise((_resolve, reject) => { + failFirst = reject; + }); + const node = questionNode(''); + const { queue, state } = makeQueue(node); + queue.seedBaselines([node]); + const edited = questionNode('Mine'); + state.nodes = [edited]; + queue.scheduleChanges('c1', [node], [edited]); + putMock + .mockReturnValueOnce(first) + .mockResolvedValueOnce({ nodeId: 'q1', label: 'Remote', rev: 'MINE' }); + + const flushing = queue.flushAll(); + await vi.waitFor(() => expect(putMock).toHaveBeenCalledOnce()); + queue.seedBaselines([questionNode('Remote prompt', 'Remote')]); + failFirst( + new CanvasConflictError({ + code: 'NODE_CONTENT_CONFLICT', + message: 'changed elsewhere', + nodeId: 'q1', + currentRev: 'REMOTE', + }), + ); + await flushing; + await queue.flushAll(); + + expect(toastMock).not.toHaveBeenCalled(); + expect(putMock).toHaveBeenCalledTimes(2); + }); + it('forgetting a removed restore clears both barriers before an obsolete acknowledgement', async () => { const node = noteNode('restored'); const { queue, state } = makeQueue(node); @@ -217,9 +367,7 @@ describe('nodeContentQueue baseline lifecycle', () => { }; putMock.mockResolvedValueOnce({ nodeId: 'n1', label: 'Note', rev: 'SRV9' }); toastOpts.secondaryAction.onClick(); - // Let the forced flush settle. - await Promise.resolve(); - await Promise.resolve(); + await vi.waitFor(() => expect(putMock).toHaveBeenCalledTimes(2)); // The forced write carries the disk rev we collided with, so it // deliberately overwrites the other change. diff --git a/apps/web/src/store/canvasStore/save/nodeContentQueue.ts b/apps/web/src/store/canvasStore/save/nodeContentQueue.ts index a2f83cfbb..ceea33b33 100644 --- a/apps/web/src/store/canvasStore/save/nodeContentQueue.ts +++ b/apps/web/src/store/canvasStore/save/nodeContentQueue.ts @@ -42,7 +42,10 @@ import { } from './nodeContentFields'; import { createPerKeyDebouncer } from './perKeyDebouncer'; -import type { PutNodeContentRequest } from '@huabu/shared'; +import type { + PutNodeContentRequest, + PutNodeContentResponse, +} from '@huabu/shared'; import type { Node } from '@xyflow/react'; /** @@ -80,6 +83,12 @@ export type NodeContentQueueState = { * Public shape returned by {@link createNodeContentQueue}. */ export type NodeContentQueue = { + /** + * Seed Questions whose CREATE_NODES command is being persisted by the + * server, and suppress the redundant "new node" sidecar PUT. A real edit + * after the optimistic insertion still schedules normally. + */ + prepareServerCreatedNodes(nodes: readonly Node[]): void; /** Hold resurrected sidecars until the matching topology is acknowledged. */ holdRestoredNodes(canvasId: string, nodes: readonly Node[]): void; restoreTokens(): ReadonlyMap; @@ -177,6 +186,7 @@ export type NodeContentQueue = { * user is mid-editing from an incoming agent write. */ pendingNodeIds(): string[]; + pendingFields(nodeId: string): ReadonlySet; /** * Seed (or refresh) the optimistic-concurrency baseline revision for @@ -202,6 +212,7 @@ export type NodeContentQueue = { export function createNodeContentQueue(opts: { delayMs: number; getState: () => NodeContentQueueState; + waitForCreation?: (canvasId: string, nodeId: string) => Promise; retryStructure?: () => Promise; onRestoredContentPersisted?: (canvasId: string, nodeId: string) => void; }): NodeContentQueue { @@ -211,6 +222,8 @@ export function createNodeContentQueue(opts: { const restoredContent = new Map(); const generations = new Map(); const failures = new Map(); + const pendingFields = new Map>(); + const suppressNextNewNodeSave = new Set(); /** * Last `(label, labelSource)` the server confirmed it persisted for * each nodeId. Used by {@link handleSaveFailure} to revert an @@ -236,6 +249,7 @@ export function createNodeContentQueue(opts: { * "I believe this is a fresh create"). */ const baselineRev = new Map(); + const baselineGeneration = new Map(); /** * Node ids for which the persistent `NODE_CONTENT_CONFLICT` toast is @@ -285,7 +299,10 @@ export function createNodeContentQueue(opts: { * away (e.g. deleted between debounce-schedule and flush) or its * type is not markdown-backed. */ - function buildRequest(nodeId: string): PutNodeContentRequest | null { + function buildRequest( + nodeId: string, + requestFields: ReadonlySet | undefined, + ): PutNodeContentRequest | null { const node = opts.getState().nodes.find((n) => n.id === nodeId); if (!node) return null; const nodeType = typeof node.type === 'string' ? node.type : ''; @@ -298,41 +315,51 @@ export function createNodeContentQueue(opts: { const data = (node.data ?? {}) as Record; if (data['contentMissing'] === true) return null; + const partialQuestionWrite = + nodeType === 'question' && !restoredContent.has(nodeId); + if (partialQuestionWrite && !requestFields?.size) return null; + const includes = (key: string) => + !partialQuestionWrite || requestFields?.has(key) === true; const body: PutNodeContentRequest = { nodeType }; - if (TEXT_BEARING_NODE_TYPES.has(nodeType)) { + if (TEXT_BEARING_NODE_TYPES.has(nodeType) && includes('content')) { const content = data['content']; if (typeof content === 'string') body.content = content; } const label = data['label']; - if (typeof label === 'string') body.label = label; - else if (label === null) body.label = null; + if (includes('label')) { + if (typeof label === 'string') body.label = label; + else if (label === null) body.label = null; + } const labelSource = data['labelSource']; if ( - labelSource === 'user' || - labelSource === 'auto' || - labelSource === 'agent' + includes('labelSource') && + (labelSource === 'user' || + labelSource === 'auto' || + labelSource === 'agent') ) { body.labelSource = labelSource; } const src = data['src']; - if (typeof src === 'string') body.src = src; + if (includes('src') && typeof src === 'string') body.src = src; const summary = data['summary']; - if (typeof summary === 'string') body.summary = summary; + if (includes('summary') && typeof summary === 'string') + body.summary = summary; const keywords = data['keywords']; if ( + includes('keywords') && Array.isArray(keywords) && keywords.every((k) => typeof k === 'string') ) { body.keywords = keywords as string[]; } - if ('provenance' in data) { + if (includes('provenance') && 'provenance' in data) { body.provenance = data['provenance']; } @@ -367,68 +394,100 @@ export function createNodeContentQueue(opts: { ): Promise { if (opts.getState().canvasId !== canvasId) return; const generation = generations.get(nodeId); - const body = buildRequest(nodeId); - if (!body) return; - const response = await putNodeContent(canvasId, nodeId, body, kOpts); + await opts.waitForCreation?.(canvasId, nodeId); if ( opts.getState().canvasId !== canvasId || - generations.get(nodeId) !== generation || - !opts.getState().nodes.some((node) => node.id === nodeId) + generations.get(nodeId) !== generation ) return; - // Content and its baseline update together: record the rev the server - // actually persisted so the next edit's `expectRev` is fresh (and a - // rapid follow-up edit doesn't 409 against our own just-committed - // write). Also clear any content-conflict toast guard — a success - // means the node is no longer blocked. - baselineRev.set(nodeId, response.rev); - const completedRestore = restoredContent.delete(nodeId); - failures.delete(nodeId); - contentConflictToasted.delete(nodeId); - saveErrorToasted.delete(nodeId); - // A write that succeeded means any prior duplicate has been - // resolved — drop the once-per-node toast guard so a future - // recurrence alerts again, and clear the node's duplicate banner - // (it was only set transiently by `notifyDuplicate`, never - // persisted) so editing re-enables without a reload. - if (duplicateToasted.delete(nodeId)) { - opts.getState().patchNodeSilent(nodeId, { - contentDuplicate: false, - duplicateFiles: [], - }); - } - // Record the label the server actually persisted so a later - // failure can revert to it. Capture `labelSource` from the body - // we just sent — it's the provenance attached to that label - // server-side. - lastSuccessful.set(nodeId, { - label: response.label, - labelSource: - typeof body.labelSource === 'string' ? body.labelSource : undefined, - }); - // Only patch when the resolved label actually differs from what's - // in the store right now — avoids spurious re-renders when the - // server echoes back exactly what we sent. - const state = opts.getState(); - const currentNode = state.nodes.find((n) => n.id === nodeId); - if (!currentNode) return; - const currentLabel = - typeof currentNode.data?.['label'] === 'string' - ? (currentNode.data['label'] as string) - : null; - if (response.label !== null && response.label !== currentLabel) { - state._setStateNoAutosave({ - nodes: state.nodes.map((n) => - n.id === nodeId - ? { - ...n, - data: { ...(n.data ?? {}), label: response.label }, - } - : n, - ), + while (true) { + const requested = new Map(pendingFields.get(nodeId)); + const body = buildRequest(nodeId, new Set(requested.keys())); + if (!body) return; + const baselineAtRequest = baselineGeneration.get(nodeId); + let response: PutNodeContentResponse; + try { + response = await putNodeContent(canvasId, nodeId, body, kOpts); + } catch (error) { + if ( + error instanceof CanvasConflictError && + error.code === 'NODE_CONTENT_CONFLICT' && + baselineGeneration.get(nodeId) !== baselineAtRequest + ) { + continue; + } + throw error; + } + if ( + opts.getState().canvasId !== canvasId || + generations.get(nodeId) !== generation || + !opts.getState().nodes.some((node) => node.id === nodeId) + ) + return; + if (baselineGeneration.get(nodeId) !== baselineAtRequest) continue; + + // Content and its baseline update together: record the rev the server + // actually persisted so the next edit's `expectRev` is fresh (and a + // rapid follow-up edit doesn't 409 against our own just-committed + // write). Also clear any content-conflict toast guard — a success + // means the node is no longer blocked. + baselineRev.set(nodeId, response.rev); + baselineGeneration.set(nodeId, {}); + const livePending = pendingFields.get(nodeId); + for (const [key, token] of requested) { + if (livePending?.get(key) === token) livePending.delete(key); + } + if (livePending?.size === 0) pendingFields.delete(nodeId); + const completedRestore = restoredContent.delete(nodeId); + failures.delete(nodeId); + contentConflictToasted.delete(nodeId); + saveErrorToasted.delete(nodeId); + // A write that succeeded means any prior duplicate has been + // resolved — drop the once-per-node toast guard so a future + // recurrence alerts again, and clear the node's duplicate banner + // (it was only set transiently by `notifyDuplicate`, never + // persisted) so editing re-enables without a reload. + if (duplicateToasted.delete(nodeId)) { + opts.getState().patchNodeSilent(nodeId, { + contentDuplicate: false, + duplicateFiles: [], + }); + } + // Record the label the server actually persisted so a later + // failure can revert to it. Capture `labelSource` from the body + // we just sent — it's the provenance attached to that label + // server-side. + lastSuccessful.set(nodeId, { + label: response.label, + labelSource: + typeof body.labelSource === 'string' ? body.labelSource : undefined, }); + // Only patch when the resolved label actually differs from what's + // in the store right now — avoids spurious re-renders when the + // server echoes back exactly what we sent. + const state = opts.getState(); + const currentNode = state.nodes.find((n) => n.id === nodeId); + if (!currentNode) return; + const currentLabel = + typeof currentNode.data?.['label'] === 'string' + ? (currentNode.data['label'] as string) + : null; + if (response.label !== null && response.label !== currentLabel) { + state._setStateNoAutosave({ + nodes: state.nodes.map((n) => + n.id === nodeId + ? { + ...n, + data: { ...(n.data ?? {}), label: response.label }, + } + : n, + ), + }); + } + if (pendingFields.get(nodeId)?.size) schedule(canvasId, nodeId); + if (completedRestore) opts.onRestoredContentPersisted?.(canvasId, nodeId); + return; } - if (completedRestore) opts.onRestoredContentPersisted?.(canvasId, nodeId); } /** @@ -843,6 +902,15 @@ export function createNodeContentQueue(opts: { } return { + prepareServerCreatedNodes(nodes) { + for (const node of nodes) { + if (node.type !== 'question') continue; + suppressNextNewNodeSave.add(node.id); + baselineRev.set(node.id, revOfNode(node)); + baselineGeneration.set(node.id, {}); + } + }, + holdRestoredNodes(canvasId, nodes) { for (const node of nodes) { if (!MD_BACKED_NODE_TYPES.has(node.type ?? '')) continue; @@ -924,7 +992,12 @@ export function createNodeContentQueue(opts: { if (!MD_BACKED_NODE_TYPES.has(nodeType)) continue; const before = prevById.get(next.id); if (!before) { + if (suppressNextNewNodeSave.delete(next.id)) continue; // Brand new node — its `.md` does not exist yet. + pendingFields.set( + next.id, + new Map([...NODE_CONTENT_KEYS].map((key) => [key, {}])), + ); schedule(canvasId, next.id); continue; } @@ -933,8 +1006,11 @@ export function createNodeContentQueue(opts: { const afterData = (next.data ?? {}) as Record; for (const key of NODE_CONTENT_KEYS) { if (beforeData[key] !== afterData[key]) { + const fields = + pendingFields.get(next.id) ?? new Map(); + fields.set(key, {}); + pendingFields.set(next.id, fields); schedule(canvasId, next.id); - break; } } } @@ -1015,6 +1091,9 @@ export function createNodeContentQueue(opts: { failures.delete(nodeId); debouncer.cancel(nodeId); baselineRev.delete(nodeId); + baselineGeneration.delete(nodeId); + pendingFields.delete(nodeId); + suppressNextNewNodeSave.delete(nodeId); frozen.delete(nodeId); contentConflictToasted.delete(nodeId); saveErrorToasted.delete(nodeId); @@ -1027,6 +1106,7 @@ export function createNodeContentQueue(opts: { const nodeType = typeof node.type === 'string' ? node.type : ''; if (!MD_BACKED_NODE_TYPES.has(nodeType)) continue; baselineRev.set(node.id, revOfNode(node)); + baselineGeneration.set(node.id, {}); failures.delete(node.id); // A fresh authoritative baseline means any prior conflict for this // node is resolved — drop the toast guard and unfreeze it so a @@ -1043,11 +1123,17 @@ export function createNodeContentQueue(opts: { new Set([ ...debouncer.pendingKeys(), ...inflight.keys(), + ...pendingFields.keys(), ...[...restoredContent] .filter(([, canvasId]) => canvasId === opts.getState().canvasId) .map(([id]) => id), ]), ); }, + + pendingFields(nodeId) { + if (restoredContent.has(nodeId)) return new Set(NODE_CONTENT_KEYS); + return new Set(pendingFields.get(nodeId)?.keys()); + }, }; } diff --git a/docs/architecture/canvas-realtime-sync.md b/docs/architecture/canvas-realtime-sync.md index e2ff47e43..9adda66ee 100644 --- a/docs/architecture/canvas-realtime-sync.md +++ b/docs/architecture/canvas-realtime-sync.md @@ -90,20 +90,18 @@ batch — there is no per-caller broadcast flag. This unifies all writers: Loading a Space may attempt to persist a legacy Hug Frame header adjustment before displaying the adjusted geometry. This optional layout write is not a prerequisite for reading the Space: if it fails (including version conflicts or Agent execution-record validation), loading completes with the original loaded geometry, content, edges, and snapshot version. The failure is logged without a blocking or persistent toast, and neither the rejected geometry nor its warmup hints are automatically resubmitted by the load path. Normal save validation and SSE reconciliation remain in force; this fallback does not repair missing execution records or force an overwrite. -`applyDeltasFromAgent` filters incoming deltas against the set of nodes with -un-persisted local content edits (`nodeContentQueue.pendingNodeIds()` — -debounced-but-unsaved plus in-flight PUTs): +`applyDeltasFromAgent` filters incoming deltas against nodes with un-persisted local content edits (`nodeContentQueue.pendingNodeIds()` — debounced-but-unsaved plus in-flight PUTs). The queue also exposes the exact pending sidecar fields for Question reconciliation: ``` INSERT_NODE (new id) → apply unless a local pending Question creation was undone/deleted -REPLACE_NODE changing content / DELETE_NODE on a dirty id → SKIP (keep the human's unsaved edit) +REPLACE_NODE changing a pending Question field → keep that local field, apply independent remote fields, report the overlap +REPLACE_NODE changing content on another dirty node / DELETE_NODE on a dirty id → SKIP (keep the human's unsaved edit) REPLACE_NODE changing only non-content fields on a dirty id → apply while preserving local content fields otherwise → apply ``` -- Resolution is deterministic **local-first**: a skipped node keeps the human's - value, and its post-effects (preprocessing / fit) are skipped too. `version` - still advances to `toVersion` so the next autosave doesn't 409. +- Resolution is deterministic **local-first**: a skipped field or node keeps the human's value, and skipped node post-effects (preprocessing / fit) are skipped too. `version` still advances to `toVersion` so the next autosave does not 409. +- Question sidecar reconciliation is field-aware. A pending body edit no longer blocks an independent generated label, and a pending manual label no longer blocks the server-seeded first prompt or lifecycle metadata. The eventual Question PUT contains only locally pending fields, so it cannot write an old label back while saving a body edit. Same-field overlap remains local-first, is reported through the existing amber skipped-write surface, and adopts the remote authored-content revision before retrying the local value. - Coarse `REPLACE_NODE` deltas include the server's full node even when only a lifecycle field such as Question `status` or `viewed` changed. Markdown-sidecar fields are compared by value, including nested `keywords` and `provenance` metadata, rather than object identity or object property order after JSON transport. When the remote delta leaves every markdown-sidecar field unchanged, the applier overlays the locally pending content fields onto the incoming node and applies the remaining update. This avoids treating the initiating tab's own lifecycle broadcast as an edit conflict without allowing stale server content to overwrite the local edit. - **Baseline rebase (no false conflict):** for a skipped `REPLACE_NODE`, the applier adopts the agent's just-written revision as that node's content-CAS @@ -120,7 +118,7 @@ otherwise → apply - Structure-save acknowledgements reconcile monotonically with Canvas Sync: a delayed HTTP success cannot lower the local version, and a delayed 409 whose reported server version has already arrived over SSE is retried against that fresh baseline instead of opening the global conflict state. If the 409 arrives first, the client records its server version; a later SSE update that reaches that version clears the warning and schedules the latest structure for retry. - **Scope:** content only. Same-node _structure_ conflicts (geometry / parent) stay coarse — there is no per-node structure-dirty tracking yet. -- **Mixed Question content/FSM deltas:** when a dirty node rejects incoming authored content, its authoritative binding, invocation, attention, and association metadata still applies. The local content, existing amber conflict notice, skipped change-card state, and content-CAS baseline rebase remain intact. Geometry and deletion conflict policy is unchanged; this is not general field-level merging. +- **Mixed Question content/FSM deltas:** when a pending field rejects an overlapping remote authored value, authoritative binding, invocation, attention, association metadata, and independent sidecar fields still apply. The local overlapping value, existing amber conflict notice, skipped change-card state, and content-CAS baseline rebase remain intact. Geometry and deletion conflict policy is unchanged; this field-level exception is specific to Question sidecar reconciliation, not general structure merging. Skipped ids flow back through `canvasSyncStore` into `acpThreadChangesStore` (`conflictedByThread`) and surface as: diff --git a/docs/architecture/canvas-storage.md b/docs/architecture/canvas-storage.md index bfbd385b6..cfd67e949 100644 --- a/docs/architecture/canvas-storage.md +++ b/docs/architecture/canvas-storage.md @@ -248,12 +248,9 @@ Web undo/redo resurrection must acknowledge the restored topology before writing The content queue's generation-checked restored-content success callback notifies the preprocessing queue that waiting work may proceed. Structure acknowledgement alone is not sufficient: preprocessing remains blocked during the restored content PUT and after a failed write until Retry succeeds. The preprocessing queue owns whether interrupted or newly requested work is pending; storage does not decide this or automatically reprocess completed nodes. See the authoritative [client responsibility boundary](./node-preprocessing.md#client-responsibility-boundary). -Standalone content PUT and preprocessing persist call -`updateNode(SpaceNodes, ...)`. The Canvas-domain promise-chain mutex remains -per Space and stays held across the repository's asynchronous read and CAS put. -The caller still owns field-merging policy; authored-content revisions remain -`nodeRevisionOf({ content, src })`, and a stale baseline still maps to the same -`NODE_CONTENT_CONFLICT` response. +For optimistic Question creation, the canonical create command writes the initial sidecar and the web content queue suppresses its generic new-node PUT. A genuine edit made before acknowledgement waits for the tracked create command rather than assuming the sidecar is absent. Question content requests may contain only the locally dirty sidecar fields; the route preserves omitted fields from the existing record, while authored-content CAS remains based only on `nodeRevisionOf({ content, src })`. The web fences PUT results with its authoritative-baseline generation: if Canvas Sync seeds a newer revision while a request is in flight, the older success cannot regress the baseline or label and an older conflict is retried instead of surfacing a false `NODE_CONTENT_CONFLICT`. + +Standalone content PUT and preprocessing persist call `updateNode(SpaceNodes, ...)`. The Canvas-domain promise-chain mutex remains per Space and stays held across the repository's asynchronous read and CAS put. The caller still owns field-merging policy; authored-content revisions remain `nodeRevisionOf({ content, src })`, and a genuinely stale baseline still maps to the same `NODE_CONTENT_CONFLICT` response. Executor and revert batches already hold that mutex for their whole command batch, so they make one `SpaceHandle.write(...)` call instead of diff --git a/docs/architecture/question-node.md b/docs/architecture/question-node.md index 7e4f81a84..2961e7848 100644 --- a/docs/architecture/question-node.md +++ b/docs/architecture/question-node.md @@ -103,10 +103,9 @@ The single-node toolbar exposes **Scale** as an absolute percentage of the defau ## 4. As a content node -The question text lives in the markdown sidecar, so agents read it like any other -text node — `read("nodes/.md")` returns the body, and `get_space_outline` -lists it as `type: 'question'`. No trigger needed; it is just canvas content that -also happens to own a thread. +The question text lives in the markdown sidecar, so agents read it like any other text node — `read("nodes/.md")` returns the body, and `get_space_outline` lists it as `type: 'question'`. No trigger is needed; it is Canvas content that also owns a thread. + +Canonical Question creation owns the initial sidecar write. The web marks the optimistic node before inserting it into local state, so the generic content queue suppresses its otherwise redundant new-node PUT for both create-only and create-and-start flows. A real edit made before the create command acknowledges remains pending and waits for that same creation promise before writing; subsequent Canvas Sync content seeds the current sidecar revision, so the edit uses the authoritative baseline rather than the absent-file revision. This ordering prevents the optimistic creation path from competing with its own server command or overwriting the first prompt. The sidecar body holds only the **first** user prompt, so canvas search (Cmd+F) also has a dedicated **`conversation`** tier