feat(map): Important Links overlay — B-weighted top routes

[ArcanConsulting]

Public map overlay drawing the most important affinity links, weighted by the #672 usefulness axes (rank-by selector + top-N slider). Frontend-only. Pairs with #1762.

[Kpa-clawbot]

Polish review — Important Links overlay

Verdict: APPROVE-WITH-CONCERNS (0 BLOCKER · 0 MAJOR · 4 MINOR · 2 NIT)

Solid net-new UI surface. Pure ranking core is well factored out and behaviorally tested, CSS-var theming is respected, persistence works, and the visual encoding (weight + opacity ∝ importance) reads cleanly. Net-new-UI TDD exemption applies cleanly (test lands in same PR, asserts behavior, not just structure). Notes below are polish, not blockers.

MINOR

1. Stale edge cache after node reload — public/map.js:1348 re-renders the overlay when nodes reloads, but topRoutesEdges is only refreshed by toggling the checkbox or via the api() TTL on loadTopRoutes. After a long session with node coords/scores moving, the displayed graph is the old neighbor-graph snapshot joined to new node scores. Consider re-fetching topRoutesEdges on a longer interval or invalidating it when nodes is fully reloaded.

2. No debounce on the top-N slider — public/map.js:548 fires renderTopRoutes() on every input event. At N=200 with high edge counts this rebuilds 200 Leaflet polylines per tick while the user drags. A 50–100ms debounce (or switching to change for the heavy render and keeping input only for the label) would smooth the drag.

3. computeTopRouteEdges reference extraction is brittle — test-top-routes-overlay.js:39-43 slices the source by indexOf('const TOP_ROUTES_AXES') … indexOf('function clearTopRoutes'). Any future rename of either anchor silently shrinks the slice and the behavioral block degrades into a structural-only test with no signal. Either pin both anchors with an explicit assert (if (start < 0 || end < 0) process.exit(1) is there — good — but add an assert that block.includes('function computeTopRouteEdges') to fail loudly if the body changes shape), or export the function on a test-only global behind a if (typeof module !== 'undefined') shim.

4. Empty-state UX — public/map.js:2217 builds topRoutesLayer = L.layerGroup() and adds it to the map even when top.length === 0 (e.g. neighbor-graph returned no edges, or every endpoint lacks GPS). The checkbox stays checked, the options panel stays open, but nothing renders and no hint tells the user why. A small <div id="mcTopRoutesHint"> similar to #mcNeighborHint ("No georeferenced edges to display") would close the loop.

NIT

5. getComputedStyle per render — public/map.js:2209 reads --accent from the root element on every renderTopRoutes() call. Cache it once or read on theme-change events; trivial impact but the affinity-debug code path nearby has the same pattern, so it might be worth a follow-up sweep.

6. topN | 0 — public/map.js:2173 uses bitwise OR to coerce. Works for the slider range (10–200) but caps silently at 2³¹. Math.max(0, Math.floor(Number(topN) || 0)) would be more honest about intent.

Positive notes

• computeTopRouteEdges is a clean pure function — the test harness extracting and executing it in isolation is the right shape for this codebase.
• Importance formula edge.score × mean(endpoint scores) (with affinity axis bypassing the weighting) is correct and matches the test fixtures (0.5 × (0.9+0.8)/2 = 0.425 ✓).
• Zero-coord (0,0) treated as missing — good defensive choice.
• escapeHtml applied to both the node-name fallbacks and the axis label in the popup — XSS-safe.
• Theme-aware via --accent CSS variable rather than a hardcoded hex.
• localStorage persistence for both axis and top-N — nice UX touch.
• Test covers axis-swap reordering, top-N capping, GPS-less skip, and zero-score skip on score axes vs. affinity axis. Real behavioral coverage.

TDD gate (kent-beck)

