diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 055af8cb1..1e3448df7 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -14,12 +14,27 @@ 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 } +/** + * 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' && + [400, 401, 403, 413, 422, 429].includes(status) + ) +} + export function useProposalRevisions( activeProposal: Ref, /** @@ -194,6 +209,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..fda559428 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -166,6 +166,84 @@ 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.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 a37bc4eb1..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 @@ -4590,6 +4590,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( @@ -4775,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' }), ) diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index 9d4620e90..74051cbc7 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -1921,10 +1921,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