Skip to content

fix(region-filter): honor URL regions across views and clean up page listeners - #549

Merged
dborup merged 6 commits into
masterfrom
fix/531-region-url-across-views
Oct 24, 2026
Merged

fix(region-filter): honor URL regions across views and clean up page listeners#549
dborup merged 6 commits into
masterfrom
fix/531-region-url-across-views

Conversation

@dborup-agent

@dborup-agent dborup-agent commented Oct 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Relates to #531

Problem

  • Only Packets read ?region= from the hash, and it applied the link after its first /api/packets request, without telling anyone. A fresh #/packets?region=SJC over a saved SFO fetched SFO and labelled it SJC.
  • Nodes, Live, Channels and Analytics ignored the parameter. Their first request used the saved selection.
  • Packets registered its RegionFilter and AreaFilter listeners without keeping a handle. After a visit to Packets, a region click on any other page reloaded packets and replaceState'd the URL to #/packets?.... Nodes (area) and Live (two area listeners) also leaked one listener per visit.

Plan and changes

public/region-filter.js: RegionFilter.syncFromUrl(hash) owns the hash-query region parameter:

Input Result
no query, no region, region=, region=,,, region=%20 no override: the saved selection stands (unchanged behaviour)
region= SJC ,OAK,SJC SJC,OAK: codes trimmed, empty entries and repeats (case-insensitive) dropped, order kept
region=SJC%2COAK same as a plain comma (the form Packets writes)
region=OAK&region=SJC the first parameter wins (URLSearchParams.get semantics)
regions loaded, region=SJC,XXX SJC: unknown codes are dropped
regions loaded, region=XXX all regions, nothing saved
regions loaded, region=sjc SJC: codes match case-insensitively, as the server does, and take the configured spelling
fresh load, before regions load codes kept as given; the existing fetchRegions() cleanup drops unknown ones
region=All (any case, the server's Kpa-clawbot#770 catch-all) all regions, saved selection cleared, no request carries All; next to real codes (SJC,All) it is dropped
  • A link with codes overrides the saved selection and is saved like a click.
  • Listeners hear a genuine change once. A link equal to the current selection, in any order, notifies nobody.
  • On a fresh load, the cleanup notifies once only when a link's codes were all unknown and the selection falls back to all regions. Dropping some unknown codes notifies nobody: /api/config/regions lists every observer IATA (handleConfigRegions), so a dropped code matches no observer and the request already returned the same data.
  • A saved selection with no link keeps today's silent cleanup.

public/region-filter.js: hash write-back after a control change. When the user changes the region control, a region parameter already in the hash follows the selection: it is rewritten, or removed for all regions, with URLState.updateHashParams + history.replaceState, keeping the route and the other parameters (Analytics' tab, Live's node, ...), before listeners are told. One helper in toggleRegion() covers every view with a control (Packets, Nodes, Live, Channels, Analytics, Observers), so a stale link value cannot override the choice on Back or on reload. A hash without region is left alone; Packets still writes its own full URL. Map has no region control.

public/app.js: navigate() calls RegionFilter.syncFromUrl(location.hash) after the old page's destroy() and before the new page's init(). This covers the fresh load, SPA links and back/forward. Every view's first region-dependent request carries the link's region, including Live's node fetch, which runs before Live calls RegionFilter.init(). No second load is needed. Calling it after destroy() means the outgoing page's listeners are gone and cannot react.

public/packets.js: drops _pendingUrlRegion, its own URL parsing and its own setSelected() call. It keeps handles for its region and area listeners and removes them in destroy().

public/nodes.js, public/live.js: keep and remove their AreaFilter listener handles in destroy(). The other views already did this.

Compared with upstream Kpa-clawbot#2256, this applies the link in the router rather than in init() plus a notification after fetchRegions(). That removes the double load the upstream review raised: Live reloaded after its early fetch, and Nodes and Analytics loaded twice on in-app links. setSelected() stays non-notifying, which leaves the #416 notification question separate.

No config, schema, workflow or Go change. No customizer values.

Tests

  • test-issue-531-region-url.js (unit, registered in test-all.sh), 36 cases. Covers parsing and precedence, unknown codes and case before and after regions load, one notification versus a no-op, fresh-load cleanup notifications, the router order destroy → syncFromUrl → init, ?region=All, and the hash write-back after a control change (route, subpath and other parameters kept, removed for All, replaceState only, before listeners are told, a hash without region untouched). Against master: 1/26 of the first-round cases pass (the click-notifies refactor guard); against round 1 (496fba6d): 29/36.
  • test-issue-531-region-url-e2e.js (Playwright, shard 2), 18 cases on the CI fixture (regions MRY/OAK/SFO/SJC, saved selection SFO):
    • fresh #/<view>?region=SJC on Packets, Nodes, Live, Channels and Analytics: the first region-dependent request carries SJC, the control shows SJC, SJC is saved, and the per-path API request counts equal a load with SJC already saved
    • SPA navigation into each of the five views: the same checks, with one region-dependent request and the same API counts as navigating with SJC saved
    • comma list, fresh unknown-only link, in-app SJC,XXX, Packets writing back SJC, and a no-op link that loads once
    • 10 navigations across the five views: exactly one RegionFilter listener per view, AreaFilter counts stable, 0/0 on Home
    • after Packets, a region click on Nodes: no /api/packets request and the URL stays #/nodes
    • back/forward restore each entry's region
    • after a link ?region=SJC, OAK in the control on Packets, Nodes, Live, Channels, Analytics and Observers: the hash says OAK, other parameters kept, and a reload keeps OAK
    • fresh #/nodes?region=SJC, OAK in the control, Channels, Back: OAK, not SJC; picking All removes region and Back and reload keep All
    • fresh ?region=All / ?region=aLL on Nodes and Live: one unfiltered request
    • Against master: 1/18 of the first-round cases pass (the no-op case); against round 1 (496fba6d): 19/28.
  • Mutants: 21 in round 1 and 8 in round 2, all killed. The tables are in the report comments.

Perf

  • No new requests. syncFromUrl is O(codes + regions) and runs once per navigation. The E2E compares the per-path counts of every /api/ request (minus the /api/stats and /api/healthz pollers) against a baseline for each view, both fresh and after an SPA link.
  • Fewer requests than before in two cases: Packets no longer accumulates listeners that reload /api/packets on region or area changes elsewhere, and Nodes and Live no longer reload once per earlier visit on an area change.

Local validation

Residuals

  • Views other than Packets still do not add region to a URL that has none; only an existing parameter is kept in step. Older history entries keep the region they were opened with (the link wins on Back to them, by design).
  • A saved selection whose codes all disappeared (no link) still falls back to all regions without a notification, as before.

🤖 Generated with Claude Code

dborup and others added 2 commits October 24, 2026 07:58
@dborup @claude
test(region-filter): reproduce ?region= ignored across views and leak…
79894d2 
…ed listeners (#531)

Unit tests for RegionFilter.syncFromUrl (parsing, precedence over the
saved selection, unknown codes, one notification) and the router order,
plus a Playwright suite: fresh and in-app ?region= links on Packets,
Nodes, Live, Channels and Analytics, listener counts over ten
navigations, the stale Packets listener and back/forward.

Against master: unit 1/26 and E2E 1/18 pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(region-filter): honor URL regions across views and clean up page …
496fba6 
…listeners (#531)

RegionFilter.syncFromUrl() owns the hash-query region parameter: codes
are trimmed, empty entries and repeats dropped, unknown codes dropped
once regions are known (none left means all regions), and a link with
codes overrides the saved selection and is saved like a click. No
parameter or an empty one leaves the saved selection as before.

The router calls it after the old page's destroy() and before the new
page's init(), so every view's first region-dependent request, Live's
early node fetch included, carries the link's region, with no reload.
Listeners hear a genuine change once and a no-op notifies nobody.

Packets drops its own ?region= handling, which applied the link after
its first fetch. Packets, Nodes and Live now remove their RegionFilter
and AreaFilter listeners in destroy(), so a region change elsewhere no
longer reloads packets or rewrites the URL to #/packets.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

dborup-agent commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent1 PR#549 #531 — head 496fba6

Status: All requirements implemented and tested; 21/21 mutants killed; CI green on the first attempt (no reruns); draft, not marked ready.

Evidence: [T] test run locally against a Go server on the CI-prepared e2e-fixture.db · [A] analysis / code reading · [K] CI on this head (run 40076266623, attempt 1)

Red/green counts: unit test-issue-531-region-url.js is 1/26 on master and 26/26 on the fix. E2E test-issue-531-region-url-e2e.js is 1/18 on master and 18/18 on the fix [T], and 18/18 in Playwright shard 2/3 [K]. The test that is green on master is a no-op or refactor guard in each file.

Requirements

# Requirement Test (red before → green after) Mutant (killed by)
1 ?region= handled centrally in region-filter.js, before each view's first region-dependent fetch, Live's early node fetch included E2E: fresh #/<view>?region=SJC over saved SFO on Packets, Nodes, Live, Channels and Analytics, plus SPA navigation into each; the first request carries SJC (master: SFO on all 10) [T][K]. Unit: router order destroy → syncFromUrl → init [T] E-router-drop: 15/18 E2E red; U-router-drop, U-router-order [T]
2a Trimming Unit: SJC ,,OAK,SJC → SJC,OAK; %20,%20 leaves the saved selection [T]; E2E fresh comma list [T][K] U-trim [T]
2b Duplicates (case-insensitive) Unit: SJC,sjc,%20Sjc → &region=SJC before regions load; exact repeats [T] U-dedupe (survived at first; test added, now killed) [T]
2c Unknown codes Unit: loaded SJC,XXX → SJC, XXX → all with nothing saved, fresh-load cleanup; E2E: in-app SJC,XXX → one /api/nodes?region=SJC, fresh XXX ends on All [T][K] U-unknown [T]
2d Empty or missing parameter keeps today's behaviour Unit: no query, ?tab=x, regions=, region=, region, region=,, all keep saved SFO; empty with nothing saved stays All [T] U-empty [T]
2e Precedence over the saved selection Unit: link overrides and saves; E2E: control and localStorage show SJC [T][K] U-precedence [T]
2f Case and spelling, repeated parameter, malformed escape Unit: sjc → SJC; first region wins; %E0 does not throw [T] U-case, U-first-wins, U-malformed [T]
3 SPA navigation notifies active listeners exactly once on a genuine change, never on a no-op Unit: one call on change; none for same set in another order or a repeated link [T]. E2E: SPA link → exactly one region-dependent request, equal to the baseline [T][K] U-noop, U-notify-twice, U-no-notify [T]
3b Fresh load: only an all-unknown link that falls back to All notifies; a saved selection with no link keeps the silent cleanup Unit tests for both [T] U-cleanup-all, U-cleanup-any [T]; E-cleanup-all (extra load on Packets and Live) [T]
4 Packets' duplicate URL handling removed E2E: Nodes → #/packets?region=SJC,XXX: first request SJC, saved ["SJC"], packets URL writes region=SJC [T][K] E-packets-dup (restores the old setSelected, which saves ["SJC","XXX"]) [T]
5 Listeners removed on destroy; no stale listener rewrites another view's URL E2E: 10 navigations across 5 views, exactly 1 RegionFilter listener per view, AreaFilter count stable, 0/0 on Home (master: 2 on the second view). After Packets, a region click on Nodes sends no /api/packets and the URL stays #/nodes (master: reload plus rewrite) [T][K] E-packets-off, E-nodes-off, E-live-off [T]
6 Back button E2E: #/nodes?region=SJC → #/channels?region=OAK → back gives SJC, forward gives OAK, and every request carries it [T][K] E-router-drop [T]
7 Perf: no extra fetches; API calls per view load unchanged E2E: per-path counts of every /api/ request (minus the /api/stats and /api/healthz pollers) equal the baseline for all 5 views, both fresh and SPA [T][K]. syncFromUrl is O(codes + regions), once per navigation [A] E-cleanup-all, E-packets-off (count mismatch) [T]

Invariants

Check Result
cmd/server read-only, no new map[string]interface{} No Go files changed (git diff --stat origin/master -- cmd .github is empty) [A]
No hardcoded colours No CSS or colour changes [A]
scripts/check-xss-sinks.sh --diff origin/master Clean [T]
Fork guards deploy.yml 9, release-fast-path.yml 1, same as master [T]
node scripts/run-e2e-shard.js --validate Valid, 175 suites [T]; Verify E2E Shards & Coverage passes [K]

Local runs [T]

CI per job [K]

Job Result
Go Server Tests 1/3, 2/3, 3/3 pass
Go Ingestor Tests pass
Go Libraries & CLI pass
Verify Go Test Assignments pass
JavaScript Unit Tests & Lint pass
Script & Container Unit Checks pass
Playwright E2E Tests 1/3, 2/3, 3/3 pass (new suite 18/18 in 2/3)
Verify E2E Shards & Coverage pass
Validate Docker Image pass
Build & Publish Docker Image pass
Go Build & Test pass
Publish Badges & Summary, Release Artifacts, Deploy Staging skipped

Known flaky #271 did not occur. No job was re-run.

Remaining items

  • Views other than Packets do not write the region back to their URL. After a link #/nodes?region=SJC, a dropdown change and then a reload brings SJC back from the URL. This is unchanged and out of scope [A].
  • A saved selection whose codes all disappeared, with no link, still falls back to All without a notification, as before. A mutant pins this [T].
  • RegionFilter.setSelected() still does not notify. That is left to feat(region-filter): add configurable named region quick-pick groups #416 [A].
  • Not verified on staging or production, by design.

@dborup-agent

dborup-agent commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent3 PR#549 — head 496fba6

Dom: APPROVE with nits

Independent, read-only review. Evidence tags: [T] run by me locally · [A] code reading / analysis · [K] CI on this head.

Scope reviewed: git diff origin/master <merged tree>, where the merged tree is git merge-tree --write-tree origin/master 496fba6d… = 609d6311 on origin/master e08072be (clean merge, no conflicts). The branch is one merge behind master (#546). CI ran on the older base, so I ran every local test on the merged tree. Head was 496fba6d at the start and at the end (git ls-remote).

Findings

# Severity Where Finding Evidence
1 Low (follow-up) public/region-filter.js:119 (syncFromUrl), Nodes/Live/Channels/Analytics A stale ?region= in a non-Packets URL now overrides a later dropdown choice on Back and on reload, and is saved. Only Packets writes the region back to its URL. On the other views the link's region= stays in the hash after the user changes the dropdown, and Analytics (URLState.updateHashParams) and Live (?node= replaceState) carry it forward. Probe: fresh #/nodes?region=SJC, user picks OAK in the dropdown (saved ["OAK"], hash still ?region=SJC), goes to #/channels, presses Back: selection and saved value are SJC again. A reload does the same. On master the same steps keep the user's choice, because the link was ignored. The PR's residual note calls this "unchanged from before". The stale parameter was already there, but it only became effective with this PR. Suggested follow-up: on a dropdown change, have the other views drop or rewrite region in the hash with replaceState, as Packets does. Not blocking: the issue does not ask for write-back, and the E2E back/forward case pins "the URL entry wins" on purpose. [T] probe script against merged and master servers; [A]
2 Nit public/region-filter.js:122, :46 A fresh ?region=All (the server's Kpa-clawbot#770 sentinel for "all") makes one redundant request. Before regions load, {All} becomes the selection and the first /api/nodes?region=All already returns everything. The cleanup then falls back to null and notifies, so a second, unfiltered /api/nodes follows. Measured: ["All", null] vs [null] for #/nodes. Harmless; a case-insensitive ALL skip in parseUrlRegions would avoid it. [T]
3 Nit syncFromUrl vs toggleRegion A link that names every region (?region=MRY,OAK,SFO,SJC) becomes an explicit four-code filter. toggleRegion collapses that case to "all" (null). With a filter, observers without an IATA are excluded. This edge is consistent with "the link is what it says". It is mentioned only for completeness. [A]
4 Info Map Map has no RegionFilter-dependent fetch: map.js reads /api/config/regions only for its jump buttons and never calls RegionFilter. So "before the first region-dependent fetch" is vacuous there. #/map?region=SJC still saves SJC through the router, which is consistent with the other views. Observers (client-side region filter at render) is also covered by the router call, but has no E2E. [A]

No blocking defects found.

Answers to the review points

  • URL region applied before the first region-dependent fetch, every view. navigate() calls RegionFilter.syncFromUrl(location.hash) after the old page's destroy() and before the new page's init() (public/app.js:1508). navigate() always runs destroy+init, including same-page hash changes, so this covers fresh loads, SPA links and back/forward [A]. In the E2E, Packets, Nodes, Live (early /api/nodes), Channels and Analytics send SJC on their first region-dependent request, both fresh and via SPA. On master the same requests send SFO: 1/18 vs 18/18 [T][K]. Map: see finding 4.
  • Precedence, trimming, duplicates, unknown codes. URL > saved, saved like a click. Codes are trimmed, empty entries dropped and repeats dropped case-insensitively. Once regions are loaded, unknown codes are dropped and codes take the configured spelling; all-unknown means all regions with nothing saved. Before load, codes are kept and fetchRegions() cleans up. A silent partial drop is correct: handleConfigRegions lists every observer IATA, and GetObserverIdsForRegion matches UPPER(TRIM(iata)), so a dropped code matched no observer anyway [A]. Unit tests cover each rule [T].
  • Empty or missing parameter unchanged. Unit tests cover '', no query, ?tab=x, regions=, region, region=, region=,, and %20,%20 [T]. Independently I ran a probe for 7 views (Packets, Nodes, Live, Channels, Analytics, Observers, Map) on fresh load and SPA, with SFO saved and no link. The per-path /api/ counts, including the region values sent, are identical on master and merged [T].
  • Listener lifecycle. Packets now keeps and removes its RegionFilter and AreaFilter handles. Nodes and Live remove their AreaFilter handles; Live has two. The other views already did. E2E: over 10 navigations there is exactly 1 region listener per view, the area count is stable, and Home shows 0/0. On master the second view already had 2 [T][K].
  • No cross-view URL rewrite. E2E: after Packets, a region click on Nodes sends no /api/packets and the hash stays #/nodes. On master there was a reload plus a rewrite [T][K].
  • No double fetch. The SPA and fresh E2E cases compare per-path API counts against a baseline with SJC already saved [T][K]. Combined with my master-vs-merged no-link probe above, that shows no added requests. The one exception is the ?region=All nit (finding 2).
  • Back/forward and deep links. E2E #/nodes?region=SJC → #/channels?region=OAK, then back gives SJC and forward gives OAK [T][K]. See finding 1 for the stale-entry side effect.
  • E2E flakiness. test-issue-531-region-url-e2e.js passed 18/18 in 5 of 5 consecutive runs against a local server, about 1m46s per run [T]. In CI it passed 18/18 in Playwright shard 2/3 [K].
  • XSS gate, CSS variables. scripts/check-xss-sinks.sh --diff origin/master exits 0 on head and on the merged tree, run in a scratch clone [T]. No CSS changes and no colour literals were added; the only #… matches are issue refs like #531 [A].

Invariants

Check Result
Each acceptance criterion has a red→green test Yes. Unit test-issue-531-region-url.js: master 1/26, merged 26/26. E2E: master 1/18, merged 18/18. The test that is green on master is a no-op or refactor guard in each file [T]
No behaviour change beyond the purpose Holds for the no-link path (identical API calls, see above). The link path changes only as intended, plus finding 1 [T][A]
cmd/server read-only; no new map[string]interface{} No Go, cmd/ or .github/ changes; 0 added map[string]interface{} [A]
No hardcoded colours None added [A]
Fork guards deploy.yml 9, release-fast-path.yml 1, same on head, merged and master [T]
Closing keywords None in the commits, PR title or PR body ("Relates to #531") [T]
Commit author Both commits: author and committer dborup <kontakt@meshview.dk> [T]
E2E shard manifest node scripts/run-e2e-shard.js --validate exits 0 on merged (176 suites) [T]

Tests run [T] (merged tree 609d6311 on origin/master e08072be)

  • cmd/server: go test ./... → ok (59.9s)
  • cmd/ingestor: go test ./... → ok (218.0s)
  • sh test-all.sh → 264 passed, 0 failed (264 files)
  • node test-frontend-helpers.js → 719 passed, 0 failed
  • E2E against a local Go server on e2e-fixture.db, prepared as in deploy.yml: freshen, the inline seed SQL, corescope-migrate, then seeds 2073 and 199, plus 245 and 122 as CI also applies them. Region list MRY, OAK, SFO, SJC.
    • test-issue-531-region-url-e2e.js: 18/18 ×5 on merged; 1/18 on master (same fixture, master server)
  • Servers were stopped by port (fuser -k <port>/tcp).

Mutants (my own) [T]

Mutant Killed by
U1: knownCodes matches case-sensitively unit: "codes match case-insensitively…"
U2: sameSelection becomes order-sensitive unit: "an unchanged selection (any order) notifies nobody"
U3: drop the all-unknown notify() after fetchRegions() unit: "fresh load: link with only unknown codes … notifies once"
U4: dedupe case-sensitively in parseUrlRegions unit: "fresh load: repeats differing only in case…"
U5: router calls syncFromUrl before destroy() unit: router order test
E1: same as U5, in the browser E2E 5/18 red: the outgoing page's listener reloads (extra /api/packets or /api/nodes) on every SPA link
E2: Live's destroy() keeps its area-outline listener E2E: "10 navigations…": area listeners on packets grew 1 → 2

7/7 killed. E1 also shows why the destroy→sync→init order matters.

CI per job [K] (run 40076266623, attempt 1, no reruns)

Go Server Tests 1/3, 2/3, 3/3 · Go Ingestor Tests · Go Libraries & CLI · Verify Go Test Assignments · JavaScript Unit Tests & Lint · Script & Container Unit Checks · Playwright E2E 1/3, 2/3 (new suite 18 passed, 0 failed), 3/3 · Verify E2E Shards & Coverage · Validate Docker Image · Build & Publish Docker Image · Go Build & Test: all pass. Publish Badges, Release Artifacts and Deploy Staging were skipped (fork). Known flaky tests #256 and #267 did not show up.

Not verified

  • I did not re-run the 14 other existing E2E suites the author lists. I relied on CI shards 1–3 passing on head [K], and the merge with fix(route-view): exclude payload-derived endpoints from displayed hop count #546 touches no file in this PR's frontend path except the shard manifest and test-all.sh registration.
  • I did not exercise Observers or Map with ?region= in a browser (only [A]).
  • I did not test races where /api/config/regions is slow or fails during an SPA navigation; I only read that code path [A].
  • No staging or production checks, by design.

dborup and others added 4 commits October 24, 2026 08:59
@dborup
Merge origin/master into fix/531-region-url-across-views
7dce768
@dborup @claude
test(region-filter): reproduce a stale ?region= overriding a later ch…
904870b 
…oice and ?region=All loading twice (#531)

- A change in the region control leaves an existing ?region= in the hash,
  so Back or a reload brings the link's region back over the user's
  choice. Unit and E2E cases for Packets, Nodes, Live, Channels, Analytics
  and Observers; other hash parameters and the route must be kept.
- ?region=All (any case) must mean all regions with one unfiltered
  request.
- The E2E seeds the saved selection once per tab, so a reload keeps what
  the page saved.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(region-filter): keep the hash ?region= in step with the control a…
e1b4b38 
…nd treat All as all regions (#531)

- RegionFilter rewrites a region parameter already in the hash after a
  change in the control (URLState.updateHashParams + replaceState, other
  parameters and the route kept, removed for all regions), before
  listeners are told. One helper covers every view with a control; a hash
  without the parameter is left alone, and Packets still writes its own.
- "All" in any case in ?region= is the server's Kpa-clawbot#770 catch-all: alone it
  selects all regions without a request carrying it, next to real codes
  it is dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
test(e2e): update the #531 suite's shard note for the added cases
928642a 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

dborup-agent commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#549 runde-2 — head 928642a

Review feedback addressed (commit e1b4b383, tests in 904870be, shard note in 928642a6, merge of origin/master e08072be in 7dce768d)

Evidence: [T] run locally against a Go server on the CI-prepared e2e-fixture.db · [A] code reading / analysis · [K] CI on this head (run 40083574500, attempt 1)

  1. Finding 1 (blocking): a stale ?region= overrode a later dropdown choice on Back and reload. Fixed centrally in public/region-filter.js with one helper, syncUrlRegion(), called from toggleRegion(). That function is the only path a click in the control takes, on every view. After a change, a region parameter already in the hash is rewritten to the new selection, or removed for all regions. The helper uses URLState.updateHashParams + history.replaceState(history.state, …) and keeps the route, the subpath and the other parameters (Analytics' tab, Live's node, …). It runs before listeners are notified, so a view that rewrites its own hash (Analytics, Packets) starts from the updated one. This covers Packets, Nodes, Live, Channels, Analytics and Observers. Map has no region control, so nothing can go stale there through a dropdown [A].
    • Design choice: only a region that is already in the hash is kept in step; none is added where there was none. That is enough to stop the stale override: the URL can no longer disagree with the selection, and a hash without region falls back to the saved selection, which is the user's choice. It also keeps the change narrow. Nodes, Live, Channels, Analytics and Observers keep their current URL shapes after a region click, and existing tests that pin exact hashes stay valid, e.g. the fix(channels): keep client-only state (PSK selection, My Channels, unread) across a channel-list refresh #152 decrypt suite's #/channels/<hash> check (18/18) [T][A]. Packets still writes its full URL itself, as before. Adding region to every view's URL (full deep-linking of the filter) would be a separate change.
    • Back to an older entry that still carries its own link region applies that region. That is the "the URL entry wins" behaviour the round-1 back/forward test pins on purpose, and it is unchanged [A].
  2. Nit 2: ?region=All made a redundant request. parseUrlRegions() now treats All in any case as the server's Region filtering - very strange behavior, no documentation Kpa-clawbot/CoreScope#770 catch-all (normalizeRegionCodes in cmd/server/db.go). Alone, or as All,all, it selects all regions and clears the saved selection; no request carries All, and fetchRegions() has nothing to clean up, so there is no second load. Next to real codes it is dropped, as on the server: SJC,All,OAK → SJC,OAK. An empty region= still leaves the saved selection, unchanged [T][A].
  3. Nit 3 (a link naming every region becomes a four-code filter): no change. I agree with the review that this is consistent with "the link is what it says". Collapsing it to "all" would silently widen the filter to observers without an IATA [A].
  4. Info 4 (Map / Observers): Map has no region control and no region-dependent fetch, so the write-back does not apply there [A]. Observers now has E2E coverage: link ?keep=1&region=SJC → OAK in the control → the hash says OAK with keep kept → a reload keeps OAK [T][K].

Tests (red before → green after)

Test Round 1 head 496fba6d (merged with master) This head
Unit test-issue-531-region-url.js 29/36 [T] 36/36 [T][K]
E2E test-issue-531-region-url-e2e.js 19/28 [T] 28/28 ×5 locally (about 2m25s per run) [T]; 28/28 in Playwright shard 2/3 [K]

New unit cases (10). The 3 marked guard also pass on round 1:

  • ?region=All in any case means all regions, saved selection cleared, listeners told once (guard: after regions load an unknown ALL already fell back to all)
  • fresh load: ?region=All in any case (All, ALL, all, %20aLl%20) sends no region and does not notify when regions load
  • ?region=All with all regions already selected is a no-op (guard)
  • All next to real codes is dropped
  • a change rewrites the hash region with replaceState, keeps tab and window, and a reload keeps the choice
  • a change keeps the route subpath (#/nodes/abc%20def)
  • a change to all regions removes region (#/live?node=abc&region=SJC → #/live?node=abc)
  • repeated, empty or unknown-only region parameters are replaced by exactly one
  • listeners see the updated hash
  • a hash without region is left alone, with no replaceState (guard)

New E2E cases (10). Each one before the fix:

  • #/<view>?…&region=SJC over saved SFO → OAK in the control on Packets, Nodes, Live, Channels, Analytics (tab=topology) and Observers: the hash says OAK, other parameters are kept, saved OAK, and after a reload the first region-dependent request carries OAK and the control shows OAK. Before the fix: 5 red ("hash region after the switch: got SJC"); Packets green, since it already wrote back.
  • The requested scenario: fresh #/nodes?region=SJC → OAK in the control → #/channels → Back → control, selection and saved value are OAK, every /api/nodes carries OAK, a reload keeps OAK, and the hash after the switch was #/nodes?region=OAK. Before the fix: red with "control after back: got SJC" (the bug as a user sees it).
  • fresh #/nodes?keep=1&region=SJC → All → hash #/nodes?keep=1; Back and reload keep All. Before the fix: red.
  • fresh #/nodes?region=All and #/live?region=aLL over saved SFO: exactly one /api/nodes, unfiltered; the control shows All and nothing is saved. Before the fix: red, ["All", null] / ["aLL", null].

The E2E harness now seeds the saved selection once per tab, using a sessionStorage flag. Before, the init script wrote SFO again on every reload, which hid what the page had saved. Existing cases are unaffected: they load each document once.

Mutants (8 unit, 4 of them also in E2E) — all killed [T]

Finding Mutant Killed by
1 M1-no-writeback: drop the syncUrlRegion() call unit 5 red; E2E 7 red, including "Back: OAK, not SJC" (got SJC)
1 M1-after-notify: write the hash after notifying unit "listeners see the updated hash"
1 M1-always-write: also add region to a hash without one unit "a hash without a region parameter is left alone"
1 M1-drop-params: rebuild the hash from the route only unit 2 red; E2E 6 red (keep=1 / tab=topology lost)
1 M1-pushState: pushState instead of replaceState unit 5 red
2 M2-all-is-code: no All handling unit 2 red; E2E 2 red (["All", null])
2 M2-case-sensitive: only All exactly unit "fresh load … in any case" (ALL; it survived at first, so I added the case variants); E2E aLL red
2 M2-all-only-keeps-saved: ?region=All leaves the saved selection unit 2 red

Local runs [T] (this head)

CI per job [K] (run 40083574500, attempt 1, head 928642a6)

Job Result
Go Server Tests 1/3, 2/3, 3/3 pass
Go Ingestor Tests pass
Go Libraries & CLI pass
Verify Go Test Assignments pass
JavaScript Unit Tests & Lint pass
Script & Container Unit Checks pass
Playwright E2E Tests 1/3, 2/3, 3/3 pass (new suite 28 passed, 0 failed in 2/3, about 3 min)
Verify E2E Shards & Coverage pass
Validate Docker Image pass
Build & Publish Docker Image pass
Go Build & Test pass
Publish Badges & Summary, Release Artifacts, Deploy Staging skipped

Known flaky tests #271 and #301 did not occur. No job was re-run.

Invariants

  • No Go, cmd/ or .github/ changes; no new map[string]interface{}; no CSS or colour changes [A]
  • Commits: author and committer dborup <kontakt@meshview.dk>; no rebase, amend or force-push (the push was a fast-forward from 496fba6d) [T]
  • PR description updated (Plan, Tests, Residuals); the PR stays a draft [T]
  • Perf: one URLState parse and rebuild per click in the control, only when the hash has region; no new requests, and one fewer on a fresh ?region=All [A][T]

Not verified

  • No staging or production checks, by design.
  • Safari's replaceState throttling path (caught and ignored) was not exercised in a browser.

@dborup

dborup commented Oct 24, 2026

Copy link
Copy Markdown
Owner

Review — CS-Minimax PR#549 — head 928642a

Dom: APPROVE with nits

Independent, read-only re-review of the whole PR (rounds 1 and 2), not only the round-2 delta. Evidence tags: [T] run by me locally · [A] code reading / analysis · [K] CI on this head.

Scope: git diff origin/master <merged tree>, where the merged tree is git merge-tree --write-tree origin/master 928642a6… = d85ce920 on origin/master 9080b6aa (clean merge). The diff is 9 files: app.js, live.js, nodes.js, packets.js, region-filter.js, the shard manifest, test-all.sh and the two new tests. Head was 928642a6 at the start and at the end (git ls-remote). While I was reviewing, master moved to a4126c63 (#550, Go and config only, no file shared with this PR). The merge is still clean; I did not re-run the tests on that base.

Findings

# Severity Where Finding Evidence
1 Low (follow-up) region-filter.js syncFromUrl, router Back to an older entry that still carries a link ?region= applies that region again and saves it, over a choice the user made later on another entry. Probe: fresh #/nodes?region=SJC over saved SFO, then SPA to #/channels, then OAK in the Channels control. The hash has no region, so nothing is written back. Back gives #/nodes?region=SJC, the selection is SJC and the saved value is ["SJC"]. Round 2 fixed the same-entry case (the dropdown rewrites the entry's own region). The PR lists this cross-entry case as a residual, and master Packets already behaved this way through _pendingUrlRegion. So "Back keeps the user's choice" holds within an entry, but not across entries. A possible follow-up: mark an entry whose link has been applied in history.state, as Analytics does for #208, and skip the override on Back/Forward to it. Not blocking. [T][A]
2 Nit (test flake risk) test-issue-531-region-url-e2e.js, section 2 (SPA API-count baseline) The SPA API-count comparison depends on timing. Packets' ensureHopResolver() fetches /api/nodes and /api/iata-coords only while HopResolver.ready() is false. Whether it is still false depends on how far the previous page got within SETTLE_MS. 1 of my 10 runs failed #/nodes → #/packets?region=SJC with got {observers, packets, channels}, want {… nodes:1, iata-coords:1 …}. The link run made fewer calls than the baseline, so this is not a product regression. That run overlapped with my own probe load; the other 9 runs passed. Suggestion: leave /api/nodes and /api/iata-coords out of that comparison when the target is Packets, or wait for HopResolver.ready() before navigating. [T][A]
3 Nit (test gap) region-filter.js:123 syncUrlRegion replaceState(window.history.state, …) correctly keeps the entry's state, which Analytics' #208 entry-view record and packet-path-map.js depend on. No test pins it: my mutant replaceState(null, …) survived both suites. A unit case that seeds history.state and asserts that it is unchanged after a click would close this gap. [T]
4 Info syncUrlRegion via URLState.updateHashParams The write-back rebuilds the query with URLSearchParams. This normalises encoding (search=foo%20bar becomes search=foo+bar, and , becomes %2C) and drops parameters with an empty value. I checked that the Nodes search still reads back as foo bar after a reload, and that Analytics tab, Live node and Observers keep survive (the E2E covers this too). Analytics' own URL sync uses the same helper. No action needed. [T][A]
5 Info fetchRegions Removing _urlBeforeLoad = false survives every test. The mutant is equivalent on the success path, because fetchRegions() returns early once _loaded is set. It would only matter after a failed /api/config/regions followed by a retry. No action needed. [T][A]

No blocking defects found.

Answers to the review points

  • One shared write-back helper, every view. syncUrlRegion() in region-filter.js is called only from toggleRegion(), which is the single path a pill or dropdown change takes. It runs before notify(), uses replaceState and keeps the route, subpath and other parameters [A]. E2E: after a link with region=SJC, OAK in the control on Packets, Nodes, Live, Channels, Analytics (tab=topology) and Observers rewrites the hash to OAK, keeps the other parameters, and a reload keeps OAK [T][K]. Map has no region control and map.js never uses RegionFilter, so the write-back does not apply there. #/map?region=SJC still saves SJC through the router, with the same API calls as a load with SJC saved [T]. Only a region already in the hash is kept in step; none is added where there was none (a deliberate design choice, see finding 1). The Packets "Clear" button calls setSelected([]) instead of toggleRegion, and then writes its own URL [A].
  • Back/forward and reload keep the choice; a deep link wins on first load. Same entry: fresh #/nodes?region=SJC, OAK in the control, #/channels, Back gives OAK, and a reload keeps OAK. All removes region, and Back and reload keep All [T][K]. Across entries, see finding 1. A deep link wins on a fresh load and on SPA links: in all 10 fresh and SPA cases the first region-dependent request carries SJC. On master all 10 send SFO [T].
  • ?region=All, case-insensitive, no extra request. All, ALL, all and aLL select all regions and clear the saved selection, and SJC,All drops All. Measured on fresh loads over saved SFO: Nodes and Live send exactly one unfiltered /api/nodes (PR E2E). In my own probe, Channels, Packets and Observers send one unfiltered request each, and Analytics sends each of its six endpoints once, unfiltered, with nothing saved [T].
  • Listener lifecycle over 10+ navigations. My probe made 14 SPA navigations over all 7 views (Packets, Nodes, Live, Channels, Analytics, Observers, Map), then went Home. Region/area listener counts were identical on both passes (Packets 1/1, Nodes 1/1, Live 1/2, Channels 1/0, Analytics 1/1, Observers 1/0, Map 0/2), Home showed 0/0, and there were no page errors [T]. The PR E2E covers the 5-view case, and on master the second view already had 2 region listeners [T].
  • API calls per view load unchanged. With no link and saved SFO, the per-path counts including the region value sent are identical on master and merged for all 7 views, both fresh and via an SPA link from Home [T]. With a link ?region=SJC over saved SFO, Observers and Map make the same calls as a load with SJC saved, both fresh and SPA [T]. The PR E2E covers the other 5 views the same way [T][K].
  • E2E flakiness (5×). test-issue-531-region-url-e2e.js on the merged tree: 28/28 in runs 1–4 and 6–10. Run 5 was 27/28, because of finding 2 (a test-harness race while the machine was loaded). That is 9 of 10 runs green, each about 142 s [T]. CI: 28/28 in Playwright shard 2/3 [K].

Invariants

Check Result
Each acceptance criterion has a red→green test Yes. Unit: master 2/36 (both green cases are guards: "a click still notifies once" and "a hash without region is left alone"), round-1 code 29/36, merged 36/36. E2E: master server 2/28 (the no-op link and the Packets write-back, which master already did), merged 28/28 [T]
No behaviour change beyond the purpose No-link path: identical API calls on 7 views [T]. Link path: as intended. A link's region is now saved globally from any route, as Packets did on master [A]
cmd/server read-only; no new map[string]interface{} No cmd/ or .github/ changes; 0 added map[string]interface{} [A]
No hardcoded colours No CSS changes and no colour literals added [A]
scripts/check-xss-sinks.sh --diff origin/master Exit 0 on head and on the merged tree (scratch clone) [T]
Fork guards github.repository == 'Kpa-clawbot/CoreScope': deploy.yml 9 and release-fast-path.yml 1 on master, head and merged [T]
Closing keywords None in the commits, PR title or body ("Relates to #531") [T]
Commit author All 6 commits: author and committer dborup <kontakt@meshview.dk> [T]
Shard manifest node scripts/run-e2e-shard.js --validate: 177 suites, valid [T]

Tests run [T] (merged tree d85ce920 on origin/master 9080b6aa)

Mutants (my own; none taken from the author's or the round-1 review's lists) [T]

Mutant Result
MA: knownCodes keeps the link's spelling instead of the configured one killed (unit: "take the configured spelling")
MC: the last region parameter wins instead of the first killed (unit: "the first region parameter wins")
ME (unit): syncFromUrl does not saveToStorage() killed (unit: 4 red)
MF: only exactly All is the catch-all killed (unit: "in any case … ALL")
MD: syncUrlRegion passes null instead of history.state survived, see finding 3
MB: drop _urlBeforeLoad = false in fetchRegions survived, equivalent on the success path (finding 5)
EA (E2E): syncFromUrl does not save killed (E2E 4 red: in-app SJC,XXX, Packets shared handling, both ?region=All cases)
EB (E2E): drop the all-unknown notify() after fetchRegions() killed (E2E: "fresh link with only an unknown code": last /api/nodes region XXX, want null)

6 of 8 killed. One survivor is equivalent; the other is a test gap (finding 3).

CI per job [K] (run 40083574500, attempt 1, head 928642a6, base e08072be)

Go Server Tests 1/3, 2/3, 3/3 · Go Ingestor Tests · Go Libraries & CLI · Verify Go Test Assignments · JavaScript Unit Tests & Lint · Script & Container Unit Checks · Playwright E2E 1/3, 2/3, 3/3 · Verify E2E Shards & Coverage · Validate Docker Image · Build & Publish Docker Image · Go Build & Test: all pass. Publish Badges & Summary, Release Artifacts and Deploy Staging were skipped. No reruns. Known flaky tests #256 and #267 did not show up. CI ran on base e08072be; the later master commits (#543, #547, #550) touch no file in this PR, and I ran the local suites on the merged tree.

Not verified

  • I did not re-run the tests on the newest master a4126c63. The merge is clean, and neighbors: follow-ups from #541 (graph-age clamp, cache/publish tests, atomic.Pointer) #550 changes only Go and config.
  • I did not run test-channels-client-state-152-decrypt-e2e.js, test-region-scope-approval-e2e.js or test-issue-406-packets-live-channel-e2e.js. They start their own server or ingestor; I relied on CI [K].
  • I did not test races where /api/config/regions is slow or fails during an SPA navigation, and I did not test Safari's replaceState throttling (code reading only) [A].
  • No staging or production checks, by design.

@dborup
dborup marked this pull request as ready for review October 24, 2026 11:12
@dborup
dborup merged commit ff1acc0 into master Oct 24, 2026
18 checks passed
This was referenced Oct 24, 2026
Closed
Closed
@adminopenclaw8-sketch adminopenclaw8-sketch mentioned this pull request Oct 24, 2026
Merged
This was referenced Oct 24, 2026
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dborup-agent @dborup