Branch has a single commit (ada541d3), so no red→green history is visible. Net-new UI surface exemption applies (the "Important Links" surface didn't exist on master, so there are no prior assertions to break). Test lands in the same PR and exercises computeTopRouteEdges with behavioral assertions — not a snapshot or import-only check. Exemption applies cleanly. ✅

---

Scope: 237-line frontend-only diff (public/map.js, test-top-routes-overlay.js, test-all.sh). No backend / no API contract changes.

[Kpa-clawbot]

Re-review (round 2) — Important Links overlay

Verdict: merge-ready (0 BLOCKER · 0 MAJOR · 0 MINOR remaining)

All 6 polish items from the prior review (4 MINOR + 2 NIT) landed in 158cc343. Verified by grep on the rebuilt diff + behavioral test rerun (20/20 passed) + CDP visual check on local chromium against staging.

Findings closed

#	Prior finding	Fix landed
M1	Stale edge cache after node reload	public/map.js:1359 — re-fetches topRoutesEdges via loadTopRoutes() when nodes reload, not just re-renders the cache.
M2	No debounce on top-N slider	public/map.js:557-558 — clearTimeout + setTimeout(renderTopRoutes, 80); label still updates on input.
M3	Brittle test extraction	test-top-routes-overlay.js:48 — explicit assert(block.includes('function computeTopRouteEdges')) fails loudly if the anchors drift.
M4	Empty-state UX	public/map.js:243 adds #mcTopRoutesHint; :2221 toggles it when top.length === 0.
N5	getComputedStyle per render	public/map.js:2206-2210 caches --accent in topRoutesAccentCache; :2254 invalidates on theme-refresh.
N6	topN | 0 bitwise coercion	public/map.js:2185 uses Math.max(0, Math.floor(Number(topN) || 0)).

Verification

• Tests: node test-top-routes-overlay.js → 20 passed, 0 failed (7 structural + 13 behavioral).
• CDP visual: loaded staging with the PR's map.js swapped in, toggled the overlay → options panel reveals 5 axes (usefulness/bridge/redundancy/traffic/affinity), Top-50 slider, and the map renders weighted polylines with visible weight + opacity gradient (importance encoding reads cleanly). Staging restored to master afterwards.
• TDD gate: net-new-UI surface exemption still applies cleanly — behavioral test lands in the same PR and asserts on computeTopRouteEdges outputs (not structure).

Three-axis

• mergeable: MERGEABLE (mergeStateStatus: BLOCKED — likely a required-review rule, not code state)
• CI: no checks reported on this branch (frontend-only, no workflow triggered)
• review blockers: none

Nothing further from this round. Ready when you are.

[Kpa-clawbot]

Status check — round 3 (merge-from-master)

Head moved 158cc343 → f9dedfd1 (28 commits inherited from master + the merge commit f9dedfd1). No new feature commits on this branch — git diff 158cc343..f9dedfd1 -- public/ cmd/ against the round-2 head shows only conflicts already resolved cleanly by GitHub's auto-merge. Round-2 verdict (merge-ready, 0 BLOCKER · 0 MAJOR · 0 MINOR) carries forward.

Auto-merge blocked: Go CI is RED on TestDistanceConcurrentRequestsDuringBuildReturn202 (cmd/server/distance_lazy_index_test.go:81) — same flake currently red on master HEAD (confirmed in #1795's review against the merge-commit run for #1769). Unrelated to this diff.

Recommendation: wait for the master-side flake fix to land + rerun, or rerun CI now since it's a known race (the test name itself says "concurrent requests during build"). I'm not flipping the auto-merge gate on this run — mergeStateStatus: BLOCKED. Operator: feel free to gh pr merge 1771 --squash --auto once CI is green; review side has nothing left.

— mc cron v2

[Kpa-clawbot]

Status check — merge-from-master only (round 4)

Head moved f9dedfd1 → a97952d7 via merge of ec0ebeda + ae2e3933 from master. git log --no-merges f9dedfd1..a97952d7 shows zero new commits on this branch — both inherited commits are master-side (WS allowlist #1793, store-memory diagnostics #1773). Round-2 verdict (merge-ready, 0 BLOCKER · 0 MAJOR · 0 MINOR) carries forward on the feature itself.

Auto-merge blocked: Playwright now red on a different test than last cycle — test-issue-1329-map-controls-accordion-e2e.js:167:

✗ desktop (1280px): panel position:absolute, all section contents visible: desktop must show all controls (got 23/26)

This is the map-controls accordion E2E (#1329), not anything this PR touches. 3 of 26 controls aren't visible at 1280px — likely a CSS regression from one of the master-side commits inherited via the merge, NOT from this branch.

Master is reporting "success" on its own CI runs, so this may be a layout race that's deterministic only when the Important-Links overlay code from this branch interacts with the accordion panel sizing. Worth a CDP repro before declaring upstream-cause.

Action: community branch, leaving as-is. Operator: if you want a debug-repro spawn against staging to confirm overlay-vs-accordion interaction, say the word.

— mc cron v2

[Kpa-clawbot]

Delta review — cd71c7f

New commit narrowly scopes the #1329 desktop-count selector to exclude controls inside an inline display:none progressive-disclosure container (geo-filter, affinity-debug, this PR's Important-Links rank-by). The accordion regression this test guards against still gets caught — that path uses the class-based .mc-collapsed > *:not(legend){display:none} rule, not inline styles, so the exclusion filter doesn't touch it. Correct, surgical fix. 0 BLOCKER · 0 MAJOR.

Still blocking merge: mergeable=CONFLICTING against master. Rebase/merge-master needed before this can ship. CI also won't re-run until the conflict is resolved.

Action: waiting on rebase. Once green + clean, prior round-2 merge-ready verdict carries forward.

— mc cron v2

[Kpa-clawbot]

Delta Review — commits since cd71c7f

Verdict: merge-ready | 0 BLOCKER | 0 MAJOR | 1 MINOR

Adversarial

New code in map.js is well-isolated: dedicated layer group, debounced slider, theme-cache invalidation. No XSS vectors (node names flow through Leaflet bindTooltip which escapes by default). The fetch to /analytics/neighbor-graph is the same public endpoint used elsewhere — no auth or data-exposure concern. The accordion test fix (#1329) correctly filters controls inside display:none containers rather than masking the desktop-accordion regression the test guards against.

Tufte (frontend/UI)

Overlay controls follow established map.js fieldset pattern (mc-section + mc-label). Slider + select + checkbox is a tight, discoverable interaction. The ph-graph icon choice is semantically appropriate. No new CSS files; polyline styling uses --accent with opacity fallback. No visual regression risk to existing surfaces.

Kent Beck (TDD gate)

test-top-routes-overlay.js covers structural wiring (toggle, rank-by options, slider, fetch target, load/clear handlers) AND behavioral ranking via computeTopRouteEdges extraction. The #1329 test fix is a test-only change that eliminates a flake without weakening the assertion. TDD gate: satisfied.

MINOR

1. topRoutesRenderTimer debounce uses a hardcoded 120ms — consider extracting to a named constant for discoverability (non-blocking, style-only).

[Kpa-clawbot]

Automated review (parallel personas, consolidated)

Verdict: needs-visual-verify

carmack: Ranking math (map.js:2178) is sound — edge.score × mean(endpoint axis scores), with affinity axis falling through to raw edge weight; null/zero-island coords and null e.score are dropped via !(importance > 0); slice(0, Math.max(0, Math.floor(Number(topN)||0))) is defensive. One design note: a well-scored node linking to an unscored (0) node yields half its "true" importance rather than being excluded — intentional per the docstring, but worth confirming vs. #672's spec (should low-score endpoints attenuate or veto a link?).

tufte: Overlay uses a single --accent hue with weight 1–7 px and opacity 0.25–0.8 linearly ∝ importance (map.js:2231); axis identity lives only in the popup — no legend or per-axis hue, so a screenshot of the map can't tell "usefulness top-50" from "bridge top-50" without clicking. Unable to CDP-verify — feature is not deployed to analyzer-stg.00id.net (grep -c mcTopRoutes on served map.js = 0) and PR body has no screenshot; flagging as MAJOR need visual proof before merge.

kent-beck: test-top-routes-overlay.js extracts and executes the pure computeTopRouteEdges core against fixtures (importance math, GPS-less skip, zero-score skip on scored axes, axis-reorder, affinity fallback, top-N cap) plus structural pins for the DOM/Leaflet wiring — would fail red on a revert of the ranking core. Good coverage; TDD bar met.

MAJOR:

• No visual proof of the overlay (not on staging, no PR screenshot) — attach a screenshot per axis (usefulness / bridge / affinity) before merge.

MINOR:

• Single-color rendering + popup-only axis label: consider a small legend or per-axis stroke hue so the active ranking is legible without interaction.
• Confirm with feat: repeater usefulness score — is this repeater actually adding value to the mesh? #672 spec whether an unscored endpoint should attenuate (current) or veto a link.
• Empty-state hint text mentions "the feat: repeater usefulness score — is this repeater actually adding value to the mesh? #672 scores need a server that ships them" — surface a link to the tracking issue for operators.

[Kpa-clawbot]

Polish review — round 1 (parallel personas)

Verdict: MAJOR (one race bug; rest is nits).

[carmack] Race: fetch-vs-toggle can render an unchecked overlay. loadTopRoutes() (map.js:2208) calls renderTopRoutes() unconditionally on resolve. Toggle ON → fetch in flight → toggle OFF → clearTopRoutes() runs → fetch resolves → renderTopRoutes() draws the layer, checkbox is off. Guard the render call with document.getElementById('mcTopRoutes')?.checked, same pattern you already use at map.js:1360.

[carmack] Per-frame allocs on slider drag are avoidable. computeTopRouteEdges rebuilds pos/score dicts and renderTopRoutes rebuilds nameByPk every 80ms while dragging. Hoist to module scope, invalidate on node reload (same trigger as the loadTopRoutes() call at map.js:1358). Not a blocker at current node counts, but the hot-path comment invites it.

[tufte] Doc-vs-code drift on the importance formula. Header comment (map.js:~2155) says "edge.affinity × mean(endpoint axis scores)"; code reads e.score. Neighbor-graph API returns score, not affinity — align the comment or the field access so a future reader isn't hunting a nonexistent field.

[tufte] Hardcoded color fallback '#4a9eff' at map.js:2226 bypasses the CSS-var discipline the rest of the file follows. Fallback fires only if --accent is empty (never in practice), but drop it to '' and let Leaflet pick its default, or read --accent-fallback if you want a documented default.

[kent-beck] Coverage gaps in test-top-routes-overlay.js. No case for: empty edges/nodes (defensive against first-load), unknown axis string, negative topN. TDD exemption (net-new UI, per AGENTS.md) is valid — add those three assertions and it's airtight.

[kent-beck] Nit. Grep pins with .test(src) are fine; consider one pin for ?.checked in loadTopRoutes once the race fix lands so it can't regress.

3-axis: mergeable=MERGEABLE · CI=SUCCESS (Go+E2E; Docker cancelled on fork, expected) · reviews=none. Address the race + doc drift and this is merge-ready.

[efiten]

Closing and immediately reopening this to get a CI result. Nothing is wrong with your PR and no action is needed from you.

This one never actually failed: its last pipeline was CANCELLED, so there has been no verdict on it at all. Meanwhile master has moved (#1924 fixed #1923, an E2E bug that was failing PRs which never touched the affected page), and a plain re-run would reuse the merge commit from the original run rather than testing against current master. A close/reopen is the way to get a fresh one.

Apologies for the notification noise.

[efiten]

Correction to what I just wrote: I said the last pipeline was CANCELLED. The run is recorded as failure, not cancelled. The substance holds and is actually better than I put it: no test failed. On run 28999632816, ✅ Go Build & Test and 🎭 Playwright E2E Tests both came back success, and only 🏗️ Build & Publish Docker Image was cancelled, which is what sank the run's overall conclusion. The CANCELLED I quoted came from the PR's status-check rollup, which reflects the cancelled downstream job rather than the test jobs.

So this PR has never had a real test failure. The fresh run will confirm whether that still holds against current master.

[efiten]

Review from the queue triage. Written 2026-08-30 against the tree at that time; posting now that the maintenance window in #1922 has opened.

Verdict: approve the feature, request a change to how it is tested. CI was CANCELLED, not
failed, so re-run it first.

The overlay is sensibly built: a pure ranking core (computeTopRouteEdges) separated from the
Leaflet drawing, importance as edge affinity weighted by the mean of the endpoints' #672 axis
score, a debounced slider, --accent cached and invalidated on theme-refresh, escapeHtml in
popups, and an empty-state hint that names the two reasons nothing rendered.

The test approach is the thing to push back on. test-top-routes-overlay.js extracts the
ranking core by indexOf-slicing public/map.js between the literals const TOP_ROUTES_AXES and
function clearTopRoutes, then new Functions the result. The author added a guard assertion for
the rename case, which is thoughtful, but the approach still breaks on any reordering of map.js
and it tests a string, not the module. Two other PRs in this same queue do it properly and are
worth pointing at: #1821 exports applyObserverFilter through _packetsTestAPI, and #1912 puts
hashPrefixInfo on window. Ask for the same here.

Smaller notes: modifying the existing accordion regression test
(test-issue-1329-map-controls-accordion-e2e.js) to skip the new hidden controls is justified and
the comment correctly explains why the accordion check still bites (that collapse is class-based,
not inline style), but it deserves a second reviewer's eye on principle. And
/analytics/neighbor-graph?min_count=1&min_score=0 pulls the entire unfiltered graph to filter
client-side; that matches this project's bulk-fetch convention, but the payload grows with the
mesh.

---

Update 2026-09-02. Correction to the line above: this PR's run was recorded as failure, not CANCELLED. It still was not a test failure. On run 28999632816 both Go Build & Test and Playwright E2E Tests passed and only Build & Publish Docker Image was cancelled, which sank the run's overall conclusion. After a fresh run against master it is fully green. Only the test-extraction question above is still open.

[efiten]

Rebased and continued as #1928. Your commits, your authorship, unchanged.

This went CONFLICTING through no fault of yours: #1760 landed first in the #1922 queue run and both PRs append a line to test-all.sh at the same spot. That was the whole conflict, and I resolved it by keeping both test lines rather than either.

Nothing else was touched. public/map.js and test-issue-1329-map-controls-accordion-e2e.js are byte-for-byte as you wrote them. Verified on the rebased tree: your test-top-routes-overlay.js 20 passed, #1760's test-repeater-metric-scatter.js 31 passed, test-frontend-helpers.js 627 passed, 0 failed.

I opened it as a new PR rather than force-pushing to your branch. If you would rather #1771 stayed the vehicle, say so and I will push the rebase here and close #1928 instead.

Also worth knowing: this PR's run was recorded as CANCELLED for a long time, which is part of why it sat. Its tests were never the problem.

[efiten]

Merged via #1928, which is your two commits rebased onto master with your authorship intact. Closing this as the vehicle rather than the work.

The only thing I changed was the test-all.sh conflict, and I kept both test lines rather than choosing between yours and #1760's. public/map.js and the accordion E2E are byte-for-byte as you wrote them.

The one review point still open is the test-extraction approach — test-top-routes-overlay.js slices map.js by indexOf and new Functions the result, so it breaks on any reordering of that file. #1821 and #1912 both export the function under test instead. Happy to take that as a follow-up if you would rather not.