fix(review): keep revision metadata truthful across an in-flight GET

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<number, ProposalRevision>()
+    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