fix(packets): empty observer/type selections on Clear Filters

[dborup]

Relates to #121

Summary

Clear Filters on the Packets page now actually empties the observer and packet-type selections. Before this change, the next pick after Clear added to the old selection: select observer A, Clear, select B gave observer=obsA,obsB. The "All Observers" and "All Types" rows were also left unchecked after Clear.

Upstream reference (read only): Kpa-clawbot/CoreScope#2015.

Plan and design

Autonomous run, so the plan is written here instead of waiting for approval (AGENTS.md rule 5).

1. Test first (commit a443333c). A new unit test runs the real code from public/packets.js in a sandbox with a minimal fake DOM. It drives the menus and the Clear button as a user would. It is red on master.
2. Fix (commit 8b8b5bcd).
   • Clear handler: it now calls selectedObservers.clear() and selectedTypes.clear(), then rebuilds both menus and trigger labels. The rebuild uses the existing buildObserverMenu()/buildTypeMenu() and updateObsTrigger()/updateTypeTrigger(). It replaces the hand-written loops that only unchecked checkboxes.
   • Type change handler: it must refresh the Clear button, otherwise the button stays hidden with only a type selected and the type half of the regression test cannot be reached by a click.
3. Review fix (commit 7f37e0b7). 8b8b5bcd did that refresh by calling updatePacketsUrl(). That function rebuilds the query from filter params only, so #/packets/<hash>?obs=123&viewPath=1 lost obs and viewPath when a type was picked. The Clear-button visibility is now its own function, updateClearFiltersVisibility(), which updatePacketsUrl() also uses. The type handler calls only that function, so the hash is left untouched (type is not a URL parameter).

Fork compared with upstream. The core change matches upstream. Upstream's merge also resets an observer search box from its Kpa-clawbot#1884. This fork has no such box, so that part is not ported. The test is fork-specific: it covers repeated Clears, SPA remount, the one-reload guarantee and the other filters, which upstream's test does not.

Config and customizer (AGENTS.md rule 8). No new configurable values.

Acceptance criteria

Criterion	Status	Evidence
Clear both internal Sets and rebuild their menus from an empty state	Met	Clear handler; unit tests: observer, type, repeated Clears
"All Observers" and "All Types" checked, with matching trigger labels and titles	Met	Unit test "after Clear: … checked"; browser at 1440 and 1024
URL, local storage and filters keep no stale observer/type values	Met	Unit tests check all three after Clear and after the next pick; browser
Hash, node, channel, language, time-window, region and My Nodes behaviour unchanged	Met	Unit test "the other filters are still reset by Clear"; test-clear-filters.js 8/8; test-filter-ux-e2e.js 12/12
Clear reloads/renders exactly once	Met	Unit test: loadPackets +1 and renderTableRows +0 per Clear; browser: exactly one /api/packets? request per Clear. RegionFilter.setSelected() does not notify listeners, so there is no second reload.
Regression test: A then Clear then B, repeated Clears, SPA remount	Met	8 cases in test-issue-121-clear-filters-selection.js
Picking a type keeps ?obs= and ?viewPath= in the URL and shows Clear (review P2)	Met	2 new cases in test-issue-121-clear-filters-selection.js (?obs=123, ?obs=123&viewPath=1); red on 8b8b5bcd (8 passed, 2 failed)
Type handler does not call updatePacketsUrl(), and does call the visibility function	Met	Mutants: updatePacketsUrl() back in the type handler gives 8 passed, 2 failed; visibility call removed gives 7 passed, 3 failed

Tests

node test-issue-121-clear-filters-selection.js

• master: 2 passed, 6 failed. The failures are the bug: 'obsA,obsB' !== 'obsB', '4,5' !== '5', All rows unchecked, Clear hidden with only a type selected. The two passing cases are the controls.
• This branch: 10 passed, 0 failed.

node test-clear-filters.js

