Claude encountered an error —— View job

I'll analyze this and get back to you.

Claude finished @Yeraze's task —— View job

PR Review: feat(map): spiderfy overlapping markers on Map Analysis and Unified maps

Tasks

• Gather context and understand the request
• Read changed files
• Analyze code quality, bugs, performance, security, and test coverage
• Post review feedback

Overview

This PR is well-structured and correctly extends the existing spiderfy pattern across all three map surfaces. The code is consistent, the approach is sound, and the test coverage is appropriately scoped. Below are observations from most critical to least.

Issues Found

1. getMarkerRef is recreated on every render — potential stale-closure / ref leak

DashboardMap.tsx:168 — getMarkerRef is defined as a plain function body inside the component's render function. On every re-render, a new getMarkerRef function is created; while the refHandlers Map is stable (it's a useRef), the function that mutates it is not memoized. This is identical to the same pattern already present in NodeMarkersLayer.tsx:59, so it's a pre-existing pattern and consistent — but it means the Marker ref prop receives a new function object on every render, which forces React to call ref(null) then ref(element) on every re-render. In practice this calls removeMarker then addMarker on every prop change.

The same pattern exists in NodeMarkersLayer.tsx, so this is pre-existing behavior, but now it's duplicated. A shared utility or useCallback-based wrapper would prevent the unnecessary remove/add cycle.

2. Marker key uses nodeId but markerKey (spiderfier) uses nodeNum

DashboardMap.tsx:425:

<Marker
  key={nodeId}           // React reconciliation key
  ref={getMarkerRef(markerKey)}  // spiderfier key = sourceId:nodeNum

When nodeId is undefined (nodes without a resolved nodeId), the fallback at line 418-422 produces a non-null markerKey, but key={nodeId} would be key={undefined}. React doesn't error on an undefined key but treats it as key={null}, potentially causing instability in reconciliation. The marker key and React key should be in sync, or at least both non-null.

3. SpiderfierOptions type is defined in the hook file but used externally

useMarkerSpiderfier.ts:45 — SpiderfierOptions is a manually maintained interface that duplicates what ts-overlapping-marker-spiderfier-leaflet should export as OmsOptions or similar. If the library updates its option types, this local interface could silently drift. Consider importing the library's type directly and aliasing it, or adding a type-compatibility check.

This is pre-existing but worth noting since SHARED_SPIDERFIER_OPTIONS is now typed against this local interface.

4. SpiderfierController displays an unused destructured props warning

SpiderfierController.tsx:36:

({}, ref) => {

The empty destructure {} for props is unusual but harmless. A more idiomatic form is (_props, ref) or simply (_, ref) to signal intentional unused parameter. Minor, but could confuse readers.

Code Quality Observations

Duplication of getMarkerRef pattern

The identical ref-handler factory appears in both DashboardMap.tsx:168-185 and NodeMarkersLayer.tsx:59-76. Now that this pattern is used in three places (including the NodesTab which presumably contains the reference implementation), it should be extracted to a shared hook, e.g. useSpiderfierRefHandlers(spiderfierRef). This would eliminate the duplication and make the pattern testable in isolation.

SHARED_SPIDERFIER_OPTIONS is well-documented

The JSDoc on lines 11-21 of useMarkerSpiderfier.ts is clear and explains the rationale for the 50px choice. Good.

SpiderfierController.tsx is now minimal and clean

The simplification from removing the zoom-dependent logic is good — the component is now a thin adapter that delegates entirely to the hook.

Test Coverage Assessment

Good:

• SpiderfierController.test.tsx correctly verifies the shared tuning constants and the imperative ref API using mock injection — this is the right testing strategy given the Leaflet DOM dependency.
• DashboardMap.test.tsx correctly stubs SpiderfierController so it renders without a live map and asserts it's mounted — appropriate smoke test.

Missing / Gaps:

• No test verifies that NodeMarkersLayer now uses SHARED_SPIDERFIER_OPTIONS (the equivalent of the SpiderfierController test). A short test similar to SpiderfierController.test.tsx that mocks useMarkerSpiderfier and checks the options passed in would close this gap.
• The ref-handler bridge behavior (add/remove cycle on remount, deduplication of same-position markers) is only tested at the useMarkerSpiderfier level, not at the DashboardMap integration level. Acceptable given the Leaflet mock complexity, but worth noting.

Performance Considerations

• 50px nearbyDistance means the spiderfier checks a larger radius on every map interaction. This is the intended behavior, but on very large meshes (hundreds of markers) it could slow down the grouping calculation. This matches the existing per-source map behavior, so it's consistent.
• The useEffect in useMarkerSpiderfier.ts:154-158 that updates nearbyDistance without recreating the instance is a good optimization for live tuning, but it's not called by SpiderfierController (which passes SHARED_SPIDERFIER_OPTIONS as a constant object). This is fine — the object reference is stable and the effect won't re-run.

Summary

The PR achieves its goal cleanly. The main issues to consider:

1. Moderate: The getMarkerRef duplication between DashboardMap.tsx and NodeMarkersLayer.tsx — should be extracted to a shared utility before this pattern spreads further.
2. Minor: key={nodeId} can be undefined for some nodes; should align with markerKey.
3. Minor: Missing test for NodeMarkersLayer receiving SHARED_SPIDERFIER_OPTIONS.
4. Nitpick: SpiderfierController's ({}, ref) destructure style.

The approach is correct and the consistency across all three map surfaces is well-executed. No security concerns. No regressions to existing behavior are apparent.