Skip to content

fix(picker): show multi-state border gauges in the gauge picker - #96

Merged
mousebrains merged 1 commit into
mainfrom
fix-gauge-picker-multistate
May 30, 2026
Merged

fix(picker): show multi-state border gauges in the gauge picker#96
mousebrains merged 1 commit into
mainfrom
fix-gauge-picker-multistate

Conversation

@mousebrains

Copy link
Copy Markdown
Owner

Problem

The entire Columbia mainstem shows on the static gauges.html but is missing from gauge_picker.php (the "Build Your Own Gauges Page" picker).

Root cause: every Columbia mainstem gauge is a state-line border gauge whose gauge.state is the comma list OR,WA (John Day, The Dalles, Bonneville, Vancouver, St. Helens — all 5, all with live data). The picker filtered with an exact match:

WHERE g.state IN ('OR','WA')   -- never equals the literal 'OR,WA'

so the border gauges matched no state and were dropped — both server-side (the SQL) and client-side (gauge_picker.js partitioned AJAX rows with byState.get(r.state), an exact lookup that also misses "Oregon,Washington").

The static build never had this bug: web/build/gauges.py splits gauge.state on the comma so a border gauge renders under every one of its states (and filters.js splits data-state the same way). This PR brings the picker in line.

Fix

php/gauge_picker.php (3 spots) + static/gauge_picker.js (1 spot):

where before after
AJAX WHERE g.state IN (?,?) INSTR(',' || g.state || ',', ',OR,') > 0 OR-ed per abbrev — matches OR, OR,WA, WA,OR; not a substring of an unrelated code
AJAX row state $ABBREV_TO_STATE['OR,WA'] → '' split → "Oregon,Washington"
state pills 'OR,WA' → no pill split → both Oregon and Washington pills (deduped)
gauge_picker.js byState.get(r.state) split r.state on comma, bucket under each requested state (buildAllRows() already dedupes by id)

No schema/data change — it's purely how the picker reads the existing state column.

Tests

tests/php/GaugePickerIntegrationTest.php gains an OR,WA seed gauge and asserts:

  • it surfaces a Washington pill even with no single-state WA gauge seeded;
  • it matches a Washington-only and an Oregon-only AJAX filter, returning state: "Oregon,Washington";
  • a single-state MT gauge does not leak into a Washington filter (the comma-wrapped INSTR is anchored).

Local verification (full gate, green)

  • phpunit GaugePickerIntegrationTest — 8/8 (4 new)
  • full PHP suite — 523 tests, 2112 assertions, OK
  • PHPStan level 9 + strict — no errors
  • php-cs-fixer (dry-run) — clean
  • biome — clean

🤖 Generated with Claude Code

gauge_picker.php filtered gauges with an exact `g.state IN ('OR','WA')`,
but a border gauge on a state-line river stores a comma list ('OR,WA')
that equals neither 'OR' nor 'WA'. So the entire Columbia mainstem
(John Day, The Dalles, Bonneville, Vancouver, St. Helens — all 'OR,WA')
showed on the static gauges.html but vanished from gauge_picker.php.

The static build already handles this: web/build/gauges.py splits
gauge.state on the comma so a border gauge renders under every one of
its states. Mirror that in the picker, in three spots + the client:

- AJAX WHERE: match each selected abbrev against the comma-wrapped value
  (INSTR(',' || g.state || ',', ',OR,') > 0) instead of `IN (...)`.
- AJAX row mapping: re-join the matched abbrevs as full names
  ('Oregon,Washington') so the client can split them.
- State pills: split each distinct gauge.state so a border gauge with no
  single-state sibling still contributes both pills.
- gauge_picker.js: split the row's state on comma and bucket it under
  each requested state (buildAllRows() already dedupes by id).

Extends GaugePickerIntegrationTest with an OR,WA seed: the border gauge
now surfaces a Washington pill and matches an Oregon- or Washington-only
AJAX filter, while a single-state MT gauge still doesn't leak into a
Washington filter.

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

Copy link
Copy Markdown
Owner Author

Code Review — #96 fix(picker): show multi-state border gauges in the gauge picker

