fix(review): tell the truth before the similar-past and confidence disclosures open - #2662
Conversation
…pens
The card put its entire empty state inside a collapsed region, so a reviewer
looking at a proposal with no comparable history saw only "Show similar
decisions" and had to open the control to learn there was nothing behind it.
Hoist the empty sentence to the card, above the disclosure, and give the
closed control an honest label ("Show similar decisions (none found)"). The
sentence is not duplicated inside the region; the region gets a short line
saying where comparable decisions will appear, so opening the control explains
itself instead of revealing a zero-height void that the required E2E smoke
asserts is visible.
The disclosure and its toggle stay present in every state: PaperReviewView.spec.ts,
PaperReviewView.language.spec.ts and tests/e2e/review-proposals.spec.ts all drive
that button on an empty fixture, so removing it on empty is not available here.
This layer cannot tell "still loading" from "loaded and empty" — it receives
rows and an apply rate, nothing about the fetch. The empty copy is the honest
reading of an empty array at this layer; distinguishing the two needs a prop
from the rail and stays tracked on #1940.
New keys land in en, es and it together (catalog parity guard).
Refs #1940
The sentence naming where a proposal's confidence came from lived only inside the collapsed confidence region. On an Applied record that made it the only statement on the whole screen about the source — ReviewMain gates its confidence-source badge off there, and the view passes an empty authorMeta for the deterministic and not-reported sources — and it was unreadable until the reviewer opened a control that gave no reason to be opened. Hoist the sentence to the card, above the disclosure, keeping its paper-review-author-confidence-source testid. It renders whenever the breakdown carries no components, which is exactly when the region has no bars to show. The region keeps the heading, the bars and the model note. Derive the region heading from the bars actually rendered rather than from the claimed source. A model-reported breakdown with an empty components array otherwise announced "Model-reported item confidence" directly above a body saying no model confidence was reported. The backend does not emit that pair (only a view-spec fixture builds it), but a derived heading cannot state it at all. The deterministic and not-reported wording is unchanged; only its position is. Refs #1940
…rom the DOM Both regions relied on v-show alone, so a collapsed region carried no `hidden` attribute and stayed in the accessibility tree for anything reading the DOM rather than the computed style. The sibling provenance card already binds `hidden` to its collapsed state; these two now match it. Refs #1940
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
CI attribution at head 5cd7f5e: the required CI run 33951635415 went red only on "Docs Governance / Worktree Helper (Windows PowerShell)", the worktree-helper script suite, at its test "Late branch collision must not leave an orphan helper-created worktree." (RuntimeException, exit 1) after PASS marks on tests 4, 6 and 10. This PR touches two review components, three catalogs and two new specs and nothing under scripts/; the same job passed on #2658 (ab1ebcd) minutes earlier, so this is an intermittent in the helper suite on the Windows runner (platform-lane seam, reported there). Re-run of the failed job requested once with the head unchanged; the merge gate stays ci-required green at this exact head. |
|
Beta lane: the Worktree Helper (Windows PowerShell) red at 5cd7f5e (run 33951635415, job 101267419144) is tracked as #2664. The scenario's fixed 5 s poll expired before the initializer's scheduled orphan removal (a detached PowerShell host plus six Git calls) finished on the hosted runner; the printed-cleanup assertion passed and the registration check never ran, so it is a test-side ceiling, not a helper defect, and unrelated to this PR's diff. The identical-head rerun is the right disposition; the fix is a deadline-based wait in scripts/git/Test-New-CodexIssueWorktree.ps1 under #2664. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head 5cd7f5e). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff plus the worktree at the head. Verdict: SHIP, no CRITICAL or HIGH. Confirmed clean: the required E2E review-proposals.spec.ts keeps passing by construction (toBeHidden agrees with v-show plus :hidden; the confidence region is never zero-height on a settled real response; the similar-past region now holds the empty-detail line where it held an empty div; no text or aria-expanded assertion on the disclosure labels; the axe scan runs with the regions expanded over the same classes and contrast as before; no new ids); the held PaperReviewView.spec.ts and the language spec keep passing unedited (the disclosure testids and the aria-expanded flip untouched; the Italian assertion uses toContain, which the extended label satisfies; the raw-key guard holds because every new key exists in all three catalogs); the empty sentence renders only with no rows and exactly once; the author-card source sentence renders exactly once; the heading derivation is behaviour-preserving for every backend-reachable breakdown (only the fixture-only model-reported-with-zero-components pair changes); both hoisted lines are siblings of the button, the aria pairing is untouched, and the hidden binding matches the ReviewProvenance precedent byte for byte; catalog parity holds; ReviewWhyNow, ReviewRightRail, PaperReviewView and its specs untouched. Triage:
Merge gate: round-2 push, ci-required green at the round-2 head (the round-1 head's only red was the worktree-helper Windows test, #2664, unrelated to this PR), aged three minutes, then merge commit; a scoped verification pass only if the round-2 diff threads new state through the rail. |
… inherit Review round 2. Both hoisted sentences state emptiness from an empty default that also means "still loading" and "the read failed". usePaperReviewSelectors initialises confidenceData to EMPTY_CONFIDENCE and similarPastData to an empty array, clearSelectorData resets both on every proposal switch, and a failed batch deliberately leaves them there so a partial snapshot cannot publish; no flag distinguishes that state, and evidenceUnavailable on ReviewRightRail is set only from the Apply-time refresh (activeRevisionReviewUnavailable), never from the page-load batch. Neither card can fix this. ReviewRightRail is a pure props component and the twelve props it receives are all bound in PaperReviewView.vue, which is held by open PR #2629. The composable does expose `loading`, but nothing threads it in. Adding a settled prop defaulting to true would compile and change nothing on the real surface, while presenting the defect as handled, so this commit records the gap instead of simulating a fix. Record it in both doc blocks, naming the three situations that arrive as the same empty value and the one place that could tell them apart. The copy itself is not new: the same sentences rendered inside the disclosures before #1940, so hoisting made an existing false claim easier to see rather than creating one. Also use the isEmpty computed for the apply-rate guard, which still read rows.length > 0 while the rest of the template had moved on. Refs #1940
1 participant
Summary
Two right-rail review cards stated their most important fact only inside a collapsed disclosure, so a reviewer had to open a control to find out whether it had anything behind it.
ReviewSimilarPastrendered "Show similar decisions" unconditionally and kept "No comparable past decisions." inside the collapsed region. On a proposal with no comparable history the closed card looked identical to one with three prior decisions waiting.ReviewAuthorCardkept the confidence-source sentence inside its collapsed region. On an Applied record that sentence is the only statement on the entire screen about where the confidence number came from:ReviewMain.vuegates its own confidence-source badge off for applied records, andPaperReviewViewpasses an emptyauthorMetafor the deterministic and not-reported sources. The one honest line was hidden behind a control with no reason to be opened.Both facts now render at first paint, above the disclosure. The disclosures themselves stay present and operable in every state. Both collapsed regions now bind
hidden, matching the siblingReviewProvenance.vue.This closes the residual retained from #2166 (recorded on the issue 2026-08-28T07:43). It does not touch the maintainer's D-9 questions (waves, request-edit fields, defer durations).
Refs #1940
Changes
fix(review): state an empty similar-past card before its disclosure opens(4309b7f)ReviewSimilarPast.vue: the empty sentence moves out of the region and onto the card, above the disclosure, with apaper-review-similar-past-emptytestid. The closed label becomes "Show similar decisions (none found)" on empty. The region carries a distinct line naming where comparable decisions will appear, not a second copy of the hoisted sentence.src/locales/{en,es,it}/review.ts: newsimilarPast.details.showEmptyandsimilarPast.emptyDetail, added to all three catalogs together for the parity guard.fix(review): read the confidence source without opening the author card(3851022)ReviewAuthorCard.vue: the confidence-source sentence moves onto the card above the disclosure, keeping itspaper-review-author-confidence-sourcetestid, rendered whenever the breakdown has no components. The region keeps the heading, the bars and the model note. The region heading is now derived from the bars actually rendered rather than from the claimed source.fix(review): hide the collapsed confidence and similar-past regions from the DOM(5cd7f5e):hiddenbound to the collapsed state on both regions, matchingReviewProvenance.vueline 113.docs(review): record the unsettled-evidence gap the hoisted sentences inherit(0a726cf, review round 2)ReviewSimilarPast.vue: the apply-rate guard uses theisEmptycomputed instead of the leftoverrows.length > 0. Behaviour is identical; the two spellings had drifted apart in the same template.New component specs:
src/tests/views/paper/review/ReviewSimilarPast.spec.tsandsrc/tests/views/paper/review/ReviewAuthorCard.spec.ts. Neither component had a component-level spec before; the only coverage was one shared assertion inReviewWhyNow.spec.tsmounting a populated rail.Test plan
Run from
frontend/taskdeck-web.Red first, before any component was edited, with both new spec files in place:
The eleven failures were the card-level truth cases and the hidden bindings:
hiddento the collapsed state.hiddento the collapsed state.Unable to get [data-testid="paper-review-similar-past-empty"], andexpected 'No comparable past decisions.' not to contain 'No comparable past decisions.'on the region text.Round 2 changed no rendered output, so it has no red-first run of its own. The doc blocks and the
isEmptyguard are covered by re-running everything below; all counts are from the round-2 head.Verified after the change:
The regression gate is the second command.
PaperReviewView.spec.ts,PaperReviewView.language.spec.tsandReviewWhyNow.spec.tswere NOT edited:git statusshows only the seven files listed above. The language spec is the sharpest constraint here, since it mounts an empty similar-past fixture and asserts the Italian label.NOT verified:
tests/e2e/review-proposals.spec.ts(Playwright) was not run locally; it needs a running stack. CI runs it in the required E2E smoke lane. It drives all three disclosures by testid on a fresh account with zero similar rows, and asserts each region is hidden before the click and visible after it, plus an axe scan over.paper-review-author,.paper-review-provand.paper-review-past.catalogs.spec.ts; its tone is a review question, not a test result.Boundaries and risks
Shape A, keeping the disclosure in every state, is forced rather than chosen. Three call sites drive that button on an empty fixture and none of them may be edited here:
PaperReviewView.spec.ts(iterates the three disclosure testids and assertsaria-expandedflips),PaperReviewView.language.spec.ts(asserts the Italian label on an emptygetSimilarPastfixture), and the required-CItests/e2e/review-proposals.spec.ts(clicks the expander with zero rows). Removing the expander on empty would break all three, so the truth goes above the button instead.Two consequences of that shape, both deliberate:
Known gap: the hoisted sentences inherit an unsettled-evidence problem (review round 2)
Both hoisted sentences are built from an empty default that means three different things, and neither card can tell them apart.
usePaperReviewSelectorsinitialisesconfidenceDatatoEMPTY_CONFIDENCE(zero components, sourcenot-reported, no note) andsimilarPastDatato an empty array.clearSelectorDataresets both on every proposal switch. A failed batch deliberately leaves both at those defaults, because publishing successful siblings from a partial snapshot "would turn unavailable evidence into affirmative empty states" in the composable's own words. Nothing flags that state:evidenceUnavailableonReviewRightRailis bound toactiveRevisionReviewUnavailable, which is the Apply-time refresh failure, never the page-load batch.So on a failed similar-past read the rail states "No comparable past decisions." and offers "Show similar decisions (none found)" as a positive claim about a response that never arrived, and on a slow read the sentence paints and then vanishes when rows land. The confidence-source sentence has the same shape:
EMPTY_CONFIDENCErenders as "No model confidence reported".This is not fixable inside these two cards.
ReviewRightRailis a pure props component, and all twelve props it receives are bound inPaperReviewView.vue(lines 2943-2960), which is held by open PR #2629. The composable does exposeloading, but nothing threads it into the rail. Adding asettledprop defaulting totruewould compile and pass tests while changing nothing on the real surface, and would leave the next reader believing the defect was handled, so this PR records the gap in both doc blocks instead of simulating a fix.Two things worth keeping in proportion. The copy is not new: the same sentences rendered inside the disclosures before this PR, so the false claim in the unsettled state already shipped. Hoisting makes it easier to see rather than creating it. And the fix has one obvious shape once
PaperReviewView.vueis free: thread the selectors' settled or loading state into the rail and gate the claim, the label variant and the source sentence on it.Loading is still indistinguishable from empty at this layer.
ReviewSimilarPastreceivesrowsandapplyRateand nothing about the fetch, so an in-flight load renders as "none found". Fixing that needs a prop fromPaperReviewView.vueorReviewRightRail.vueand is out of scope here; it stays recorded on #1940. The related rail-level defect (proposal A's data showing under proposal B while B loads) is also untouched for the same reason.The confidence region can now open onto a heading and nothing else, when the breakdown has no components and no note. That is the shape of the composable's own
EMPTY_CONFIDENCEdefault.mapConfidencepasses the note straight through from the DTO (note: dto.note ?? undefined), so a real deterministic or not-reported breakdown shows heading plus note; whether the backend attaches a Note to every such payload is a backend fact this frontend slice did not verify. A heading-only region is thin but states nothing false, and the source sentence itself is now on the card where it is read without opening anything.The contradictory heading pair,
model-reportedwith an empty components array, is fixture-only. The backend does not emit it;PaperReviewView.spec.tsbuilds it. Deriving the heading from the rendered bars means it cannot be constructed at all, which is cheaper than leaving a latent contradiction in the template.Held files, not touched:
PaperReviewView.vueandPaperReviewView.spec.ts(held by open PR #2629),PaperReviewView.language.spec.ts,ReviewWhyNow.spec.ts,ReviewRightRail.vue(needs no prop change for this shape), and the E2E specs. The author card's own disclosure label is unchanged: making it count-aware was not part of this slice.No docs changed. This is a copy-position and accessibility fix inside two rail cards; shipped behaviour in
docs/STATUS.mdterms (review-first, explicit approve then execute) is unaffected.