Claude finished @Yeraze's task —— View job

PR Review: feat(ui): #3557 UI tweaks + map unification pass

• Gather context and understand the request
• Read changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Check security concerns
• Evaluate test coverage

Overall this is a well-structured, careful PR. The map unification is consistent across all three map surfaces, the sidebar pin-state fix is correct, and the test coverage for the new popup click behavior is thorough. Below are a few observations ranging from real bugs to minor polish notes.

Potential Bugs

1. GeoJSON fetch in DashboardMap uses an unauthenticated fetch instead of csrfFetch

DashboardMap.tsx:208-210:

const response = await fetch(`${baseUrl}/api/geojson/layers`);

This is a raw fetch, which means the browser sends no session cookie in some cross-origin or SameSite=Strict setups. MeshCoreMap.tsx has the same pattern (line 126). The toggleGeoJsonLayer helper in both files correctly uses csrfFetch for writes, but the initial read uses bare fetch. The existing NodesTab fetches GeoJSON layers via api.get() (which handles auth), so these two new fetch sites are inconsistent. Low-risk in same-origin deployments but worth aligning.

2. handleClose makes sequential await API calls that block each other

NewsPopup.tsx:117-124:

for (const item of newsItems) {
  await api.dismissNewsItem(item.id);
}

These are serialized; if there are 10 unread news items, closing the popup blocks for 10 round-trips before calling onClose(). The user sees the popup stay open while this runs. A simple Promise.all would fan these out:

await Promise.all(newsItems.map(item => api.dismissNewsItem(item.id)));

Then api.updateUserNewsStatus() can proceed immediately after.

3. handleClose re-fetches getUserNewsStatus unnecessarily

NewsPopup.tsx:135-136:

const status = await api.getUserNewsStatus();
await api.updateUserNewsStatus(mostRecentId, status.dismissedNewsIds || []);

The dismissedNewsIds were already fetched during fetchData at popup-open time and stored in local state. The extra GET here adds a round-trip and introduces a TOCTOU window. It's safer to pass the already-loaded dismissedNewsIds through (or keep them in state alongside fullFeed).

Code Quality / Best Practices

4. Duplicated GeoJSON fetch + toggleGeoJsonLayer logic across three components

DashboardMap.tsx:203-231, MeshCoreMap.tsx:121-146 are identical blocks. The pattern (fetch on mount, cancel-on-unmount, toggle-via-PUT) has now been copy-pasted three times (also in the NodesTab map). A small custom hook — e.g. useGeoJsonLayers() — would eliminate the repetition and make future API changes a single-file fix.

5. Duplicated tile-selector localStorage state across three components

DashboardMap.tsx:162-173, MeshCoreMap.tsx:104-115 each declare two useState + two useEffect blocks for the same two localStorage keys. Same extraction opportunity as above.

6. MeshCore popup source-row click navigates to source root, not DM

DashboardPage.tsx:71-73:

if (source.protocol === 'MeshCore') {
  navigate(`/source/${source.sourceId}/`);
  return;
}

The PR description acknowledges MeshCore sources "just switch to that source," which is reasonable for now, but from a UX standpoint the user's intent was to see that node's details. This could be a follow-up item.

7. node-popup-source-row-button CSS class is added in nodes.css but the base node-popup-source-row class likely needs a cursor: pointer even for the enabled state

The disabled button already gets cursor: not-allowed via browser default, but enabled buttons in this custom style may not have an explicit pointer cursor if the base .node-popup-source-row class doesn't set it. Worth a quick visual check.

8. Minor: node-popup-source-row wraps the entire row in a <button> including the protocol badge span

The title attribute is on the outer <button>, which is correct, but screen readers will announce the entire concatenated text ("MeshtasticTower Alpha") without a space or separator. A small aria-label would improve accessibility:
```tsx ${s.sourceName} in ${s.protocol}`}

---

### Performance

**9. `MapBoundsUpdater` dependency array includes `sourceId` but the comment says "only auto-fit once"**

`DashboardMap.tsx:122-138`:
```ts
useEffect(() => {
  if (hasFittedRef.current) return;
  ...
  hasFittedRef.current = true;
}, [map, positions, sourceId]);

Because sourceId is in the dep array, the effect re-runs on source switch — but hasFittedRef.current is already true, so it's a no-op. This is correct but slightly confusing; the sourceId dep could be removed since the ref gates re-fitting anyway. (This is pre-existing behavior, not introduced by this PR, but it's in the file.)

Test Coverage

The new test suite (DashboardNodePopup.test.tsx) is comprehensive:

• Flat-field rendering ✓
• Nested fallback fields ✓
• Source list rendering ✓
• Click handler callback ✓
• Disabled state when no onSourceSelect ✓
• Sources section hidden for single-source nodes ✓

One gap: there's no test for the App.tsx mount effect that consumes location.state.focusDmNodeId and opens the DM panel (App.tsx:565-579). That path is hard to unit-test in isolation, but a brief integration or routing test asserting that setActiveTab('messages') and setSelectedDMNode are called on navigation with the right state would give confidence this cross-component wiring stays intact.

Locale

The rename of nav.messages → "Node Details" in en.json is correct. However, the other locale files (de.json, es.json, pt_BR.json, etc.) still have the old translation strings that were previously "Messages"/"Nachrichten"/etc. Those will now show the i18n key or English fallback for non-English users. Not a blocker, but worth flagging for translators.

Summary: Solid PR. The main actionable items are (1) the sequential await in handleClose blocking the popup close, (2) the unauthenticated fetch for GeoJSON layers, and (3) the redundant getUserNewsStatus call. The code-duplication concerns are good candidates for a follow-up refactor.