[ERA-8348] Observation/Track Vector Layer#1387
[ERA-8348] Observation/Track Vector Layer#1387JoshuaVulcan wants to merge 34 commits intodevelopfrom
Conversation
|
To keep the environment alive:
|
|
🗑️ Environment torn down due to inactivity |
18 similar comments
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
|
🗑️ Environment torn down due to inactivity |
…before first reload. fix:prevent blank screen on logout from bad map lifecycle cleanup
…before first reload. fix:prevent blank screen on logout from bad map lifecycle cleanup
🚀 PR Environment Deployed
Access: https://era-8348.dev.pamdas.org |
… measure+test service speed+reliability
…ity. harmonizing vector tracks + live geojson overlay for nice timeslider responsiveness.:
There was a problem hiding this comment.
Pull request overview
This PR introduces vector-tile based rendering for subject tracks/observation segments and “stale” subjects, adds a realtime GeoJSON overlay to bridge vector tile TTL gaps, and updates track settings/UI to support segment filtering and time-of-day styling.
Changes:
- Add new vector-tile layers:
SubjectTileLayer(stale subjects) andTrackSegmentsLayer(observation/track segments), plus a GeoJSONRealtimeOverlayLayer. - Add new selectors/utilities to support vector tile URL range selection, legend data without GeoJSON tracks, and time-of-day Mapbox expressions.
- Add track segmentation settings (time-gap / speed filters) and update subject fetching to only pull “fresh” subjects via GeoJSON.
Reviewed changes
Copilot reviewed 47 out of 49 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/withSocketConnection/useRealTimeImplementation/config.js | Dispatch realtime overlay updates on subject_status socket events. |
| src/utils/tracks.test.js | Add tests for new track helpers; adjust time-of-day expectations. |
| src/utils/tracks.js | Add helpers for counting track points and finding closest position in descending series. |
| src/utils/map.test.js | Add tests for time-of-day Mapbox line-color expression builder. |
| src/utils/map.js | Add safe Mapbox cleanup helpers + time-of-day expression builder. |
| src/utils/datetime.js | Add timezone offset helper for Mapbox expressions. |
| src/selectors/tracks/index.test.js | Update track envelope test to expect Date object for until. |
| src/selectors/tracks/index.js | Export selectors; add vector tile range param + legend/visible-id selectors. |
| src/selectors/subjects/index.js | Add selectors for “fresh” subjects and fresh-only feature collection. |
| src/reducers/storage-config.js | Make useOptionalPersistence default consistently when storage value is missing. |
| src/reducers/index.js | Register realtimeOverlay reducer in root state. |
| src/hooks/useMapSources/index.js | Use safe source removal helper to avoid Mapbox teardown errors. |
| src/hooks/useMapLayers/index.js | Use safe layer removal helper to avoid Mapbox teardown errors. |
| src/hooks/useClusterPolygon/index.js | Use safe layer/source removal helpers on unmount. |
| src/ducks/tracks/index.test.js | Update expected initial track settings with new segmentation fields. |
| src/ducks/tracks/index.js | Add segmentation settings to track settings reducer + action creators. |
| src/ducks/subjects.js | Fetch only “fresh” subjects outside timeslider mode via updated_since. |
| src/ducks/realtime-overlay.js | New duck to fetch/maintain realtime overlay (subjects + segments). |
| src/constants/index.js | Add freshness/overlay windows; fix time-of-day minute ranges. |
| src/TrackSegmentsLayer/index.test.js | New tests for vector tile track segments layer setup/filter/paint/cleanup. |
| src/TrackSegmentsLayer/index.js | New vector-tile based observation/track segments layer. |
| src/TrackLegend/index.js | Add titleSuffix slot to support legend info trigger. |
| src/TrackLegend/TrackSettings/styles.module.scss | Add styling for new segmentation controls. |
| src/TrackLegend/TrackSettings/index.test.js | Update slider queries to include accessible name. |
| src/TrackLegend/TrackSettings/index.js | Add UI controls for segmentation settings (time gap / speed limit). |
| src/SubjectsLayer/index.js | Use fresh-only subjects by default; allow override (timeslider). |
| src/SubjectTrackLegend/styles.module.scss | Add styles for info button + details popover. |
| src/SubjectTrackLegend/index.js | Rework legend to not depend on fetched track GeoJSON; add details popover. |
| src/SubjectTileLayer/index.test.js | New tests for stale-subject vector tile click hydration behavior. |
| src/SubjectTileLayer/index.js | New vector-tile subject layer that excludes “fresh” GeoJSON subjects. |
| src/SubjectPopup/index.js | Default missing coordinateProperties to {}. |
| src/SubjectControls/index.js | Stop eagerly fetching tracks before toggling (commented out). |
| src/SpatialFeaturesLayer/index.test.js | Update to expect range param in tile URL; mock safe removals. |
| src/SpatialFeaturesLayer/index.js | Add range param to spatial features tiles; use safe cleanup helpers. |
| src/SideBar/SettingsPane/GeneralTab/AppRefreshFieldSet/index.js | Add effect attempting to set default restorable map position flag. |
| src/SideBar/MapLayersTab/SubjectsTab/Content.js | Stop eagerly fetching tracks before toggling group tracks (commented out). |
| src/ReportManager/DetailsSection/SchemaForm/utils/useMapLocationMarkers/index.js | Use safe layer/source removal helpers in marker cleanup. |
| src/RealtimeOverlayLayer/index.js | New realtime overlay Mapbox layer for subjects + recent segments. |
| src/MessageList/MessageListItem.js | Stop eagerly fetching tracks when jumping to message location (commented out). |
| src/Map/layers/useCrsBoundingBoxLayer/index.js | Use safe layer/source removal helpers in cleanup. |
| src/Map/index.js | Replace legacy tracks layer with vector segments + subject tiles + realtime overlay. |
| public/locales/sw/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| public/locales/pt/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| public/locales/ne-NP/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| public/locales/fr/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| public/locales/es/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| public/locales/en-US/tracks.json | Update subject track legend text + add popover/segmentation strings. |
| .gitignore | Ignore .yarn/install-state.gz. |
| /* add the vector source (URL includes range=45|all); only clean up on unmount */ | ||
| useEffect(() => { | ||
| if (!map) return; | ||
|
|
||
| if (!map.getSource(TRACK_SEGMENTS_SOURCE)) { | ||
| map.addSource(TRACK_SEGMENTS_SOURCE, { | ||
| type: 'vector', | ||
| tiles: [buildVectorTileUrl(rangeParam)], | ||
| minzoom: 0, | ||
| maxzoom: 22, | ||
| }); | ||
| } |
There was a problem hiding this comment.
When rangeParam changes, this effect reruns but it only calls addSource if the source does not already exist. That means the tile URL will not update for an existing source, so switching the track length across the 45-day threshold can leave the map using stale tiles (missing data). Consider recreating the shared source when rangeParam changes (remove/re-add + re-add layers, or centralize source management so it can be updated safely).
| useEffect(() => { | ||
| if (mapPositionRestorable === undefined) { | ||
| setMapPositionIsRestorable(true); | ||
| } | ||
| }, [mapPositionRestorable, setMapPositionIsRestorable]); |
There was a problem hiding this comment.
This useEffect is now effectively dead code: useOptionalPersistence(MAP_POSITION_STORAGE_KEY, true) initializes the local-storage value to { restore: true }, and useOptionalPersistence returns restorable = value?.restore ?? defaultRestorable, so mapPositionRestorable will never be undefined. Consider removing this effect (or, if you intended to detect “no stored value yet”, expose that explicitly from useOptionalPersistence).
src/SubjectControls/index.js
Outdated
| const onTrackButtonClick = async () => { | ||
| setTrackLoadingState(true); | ||
|
|
||
| if (!tracksLoaded) { | ||
| await fetchTracksIfNecessary([subject.id]); | ||
| } | ||
| // if (!tracksLoaded) { | ||
| // await fetchTracksIfNecessary([subject.id]); | ||
| // } | ||
|
|
There was a problem hiding this comment.
There is commented-out production code here for fetchTracksIfNecessary. If track fetching is no longer required for subject tracks (vector tiles), consider removing this commented block (or placing the legacy path behind a feature flag) so the current behavior is unambiguous.
| export const getTimeOfDayLineColorExpression = (propertyName, fallbackExpression, timeZoneOffsetMinutes = 0) => { | ||
| // Parse UTC hour and minute from ISO string (indices 11-12 = HH, 14-15 = MM) | ||
| const hourExpr = ['to-number', ['slice', ['get', propertyName], 11, 13]]; | ||
| const minuteExpr = ['to-number', ['slice', ['get', propertyName], 14, 16]]; | ||
| const utcMinutesSinceMidnight = ['+', ['*', hourExpr, 60], minuteExpr]; | ||
| // Localize: localMinutes = (utcMinutes + offset + 4320) % 1440 (4320 ensures positive before mod) | ||
| const localMinutesSinceMidnight = ['%', ['+', utcMinutesSinceMidnight, timeZoneOffsetMinutes, 4320], 1440]; | ||
|
|
There was a problem hiding this comment.
getTimeOfDayLineColorExpression computes minutes-since-midnight using % 1440, which yields 0 at exactly 00:00. TIME_OF_DAY_PERIODS uses rangeMinutesMax: 1440 for the 21:01–00:00 bucket and rangeMinutesMin: 1 for the next bucket, so midnight will not match any branch and will fall back to #2ec27e instead of the intended period color. Consider normalizing 0 to 1440 (or adjusting the period ranges / adding a special-case branch) so the Mapbox expression matches getTimeOfDayPeriodBasedOnTime behavior.
| const RealtimeOverlayLayer = ({ onSubjectClick }) => { | ||
| const map = useContext(MapContext); | ||
| const overlay = useSelector((state) => state.data.realtimeOverlay); | ||
| const isTimeOfDayColoringActive = useSelector( | ||
| (state) => state.view.trackSettings.isTimeOfDayColoringActive | ||
| ); | ||
| const timeOfDayTimeZone = useSelector((state) => state.view.trackSettings.timeOfDayTimeZone); | ||
|
|
There was a problem hiding this comment.
onSubjectClick is accepted as a prop but never used (no click handler is bound to OVERLAY_SUBJECTS_LAYER_ID). If overlay subjects are meant to be interactive, bind a click handler (similar to SubjectTileLayer) and call onSubjectClick; otherwise, remove the unused prop to avoid confusion.
| id, | ||
| title, | ||
| description: t('itemDescription', { count: 0 }), | ||
| icon: <img alt={t('itemIcon', { title })} className={styles.itemIcon} src={imageUrl || ''} />, |
There was a problem hiding this comment.
Using src={imageUrl || ''} can cause the browser to request the current document URL when imageUrl is missing/empty, creating noisy network requests and broken images in the legend. Consider conditionally rendering the <img> only when imageUrl is truthy, or using a dedicated placeholder image instead of an empty string.
| icon: <img alt={t('itemIcon', { title })} className={styles.itemIcon} src={imageUrl || ''} />, | |
| icon: imageUrl | |
| ? <img alt={t('itemIcon', { title })} className={styles.itemIcon} src={imageUrl} /> | |
| : null, |
src/Map/index.js
Outdated
| // await tracks_available ? fetchTracksIfNecessary([id]) : new Promise(resolve => resolve()); | ||
|
|
There was a problem hiding this comment.
There is commented-out production code here that used to ensure track GeoJSON was fetched before toggling track state. If this behavior is intentionally disabled due to the vector-tile migration, consider removing the commented code (or guarding the old path behind an explicit feature flag) to avoid confusion and accidental reintroduction of partial behavior later.
| // await tracks_available ? fetchTracksIfNecessary([id]) : new Promise(resolve => resolve()); |
| const onTrackButtonClick = async (e) => { | ||
| e.stopPropagation(); | ||
|
|
||
| if (subjectTrackIDsToLoad.length) { | ||
| setTrackLoadingState(true); | ||
|
|
||
| await fetchTracksIfNecessary(subjectTrackIDsToLoad); | ||
| // await fetchTracksIfNecessary(subjectTrackIDsToLoad); | ||
|
|
||
| setTrackLoadingState(false); | ||
| } |
There was a problem hiding this comment.
There is commented-out production code here for loading tracks (fetchTracksIfNecessary) before toggling group track visibility. If this is intentionally disabled for the vector-tile migration, consider removing the commented line or gating legacy behavior behind a feature flag to keep the codepath clear.
luixlive
left a comment
There was a problem hiding this comment.
Ok, this is the first pass and I didn't hold on any comments. With all the new sources, layers, filters, and all the old code that was touched, I'd say we really need thorough manual testing here. But the branch env is down: https://era-8348.dev.pamdas.org/ . Is there an env where I can test these changes?
About my review, I see a lot of commented code, Copilot already signaled it, and also lots of new code without a single test. I think the a way to summarize my review is that I notice inconsistencies in the code, between the new code and our existing practices, but also comparing the new code against itself. I guess I've been a bit defensive about moving to a total vibe-code approach and just let the AI do stuff based on only prompting. But I guess we are moving towards that. If we are going to do it, I think we need to establish practices, and definitely go with smaller PRs. This PR has big undocumented code blocks, without tests, and with obscure variable names like x; and some other blocks with a JSDoc twice the size of the function itself. It feels random, and it's hard to review.
| rangeString: '06:01 - 09:00', | ||
| rangeMinutesMin: 361, | ||
| rangeMinutesMax: 54, | ||
| rangeMinutesMax: 540, |
There was a problem hiding this comment.
Why are we modifying these values?
src/ducks/subjects.js
Outdated
|
|
||
| // When not in timeslider mode, only fetch subjects updated in the last | ||
| // hour — stale subjects are already rendered from the vector tile layer. | ||
| // The 1-hour window matches selectFreshSubjectIds in selectors/subjects. |
There was a problem hiding this comment.
I'd remove the The 1-hour window because we may change that value later in the constant and this comment would be outdated.
Also, defining an object just optionally apend a property and then spread it seems weird. Why not to instead simply calculate the updated_since parameter?
let updated_since = params?.updated_since;
if (!updated_since && !timeSliderActive) {
// The parameters didn't provide an updated_since value and the time slider is inactive
// Set the fresh subject window.
updated_since = new Date(Date.now() - FRESH_SUBJECT_WINDOW_MS).toISOString();
}| return () => { | ||
| map.removeLayer(LAYER_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); | ||
| map.removeSource(SOURCE_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); | ||
| safeRemoveMapLayer(map, LAYER_IDS.COORDINATE_REFERENCE_SYSTEM_BBOX); |
There was a problem hiding this comment.
The job is already done, but I think we could have simply added an optional chaining operator right? map?.removeLayer();. Jeje.
Of course that wouldn't help for any errors thrown by map, but we shouldn't hit any if properly handling the sources and layer. Which brings me back to the conversation of how unreliable the useMapLayer and useMapSource hooks are. I have the guess that we needed to add safeRemoveMapLayer just to avoid issues with those hooks poor map management. The name safeRemoveMapLayer implies that we are actually making sure to properly remove the layer, but we are not. If the map is defined and an error is thrown, we just ignore it, post a log, and go on without really cleaning the map layer or source.
src/Map/index.js
Outdated
| const popup = useSelector(state => state.view.popup); | ||
| const showReportHeatmap = useSelector(state => state.view.showReportHeatmap); | ||
| const showTrackTimepoints = useSelector(state => state.view.showTrackTimepoints); | ||
| // const showTrackTimepoints = useSelector(state => state.view.showTrackTimepoints); |
There was a problem hiding this comment.
Why to keep this code commented? Should we remove it? I see more commented lines below.
src/SubjectTrackLegend/index.js
Outdated
| const untilParam = until ? until.toISOString() : new Date().toISOString(); | ||
| const params = { since, until: untilParam }; | ||
| const responses = await Promise.all( | ||
| subjectIds.map((id) => axios.get(TRACKS_API_URL(id), { params, signal })) |
There was a problem hiding this comment.
Why are we doing axios calls here? We should use a thunk action creator and sanitize / store the data in a reducer.
src/SubjectTrackLegend/index.js
Outdated
| return total; | ||
| }; | ||
|
|
||
| const TrackDetailsTooltipContent = memo(function TrackDetailsTooltipContent({ |
There was a problem hiding this comment.
Moving this to a subfolder would keep this file cleaner and easier to read! Also, weird to see a function here. This repository favors arrow functions always. AI agent?
|
@luixlive thanks for the review! I'll get going on the first pass of changes, and in the meantime the env should be building correctly now. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…time-of-day styles with timeslider





No description provided.