Claude finished @Yeraze's task —— View job

PR Review: fix(map): anchor neighbor-info lines to the merged node marker position (#3642)

• Read changed files
• Review code quality and best practices
• Check for potential bugs
• Check performance considerations
• Check test coverage
• Post final review

Summary

This is a clean, well-scoped fix. The root cause was clearly identified and the solution is minimal and correct: neighbor-info line endpoints now resolve to the rendered marker position via the existing nodePositions map rather than the source-specific embedded coordinates in the neighbor record.

Code Quality

nodeHelpers.ts — resolveMapEndpoint (src/utils/nodeHelpers.ts:122-132)

The helper is small, pure, and well-documented. The JSDoc block thoroughly explains the problem context, which is appreciated for future maintainers.

One minor nit: the function signature uses number | null | undefined for the embedded coord params. Since null and undefined are both handled by != null, this is correct. No issues.

NodesTab.tsx (src/components/NodesTab.tsx:2449-2456)

The replacement is clean. The two resolveMapEndpoint calls mirror each other symmetrically, and the early return on !nodeEndpoint || !neighborEndpoint (line 2452) correctly replaces what was previously four separate null checks. The downstream uses of nodeLat, nodeLng, neighborLat, neighborLng (lines 2455-2456) are unchanged from before, which is good — only the source of truth changed, not the consumption pattern.

One observation: nodePositions is keyed by nodeNum (numeric) and ni.nodeNum/ni.neighborNodeNum must also be numeric for Map.get to work. Worth confirming these types match — though since this code was already working for the marker rendering path, it's almost certainly fine.

Potential Bugs

No bugs found. The fallback path (embeddedLat != null && embeddedLng != null) correctly handles the case where a node isn't currently rendered on the map (e.g., it's outside the max-age filter). This was the previous behavior and is preserved.

One edge case that the fix doesn't address (and likely doesn't need to): nodes where hideFromMap is true are excluded from nodePositions (line 1344), so their lines would fall back to embedded coords. This is correct behavior — a hidden node has no marker to anchor to, so falling back to embedded coords is the best option.

Performance

No regressions. resolveMapEndpoint is O(1) (Map.get). It's called twice per neighbor-info record, same as the previous coordinate lookups. nodePositions was already memoized (useMemo, line 1349).

Security

No security concerns. This is purely a UI coordinate resolution fix.

Test Coverage

The three unit tests in nodeHelpers.test.ts (lines 6-25) cover the three meaningful cases:

1. Marker position preferred over embedded coords ✓
2. Fallback to embedded coords when node not on map ✓
3. Returns null when neither is available ✓

The mixed-null test on line 23 (resolveMapEndpoint(markers, 1, 40.5, undefined)) is a good edge case — it verifies that having one coord but not the other returns null.

One optional improvement would be a test where embeddedLat is valid but embeddedLng is null (the reverse of line 23), but this is already implicit from the implementation and not strictly necessary.

Minor Suggestions (non-blocking)

1. nodeHelpers.test.ts:23 — The test description says "returns null when neither a marker nor embedded coords are available" but the second assertion (resolveMapEndpoint(markers, 1, 40.5, undefined)) actually tests "one coord present but not the other." Consider splitting this into a separate it block with a more accurate description for clarity.

2. The existing useMemo dependency array at line 1358 uses a .join(',') string of node positions — this already picks up position changes, so the new resolveMapEndpoint calls will automatically see updated positions when nodes move. No action needed, just confirming the dependency tracking is correct.

Verdict

Approved. The fix is correct, minimal, well-tested, and follows the existing patterns in the codebase. The new helper function is properly extracted into nodeHelpers.ts where it belongs, and the JSDoc explains the multi-source context clearly.