Keep Review health disclosed while retained rows render - #2911
Conversation
A queue read only replaces `proposals` when it LANDS. Widening a known-degraded board queue to All boards and failing that read therefore left board B's rows on screen -- an unscoped filter admits every one of them -- while the scope reset retired B's stale/refused disclosure. Once the transient failure toast went, Review presented a partial, known-stale board queue as the ordinary all-boards queue. Refresh health is now owned by the queue that LANDED rather than by the scope being requested. A scope transition retires it only when the retained rows it describes stop rendering; the retained snapshot is restored whenever those rows come back on screen, including by widening rather than by returning to the same board. A 403 keeps its behaviour: `recordQueueAccessRevoked` drops the rows and the landed scope, so the health has nothing left to describe and the revoked panel stays the single owner of that fact. Refs #2214.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d7ec990c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (healthScope === undefined) return false | ||
| if (landedQueueScope.value !== healthScope) return false | ||
| if (healthScope === scope) return true | ||
| return visibleProposals.value.length > 0 |
There was a problem hiding this comment.
When a degraded board queue is displaying a hash-pinned completed proposal with “Show completed” off, clearing the live board filter preserves the hash, and a failed All-boards load retains that proposal. Both review views resolve such a pin directly from proposals rather than visibleProposals, so this predicate returns false and clears the stale/refused disclosure even though the row remains rendered; after the transient toast expires, the partial queue appears healthy. Include the matching rendered hash target in this check.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| return visibleProposals.value.length > 0 | ||
| } | ||
|
|
||
| function resetQueueRefreshHealthForScope(scope: string | null) { |
There was a problem hiding this comment.
Restoration is evaluated only when this reset function runs at the start of a load or poll. If board B’s degraded completed row is hidden, failed B → C → All transitions leave B’s health in retainedQueueRefreshHealth; subsequently enabling “Show completed” exposes that retained row without starting another read, so the snapshot remains dormant and the known-stale queue renders without its persistent error state until a later poll. Restore retained health reactively when visibility changes.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
Independent review outcome — SHIPOne fresh-context adversarial review (read-only toolset: no command execution, no edits) against CONFIRMED HIGH/CRITICAL: none. The reviewer explicitly reports failing to construct:
On the test lens it found no assertion that would still hold if the rows silently vanished, and Non-blocking (2 MEDIUM, 3 LOW) — tracked in #2915 under global law 2c rather than fixed
No fix round was opened: nothing met law 2a's merge-blocking bar, and the two MEDIUMs are bounded, |
1 participant
Problem
PR #2823 scoped Review's refresh health to the rendered queue so board B's stale/refused
warning could not appear as though it described board C. It was parked at the two-round
review ceiling on a confirmed HIGH
(r3968119184):
A queue read only replaces
proposalswhen it lands. So when a known-degradedboard-scoped queue is widened to All boards and that unscoped read fails (500), board B's
rows stay on screen — and an unscoped filter admits every one of them via
matchesActiveBoardFilter— while the scope reset retired both persistent health warnings.Observed on
19a987800:retainedRows=[b-1],queueScopeLoaded=false,stale=false,refused=false. Once the transient failure toast disappeared, Review presented a partial,known-stale board queue as the ordinary all-boards queue. Withholding the count
(
queueScopeLoaded=false) is necessary but not sufficient: the rows still render and arestill actionable.
The same shape existed on the narrowing transition (a degraded unscoped queue narrowed into a
board it already returned rows for) and on the third hop
(B degraded → C fails → back to All boards).
Per the resume note on #2214
(comment) no
third fix was authorized on #2823, so this is a fresh bounded slice from current
mainthatcarries #2823's reviewed work in as a merge and re-scopes the contract on top.
Contract chosen
Keep the landed scope's health disclosed while its retained rows still render. The rows
are not retired, so held drafts and focus are untouched.
Refresh health is now owned by the queue that landed, not by the scope being requested:
queueRefreshHealthStillDescribesScreen(scope, healthScope)— health still describes thescreen when it is the same read identity, or when the queue that landed for
healthScopeis still rendering (
visibleProposals.value.length > 0) under the new filter.(e.g. B → C, where board C's filter hides every retained B row). The one-slot retained
snapshot is kept, exactly as Scope Review refresh health and preserve retained warnings #2823 introduced it.
not only by returning to the same board.
loadProposalssuccess, poll success) now hand health ownership to thescope that just landed, alongside
landedQueueScopeand clearing the retained snapshot.The 403 path is deliberately untouched:
recordQueueAccessRevokeddrops the rows andlandedQueueScope, so the old health has nothing left to describe and the durable revokedpanel stays the single owner of that fact.
queueScopeLoaded, the recovery-sentence rules(#2638), the late-response/
requestIdguards and the two separate failure thresholds areunchanged.
Composable-only. Both skins already render the disclosure straight off
queueRefreshStale/queueRefreshRefusedgated on!queueAccessRevoked, so no view,locale or decision-control change was needed.
Tests
Four new regressions in
src/tests/composables/useReviewProposals.spec.ts, all red-firstagainst the merged #2823 head (4 failed / 6 passed in the
refresh-health scope attributionblock before the fix):
keeps board B %s health while its retained rows render as the All boards queue(
staleandrefused) — the parked HIGH. Asserts real composable state on both halves ofthe contract:
visibleProposalsstill['b-1']and the matching disclosure still up,plus
queueScopeLoaded === falseandqueueAccessRevoked === false. Asserting only theflag would pass if a future change silently dropped the rows instead, so the row assertion
is the negative control.
retires the retained board B warning once the All boards read lands— a real landingclears it and hands ownership over.
restores retained board B health when a failed All boards read re-exposes its rows—the B → C (rows hidden, warning correctly stands down) → All boards (rows back, warning
back) third hop.
does not resurrect a revoked board B warning when the All boards read fails— the403-refused intent: rows and landed scope are gone, so no health comes back.
The B degraded → C fails → back to B shape is covered by #2823's existing
restores retained board B %s stale/refused health after a failed board C detourpair, whichstill passes unchanged.
Proving checks in this worktree at
60d7ec990:npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts—190 passed / 190 (was 185 + the 4 new cases from 2
it.eachentries + 3its).views/ReviewView.spec.ts,views/ReviewView.coverage.spec.ts,views/paper/review/**,composables/usePaperReviewSelectors|useReviewActions|useReviewActions.toastLabel|useReviewCadence|useReviewKeymap,store/queueStore{,.polling,.integration,.demo},utils/queue,api/queueApi,router/workspaceReviewRoute— 31 files, 714 passed / 714.npm run typecheck— clean.npm run build— clean.npm run lint— 0 errors,9 pre-existing warnings, none in the changed files.
What was NOT verified
disclosure directly to the two flags, but the rendered result was not observed.
vitest --runOOMs onthis box); only the composable spec plus the 31 Review/queue spec files above. Hosted
ci-required.ymlcovers the rest.existing scope-identity tests, not with a dedicated archived-mode retained-rows case.
Supersedes
Supersedes #2823 and #2824. #2823's reviewed commits (
3484cf9bf,19a987800) are carried inhere by merge, so its history is preserved rather than re-authored. #2824's other sources
(#2820, #2821, #2822) have all merged, which makes that integration PR redundant.
Refs #2214. (#2214 is a multi-item tracker — items 1 and 2 remain open, so this does not
close it.)