diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 93b85ea45..e4a9d8142 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -95,16 +95,20 @@ export function useProposalRevisions( return created } + function isUsableRevision(revision: ProposalRevision, proposalId: string): boolean { + return ( + revision.proposalId === proposalId && + Number.isInteger(revision.revisionNumber) && + revision.revisionNumber >= 1 + ) + } + function mergeRevision( history: RevisionHistory, proposalId: string, revision: ProposalRevision, ) { - if ( - revision.proposalId !== proposalId || - !Number.isInteger(revision.revisionNumber) || - revision.revisionNumber < 1 - ) { + if (!isUsableRevision(revision, proposalId)) { history.invalid = true return } @@ -117,10 +121,45 @@ export function useProposalRevisions( history.revisions.set(revision.revisionNumber, revision) } + /** + * A revision GET is the authoritative answer for the numbers it reports, so an + * internally consistent response clears an earlier inconsistency instead of + * leaving the proposal unknown for the whole composable lifetime (#2524 (2)): + * badges and diff panes came back only after a remount. A response that + * contradicts itself — a foreign proposal, a malformed number, or one number + * under two ids — is not trusted at all: nothing from it is stored and the + * metadata stays unknown until a consistent answer arrives. Numbers the + * response does not cover keep the revisions their POST responses proved, so a + * GET that predates a save still cannot erase it. + * + * "Consistent" is the stricter reading: the response must be a whole chain of + * its own, 1..max or an explicit empty list. A partial answer with a gap is + * still merged, because each of its revisions is trustworthy, but it does not + * clear the flag — it proves nothing about the number that was disputed. + */ function mergeLoadedRevisions(proposalId: string, revisions: ProposalRevision[]) { const history = getRevisionHistory(proposalId) + const loaded = new Map() + let highestLoaded = 0 for (const revision of revisions) { - mergeRevision(history, proposalId, revision) + if (!isUsableRevision(revision, proposalId)) { + history.invalid = true + history.loaded = true + return + } + const conflicting = loaded.get(revision.revisionNumber) + if (conflicting && conflicting.id !== revision.id) { + history.invalid = true + history.loaded = true + return + } + loaded.set(revision.revisionNumber, revision) + if (revision.revisionNumber > highestLoaded) highestLoaded = revision.revisionNumber + } + + if (loaded.size === highestLoaded) history.invalid = false + for (const [revisionNumber, revision] of loaded) { + history.revisions.set(revisionNumber, revision) } history.loaded = true } @@ -153,11 +192,23 @@ export function useProposalRevisions( * A successful POST proves only the returned revision. Publish metadata only * when the stored responses and/or a completed GET prove every prior number; * otherwise leave the state unknown and request one authoritative reload. + * + * A revision GET already in flight is deliberately NOT suppressed here (#2524 + * (1)). It can carry a revision another session saved while this POST was in + * flight, and `mergeLoadedRevisions` only ever adds to what the POST proved, + * so a pre-save answer cannot lower the published state while a strictly newer + * one is no longer thrown away: bumping the generation here published a + * complete-looking prefix and let the next edit build on a superseded revision + * until the ~15 s poll moved `latestRevisionId`. + * + * That in-flight GET can also FAIL, and its catch then runs against a state + * this function has already published. `loadRevisionState` therefore re-checks + * the generation and the active proposal on BOTH paths, and its catch clears + * metadata only when nothing authoritative has been published — a failed read + * must not turn a proven revision count into an authoritative zero. */ function publishPersistedRevisionMetadata(proposalId: string): boolean { if (activeProposal.value?.id !== proposalId) return false - // Suppress any revision GET that started before this save committed. - loadGeneration += 1 const metadata = getCompleteRevisionMetadata(proposalId) if (!metadata) { revisionCount.value = 0 @@ -210,10 +261,28 @@ export function useProposalRevisions( revisionsLoaded.value = true } catch (e: unknown) { if (gen !== loadGeneration || activeProposal.value?.id !== proposalId) return - revisionCount.value = 0 - latestRevision.value = null - // Leave revisionsLoaded false: the count is not authoritative, so callers - // must fetch (let the backend decide) rather than short-circuit to a no-op. + // A failed GET proves nothing, so it may neither publish nor destroy. It + // clears only metadata that was ALREADY non-authoritative, leaving + // `revisionsLoaded` false so callers fetch (let the backend decide) rather + // than short-circuit to a no-op. + // + // Zeroing unconditionally was safe only while every path into this catch + // had already dropped `revisionsLoaded`. Since a save can now publish a + // proven chain with a GET still in flight (#2524), that GET's rejection + // would leave `revisionsLoaded` true beside a zeroed count: PaperReviewView + // renders the diff as no-operations, blocks Apply with a false zero-op + // toast, and pins the editor to the pre-revision operations, so the next + // save silently discards the revision that was already persisted. + // + // Republishing from the recorded history instead would be wrong the other + // way: the resync path drops `revisionsLoaded` precisely because the + // proposal moved to a revision this history has never seen, and + // re-publishing the old chain there is the false authority #2215 round 1 + // M-1 forbids. + if (!revisionsLoaded.value) { + revisionCount.value = 0 + latestRevision.value = null + } if (!options?.silent) { toast.error(getErrorDisplay(e, 'Failed to load revision history').message) } @@ -306,8 +375,10 @@ export function useProposalRevisions( options?.onRevisionSaved?.() return { proposalId, outcome: 'persisted', current: false } } - // `publishPersistedRevisionMetadata` already invalidated any in-flight - // revision load before the metadata was published. + // A revision GET still in flight is left to land: when it answers it can + // only add to the revisions this save proved, and it may carry a newer one + // (#2524). When it FAILS instead, its catch preserves what was published + // here rather than zeroing it. // Same hazard, different list: a review-queue read that predates this save // would restore the pre-revision proposal. Called synchronously here, in // the same continuation as the POST, so no queue answer can slip between diff --git a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts index 286540d17..81ef8b4a6 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -241,9 +241,17 @@ describe('useProposalRevisions', () => { saveResolvers[0](makeRevision({ id: 'rev-a1', revisionNumber: 1 })) await flushMicrotasks() + + // A1 proves the whole chain on its own, so it publishes straight away + // instead of waiting for A2 to land. + expect(revisions.revisionCount.value).toBe(1) + expect(revisions.latestRevision.value?.id).toBe('rev-a1') + expect(revisions.revisionsLoaded.value).toBe(true) + saveResolvers[1](makeRevision({ id: 'rev-a2', revisionNumber: 2 })) await Promise.all([firstSave, secondSave]) + // Monotonic: the later response raises the published count, never lowers it. expect(revisions.revisionCount.value).toBe(2) expect(revisions.latestRevision.value?.id).toBe('rev-a2') expect(revisions.revisionsLoaded.value).toBe(true) @@ -263,6 +271,158 @@ describe('useProposalRevisions', () => { expect(revisions.revisionsLoaded.value).toBe(true) }) + it('merges a newer revision GET that was in flight when a save landed (#2524)', async () => { + // Another session saved a third revision while this session's own POST was + // in flight. Discarding the GET that was already running would publish a + // complete-looking 1..2 chain and let the next edit build on a superseded + // revision until the ~15 s poll moved latestRevisionId. + let resolveLoad!: (revisions: ProposalRevision[]) => void + let resolveSave!: (revision: ProposalRevision) => void + vi.mocked(proposalRevisionsApi.getRevisions) + .mockResolvedValueOnce([makeRevision({ id: 'rev-1', revisionNumber: 1 })]) + .mockImplementationOnce( + () => new Promise((resolve) => { resolveLoad = resolve }), + ) + vi.mocked(proposalRevisionsApi.createRevision).mockImplementationOnce( + () => new Promise((resolve) => { resolveSave = resolve }), + ) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const { + revisionCount, + latestRevision, + revisionsLoaded, + startEditing, + saveRevision, + loadRevisionState, + } = useProposalRevisions(proposal) + await vi.waitFor(() => expect(revisionsLoaded.value).toBe(true)) + + startEditing() + const savePromise = saveRevision({ revisedPayload: '{"title":"Edited"}', reason: 'Edit' }) + void loadRevisionState('p-1') + await vi.waitFor(() => expect(resolveLoad).toBeTypeOf('function')) + + resolveSave(makeRevision({ id: 'rev-2', revisionNumber: 2 })) + await savePromise + + // The POST alone proves 1..2, so the intermediate publish still happens. + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-2') + expect(revisionsLoaded.value).toBe(true) + + resolveLoad([ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-2', revisionNumber: 2 }), + makeRevision({ id: 'rev-3-other-session', revisionNumber: 3 }), + ]) + await flushMicrotasks() + + expect(revisionCount.value).toBe(3) + expect(latestRevision.value?.id).toBe('rev-3-other-session') + expect(revisionsLoaded.value).toBe(true) + }) + + it('leaves revision metadata unknown when a GET reports another proposal (#2524)', async () => { + vi.mocked(proposalRevisionsApi.getRevisions).mockResolvedValueOnce([ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-foreign', proposalId: 'p-9', revisionNumber: 2 }), + ]) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const { revisionCount, latestRevision, revisionsLoaded } = useProposalRevisions(proposal) + await vi.waitFor(() => + expect(proposalRevisionsApi.getRevisions).toHaveBeenCalledTimes(1), + ) + await flushMicrotasks() + + expect(revisionCount.value).toBe(0) + expect(latestRevision.value).toBeNull() + expect(revisionsLoaded.value).toBe(false) + }) + + it('recovers revision metadata once a consistent GET follows an inconsistent one (#2524)', async () => { + // One number reported under two ids is not a trustworthy answer, but it + // must not blind the composable for the rest of its lifetime. + vi.mocked(proposalRevisionsApi.getRevisions) + .mockResolvedValueOnce([ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-1-conflict', revisionNumber: 1 }), + ]) + .mockResolvedValueOnce([ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-2', revisionNumber: 2 }), + ]) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const { revisionCount, latestRevision, revisionsLoaded, loadRevisionState } = + useProposalRevisions(proposal) + await vi.waitFor(() => + expect(proposalRevisionsApi.getRevisions).toHaveBeenCalledTimes(1), + ) + await flushMicrotasks() + + expect(revisionCount.value).toBe(0) + expect(latestRevision.value).toBeNull() + expect(revisionsLoaded.value).toBe(false) + + await loadRevisionState('p-1') + + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-2') + expect(revisionsLoaded.value).toBe(true) + }) + + it('does not clear an inconsistency on a response that skips the disputed number (#2524 review)', async () => { + // A partial answer is trustworthy revision by revision, but it proves + // nothing about the number that was reported twice. Without the completeness + // requirement it would clear the flag and republish the FIRST id seen for + // that number as authoritative, even though the server contradicted it. + // Scripted rather than a `mockResolvedValueOnce` queue: an assertion that + // fails part way through a queue leaves its remaining entries armed for the + // NEXT test, which turns one red into a cascade of unrelated ones. + const responses: ProposalRevision[][] = [ + [makeRevision({ id: 'rev-1', revisionNumber: 1 })], + [ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-1-conflict', revisionNumber: 1 }), + ], + [makeRevision({ id: 'rev-2', revisionNumber: 2 })], + [ + makeRevision({ id: 'rev-1', revisionNumber: 1 }), + makeRevision({ id: 'rev-2', revisionNumber: 2 }), + ], + ] + let call = 0 + vi.mocked(proposalRevisionsApi.getRevisions).mockImplementation(() => + Promise.resolve(responses[call++] ?? []), + ) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const { revisionCount, latestRevision, revisionsLoaded, loadRevisionState } = + useProposalRevisions(proposal) + await vi.waitFor(() => expect(revisionsLoaded.value).toBe(true)) + expect(revisionCount.value).toBe(1) + + // The server now reports revision 1 under two ids. + await loadRevisionState('p-1') + expect(revisionCount.value).toBe(0) + expect(revisionsLoaded.value).toBe(false) + + // Revision 2 alone would complete the stored chain 1..2, but revision 1 is + // exactly the number in dispute, so the metadata stays unknown. + await loadRevisionState('p-1') + expect(revisionCount.value).toBe(0) + expect(latestRevision.value).toBeNull() + expect(revisionsLoaded.value).toBe(false) + + // A whole chain settles it. + await loadRevisionState('p-1') + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-2') + expect(revisionsLoaded.value).toBe(true) + }) + it.each([404, 409, 500])( 'treats HTTP %s as indeterminate because revision persistence is unknown', async (status) => { @@ -299,9 +459,12 @@ describe('useProposalRevisions', () => { }, ) - it('ignores a pre-save revision load that resolves after the save (no stale overwrite)', async () => { + it('merges a pre-save revision load that resolves after the save without lowering the count', 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. + // overwrite the save's state when it resolves with the pre-save (empty) + // list. Since #2524 that answer is merged rather than dropped, which is the + // same outcome here: merging only ever adds, so an older, emptier list + // cannot take the saved revision back out. let resolveLoad: (v: ProposalRevision[]) => void = () => {} const loadPromise = new Promise((r) => { resolveLoad = r @@ -320,7 +483,7 @@ describe('useProposalRevisions', () => { expect(revisionCount.value).toBe(1) expect(revisionsLoaded.value).toBe(true) - // The stale load now resolves with the OLD (empty) list — must be ignored. + // The stale load now resolves with the OLD (empty) list — it adds nothing. resolveLoad([]) await nextTick() await nextTick() @@ -378,6 +541,57 @@ describe('useProposalRevisions', () => { expect(revisionsLoaded.value).toBe(true) }) + it('keeps the proven metadata when the re-entered A GET REJECTS after the save (#2524 review)', async () => { + // Same interleaving as the test above, except the pending GET fails instead + // of answering. A failed GET proves nothing, so it must not zero a count a + // save already proved: `revisionsLoaded` true beside `revisionCount` 0 is + // the state that makes PaperReviewView render the diff as no-operations, + // block Apply with a false zero-op toast, and pin the editor to the + // pre-revision operations so the next save discards the saved revision. + let rejectReopenedA!: (error: Error) => void + let resolveSave!: (revision: ProposalRevision) => void + vi.mocked(proposalRevisionsApi.getRevisions) + .mockResolvedValueOnce([makeRevision({ id: 'rev-a', proposalId: 'p-1' })]) + .mockResolvedValueOnce([makeRevision({ id: 'rev-b', proposalId: 'p-2' })]) + .mockImplementationOnce( + () => new Promise((_resolve, reject) => { rejectReopenedA = reject }), + ) + vi.mocked(proposalRevisionsApi.createRevision).mockImplementationOnce( + () => new Promise((resolve) => { resolveSave = resolve }), + ) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const { revisionCount, latestRevision, revisionsLoaded, startEditing, saveRevision } = + useProposalRevisions(proposal) + await vi.waitFor(() => expect(latestRevision.value?.proposalId).toBe('p-1')) + + startEditing() + const savePromise = saveRevision({ revisedPayload: '{"title":"Saved"}', reason: 'Save A' }) + proposal.value = makeProposal({ id: 'p-2' }) + await vi.waitFor(() => expect(latestRevision.value?.proposalId).toBe('p-2')) + proposal.value = makeProposal({ id: 'p-1' }) + await vi.waitFor(() => expect(rejectReopenedA).toBeTypeOf('function')) + + resolveSave(makeRevision({ id: 'rev-saved', proposalId: 'p-1', revisionNumber: 2 })) + await expect(savePromise).resolves.toEqual({ + proposalId: 'p-1', + outcome: 'persisted', + current: false, + }) + expect(revisionCount.value).toBe(2) + expect(revisionsLoaded.value).toBe(true) + + rejectReopenedA(new Error('network')) + await flushMicrotasks() + + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-saved') + expect(revisionsLoaded.value).toBe(true) + // The invariant that must hold whatever else changes: never authoritative + // and empty at the same time. + expect(revisionsLoaded.value && revisionCount.value === 0).toBe(false) + }) + it('keeps B metadata authoritative when a stale A save succeeds while B is active', async () => { let resolveSave!: (revision: ProposalRevision) => void vi.mocked(proposalRevisionsApi.getRevisions)