fix(custom-gauges): filter multi-state border gauges on the custom page - #97
Merged
mousebrains merged 1 commit intoMay 30, 2026
Merged
fix(custom-gauges): filter multi-state border gauges on the custom page#97mousebrains merged 1 commit into
mousebrains merged 1 commit into
Conversation
Copy link
Copy Markdown
Owner
Sorry, something went wrong.
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>
Copy link
Copy Markdown
Owner
Author
Code Review — #97 fix(custom-gauges): filter multi-state border gauges on the custom pageVerdict: ✅ 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 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. TeststestBorderGaugeSurfacesBothStatesAndStaysFilterable 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 |
Sorry, something went wrong.
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>
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
1 participant
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.
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:
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)
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)
Independent of #96 (different files); merges in any order.
🤖 Generated with Claude Code