From 7bb19fc7e8e62578529e6a6fe18240f097a1d074 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci Date: Fri, 4 Sep 2026 22:41:10 +0100 Subject: [PATCH 1/2] fix(review): keep revision metadata truthful across an in-flight GET publishPersistedRevisionMetadata no longer bumps loadGeneration, so a revision GET already running when a save lands is merged instead of dropped. Merging only ever adds to what the POST proved, so a pre-save answer cannot lower the published state, while a strictly newer one no longer disappears until the next poll. mergeLoadedRevisions now validates a GET response on its own terms. An internally consistent response is authoritative for the numbers it reports and clears an earlier inconsistency instead of latching history.invalid for the composable lifetime; an inconsistent one stores nothing and leaves the metadata unknown. Numbers the response does not cover keep the revisions their POST responses proved. Refs #2524 --- .../src/composables/useProposalRevisions.ts | 59 ++++++++-- .../composables/useProposalRevisions.spec.ts | 110 ++++++++++++++++++ 2 files changed, 159 insertions(+), 10 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 93b85ea45..926736b26 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,38 @@ 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. + */ function mergeLoadedRevisions(proposalId: string, revisions: ProposalRevision[]) { const history = getRevisionHistory(proposalId) + const loaded = new Map() 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) + } + + history.invalid = false + for (const [revisionNumber, revision] of loaded) { + history.revisions.set(revisionNumber, revision) } history.loaded = true } @@ -153,11 +185,18 @@ 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`. `loadRevisionState` re-checks + * the generation and the active proposal before it publishes anything. */ 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 @@ -306,8 +345,8 @@ 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: it can only add to the + // revisions this save proved, and it may carry a newer one (#2524). // 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..602690215 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,108 @@ 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.each([404, 409, 500])( 'treats HTTP %s as indeterminate because revision persistence is unknown', async (status) => { From b0421100d9aeb37e65614a87124463a023e586d9 Mon Sep 17 00:00:00 2001 From: Cristian Tcaci Date: Fri, 4 Sep 2026 23:16:20 +0100 Subject: [PATCH 2/2] fix(review): never let a failed revision GET erase a proven count Letting an in-flight GET land after a save meant its REJECTION could also land after the save published. loadRevisionState's catch zeroed the count and nulled the latest revision while leaving revisionsLoaded true, which 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 revision that was already persisted. The catch now clears only metadata that was already non-authoritative. Republishing from the recorded history instead was rejected: the resync path drops revisionsLoaded precisely because the proposal moved to a revision that history has never seen, and republishing the old chain there is the false authority #2215 round 1 M-1 forbids. mergeLoadedRevisions now clears the inconsistency flag only for a response that is a whole chain of its own. A partial answer is still merged, because each of its revisions is trustworthy, but it proves nothing about the number that was reported twice. The two comments that described only the success path are corrected, and the pre-save-load test title now says merges rather than ignores. Refs #2524 --- .../src/composables/useProposalRevisions.ts | 50 ++++++-- .../composables/useProposalRevisions.spec.ts | 110 +++++++++++++++++- 2 files changed, 148 insertions(+), 12 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 926736b26..e4a9d8142 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -131,10 +131,16 @@ export function useProposalRevisions( * 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) { if (!isUsableRevision(revision, proposalId)) { history.invalid = true @@ -148,9 +154,10 @@ export function useProposalRevisions( return } loaded.set(revision.revisionNumber, revision) + if (revision.revisionNumber > highestLoaded) highestLoaded = revision.revisionNumber } - history.invalid = false + if (loaded.size === highestLoaded) history.invalid = false for (const [revisionNumber, revision] of loaded) { history.revisions.set(revisionNumber, revision) } @@ -192,8 +199,13 @@ export function useProposalRevisions( * 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`. `loadRevisionState` re-checks - * the generation and the active proposal before it publishes anything. + * 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 @@ -249,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) } @@ -345,8 +375,10 @@ export function useProposalRevisions( options?.onRevisionSaved?.() return { proposalId, outcome: 'persisted', current: false } } - // A revision GET still in flight is left to land: it can only add to the - // revisions this save proved, and it may carry a newer one (#2524). + // 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 602690215..81ef8b4a6 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -373,6 +373,56 @@ describe('useProposalRevisions', () => { 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) => { @@ -409,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 @@ -430,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() @@ -488,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)