Claude finished @Yeraze's task —— View job

PR Review

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

This is a well-motivated and clearly explained fix for two distinct bugs. Overall the implementation is solid. Here are my observations:

useMarkerSpiderfier.ts

Correctness — pending buffer:
The core fix (buffering markers into pendingRef when spiderfierRef.current is null and flushing on init) is correct and directly addresses the described race. The flush loop at lines 149–158 properly clears pendingRef after flushing, and removeMarker correctly removes from the pending buffer before the spiderfier exists (lines 277–282).

Minor: duplicate code path in addMarker (lines 222–249):
The "same position" and "different position" branches at lines 215–249 both execute identical code (remove old marker, fall through to add new). The isSamePosition check and the branched warning have no behavioral difference — one branch suppresses the console.warn, the other emits it. This is a mild readability issue, not a bug. Consider unifying those branches.

Minor: removeMarker linear scan (lines 277–281, 294–299):
Iterating pendingRef.current and markerByIdRef.current to find the key for a given marker is O(n). For typical Meshtastic deployments (dozens to low hundreds of nodes) this is fine. Worth noting if the node count grows significantly.

addListener warns when spiderfier isn't ready (line 326):
This is an existing pattern, not introduced by this PR, but if consumers call addListener in an effect that runs before init, they'll silently lose their listener. The pendingRef pattern doesn't cover event listeners. This is out of scope for this PR but worth a follow-up comment/issue.

DashboardMap.tsx & NodeMarkersLayer.tsx

stablePosition / stableIcon are plain functions defined inside the component body (lines 179–192, 186–192 in DashboardMap; lines 67–84 in NodeMarkersLayer):
These functions close over positionCacheRef / iconCacheRef from the enclosing scope, so they're recreated on every render but remain functionally equivalent. Since they only operate on refs (not state), this is not a correctness issue and React won't cause problems, but wrapping them in useCallback would make the intent clearer and align with the rest of the hook-style code in the file.

getMarkerRef on every render:
getMarkerRef (DashboardMap line 193, NodeMarkersLayer line 85) is also recreated on every render. Because it uses refHandlers.current.get(key) for memoization, the functional result is stable — but the function itself is a new reference each time. This is consistent with the existing codebase pattern, so no action needed, but worth noting.

Icon signature in DashboardMap (line 445) omits nodeId:

const iconSig = `${hops}|${shortName ?? ''}|${isRouter ? 1 : 0}|${mapPinStyle}`;

