fix(review): keep the Legacy diff pane keyed across a proposal's absence

[Chris0Jeky]

Summary

In the Legacy review skin the diff pane is rendered per row inside components/review/ReviewProposalCard.vue, gated on proposalIdsEqual(props.selectedDiffProposalId, props.proposal.id) and wired from views/LegacyReviewView.vue. The #2215 B revision watcher in src/composables/useReviewActions.ts closes that pane when the revision moves under it, but it dropped its tracked (proposal, revision) pair whenever the open pane's proposal was absent from the queue snapshot. It nulled only the tracker and never reset the pane, so selectedDiffProposalId and selectedDiff survived the absence window unkeyed. When the row returned in a later refresh carrying a revision another reviewer had saved meanwhile, openDiffProposalId was already null, the proposalRevisionMoved guard could not fire, and the re-mounting card re-adopted the pre-revision diff while Approve pins, and Apply executes, the server's latest.

Design choice for the residual: retain the last known (proposalId, revisionIdentity) pair across the absence window and close the pane on return when proposalRevisionMoved is true, rather than resetting the pane whenever the row disappears. Two reasons. Resetting on absence alone would also discard a pane whose diff is still exactly what Apply would execute, when a partial read drops and restores a row unchanged. More importantly it would break the #2215 round-2 asymmetry: a proposal that leaves the queue and returns Rejected or Expired legitimately reports a null effective revision, and that case must convert to the decision-time stored presentation (#1397 LOW-5), not be wiped as a collaborator edit. Retention keeps that conversion working across an absence window; a blanket reset would not.

The pair is now cleared only when the pane is actually closed or moved to another row. proposalIdsEqual returns false for a null selection, so a closed pane clears it through the same check. Without that distinction a pane reopened after the revision moved while it was closed would have been wiped on the spot by its own opening.

Paper is unaffected and untouched: it owns its own diff flow with a watcher in PaperReviewView.vue.

Refs #2215

