fix(coding): keep optimistic identity across snapshots
This commit is contained in:
@@ -107,7 +107,10 @@
|
|||||||
and replayed in generation/sequence order after a valid Snapshot; unresolved
|
and replayed in generation/sequence order after a valid Snapshot; unresolved
|
||||||
pending/accepted/uncertain prompt requests re-overlay their local optimistic
|
pending/accepted/uncertain prompt requests re-overlay their local optimistic
|
||||||
nodes after snapshot-first reconnect until a durable `clientRequestId`
|
nodes after snapshot-first reconnect until a durable `clientRequestId`
|
||||||
reconciliation arrives. Neither mechanism replays a mutation.
|
reconciliation arrives. If the reconnect Snapshot already contains that
|
||||||
|
durable message, its message id is first reconciled to the local request's
|
||||||
|
stable UI node id before the request is removed. Neither mechanism replays a
|
||||||
|
mutation.
|
||||||
- Added optimistic user nodes keyed by `clientRequestId`. Durable upsert keeps
|
- Added optimistic user nodes keyed by `clientRequestId`. Durable upsert keeps
|
||||||
the optimistic UI node id; definite rejection restores an untouched draft
|
the optimistic UI node id; definite rejection restores an untouched draft
|
||||||
and marks the node failed; uncertain delivery restores the draft while
|
and marks the node failed; uncertain delivery restores the draft while
|
||||||
@@ -129,11 +132,16 @@
|
|||||||
|
|
||||||
- `pnpm exec vitest run tests/unit/coding-conversations-facade.test.ts tests/unit/coding-conversations-store.test.tsx`:
|
- `pnpm exec vitest run tests/unit/coding-conversations-facade.test.ts tests/unit/coding-conversations-store.test.tsx`:
|
||||||
initial implementation 2 files / 12 tests passed; after review fixes, 2 files
|
initial implementation 2 files / 12 tests passed; after review fixes, 2 files
|
||||||
/ 13 tests passed, including delayed gap GET + concurrent patch replay and
|
/ 14 tests passed, including delayed gap GET + concurrent patch replay,
|
||||||
reconnect Snapshot optimistic-node continuity.
|
reconnect Snapshot optimistic-node continuity, and a reconnect Snapshot that
|
||||||
|
already contains the durable user message while preserving `node-1`.
|
||||||
- `pnpm exec vitest run` for the two PI-110 tests plus PI-010 contracts,
|
- `pnpm exec vitest run` for the two PI-110 tests plus PI-010 contracts,
|
||||||
coding core routes, event projector, Conversation runtime, product tools,
|
coding core routes, event projector, Conversation runtime, product tools,
|
||||||
and subagent suites: 8 files / 75 tests passed.
|
and subagent suites: initial 8 files / 75 tests; final review-fix range 8
|
||||||
|
files / 77 tests passed. One intervening parallel run hit a non-reproducible
|
||||||
|
`CODING_STORAGE_WRITE_FAILED` in the unrelated Conversation runtime temp
|
||||||
|
persistence test; that file passed alone immediately afterward and the same
|
||||||
|
8-file command then passed in full.
|
||||||
- `pnpm run typecheck`: passed after the shared product seam removed the clean
|
- `pnpm run typecheck`: passed after the shared product seam removed the clean
|
||||||
Renderer -> composite Electron declaration dependency.
|
Renderer -> composite Electron declaration dependency.
|
||||||
- `pnpm run lint:check`: passed with zero errors and the same six unrelated
|
- `pnpm run lint:check`: passed with zero errors and the same six unrelated
|
||||||
@@ -150,9 +158,12 @@
|
|||||||
PI-150. Neither is recorded as Pass.
|
PI-150. Neither is recorded as Pass.
|
||||||
- Planner review of `a072257...36626c8`: Standards PASS / 0 findings; Spec
|
- Planner review of `a072257...36626c8`: Standards PASS / 0 findings; Spec
|
||||||
Needs Fix with two P1 findings for recovery-window patch loss and reconnect
|
Needs Fix with two P1 findings for recovery-window patch loss and reconnect
|
||||||
Snapshot optimistic-node loss. Both independent reproductions were accepted
|
Snapshot optimistic-node loss. The first fixed-range re-review accepted gap
|
||||||
and fixed locally with the focused regressions above; fixed-range re-review
|
replay and missing-durable-node overlay, then found one remaining P1 where a
|
||||||
remains required before PI-110 can be marked Done.
|
Snapshot already containing the durable node adopted the server id. That
|
||||||
|
reproduction is now fixed by `clientRequestId` id reconciliation before
|
||||||
|
request cleanup, with the focused regression above. Final fixed-range
|
||||||
|
re-review remains required before PI-110 can be marked Done.
|
||||||
|
|
||||||
## Follow-ups
|
## Follow-ups
|
||||||
|
|
||||||
|
|||||||
@@ -160,6 +160,22 @@ function withoutReconciledRequests(
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function withReconciledOptimisticNodeIds(
|
||||||
|
snapshot: ConversationSnapshot,
|
||||||
|
requests: Record<string, CodingPromptRequestState> | undefined,
|
||||||
|
): ConversationSnapshot {
|
||||||
|
if (!requests) return snapshot;
|
||||||
|
let changed = false;
|
||||||
|
const nodes = snapshot.nodes.map((node) => {
|
||||||
|
if (node.kind !== 'message' || !node.clientRequestId) return node;
|
||||||
|
const nodeId = requests[node.clientRequestId]?.nodeId;
|
||||||
|
if (!nodeId || node.id === nodeId) return node;
|
||||||
|
changed = true;
|
||||||
|
return { ...node, id: nodeId };
|
||||||
|
});
|
||||||
|
return changed ? { ...snapshot, nodes } : snapshot;
|
||||||
|
}
|
||||||
|
|
||||||
function optimisticNode(
|
function optimisticNode(
|
||||||
nodeId: string,
|
nodeId: string,
|
||||||
clientRequestId: string,
|
clientRequestId: string,
|
||||||
@@ -601,12 +617,16 @@ export function createCodingConversationStore(
|
|||||||
if (!current.reducer.invalidation && snapshotIsOlder(current.reducer.snapshot, snapshot)) {
|
if (!current.reducer.invalidation && snapshotIsOlder(current.reducer.snapshot, snapshot)) {
|
||||||
return state;
|
return state;
|
||||||
}
|
}
|
||||||
|
const reconciledSnapshot = withReconciledOptimisticNodeIds(
|
||||||
|
snapshot,
|
||||||
|
state.requestsByConversationId[event.conversationId],
|
||||||
|
);
|
||||||
const requests = withoutReconciledRequests(
|
const requests = withoutReconciledRequests(
|
||||||
state.requestsByConversationId[event.conversationId],
|
state.requestsByConversationId[event.conversationId],
|
||||||
snapshot,
|
reconciledSnapshot,
|
||||||
);
|
);
|
||||||
const reducer = withUnreconciledOptimisticNodes(
|
const reducer = withUnreconciledOptimisticNodes(
|
||||||
replaceConversationSnapshot(current.reducer, snapshot),
|
replaceConversationSnapshot(current.reducer, reconciledSnapshot),
|
||||||
requests,
|
requests,
|
||||||
);
|
);
|
||||||
const entry: CodingConversationEntry = {
|
const entry: CodingConversationEntry = {
|
||||||
|
|||||||
@@ -337,6 +337,43 @@ describe('coding Conversation store', () => {
|
|||||||
expect(store.getState().requestsByConversationId['conversation-a']).toEqual({});
|
expect(store.getState().requestsByConversationId['conversation-a']).toEqual({});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('preserves the optimistic UI id when a reconnect snapshot is already durable', async () => {
|
||||||
|
const store = createCodingConversationStore({
|
||||||
|
getSnapshot: vi.fn(),
|
||||||
|
openEvents: vi.fn(),
|
||||||
|
submitPrompt: vi.fn(async (input) => acceptance(
|
||||||
|
input.conversationId,
|
||||||
|
input.clientRequestId,
|
||||||
|
)),
|
||||||
|
createId: ids(),
|
||||||
|
});
|
||||||
|
store.getState().applySnapshotEvent(snapshotEvent(snapshot('conversation-a')));
|
||||||
|
store.getState().setDraft('conversation-a', 'Already durable');
|
||||||
|
await store.getState().submitPrompt({ conversationId: 'conversation-a', mode: 'prompt' });
|
||||||
|
const durable = snapshot('conversation-a', 1, 1);
|
||||||
|
store.getState().applySnapshotEvent(snapshotEvent({
|
||||||
|
...durable,
|
||||||
|
nodes: [{
|
||||||
|
kind: 'message',
|
||||||
|
id: 'durable-user-1',
|
||||||
|
sourceEntryId: 'entry-user-1',
|
||||||
|
clientRequestId: 'request-1',
|
||||||
|
role: 'user',
|
||||||
|
status: 'complete',
|
||||||
|
blocks: [{ kind: 'text', id: 'durable-text-1', text: 'Already durable', status: 'complete' }],
|
||||||
|
}],
|
||||||
|
}));
|
||||||
|
|
||||||
|
expect(selectCodingConversationSnapshot('conversation-a')(store.getState())?.nodes[0])
|
||||||
|
.toMatchObject({
|
||||||
|
id: 'node-1',
|
||||||
|
sourceEntryId: 'entry-user-1',
|
||||||
|
clientRequestId: 'request-1',
|
||||||
|
status: 'complete',
|
||||||
|
});
|
||||||
|
expect(store.getState().requestsByConversationId['conversation-a']).toEqual({});
|
||||||
|
});
|
||||||
|
|
||||||
it('restores draft attachments and marks the optimistic node on definite rejection', async () => {
|
it('restores draft attachments and marks the optimistic node on definite rejection', async () => {
|
||||||
const submitPrompt = vi.fn(async () => {
|
const submitPrompt = vi.fn(async () => {
|
||||||
throw new AppError('RUNTIME', 'Model unavailable', undefined, {
|
throw new AppError('RUNTIME', 'Model unavailable', undefined, {
|
||||||
|
|||||||
Reference in New Issue
Block a user