fix(map): reactive vehicle popup props via $state for OSM and Google providers#419
fix(map): reactive vehicle popup props via $state for OSM and Google providers#419mukesharyal wants to merge 2 commits intoOneBusAway:developfrom
Conversation
…iders
- Create marker.vehicleData as a object at marker creation
- Pass marker.vehicleData directly to mount() so Svelte tracks mutations
- Replace Object.assign instead of reassigning vehicleData reference on update
- OSM: fixes popupComponent being overwritten with a plain data object,
which broke unmount on popup close and prevented live prop updates
- Google: fixes usage on a runes-mode component (()), which
was a no-op and left open popups stale on poll
- Align nextStopName fallback between providers ('N/A')
aaronbrethorst
left a comment
There was a problem hiding this comment.
Mukesh — the PR description here is exceptional. The root cause analysis of both the OSM and Google bugs is clear and accurate, and the reactive update idea is on the right track. However, the CI build is currently failing, and I found a couple of additional issues that should be addressed.
Critical Issues (2 found)
1. Build fails: $state(...) cannot be used as an assignment to an object property
The Svelte compiler rejects marker.vehicleData = $state({...}) with:
$state(...)can only be used as a variable declaration initializer or a class field
$state() is a compile-time rune with strict placement rules — it must appear as let x = $state(...) or as a class field initializer (vehicleData = $state(...)), not as an assignment to an arbitrary object property like a Leaflet marker or Google Maps marker.
Fix: Declare the reactive state as a local variable first, then store the reference on the marker:
let vehicleData = $state({
nextDestination: activeTrip.tripHeadsign,
vehicleId: vehicle.vehicleId,
lastUpdateTime: vehicle.lastUpdateTime,
nextStopName: this.stopsMap.get(vehicle.nextStop)?.name,
predicted: vehicle.predicted
});
marker.vehicleData = vehicleData;The $state() call creates the reactive proxy at declaration time. Assigning that proxy to marker.vehicleData is just storing a reference — Object.assign(marker.vehicleData, {...}) will still mutate the proxy and trigger reactivity. This needs to be done in both GoogleMapProvider.svelte.js and OpenStreetMapProvider.svelte.js.
2. lastUpdatedText won't actually update on poll — it's a plain const, not $derived
File: src/components/map/VehiclePopupContent.svelte:8-12
This PR correctly makes lastUpdateTime reactive via $state, but the value displayed in the popup — lastUpdatedText — is computed once at mount time and never recomputed:
let { nextDestination, vehicleId, lastUpdateTime, nextStopName, predicted } = $props();
const lastUpdatedText = formatLastUpdated(lastUpdateTime, {
min: $t('time.min'),
sec: $t('time.sec'),
ago: $t('time.ago')
});In Svelte 5, the <script> block runs once. $props() gives you reactive bindings, but passing lastUpdateTime to a plain function call in a const assignment captures its value at that moment. When the 30-second poll fires and Object.assign updates marker.vehicleData.lastUpdateTime, the prop updates reactively, but lastUpdatedText stays frozen at the first-open value.
Fix: Change the const to a $derived:
const lastUpdatedText = $derived(formatLastUpdated(lastUpdateTime, {
min: $t('time.min'),
sec: $t('time.sec'),
ago: $t('time.ago')
}));This is critical because "data last updated X ago" is arguably the most important piece of information in the vehicle popup — if a user watches a popup across two poll cycles, they'd see the timestamp frozen, which is exactly the staleness bug this PR aims to fix.
Note: This is technically a pre-existing issue (the old code never updated the popup at all), but since this PR's stated goal is making popup content reactive on poll, it should be fixed here.
Important Issues (1 found)
1. Hardcoded 'N/A' fallback in update path bypasses i18n
Files: GoogleMapProvider.svelte.js:400, OpenStreetMapProvider.svelte.js:355
Both providers use || 'N/A' in the update path:
nextStopName: this.stopsMap.get(vehicleStatus.nextStop)?.name || 'N/A',But the component already handles this with an i18n-aware fallback:
<strong>{nextStopName || $t('NA')}</strong>The 'N/A' at the provider level means the component will never see a falsy nextStopName after the first poll, so $t('NA') is bypassed and users see hardcoded English "N/A" regardless of locale.
Additionally, the creation path in both providers does not have this fallback:
nextStopName: this.stopsMap.get(vehicle.nextStop)?.name, // no || 'N/A'So on first render, a missing stop shows the localized $t('NA'), but after the first poll update it switches to hardcoded "N/A". This is inconsistent.
Fix: Remove || 'N/A' from both update paths. Let the component handle the fallback consistently.
Suggestions (0 found)
None — the approach is clean and the PR description's follow-up suggestion about a shared buildVehiclePopupData helper is a good future improvement.
Strengths
- Outstanding PR description: clear root cause, clear fix, clear testing plan
- The
$state()+Object.assign()approach is the right idea — just needs the$state()call moved to a local variable declaration to satisfy the compiler - Fixes a real bug where OSM was corrupting
marker.popupComponentby overwriting it with data - Fixes Google's use of the unsupported
$set()API - Both providers now follow the same reactive pattern, making the codebase more consistent
- The unmount path is now safe —
marker.popupComponentalways holds the mount instance
Recommended Action
- Must fix: Move
$state()calls to local variable declarations so the build passes (both providers) - Must fix: Make
lastUpdatedTexta$derivedinVehiclePopupContent.svelteso the timestamp actually updates on poll - Should fix: Remove
|| 'N/A'from both providers' update paths — let the component's$t('NA')handle it - After fixes, verify CI passes and re-run review
…r updated props and remove the hardcoded error message which was already being handled gracefully at the component level
|
I have made the necessary changes and also verified that it works with the lint, test as well as the build test. |
Problem
Two related bugs affected vehicle popup updates during the 30-second polling cycle:
OSM provider:
updateVehicleMarkerwas assigningvehicleData(a plain object) directly tomarker.popupComponent, which should always hold the Svelte mount instance. This meant:unmount(marker.popupComponent)received the plain data object instead of the component instance, silently leaking the real mounted component and its effectsGoogle provider:
updateVehicleMarkercalledmarker.popupComponent.$set(updatedData). SinceVehiclePopupContentuses$props()(runes mode),$setis not a supported update mechanism in Svelte 5 and was effectively a no-op — leaving open popups stale on poll.Fix
Both providers now follow the same pattern:
marker.vehicleDatais initialized as a$state({...})reactive objectmount(VehiclePopupContent, { target, props: marker.vehicleData })passes that same reactive object, so Svelte tracks mutations to itupdateVehicleMarker,Object.assign(marker.vehicleData, {...})mutates the existing object in place rather than replacing it, propagating updates reactively into any open popupThis keeps
marker.popupComponentas a stable mount instance throughout the marker's lifetime, sounmounton popup close always receives the correct value.Also aligns the
nextStopNamefallback between providers — OSM was returning'N/A'for missing stops while Google returnedundefined. Both should behave the same; pick one and apply consistently.Follow-up suggestion
The vehicle data shape is currently duplicated between the creation block and the
Object.assignblock in both providers. If a new prop is added toVehiclePopupContentand only one path is updated, that field will show stale data silently. A small shared helper (buildVehiclePopupData(vehicle, activeTrip, stopsMap)) would eliminate this risk and keep both providers in sync automatically.Testing
PUBLIC_OBA_MAP_PROVIDER=osm, select a route with live vehicles, open a vehicle popup, wait for the 30-second poll — popup content should update in placeunmount