From 3873bb453b9c60481cd0b64cfb2fd078805d419c Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 4 Sep 2026 05:31:58 +0100 Subject: [PATCH] fix(review): reconcile overlapping revision saves --- .../src/composables/useProposalRevisions.ts | 136 ++++++++++++++++-- .../composables/useProposalRevisions.spec.ts | 74 ++++++++-- 2 files changed, 189 insertions(+), 21 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts index 45e2f9c8b..ec0271b05 100644 --- a/frontend/taskdeck-web/src/composables/useProposalRevisions.ts +++ b/frontend/taskdeck-web/src/composables/useProposalRevisions.ts @@ -32,6 +32,17 @@ function isDefiniteRevisionSaveRejection(error: unknown): boolean { return typeof status === 'number' && status >= 400 && status < 500 } +type RevisionHistory = { + revisions: Map + loaded: boolean + invalid: boolean +} + +type RevisionMetadata = { + count: number + latest: ProposalRevision | null +} + export function useProposalRevisions( activeProposal: Ref, /** @@ -64,6 +75,99 @@ export function useProposalRevisions( let loadGeneration = 0 let saveGeneration = 0 + // Successful save responses survive A -> B -> A navigation so a delayed + // response can be reconciled with the response that became current later. + // The map is deliberately conservative: a gap in the chain stays unknown. + const revisionHistoryByProposal = new Map() + + function getRevisionHistory(proposalId: string): RevisionHistory { + const existing = revisionHistoryByProposal.get(proposalId) + if (existing) return existing + const created: RevisionHistory = { + revisions: new Map(), + loaded: false, + invalid: false, + } + revisionHistoryByProposal.set(proposalId, created) + return created + } + + function mergeRevision( + history: RevisionHistory, + proposalId: string, + revision: ProposalRevision, + ) { + if ( + revision.proposalId !== proposalId || + !Number.isInteger(revision.revisionNumber) || + revision.revisionNumber < 1 + ) { + history.invalid = true + return + } + + const existing = history.revisions.get(revision.revisionNumber) + if (existing && existing.id !== revision.id) { + history.invalid = true + return + } + history.revisions.set(revision.revisionNumber, revision) + } + + function mergeLoadedRevisions(proposalId: string, revisions: ProposalRevision[]) { + const history = getRevisionHistory(proposalId) + for (const revision of revisions) { + mergeRevision(history, proposalId, revision) + } + history.loaded = true + } + + function rememberPersistedRevision(proposalId: string, revision: ProposalRevision) { + mergeRevision(getRevisionHistory(proposalId), proposalId, revision) + } + + function getCompleteRevisionMetadata(proposalId: string): RevisionMetadata | null { + const history = revisionHistoryByProposal.get(proposalId) + if (!history || history.invalid) return null + + const revisionNumbers = [...history.revisions.keys()].sort((a, b) => a - b) + if (revisionNumbers.length === 0) { + return history.loaded ? { count: 0, latest: null } : null + } + + const highestRevisionNumber = revisionNumbers[revisionNumbers.length - 1] + for (let revisionNumber = 1; revisionNumber <= highestRevisionNumber; revisionNumber += 1) { + if (!history.revisions.has(revisionNumber)) return null + } + + return { + count: highestRevisionNumber, + latest: history.revisions.get(highestRevisionNumber) ?? null, + } + } + + /** + * 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. + */ + 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 + latestRevision.value = null + revisionsLoaded.value = false + return false + } + revisionCount.value = metadata.count + latestRevision.value = metadata.latest + revisionsLoaded.value = true + return true + } + /** * A save can outlive an A -> B -> A navigation. If A is active again when its * old continuation lands, its current metadata may have been read before that @@ -90,11 +194,16 @@ export function useProposalRevisions( try { const revisions = await proposalRevisionsApi.getRevisions(proposalId) if (gen !== loadGeneration || activeProposal.value?.id !== proposalId) return - revisionCount.value = revisions.length - latestRevision.value = - revisions.length > 0 - ? revisions.reduce((a, b) => (a.revisionNumber > b.revisionNumber ? a : b)) - : null + mergeLoadedRevisions(proposalId, revisions) + const metadata = getCompleteRevisionMetadata(proposalId) + if (!metadata) { + revisionCount.value = 0 + latestRevision.value = null + revisionsLoaded.value = false + return + } + revisionCount.value = metadata.count + latestRevision.value = metadata.latest revisionsLoaded.value = true } catch (e: unknown) { if (gen !== loadGeneration || activeProposal.value?.id !== proposalId) return @@ -180,27 +289,30 @@ export function useProposalRevisions( saving.value = true const revision = await proposalRevisionsApi.createRevision(proposalId, payload) const current = gen === saveGeneration && activeProposal.value?.id === proposalId + rememberPersistedRevision(proposalId, revision) + const metadataKnown = publishPersistedRevisionMetadata(proposalId) if (!current) { // The persisted A1 response is stale as an editor continuation, but it // still makes matching re-entered A metadata (and any pending A GET) // unsafe to treat as an authoritative empty revision list. - invalidateActiveRevisionMetadata(proposalId) + if (!metadataKnown && activeProposal.value?.id === proposalId) { + void loadRevisionState(proposalId) + } // Same hazard, different list: a queue GET that predates this save must // not restore the pre-revision proposal, even if this UI continuation is stale. options?.onRevisionSaved?.() return { proposalId, outcome: 'persisted', current: false } } - // Invalidate any in-flight revision load so a pre-save (stale, empty) list - // can't overwrite this save's state when it resolves after the save. - loadGeneration += 1 + // `publishPersistedRevisionMetadata` already invalidated any in-flight + // revision load before the metadata was published. // 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 // the save landing and the invalidation. options?.onRevisionSaved?.() - latestRevision.value = revision - revisionCount.value += 1 - revisionsLoaded.value = true + if (!metadataKnown) { + void loadRevisionState(proposalId) + } editing.value = false toast.success('Revision saved') return { proposalId, outcome: 'persisted', current: true } diff --git a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts index 05b42e5e8..7349ff3b0 100644 --- a/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useProposalRevisions.spec.ts @@ -80,6 +80,34 @@ async function flushMicrotasks() { await nextTick() } +async function arrangeOverlappingSaves() { + const saveResolvers: Array<(revision: ProposalRevision) => void> = [] + vi.mocked(proposalRevisionsApi.createRevision).mockImplementation( + () => new Promise((resolve) => saveResolvers.push(resolve)), + ) + vi.mocked(proposalRevisionsApi.getRevisions).mockResolvedValue([]) + + const proposal = ref(makeProposal({ id: 'p-1' })) + const revisions = useProposalRevisions(proposal) + await vi.waitFor(() => expect(revisions.revisionsLoaded.value).toBe(true)) + + revisions.startEditing() + const firstSave = revisions.saveRevision({ revisedPayload: '{"title":"A1"}', reason: 'A1' }) + await vi.waitFor(() => expect(saveResolvers).toHaveLength(1)) + + proposal.value = makeProposal({ id: 'p-2' }) + await nextTick() + await flushMicrotasks() + proposal.value = makeProposal({ id: 'p-1' }) + await vi.waitFor(() => expect(revisions.revisionsLoaded.value).toBe(true)) + + revisions.startEditing() + const secondSave = revisions.saveRevision({ revisedPayload: '{"title":"A2"}', reason: 'A2' }) + await vi.waitFor(() => expect(saveResolvers).toHaveLength(2)) + + return { proposal, revisions, saveResolvers, firstSave, secondSave } +} + describe('useProposalRevisions', () => { beforeEach(() => { vi.clearAllMocks() @@ -208,6 +236,33 @@ describe('useProposalRevisions', () => { expect(toastMocks.error).toHaveBeenCalledWith('Revision payload is invalid') }) + it('converges on both persisted revisions when A1 resolves before A2', async () => { + const { revisions, saveResolvers, firstSave, secondSave } = await arrangeOverlappingSaves() + + saveResolvers[0](makeRevision({ id: 'rev-a1', revisionNumber: 1 })) + await flushMicrotasks() + saveResolvers[1](makeRevision({ id: 'rev-a2', revisionNumber: 2 })) + await Promise.all([firstSave, secondSave]) + + expect(revisions.revisionCount.value).toBe(2) + expect(revisions.latestRevision.value?.id).toBe('rev-a2') + expect(revisions.revisionsLoaded.value).toBe(true) + }) + + it('converges on both persisted revisions when A2 resolves before A1', async () => { + const { revisions, saveResolvers, firstSave, secondSave } = await arrangeOverlappingSaves() + + saveResolvers[1](makeRevision({ id: 'rev-a2', revisionNumber: 2 })) + await flushMicrotasks() + expect(revisions.revisionsLoaded.value).toBe(false) + saveResolvers[0](makeRevision({ id: 'rev-a1', revisionNumber: 1 })) + await Promise.all([firstSave, secondSave]) + + expect(revisions.revisionCount.value).toBe(2) + expect(revisions.latestRevision.value?.id).toBe('rev-a2') + expect(revisions.revisionsLoaded.value).toBe(true) + }) + 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. @@ -271,19 +326,20 @@ describe('useProposalRevisions', () => { current: false, }) - // The response belongs to an old edit session, but it still persisted for - // the currently re-entered A. Empty metadata and the pending A GET can no - // longer certify that A has no revisions. - expect(revisionCount.value).toBe(0) - expect(latestRevision.value).toBeNull() - expect(revisionsLoaded.value).toBe(false) + // The response belongs to an old edit session, but the previously loaded + // revision 1 plus the returned revision 2 prove the complete chain. The + // pending GET must not replace that authoritative metadata with its old + // empty answer. + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-saved') + expect(revisionsLoaded.value).toBe(true) resolveReopenedA([]) await flushMicrotasks() - expect(revisionCount.value).toBe(0) - expect(latestRevision.value).toBeNull() - expect(revisionsLoaded.value).toBe(false) + expect(revisionCount.value).toBe(2) + expect(latestRevision.value?.id).toBe('rev-saved') + expect(revisionsLoaded.value).toBe(true) }) it('keeps B metadata authoritative when a stale A save succeeds while B is active', async () => {