From 1abb2fff363c6fe92ee2665f45d15ce6ae607f56 Mon Sep 17 00:00:00 2001 From: brother7 <7brother7@gmail.com> Date: Sun, 23 Aug 2026 22:27:21 +0800 Subject: [PATCH] fix(coding): preserve renderer recovery state --- .../20260823-pi-renderer-store-b7e2c4a1.md | 15 +++- src/stores/coding-conversations.ts | 73 +++++++++++++++++-- .../unit/coding-conversations-store.test.tsx | 44 +++++++++++ 3 files changed, 125 insertions(+), 7 deletions(-) diff --git a/.project-docs/30-worklog/tasks/20260823-pi-renderer-store-b7e2c4a1.md b/.project-docs/30-worklog/tasks/20260823-pi-renderer-store-b7e2c4a1.md index 2146f3e..9d6bcf8 100644 --- a/.project-docs/30-worklog/tasks/20260823-pi-renderer-store-b7e2c4a1.md +++ b/.project-docs/30-worklog/tasks/20260823-pi-renderer-store-b7e2c4a1.md @@ -102,6 +102,12 @@ discard, per-target sequence/generation-gap recovery, load single-flight, and connection-generation protection. Reconnect never replays a prompt and an obsolete connection flight cannot clear a newer one. +- Closed the planner review's two P1 recovery windows: patches arriving while + a target GET is in flight are buffered only for that invalidated Conversation + and replayed in generation/sequence order after a valid Snapshot; unresolved + pending/accepted/uncertain prompt requests re-overlay their local optimistic + nodes after snapshot-first reconnect until a durable `clientRequestId` + reconciliation arrives. Neither mechanism replays a mutation. - Added optimistic user nodes keyed by `clientRequestId`. Durable upsert keeps the optimistic UI node id; definite rejection restores an untouched draft and marks the node failed; uncertain delivery restores the draft while @@ -122,7 +128,9 @@ ## Verification - `pnpm exec vitest run tests/unit/coding-conversations-facade.test.ts tests/unit/coding-conversations-store.test.tsx`: - 2 files / 12 tests passed. + initial implementation 2 files / 12 tests passed; after review fixes, 2 files + / 13 tests passed, including delayed gap GET + concurrent patch replay and + reconnect Snapshot optimistic-node continuity. - `pnpm exec vitest run` for the two PI-110 tests plus PI-010 contracts, coding core routes, event projector, Conversation runtime, product tools, and subagent suites: 8 files / 75 tests passed. @@ -140,6 +148,11 @@ - Real Provider remains Explicitly Waived / Accepted Risk with `realTurnVerified=false`. macOS x64/arm64 remains deferred to mandatory PI-150. Neither is recorded as Pass. +- Planner review of `a072257...36626c8`: Standards PASS / 0 findings; Spec + Needs Fix with two P1 findings for recovery-window patch loss and reconnect + Snapshot optimistic-node loss. Both independent reproductions were accepted + and fixed locally with the focused regressions above; fixed-range re-review + remains required before PI-110 can be marked Done. ## Follow-ups diff --git a/src/stores/coding-conversations.ts b/src/stores/coding-conversations.ts index e35386d..7046c5e 100644 --- a/src/stores/coding-conversations.ts +++ b/src/stores/coding-conversations.ts @@ -216,6 +216,27 @@ function withRejectedNode( }); } +function withUnreconciledOptimisticNodes( + reducer: ConversationReducerState, + requests: Record, +): ConversationReducerState { + const snapshot = reducer.snapshot; + if (!snapshot) return reducer; + const existingRequestIds = requestIdsInSnapshot(snapshot); + const nodes = Object.values(requests).flatMap((request) => ( + request.nodeId + && request.status !== 'rejected' + && !existingRequestIds.has(request.clientRequestId) + ? [optimisticNode(request.nodeId, request.clientRequestId, request.submittedDraft)] + : [] + )); + if (nodes.length === 0) return reducer; + return replaceConversationSnapshot(reducer, { + ...snapshot, + nodes: [...snapshot.nodes, ...nodes], + }); +} + function snapshotIsOlder( current: ConversationSnapshot | null, incoming: ConversationSnapshot, @@ -241,11 +262,33 @@ export function createCodingConversationStore( ): StoreApi { const deps = { ...defaultDependencies(), ...dependencies }; const snapshotLoads = new Map>(); + const recoveryPatches = new Map(); let eventSource: EventSource | null = null; let connectFlight: Promise | null = null; let connectionGeneration = 0; + let store!: StoreApi; - return createStore((set, get) => ({ + const bufferRecoveryPatch = (event: CodingConversationPatchEvent) => { + const buffered = recoveryPatches.get(event.conversationId) ?? []; + if (buffered.some((candidate) => ( + candidate.workerGeneration === event.workerGeneration && candidate.seq === event.seq + ))) return; + recoveryPatches.set( + event.conversationId, + [...buffered, event].sort((left, right) => ( + left.workerGeneration - right.workerGeneration || left.seq - right.seq + )), + ); + }; + + const replayRecoveryPatches = (conversationId: string) => { + const buffered = recoveryPatches.get(conversationId); + if (!buffered?.length) return; + recoveryPatches.delete(conversationId); + for (const event of buffered) store.getState().applyPatchEvent(event); + }; + + store = createStore((set, get) => ({ selectedConversationId: null, entriesByConversationId: {}, summariesByConversationId: {}, @@ -316,6 +359,8 @@ export function createCodingConversationStore( }) .finally(() => { if (snapshotLoads.get(conversationId) === flight) snapshotLoads.delete(conversationId); + const entry = get().entriesByConversationId[conversationId]; + if (!entry?.reducer.invalidation) replayRecoveryPatches(conversationId); }); snapshotLoads.set(conversationId, flight); return flight; @@ -556,7 +601,14 @@ export function createCodingConversationStore( if (!current.reducer.invalidation && snapshotIsOlder(current.reducer.snapshot, snapshot)) { return state; } - const reducer = replaceConversationSnapshot(current.reducer, snapshot); + const requests = withoutReconciledRequests( + state.requestsByConversationId[event.conversationId], + snapshot, + ); + const reducer = withUnreconciledOptimisticNodes( + replaceConversationSnapshot(current.reducer, snapshot), + requests, + ); const entry: CodingConversationEntry = { ...current, reducer, @@ -572,17 +624,25 @@ export function createCodingConversationStore( summariesByConversationId: summariesWithEntry(state.summariesByConversationId, entry), requestsByConversationId: { ...state.requestsByConversationId, - [event.conversationId]: withoutReconciledRequests( - state.requestsByConversationId[event.conversationId], - snapshot, - ), + [event.conversationId]: requests, }, }; }); + if (!snapshotLoads.has(event.conversationId)) { + replayRecoveryPatches(event.conversationId); + } }, applyPatchEvent(event) { if (event.type !== 'patch') return; + const currentBeforePatch = get().entriesByConversationId[event.conversationId]; + if (currentBeforePatch?.reducer.invalidation) { + bufferRecoveryPatch(event); + if (!snapshotLoads.has(event.conversationId)) { + void get().loadSnapshot(event.conversationId, true).catch(() => undefined); + } + return; + } let recover = false; set((state) => { const current = state.entriesByConversationId[event.conversationId] ?? emptyEntry(); @@ -626,6 +686,7 @@ export function createCodingConversationStore( } }, })); + return store; } export const codingConversationStore = createCodingConversationStore(); diff --git a/tests/unit/coding-conversations-store.test.tsx b/tests/unit/coding-conversations-store.test.tsx index 85b4ec6..a593d64 100644 --- a/tests/unit/coding-conversations-store.test.tsx +++ b/tests/unit/coding-conversations-store.test.tsx @@ -194,6 +194,46 @@ describe('coding Conversation store', () => { expect(store.getState().entriesByConversationId['conversation-b'].reducer.invalidation).toBeNull(); }); + it('replays target patches that arrive while a gap snapshot is loading', async () => { + const recovery = deferred(); + const getSnapshot = vi.fn(() => recovery.promise); + const store = createCodingConversationStore({ + getSnapshot, + openEvents: vi.fn(), + submitPrompt: vi.fn(), + createId: ids(), + }); + store.getState().applySnapshotEvent(snapshotEvent(snapshot('conversation-a'))); + + store.getState().applyPatchEvent(patchEvent('conversation-a', 2, { + op: 'run.state', + run: { status: 'running', runId: 'run-gap' }, + })); + store.getState().applyPatchEvent(patchEvent('conversation-a', 3, { + op: 'run.state', + run: { + status: 'error', + runId: 'run-gap', + terminalReason: 'failed', + }, + })); + recovery.resolve({ + ...snapshot('conversation-a', 1, 2), + run: { status: 'running', runId: 'run-gap' }, + }); + + await waitFor(() => { + expect(selectCodingConversationSnapshot('conversation-a')(store.getState())) + .toMatchObject({ + cursor: { workerGeneration: 1, seq: 3 }, + run: { status: 'error', runId: 'run-gap', terminalReason: 'failed' }, + }); + }); + expect(getSnapshot).toHaveBeenCalledTimes(1); + expect(store.getState().entriesByConversationId['conversation-a'].reducer.invalidation) + .toBeNull(); + }); + it('uses native EventSource reconnect without reopening or replaying a mutation', async () => { const source = new FakeEventSource(); const openEvents = vi.fn(async () => source as unknown as EventSource); @@ -276,6 +316,9 @@ describe('coding Conversation store', () => { await submission; expect(store.getState().requestsByConversationId['conversation-a']['request-1'].status) .toBe('accepted'); + store.getState().applySnapshotEvent(snapshotEvent(snapshot('conversation-a'))); + expect(selectCodingConversationSnapshot('conversation-a')(store.getState())?.nodes[0]) + .toMatchObject({ id: 'node-1', clientRequestId: 'request-1', status: 'optimistic' }); store.getState().applyPatchEvent(patchEvent('conversation-a', 1, { op: 'message.upsert', node: { @@ -350,6 +393,7 @@ describe('coding Conversation store', () => { })).rejects.toThrow('Delivery is uncertain'); source.fail(); source.open(); + source.emit('snapshot', snapshotEvent(snapshot('conversation-a'))); expect(store.getState().draftsByConversationId['conversation-a'].text).toBe('Do not replay'); expect(store.getState().requestsByConversationId['conversation-a']['request-1'])