Add Freedom to Vote test and configurable competitiveness metrics to the evaluation panel - #723
Add Freedom to Vote test and configurable competitiveness metrics to the evaluation panel#723fangge518 wants to merge 32 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🧹 Preview torn downPreview resources for this PR have been destroyed (preview label removed). |
83797ac to
9c9d2f6
Compare
9c9d2f6 to
e4d53ea
Compare
Surfaces the S.2747 FTV redistricting test on the evaluation panel: a
plan passes if its disproportionality stays within max(7%, 1/k) for at
least 3 of the 2 most recent Presidential and 2 most recent Senate
elections. Computed entirely from data the API already returns (no
backend change) since election columns follow a uniform
{pres|sen}_{yy}_{dem|rep} naming convention across all states.
States without both election pairs (e.g. DC, which has no full voting
Senator) show "Not applicable" instead of a scored verdict.
The dedicated Freedom to Vote Test table duplicated data already shown in the Proportionality table above it. Replaced with a single sentence appended to Other Partisanship Metrics' intro paragraph, computing the same pass/fail verdict inline. A HelpTip on "Freedom-to-Vote test" carries the methodology detail and cites S.2747 as proposed but not enacted legislation, plus the Duchin/Schoenbach paper it's drawn from.
Match the dictated phrasing verbatim (Freedom-To-Vote capitalization, "absolute value of disproportionality", "larger than or equal to 3") instead of the earlier paraphrase.
…d text Hovering "the 4 recent statewide elections" in the FTV sentence now outlines the corresponding Disproportionality cells in both the Proportionality table and the Other Partisanship Metrics table, so a reader can see exactly which rows the sentence refers to. The Freedom-To-Vote test HelpTip trigger switches from an info icon to the same underlined-dotted text style, matching BasicsSection's existing hover-highlight convention.
<button> defaults to inline-block with nowrap text, invisible on BasicsSection's short single-token labels but breaking layout for the longer "the 4 recent statewide elections" trigger. Override to display: inline / white-space: normal so it wraps like the surrounding paragraph text.
Hovering "N election(s)" now highlights just the subset of the 4 FTV rows that actually pass the threshold, distinct from hovering "the 4 recent statewide elections" (all 4). Handles singular/plural. Also removed fontWeight: bold from HOVER_BTN_STYLE — the underline already signals hoverability, and these triggers explain rather than emphasize.
Radix's own HoverCard close logic refuses to schedule its close timer once any text inside the card has been selected (hasSelectionRef in its source), and that flag only clears on a fresh pointerdown inside the content. Combined with our own onPointerDownOutside preventDefault (added earlier to stop a click-to-reopen bug), there was no path left to close the card after copying its text and moving the pointer away. Fix: HelpTip now owns closing the same way it already owns opening — its own pointerenter/pointerleave timer on both the trigger and the content, fully independent of Radix's internal selection-tracking state.
…lose timer The previous fix (own close timer on pointerleave) closed the stuck-open bug but also broke Radix's legitimate "don't interrupt an active text selection" protection, making the card's text painful to select/copy. Reverted that timer entirely. Fix instead: HoverCard's onPointerDownOutside now dismisses on any outside click except one that lands back on the trigger itself (preserving the original click-to-reopen guard) — giving a guaranteed manual escape hatch out of Radix's "stuck open after a selection" state, without touching the hover-based auto-close at all.
BasicsSection and PartisanSection each defined their own HOVER_BTN_STYLE with five identical properties (background, border, padding, font, textDecoration) and only cursor/fontWeight/wrap behavior actually differing per use case. Factored the shared base into a new file; each caller layers its own overrides on top.
…lt cursor HOVER_BTN_STYLE is now a single constant (default cursor, no bold) used directly everywhere; bold is merged in per instance instead of being baked into per-file variants. Triggers naming a specific result value (District N, N election(s)) stay bold; triggers introducing or explaining a concept (Freedom-To-Vote test, the 4 recent statewide elections) don't. The wrap fix for the one long trigger phrase moves to its own WRAPPING_HOVER_BTN_STYLE, layered on the shared base.
The two branches of the FTV sentence (scored vs not-enough-data) each rendered an identical HelpTip trigger; hoisted it to one ftvHelpTipTrigger element and reused it, and split the ternary into two ftvPassCount checks. Also dropped WRAPPING_HOVER_BTN_STYLE — it didn't actually fix wrapping in practice, so the "the 4 recent statewide elections" trigger now just uses the plain shared HOVER_BTN_STYLE like the others.
…shares New metric alongside the existing competitiveness aggregate (untouched, same fixed ±3% band). Returns dem_sweep_districts/rep_sweep_districts/ swing_districts as district-ID lists (independent of any threshold — sweep/swing is just plain-majority win/loss per election) and a flat sorted contest_dem_vote_shares list (one entry per district x election pair) for the frontend to apply any competitiveness band to without duplicating classification logic server-side. Cross-checked against the same grid_context ground truth competitive_metrics is validated against, plus a hypothesis fuzz test mirroring the existing one for competitive_metrics.
onDistrictEnter now takes (number | string)[] instead of a single zone — a single district is just the one-element case. All four existing call sites (BasicsSection x3, CompactnessSection x1) wrap their zone argument in an array. Enables highlighting a whole category of districts (swing, sweep) at once, not just one. Also adds the competitive_districts field to DocumentEvaluation, matching the new backend metric.
…tead Reverts the previous approach of adding a second, parallel metric. competitive_metrics (key "competitiveness") now returns the district-ID lists and sorted contest-share array directly, replacing its old fixed ±3%-band aggregate counts. version bumped 1 -> 2 since the output shape changed, per registry.py's own documented rule for when CURRENT_PAYLOAD_VERSION should flip. Existing tests updated to match: the two ground-truth constants (originally generated against gerrychain/gerrytools reference values) are unchanged as plain reference numbers, just no longer literal CompetitiveMetrics instances since that shape moved; assertions check cardinalities and the ±3% contest count via a shared helper instead of dict equality.
Matches the backend's reshaped competitive_metrics: one field (competitiveness), not two. PartisanSection.tsx still reads the old n_* aggregate fields — fixed in the next commit alongside the competitiveness threshold picker and hover-highlight.
…eshold + hover
- FTV verdict sentence replaced with a paragraph + per-election table
(R vote share / R seat share / close-enough verdict), closing with the
pass-count sentence. Threshold phrasing now states whichever bound
actually binds ("one out of k seats (X%)" vs "7%").
- All FTV hover triggers (HelpTip, "the 4 recent statewide elections",
"N election(s)") switched from <button> to <span role="button">,
matching HelpTip's own default-trigger pattern — buttons were
interrupting text selection/copy on the paragraph.
- Competitiveness section: user-configurable band (±2/3/5/7/10, default
3, matching the old fixed backend behavior) re-filters
contest_dem_vote_shares client-side. Swing/Safe Dem/Safe Rep rows
hover-highlight their actual districts on the map via the generalized
useDistrictHover.
…ing again Dropping these in favor of deriving them frontend-side (summing the three district lists, reusing another metric's election count) turned out to have two real divergence risks: the zero-elections branch would report n_districts=0 even when real districts exist with no election data, and the frontend's election count came from a differently prefix-filtered source than this metric's own context.elections. Metrics also fail independently (MetricsEnvelope.failed), so leaning on a different metric's data for this one's own labels was a real coupling, not just a style choice. n_districts/n_elections computed directly from context, not derived from the lists, so they stay correct in the zero-elections edge case. Frontend now reads them from competitiveness itself. Version stays at 2 (not yet merged, nothing to invalidate). New test locks in the zero-elections edge case explicitly.
Dropped the "found in the political science literature..." sentence entirely. Rewrote the FTV explanation to the human's exact dictated wording rather than a paraphrase: PASSES/DOES NOT PASS in caps, the hover trigger relabeled to "the last two Senate races and the last two Presidential races", the four contests spelled out in a parenthetical right after, and the bound split into its own sentence as "1 out of k seats (X% of the seat share)".
Hovering "the last two Senate races..." (or the elections-passed count) no longer highlights the new FTV table's own rows — that table is already scoped to exactly those 4 elections, so highlighting it added nothing. Added the highlight to the Proportionality table's Vote Share and Seat Share cells instead, alongside the existing Disproportionality cell, since the FTV table shows exactly those two figures (as R vote/ seat share) and the reader can now see where they came from.
"This is close enough N out of 4 times, so it passes/does not pass the test. (3 out of 4 are needed to pass.)" — the hover trigger now wraps "N out of 4 times" instead of "N election(s)".
"...(specifically, whether absolute disproportionality stays within max(7%, 1/number of seats))..." — states the actual pass criterion, not just what it's testing for.
…y-update feat/static-copy-update's intro paragraph hardcoded "47-53" as the competitive band, written before this branch's threshold picker existed. Changed to defer to the picker instead of stating a fixed number that the picker can now change.
The dropdown sat between two separate <Text as="p"> blocks, so it rendered on its own line. Merged into one paragraph with the Select as an inline child instead, matching the human's intent to have it follow the sentence directly. Also fixed a doubled "competitive competitive" left over from the in-progress edit.
Each of the 4 parenthetical election keys is now its own hover trigger (hoveredFtvKey state, reusing isFtvHighlighted) — hovering one highlights just that row across the other tables, instead of only the all-4/all-hovered states already available via the two sentence-level triggers. Also added the missing "and" before the last item: "(2024 Pres, 2020 Pres, 2020 Sen, and 2016 Sen)".
Same hoveredFtvKey trigger as the parenthetical election list — hovering a column header highlights that election's row in the Proportionality and Other Partisanship Metrics tables (not this table's own cells, consistent with the earlier call that this table shouldn't self-highlight).
Reverses the earlier "always Republican" convention: repVoteShare/ repSeatShare become povVoteShare/povSeatShare, returning the Dem share directly when pov is dem, 1-share when rep. Row labels switch between "D"/"R" to match. The "Repub tilt"/"Dem tilt" verdict wording is unaffected — it names the actually-favored party regardless of pov, not a pov-relative framing, so nothing there needed to change.
e4d53ea to
eeb8661
Compare
nofurtherinformation
left a comment
There was a problem hiding this comment.
A few non-blocking notes from me, but otherwise looks clean!
Full agentic notes below (new skill version is a bit more plain language, I'm hoping)
Summary
PR Review: Add Freedom to Vote test and configurable competitiveness metrics to the evaluation panel (#723)
Verdict: APPROVE WITH COMMENTS
Overview
This PR adds a Freedom-To-Vote proportionality test to the evaluation panel's partisan section, reshapes the backend competitiveness metric to return district-id lists plus a flat list of per-contest vote shares so the frontend can pick its own competitiveness band, generalizes the district hover hook to take a list of districts, and fixes a pre-existing HelpTip bug where the card stayed open forever after copying its text. It does what the description says, and I found nothing in the diff that the description leaves out.
Verified by execution: CI is green, the partisan test file passes locally (30 pass; the 3 errors are database-only tests untouched here), TypeScript, ESLint, Prettier, and ruff at the repo's pinned version are all clean on the changed files. I also confirmed against the dev database that every state's election columns use the two-digit-year naming the new selectFtvElections helper depends on. The test reviewer went further and ran mutation tests against the new backend code.
Good work worth calling out: the backend sweep and swing computation now aligns on the pandas index instead of relying on positional arrays, which removes a hidden ordering assumption. The frontend degrades gracefully if it receives an old-shape payload. The HelpTip fix is narrowly scoped and its long comment accurately describes Radix's internal selection guard.
Headline risks, none blocking. First, if the backend's disproportionality metric fails for a plan (it raises on zero votes, and failures are isolated per metric), the page shows a confident "DOES NOT PASS" instead of "not enough data". Second, the new sweep lists, which drive map highlighting, have no test that checks which districts land in which list: swapping the Dem and Republican lists passes every test. Third, two small rendering and accessibility slips in the new table. All four Important items are cheap to fix and independent of each other, so this is Approve With Comments rather than Request Changes.
Severity summary
| Severity | Count |
|---|---|
| High | 0 |
| Important | 4 |
| Personal preference | 5 |
| Opinion | 3 |
Inline notes
app/src/app/components/EvalPanel/PartisanSection.tsx
- L112
IMPORTANT[correctness]A missing disproportionality value is scored as a failed contest, producing a false "DOES NOT PASS" —ftvPassingKeyskeeps only keys wheredisprop !== undefinedand within bound, so a key withseatsbut nodisproportionalitycounts as a failure. If the backend's disproportionality metric fails for this plan (it raisesZeroDivisionErroron an election with zero D+R votes, andbackend/app/evaluation/main.pyisolates per-metric failures soseatsstill arrives), the paragraph says "DOES NOT PASS" and "0 out of 4 times" while every verdict cell shows a dash. Fix: setftvPassCountto null when any of the four FTV keys lacks a disproportionality value, so the existing "Not enough data" sentence renders instead. - L738, L755, L772
IMPORTANT[repo-coherence]HOVER_BTN_STYLEun-bolds the three district-count cells — the shared style's inlinefont: 'inherit'resets font-weight and beats Radix's class-basedweight="bold", so the Swing, Dem Sweep, and Repub Sweep counts render at normal weight while the two bold count cells above them do not. Verified from Radix's stylesheet. BasicsSection and line 507 of this file already avoid this with{...HOVER_BTN_STYLE, fontWeight: 'bold'}. Same fix here, or split the button-only reset keys out of the shared constant. - L142, L381, L396, L419, L505
IMPORTANT[a11y]Five<span role="button">triggers announce as buttons but do nothing when activated — noonClick, no Enter or Space handler, no label describing the highlight. BasicsSection uses a real<button type="button">for the identical hover-to-highlight affordance, andDistrictLabel.tsxonly setsrole="button"when there is a click handler. Radix ThemesHoverCard.TriggerisasChild, so a<button>child works inside HelpTip too. If the plain<button>was interrupting text selection, that is likely fixable withuserSelectrather than by dropping the element. Alternatively droprole="button", keeptabIndex={0}, and addaria-describedbyortitle. - L191
PREF[correctness]Band filter drops exact-boundary shares because of float subtraction —Math.abs(s - 0.5) <= 0.03is false fors = 0.53(0.53 - 0.5is0.030000000000000027). Verified in node for bands 2, 3, and the top of 5. The old backend and this PR's own test helper use inclusive0.47 <= s <= 0.53. Suggests >= 0.5 - band && s <= 0.5 + band. Needs an exact percentage share to matter, so rare. - L728, L745, L762
PREF[repo-coherence]cursor: pointeron rows that do nothing when clicked — CompactnessSection rows use pointer because clicking zooms;CountySplitsSection.tsxis the hover-only analogue and usesdefault. The inner text inheritscursor: defaultfrom the shared style, so the cursor changes shape across a single row. Eithercursor: 'default', or add a click that zooms to the group. - L79
PREF[repo-coherence]sortElectionsandLEVEL_ORDERlive in the component while their sibling helper went toutils/elections.ts— see the note onelections.tsbelow. Would you consider moving them together? - L108
OPINION[correctness]Partially drawn plans get an unqualified PASS or DOES NOT PASS —kisseats.total, which is the count of non-empty districts. A plan with 3 of 14 districts drawn gets a 33% bound. Every other partisan metric already behaves this way, but this is the first one shown as a binary legal verdict. BasicsSection already computes completeness if you want to gate the sentence. Product call, no change requested. - L157
OPINION[correctness]Rep-POV seat share is1 - dem/total, which differs from the Proportionality table'srep/totalwhen a district ties — the FTV form is actually the one consistent with the verdict math, and exact ties are essentially nonexistent in precinct data. Noting only because two adjacent tables can disagree on the same number. No change requested.
backend/tests/test_partisan.py
- L433 (helper), L377 and L605 (expected counts), L1008 (fuzz)
IMPORTANT[tests]No test pins which districts land in the Dem-sweep and Rep-sweep lists — arguably High. Both gerrychain fixtures expect 0 Dem-sweep and 0 Rep-sweep districts, so the cardinality checks are0 == 0regardless of which party a list is labeled. Mutation runs confirmed: swapping the two output lists, swappingdem_winsandrep_wins, and disabling sweep detection entirely all pass all four competitiveness tests. These lists are what the map now highlights. Cheap fix: a 3 or 4 district, 2 election_StubEvaluationContextfixture (the zero-elections test at line 466 is the template) asserting exact lists such asdem_sweep_districts == [1],rep_sweep_districts == [2],swing_districts == [3, 4]. In the fuzz test, build the oracle fromctx.dem_winsand assert set equality. - L452, L1024
PREF[tests]contest_dem_vote_sharesparty orientation and exact contents are unverified — the assertions are a symmetric 47 to 53 count, range, and sortedness, so emitting Rep share instead of Dem share passes. The frontend filter is symmetric today, so nothing would break, but the field is documented as Dem share. The deterministic fixture above can assert the exact list, and the fuzz test can compare againstsorted(d / t for ... if t > 0)instead of the<= n * n_ebound. That also pins the zero-vote mask directly instead of through an accidental NaN range failure.
app/src/app/utils/elections.ts
- L24
PREF[repo-coherence]Third hand-rolled parser of the election key, with a different year index —formatElectionKeyandsortElectionstake the year from the last_part;selectFtvElectionsusessplit('_')[1]. Identical today because every key is two-part (confirmed against the dev database). A smallparseElectionKeyreturning{level, year}used by all three would remove the latent divergence. - L18
OPINION[tests]No unit test forselectFtvElectionsor the FTV scoring, but the app has no unit-test harness —package.jsonhas only Playwright e2e scripts and there are no*.test.tsfiles outsidee2e/. Out of scope for this PR. Worth a follow-up issue: a minimal vitest or jest config, starting with this helper and an extracted pure FTV scoring function.
app/src/app/components/HelpTip/HelpTip.tsx
- L192
PREF[correctness]cloneElement({ref: triggerRef})replaces any ref the caller put on the trigger child — under React 18.3 this overwrites the child's own ref. All 20 current call sites pass DOM elements or forwardRef Radix components with no ref of their own, so nothing breaks today. A future<HelpTip><Button ref={x} /></HelpTip>would silently losex. Composing refs the way Radix Slot does is a two-line change.
Verified clean
- FTV rule matches the description: bound is
max(7%, 1/k), inclusive, pass at 3 of 4, and the "1 out of k seats" wording appears exactly when1/kexceeds 7%. selectFtvElectionsreturns null unless both election pairs exist, and whenever it is non-null the district count is defined.- The inline
Selectinside a paragraph produces valid HTML; Radix Select renders phrasing content and portals its dropdown. useDistrictHoverhandles a null map, an empty district list, and an old-shape payload without error.- HelpTip's new outside-click exemption targets the same DOM node Radix positions against, and only pointerdown behavior changed.
- Backend: the pandas boolean Series share one index so alignment is exact, the three lists partition the districts, the zero-elections branch never touches
dem_wins, and the version bump invalidates cached evaluation rows. - Zone ids serialize as plain Python ints (checked by execution), so the JSON payload is safe.
backend/tests/test_fl_metrics_integration.pydoes not reference the competitiveness metric, so the registry docstring's manual-run note is not made stale.- No security surface changed: no auth, SQL, secrets, or new input parsing.
Details
PR Review Findings: Add Freedom to Vote test and configurable competitiveness metrics to the evaluation panel (#723)
Reviewed 2026-09-03 against dev at 8af55df5, head at eeb8661b.
Scope and method
- 674 reviewable lines across 13 files (9 frontend, 4 backend), medium tier. No generated or vendored files in the diff.
- Review factors: correctness, test coverage, repo coherence. Security was not given its own reviewer: the diff adds no auth, SQL, secrets, or new input parsing. All rendered strings are React text nodes fed from the existing evaluation API. Verified clean by the lead reviewer.
- Three focused reviewers ran in parallel, one per factor. Every finding below was re-checked by the lead reviewer against the full files before being kept.
Verification runs
gh pr checks 723→ all 3 checks pass (AWS preview, pipeline tests, container-job).backend/.venv/bin/pytest tests/test_partisan.py -qwith.env.testloaded → 30 passed, 3 errors. The 3 errors are database-backed Eguia and county tests that need a live Postgres and are untouched by this PR. All competitiveness tests pass. CI runs the same file green.docker compose exec backend pytest ...→ could not run. The running dev container is missingprometheus_fastapi_instrumentator, a dependency added torequirements.txtin June. The container image is stale. Not related to this PR.bun run ts(tsc) → no errors insrc/. The only errors are in stale generated files under.next/typesthat reference pages not on this branch. Not related to this PR.bunx eslint <9 changed files>→ clean.bunx prettier --check <9 changed files>→ clean.uvx ruff@0.3.4 checkandruff@0.3.4 format --check(the version pinned in.pre-commit-config.yaml) on the 4 changed backend files → clean. Latest ruff reports import-order and assert-formatting differences, but those also exist ondevand are a ruff version drift, not this PR.- Read-only query of the dev database's
gerrydbschema → every statewide election column is named{pres|sen|gov|ag}_{yy}_{dem|rep}with two-digit years. TheselectFtvElectionsyear sort is safe on real data. - Pandas Index iteration check → iterating
df.index[mask]yields Pythonint, notnumpy.int64. The zone-id lists serialize to JSON without a custom encoder. This falsified an initial suspicion. - Mutation testing (test reviewer, scratchpad-only pytest plugin, no repo files changed) on
pytest tests/test_partisan.py -k competitive:
| Mutant | Result |
|---|---|
| baseline | 4 pass |
| swap dem and rep sweep output lists | 4 pass (survives) |
swap dem_wins and rep_wins in the loop |
4 pass (survives) |
| disable sweep logic (every district becomes swing) | 4 pass (survives) |
emit Rep share instead of Dem share in contest_dem_vote_shares |
4 pass (survives) |
drop the valid zero-vote mask |
1 fail (fuzz range check catches NaN) |
invert swing |
3 fail |
node -efloat check →0.53 - 0.5 <= 0.03isfalse,0.47 - 0.5 >= -0.03isfalse,Math.abs(0.55 - 0.5) <= 0.05isfalse.
Factor: correctness (reviewer scope: full diff)
Findings
- [correctness] Missing disproportionality value renders as a failed FTV verdict, not "not enough data" —
app/src/app/components/EvalPanel/PartisanSection.tsx:112[proposed: Important] [confidence: verified code path, likely in practice]ftvPassingKeysfilters ondisprop !== undefined && Math.abs(disprop) <= ftvThreshold. A test election that has aseatsentry but nodisproportionalityentry counts as a failure. If all four are missing the page prints "This plan DOES NOT PASS" and "0 out of 4 times" while every verdict cell shows a dash. Reachable: the backenddisproportionalitymetric raisesZeroDivisionErrorwhen any election has zero D+R votes in the assigned area (backend/tests/test_partisan.py:898pins this), andbackend/app/evaluation/main.py:154catches per-metric failures soseatsstill arrives. Fix: setftvPassCountto null when any FTV key lacks a disproportionality value, so the existing "Not enough data" branch renders. - [correctness] Competitive-band filter drops exact-boundary vote shares due to float subtraction —
app/src/app/components/EvalPanel/PartisanSection.tsx:191[proposed: Personal preference] [confidence: verified]Math.abs(s - 0.5) <= bandfails fors = 0.53at band 0.03 because0.53 - 0.5is0.030000000000000027. Verified in node for bands 2, 3, and the upper end of 5. The old backend and the PR's own test helper use inclusive0.47 <= s <= 0.53. Fix:s >= 0.5 - band && s <= 0.5 + band. Rare in practice, needs an exact percentage share. - [correctness] FTV grades partially drawn plans as if complete —
app/src/app/components/EvalPanel/PartisanSection.tsx:108[proposed: Opinion] [confidence: verified mechanics, uncertain intent]kisseats.total, which isnum_nonempty_districts. A plan with 3 of 14 districts drawn gets a 33% bound and an unqualified "PASSES". This matches how every other partisan metric already behaves, but FTV is the first one shown as a binary legal verdict. BasicsSection already computes completeness. Product decision, no change requested. - [correctness]
HOVER_BTN_STYLE'sfont: inheritcancelsweight="bold"on the three district-count cells —app/src/app/components/EvalPanel/PartisanSection.tsx:738[proposed: Personal preference by this reviewer; raised to Important after the coherence reviewer confirmed the cascade] See the repo-coherence section. - [correctness]
cloneElement({ref})overwrites any ref a caller puts on the HelpTip trigger child —app/src/app/components/HelpTip/HelpTip.tsx:192[proposed: Personal preference] [confidence: verified] In React 18.3cloneElementreplaces the child's ref. All 20 current call sites pass DOM elements or forwardRef Radix components with no ref, so nothing breaks today. A future<HelpTip><Button ref={x} /></HelpTip>would silently losex. Fix: compose refs the way Radix Slot does. - [correctness] Rep-POV seat share in the FTV table is
1 - dem/total, notrep/total—app/src/app/components/EvalPanel/PartisanSection.tsx:157[proposed: Opinion] [confidence: verified] With a tied district the FTV table's R seat share differs from the Proportionality table's directly above byties/total. The FTV form is actually the one consistent with the verdict math. Exact ties are essentially nonexistent in precinct data. No change requested.
Verified clean
- FTV rule matches the stated test:
Math.max(0.07, 1/k), inclusive<=, pass at 3 of 4.ftvBoundPhraseswitches to the "1 out of k seats" wording exactly when1/k > 0.07(k of 14 or fewer). ftvVerdicttilt direction is only reached when|disprop| > threshold > 0, so no sign ambiguity at zero.selectFtvElectionsreturns null unless both pairs exist; whenever it is non-null,n >= 4andnumDistrictsis defined, becausepresandsenare both inLEVEL_ORDER.povVoteShare1 - demequalsvote_shares.repbecause the backend computes both over the same D+R total.- Inline
Select.Rootinside<Text as="p">: Radix Select renders a<button>plus a hidden native<select>, both phrasing content. Content is portaled. No invalid nesting, no hydration mismatch. - All hooks run before the early
return null. useDistrictHover: null map on enter leavesprevRefuntouched; leave always resets it; empty zones array clears previous highlights and sets none. A v1 backend payload degrades gracefully through the?? []and?? 0defaults.- HelpTip:
event.detail.originalEvent.targetmatches Radix DismissableLayer's event shape. Radix ThemesHoverCard.TriggerisasChild, so Slot composes refs andtriggerRefpoints at the same DOM node Popper measures.contains()handles nested icons. Focus-outside is already prevented by Radix HoverCard, so only pointerdown behavior changed. - Backend:
pd.Series(True, index=zones)anddem_wins[e]sharedemographic_data.index, so&=alignment is exact.~(dem | rep)partitions the zones. The zero-elections branch returns before touchingdem_winsand reports the realn_districts.n_districtsand the sum of the three lists both derive from rows withzone is not None; the fuzz test asserts they agree. Version bump to 2 flipsCURRENT_PAYLOAD_VERSIONand invalidates cached rows. - Competitive-contest denominator is now the count of valid contests (zero-vote pairs excluded), consistent with the fuzz bound.
- Pandas Index iteration yields Python ints, so no numpy JSON serialization issue.
Highlights
- Moving band classification client-side over a sorted flat list is a clean decoupling.
- The frontend can deploy ahead of the backend without crashing on a v1 payload.
- The HelpTip doc comment accurately describes Radix's
hasSelectionRefbehavior, and the trigger exemption is scoped to the one case that needs it.
Factor: tests (reviewer scope: backend tests, frontend logic)
Findings
- [tests] Sweep classification is not pinned by any test; party swap survives every test —
backend/tests/test_partisan.py:433(helper),:377and:605(expected counts),:1008(fuzz) [proposed: High by reviewer; lead sets Important, arguably High] [confidence: verified by executed mutants] Both gerrychain fixtures expect 0 Dem-sweep and 0 Rep-sweep districts, so the cardinality checks are0 == 0whichever party a list is labeled. A mutant that never marks any sweep still yields 8 swing districts. The fuzz test asserts partition and disjointness only, which any three-way split satisfies. The specific district ids indem_sweep_districtsandrep_sweep_districtsare what the frontend highlights on the map, and no assertion checks them. Fix: add a 3 or 4 district, 2 election_StubEvaluationContextfixture (the zero-elections test at line 466 is the template) and assert exact lists, for exampledem_sweep_districts == [1],rep_sweep_districts == [2],swing_districts == [3, 4]. In the fuzz test, compute the oracle independently fromctx.dem_winsand assert set equality. - [tests]
contest_dem_vote_sharesparty orientation is unverified —backend/tests/test_partisan.py:452and:1024[proposed: Important] [confidence: verified] Assertions are a symmetric 47 to 53 count, range, and sortedness. Emitting the Rep share instead passes all of them. The frontend filter is also symmetric today, so nothing would notice, but the field is documented as Dem share. Fix: assert the exact list in the deterministic fixture above, and in the fuzz test replace the<= n * n_ebound with an exact oracle built fromctx.dem_votesandctx.total_voteswhere total is greater than 0. That also pins thevalidmask directly instead of through the accidental NaN range failure. - [tests] Fuzz length assertion is a bound where an exact value is one line —
backend/tests/test_partisan.py:1024[proposed: Personal preference] [confidence: verified] Subsumed by the previous finding. - [tests] No unit tests for
selectFtvElectionsor the FTV scoring, but the app has no unit-test harness —app/src/app/utils/elections.ts:18,app/src/app/components/EvalPanel/PartisanSection.tsx:112[proposed: Opinion] [confidence: verified]app/package.jsonhas only Playwright e2e scripts.jestand testing-library are devDependencies but there is no config and no*.test.tsfiles outsidee2e/. The e2e suite only checks that the Evaluate view opens. Demanding a harness is out of scope for this PR. Suggest a follow-up issue.
Verified clean
backend/tests/test_fl_metrics_integration.pydoes not referencecompetitive_metricsor any old field name. The registry docstring's "run the FL suite on version bump" note is not made stale by this change.- No other backend test references the competitiveness metric, so nothing else was silently weakened.
- The replacement helper still asserts every piece of the old recorded ground truth (both counts, all three cardinalities, and the 47 to 53 count recomputed from the flat list). Nothing from the old oracle was dropped.
- The new zero-elections test pins
n_districts == 5with empty lists, a real gap in the old code. - The fuzz strategy generates zero-vote pairs, so the
validmask path is exercised. Theswinginversion mutant is caught.
Highlights
- Keeping the gerrychain counts as ground truth and recovering the old 47 to 53 count from the new flat list is exactly the cross-check that justifies moving band selection to the client.
Factor: repo coherence (reviewer scope: full diff, sibling files)
Findings
- [repo-coherence]
font: inheritin the shared style un-bolds the three hoverable competitiveness cells —app/src/app/components/EvalPanel/PartisanSection.tsx:738,:755,:772;app/src/app/components/EvalPanel/hoverTriggerStyle.ts:12[proposed: Important] [confidence: verified fromnode_modules/@radix-ui/themes/styles.css] Radix appliesweight="bold"through the class.rt-r-weight-bold. An inlinefont: inheritshorthand resets font-weight to the parent's normal weight and wins over any class. Font size and line height happen to come out identical because the table cell already uses--font-size-2. Result: the Swing, Dem Sweep, and Repub Sweep counts render at regular weight while the "Elections analyzed" and "Competitive contests" counts two rows up render bold. The PR already knows the footgun: BasicsSection lines 186 and 199 and PartisanSection line 507 spread{...HOVER_BTN_STYLE, fontWeight: 'bold'}for this reason. Fix: same spread on these three, or split the button-only reset keys out of the shared constant. - [a11y] Five
<span role="button">triggers have no activation behavior, no keyboard handler, and no label —app/src/app/components/EvalPanel/PartisanSection.tsx:142,:381,:396,:419,:505[proposed: Important] [confidence: verified]role="button"promises Enter or Space activation. A screen-reader user hears "button", presses it, and nothing happens. In-repo precedents: BasicsSection uses a real<button type="button">for the identical hover-to-highlight affordance (and this PR edits those).DistrictLabel.tsxsetsroleonly when there is anonClick.DistrictMeters.tsxhas a comment decliningrole="button"where it would be invalid ARIA. The one existing<span role="button">in HelpTip carriesaria-label. Radix ThemesHoverCard.TriggerisasChild, so a<button>child works with HelpTip too. The PR description says a plain<button>interrupted text selection; that may be auser-selectissue on the button rather than a reason to drop the element. Fix: use<button type="button">like BasicsSection, or droprole="button"and addaria-describedbyortitleexplaining the highlight. - [repo-coherence] Third hand-rolled parser of the election key, with a different year index —
app/src/app/utils/elections.ts:24;app/src/app/components/EvalPanel/PartisanSection.tsx:79[proposed: Important by reviewer; lead sets Personal preference] [confidence: verified]formatElectionKeyandsortElectionstake the year as the last_part;selectFtvElectionstakessplit('_')[1]. Identical today because every key is two-part.sortElectionsandLEVEL_ORDERlive in the component while the sibling helper went toutils/elections.ts. Fix: a smallparseElectionKeyinutils/elections.tsused by all three, optionally movingsortElectionsthere too. - [repo-coherence]
cursor: pointeron rows that do nothing when clicked —app/src/app/components/EvalPanel/PartisanSection.tsx:728,:745,:762[proposed: Important by reviewer; lead sets Personal preference] [confidence: verified] CompactnessSection rows usecursor: pointerbecause they haveonClickto zoom.CountySplitsSection.tsx:310is the hover-only analogue and usescursor: default. The innerTextinheritscursor: defaultfromHOVER_BTN_STYLE, so the cursor changes shape moving across one row. Fix:cursor: default, or add a click that zooms to the group. - [repo-coherence]
cast(DistrictId, z)wraps numpy scalars —backend/app/evaluation/partisans.py:284[proposed: Personal preference] DROPPED by lead. Iterating a pandas Index yields Pythonint, verified by execution. The values match how sibling call sites produceDistrictId.
Verified clean
- Switching from positional numpy arrays to index-aligned pandas Series is more coherent with the rest of
partisans.py, wheremean_medianandpartisan_biasalready operate on the context's Series. The version bump follows existing precedent for reshaped metrics. hoverTriggerStyle.tsplacement is reasonable: the repo has no shared-styles location, and the two consumers are both inEvalPanel/.var(--green-9)for pass matchesMapValidation.tsx.var(--accent-9)for the FTV outline matches other Radix token usage in EvalPanel.DEMandREPliterals are pre-existing in this file.useDistrictHoverarray generalization is minimal and all callers were updated; grep found no stragglers.- Inline
Select.Root size="1"matches existing usage in BasicsSection and CountySplitsSection. - HelpTip change matches the file's heavily commented style, and the comment describes the new behavior accurately.
helpTipContent.freedomToVoteTestmatches the entry shape andsatisfiesusage of its neighbors.getEvaluation.tsmirrors the backend TypedDict field for field, as the other result types do.
Highlights
- Index-aligned sweep computation removes a hidden ordering assumption in the backend.
- Extracting
ftvHelpTipTriggeronce avoids duplicating the HelpTip wiring across both sentence branches. - The
ftvBoundPhraselogic and comment ("state the bound in whichever form is binding") are well explained.
Lead reviewer notes on severity calls
- Sweep-membership test gap: the reviewer proposed High. The HIPPO definitions put missing tests for new behavior at Important, and the production code was verified correct by a separate reviewer. Set to Important, arguably High, because it is the PR's primary new output and a party swap would ship silently.
- Un-bolded cells: one reviewer said Preference, one said Important. Set to Important because it is a visible inconsistency inside a five-row table with a two-token fix, and the shared constant will keep defeating
weightprops. - Election-key parser duplication and
cursor: pointer: downgraded from Important to Personal preference. Both are real but small, and the author may reasonably defer them. - Numpy
castfinding dropped after execution showed Python ints.
Live environment log
Read-only only. One query against the local dev Postgres container:
select column_name, count(*) from information_schema.columns
where table_schema = 'gerrydb' and column_name ~ '^(pres|sen|gov|ag)_'
group by 1 order by 1
Result: all statewide election columns follow {type}_{yy}_{dem|rep} with two-digit years.
| ), | ||
| Metric[CompetitiveMetrics]( | ||
| key="competitiveness", version=1, compute=partisans.competitive_metrics | ||
| key="competitiveness", version=2, compute=partisans.competitive_metrics |
There was a problem hiding this comment.
Just confirming, will this invalidate old cached metrics so they are recalculated on the next fresh request?
There was a problem hiding this comment.
Yes. All metrics will be recomputed for any version bump of any metric.
| dem_districts = np.logical_and(dem_districts, context.dem_wins[election]) | ||
| rep_districts = np.logical_and(rep_districts, context.rep_wins[election]) | ||
| dem_sweep &= context.dem_wins[election] | ||
| rep_sweep &= context.rep_wins[election] |
There was a problem hiding this comment.
Nice syntax :)
|
|
||
| const ftv = selectFtvElections(Object.keys(evaluation.seats ?? {})); | ||
| const ftvThreshold = ftv && numDistricts ? Math.max(0.07, 1 / numDistricts) : null; | ||
| const ftvPassingKeys = |
There was a problem hiding this comment.
I: A test election with seats but no disproportionality value is counted as a failure rather than "not enough data". This is potentially reachable because the backend isolates per-metric failures and that metric raises on zero votes. But in practice, could a 0 disproportionality case exist?
There was a problem hiding this comment.
Added check for invalid responses.
|
|
||
| // Shared between both branches of the FTV sentence below (scored and | ||
| // not-enough-data) so the HelpTip trigger isn't duplicated. | ||
| const ftvHelpTipTrigger = ( |
There was a problem hiding this comment.
PP: This could be moved out of the PartisanSection component (nothing stateful here) and span does lack some a11y if we can use a different element here
| <Table.Cell justify="center"> | ||
| <Text size="2" weight="bold"> | ||
| {competitiveness.n_swing_districts} / {competitiveness.n_districts} | ||
| <Text size="2" weight="bold" style={HOVER_BTN_STYLE}> |
There was a problem hiding this comment.
I: I think the style will clobber the weight="bold" here. style={...HOVER_BTN_STYLE, fontWeight: 'bold'} might be needed
There was a problem hiding this comment.
Good catch. Fixed.
| <Table.Row> | ||
| <Table.Row | ||
| tabIndex={0} | ||
| style={{cursor: 'pointer'}} |
There was a problem hiding this comment.
O: Why pointer?
There was a problem hiding this comment.
Good catch. Changed to 'default'.
Per Dylan's feedback: swap the highlight line back to a vivid orange (replacing the neon magenta tried on this branch), and dim every non-hovered district while one is focused, same masking convention as the paint-mask overlay in CountyLayers.tsx. Needed a small reactive store (districtHoverStore) mirroring useDistrictHover's feature-state writes, since the dim mask layer must know whether anything is currently hovered to gate itself on/off. Exploratory on this branch pending visual confirmation; folds into this PR if it looks good.
…mask opacity Per review: a dedicated Zustand store for one piece of hover state wasn't justified when useDistrictHover already depends on mapStore, the established general-purpose reactive container for map state. Moved hoveredZones there as hoveredPublicZones/setHoveredPublicZones and deleted districtHoverStore.ts. Also cut the highlight-color comment (no longer needed) and lowered HIGHLIGHT_MASK_OPACITY 0.6 -> 0.45 per visual feedback.
…ring, sweep tests
- ftvPassCount now goes null (not a false failure) when any of the 4 FTV
keys lacks a disproportionality value — the backend metric can raise on
a zero-D+R-vote election and per-metric failures are isolated, so seats
can arrive without disproportionality.
- Fixed HOVER_BTN_STYLE's font:'inherit' silently un-bolding the three
Swing/Dem-Sweep/Repub-Sweep count cells (weight="bold" loses to the
inline style) by folding fontWeight into the style object instead of a
separate Radix prop, matching the existing pattern elsewhere in this file.
- Converted 5 <span role="button"> hover triggers to real <button
type="button">, matching BasicsSection's identical hover-to-highlight
pattern with the same HOVER_BTN_STYLE (already proven not to break
paragraph text selection) — spans announced as buttons to screen
readers but had no activation behavior.
- HelpTip's cloneElement({ref: triggerRef}) was silently replacing any ref
a caller already put on its trigger child; added a small mergeRefs
helper (same purpose as Radix's own composeRefs) so both refs populate.
- Backend: added a deterministic 4-district/2-election fixture pinning
exact dem_sweep_districts/rep_sweep_districts/swing_districts membership,
and strengthened the fuzz test with an independent oracle built from
ctx.dem_wins/rep_wins and ctx.dem_votes/total_votes. The prior tests
only checked cardinalities, so swapping the Dem/Rep sweep lists (or the
Dem/Rep vote share) silently passed everything — verified via a live
mutation test (swapped the two output lists, confirmed both new/
strengthened tests fail, then reverted).
- cursor: 'pointer' -> 'default' on the Swing/Sweep table rows: they only
respond to hover (no onClick), so pointer misleadingly implied a click
action: Dylan's own inline question ("Why pointer?").
- Hoisted ftvHelpTipTrigger to module scope: a static JSX constant with
no closure over props/state was being rebuilt every render.
Reverted, per human judgment: the Math.abs(s - 0.5) <= band float-boundary
rewrite — real vote shares landing exactly on a boundary is negligible in
practice, and the reviewer's own tag was Personal preference.
All four quality gates pass (pre-commit, frontend ts, frontend build,
backend pytest) via run-quality-gate.
deb.debian.org is a CDN in front of many mirror origins; a -security point-release rotation can leave one edge's index listing a filename another edge has already purged, producing a 404 an apt-get install can't recover from mid-run. Wrapping update+install in a retry loop lets a fresh apt-get update (which usually lands on a consistent edge) resolve it within the same build, instead of needing a whole CI job rerun. Verified live: this branch's AWS Preview workflow hit this exact failure 3 times in a row on 3 different packages (libtiff, libc-l10n, libperl); reproduced locally against the unmodified Dockerfile (libde265-0, arm64) while testing the fix. Confirmed the retry loop itself works (recovered from the same live mirror flakiness during a local arm64 build) and that the full build succeeds end-to-end on linux/amd64 (the real CI architecture) with the fix applied. No deploy-api.yml failure has ever been caused by this — checked its last ~30 runs; its only 2 failures were unrelated migration-task issues, apt-get succeeded in both. Folding this into #723 rather than a separate PR so the fix gets verified through this branch's own preview deploy, which is already hitting the failure live.
The evaluation panel's "Election Results and Partisanship" section now scores each plan against the Freedom-To-Vote Test: a plan is presumed fair if its disproportionality (seat share minus vote share) stays within
max(7%, 1/k)— k being the district count — for at least 3 of the 2 most recent Presidential and 2 most recent Senate elections. A paragraph plus a per-election table (D/R vote share and seat share, following the section's existing Dem/Rep toggle, plus a pass/tilt verdict) names the verdict, the 4 test contests, and the bound — phrased as whichever of "7%" or "1 out of k seats" actually binds. The 4 election keys are individually hoverable, both in the paragraph and in the table's own column headers, and highlight their matching row across all three partisan tables in this section; hovering the elections-passed count does the same for the passing subset. All of these use<span role="button">triggers rather than<button>(matching HelpTip's own default trigger) so selecting and copying the paragraph text works normally; a plain<button>was interrupting that. Dropped the section's old "found in the political science literature..." disclaimer sentence, which no longer fit ahead of a results-and-methods paragraph.Every metric the FTV table needs is already returned by the evaluation API (
disproportionality,seats,vote_shares) — election columns follow a uniform{pres|sen}_{yy}_{dem|rep}naming convention across every state's gerrydb table, so a newselectFtvElectionshelper inapp/src/app/utils/elections.tspicks the 2 most recent Presidential and 2 most recent Senate election keys by year, andPartisanSection.tsxscores and renders them.State coverage (checked against dev's
districtrmap/gerrydbtables across all 65 visible v2 state maps):pres_24,pres_20, and Senate data recent enough to run the intended test.pres_24in dev, so the test falls back topres_20/pres_16for them. Likely a pipeline backfill gap rather than permanent unavailability; filed as a follow-up (PA is one of the paper's marquee gerrymandering states, worth prioritizing).Configurable competitiveness threshold and district hover-highlighting
The Competitiveness section's swing/competitive classification used to run server-side against one fixed ±3% band, discarding the per-district data behind it.
competitive_metrics(backend/app/evaluation/partisans.py) now returns the sweep/swing classification as district-ID lists —dem_sweep_districts,rep_sweep_districts,swing_districts— independent of any band, since sweep/swing is just plain-majority win/loss per election, plus a flat sortedcontest_dem_vote_shareslist (one entry per district × election pair). The frontend classifies "competitive contests" at whatever band the user picks by filtering that list, instead of duplicating the classification logic client-side.n_districts/n_electionsstay in the payload directly, computed fromcontextrather than derived from the lists, so the metric stays self-describing even for districts with zero election columns.competitiveness's version bumps 1 → 2 for the shape change.Frontend: a
Selectcontrol (±2/3/5/7/10 points, defaulting to ±3 to match the old fixed behavior), inlined directly into the explanatory paragraph rather than on its own line, re-filterscontest_dem_vote_sharesclient-side — no refetch. The Swing/Dem Sweep/Repub Sweep district rows hover-highlight their actual districts on the map, which neededuseDistrictHover(app/src/app/hooks/useDistrictHover.ts) generalized to accept a list of districts instead of one — a single district is just the one-element case — updating its 4 existing call sites (BasicsSection,CompactnessSection) to pass arrays.District hover-highlight, restyled per design feedback: the highlighted district's outline is now a vivid orange (
HIGHLIGHT_LINE_COLOR,#ff7a00), and every non-hovered district dims to 45% white overlay while one is focused — the same masking conventionCountyLayers.tsxalready uses for the Super Draw paint-mask, applied here toPublicDistrictLayersvia a new conditionally-rendered mask layer keyed off MapLibre feature-state. Hover state itself lives on the existingmapStore(hoveredPublicZones/setHoveredPublicZones) rather than a new store, sinceuseDistrictHoveralready depended onmapStorefor the map ref.Also in this branch
Testing the new HelpTip surfaced a pre-existing bug in the shared
HelpTipcomponent, unrelated to FTV: selecting and copying its hover text left the card stuck open permanently. Radix's own HoverCard close logic refuses to schedule its close timer once any text inside the card has been selected, and that internal flag only clears on a fresh pointerdown inside the content — combined with this component's ownonPointerDownOutsidepreventDefault (added earlier for a different, click-to-reopen bug), there was no remaining path to close the card. Fix:onPointerDownOutsidenow dismisses the card on any outside click except one that lands back on the trigger itself (preserving the original click-to-reopen guard), giving a manual escape hatch that leaves Radix's hover-based auto-close untouched — it still correctly keeps the card open while text inside it is being selected.Tested
bun run ts(incremental typecheck) passes cleanly.pytest tests/test_partisan.pypasses (33 passed), including the newcompetitive_metricsshape and the zero-elections edge case.