Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions frontend/taskdeck-web/src/composables/useProposalRevisions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ApiProposal | null>,
/**
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<ApiProposal | null>(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<ApiProposal | null>(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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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' }),
)
Expand Down
17 changes: 15 additions & 2 deletions frontend/taskdeck-web/src/views/paper/PaperReviewView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -1921,10 +1921,23 @@ async function onSaveRevision(payload: Parameters<typeof saveRevision>[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
Expand Down
Loading