Skip to content

fix(path-inspector): remove unreachable map drawing branch (#143) - #345

Merged
dborup merged 4 commits into
masterfrom
codex/path-inspector-cleanup-143
Oct 7, 2026
Merged

fix(path-inspector): remove unreachable map drawing branch (#143)#345
dborup merged 4 commits into
masterfrom
codex/path-inspector-cleanup-143

Conversation

@dborup

@dborup dborup commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Relates to #143.

  • Remove the unreachable already-on-map branch from the standalone Path Inspector's showOnMap handler. It referenced map drawing globals that are not exported.
  • Keep the existing full-candidate handoff and navigation to the map. The map's embedded inspector keeps its own drawing handler unchanged.
  • Add a unit guard against the obsolete globals, exercise the actual candidate button handler, and extend the existing browser test to require standalone route rendering and handoff consumption.

This is dead-code cleanup, not a fix for a currently broken normal user flow. No new global, configuration, customizer, backend, dependency or deployment changes. No new API calls or hot-path work; no performance improvement is claimed.

Validation

  • Regression test failed before cleanup and passed after it.
  • Full frontend runner (sh test-all.sh): 227 test files passed, zero failures.
  • Path Inspector unit tests: 11 checks passed.
  • Chromium Path Inspector integration: 7 passed, including embedded-map and standalone route rendering.
  • Existing Path Inspector browser coverage suite: 10 passed (some existing cases are conditional on available candidates).
  • Broad Chromium browser regression: 132 passed, 3 fixture-dependent skips, zero failures.
  • git diff --check and XSS diff preflight passed.

Browser runs use an isolated localhost Go server and a disposable, freshened test fixture. No staging, production or upstream repository changes. Go source is unchanged; backend unit suites were not rerun for this frontend-only cleanup.

An independent subagent review will be posted on this PR as requested.

dborup
dborup commented Oct 7, 2026
View reviewed changes

@dborup dborup left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of commit 8b173f524dc3895352b26638ff713e160c5bfd3b against base 6444294.

No actionable findings in this change.

I inspected the fetched commit rather than working-tree files, including the complete diff, standalone button wiring, SPA routing and destruction lifecycle, map initialization consuming _pendingPathInspectorRoute, and the embedded map inspector calling its private drawing function directly. The removed branch is not needed by either supported UI flow; retaining the full candidate and navigating to the map preserves the existing handoff without introducing another global drawing API.

Independent validation: executed all 11 checks in test-path-inspector.js using the test and production source read directly from the reviewed Git commit; all passed. I also reviewed the added browser regression and existing embedded-inspector test, including assertions for a visible route and consumption of the pending handoff.

Scope limitation: I did not independently rerun the browser suite or full repository test suite. This is a focused review of the cleanup and its regression coverage, not a blanket assessment of unrelated map behavior or security.

@dborup

dborup commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Taking this PR over from here. Next: an independent review (and, where CI is red, a fix round first). The issue link was changed from a closing keyword to "Relates to" to match the fork's convention; the issue is closed manually after merge.

dborup and others added 3 commits October 7, 2026 14:48
@dborup @claude
test(map): reproduce leaving the map during an animated zoom (#345)
cad2fe4 
Leaflet 1.9.4 ends an animated zoom from a 250 ms setTimeout that
map.remove() does not cancel. Leaving the map mid-zoom (e.g. right
after a route's fitBounds) then throws "Cannot read properties of
undefined (reading '_leaflet_pos')". This is the page error that failed
test-path-inspector-e2e.js in CI on #345.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(map): make Leaflet's zoom transition-end timer inert on destroy (#…
184fa4e 
…345)

destroy() clears map._animatingZoom before map.remove(), so the
transition-end callback that Leaflet 1.9.4 schedules returns early
instead of reading the removed map pane.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup
Merge origin/master into codex/path-inspector-cleanup-143
bd31040
@dborup-agent

dborup-agent commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Rapport — CS-pve-agent2 PR#345 ci-fix — head bd31040

Review feedback addressed (commit 184fa4ea, merged up to bd310405)

Evidence tags: [T] run by me in this round, [A] derived from reading code or source, [K] taken from CI logs or GitHub.

The independent review found no actionable findings in the cleanup. The open item was the red CI.

  1. CI red: Playwright E2E. This was a real bug, not a flake. I added a test (cad2fe4b) and then the fix (184fa4ea).

    • Symptom [K]: run 37599535405, test-path-inspector-e2e.js: 6 passed, 1 failed. The failure was ✗ no page errors: Cannot read properties of undefined (reading '_leaflet_pos'). Every other suite in the job passed. This does not match the flaky: #180 packets URL E2E 'reload shows the same list' depends on fixture age (default 15-min window) #271/test(packets): make the Details advert-link E2E independent of fixture age (#244) #252 class (fixture age vs. time window) or test(server): data race between startBackgroundIndexBuilds and PacketStore.Load under -race (TestHashMigrate_LogsMaxWriteLockHold_215) #301 (Go -race).
    • Reproduction [T]: the standalone "Show on Map" flow, followed by leaving the map right after the route is drawn. This failed at 4× CPU throttling against a local Go server on a CI-prepared e2e-fixture.db, and passed unthrottled. The stack is Leaflet's _onZoomTransitionEnd → _move → _getNewPixelOrigin → _getMapPanePos.
    • Cause [A]: Leaflet 1.9.4 _animateZoom ends an animated zoom with setTimeout(bind(this._onZoomTransitionEnd, this), 250). map.remove() does not cancel that timer (_stop() only cancels flyTo and pan animations). If map.js destroy() removes the map during those 250 ms, for example right after a route's fitBounds, the callback reads the deleted map pane. The race was already in map.js before this PR. The PR's new standalone E2E step leaves the map right after a route draw, which made it visible on the slower CI runner.
    • Fix [A]: destroy() sets map._animatingZoom = false before map.remove(). _onZoomTransitionEnd returns early when that flag is false. The fix uses a private Leaflet field because Leaflet has no public API to cancel the timer. If a pending _tryAnimatedZoom rAF fires after removal, it is already inert, because _animateZoom returns when _mapPane is missing.
    • Test [T]: test-issue-123-map-lifecycle-e2e.js has a new step, "leaving during an animated zoom: Leaflet's transition-end timer is inert". It starts an animated setZoom, waits until the zoom is animating, then navigates away. Before the fix it failed 3/3 (14 passed, 2 failed). After the fix it passed 3/3 (16 passed, 0 failed). The throttled repro (4× and 6×, 3 rounds each) shows no page errors after the fix.
    • Scope: one statement plus a comment in map.js destroy(). No other behaviour changes. I did not edit the PR description; the map.js change is documented here.

  2. Merged origin/master (e8e99c4a) with a merge commit (bd310405). [T] There were no conflicts. Master did not touch the PR's files. The diff against master is the PR's three files plus public/map.js and test-issue-123-map-lifecycle-e2e.js.

  3. The PR's own claims hold. [T]

    • Unit: node test-path-inspector.js passes all checks, including the two new ones.
    • The standalone E2E step passes locally and in CI.
    • The removed branch is dead code [A]. window.drawPacketRoute and window.routeLayer are not exported by map.js. The map's embedded inspector uses its own handler.

Tests [T] (after merge, at bd310405)

Mutants [T] (each applied alone, then reverted)

# Mutant Killed by
M1 remove map._animatingZoom = false from destroy() lifecycle step "leaving during an animated zoom" (_leaflet_pos), plus the throttled repro
M2 restore the old already-on-map branch in path-inspector.js test-path-inspector.js: "standalone inspector must not depend on private map drawing globals"
M3 drop window._pendingPathInspectorRoute = candidate test-path-inspector.js ("preserve the full candidate"), plus the E2E standalone step (no route drawn)
M4 map init no longer deletes _pendingPathInspectorRoute E2E standalone step: "pending route was not consumed"

CI per job at bd310405 [K] (run 37641938861)

Notes

  • The PR is currently marked ready for review, not draft. That was its state when I took it over, and I did not change it.
  • No merge, no state change, no issue changes, no staging or production access.

@dborup-agent

dborup-agent commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Review — CS-pve-agent3 PR#345 — head bd31040

Dom: APPROVE with nits

Independent, read-only review. I read the code from git archive of the head and of the merged tree (git merge-tree --write-tree origin/master bd310405… → tree 512aa7ef, origin/master = 461803d9). The merge has no conflicts, and the diff against master is the PR's five files only. Evidence tags: [T] run by me, [A] derived from reading code or source, [K] taken from CI or GitHub.

Findings

# Severity Finding Evidence
1 nit, follow-up (out of scope, pre-existing) map.js has the same class of defect as #143. The mc-hide-1byte-hops-changed listener (public/map.js:439-440) re-renders a single route through typeof window.drawPacketRoute === 'function'. That global does not exist, so toggling "hide 1-byte hops" while a single route is on screen does not re-render it. This includes every route the Path Inspector hands off. map.js:1835 also has a dead if (window.routeLayer) window.routeLayer.clearLayers(), which is harmless because the local drawPacketRoute clears the layer itself. The PR's regex guard only scans path-inspector.js. #143's first acceptance criterion is still met, because the typeof guard means nothing calls the undefined function. I'd suggest a separate follow-up, not a change to this PR. [T] in the browser, after a Path Inspector route on the PR and on master: __mc_lastRouteDraw.kind === 'single', typeof window.drawPacketRoute === 'undefined', and dispatching the event calls neither draw function. [A] the code path.
2 nit (pre-existing test gap) The step "Show on Map" hands off to map page (hash → #/map) in test-path-inspector-coverage-e2e.js asserts nothing on the CI fixture. 2c,a1 returns no candidates, so the step returns early. My mutant R2 (no navigation) survives it. The PR's new step in test-path-inspector-e2e.js covers the flow properly, so this is non-blocking. [T] mutant R2
3 info The PR description does not mention the map.js change (one statement plus a comment in destroy()). It is documented only in the ci-fix report. Mentioning it in the description before merge would help the squash or merge message. [K]
4 info The map.js fix relies on Leaflet's private _animatingZoom flag. That is acceptable: Leaflet is pinned to 1.9.4 through the unpkg URL in index.html, there is no public API to cancel the timer, and the new lifecycle step goes red if a future Leaflet changes the behaviour. [A] [T]

No blocking findings.

1. Is the branch unreachable, and is the replacement identical?

Unreachable [A].

  • The router only mounts the standalone page for #/tools/path-inspector… (app.js:1379). The "Show on Map" buttons exist only in that page's rendered results, so while they are live, location.hash starts with #/tools/. Therefore indexOf('#/map') !== 0 is always true.
  • Leaving the page runs destroy() and the next page's init replaces #app. Old buttons are detached. A late /api/paths/inspect response renders into the detached div, so its buttons cannot be clicked.
  • map.js uses only PathInspector.validatePrefixes. The embedded pane has its own handler, which calls the local drawPacketRoute (map.js:2574-2575).

Runtime proof [T]. I served an instrumented copy of master's path-inspector.js that counts showOnMap calls and else entries:

  • Across the four flows below there were 3 showOnMap calls and 0 else entries.
  • On #/map, typeof window.drawPacketRoute and typeof window.routeLayer are both 'undefined' on PR and master alike. So even if the branch had been reached, it could not have drawn anything. Its only effect would have been to delete the pending route.

Identical replacement [A] [T]. On the reachable path, the old code did exactly window._pendingPathInspectorRoute = candidate; location.hash = '#/map', and the new code does the same. The equivalent mutant R4 (put the hash guard back without the else) survives every test, which is expected and confirms the equivalence.

Browser check [T] (Playwright Chromium 1280×800, local Go server, CI-prepared fixture, PR frontend vs. master frontend, same server binary):

Flow PR head master
S1: #/tools/path-inspector?prefixes=04,0a → Show on Map (from another page) hash #/map, route edge drawn, route view active, handoff consumed identical
S2: already on #/map, close the route view, embedded pane → Show on Map route drawn, hash stays #/map identical
S3: browser Back to the standalone page → Show on Map again route drawn, handoff consumed identical
S4: from the map to the standalone page via the hash → Show on Map route drawn, handoff consumed identical
S5: leave the map right after a route draw no page errors no page errors (this run)

There were no page errors in either run. While the route view is open, the map covers the Path Inspector pane toggle, so S2 closes the route view first. This is the same on master and is existing design. I inspected screenshots: S1 on the PR shows the drawn two-hop route with the route sidebar, and S2 looks the same on PR and master.

2. The CI fix: real bug or flake?

A real, timing-dependent bug that was already in master's map.js, not a flake. The ci-fix explanation is correct.

  • [K] Run 37599535405 failed with ✗ no page errors: Cannot read properties of undefined (reading '_leaflet_pos'). It happened right after the new standalone step, whose following step (Tools landing…) navigates away from the map immediately. The green run 37641938861 at this head shows test-issue-123-map-lifecycle-e2e.js 16/0 and test-path-inspector-e2e.js 7/0. Neither flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256 (Hash Stats sort) nor flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267 (backfill write-hold) is involved.
  • [A] Leaflet 1.9.4 (leaflet-src.js):
    • _animateZoom always schedules setTimeout(bind(this._onZoomTransitionEnd, this), 250).
    • remove() calls _stop(), which only cancels _flyToFrame and _panAnim, and then delete this._mapPane.
    • _onZoomTransitionEnd is guarded only by _animatingZoom. It then calls _move → _getNewPixelOrigin → _getMapPanePos → getPosition(undefined), which throws _leaflet_pos.
    • The route view zooms with an animated fitBounds (route-view.js:591 and others), which explains the trigger.
    • Clearing _animatingZoom before remove() makes the timer return early.
    • The pending-rAF variant is inert: _animateZoom returns when _mapPane is missing. Only zoomstart/movestart still fire on the removed map, and map.js does not listen to those.
  • [T] The new lifecycle step fails on master's map.js (14 passed, 2 failed, both _leaflet_pos). It fails deterministically under M1 (3/3) and R3 (2/2), and passes at the head.

3. Tests that pin the behaviour, and mutants

Mapping to the issue's acceptance criteria:

  • "No code path calls an undefined window.drawPacketRoute": pinned by the unit guard. [T] Red on master's path-inspector.js (AssertionError: standalone inspector must not depend on private map drawing globals) and green at the head.
  • "Show on map from the standalone page and the embedded pane still draws": this is a preservation criterion. The flow worked before the PR, so there is no red-before state. It is pinned by the existing embedded step ("Show on Map" draws the candidate route), the new standalone E2E step, and the unit handoff test. R2 turns both new tests red.
  • "No behaviour change elsewhere": the reachable Path Inspector behaviour is identical [T, table above]. The map.js statement is the only other runtime change. It is a bug fix scoped to teardown (it removes a page error after destroy()), and the CI fix justifies it.

Mutants [T], each applied alone to a copy of the merged tree and reverted (checked with cmp):

# Mutant Result
R1 (mine) handoff stores { path: candidate.path } instead of the full candidate killed by unit test (preserve the full candidate); E2E survives, so the unit test is what pins the "full candidate" contract
R2 (mine) drop location.hash = '#/map' killed by unit test and by the E2E standalone step (6/1). It survives test-path-inspector-coverage-e2e.js (finding 2)
R3 (mine) destroy(): map.stop() instead of clearing _animatingZoom killed 2/2 by lifecycle step "leaving during an animated zoom" (_leaflet_pos)
R4 (mine, equivalent) re-add the #/map hash guard without the else survives unit and E2E, as expected (equivalent on all reachable paths)
M1 (author's, re-run) remove map._animatingZoom = false killed 3/3 by the lifecycle step

Checklist

  • cmd/server and cmd/ingestor are untouched (the three-dot diff has no cmd/ changes). The read-only invariant holds, and there are no new map[string]interface{}. [T]
  • No hard-coded colours in the diff. [T]
  • scripts/check-xss-sinks.sh --diff 461803d9 at the head: clean (rc 0). git diff --check: clean. [T]
  • Fork guards: Kpa-clawbot/CoreScope appears 9 times in deploy.yml and once in release-fast-path.yml, the same as master. The PR does not touch .github/. [T]
  • No closing keywords in the PR body or commit bodies (closingIssuesReferences = 0). The body says "Relates to cleanup(path-inspector): remove unreachable showOnMap branch that calls undefined window.drawPacketRoute #143". [K]
  • All four commits are authored and committed by dborup <kontakt@meshview.dk>. Two of them carry a Co-Authored-By trailer. [K]
  • The PR is not a draft. I did not change its state.

Tests [T] (merged tree 512aa7ef = head + origin/master 461803d9)

  • node test-path-inspector.js: all checks pass, including the 2 new ones.
  • node test-frontend-helpers.js: 709 passed, 0 failed.
  • sh test-all.sh: 232 passed, 0 failed (232 files).
  • cd cmd/server && go test -timeout 25m ./...: ok (1056 s). cd cmd/ingestor && go test -timeout 25m ./...: ok (1355 s). Both ran in parallel on a loaded host. The PR changes no Go code; I ran them on the merged tree because master moved.
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL, corescope-migrate, seeds 2073, 199 and 245), plain public/:
    • test-path-inspector-e2e.js: 7/0.
    • test-issue-123-map-lifecycle-e2e.js: 16/0.
    • test-path-inspector-coverage-e2e.js: 10/0.
    • test-map-modal-fluid-e2e.js: 10/0.
    • test-map-nodes-pagination-e2e.js: 3/0.
    • test-issue-1374-route-map-a11y-e2e.js: 20/0.
    • test-pr-1490-live-map-gpu-animations-e2e.js: 14/0.
    • test-e2e-playwright.js: 132/135 passed, 3 fixture-dependent skips.
    • My first run of test-e2e-playwright.js failed at "Version info lives on Perf dashboard". That was an environment artefact: the scratch tree has no .git, so /api/health reported commit unknown. Master's frontend failed the same way. With a .git-commit file in scratch, it passed.
  • CI per job at this head (run 37641938861) [K]: Go Build & Test ✅, Playwright E2E ✅, Build & Publish Docker Image ✅; Release Artifacts, Deploy Staging and Publish Badges were skipped, as usual for a PR. No reruns.

Not verified

  • I ran the E2Es against plain public/, not public-instrumented/, and did not reproduce the CPU-throttled race. The deterministic lifecycle step stands in for it.
  • I did not check mobile viewports or touch input for the Show on Map flows.
  • I did not search master's CI history for earlier _leaflet_pos occurrences.
  • I did not test other Leaflet versions; the fix is specific to 1.9.4 internals (finding 4).
  • I did not access staging or production.

@dborup
dborup merged commit 69890b8 into master Oct 7, 2026
6 checks passed
@dborup dborup mentioned this pull request Oct 7, 2026
Closed
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 @dborup-agent