Repository navigation
fix(packets): keep ?obs=/?viewPath= on packet detail URLs (#147) - #167
Merged
Merged
Conversation
Red on master 727efca: - test-issue-147-packets-url-detail-params.js (24 of 29 fail): runs the real buildPacketsQuery/updatePacketsUrl/Clear handler in vm. Cold load, every filter type and Clear Filters drop ?obs= (and ?viewPath= while the View Path modal is open); selectPacket, the observation-row click and the detail onClose write the hash themselves and drop the filter params. buildPacketsQuery() golden output (= master) and the "list view gets no obs" / "viewPath never invented" guards pass on master by design. - test-packet-path-map.js: closing the modal must drop ?viewPath=1 from a #/packets/<hash> URL (1 new red, 2 guards green). - test-issue-147-packets-url-obs-e2e.js (7 of 7 fail): desktop + mobile Playwright; also shows that cold-load ?obs= never selects the observation (init() parses it from routeParam, which the router has already stripped of its query). test-clear-filters.js: its body-only extract of updatePacketsUrl() gets an unused `detail` parameter name so it keeps working once the function takes one; no assertion changed. New tests registered in test-all.sh and in the deploy.yml unit/E2E steps (test-packet-path-map.js was not run in CI yet). Relates to #147 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
updatePacketsUrl() is now the only writer of #/packets... and adds the
packet-detail params that buildPacketsQuery() does not own after the
filter params:
- ?obs= stays with the detail subpath on cold load (init()), every filter
change and Clear Filters.
- ?viewPath=1 describes the open View Path modal, so it is kept only while
#packetPathModal is open on the same packet; PacketPathMap.close() drops
it from a #/packets/<hash> URL (other params verbatim) so a refresh or a
copied link does not reopen a closed modal.
- selectPacket() (incl. the cold-load auto-select), the observation-row
click and the SlideOver onClose pass the new detail state as
updatePacketsUrl({ subpath, obs }) instead of writing the hash
themselves, so they no longer drop the active filter params (or
viewPath on cold load).
- init() reads ?obs= from the hash query: the router strips the query from
routeParam, so the old routeParam parse never ran and a cold-loaded
?obs= link always showed the first observation.
buildPacketsQuery() is unchanged (golden test). Per call the work is a
split of the current hash query and one getElementById: O(params), not
O(packets), and no API calls.
test-issue-147-packets-url-obs-e2e.js: catch a detail refetch still in
flight when the context closes and unroute before closing it (a teardown
race seen locally, 1 in 6 runs before; 5/5 green after).
Relates to #147
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Red on d0a9f28: - Escape with the View Path modal open closed the detail pane (desktop) or the SlideOver (800 px) too; it must close only the modal. - The detail's View Path button did not write ?viewPath=1, so a reload did not reopen the modal. - The standalone #/packet/<id> page ignored ?obs= on load, and an observation click rewrote its URL to the packets list URL. - updatePacketsUrl() wrote the list URL on any route; open() on an open modal dropped ?viewPath=1; the modal did not name its packet. New guards (green on d0a9f28; each catches a named mutant): an observation-row click and a SlideOver close keep the active filters in the URL (E2E, real clicks), viewPath only for a modal on the subpath's packet, and the default route is still written. The E2E closes the SlideOver with a real backdrop click: at 800 px its close button sits under the fixed top nav, so a real click on it hits the nav (pre-existing, CSS untouched). Relates to #147 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es viewPath (#147) Review findings on #167: - Escape: the View Path modal's handler now runs in the capture phase and stops the event. With the modal open, Escape closes only the modal and drops ?viewPath=1; the detail pane (desktop) or the SlideOver (641–1023 px) and ?obs= stay until the next Escape. - View Path button: after opening the modal it calls updatePacketsUrl(), which now writes ?viewPath=1 while the modal is open on the packet of the #/packets/<hash> subpath (the overlay records it in data-hash). A reload reopens the modal; closing drops the param. open() on an open modal replaces it without the close-side URL write, so viewPath stays. - Route guard: updatePacketsUrl() writes only while the packets list is the route (#/packets…, or the empty default route). - Standalone #/packet/<id>: a new writer, updatePacketPageUrl(), writes #/packet/<id>?obs=<id> on an observation click (other params verbatim); the page reads ?obs= on load. The router strips the query before resolving the route, so this fits its format and the URL keeps describing what is shown. - The "only writer" comment names the other hash writers (packet-path-map.js dropViewPathParam, touch-gestures.js row actions). Relates to #147 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
Review feedback addressed (commits
Tests (local; CI fixture prepared as in deploy.yml: freshen, seed SQL, migrate, seed-2073):
Mutants (each run in a copy, with the server serving the mutated files):
M3b, M7 and M8 all evade the static writer-count test, so only behaviour tests catch them. Browser (local server, real clicks and keys):
Side effect: with the route guard, leaving Packets with the SlideOver open (clicking Nodes) now keeps Fork guards: 9 before and after. No deploy. |
dborup
marked this pull request as ready for review
October 3, 2026 08:03
dborup-agent
pushed a commit
that referenced
this pull request
Oct 3, 2026
Conflicts were only in test registration: - .github/workflows/deploy.yml: master's version. The JS unit step now runs test-all.sh (#174), so the two explicit test-issue-165 lines are no longer needed there. - test-all.sh: master's version plus the two #165 files, in its new `run` format. public/packets.js (including #167) merged without conflicts. deploy.yml keeps 9 fork guards. sh test-all.sh: 202 passed, 0 failed. Relates to #165 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ANjbSkzh7MxgT51Dzd7xeZ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #147
Plan (AGENTS.md rule 5: written here because the reviewer was not present)
Milestone 1, tests (commit
2d3f92de, red on master727efca0)test-issue-147-packets-url-detail-params.js: runs the realbuildPacketsQuery/updateClearFiltersVisibility/updatePacketsUrland the real Clear Filters handler body frompublic/packets.jsinvm.createContext. It also loads the realurl-state.jsfor sort.test-packet-path-map.js: 3 new cases for?viewPath=1when the modal opens and closes.test-issue-147-packets-url-obs-e2e.js: Playwright on desktop (1400×900) and mobile (390×844, touch).test-all.shand in thedeploy.ymlunit and E2E steps.test-packet-path-map.jswas not run in CI before, and is now. The fork guards are unchanged: 9 ×github.repository == 'Kpa-clawbot/CoreScope'.Milestone 2, fix (commit
d0a9f28e): see Changes below.Browser validation: a local server with a fixture copy, see below.
Config and customizer: nothing new is configurable.
The
viewPathdecision?viewPath=1stays in the URL only while the View Path modal is open on the same packet. When the modal closes, the param is dropped.?obs=always stays together with the detail subpath.Why:
viewPath=1means "this modal is open", so the param should live as long as the modal does.viewPathsurvived every filter change, a refresh or a copied address-bar link would reopen a modal that the user had already closed.viewPathwere consumed once on load, it would have to be removed by the cold-loadupdatePacketsUrl()call while the modal is still visible. That breaks the issue's first acceptance criterion.How:
updatePacketsUrl()keepsviewPath=1only while#packetPathModalexists and the subpath is unchanged.PacketPathMap.close()dropsviewPath=1, and only from#/packets/<hash>URLs. The other params stay verbatim.open()callsclose()first, but that only removes the param when a modal actually existed. A cold-loaded link therefore keeps it.Tests:
viewPath: kept on a filter change while the View Path modal is openviewPath: dropped once the modal is closed …close() drops ?viewPath=1 …andopen() on a cold-loaded ?viewPath=1 link keeps it …desktop: ?viewPath=1 is kept while the modal is open and dropped when it closesChanges
updatePacketsUrl(detail)is now the onlyhistory.replaceStatewriter inpackets.js. A test enforces this.buildPacketsQuery(), followed by the detail params that function does not own.init()), it carries over the current subpath and?obs=.{ subpath, obs }, it writes a new selection.selectPacket(), the observation-row click and the SlideOveronClosenow callupdatePacketsUrl({ subpath, obs }). Before, they wrote the hash themselves and dropped the active filter params.selectPacket()also runs as the cold-load auto-select, so on master it droppedviewPathfrom a?obs=…&viewPath=1link.Cold-load
?obs=was never applied.init()parsed it fromrouteParam, but the router (app.jsnavigate(),hash.split('?')[0]) already strips the query. A#/packets/<hash>?obs=<id>link therefore always showed the first observation.init()now reads it fromgetHashParams(). This was verified on master in the browser:?obs=2showed observation 509.buildPacketsQuery()is unchanged. A golden test pins its output to master's while the hash carriesobs/viewPathand the modal is open.Clear Filters works as before (#121/#132). Filter params are removed. The detail subpath and
?obs=stay, like the subpath did before.viewPathfollows the modal rule. The existing Clear Filters tests pass.test-clear-filters.jsextracts only the function body. It gets the unused parameter namedetailso it can run the new signature. No assertion changed.Upstream: the
obspart also exists upstream asKpa-clawbot/CoreScope#2091.Perf
updatePacketsUrlruns on every filter change. Its added work is:getElementById.That is O(URL params) and independent of the packet count (30K+). There are no new API calls, no DOM rebuilds and no loops over packets.
Tests
Red on master, green on branch. Commit-1 test files were run against a master copy:
727efca0test-issue-147-packets-url-detail-params.jstest-packet-path-map.jstest-issue-147-packets-url-obs-e2e.jsThe 5 tests that pass on master guard the decision and the invariants:
buildPacketsQuerygolden output, "list view gets no obs", "viewPath never invented", Clear on the list view, and the cold-load call site.Unchanged, run on master and branch with identical results:
test-packet-filter.js92/0test-frontend-helpers.js705/2: the 2 failures are thefavStarbaseline, identical on mastertest-clear-filters.js8/0test-issue-121-clear-filters-selection.js10/0test-packet-filter-ux.js19/0test-packet-filter-time.js20/0test-url-state.js18/0test-packet-detail-channel-xss.js8/0test-issue-1415-packets-layout.js26/0test-observer-detail-view-path.js5/0test-analytics-distance-view-path.js8/0test-path-inspector.jspassedtest-issue-1753-copy-url-slash.js4/0test-xss-escape-sinks.js34/0test-packets.js115/13: identical failing lines on master; this file is not in CIE2E against a local server, on a fixture prepared like CI (freshen, CI seed SQL, migrate,
seed-2073). Master and branch gave identical results:test-e2e-playwright.js132/0test-filter-ux-e2e.js12/0test-issue-1122-packets-filter-ux-e2e.js6/0test-issue-1122-details-row-clamp-e2e.js18/0test-issue-1128-packets-layout-e2e.js5/0test-issue-1692-packets-init-parallel-e2e.js1/0test-issue-1486-collapse-reopens-detail-e2e.js4/0test-slideover-1056-e2e.js27/0test-slideover-1168-munger-e2e.js3/0test-issue-1146-path-link-contrast-e2e.js11/0test-path-inspector-coverage-e2e.js10/0There is no separate Clear Filters E2E for #121/#132; those tests are unit tests. The fixture has no packet with two observations that the API returns, so the new E2E stubs the detail response (
context.route) with one extra synthetic observation.Correction to commit
d0a9f28e: its message says the E2E teardown race was seen "1 in 6 runs". It was actually 2 of 3 local branch runs before the hardening. After the hardening, 5/5 runs were green.Mutants (run in a copy)
cold load #/packets/<hash>?obs=123 keeps ?obs=123obsonly, dropviewPathcold load …?obs=123&viewPath=1 (modal open) keeps both,viewPath: kept on a filter change while the View Path modal is open,cold-load auto-select of the same packet keeps ?viewPath=1 …viewPathregardless of the modalviewPath: dropped once the modal is closed …,Clear Filters on a detail URL drops viewPath=1 when the modal is closedviewPathonly, dropobsfilter change keeps ?obs=: observerbuildPacketsQueryaddsobsitselfbuildPacketsQuery() output is unchanged and never contains detail params,list view (#/packets, no detail) gets no ?obs=, plus 4 detail-writer tests and all 10 tests intest-issue-121-clear-filters-selection.jsviewPathdrop on closeclose() drops ?viewPath=1 from a #/packets/<hash> URL …obsread removeddesktop: cold load ?obs=<id> selects that observation …selectPacketwrites the hash itself againupdatePacketsUrl() is the only history.replaceState() writer in packets.js; E2Ecopied URL opened in a new tab …and?viewPath=1 is kept while the modal is open …Browser validation
Done with a local server on a fixture copy, with one extra observation added to packet
09f282f70cc1ff60. Steps:#/packets/09f282f70cc1ff60?obs=2. Observation 2 is selected ("observation 2 of 2"); master selects 509.#test. The URL became…?timeWindow=180&channel=%23test&obs=2.…?timeWindow=180&channel=%23test&obs=509.#/packets/09f282f70cc1ff60?obs=509and the Clear button is hidden.…?timeWindow=60&obs=2&viewPath=1with a full reload. The modal is open and the URL is intact. Clicking × gave…?timeWindow=60&obs=2.Not covered and follow-ups
closeDetailPanel(), × or Esc on the split pane) does not write the URL, on master or here. After closing,#/packets/<hash>?obs=…stays in the address bar. This is outside fix(packets): updatePacketsUrl drops ?obs= and ?viewPath= on load and on filter changes #147 and a candidate follow-up.#/packet/<id>: an observation-row click there rewrites the URL to#/packets/<hash>?…without navigating. That is existing behaviour; it now also carries the packets page filter params.viewPath=1. The param is only kept when it was already in the URL.🤖 Generated with Claude Code