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.