The nodeId is not part of the signature, so two nodes with the same hops, shortName, isRouter, and pinStyle but different keys will share an icon (each gets its own cache entry via markerKey, so there's no actual collision). This is fine, just noting that nodeId is intentionally excluded.

markerKey fallback in DashboardMap (line 437–441):

const markerKey = String(
  node.sourceId != null && node.nodeNum != null
    ? `${node.sourceId}:${node.nodeNum}`
    : nodeId ?? node.nodeNum,
);

If nodeId is also null/undefined and node.nodeNum is falsy (0 or undefined), the key degrades to "undefined" or "0". This is unlikely but could cause two nodes to share a key and one to be dropped from the spiderfier. Adding a fallback (e.g., node.user?.id) or an assertion would make this more robust.

Cache entries are cleaned up on marker unmount (lines 204–207 in DashboardMap, lines 96–99 in NodeMarkersLayer) — good.

useMarkerSpiderfier.test.tsx

Test is correctly structured and directly reproduces the bug:
Using useLayoutEffect to simulate <Marker ref> commit-phase timing is the right approach. The FakeOMS via vi.hoisted is well-crafted.

Coverage gaps (minor):

• No test for removeMarker on a pending (pre-init) marker — i.e., a marker added to pendingRef that is then removed before the spiderfier initializes. The code handles this (lines 277–281), but it's untested.
• No test for the position-cache stability fix (Bug 2). The PR notes this is an intentional omission since it's harder to unit-test react-leaflet prop diffing; a comment acknowledging this would aid future maintainers.

Summary

The two bugs are correctly diagnosed and fixed. The buffering approach in useMarkerSpiderfier.ts is the right architectural fix — surface-agnostic and the pre-init flush is properly idempotent. The position/icon caching in DashboardMap and NodeMarkersLayer correctly addresses the reference-churn problem. The test for Bug 1 is well-designed.

No blocking issues found. The minor points above (duplicate branch in addMarker, markerKey fallback robustness, missing test for pending-remove path) are low risk and could be addressed in a follow-up.

Update — root-caused the Map Analysis collapse and fixed it (live-verified)

Live testing against a dev container revealed that the registration + stable-ref changes were necessary but not sufficient: spiderfy registered correctly everywhere, but the fan still collapsed — on Map Analysis instantly on click, on Unified on the next data poll.

Real root cause: react-leaflet forwards its <Marker> ref via useImperativeHandle(ref, () => instance) with no dependency array, so React bounces the parent ref callback null → instance on every re-render (not just mount/unmount). The marker-bridge's null branch treated that as a removal and called removeMarker on a still-present, often spiderfied marker — and OMS auto-unspiderfy()s when a spiderfied marker is removed. So any selection change (Map Analysis) or polled refresh (Unified) collapsed the fan. (This also defeated the stable-ref caches, since the same bounce deleted them.)

Fix (commit 1a17e28): register on an instance only (addMarker is idempotent) and ignore the null bounce; drive genuine removals from a render-reconciliation effect keyed off the rendered key set. Applied to both NodeMarkersLayer (Map Analysis) and DashboardMap (Unified).

Live verification (dev container, real DOM clicks):

• Map Analysis: 249-marker pile 93px → 1445px, holds identical at 80ms / 300ms / 800ms / 2s / 4s (260 spider legs persist). Previously collapsed within 30ms.
• Unified: 43-marker pile 98px → 520px, holds through ~10s spanning multiple poll cycles.

The PR title now accurately reflects the behavior on both surfaces.

Claude finished @Yeraze's task —— View job

PR Review

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

This PR correctly diagnoses and fixes three interrelated bugs in the spiderfy stack. The root cause analysis is accurate, the fixes are well-targeted, and the live-verification results are compelling. Here's the full review:

useMarkerSpiderfier.ts

Pending buffer (lines 112–158) — correct and well-structured.
The pendingRef buffering approach is the right fix for the commit-phase race. The flush loop clears pendingRef after iterating, the try/catch handles a removed-before-flush edge case, and removeMarker correctly drains the buffer (lines 277–282) before a flush could resurrect a removed marker.

addMarker same-position branch is dead code (lines 222–249):
The if (isSamePosition) and else branches execute identically — both remove the old marker and fall through to add the new one. The only difference is the else emits a console.warn while the if path is silent. This isn't a bug but it's noise and slightly misleading (implies different behavior). Consider collapsing into one block that always logs or always suppresses.

Idempotency guard short-circuits correctly (line 209):
The existingMarker === marker early return means a stable-ref marker re-registering after init (normal re-renders) never hits the spiderfier API again. This is the critical property that makes the null-bounce fix safe — a re-registered instance does nothing.

removeMarker and markerByIdRef linear scans (lines 277–299):
Both use reverse-lookup iteration over the map. Fine for deployments in the dozens-to-hundreds-of-nodes range. If this ever needs to scale, a reverse WeakMap<LeafletMarker, string> would make both O(1).

addListener still drops silently when pre-init (line 325–328):
The pending-buffer pattern doesn't extend to event listeners, so a consumer calling addListener in a layout effect or early render will lose the listener. This is pre-existing behavior, not introduced here, but worth a follow-up ticket.

DashboardMap.tsx

Null-bounce fix (lines 196–209) — correct.
Only registering on non-null m and delegating removals to the renderedKeysSig effect is the right approach. The comment explaining the useImperativeHandle / no-dependency-array behavior is excellent context for future maintainers.

Reconciliation effect (lines 318–329):
renderedKeysSig joins keys with |. If any key itself contains |, the split at line 319 would produce wrong results. From the key construction at line 466–470 it's sourceId:nodeNum or nodeId, neither of which should contain |, but if nodeId ever can (user-controlled data from a device), this would be subtly broken. A safer separator (e.g., \0) or a Set comparison would be more robust.

markerKey fallback (lines 466–470):

const markerKey = String(
  node.sourceId != null && node.nodeNum != null
    ? `${node.sourceId}:${node.nodeNum}`
    : nodeId ?? node.nodeNum,
);

If both nodeId and node.nodeNum are null/undefined, this stringifies to "undefined". Two such nodes would share the key "undefined" and the second would clobber the first in markerByKey. This is an unlikely edge case (a node without a nodeNum in the unified data), but worth an explicit fallback.

stablePosition and stableIcon are plain functions (not useCallback) (lines 179–192):
They're recreated on every render but only operate on refs, so there's no correctness issue. Consistent with the rest of this file's style. Worth noting for anyone doing a future performance pass.

Cache cleanup on removal (lines 324–327) — good.
Both positionCacheRef and iconCacheRef are cleaned up in the reconciliation effect. This prevents unbounded memory growth as nodes age out.

<Marker key={nodeId}> vs key={markerKey} (line 488):
The <Marker> uses key={nodeId} but the ref handler is keyed on markerKey (which may differ, e.g., sourceId:nodeNum vs nodeId). If nodeId and markerKey produce a different key assignment — e.g., a node with sourceId:nodeNum = "src1:12345" but nodeId = "abc" — React's VDOM key and the spiderfier's tracking key are out of sync. React will remount the marker when nodeId changes even if markerKey is stable, and vice versa. Using key={markerKey} would be more consistent and less surprising.

NodeMarkersLayer.tsx

Null-bounce fix (lines 88–103) — correctly mirrors DashboardMap.
Same pattern, same reasoning. The comment at 89–98 is clear.

renderedKeysSig uses sourceId:nodeNum (line 151) — consistent with markerKey (line 185).
Unlike DashboardMap, key and markerKey are both markerKey = \${sourceId}:${n.nodeNum}`` (line 185, 203), so the React key and spiderfier key are aligned.

Reconciliation effect includes removeMarker in deps (line 163):
removeMarker is a useCallback(() => ..., []) from the hook — stable ref. This is correct. DashboardMap uses spiderfierRef.current?.removeMarker(m) directly (no hook dep), so there's a slight inconsistency in pattern between the two files, but both are correct.

useMarkerSpiderfier.test.tsx

Test correctly reproduces the timing bug:
Using useLayoutEffect to fire before the hook's useEffect is the right way to simulate the commit-phase ordering. vi.hoisted + vi.mock factory referencing FakeOMS is well-constructed.

Deduplication test (lines 95–105) is useful:
Calling addMarker again with the same id after init and asserting markers.length === 1 directly validates the existingMarker === marker short-circuit.

Missing test: pending-marker removal.
A marker added to pendingRef that is then removed before the spiderfier initializes should NOT appear in the OMS after flush. This code path exists (lines 277–282) but has no test. A test that adds a marker pre-init and calls removeMarker before the effect runs, then asserts oms.markers.length === 0, would guard this.

Missing test: null-bounce resilience.
No test for the null-bounce fix (Bug 3). The PR notes this is harder to unit-test given react-leaflet internals. A comment in the test file acknowledging what's not covered and why would help future maintainers.

Summary

The three bugs are correctly identified and the fixes are sound. The pending-buffer approach in useMarkerSpiderfier.ts is the right architectural level for the init-race fix. The null-bounce detection and reconciliation-effect pattern in DashboardMap and NodeMarkersLayer is the right level for the collapse fix. Test coverage for the timing bug is well-designed.

Items worth a follow-up:

1. <Marker key={nodeId}> vs key={markerKey} inconsistency in DashboardMap (lines 488 vs 466)
2. | as separator in renderedKeysSig — unsafe if nodeId contains |
3. addListener not covered by the pending-buffer pattern
4. Pending-removal test missing
5. Duplicate addMarker branch (cosmetic only)

No blocking issues. The live-verified behavior is the strongest signal here.

Claude finished @Yeraze's task —— View job

PR Review

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

This is a solid, well-motivated fix for three interrelated spiderfy bugs. The root cause analysis is thorough and the comments in the code are some of the best I've seen for explaining react-leaflet internals. Here's my detailed review:

useMarkerSpiderfier.ts

Bug 1 fix (pending buffer) — correct.
The pendingRef approach correctly handles the commit-phase race. The flush at lines 148–158 clears pendingRef after iterating, the try/catch is a sensible guard for the removed-before-flush edge case, and removeMarker (lines 277–282) properly drains the buffer so a marker unmounted before init isn't resurrected on flush.

Idempotency guard (line 209) — critical for Bug 3 fix.
The existingMarker === marker early return is what makes the null-bounce fix in the caller safe: a stable re-registered instance does nothing, so the if (m) guard in getMarkerRef can call addMarker on every non-null bounce without churn.

addMarker same-position vs different-position branches (lines 222–249) — dead code path.
Both if (isSamePosition) and else blocks do identical work (remove old marker from refs, fall through to add new). The only observable difference is whether a console.warn fires. Consider collapsing to a single block that always warns (or never warns) to reduce cognitive noise.

removeMarker reverse-lookup (lines 277–281, 294–299) — O(n) by design.
For deployments in the dozens-to-hundreds range this is fine. A reverse WeakMap<LeafletMarker, string> would make this O(1) if needed later.

addListener not covered by pending buffer (lines 325–328).
A consumer calling addListener before init will still silently drop the listener. This is pre-existing behavior, out of scope here, but worth a follow-up issue.

DashboardMap.tsx

Bug 3 fix (null-bounce, lines 196–209) — correct and well-explained.
The comment describing useImperativeHandle's no-deps-array behavior is excellent context. Only registering on non-null m and delegating removals to the reconciliation effect is the right pattern.

renderedKeysSig separator (line 317) — potential key collision.
join('|') is unsafe if any key itself contains |. From the markerKey construction at lines 466–470, keys are ${sourceId}:${nodeNum} (safe) or nodeId ?? node.nodeNum — and nodeId comes from node.user?.id, which is a user-supplied string. If a Meshtastic node's user ID contains a |, the split('|') at line 319 would produce incorrect results and the wrong markers could be removed. A zero-byte separator (\0) or a Set-based diff would be more robust.

<Marker key={nodeId}> vs spiderfier key markerKey (lines 488, 466).
The React VDOM key is nodeId (line 488) but the spiderfier/ref handler is keyed on markerKey = sourceId:nodeNum (line 466). These diverge for Meshtastic nodes where nodeId !== sourceId:nodeNum. If React remounts a marker due to a nodeId change while markerKey is stable (or vice versa), the ref handler map and spiderfier tracking fall out of sync. Using key={markerKey} would keep the two systems aligned.

markerKey fallback (lines 466–470) — edge case.
If both sourceId and nodeNum are null AND nodeId is also null, the key becomes the string "undefined" or "0". Two such nodes would share a key and only one would track in the spiderfier. An explicit fallback or assertion here would prevent silent data loss.

stablePosition / stableIcon / getMarkerRef defined in component body (not useCallback).
These are recreated on every render but are functionally stable (they only operate on refs). No correctness issue, just noting it's inconsistent with the hook-style code elsewhere. useCallback would make the intent clearer.

Cache cleanup on removal (lines 324–327) — good.
Both positionCacheRef and iconCacheRef are cleaned up in the reconciliation effect. This prevents unbounded growth as nodes age out.

NodeMarkersLayer.tsx

renderedKeysSig separator (line 160) — same | concern as DashboardMap.
Here the key is mc:${nodeId} for MeshCore or ${sourceId}:${nodeNum} for Meshtastic (line 116). MeshCore's nodeId is a public key (base64 or hex), and sourceId is an internal UUID — both are unlikely to contain |, but the edge case exists.

<Marker key={markerKey}> (line 213) — aligned with spiderfier key.
Unlike DashboardMap, both the React key and the spiderfier key use markerKey here.

MeshCore key fallback (line 116).
n.isMeshCore ? \mc:${n.nodeId ?? n.nodeNum}`— ifnodeIdis null andnodeNumis also undefined (possible for a MeshCore node mid-construction), this key becomesmc:undefined`. Extremely unlikely but worth noting.

Reconciliation effect includes removeMarker in deps (line 172) — consistent.
removeMarker is a stable useCallback(() => ..., []), so this is correct and slightly more explicit than DashboardMap's spiderfierRef.current?.removeMarker(m) pattern. Both approaches are correct.

MeshCoreMap.tsx

Consistent bridge pattern — good.
The getMarkerRef / markerByKey / reconciliation-effect pattern is faithfully applied. Using c.publicKey as both the React key and spiderfier key avoids the key vs markerKey mismatch seen in DashboardMap.

renderedKeysSig using publicKey values (line 244).
MeshCore public keys can be base64 with +, /, = but not |, so the separator is safe here.

<MapContainer key={...}> (line 326).
The container remounts whenever center or zoom changes. After this PR, center is derived from positioned.find(...) so it changes when the selected contact's position changes. This will destroy and recreate the entire MapContainer (and the spiderfier inside SpiderfierController) on center/zoom changes — resetting any open fan. This is pre-existing behavior, not introduced by this PR, but the spiderfier would be more resilient if MapContainer used a stable key.

useMarkerSpiderfier.test.tsx

Test correctly reproduces the timing bug.
useLayoutEffect firing before the hook's useEffect accurately mirrors the commit-phase ordering. vi.hoisted + vi.mock with FakeOMS is well-structured.

Deduplication test (lines 95–105) — validates the idempotency guard.

Missing coverage:

• No test for a marker added pre-init and then removed before the spiderfier creates — the pendingRef drain path at lines 277–282 is untested.
• No test for the null-bounce resilience (Bug 3), noted in the PR as harder to unit-test. A brief comment acknowledging the gap would help future maintainers.

Summary

No blocking issues. The three bugs are correctly diagnosed and the fixes are sound. The pending-buffer approach in useMarkerSpiderfier.ts, the null-bounce guard in getMarkerRef, and the reconciliation-effect pattern together form a coherent solution that's now applied consistently to all three map surfaces.

Items worth addressing (ordered by risk):

1. <Marker key={nodeId}> vs key={markerKey} inconsistency in DashboardMap.tsx line 488 — could cause ref/spiderfier key drift on certain node updates.
2. | separator in renderedKeysSig (DashboardMap.tsx:317, NodeMarkersLayer.tsx:160) — safe in practice today but fragile to user-supplied IDs.
3. markerKey fallback to "undefined" in DashboardMap.tsx:466–470 — low risk but could silently drop a node.
4. Pending-removal path (pre-init removeMarker) untested in useMarkerSpiderfier.test.tsx.
5. addMarker same-position/different-position dead-code branch — cosmetic only.

Claude finished @Yeraze's task —— View job

PR Review

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

This is a well-structured fix addressing three interrelated bugs in the spiderfy stack. The root cause analysis is solid and the code is well-commented. Here are my findings:

useMarkerSpiderfier.ts — Pending Buffer (Bug 1 fix)

Correct and well-implemented. The pendingRef approach elegantly handles the commit-phase race condition. A few minor observations:

Dead code in addMarker (lines 222–249): The isSamePosition branch and the else branch do identical work — both call removeMarker on the existing marker and fall through to add the new one. The only difference is whether console.warn fires. Consider collapsing:

// Before
if (isSamePosition) {
  try { spiderfierRef.current.removeMarker(existingMarker); ... } catch (e) { }
} else {
  try { spiderfierRef.current.removeMarker(existingMarker); ... } catch (e) { console.warn(...); }
}
// After: single block, always warn (or never warn)

This is cosmetic only — no behavioral difference.

addListener not covered by the pending buffer (lines 325–328): A caller who invokes addListener in a useLayoutEffect or early render (before the init effect) will silently lose that listener. The pendingRef pattern doesn't extend to event subscriptions. This is pre-existing behavior, not introduced here, but worth a follow-up ticket.

O(n) reverse-lookup in removeMarker (lines 277–299): Iterating both pendingRef and markerByIdRef to find a marker by reference is fine for typical deployments (dozens to low hundreds of nodes). If this ever needs to scale, a WeakMap<LeafletMarker, string> would make both lookups O(1).

DashboardMap.tsx — Null-Bounce Fix (Bug 3)

Null-bounce guard is correct (lines 196–209). The comment explaining why useImperativeHandle with no dep array causes this is excellent — exactly the kind of context future maintainers need.

<Marker key={nodeId}> vs. spiderfier key markerKey (lines 488 vs. 466): The React VDOM key is nodeId but the spiderfier tracks by markerKey = sourceId:nodeNum. These diverge for Meshtastic nodes where nodeId !== sourceId:nodeNum. If React remounts a marker (due to nodeId change) while markerKey is stable — or vice versa — the ref handler map and spiderfier tracking fall out of sync. Using key={markerKey} instead would keep both systems aligned. NodeMarkersLayer.tsx correctly uses key={markerKey} (line 213) — this is an inconsistency between the two files.

renderedKeysSig separator (line 317): Using '|' as a join/split delimiter is unsafe if any key contains '|'. Keys come from nodeId ?? node.user?.id as fallback, which is user-supplied device data. A \0 separator or a Set-based diff would be safer:

// Safer approach
const renderedKeySet = useMemo(() => 
  new Set(nodesWithPosition.map(({ node }) => computeMarkerKey(node))),
[nodesWithPosition]);

markerKey fallback to "undefined" (lines 466–470): If both sourceId/nodeNum are null AND nodeId/node.nodeNum are also null/undefined, the key becomes the string "undefined". Two such nodes would share a key, silently dropping one from the spiderfier. Extremely unlikely in practice but an assertion or explicit fallback would guard against it.

stablePosition, stableIcon, getMarkerRef defined inline (lines 179–213): These are recreated each render but only operate on refs, so there's no correctness issue. useCallback would clarify intent and align with the hook-style code elsewhere, but this is a style preference.

Cache cleanup in reconciliation effect (lines 324–327) — good. Both positionCacheRef and iconCacheRef are cleaned up when a node is removed, preventing unbounded growth.

NodeMarkersLayer.tsx — Consistent bridge pattern

Well-implemented and consistent with DashboardMap. The <Marker key={markerKey}> at line 213 correctly aligns React's VDOM key with the spiderfier's tracking key — the inconsistency mentioned above doesn't exist here.

renderedKeysSig separator concern applies here too (line 160): MeshCore keys use mc:${nodeId} — nodeId comes from device data (a public key/hash), unlikely to contain '|', but the same structural fragility exists.

keyOf fallback for MeshCore (line 116): n.isMeshCore ? \mc:${n.nodeId ?? n.nodeNum}`— if bothnodeIdandnodeNumare undefined, this producesmc:undefined`. Extremely unlikely but worth noting.

MeshCoreMap.tsx — Consistent pattern applied

<MapContainer key={center+zoom}> (line 326): The container remounts whenever center or zoom changes (derived from selectedPublicKey or first positioned contact). This destroys and recreates the entire spiderfier on every selection change — resetting any open fan. This is pre-existing behavior and out of scope for this PR, but it means spiderfy gains on this surface are limited by the container lifecycle. A follow-up to decouple map centering from MapContainer remounting (using useMap().flyTo() etc.) would make spiderfy more resilient here.

Key/spiderfier alignment (lines 345–346): Both key={c.publicKey} and getMarkerRef(c.publicKey) use the same value — correctly aligned, no drift risk.

MeshCore public key safety in renderedKeysSig (line 244): Base64 public keys use +, /, = but not '|', so the separator is safe for this surface.

useMarkerSpiderfier.test.tsx — Test quality

Well-designed test that directly targets the bug. Using useLayoutEffect to reproduce the commit-phase ordering is exactly right, and vi.hoisted + vi.mock with FakeOMS is clean.

Missing: pending-removal test. A marker added pre-init that is then removed before the spiderfier initializes should NOT appear in the OMS after flush. The code path exists (lines 277–282 of the hook) but has no test. Adding this would guard a subtle edge case:

it('does not flush a marker removed before init', () => {
  // add marker in layout effect, remove immediately before passive effect runs
  // assert oms.markers.length === 0
});

Missing: null-bounce resilience test (Bug 3). As noted in the PR, this is harder to unit-test due to react-leaflet internals. A comment in the test file acknowledging this gap would help future maintainers understand what's intentionally uncovered.

vitest.config.ts

ESM alias fix is correct and well-explained. The comment at lines 56–64 accurately describes the UMD vs. ESM constructor issue and why the alias is needed. This is a clean solution that doesn't affect the app build.

Summary

The three bugs are correctly diagnosed and the fixes are sound. Live-verification results with a real dev container are the strongest signal here.

Items worth addressing (ordered by risk):

1. <Marker key={nodeId}> vs. key={markerKey} in DashboardMap.tsx:488 — could cause ref/spiderfier tracking drift on node updates; NodeMarkersLayer already does this correctly.
2. | separator in renderedKeysSig (DashboardMap.tsx:317, NodeMarkersLayer.tsx:160) — safe in practice today but structurally fragile.
3. MapContainer key in MeshCoreMap.tsx:326 — container remount on selection resets the fan; follow-up issue recommended.
4. Pending-removal test missing in useMarkerSpiderfier.test.tsx.
5. addMarker same/different-position dead-code branch — cosmetic only.

No blocking issues found.