fix(traces): fix path graph legibility and overlapping edges#1134
fix(traces): fix path graph legibility and overlapping edges#1134efiten wants to merge 2 commits intoKpa-clawbot:masterfrom
Conversation
…pping edges Partial observations (same packet seen at 1, 2, 4, 5 hops as it propagated) were treated as separate routes, producing long shortcut edges to Dest that visually obscured the actual relay chain. Filter out any path that is a strict prefix of a longer observed path before building the graph. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Intermediate hop nodes used white text on --surface-2 background, making labels invisible in the light theme. Move labels below circles and use var(--text) for theme-aware contrast. Increase SVG height and node radius to give labels room, and switch intermediate fill to a subtle accent tint with accent border so nodes are recognisable without obscuring the label. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Kpa-clawbot
left a comment
There was a problem hiding this comment.
Independent review (PR #1134, public/traces.js)
Fix direction is correct: dropping strict-prefix observations from the path graph and moving labels outside the small node circles both address real legibility bugs. Confirmed against MeshCore TRACE semantics — each observer appends to the path as the packet hops, so a strict prefix across observations is the same packet caught earlier in the chain, not a distinct route. Filter logic correctly excludes self (other.hops.length <= hops.length) and preserves identical-length paths from different observers. CSS theming changes use var(--text) and var(--accent) which are defined in both light and dark themes (public/style.css lines 119, 216, 246, 106) — no theming illusion.
Must-fix:
-
Edge attribution undercounts observers after the prefix filter. When observer A saw
[a,b]and observer B saw[a,b,c,d], A is dropped entirely. But A did witness theOrigin→a→bsegment — the edges in that common prefix lose A from theiredgeMapobserver set, sothickness = Math.min(obsArr.length, 6)and the edge tooltip ("N observers: …") understate corroboration on the segment that was actually most-observed. Either (a) before filtering, attribute the dropped observer to the longest path that supersedes it (mergeobserverinto the kept entry's attribution for the prefix range), or (b) keep all entries inallPathsforedgeMapaccumulation and only suppress prefixes when computing the legend'suniquePathscount and the layout's column ordering. Option (a) is closer to the PR's intent. -
No automated test added. Test plan checkboxes in the PR body are unchecked and no harness assertions exist for the new filter. Per
AGENTS.md(TDD mandatory; rule 18 — "tests pass" is not "feature works"; Frontend UI fixes need browser-level assertions): add at minimum a unit test for the prefix-filter helper exercising (i) two strict-prefix observations → only longer kept, (ii) two identical-length identical-path observations → both kept, (iii) two divergent paths → both kept, (iv) emptyhops→ already filtered before the new code runs, confirm no regression, (v) prefix relationship across multiple observers (A⊂B⊂C → only C kept). Pull the filter out ofdoTraceinto a named function (e.g.dedupePrefixPaths(rawPaths)) so it's directly testable fromtest-frontend-helpers.js. For the visual half (label positioning, theme contrast), add a DOM-grep style assertion in an existing trace-related E2E (or new one) that the rendered SVG contains<text … fill="var(--textfor non-endpoint nodes — that catches future regressions to the white-on-light-surface bug. -
Path signature collision risk.
hops.join(',')is used as the comparison key. Hop values are currently hex repeater hashes so commas don't appear, but the assumption is undocumented and brittle if upstream ever changes the JSON-encoded format (e.g. observer names, longer identifiers). UseJSON.stringify(hops)or compare arrays element-wise; cost is identical, and the intent is clearer. -
Same-path / different-observer case in the dedup filter is correct but not exercised by the test plan checkboxes — please add the explicit "multiple observers reporting the identical full path" case to the manual test plan and the unit tests above.
Out-of-scope (not for this PR — file separately if not already tracked):
- The
renderPathGraphfunction as a whole is doing layout, color cycling, edge accumulation and SVG string-building inline. Splitting layout from rendering would make all of this more testable, but it's pre-existing and not introduced here. - The
O(n²)shape of the filter is fine for realistic observer counts (handful per packet); flagging only because reviewers will ask — no change requested.
Verdict: direction-correct, but ship-blocked on (1) the edge-attribution regression and (2) missing tests, both of which are required by AGENTS.md.
Summary
--surface-2background, making labels invisible in the light theme. Labels now appear below circles and usevar(--text)for theme-aware contrast. Increased SVG height and node radius to give labels room; intermediate fill uses a subtle accent tint with accent border.Test plan
🤖 Generated with Claude Code