Skip to content

fix(custom-gauges): filter multi-state border gauges on the custom page - #97

Merged
mousebrains merged 1 commit into
mainfrom
fix-custom-gauges-multistate
May 30, 2026
Merged

fix(custom-gauges): filter multi-state border gauges on the custom page#97
mousebrains merged 1 commit into
mainfrom
fix-custom-gauges-multistate

Conversation

@mousebrains

Copy link
Copy Markdown
Owner

Fast-follow to #96 (review flagged this sibling issue).

Why

#96 makes border gauges (gauge.state = 'OR,WA' — the whole Columbia mainstem) selectable in the gauge picker. Their destination, custom_gauges.php?ids=…, carried the same exact-match state handling the picker had, so a selected border gauge landed on a page that couldn't filter it:

  • _compute_custom_gauges_filters()isset(CUSTOM_GAUGES_STATE_ABBREVS['OR,WA']) is false → no state pill.
  • row render — CUSTOM_GAUGES_STATE_ABBREVS['OR,WA'] ?? '' → empty $state, so the if ($state !== '' && $huc8 !== '') guard emitted no data-state/data-huc8 → the row escaped both the state and watershed filters.
  • the State filter group lacked data-split="csv", so even a comma data-state wouldn't have matched a pill.

Milder than the picker bug (the gauge still rendered its flow/gage), but it dropped its pills and ignored the filters.

Fix — same three-spot mirror of the static build (web/build/gauges.py, levels.py:469)

  • pills: split gauge.state on the comma so each state contributes a pill.
  • row: map each abbrev → full name, re-join → data-state="Oregon,Washington".
  • State group: add data-split="csv" so filters.js splits the row value to match each pill.

No schema or data change — purely how the page reads the existing state column.

Tests

CustomGaugesIntegrationTest gains an OR,WA seed gauge and asserts a single border gauge surfaces both Oregon and Washington pills, the State group is data-split="csv", and the row renders as a filterable row (data-state="Oregon,Washington" data-huc8="17080003").

Local gate (green)

  • custom-gauges tests 15/15 · full PHP suite 520/520, 2106 assertions · PHPStan L9+strict clean · php-cs-fixer clean

Independent of #96 (different files); merges in any order.

🤖 Generated with Claude Code

Fast-follow to #96. That PR made border gauges (gauge.state = 'OR,WA',
the whole Columbia mainstem) selectable in the gauge picker, but their
destination — custom_gauges.php (?ids=...) — carried the same exact-match
state handling the picker had:

- _compute_custom_gauges_filters(): isset(STATE_ABBREVS['OR,WA']) is false,
  so a border gauge contributed no state pill.
- row render: STATE_ABBREVS['OR,WA'] ?? '' → empty $state, so the
  `$state !== '' && $huc8 !== ''` guard emitted no data-state/data-huc8 at
  all — the row escaped both the state and watershed filters.
- the State filter group lacked data-split="csv", so even a comma
  data-state wouldn't have matched a pill.

Mirror the static build (web/build/gauges.py + levels.py:469) in all three
spots: split gauge.state on the comma for the pills and the row's
data-state ('Oregon,Washington'), and render the State group
data-split="csv" so filters.js splits the row value to match each pill.

Extends CustomGaugesIntegrationTest with an OR,WA seed gauge: one border
gauge now surfaces both Oregon and Washington pills and renders as a
filterable row (data-state="Oregon,Washington" data-huc8=...). No schema
or data change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains

Copy link
Copy Markdown
Owner Author

Code Review — #97 fix(custom-gauges): filter multi-state border gauges on the custom page

Verdict: ✅ Approve. This is a faithful, minimal implementation of the fast-follow I flagged in the #96 review — it fixes the exact three spots, in the right way, with a test that pins the contract. I re-verified each against the code rather than taking the description on faith.

Verified

  • The three spots match the diagnosis. _compute_custom_gauges_filters() now splits state_abbrev so a border gauge contributes both pills; the row render splits → maps → re-joins to data-state="Oregon,Washington"; and the State group gains data-split="csv". That trio is precisely what was needed, and it mirrors the static build (gauges.py + levels.py:469 split_csv=True).
  • The end-to-end path is sound. I'd already confirmed in fix(picker): show multi-state border gauges in the gauge picker #96 that filters.js keys splitCSV off container.dataset.split === 'csv' and that matches() splits the row value on comma — so a data-state="Oregon,Washington" row now matches either the Oregon or Washington pill. The count($states_present) > 1 gate means a lone border gauge still renders the State group (2 pills), which the test relies on.
  • $state scoping is clean. The now comma-joined $state is used only by the if ($state !== '' && $huc8 !== '') guard and the data-state attribute (lines 441–442, htmlspecialchars-escaped) — it's never echoed into a visible cell, so no "Oregon,Washington" text leaks into the table. A border gauge with a HUC clears the guard and emits both data-state + data-huc8, so it's a filterable row.