Closes the Legacy diff-pane re-adoption defect described above (residual 1 on the issue's claim comment) and fills the missing proposalRevisionMoved null-transition coverage for expire. The Paper-side residuals listed on that comment remain open, so this does not close the issue.

Changes

• frontend/taskdeck-web/src/composables/useReviewActions.ts — the revision watcher keeps its tracked (proposal, revision) pair while the pane stays open on a proposal missing from the current queue snapshot, and clears it only when the pane closes or moves to another row.
• frontend/taskdeck-web/src/tests/composables/useReviewActions.spec.ts — four regressions: the leave-then-return-with-a-newer-revision case (red before the fix), the benign same-revision return, a reopen after the revision moved while the pane was closed, and the expire null-transition that must convert to the stored presentation.

No change to Paper, useReviewProposals.ts or useProposalRevisions.ts. No locale keys added or changed.

Test plan

Verified, from frontend/taskdeck-web after npm ci:

• Red first: npx vitest --run --maxWorkers=2 src/tests/composables/useReviewActions.spec.ts before the fix — 1 failed, 46 passed (47), failing on closes an open diff when its proposal leaves the queue and returns with a newer revision (#2215 residual) with expected 'p-1' to be null. The other three new tests passed before the fix and guard against over-correcting.
• After the fix, same command: 47 passed (47).
• npx vitest --run --maxWorkers=2 src/tests/composables/useReviewActions.spec.ts src/tests/views/ReviewView.spec.ts src/tests/views/ReviewView.coverage.spec.ts src/tests/components/review src/tests/views/paper/review/ReviewQueueRail.spec.ts src/tests/i18n/catalogs.spec.ts — 15 files passed, 273 tests passed.
• npm run typecheck — clean.
• npx eslint src/composables/useReviewActions.ts src/tests/composables/useReviewActions.spec.ts — clean, exit 0.
• npm run build — succeeded, PWA precache 142 entries.
• git diff --check — clean.

NOT verified: Playwright E2E (not run, no stack up); no backend tests run (no backend change); no manual browser pass; the full frontend vitest suite was not run, only the specs listed above.

Boundaries and risks

• Files held by PR fix(review): bound the post-revision truth refresh and keep it retryable #2576 (PaperReviewView.vue, useReviewProposals.ts, usePaperReviewSelectors.ts, PaperReviewView.spec.ts) were not touched.
• Behaviour change is scoped to the Legacy diff pane's lifetime across a queue snapshot that omits its proposal. The pane's open state now survives an absence window instead of being silently unkeyed; every path that previously closed the pane still closes it.
• Residual risk: while a pane is open on a proposal absent from the queue, selectedDiff holds content for a row that renders no card. That was already true before this change; the change only makes the state keyed, so the return path can act on it.

Round 2

Review found one MEDIUM in the round-1 fix, now fixed at f83981cad.

MEDIUM — a retained pair could outlive its pane, swallowing the next click. Retaining the (proposal, revision) pair across an absence window assumed every pane close reaches the revision watcher. It does not. Two teardown paths run resetDiffState directly: the non-validation diff-error path and the stored-preview access retraction. If either fires while the pane's row is absent from the queue, the watcher's getter is null both before and after, so no callback runs and the pair survives a pane that no longer exists.

The reachable sequence: the pane is open on A at rev-1; the background poll drops A's row, which is vetoed only by an in-flight action or an open dialog and never by an in-flight /diff; the /diff then fails and tears the pane down; A returns at rev-2 with no callback firing; the reviewer clicks show-diff on A; the pre-flush watcher reads the stale openDiffProposalId, finds proposalRevisionMoved true, resets the pane and bumps latestDiffRequestId, and the response for the click is discarded. The click is swallowed once, and the second click works.

The fix is to clear the pair inside resetDiffState(). A closed pane is exactly the event the watcher cannot always see, and the absence-window retention never calls that function, so the two rules do not conflict. The redundant clearing in the watcher's move branch is removed, since resetDiffState now owns it.

Comment. The rationale block now records that Paper resolves the same disappearance the other way, and that the divergence is deliberate under #1124 / ADR-0038. Paper has exactly one active proposal, so its watcher on activeProposal.value?.id sees the target drop to null and replaces the surface with its settled-elsewhere notice. Legacy renders a pane per row with no single active proposal, so nothing there observes the drop, which is why the pane is keyed on the pair instead. The skins must not drift in what a reviewer is shown; they are not obliged to detect it by the same mechanism.

Round 2 verification

• Red first: npx vitest --run --maxWorkers=2 src/tests/composables/useReviewActions.spec.ts with the new case only — 1 failed, 47 passed (48), failing with expected null to be 'p-1', the swallowed click. After the fix: 48 passed (48).
• npx vitest --run --maxWorkers=2 src/tests/composables/useReviewActions.spec.ts src/tests/views/ReviewView.spec.ts src/tests/views/ReviewView.coverage.spec.ts src/tests/components/review src/tests/views/paper/review/ReviewQueueRail.spec.ts src/tests/i18n/catalogs.spec.ts — 15 files passed, 274 tests passed.
• npm run typecheck — clean. npx eslint on both changed files — clean, exit 0. npm run build — succeeded. git diff --check — clean.

NOT verified in round 2: unchanged from round 1. No Playwright, no manual browser pass, no full frontend suite.

The Windows Frontend Unit red on this PR is the dev-up launcher suite (dev-up.test.mjs, the invalid-Vite-outcome and foreign-port-owner cases, 30 s timeout), the known CI-control cohort, unrelated to this change and untouched here.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Review gate (Codex credits exhausted, SC-9): one fresh-context adversarial reviewer on head 2ab5bb7 (merge base 330ccb4). Verdict: SHIP. Confirmed: at the base the absent branch nulled openDiffProposalId, which made the close branch unreachable on the first present tick after an absence, so retention only ever adds closes and both versions converge after the return tick; every must-not-close case (reject, expire, dismiss, approve with a pin) is protected by proposalRevisionMoved's null-identity asymmetry, unchanged; the state table across close-on-newer, keep-on-same, convert-on-null, pane closed while present, pane moved to B, two absences with a move between, and wholesale poll replacement holds; Preview == Apply holds because the only way back after a close is a fresh /diff and the close bumps the request id first; the four specs are mutation-sensitive in both directions and deterministic; two files, no Paper or composable change.

Findings by bin:

• Fixed in the same round (introduced by the retention): MEDIUM, a pane closed by a non-toggle path while its row is absent (the diff-error teardown at resetDiffState ~527 or the stored-preview retraction at ~138) leaves the pair armed because the watcher getter is null before and after, so when the proposal returns at a newer revision the reviewer's next show-diff click is swallowed once (the stale pair triggers a close and the response is discarded). Fix: clear the pair inside resetDiffState(), with a spec for open, row absent, diff 500, row returns newer, re-toggle opens on the first click.
• Tracked: [Frontend][Review] Legacy diff pane: a proposal returning Applied with a pinned newer revision is wiped instead of converted (from PR #2590 review) #2598 (a proposal returning Applied with a pinned newer revision is now wiped instead of converted; the rationale comment should note Paper resolves disappearance the other way).
• Declined: none.

Evidence at the reviewed head (implementation run): red-first 1 failed / 46 passed then 47/47 on the composable spec, 15 files / 273 across the Legacy Review set, typecheck, ESLint, build, diff check. Not verified: Playwright, real-browser behaviour of the virtualized Legacy list. Merge after the fix commit ages and ci-required is green at the new head.

[Chris0Jeky]

Round 2 (head f83981c): the fix diff was verified by the coordinator reading it. The retained (proposalId, revisionIdentity) pair is now cleared inside resetDiffState(), the event the watcher could not see, and the now-redundant clearing in the watcher's move branch is removed; the reachable sequence (pane open on A, row absent, diff 500, A returns at rev-2, first click swallowed) reproduced red with expected null to be 'p-1' and is green after; the rationale comment records that Paper resolves disappearance through its single activeProposal watcher while Legacy has per-row panes, a deliberate divergence under #1124 / ADR-0038 in mechanism, not in what the reviewer is shown. Counts at this head (implementation run): composable spec 48/48 (red first 1 failed / 47 passed), Legacy set 15 files / 274, typecheck, scoped ESLint, build, diff check. Round count: 2. Merge after ci-required is green at f83981c and the head has aged.