Verdict: ✅ Correct, well-tested, and faithful to the established pattern — ready to merge. CI is green; I also traced every path independently below. One out-of-scope sibling issue is worth a fast-follow (details at the end).

What it does

The 5 Columbia-mainstem gauges store gauge.state = 'OR,WA'. The picker filtered with g.state IN ('OR','WA'), which never equals the literal 'OR,WA', so all 5 border gauges silently dropped out of the picker (server SQL and the client's byState.get(r.state) exact lookup). This brings the picker in line with the static build, which already splits gauge.state on the comma.

Correctness — verified against the live DB and the surrounding code

  • SQL match (INSTR(',' || g.state || ',', ',' || ? || ',') > 0) — correct and anchored: comma-wrapping both sides means WA matches OR,WA / WA,OR but not a substring of an unrelated code. Confirmed against the data: the live DB has exactly 5 OR,WA rows and zero state values containing whitespace, so the no-trim INSTR is safe. Placeholders stay positional and 1:1 with $abbrevs, the values are whitelisted via $STATE_TO_ABBREV, and the empty-$abbrevs early-return upstream prevents a degenerate WHERE (). The ($state_match) parens correctly isolate the OR-group from the trailing AND.
  • Row re-join → 'Oregon,Washington' and pill split+dedupe — both mirror web/build/gauges.py:291‑296 exactly (split → map abbrev→name → re-join / keyed-set dedupe). The ?state=Washington deep-link now also pre-checks the right pill for a border gauge, which is a nice side win.
  • gauge_picker.js bucketing — splitting r.state and pushing into each requested bucket is right; I confirmed buildAllRows() (lines 75‑87) dedupes by r.id via a Map, so a gauge landing in both Oregon and Washington buckets renders once.
  • The comma data-state is harmless in the picker. I checked filters.js collectGroups() — it only builds a group per .filter-pills[data-group], and the picker's filter bar has only data-group="huc8" (no state group). So the rendered data-state="Oregon,Washington" is passive here; the splitCSV path is irrelevant to the picker. (On the static gauges.html the state group is rendered with split_csv=True at levels.py:469, so the PR's "filters.js splits data-state the same way" claim checks out there.)

Tests — good coverage

The 4 new cases hit the meaningful axes: WA pill surfaced from an OR,WA gauge with no single-state WA sibling; WA-only AJAX filter matches it and returns state: "Oregon,Washington"; OR filter returns both the single-state and border gauge; and the negative test that MT doesn't leak into a WA filter (guards the anchoring). The rowById() helper is mixed-safe, consistent with PHPStan level 9.

Sibling issue — recommend a fast-follow (not a blocker for this PR)

This PR makes border gauges selectable for the first time, and their destination — custom_gauges.php (?ids=…) — carries the same exact-match pattern this PR fixes in the picker:

  • php/includes/custom_gauges_handler.php:218‑220isset(CUSTOM_GAUGES_STATE_ABBREVS['OR,WA']) is false → a border gauge contributes no state pill.
  • :395CUSTOM_GAUGES_STATE_ABBREVS['OR,WA'] ?? '' → empty $state, so the :441 guard if ($state !== '' && $huc8 !== '') emits no data-state/data-huc8 at all → the row isn't a filterable row and silently ignores the state/watershed filters.
  • :326 — the state group is hand-written <div class="filter-pills" data-group="state"> with no data-split="csv" (unlike the static build's levels.py:469).

Net effect there is milder than the picker bug (the gauge still renders with its flow/gage; it just escapes filtering and drops its pills), so it's fine to land this PR as-is. The complete fix is the same three-spot mirror of the static build: split at 218‑220 and 395, and add data-split="csv" at 326. Happy to do it as a follow-up.

Minor / nits

  • The INSTR(...) predicate can't use an index on g.state, but gauge is tiny and the query is already gated by the latest_gauge_observation subquery — negligible.
  • No schema/data change; purely how the picker reads the existing state column. 👍

🤖 Reviewed with Claude Code

@mousebrains
mousebrains merged commit c4c7b7e into main May 30, 2026
8 checks passed
@mousebrains
mousebrains deleted the fix-gauge-picker-multistate branch May 30, 2026 20:34
mousebrains added a commit that referenced this pull request May 30, 2026
…ge (#97)

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 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