The typing subtlety — done right

#96's picker fix needed (string)($row['state_abbrev'] ?? '') because those PDO rows are untyped (mixed). #97 correctly omits the cast: the custom-gauges rows are explicitly shaped (@param list<array{state_abbrev: string|null, …}>, lines 205/374), so $r['state_abbrev'] ?? '' is already a string and explode() gets a string — L9-strict stays clean. Nice that this wasn't blindly copied from the picker.

Tests

testBorderGaugeSurfacesBothStatesAndStaysFilterable pins the right server-side contract: both pills emitted from a single OR,WA seed, the data-group="state" data-split="csv" attribute, and the filterable-row regex data-state="Oregon,Washington"\s+data-huc8="17080003". The seed is realistic (Columbia/Vancouver, real HUC). No server-side negative test is needed here (unlike #96, the page is id-based, not state-filtered server-side — there's no anchoring to guard); the actual pill-matching is the shared filters.js splitCSV path, already exercised by the static page.

Note

🤖 Reviewed with Claude Code

@mousebrains
mousebrains merged commit 9fd13a1 into main May 30, 2026
8 checks passed
@mousebrains
mousebrains deleted the fix-custom-gauges-multistate branch May 30, 2026 20:34
mousebrains added a commit that referenced this pull request May 31, 2026
* docs: round-6 deep project review (graded B+, ▲ from B−)

Sixth deep project review of the entire tracked repo — 6 cold facet
auditors (Python, PHP/security, schema/data, tests/CI, ops, docs) +
synthesizer hand-re-verification, judging two bands: (A) did round-5's
fixes durably stick, and (B) what did #93#98 + migrations 0069–0071 +
the two direct-to-main commits introduce.

The recursive integrity check passes cleanly for the first time in the
series: every round-5 fix (R1.1/R1.2/R1.3/R1.5/R2.1/R3.x/R4.x) landed as
a committed PR and is still present at HEAD, and every mechanized guard
is proven non-vacuous by break-it experiment. New code is clean — no
CRIT/HIGH: #93 USACE kcfs→cfs (correct, per-series), migrations
0069/0070/0071 (idempotent, FK-clean, Bridgeport DROP cascade
residue-free), #96/#97 multi-state pickers, #95/#98 gradient JS.

Two MED findings, both recurrences of round-5 classes closed by
documentation not mechanization: (1) two direct-to-main commits, one of
which broke CI on main (the {}-is-a-dict bug); (2) a nightly snapshot
overrode migration 0067's sort_name for gauge 217 with no migration.
Root cause is shared — main accepts un-CI-gated direct pushes from both
humans and the snapshot bot. Lever: route everything through a CI gate
(branch protection + a self-gating/auto-merging snapshot), a
snapshot-column drift guard, and teach seed_gauge_display to preserve
migration-pinned sort_names.

Two facet over-claims dissolved on hand-re-verification (the USACE
temperature-docstring drop is a correct fix; check_reaches DOES
range-check vertices via validate_lat_lon).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: fold external-review corrections into the round-6 review (#99)

The PR #99 external verification pass re-confirmed every finding,
severity, and the B+ grade against db34ae0 (recommendation: merge), and
flagged one inaccurate evidence line plus three off-by-one citations.
Corrected:

  - MED #1: drop the `git branch --contains` "reachable only from main"
    claim — feature branches later cut from main now contain 9b428bb /
    6007c21, so containment no longer distinguishes them. The direct-to-
    main conclusion stands on the durable evidence (linear f3ed673..HEAD,
    no merge commit, missing (#NN) suffix).
  - citations: ci.yml:114→115, SourceUrlTest.php:83-84→84-85,
    check_reaches.py:212→213.

Added an External-review note recording the pass + the one below-LOW item
it surfaced (the 0069/0070 header comments' now-stale PENDING_RECONCILIATION
wording).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant