feat(AF-654): show advisory approval likelihood to reviewers - #706
Merged
Conversation
Contributor
Contributor
Coverage Report for Frontend Coverage (frontend)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #654. Part of the approval-outcome prediction epic (#645); consumes the read side landed in #653.
What this does
Renders the advisory approval-outcome prediction to reviewers:
components/review/ApprovalPredictionBadge.tsx— compact percentage pill for review-queue rows, with a tooltip explaining it is a statistical estimate. Renders nothing when the row is unscored.components/review/ApprovalPredictionPanel.tsx— "Approval likelihood"DetailCardbody on the query detail page: the label, the percentage, and a permanent advisory note, with pending / unavailable / failed / skipped fallbacks.—with an explanatory tooltip when the row carries no probability.query.prediction_completenow invalidates['queries','detail',id]and['reviews','pending'], so both surfaces fill in without polling.approval_prediction.*plusqueries.detail.card_approval_predictionandreviews.col_approval_likelihood, in all 7 locales.Advisory-only guarantee. Copy is strictly non-directive — "Historical approval likelihood: 78%" and "Statistical estimate based on this organization's past review decisions. Advisory only — it never approves or rejects anything." The badge deliberately uses the neutral
--fg-muted/--bg-sunken/--bordertokens rather than the--risk-*palette, so the number never reads as a risk level. No hardcoded colours anywhere.Deliberate decisions worth a reviewer's attention
QUERY_REVIEW. The backend servesapproval_predictionto any authorized reader, so this is a client-side product call: showing a submitter how their peers are likely to vote on their own still-open request invites cancel-and-resubmit gaming. Issue frontend: approval prediction — badge + detail panel + i18n #654's goal line is "Show the prediction to reviewers".—tooltip does not name the skip reason.PendingReviewItemcarries onlyapprovalProbability— no reason field exists on the list payload — so the tooltip points at the detail page rather than guessing which sentinel applies. This is a deviation from issue step 3, which asked for the reason in the queue tooltip.updated_at. Scoring fires off the transition into review and takes seconds; without the bound, any query already sitting inPENDING_REVIEWat deploy time would show "Computing the approval likelihood…" forever.skipped_reasonis modelled as a closedApprovalPredictionSkipReasonunion, and the lookup map isRecord<ApprovalPredictionSkipReason, string>so a future backend token breaks compilation instead of silently degrading. The runtime fallback stays, for the window where the server ships a token ahead of the frontend. The raw machine token can never reach the DOM.approval_prediction?,approval_probability?) because the backend setsdefault-property-inclusion: non_null— these fields are absent, notnull.E2E
No new Playwright spec, deliberately. Per the issue: the model needs 50+ decided queries before it serves, so the only state reachable in the e2e stack is the cold-start "skipped" notice. Regression coverage was verified instead — the full suite ran locally against a stack rebuilt from this branch: 251 passed, 2 skipped. No reviews spec addresses the queue table positionally (they all use
getByRole('row').filter({ hasText })), so the new column shifts nothing.Docs
docs/06-frontend.md— the approval-likelihood card and its states, the new review-queue column, and thequery.prediction_completerow in the default-invalidations table.website/index.html,website/docs/index.htmland the remaining docs sweep are out of scope by design — they are issue #655, the last item in the epic.Verification
npm run lint(0 errors; 24 pre-existing warnings, none in touched files) ·npm run typecheck·npm run test:coverage(175 files, 1361 tests passed, 93.88% lines / 83.67% branches) ·npm run build· locale parity green · e2e 251 passed.Review notes
Three agents reviewed this branch before it opened (
af-verifier,af-reviewer,af-frontend-reviewer). No Blockers from any of them. Every concern raised is fixed in49708d7f— the reviewer gate, the queue tooltip, the bounded pending state, the closed skip-reason union, and a docs correction (the panel has five states, not four). Two nits were consciously not taken:--risk-medon the failure-warning icon (af-frontend-reviewer) — it colours the warning glyph, not the prediction value, and is copied verbatim fromCostEstimatePanel.tokens.csshas no semantic--warntoken; introducing one is wider than this PR. A comment marks the intent.Math.round(0.996 * 100)renders "100%" (af-reviewer) — real, but clamping the display would misreport the model's actual output. The permanent "Advisory only" note is the mitigation.Two standing observations, neither introduced here:
frontend/vite.config.ts:136setsthresholds(90 lines/functions/statements, 80 branches) andnpm run test:coveragefails below them. But itsincludelist scopes coverage to pure.tsmodules plus a couple of named components, so the new.tsxcomponents sit outside the percentage entirely. Their guarantee is the 19 dedicated tests, not the headline number.ApprovalPredictionBadgehand-rolls theaf-pillclasses rather than using thePillprimitive, following the precedent set byAnomalyBadge.