Skip to content

fix(traces): fix path graph legibility and overlapping edges#1134

Open
efiten wants to merge 2 commits intoKpa-clawbot:masterfrom
efiten:fix/trace-path-graph
Open

fix(traces): fix path graph legibility and overlapping edges#1134
efiten wants to merge 2 commits intoKpa-clawbot:masterfrom
efiten:fix/trace-path-graph

Conversation

@efiten
Copy link
Copy Markdown
Contributor

@efiten efiten commented May 6, 2026

Summary

  • Drop prefix-only paths from path graph: 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. Now filters out any path that is a strict prefix of a longer observed path before building the graph.
  • Fix invisible node labels: intermediate hop nodes used white text on --surface-2 background, making labels invisible in the light theme. Labels now appear below circles and use var(--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

  • Open a TRACE packet's path graph with a node that has multiple partial observations — verify no spurious shortcut edges
  • Check path graph in light theme — verify intermediate hop labels are visible
  • Check path graph in dark theme — verify no regression

🤖 Generated with Claude Code

efiten and others added 2 commits May 6, 2026 09:52
…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>
Copy link
Copy Markdown
Owner

@Kpa-clawbot Kpa-clawbot left a comment

Choose a reason for hiding this comment

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

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:

  1. 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 the Origin→a→b segment — the edges in that common prefix lose A from their edgeMap observer set, so thickness = 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 (merge observer into the kept entry's attribution for the prefix range), or (b) keep all entries in allPaths for edgeMap accumulation and only suppress prefixes when computing the legend's uniquePaths count and the layout's column ordering. Option (a) is closer to the PR's intent.

  2. 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) empty hops → 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 of doTrace into a named function (e.g. dedupePrefixPaths(rawPaths)) so it's directly testable from test-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(--text for non-endpoint nodes — that catches future regressions to the white-on-light-surface bug.

  3. 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). Use JSON.stringify(hops) or compare arrays element-wise; cost is identical, and the intent is clearer.

  4. 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 renderPathGraph function 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants