Skip to content

Frontend: scope memo deps + fix freezing source ages (PR #4 review)#9

Merged
kninetimmy merged 1 commit into
mainfrom
frontend-perf-from-review
Jun 17, 2026
Merged

Frontend: scope memo deps + fix freezing source ages (PR #4 review)#9
kninetimmy merged 1 commit into
mainfrom
frontend-perf-from-review

Conversation

@kninetimmy

Copy link
Copy Markdown
Owner

Addresses the valid frontend items from the Gemini review on the M1.3 shell.

The reducer (applyDelta) keeps a stable Map reference for any collection that didn't change in a frame, so components can subscribe to exactly the slice they render and skip work on unrelated frames. Verified that before applying the suggestions.

Changes

  • recordLayerstrackFeatureCollection / featureFeatureCollection / activeLayers now take the specific Map(s) they iterate instead of the whole LiveState. This lets callers key memos on the narrow slice without tripping react-hooks/exhaustive-deps.
  • MapView / TrackList / EventFeed / LayerControl — select s.live.tracks / .features / .alerts / .events and key each useMemo on that slice, so a high-frequency track update no longer re-sorts alerts or regenerates the feature GeoJSON (and vice versa). Matters on the Pi.
  • MapView — guard the map "load" handler against firing after unmount (mapRef.current !== map) so a fast unmount can't touch a removed map.
  • SourceHealthPanel — "last record age" is derived from Date.now() at render and would freeze when no frames arrive; tick once a second so a silent/stalled source visibly ages (PRD §28.3).

Verification

Frontend gate green: tsc, eslint, vitest (19 tests), vite build.

This clears the review backlog from #4/#5 (backend half landed in #8).

🤖 Generated with Claude Code

Address the valid frontend items from the Gemini review on the M1.3
shell. The reducer (applyDelta) keeps a stable Map reference for any
collection that didn't change in a frame, so components can subscribe to
exactly the slice they render and skip work on unrelated frames.

- recordLayers: trackFeatureCollection/featureFeatureCollection/
  activeLayers now take the specific Map(s) they iterate instead of the
  whole LiveState. This lets callers key memos on the narrow slice without
  tripping react-hooks/exhaustive-deps.
- MapView, TrackList, EventFeed, LayerControl: select s.live.tracks /
  .features / .alerts / .events and key each useMemo on that slice, so a
  high-frequency track update no longer re-sorts alerts or regenerates the
  feature GeoJSON (and vice versa). Matters on the Pi.
- MapView: guard the map "load" handler against firing after unmount
  (mapRef.current !== map) so a fast unmount can't touch a removed map.
- SourceHealthPanel: "last record age" is derived from Date.now() at
  render and would freeze when no frames arrive; tick once a second so a
  silent/stalled source visibly ages (PRD §28.3).

Frontend gate green: tsc, eslint, vitest (19), vite build.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kninetimmy kninetimmy merged commit eab2a79 into main Jun 17, 2026
5 checks passed
@kninetimmy kninetimmy deleted the frontend-perf-from-review branch June 17, 2026 00:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request optimizes rendering performance across several components (EventFeed, MapView, LayerControl, TrackList) by subscribing to specific sub-collections from the store instead of the entire live state object, preventing unnecessary re-renders. It also introduces a periodic 1-second tick in SourceHealthPanel to keep the source age updated and adds an unmount guard in MapView. The review feedback highlights critical issues: a potential stale closure in the map loading callback, missing visibility filters for geo-feature layers, redundant sorting on every timer tick in SourceHealthPanel, and an opportunity to optimize active layer calculations in LayerControl.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

// "Last record age" is derived from Date.now() at render. Without a periodic
// re-render it would freeze whenever no frames arrive; tick once a second so a
// silent/stalled source visibly ages instead of looking fresh (PRD §28.3).
const [, setTick] = useState(0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

With the addition of the 1-second timer (setTick), the component now re-renders every second. Currently, statuses.sort(...) is executed on every single render (including every timer tick).

To avoid redundant sorting, consider selecting the stable sourceStatus Map reference from the store, and memoizing the sorted array so it only re-runs when the actual status data changes:

const sourceStatusMap = useStore((s) => s.live.sourceStatus);
const sortedStatuses = useMemo(() => {
  return [...sourceStatusMap.values()].sort((a, b) => a.source.localeCompare(b.source));
}, [sourceStatusMap]);

This also solves the issue where the current selector useStore((s) => [...s.live.sourceStatus.values()]) returns a new array reference on every store update, which would otherwise trigger unnecessary re-renders of this panel on any state change.

Comment on lines +35 to +36
const trackFc = useMemo(() => trackFeatureCollection(tracks), [tracks]);
const featureFc = useMemo(() => featureFeatureCollection(features), [features]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

While updating the map's memoized feature collections, note that the useEffect applying layerVisible filters (around line 134) only loops over ["tracks-point"]. It does not apply the filter to "features-fill", "features-outline", or "features-point".

As a result, toggling geo-feature layers (like fire detections or geofences) in the layer control UI will have no effect on the map. Consider updating that useEffect to also apply the filter to the feature layers:

for (const id of ["tracks-point", "features-fill", "features-outline", "features-point"]) {
  if (map.getLayer(id)) map.setFilter(id, trackFilter);
}

Comment on lines +51 to +53
// Bail if the component unmounted before the style finished loading —
// the cleanup has already removed this map, so touching it would throw.
if (mapRef.current !== map) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The map.on("load", ...) callback is registered once on mount and captures the initial pushData function (and thus the initial trackFc and featureFc values) due to a stale closure.

If tracks or features update while the map is loading, the useEffect that watches [trackFc, featureFc] will run and bail because readyRef.current is still false. Once the map loads, it will call the stale pushData with the initial (possibly empty) data, and the map will remain stale until the next update.

To fix this, you can use a ref to always access the latest trackFc and featureFc inside pushData, or use a state for ready instead of a ref so that the useEffect can re-run when the map becomes ready.

counts.set(p.layer, row);
}
return activeLayers(live).map((layer) => ({
return activeLayers(tracks, features).map((layer) => ({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Instead of calling activeLayers(tracks, features) which redundantly iterates over all tracks and features a second time, you can derive the active layers directly from the keys of the counts Map you just populated. This avoids the extra O(N) iteration and simplifies the logic.

Suggested change
return activeLayers(tracks, features).map((layer) => ({
return Array.from(counts.keys()).sort().map((layer) => ({

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.

1 participant