Repository navigation
fix(packets): URL and modal leftovers from #167 (#180) - #204
Conversation
) A #/packets/<hash> subpath sets filters.hash on load, so a Clear Filters that kept the subpath was undone by a reload: the list went back to one row and the Clear button came back. Clear now closes the detail and writes the list URL #/packets?… (decision for #180 item 2). The rest of the #121/#132 Clear behaviour and buildPacketsQuery() are unchanged. closeDetailPanel() re-renders the rows only when a row was selected, so Clear with no detail open still renders nothing extra (#121 test). Tests: the #147 unit and E2E Clear steps now expect #/packets; new test-issue-180-packets-detail-close.js and test-issue-180-packets-url-modal-e2e.js (Clear → reload at 1400 px). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tton (#180) At 641–1023 px the SlideOver panel (z-index 1001) sat under the sticky .top-nav (1100), so its header and × were covered and a real click on × hit the nav. The SlideOver is a modal dialog (role=dialog, aria-modal, focus trap), so its backdrop and panel now use the existing --z-modal-backdrop / --z-modal tokens, per the z-index scale's migration policy. The close button's tap target is 48×48 (Kpa-clawbot#2052 house rule). E2E at 641, 800 and 1023 px: × is the topmost element at its centre, at least 48×48, and a real click closes the SlideOver. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ow count Clear restores the default 15 min window, and the fixture's packets can age out of it during a long E2E run, so "more than one row after Clear" is not stable. The step now waits for the /api/packets response and checks that the reload shows the same count, URL and Clear state as Clear did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat layer's (#180) The modal's capture-phase Escape handler stopped every Escape, so with the global search (Ctrl+K) or the nav More menu open over the modal, the first Escape closed the modal underneath and the search/menu stayed. The handler now leaves Escape alone when focus is in a floating layer drawn over the modal: outside it, inside a position:fixed ancestor that is not a packets detail surface the modal opens over (SlideOver, mobile sheet), and topmost at its own centre. Focus on the page, a control under the modal's backdrop, or the sticky top nav still closes the modal, and the #167 "Escape closes only the top layer" behaviour over the detail pane and the SlideOver is unchanged. Tests: unit cases for each layer kind in test-packet-path-map.js; E2E at 1400 px for the search and the More menu over the modal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d does not reopen it (#180) The modal stayed open over the next page after Back, and the packets history entry kept ?viewPath=1 (#167), so Forward onto it reopened a modal the user had already closed. - The modal closes on a hashchange to another path (Back/Forward, a nav link). A query-only change keeps it. - open() marks the #/packets/<hash> entry it opened on in history.state (no URL write). When the modal is closed while another route is shown, that mark goes on a bounded list (50), and restore() -- now used by packets.js init() for ?viewPath=1 -- does not reopen it. The cold-load updatePacketsUrl() then drops ?viewPath=1, so the URL says what is shown. A new link to the same URL (no state) and a reload of an entry whose modal was open still open it. - updatePacketsUrl() keeps history.state instead of wiping it. Tests: unit cases in test-packet-path-map.js and the #147 unit test; E2E at 800 and 1400 px: Back closes the modal, Forward does not reopen it and drops viewPath. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Escape handler calls closeDetailPanel(), which only reset the desktop pane (#pktRight), so the ≤640 px bottom sheet never closed on Escape. It now closes the sheet too, the same way its × does. With the View Path modal open over the sheet, the modal's capture-phase handler still takes the first Escape. Tests: unit cases in test-issue-180-packets-detail-close.js; E2E at 390 px: Escape closes the sheet, and with View Path over it the first Escape closes only the modal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent3 PR#204 #180 — head 4c7c35eStatus: Items 2–6 of #180 are fixed and tested on this draft PR. Every new test fails against master and passes here, and at least one mutant per item is killed. CI is green on Evidence tags: [T] = tested (a command was run and its output read), [A] = analysis only (code reading, not executed), [K] = known leftover or limitation. Commits (base
|
| Commit | Item |
|---|---|
fac293fd |
2: Clear Filters on a detail URL also leaves the detail |
2a5bf642 |
3: the SlideOver stacks above the top nav; × is 48×48 |
6071dbab |
test-only: the item-2 E2E compares Clear with the reload instead of counting rows |
c3bf7190 |
4: Escape in a layer opened over the View Path modal is that layer's |
696fab84 |
5: the modal closes on route change; Back/Forward does not reopen it |
4c7c35e1 |
6: Escape closes the ≤640 px detail sheet |
Item 2: Clear Filters on a detail URL
| Decision | Clear also removes the detail subpath. The detail closes and the URL becomes #/packets?…, with no hash filter and no ?obs=/?viewPath=. I found no strong reason for the other option: letting a cold-loaded subpath select the packet without setting filters.hash would change what every existing #/packets/<hash> link shows. buildPacketsQuery() is unchanged; the #147 golden test passes [T]. |
| Fix | The Clear handler calls closeDetailPanel() and updatePacketsUrl({ subpath: '', obs: null }). closeDetailPanel() re-renders only if a row was selected, so the #121 test "Clear renders nothing extra" still holds [T]. |
| Tests | Unit, #147 file: 3 Clear cases now expect #/packets. They are red on master (3 fail) and green here [T]. Unit, new test-issue-180-packets-detail-close.js: no render when nothing is selected; red on master [T]. E2E at 1400 px: detail URL → Clear → #/packets, detail closed, Clear hidden → reload gives the same count, Clear hidden, empty hash input [T]. The #147 E2E Clear step is updated to the decision [T]. |
| Mutants | M2a: Clear calls updatePacketsUrl() without a detail argument. Killed: 3 unit cases and the E2E fail [T]. M2b: Clear does not close the detail. Killed: the #147 unit, the #121/#132 harness and the E2E fail [T]. |
Item 3: SlideOver × under the top nav (641–1023 px)
| Fix | .slide-over-backdrop and .slide-over-panel now use the existing var(--z-modal-backdrop) and var(--z-modal) tokens instead of 1000/1001, so they sit above .top-nav (1100). .slide-over-close is 48×48 (it was 44). No colours changed. |
| Tests | E2E at 641, 800 and 1023 px. elementFromPoint at the × centre returns ×, the button is at least 48×48, and a real page.click closes the SlideOver. On master the element at the × centre is top-nav or theme-toggle-track, so all 3 fail [T]. A screenshot at 800 px shows the panel header and × above the nav [T]. |
| Mutants | M3a: panel back to z-index: 1001. Killed at all 3 widths; the backdrop is now on top of × [T]. M3b: 44 px. Killed at all 3 widths [T]. |
| Behaviour change | While the SlideOver is open, its backdrop covers the top nav and the ≤768 px bottom nav. A click there closes the SlideOver instead of navigating [A]. test-slideover-1056/1168, test-bottom-nav-1061 and the gesture E2Es are green [T]. |
Item 4: Escape swallowed by the View Path modal
| Fix | The capture-phase handler leaves Escape alone when the focused element is in a floating layer drawn over the modal. That means: it is outside the modal; it has a position: fixed ancestor; that ancestor is not the SlideOver or the mobile sheet; and the element is topmost at its own centre. Otherwise the handler still stops the event and closes the modal (#167). |
| Tests | Unit, test-packet-path-map.js: 6 cases. The search and fixed-menu cases are left alone; the covered-control, SlideOver, mobile-sheet and sticky-top-nav cases close the modal. The 2 "left alone" cases are red on master [T]. E2E at 1400 px: Ctrl+K → Escape closes the search while the modal stays with viewPath=1, and the next Escape closes the modal. The same flow works with the nav More menu. Both are red on master [T]. The #147 tablet Escape step still passes [T]. |
| Mutants | M4a: always swallow Escape. Killed: 2 unit cases and 2 E2E steps fail [T]. M4b: no SlideOver/sheet exclusion. Killed: 2 unit cases and the #147 tablet step fail [T]. M4c: no hit test. Killed: the covered-control unit case fails [T]. |
Item 5: Back/forward reopens a closed View Path modal
| Fix | The modal closes on a hashchange to another path; a change to the query only keeps it. open() marks the #/packets/<hash> history entry with an id in history.state, without writing a URL. If the modal is closed while another route is shown, the id goes on a list capped at 50. PacketPathMap.restore(), now used by init() for ?viewPath=1, does not reopen a modal whose entry is on that list. updatePacketsUrl() now keeps history.state instead of writing null, and drops viewPath when no modal is open. A new link to the same URL and a reload of an entry whose modal was open still open the modal [T, unit]. |
| Tests | Unit: 9 cases in test-packet-path-map.js and 1 in the #147 file. The relevant ones are red on master [T]. E2E at 800 and 1400 px: #/nodes → packet → View Path → Back closes the modal → Forward does not reopen it and drops viewPath. Red on master: "the View Path modal stayed open over #/nodes after Back" [T]. |
| Mutants | M5a: no hashchange listener. Killed: 4 unit cases and both E2E steps fail [T]. M5b: restore() ignores the closed list. Killed: 3 unit cases and both E2E steps fail [T]. M5c: updatePacketsUrl() writes null state. Killed: the #147 unit case and both E2E steps fail [T]. M5d: init() calls open() instead of restore(). Killed: both E2E steps fail [T]. |
Item 6: Escape never closes the mobile detail sheet
| Fix | closeDetailPanel(), which the Escape handler calls, also removes .open from #mobileDetailSheet, the same way its × does. |
| Tests | Unit, test-issue-180-packets-detail-close.js: the sheet closes and the selection clears; red on master [T]. E2E at 390 px (mobile): Escape closes the sheet. With View Path open over the sheet, the first Escape closes only the modal and the next closes the sheet. Both are red on master [T]. |
| Mutant | M6: the sheet is not closed. Killed: the unit case and both E2E steps fail [T]. |
Tests, browser and E2E
- [T]
sh test-all.sh: 202 files passed, 0 failed (locally and in CI).node test-frontend-helpers.js: 707 passed, 0 failed. - [T] The new E2E
test-issue-180-packets-url-modal-e2e.jsgives 10/10 on this branch. Against a second server that serves master'spublic/with the same fixture, it gives 0/10. - [T] 31 E2E files from
deploy.yml(packets, path map, SlideOver, filters, gestures, nav, a11y axe,test-e2e-playwright.js) all exit 0. They ran against a local Go server on this worktree withe2e-fixture.db, prepared as in CI: freshen, grouped-row seed SQL,corescope-migrate, [feat] node detail, separate flood-adv from zero-hop adv Kpa-clawbot/CoreScope#2073 seed. I confirmed the server's cwd and-publicpath, and that the servedpacket-path-map.jscontainsrestore. I stopped servers by the pid thatlsoffound on the port. - [T]
scripts/check-xss-sinks.sh --diff origin/masterexits 0 with no findings. - [T] Registration: the unit file is in
test-all.sh, and the E2E is in thedeploy.ymlPlaywright step after the fix(packets): updatePacketsUrl drops ?obs= and ?viewPath= on load and on filter changes #147 E2E. The fork-guard count is still 9. - [T] Screenshots: at 800 px, the SlideOver header and × are above the nav. At 1400 px, after Ctrl+K and Escape, the search is closed and the modal is still open.
CI (run for head 4c7c35e1)
| Job | Result |
|---|---|
| ✅ Go Build & Test | success (test-all.sh 202/0, packet-path-map.js 64/0) |
| 🎭 Playwright E2E Tests | success (#180 E2E 10/10, #147 E2E green) |
| 🏗️ Build & Publish Docker Image | success |
| 📦 Release Artifacts | skipped (fork guard) |
| 🚀 Deploy Staging | skipped (fork guard) |
| 📝 Publish Badges & Summary | skipped (fork guard) |
Known leftovers
- [K][T] At 641–1023 px, the View Path modal (
.modal-overlay, z-index 200) is drawn under the SlideOver. This was already the case on master at z-index 1001. Fixing it means raising the modal above the SlideOver, and then the global search overlay (300) would also have to go above it. That is a separate stacking decision. - [K][T] Escape in the global search also closes the desktop detail pane under it. The cause is that the packets Escape handler listens on
document. Master does the same, even without the modal. With this PR, the modal itself stays open. - [K][A] With the modal open, if focus is on a tab of the fixed bottom nav (≤768 px), Escape is left to that layer. The bottom nav has no Escape handler, so the modal stays open until focus moves.
- [K][A] Closing the desktop detail pane or the mobile sheet (× or Escape) still does not write the URL. This was out of scope and is noted in the fix(packets): updatePacketsUrl drops ?obs= and ?viewPath= on load and on filter changes #147 E2E.
- [K][A] Item 1 has a unit test for the route guard but still no E2E (not part of this task). Since item 3, the SlideOver backdrop covers the nav, so the item-1 scenario can no longer be reached by clicking the nav.
- [K][T]
test-packets.jsfails 13 cases on master too. It is not intest-all.shand was not changed here.
Review — CS-pve-agent1 PR#204 packets-url-modal — head 4c7c35eDom: APPROVE with nits Independent read-only review of head Evidence: [T] = test or CI run by me, [A] = analysis (code reading), [K] = taken from the author's report, not re-run. Findings
No P0–P2 findings. Verification items1. Items 2–6 of #180, each fixed and tested
2. No regression in Clear Filters (#121/#132), and 3. Conflict with #201 (head 4. Tests
Own mutants (applied to a copy of the merged tree; unit tests, plus E2E against a separate mutant server for CSS/UI):
5. Rules
Not verified
|
Relates to #180
Fixes the open items 2–6 of #180 (URL and modal leftovers found in the #167 review). Item 1 was fixed by #167 and is not touched here.
Plan
One commit per item, each with its test and fix, plus one test-only commit:
#/packets?….There are no new config values, so nothing is needed in the customizer.
Decision for item 2
Clear Filters also removes the detail subpath. The detail closes,
?obs=and?viewPath=1go with it, and the URL becomes the list URL#/packets?…with no hash filter.#/packets/<hash>subpath setsfilters.hashon load. If Clear kept the subpath, a reload undid the Clear. With the subpath gone, the URL describes what is shown (AGENTS.md deep-link rule).filters.hash. That would change what every existing#/packets/<hash>link shows, so I did not choose it.buildPacketsQuery()is unchanged; the fix(packets): updatePacketsUrl drops ?obs= and ?viewPath= on load and on filter changes #147 golden-value test still passes.Items
Item 2: Clear Filters on a detail URL
selectedObservationId, callscloseDetailPanel()and thenupdatePacketsUrl({ subpath: '', obs: null }).closeDetailPanel()now re-renders the rows only if a row was selected, so a Clear with no detail open still renders nothing extra (#121 test).test-issue-147-packets-url-detail-params.js: the Clear cases now expect#/packets(3 cases are red on master).test-issue-180-packets-detail-close.js: no render when nothing is selected. E2E at 1400 px: detail URL → Clear →#/packets, Clear hidden, detail closed → reload shows the same count, Clear hidden, empty hash input. The #147 E2E Clear step is updated to the decision.updatePacketsUrl()without a detail argument (keeps the subpath): 3 unit cases and the E2E fail. Clear does not close the detail: the unit tests (#147, #121/#132 harness) and the E2E fail.Item 3: SlideOver × under the top nav at 641–1023 px
.slide-over-backdropand.slide-over-paneluse the existing--z-modal-backdropand--z-modaltokens instead of 1000/1001. That puts them above the sticky.top-nav(1100), as the z-index scale's migration policy asks. The SlideOver is alreadyrole=dialog,aria-modaland focus-trapped..slide-over-closeis 48×48 (it was 44).elementFromPoint), is at least 48×48, and a realpage.clickon it closes the SlideOver and writes the list URL. On master, the nav (top-nav/theme-toggle-track) is at the × centre.z-index: 1001: all 3 widths fail (the backdrop is now on top of ×).min-width/height: 44px: all 3 widths fail on the size check.Behaviour change: while the SlideOver is open, its backdrop now covers the top nav and the ≤768 px bottom nav. A click there closes the SlideOver instead of navigating.
Item 4: Escape is swallowed by the View Path modal
position: fixedancestor; that ancestor is not one of the packets detail surfaces the modal opens over (SlideOver, mobile sheet); and the element is topmost at its own centre. Examples are the global search (Ctrl+K), the nav More menu, a hamburger menu, the More sheet and a filter popover. If focus is on the page, on a control under the modal's backdrop, or in the sticky top nav, Escape still closes the modal and stops the event (#167).test-packet-path-map.js: 6 cases (search, fixed nav menu → left alone; covered control, SlideOver, mobile sheet, sticky top nav → modal closes). The 2 "left alone" cases are red on master. E2E at 1400 px: Ctrl+K → Escape closes the search while the modal stays withviewPath=1, and the next Escape closes the modal. The same flow is tested with the nav More menu.Item 5: Back/forward reopens a closed View Path modal
packet-path-map.js: while open, the modal listens forhashchangeand closes when the hash path changes. A change to the query only keeps it open.open()marks the#/packets/<hash>history entry with an id inhistory.state; this changes no URL. If the modal is closed while another route is shown, the id goes on a list of closed entries, capped at 50. The newPacketPathMap.restore(hash)(used bypackets.jsinit()for?viewPath=1) does not reopen a modal whose entry is on that list. The cold-loadupdatePacketsUrl()then drops?viewPath=1, because no modal is open.updatePacketsUrl()keepshistory.stateinstead of writingnull. A new link to the same URL (no state) and a reload of an entry whose modal was open still open the modal.test-packet-path-map.js: 9 cases. They cover: the mark in state without a URL write; no mark off the packets route; route change closes the modal without a URL write; a query-only change keeps it; Forward after a route-change close; Escape-close while on another route; a new link still opens; a reload still opens; the 50-entry bound.test-issue-147-…:updatePacketsUrl()keepshistory.state. E2E at 800 and 1400 px: from#/nodes→ packet → View Path → Back closes the modal → Forward does not reopen it andviewPathleaves the URL.hashchangelistener: 4 unit cases and both E2E steps fail ("stayed open over #/nodes").restore()ignores the closed list: 3 unit cases and both E2E steps fail.updatePacketsUrl()writesnullstate again: the #147 unit case and both E2E steps fail.init()callsopen()instead ofrestore(): both E2E steps fail.Item 6: Escape never closes the mobile detail sheet
closeDetailPanel()(called by the packets Escape handler) also removes.openfrom#mobileDetailSheet, as its × does.test-issue-180-packets-detail-close.js: the sheet closes and the selection clears (red on master). E2E at 390 px (mobile): Escape closes the sheet. With View Path open over the sheet, the first Escape closes only the modal and the second closes the sheet.Tests run locally
sh test-all.sh: 202 files, 0 failed.node test-frontend-helpers.js: 707 passed.test-issue-180-packets-url-modal-e2e.js: 10/10 on this branch and 0/10 against a server that serves master'spublic/(same fixture). It is registered indeploy.ymlright after the fix(packets): updatePacketsUrl drops ?obs= and ?viewPath= on load and on filter changes #147 E2E. The fork guard count is unchanged at 9.deploy.ymlthat touch packets, path map, SlideOver, filters, gestures, nav and a11y all exit 0 against a local Go server withe2e-fixture.db, prepared as in CI (freshen, seed SQL,corescope-migrate, [feat] node detail, separate flood-adv from zero-hop adv Kpa-clawbot/CoreScope#2073 seed).scripts/check-xss-sinks.sh --diff origin/masteris clean.Performance
elementFromPoint.hashchangelistener exists only while the modal is open.closeDetailPanel()now skipsrenderTableRows()when no row is selected. Before this change, every Escape on the packets page re-rendered the visible rows.Known leftovers (not in this PR)
.modal-overlay, z-index 200) is drawn under the SlideOver. This was already the case at 1001, before this PR. Raising it above the SlideOver also needs the global search overlay (300) raised, so it needs its own stacking decision.pktEsc) listens ondocument. So Escape in the global search also closes the desktop detail pane under it. On master this happens even without the modal. With this PR, the modal itself stays.🤖 Generated with Claude Code