fix(review): keep overlapping revision metadata truthful

frontend/taskdeck-web/src/composables/useProposalRevisions.ts

@@ -35,6 +35,17 @@ function isDefiniteRevisionSaveRejection(error: unknown): boolean {
   )
 }
 
+type RevisionHistory = {
+  revisions: Map<number, ProposalRevision>
+  loaded: boolean
+  invalid: boolean
+}
+
+type RevisionMetadata = {
+  count: number
+  latest: ProposalRevision | null
+}
+
 export function useProposalRevisions(
   activeProposal: Ref<ApiProposal | null>,
   /**
@@ -67,6 +78,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<string, RevisionHistory>()
+
+  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
@@ -93,11 +197,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
@@ -183,27 +292,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 }

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<ApiProposal | null>(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.each([404, 409, 500])(
     'treats HTTP %s as indeterminate because revision persistence is unknown',
     async (status) => {
@@ -307,19 +362,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 () => {