From 7db480b7bddfa7dc489ff46e01ae95342bba3668 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 4 Sep 2026 05:06:47 +0100 Subject: [PATCH 1/2] fix(review): preserve revision state after definite rejection --- .../src/composables/useProposalRevisions.ts | 25 +++++++- .../composables/useProposalRevisions.spec.ts | 42 ++++++++++++++ .../paper/review/PaperReviewView.spec.ts | 58 +++++++++++++++++++ .../src/views/paper/PaperReviewView.vue | 17 +++++- 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 055af8cb1..45e2f9c8b 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -14,12 +14,24 @@ import { export type SaveRevisionResult = { proposalId: string - /** Whether the API response confirmed persistence or left it unknown. */ - outcome: 'persisted' | 'indeterminate' + /** Whether the API response confirmed persistence, rejection, or left it unknown. */ + outcome: 'persisted' | 'rejected' | 'indeterminate' /** The active proposal/save generation still owns this continuation. */ current: boolean } +/** + * A revision endpoint 4xx is a definite non-commit: the existing API contract + * uses 4xx responses for validation, authorization, missing proposals, and + * revision conflicts. A timeout, network failure, or 5xx remains indeterminate + * because the server may have committed before the client lost the response. + */ +function isDefiniteRevisionSaveRejection(error: unknown): boolean { + if (typeof error !== 'object' || error === null) return false + const status = (error as { response?: { status?: unknown } }).response?.status + return typeof status === 'number' && status >= 400 && status < 500 +} + export function useProposalRevisions( activeProposal: Ref, /** @@ -194,6 +206,15 @@ export function useProposalRevisions( return { proposalId, outcome: 'persisted', current: true } } catch (e: unknown) { const current = gen === saveGeneration && activeProposal.value?.id === proposalId + if (isDefiniteRevisionSaveRejection(e)) { + // The API rejected this request before reporting persistence. Keep the + // authoritative metadata and retryable draft intact; unlike an + // indeterminate response, no queue read can have become stale. + if (current) { + toast.error(getErrorDisplay(e, 'Failed to save revision').message) + } + return { proposalId, outcome: 'rejected', current } + } // A rejected POST can have committed before a timeout, network break, or // 5xx reached the client. Invalidate queue reads synchronously even for a // stale continuation: a pre-write answer can restore old operations after diff --git a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts index 0bb04ef50..05b42e5e8 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -166,6 +166,48 @@ describe('useProposalRevisions', () => { expect(revisionsLoaded.value).toBe(false) }) + it('preserves known revision metadata for a definite 4xx rejection', async () => { + const knownRevision = makeRevision({ + revisedPayload: '{"title":"Existing revision"}', + }) + vi.mocked(proposalRevisionsApi.getRevisions).mockResolvedValueOnce([knownRevision]) + vi.mocked(proposalRevisionsApi.createRevision).mockRejectedValueOnce({ + response: { + status: 400, + data: { + errorCode: 'ValidationError', + message: 'Revision payload is invalid', + }, + }, + }) + const onRevisionSaved = vi.fn() + const onRevisionStateUncertain = vi.fn() + const proposal = ref(makeProposal()) + const { + editing, + revisionCount, + latestRevision, + revisionsLoaded, + startEditing, + saveRevision, + } = useProposalRevisions(proposal, { onRevisionSaved, onRevisionStateUncertain }) + + await vi.waitFor(() => expect(revisionsLoaded.value).toBe(true)) + startEditing() + + await expect( + saveRevision({ revisedPayload: '{"title":"Invalid"}', reason: 'Try invalid payload' }), + ).resolves.toEqual({ proposalId: 'p-1', outcome: 'rejected', current: true }) + + expect(onRevisionSaved).not.toHaveBeenCalled() + expect(onRevisionStateUncertain).not.toHaveBeenCalled() + expect(editing.value).toBe(true) + expect(revisionCount.value).toBe(1) + expect(latestRevision.value).toEqual(knownRevision) + expect(revisionsLoaded.value).toBe(true) + expect(toastMocks.error).toHaveBeenCalledWith('Revision payload is invalid') + }) + it('ignores a pre-save revision load that resolves after the save (no stale overwrite)', async () => { // Codex review: a getRevisions request in flight when a save lands must not // overwrite the save's state when it resolves with the pre-save (empty) list. diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index 1609cf09d..4ceb0a1ef 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -4565,6 +4565,64 @@ describe('PaperReviewView', () => { wrapper.unmount() }) + it('keeps known revision metadata and the draft after a definite 4xx rejection', async () => { + const now = new Date().toISOString() + mocks.getRevisions.mockResolvedValueOnce([ + { + id: 'rev-known-1', + proposalId: 'proposal-001', + revisionNumber: 1, + editorUserId: 'u-1', + revisedPayload: '{"headline":"Existing revision","notes":"Original notes"}', + revisedAt: now, + reason: 'First revision', + createdAt: now, + }, + ]) + mocks.createRevision.mockRejectedValueOnce({ + response: { + status: 400, + data: { + errorCode: 'ValidationError', + message: 'Revision payload is invalid', + }, + }, + }) + const wrapper = await mountView([makeProposal()], '/workspace/review', [], [], { + attachTo: true, + }) + await vi.waitFor(() => expect(mocks.getRevisions).toHaveBeenCalledTimes(1)) + + const proposalFetchesBeforeSave = mocks.getProposals.mock.calls.length + await wrapper.get('[data-testid="decision-edit"]').trigger('click') + await flushPromises() + const editor = wrapper.get('[data-testid="revision-editor"]') + await editor.get('[data-testid="revision-field-headline"]').setValue('Reviewer headline') + await editor.get('[data-testid="revision-field-notes"]').setValue('Reviewer notes') + await editor.get('[data-testid="revision-reason"]').setValue('Second revision') + await editor.get('[data-testid="revision-save"]').trigger('click') + await flushPromises() + + const stillOpen = wrapper.get('[data-testid="revision-editor"]') + expect( + (stillOpen.get('[data-testid="revision-field-headline"]').element as HTMLTextAreaElement).value, + ).toBe('Reviewer headline') + expect( + (stillOpen.get('[data-testid="revision-field-notes"]').element as HTMLTextAreaElement).value, + ).toBe('Reviewer notes') + expect( + (stillOpen.get('[data-testid="revision-reason"]').element as HTMLInputElement).value, + ).toBe('Second revision') + expect(wrapper.get('[data-testid="revision-badge"]').text()).toContain('1 revision') + expect(mocks.errorToast).toHaveBeenCalledWith('Revision payload is invalid') + expect(mocks.getProposals.mock.calls.length).toBe(proposalFetchesBeforeSave) + expect( + stillOpen.element.contains(document.activeElement), + ).toBe(true) + + wrapper.unmount() + }) + it('does not let a stale A1 save clear a newer A2 editor seed', async () => { let resolveA1Save!: (value: unknown) => void mocks.createRevision.mockImplementationOnce( diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index 0d10b4d27..4dac2f31f 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -1912,10 +1912,23 @@ async function onSaveRevision(payload: Parameters[0]) { const saveEpoch = revisionEditEpoch const saveResult = await saveRevision(payload) if (!saveResult) return + if (saveResult.outcome === 'rejected') { + // A definite 4xx cannot have committed, so the known revision metadata and + // any open preview remain authoritative. Keep the retryable draft visible + // and return focus inside the editor after the error toast. + if ( + saveResult.current && + proposalIdsEqual(activeProposal.value?.id, saveResult.proposalId) && + revisionEditorPayloadEpoch === saveEpoch + ) { + retainRevisionEditorFocus() + } + return + } // A rejected response may still have committed, so both confirmed and // indeterminate saves require the same later read barrier. Record it before - // branching on the editor outcome, including for stale A continuations that - // finish while another proposal is active. + // branching on the indeterminate outcome, including for stale A continuations + // that finish while another proposal is active. requireRevisionReviewRefresh(saveResult.proposalId) if (saveResult.outcome === 'indeterminate') { // A rejected POST may still have committed. Clear only a preview of the From 9123a4cbbd7b28591ea215434c3cb9da6f4264c6 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 4 Sep 2026 11:09:46 +0100 Subject: [PATCH 2/2] fix(review): preserve uncertainty for revision conflicts --- .../src/composables/useProposalRevisions.ts | 13 ++++--- .../composables/useProposalRevisions.spec.ts | 36 +++++++++++++++++++ .../paper/review/PaperReviewView.spec.ts | 6 +++- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 45e2f9c8b..1e3448df7 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -21,15 +21,18 @@ export type SaveRevisionResult = { } /** - * A revision endpoint 4xx is a definite non-commit: the existing API contract - * uses 4xx responses for validation, authorization, missing proposals, and - * revision conflicts. A timeout, network failure, or 5xx remains indeterminate - * because the server may have committed before the client lost the response. + * These statuses are emitted before the revision write can commit. Other + * statuses remain indeterminate because the server may have committed before + * the client received an error (notably 404/409 from a concurrent or deleted + * proposal, and 5xx responses after the write path was reached). */ function isDefiniteRevisionSaveRejection(error: unknown): boolean { if (typeof error !== 'object' || error === null) return false const status = (error as { response?: { status?: unknown } }).response?.status - return typeof status === 'number' && status >= 400 && status < 500 + return ( + typeof status === 'number' && + [400, 401, 403, 413, 422, 429].includes(status) + ) } export function useProposalRevisions( diff --git a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts index 05b42e5e8..fda559428 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -208,6 +208,42 @@ describe('useProposalRevisions', () => { expect(toastMocks.error).toHaveBeenCalledWith('Revision payload is invalid') }) + it.each([404, 409, 500])( + 'treats HTTP %s as indeterminate because revision persistence is unknown', + async (status) => { + const knownRevision = makeRevision() + vi.mocked(proposalRevisionsApi.getRevisions).mockResolvedValueOnce([knownRevision]) + vi.mocked(proposalRevisionsApi.createRevision).mockRejectedValueOnce({ + response: { status }, + }) + const onRevisionSaved = vi.fn() + const onRevisionStateUncertain = vi.fn() + const proposal = ref(makeProposal()) + const { + editing, + revisionCount, + latestRevision, + revisionsLoaded, + startEditing, + saveRevision, + } = useProposalRevisions(proposal, { onRevisionSaved, onRevisionStateUncertain }) + + await vi.waitFor(() => expect(revisionsLoaded.value).toBe(true)) + startEditing() + + await expect( + saveRevision({ revisedPayload: '{"title":"Uncertain"}', reason: 'Check boundary' }), + ).resolves.toEqual({ proposalId: 'p-1', outcome: 'indeterminate', current: true }) + + expect(onRevisionSaved).not.toHaveBeenCalled() + expect(onRevisionStateUncertain).toHaveBeenCalledOnce() + expect(editing.value).toBe(true) + expect(revisionCount.value).toBe(0) + expect(latestRevision.value).toBeNull() + expect(revisionsLoaded.value).toBe(false) + }, + ) + it('ignores a pre-save revision load that resolves after the save (no stale overwrite)', async () => { // Codex review: a getRevisions request in flight when a save lands must not // overwrite the save's state when it resolves with the pre-save (empty) list. diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index 0ff60faff..293cd77bd 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -4833,7 +4833,11 @@ describe('PaperReviewView', () => { it('consumes one Approve after an indeterminate save even when the revision key is unchanged', async () => { const original = makeProposal({ id: 'uncertain-truth' }) - mocks.createRevision.mockRejectedValueOnce(new Error('Request timed out after commit')) + // A 409 can be raised after a competing writer has committed the next + // revision. It must take the same refresh barrier as a timeout. + mocks.createRevision.mockRejectedValueOnce({ + response: { status: 409 }, + }) mocks.approveProposal.mockResolvedValueOnce( makeProposal({ id: 'uncertain-truth', status: 'Approved' }), )