• This file existed but was not run by CI.
• master: 6 passed, 2 failed. Both failures were location is not defined in its own harness.
• This branch: 8 passed, 0 failed. It now gets bindings for the new identifiers and extracts updateClearFiltersVisibility() together with updatePacketsUrl(). Its checkbox case asserts that the Sets are emptied and the menus rebuilt.
• It is now registered in CI and test-all.sh.

Other suites, same results on this branch as on master

Suite	Result	In CI
test-packet-filter.js	92/0	yes
test-packet-filter-ux.js	19/0	yes
test-packet-filter-time.js	20/0	yes
test-aging.js	19/0	yes
test-frontend-helpers.js	705 passed / 2 failed	test-all.sh only
test-packets.js	115 passed / 13 failed	no

The failures in the last two are pre-existing: a diff of the failing test names between origin/master 85bfee49 and this branch is empty.

Static checks

• scripts/check-xss-sinks.sh --diff origin/master: exit 0 (re-run after 7f37e0b7).
• eslint@8 public/*.js: 0 errors. There are 88 warnings, all pre-existing no-unused-vars.

Browser validation (Playwright with Chromium, local server on the migrated and freshened test-fixtures/e2e-fixture.db; run on 8b8b5bcd, not repeated after 7f37e0b7)

The scenario:

1. Select observer A and type ADVERT.
2. Click Clear.
3. Check the All rows, triggers, localStorage, the URL and the request count.
4. Select observer B.
5. Check that only B is selected in the menu, localStorage and URL.

Results:

• This branch: pass at 1440×900 and 1024×768, with exactly 1 packet reload per Clear.
• master: fails at both viewports. After Clear the All rows are unchecked. After picking B, both observers are checked and stored.

test-filter-ux-e2e.js (in CI): 12/12 on this branch and on master.

The sandbox blocks the Leaflet CDN (unpkg.com), so every page logs L is not defined. That comes from the environment and is the same on master.

Performance. Not a hot path. Clear rebuilds two small menus once per click.

Not verified

• No manual test on a real device or on staging.
• Mobile width (below 768 px) was not driven in the browser. The Clear logic is the same code at every width.
• The 7f37e0b7 fix is covered by unit tests only, not re-driven in a browser.

Observed but out of scope (follow-ups)

• The observer change handler has the same URL-dropping problem: it calls updatePacketsUrl() and so drops ?obs=/?viewPath= from a detail deep link. This is on master too and is deliberately not changed here.
• init() also calls updatePacketsUrl() on page load (after wiring the Clear handler), which rewrites the query the same way. Also on master; left for a separate issue.
• init() registers RegionFilter.onChange(...) on every mount, and destroy() never removes it. After several SPA remounts, a region change therefore reloads packets more than once. Clear is not affected, because RegionFilter.setSelected() does not notify listeners.

Overlap with my other open PRs

None: no other open PR of mine touches these files.

🤖 Generated with Claude Code

https://claude.ai/code/session_019TcZHooUiiknVWbECVWzk8

---

Generated by Claude Code

[dborup]

Independent review of 8b8b5bcd

Verdict: APPROVE with nits. This is a recommendation only; merging is the owner's call.

Reviewed head: 8b8b5bcdfa038418e381c1b08bfd53179797e42c (unchanged before and after the review). Work done on a git archive of the PR head and of origin/master (85bfee49).

Labels: [F] freshly verified by me · [T] taken from the PR text · [A] assumption · [K] known limitation.

Findings

1. P3 (pre-existing, not introduced by this PR). An SPA remount forgets the observer/type selection while localStorage keeps it. destroy() sets filters = {} (public/packets.js:1405), and the localStorage values are only read at script load (public/packets.js:718-719).
   • Scenario: pick observer B and type GRP_TXT, go to #/nodes, then back to #/packets. The menus show "All", but meshcore-observer-filter and meshcore-type-filter still hold B and 5. A hard reload brings them back.
   • Reproduced [F] in a browser, identical on master and on the PR head.
   • Effect on this PR: the test "SPA remount: a new mount starts from an empty selection after Clear" (test-issue-121-clear-filters-selection.js:258) would start empty whether or not Clear was pressed. Its value is in the second half (Clear and a new pick after remount), and that part is red on master.
   • Not blocking. A good candidate for a separate issue.
2. nit. updatePacketsUrl() in the type handler (public/packets.js:1850) now rewrites the hash on every type click.
   • buildPacketsQuery() only emits known filter params, so a hash like #/packets/<hash>?viewPath=1 loses viewPath=1 when the type changes.
   • The observer handler on master already behaves this way, so this is not a new kind of behaviour.
   • Not reproduced [A]; derived from the code only.
3. nit. The new test finds the code by text markers. extractFilterSection() needs the comment // --- Observer multi-select --- and the exact clearBtn.addEventListener line. It fails loudly through assert if the markers move, so the risk is low. It does test a sliced string of the real file, not the module itself.
4. Positive, not mentioned in the PR text. On master the type trigger kept the title Selected: Advert after Clear, even though its text showed "All Types". The PR head resets it to Filter by packet type [F], which meets the "matching trigger labels/titles" criterion.

Metadata

• [F] Both commits (a443333c, 8b8b5bcd) have author and committer dborup <kontakt@meshview.dk>.
• [F] Base is origin/master 85bfee49. merge-tree against master has no conflicts.
• [F] No closing keywords ("Relates to fix(packets): Clear Filters must reset observer and type selection state #121"); closingIssuesReferences = 0; no @mentions; no full upstream URLs (only Kpa-clawbot/CoreScope#2015 in code format).
• [F] .github/workflows/deploy.yml: only 2 added test lines (node test-issue-121-…, node test-clear-filters.js). The github.repository == 'Kpa-clawbot/CoreScope' lines, triggers, permissions and jobs are untouched.
• [F] Overlap with batch PR fix(analytics): treat the distance index's 202 as a transient building state #133 (issue fix(analytics): treat lazy distance-index 202 responses as transient #120): both touch deploy.yml and test-all.sh, on different lines. A sequential merge-tree master → fix(packets): empty observer/type selections on Clear Filters #132 → fix(analytics): treat the distance index's 202 as a transient building state #133 has no conflicts.
• [F] CI on 8b8b5bcd is complete: Go Build & Test, Playwright E2E and Docker are SUCCESS; the rest are SKIPPED.

Acceptance criteria (issue #121)

Criterion	Result
Empty both Sets and rebuild the menus from an empty state	Met [F]: code, and mutants M1 to M3
"All Observers"/"All Types" checked, triggers and titles match	Met [F]: browser at 1440×900 and 1024×768, plus the unit test (master: both "All" unchecked, stale title)
URL, localStorage and filters hold no stale values	Met [F]: after Clear, ls* is null and the hash is #/packets; after B, only B/5 in the menu, localStorage and URL
Other filters unchanged	Met [F]: unit test "the other filters…", test-clear-filters.js 8/8, test-filter-ux-e2e.js 12/12
Clear gives exactly one reload/render	Met [F]: exactly 1 /api/packets? request per Clear in the browser; mutant M6 (double loadPackets) is caught
Regression test: A, Clear, B; repeated Clears; SPA remount	Met [F], with the caveat in finding 1

Test-first and mutants

• [F] The test file is unchanged from commit A to the head. On master: 2 passed, 6 failed. On the head: 8 passed, 0 failed.
• [F] test-clear-filters.js: master 6/2 (the location is not defined harness failure), head 8/0.
• [F] Six mutants of public/packets.js, all caught:

Mutant	test-issue-121	test-clear-filters
M1 drop selectedObservers.clear()	4 fail	1 fail
M2 drop selectedTypes.clear()	3 fail	1 fail
M3 drop buildObserverMenu() in Clear	1 fail	1 fail
M4 drop updateTypeTrigger() in Clear	1 fail	1 fail
M5 drop updatePacketsUrl() in the type handler	1 fail	green
M6 double loadPackets() in Clear	1 fail	green

Suites run locally (master vs head)

• [F] test-packet-filter.js 92/0, test-packet-filter-ux.js 19/0, test-packet-filter-time.js 20/0, test-aging.js 19/0. Identical on both.
• [F] test-frontend-helpers.js 705/2 and test-packets.js 115/13. The failing test names are the same on master and the head, so the failures are pre-existing. This matches the PR text.
• [F] Playwright against a local Go server on a freshened and migrated test-fixtures/e2e-fixture.db, with CI's seed block:
  • test-filter-ux-e2e.js: 12/0.
  • My own scenario at 1440×900 and 1024×768: only a type selected, so Clear is visible; observer A and type ADVERT; Clear; observer B and type GRP_TXT; remount. The head passes every Clear step. Master fails: Clear is hidden with only a type selected, "All" is unchecked after Clear, and after B obsA,obsB and 4,5 are stored. No page errors.
  • test-e2e-playwright.js stops locally (fail-fast) after 6 tests on "Version info lives on Perf dashboard", identically on master and the head. I put this down to my local environment. CI's Playwright job is green.
• Go: no Go changes, so I did not run go test locally. CI's Go job is green [F].

Performance and security

• [F] Not a hot path. Clear rebuilds two small menus (O(observers + types)) once per click.
• [F] No new DOM sinks. The menu HTML is unchanged, and observer names still go through escapeHtml. No Go, API or DB changes, so the mode=ro invariant and the map[string]interface{} rule are not affected.

Not verified

• I did not run scripts/check-xss-sinks.sh --diff origin/master or eslint myself [T].
• Mobile widths below 768 px, real devices and staging were not tested. The PR's "Not verified" list says so, and that list is honest and complete.
• The rest of test-e2e-playwright.js after the local fail-fast stop. I rely on CI's green Playwright job for it.
• I did not check the region-listener leak on repeated mounts that the PR lists as out of scope [T].

[dborup]

Review feedback addressed (commit 7f37e0b7)

1. Type handler no longer calls updatePacketsUrl(), so #/packets/<hash>?obs=123&viewPath=1 keeps its query when a type is picked.
2. Clear-button visibility is split out into updateClearFiltersVisibility(). updatePacketsUrl() uses it, and the type handler calls only it.
3. Tests first, red on 8b8b5bcd (8 passed, 2 failed): ?obs=123 and ?obs=123&viewPath=1 keep the hash and show Clear. test-clear-filters.js now also extracts the new function.
4. Results: test-issue-121-clear-filters-selection.js 10/0, test-clear-filters.js 8/0, test-packet-filter.js 92/0, test-packet-filter-ux.js 19/0, test-packet-filter-time.js 20/0, check-xss-sinks.sh --diff origin/master clean. Mutants: updatePacketsUrl() back in the type handler gives 8 passed, 2 failed; visibility call removed gives 7 passed, 3 failed.
5. Not changed: the observer handler (same URL-dropping on master) and the init() call to updatePacketsUrl() on load. Both are listed as follow-ups in the description.
6. The commit was amended once before pushing, to use the account's noreply author address. Nothing on origin was rewritten; this is a plain fast-forward push.

---

Generated by Claude Code

[dborup]

Independent review of 7f37e0b7 (delta since 8b8b5bcd)

Verdict: BLOCK, on commit identity only. The code change itself is approvable (it would be APPROVE with nits). This is a recommendation only; merging is the owner's call.

Reviewed head: 7f37e0b7bea34ce5dc08322262463e0474a359c2 (unchanged before and after the review). This is a fast-forward on top of the previously reviewed 8b8b5bcd, one commit. My earlier review of 8b8b5bcd still stands for everything outside this delta.

Labels: [F] freshly verified by me · [T] taken from the PR text · [A] assumption · [K] known limitation.

Findings

1. P2, process. The new commit has the wrong identity. 7f37e0b7 has author and committer dborup <3627142+dborup@users.noreply.github.com> [F]. This batch requires dborup <kontakt@meshview.dk>, and the first two commits (a443333c, 8b8b5bcd) use it.
   • The follow-up comment says the commit "was amended once before pushing, to use the account's noreply author address". That was a deliberate change, but in the wrong direction for this batch.
   • Fix: re-author the commit as dborup <kontakt@meshview.dk>, for example with git commit --amend --reset-author using that identity, then force-push the branch. Nobody else builds on this draft branch.
   • This is the only reason for BLOCK.
2. P3 (pre-existing, from my previous review, not addressed and not listed as a follow-up). An SPA remount still forgets the observer/type selection while localStorage keeps it [F]: reproduced again on 7f37e0b7 at 1440×900 and 1024×768. The body's "Observed but out of scope (follow-ups)" section does not mention it. Suggestion: add it there so it is not lost.
3. My earlier nit about the type handler rewriting the hash is resolved [F]: the type handler now calls only updateClearFiltersVisibility(). The remaining URL-dropping in the observer handler and in init() is on master and is now listed as a follow-up [T], which is fair.

What the delta does

• updateClearFiltersVisibility() is split out of updatePacketsUrl() (public/packets.js:795). updatePacketsUrl() still calls it, so every existing caller behaves as before.
• The type handler (public/packets.js:1853) no longer touches the URL. #/packets/<hash>?obs=…&viewPath=1 survives a type pick.

Test-first and mutants

• [F] The new tests run against 8b8b5bcd give 8 passed, 2 failed (both ?obs= cases lose the query). On 7f37e0b7 they give 10 passed, 0 failed.
• [F] Mutants against 7f37e0b7, all caught:

Mutant	test-issue-121	test-clear-filters
N1 updatePacketsUrl() back in the type handler	2 fail	green
N2 no visibility refresh in the type handler	3 fail	green
N3 updatePacketsUrl() no longer refreshes visibility	1 fail	1 fail
N4 visibility ignores filters.type	3 fail	green
M1 drop selectedObservers.clear() (re-run)	4 fail	1 fail
M2 drop selectedTypes.clear() (re-run)	3 fail	1 fail

Suites and browser

• [F] test-issue-121-clear-filters-selection.js 10/0, test-clear-filters.js 8/0, test-packet-filter.js 92/0, test-packet-filter-ux.js 19/0, test-packet-filter-time.js 20/0, test-aging.js 19/0.
• [F] test-frontend-helpers.js 705/2 and test-packets.js 115/13, the same pre-existing failures as on master.
• [F] Browser (Playwright/Chromium, local Go server on the freshened and migrated fixture):
  • The A → Clear → B scenario at 1440×900 and 1024×768 passes every Clear step: "All" checked, triggers and title reset, localStorage empty, hash #/packets, exactly 1 /api/packets? request per Clear, only B/5 after the new pick. No page errors.
  • Deep link #/packets/<hash>?obs=123&viewPath=1, then pick type ADVERT: the hash is unchanged and Clear is visible.

Metadata

• [F] Fast-forward from 8b8b5bcd. merge-tree against the current origin/master (d264716c) has no conflicts.
• [F] No workflow change in the delta. No closing keywords, no @mentions, no full upstream URLs in the updated body.
• [F] CI on 7f37e0b7 is complete: Go Build & Test, Playwright E2E and Docker are SUCCESS; the rest are SKIPPED.

Not verified

• scripts/check-xss-sinks.sh and eslint on the delta [T]. The delta adds no DOM sinks [F].
• Mobile widths below 768 px, real devices and staging.