Skip to content

fix(map): reactive vehicle popup props via $state for OSM and Google providers#419

Open
mukesharyal wants to merge 2 commits intoOneBusAway:developfrom
mukesharyal:vehicle-popup-reactive-state
Open

fix(map): reactive vehicle popup props via $state for OSM and Google providers#419
mukesharyal wants to merge 2 commits intoOneBusAway:developfrom
mukesharyal:vehicle-popup-reactive-state

Conversation

@mukesharyal
Copy link
Copy Markdown
Contributor

Problem

Two related bugs affected vehicle popup updates during the 30-second polling cycle:

OSM provider: updateVehicleMarker was assigning vehicleData (a plain object) directly to marker.popupComponent, which should always hold the Svelte mount instance. This meant:

  • The open popup never received prop updates — content stayed frozen at first-open values
  • On popup close, unmount(marker.popupComponent) received the plain data object instead of the component instance, silently leaking the real mounted component and its effects

Google provider: updateVehicleMarker called marker.popupComponent.$set(updatedData). Since VehiclePopupContent uses $props() (runes mode), $set is 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:

  1. At marker creation, marker.vehicleData is initialized as a $state({...}) reactive object
  2. mount(VehiclePopupContent, { target, props: marker.vehicleData }) passes that same reactive object, so Svelte tracks mutations to it
  3. In updateVehicleMarker, Object.assign(marker.vehicleData, {...}) mutates the existing object in place rather than replacing it, propagating updates reactively into any open popup

This keeps marker.popupComponent as a stable mount instance throughout the marker's lifetime, so unmount on popup close always receives the correct value.

Also aligns the nextStopName fallback between providers — OSM was returning 'N/A' for missing stops while Google returned undefined. 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.assign block in both providers. If a new prop is added to VehiclePopupContent and 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

  • Set 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 place
  • Close the popup — no console errors from unmount
  • Repeat with Google provider

…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')
Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

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

CI: build job failing

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.popupComponent by 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.popupComponent always holds the mount instance

Recommended Action

  1. Must fix: Move $state() calls to local variable declarations so the build passes (both providers)
  2. Must fix: Make lastUpdatedText a $derived in VehiclePopupContent.svelte so the timestamp actually updates on poll
  3. Should fix: Remove || 'N/A' from both providers' update paths — let the component's $t('NA') handle it
  4. 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
@mukesharyal
Copy link
Copy Markdown
Contributor Author

Hi @aaronbrethorst

I have made the necessary changes and also verified that it works with the lint, test as well as the build test.

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.

2 participants