Migrate to PMTiles for routes and stations with API integration#38
Open
Mr-Technician wants to merge 11 commits into
Open
Migrate to PMTiles for routes and stations with API integration#38Mr-Technician wants to merge 11 commits into
Mr-Technician wants to merge 11 commits into
Conversation
Replace the client-side GTFS shape and station rendering with MapLibre VectorSources pointing at one .pmtiles file per provider (amtrak, brightline, cta, metra, metrotransit, trirail). Routes render from the transit_routes source-layer with each route's authored color; stations render from transit_stops as circles with zoom-interpolated radius. Per-provider min-zoom thresholds keep larger systems (Amtrak) visible from low zooms while local systems only appear when the user zooms into their service area. The old useShapes/useStations hooks, AnimatedRoute, AnimatedStationMarker, and station clustering are no longer rendered (kept on disk for now; deleted in the API-rewiring step). Also fix an initial-camera bug surfaced by the rewrite: when location permission resolves after the first render, Camera.initialViewState is already evaluated, so the map stayed at world view. Jump the camera to the resolved region the first time mapReady flips true. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
apps/mobile now has a thin layer for talking to api.trackyapp.net: - types/api.ts mirrors apps/api/spec.* and the static_read.go response shapes one-for-one (camelCase), so spec changes on the backend show up as TS errors here. - services/api-client.ts wraps every /v1/* endpoint we use (provider, stop, route, route/trains, trip, trip/stops, trips/lookup, departures, connections, trains/service, search) with a 1h in-memory cache for cacheable lookups. Three pending endpoints (run stops, stops/nearby, run position) are stubbed so call sites can be written ahead of the backend landing them. - services/ws-client.ts is a singleton WebSocket manager: opens on first subscribe, ref-counts provider topics, re-sends subscribe on reconnect (exponential backoff to 30s), and closes when the last listener unsubscribes. - context/RealtimeContext.tsx wraps the WS singleton, subscribes to all providers on mount, and exposes useRealtimePositions(provider?) for consumers. No call sites are wired up yet — the existing GTFS/Transitdocs paths still power live trains, search, and modals. The next commit swaps consumers over to these new services. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switches the highest-traffic consumers from local GTFS parsing /
Transitdocs polling to the new backend:
- services/api.ts (TrainAPIService) now goes through services/api-client.
getTrainDetails resolves namespaced trip ids via /v1/trips/{tripId} or
falls back to /v1/trips/lookup by train number + date. getTrainsForStation
hits /v1/departures and fans out /v1/trips/{tripId}/stops per departure
(1h cache makes repeats cheap). getTrainDetails attaches realtime
position from the WS snapshot rather than polling Transitdocs. Drops
the AMTRAK_ROUTE_NAMES table.
- hooks/useLiveTrains is now a thin adapter over useRealtimePositions —
no more 15s polling. The intervalMs prop is kept for backwards-compat
but ignored.
- hooks/useRealtime re-runs enrichment whenever the WS feed updates
rather than on a 20s timer.
- context/RealtimeContext prefetches route metadata for incoming routeIds
so display sites resolve route names without a flicker.
- services/ws-client tracks a per-provider snapshot and exposes
findPosition() / getLatestPositions() for non-React readers.
- services/api-client exposes getCachedRoute()/prefetchRoute() so
render-time route lookups work without async.
- hooks/useFrequentlyUsed swaps the bulk-fetch of routes/stops (gone
with the GTFS parser) for a small static default list.
- MapScreen tree wraps with RealtimeProvider; drops the GTFSRefresh
provider and refresh-button plumbing. Loading overlay + GTFS-ready
gating removed.
Modal/search components that still go through gtfsParser directly
(TrainDetailModal, TwoStationSearch, search, etc.) are left for the
next pass — they're broken at runtime until the gtfsParser shim or
direct API rewiring lands.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…d consumers - services/location-suggestions.ts now uses /v1/stops/nearby (stub — fails until backend lands) and /v1/departures for upcoming trains. Drops the parser argument; permissions and best-effort failure handling preserved. - screens/MapScreen restores GTFSRefreshProvider in its tree. The new API + WS paths (TrainAPIService, useLiveTrains, useRealtime, RealtimeContext) bypass the local GTFS parser, but several modal/ search consumers (TrainDetailModal, TwoStationSearch, search, calendar-sync) still use gtfsParser sync methods. Keeping the fallback loader means those consumers continue to work while migration happens piecemeal. - context/GTFSRefreshContext drops the now-unused gtfsParser import forwarded into LocationSuggestionsService. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- getTrainsForStation no longer drops a departure when its trip-stops fan-out request fails or returns empty. We now fall back to a minimal Train carrying the user's stop time directly from the DepartureItem, with a single-entry intermediateStops so DepartureBoardModal's per-stop time lookup still resolves. List view never silently empties on a partial backend. - Route line taps now register a handler. With no dedicated route detail UI yet, the tap fires haptic feedback and an Alert showing the long/short name and provider — enough to confirm selection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates the Tracky mobile app from local GTFS parsing and polling to a REST API-backed model with WebSocket realtime position streaming, PMTiles vector-tile map rendering, typed API shapes, an API client with TTL caching, a singleton WS client, a RealtimeProvider context and hooks, and bundled map style assets. ChangesAPI-Backed Realtime & PMTiles Map Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mobile/services/location-suggestions.ts (1)
70-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark initialization complete before the async work succeeds.
Because
initializedflips totruebefore permissions/location/API calls finish, any first-run failure here permanently disables suggestions until the app restarts. That includes temporary network issues and the current/v1/stops/nearbystub path.Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/services/location-suggestions.ts` around lines 70 - 99, The initialize function sets the module-level flag initialized to true before async calls complete, which causes permanent disablement on any first-run failure; move or defer setting initialized = true until after the permission check, location retrieval (Location.requestForegroundPermissionsAsync and Location.getCurrentPositionAsync), and getNearbyStops call have succeeded (or alternatively reset initialized = false in every catch/failure branch), and ensure any early returns (permission denied or API errors) do not leave initialized true; update the initialize function and the same pattern around lines 141-143 to only mark initialization complete on success or to clear the flag on failure.
🧹 Nitpick comments (2)
apps/mobile/context/RealtimeContext.tsx (1)
83-92: ⚡ Quick winReturn a shared empty array for missing-provider lookups.
The hook comment says “stable empty array,” but
positionsByProvider[provider] ?? []creates a new array whenever memo recomputes, causing avoidable downstream effect/rerender churn.♻️ Suggested fix
const EMPTY_POSITIONS: PositionsByProvider = {}; +const EMPTY_LIST: ApiTrainPosition[] = []; @@ return useMemo(() => { - if (provider) return positionsByProvider[provider] ?? []; + if (provider) return positionsByProvider[provider] ?? EMPTY_LIST;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/context/RealtimeContext.tsx` around lines 83 - 92, The hook useRealtimePositions creates a new empty array on missing-provider lookups (positionsByProvider[provider] ?? []), which breaks the “stable empty array” promise; fix by declaring a single module-level constant EMPTY_ARRAY: ApiTrainPosition[] = [] and return positionsByProvider[provider] ?? EMPTY_ARRAY in useRealtimePositions so callers get a stable reference when a provider has no positions; keep all other logic the same and update any type annotations if needed.apps/mobile/types/api.ts (1)
12-151: ⚡ Quick winUse the shared
ProviderIdtype for provider fields.Typing
provider/providerIdas rawstringdrops useful compile-time guarantees and makes provider-key mismatches easier to miss.♻️ Suggested type-tightening diff
+import type { ProviderId } from '../constants/providers'; + export interface ApiAgency { - providerId: string; + providerId: ProviderId; gtfsAgencyId: string; name: string; @@ export interface ApiRoute { - providerId: string; + providerId: ProviderId; @@ export interface ApiStop { - providerId: string; + providerId: ProviderId; @@ export interface ApiTrip { - providerId: string; + providerId: ProviderId; @@ export interface ApiScheduledStopTime { - providerId: string; + providerId: ProviderId; @@ export interface ApiTrainItem { - providerId: string; + providerId: ProviderId; @@ export interface ApiServiceInfo { - providerId: string; + providerId: ProviderId; @@ export interface ApiTrainPosition { - provider: string; + provider: ProviderId; @@ export interface ApiTrainStopTime { - provider: string; + provider: ProviderId; @@ export interface RealtimeUpdate { type: 'realtime_update'; - provider: string; + provider: ProviderId; positions: ApiTrainPosition[]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/types/api.ts` around lines 12 - 151, Replace all provider/providerId string annotations with the shared ProviderId type to tighten typing: update ApiAgency.providerId, ApiRoute.providerId, ApiStop.providerId, ApiTrip.providerId, ApiScheduledStopTime.providerId, ApiTrainItem.providerId, ApiServiceInfo.providerId, ApiTrainPosition.provider, ApiTrainStopTime.provider, ApiSearchHit.provider and any other provider/providerId fields (ApiDepartureItem/ApiConnectionItem inherit from ApiTrip) to use ProviderId; import ProviderId from the shared types module at the top of the file and run typechecks to ensure no remaining plain string usages remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/context/RealtimeContext.tsx`:
- Around line 39-68: The effect builds next from latestRef.current so removed
providers remain in positionsByProvider; update the effect to prune stale
provider keys whenever providerIds change: compute ids = [...providerIds], then
build next as an object containing only keys for those ids (e.g., map ids to
latestRef.current[id] or EMPTY list if missing), set latestRef.current = next
and setPositionsByProvider(next) before subscribing; keep the existing onUpdate
handler and wsClient.subscribe usage but ensure any merge uses only current ids
(so removed provider buckets are dropped).
In `@apps/mobile/hooks/useLiveTrains.ts`:
- Around line 56-58: The current lastUpdated value (in useLiveTrains) uses
Date.now() which changes on re-renders once liveTrains is non-empty; instead
compute lastUpdated from the latest parsed position timestamp contained in the
feed entries so it only moves when new feed data arrives: iterate liveTrains to
extract each train's parsed position timestamp (e.g. the timestamp field on the
parsed position object inside each train), take the maximum timestamp (or null
if none), and assign that numeric timestamp (or null) to lastUpdated rather than
Date.now(); update the code that sets loading/error/lastUpdated accordingly so
consumers see a freshness signal derived from feed data.
In `@apps/mobile/hooks/useRealtime.ts`:
- Around line 45-65: The effect in useRealtime.ts currently triggers
TrainAPIService.refreshRealtimeData for every WS position event and can apply
stale results; change it to coalesce rapid position updates and add a
stale-write guard: batch or debounce events before calling the async refresh (so
useEffect/debounced timer or an in-flight promise map keyed by train id) and
when each Promise resolves, verify the resolved data still matches the latest
trainsRef snapshot (using hasRealtimeChanged or a sequence/version token) before
calling setTrainsRef.current(updated); also ensure
TrainActivityManager.onRealtimeUpdate runs only for non-stale transitions and
that cancelled/in-flight work is ignored (use a local cancelled flag or
incremental version counter to drop old responses).
In `@apps/mobile/screens/MapScreen.tsx`:
- Around line 683-692: The adapter handleProviderTileStationPress builds a
stations array using payload.stopCode for stations[0].id which loses the
provider namespace; change the adapter to populate stations[0].id with
payload.stopId (the namespaced id) so that handleStationPress and downstream
Stop.stop_id use the correct namespaced identifier (StationTapPayload ->
handleProviderTileStationPress -> stations[0].id -> handleStationPress).
In `@apps/mobile/services/api.ts`:
- Around line 195-219: The Train builder is emitting bare stop codes (fromCode,
toCode, and intermediateStops[].code) which loses provider namespace; update the
Train construction (the object assigned to const train and the similar block
around lines 253-273) to prefix stop ids with the provider (use trip.providerId
when available, falling back to d.providerId) so station references are
namespaced (e.g., `${provider}:${stop_id}`) before assigning to fromCode,
toCode, and each intermediateStops[].code, and ensure any helper calls like
convertGtfsTimeForStop still receive the original stop id if needed but the
Train fields store the namespaced id to avoid DEFAULT_PROVIDER round-trips.
In `@apps/mobile/services/location-suggestions.ts`:
- Line 85: Remove the detailed coordinate log that prints latitude/longitude in
the LocationSuggestions flow: stop calling logger.info with the precise
latitude.toFixed(3) and longitude.toFixed(3) values; instead log a generic
success/failure message (e.g., "LocationSuggestions: location received" or
"LocationSuggestions: location retrieval failed") using the existing
logger.info/logger.error, keeping references to the same LocationSuggestions
context and the variables latitude/longitude only for internal use (no persisted
or logged PII).
- Around line 30-37: The adapter currently drops provider info by setting
stop_id to s.code; update adaptStop to preserve the provider namespace (so
downstream lookup uses the correct provider). Change stop_id assignment in
adaptStop to include the provider when present (for example use a composite id
like `${s.provider}:${s.code}` or otherwise keep s.provider prefixed) while
leaving stop_name, stop_lat, stop_lon, and stop_timezone as-is; ensure the logic
falls back to s.code if provider is missing.
In `@apps/mobile/services/ws-client.ts`:
- Around line 125-129: There’s a stale reconnect timer that can fire after
ensureConnected()/subscribe() creates a new socket; before calling connect()
cancel the pending timer (e.g. clearTimeout(this.reconnectTimer) and set
this.reconnectTimer = null) so a later timer won’t open a second socket, and
apply the same cancellation in the subscribe() path and at the start of
connect() so any existing timer is cleared whenever a new connection is
initiated; also ensure the code that schedules a reconnect assigns
this.reconnectTimer so it can be cleared later.
---
Outside diff comments:
In `@apps/mobile/services/location-suggestions.ts`:
- Around line 70-99: The initialize function sets the module-level flag
initialized to true before async calls complete, which causes permanent
disablement on any first-run failure; move or defer setting initialized = true
until after the permission check, location retrieval
(Location.requestForegroundPermissionsAsync and
Location.getCurrentPositionAsync), and getNearbyStops call have succeeded (or
alternatively reset initialized = false in every catch/failure branch), and
ensure any early returns (permission denied or API errors) do not leave
initialized true; update the initialize function and the same pattern around
lines 141-143 to only mark initialization complete on success or to clear the
flag on failure.
---
Nitpick comments:
In `@apps/mobile/context/RealtimeContext.tsx`:
- Around line 83-92: The hook useRealtimePositions creates a new empty array on
missing-provider lookups (positionsByProvider[provider] ?? []), which breaks the
“stable empty array” promise; fix by declaring a single module-level constant
EMPTY_ARRAY: ApiTrainPosition[] = [] and return positionsByProvider[provider] ??
EMPTY_ARRAY in useRealtimePositions so callers get a stable reference when a
provider has no positions; keep all other logic the same and update any type
annotations if needed.
In `@apps/mobile/types/api.ts`:
- Around line 12-151: Replace all provider/providerId string annotations with
the shared ProviderId type to tighten typing: update ApiAgency.providerId,
ApiRoute.providerId, ApiStop.providerId, ApiTrip.providerId,
ApiScheduledStopTime.providerId, ApiTrainItem.providerId,
ApiServiceInfo.providerId, ApiTrainPosition.provider, ApiTrainStopTime.provider,
ApiSearchHit.provider and any other provider/providerId fields
(ApiDepartureItem/ApiConnectionItem inherit from ApiTrip) to use ProviderId;
import ProviderId from the shared types module at the top of the file and run
typechecks to ensure no remaining plain string usages remain.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e960cb3-750d-46ae-9a48-7bbaf8f6f62b
📒 Files selected for processing (15)
apps/mobile/app.config.tsapps/mobile/components/map/ProviderTiles.tsxapps/mobile/constants/config.tsapps/mobile/constants/providers.tsapps/mobile/context/GTFSRefreshContext.tsxapps/mobile/context/RealtimeContext.tsxapps/mobile/hooks/useFrequentlyUsed.tsapps/mobile/hooks/useLiveTrains.tsapps/mobile/hooks/useRealtime.tsapps/mobile/screens/MapScreen.tsxapps/mobile/services/api-client.tsapps/mobile/services/api.tsapps/mobile/services/location-suggestions.tsapps/mobile/services/ws-client.tsapps/mobile/types/api.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 9 file(s) based on 8 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 9 file(s) based on 8 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/mobile/package.json (1)
65-65: ⚡ Quick winConsider upgrading
@types/react-native-vector-iconsto track RNVI 10.x major version.Line 65 adds
@types/react-native-vector-icons@^6.4.18for runtimereact-native-vector-icons@^10.3.0. The 4-major-version gap indicates the types package is from an older era—react-native-vector-icons 10.3.0 does not ship built-in types, so the separate@typespackage is necessary, but the 6.x definitions may diverge from 10.x API surface.Verify
npm run type-checkpasses cleanly with this combination. If type errors emerge around icon names, props, or exports, consider checking if a newer or maintained types package exists, or file an issue with the types maintainers for RNVI 10.x support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/package.json` at line 65, The package entry uses an old types package "@types/react-native-vector-icons@^6.4.18" while runtime uses "react-native-vector-icons@^10.3.0"; run "npm run type-check" and if type errors appear around icon names/props/exports, upgrade or replace the typings: first try finding a newer `@types` package or an official types export from react-native-vector-icons, update the dependency in package.json to the appropriate package/version, and re-run "npm run type-check" to confirm errors are resolved (if no newer types exist, add a short follow-up note or open an issue with the types maintainers).apps/mobile/screens/MapScreen.tsx (1)
551-562: 💤 Low valueConsider fetching the stop name asynchronously for better UX.
The synthesized
StopusesstationCodeasstop_name, which means users will briefly see a code like "NYP" instead of "New York Penn Station" until the modal fetches full details. This is functional but could be slightly jarring.If the full station name is available from the calling context (e.g., from
train.intermediateStops), passing it through would provide a smoother experience without an extra API call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/screens/MapScreen.tsx` around lines 551 - 562, The synthesized Stop currently sets stop_name to stationCode which shows a code instead of a human-friendly name; update the code that constructs the Stop (the object passed to navigateToStation in MapScreen.tsx) to use an available full station name from the calling context (for example from train.intermediateStops or any nearby prop/state) and fall back to stationCode only if no name is present; if the name must be fetched, trigger an async lookup before calling navigateToStation (or await it inside the handler) so navigateToStation receives a Stop with stop_name populated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/mobile/package.json`:
- Line 65: The package entry uses an old types package
"@types/react-native-vector-icons@^6.4.18" while runtime uses
"react-native-vector-icons@^10.3.0"; run "npm run type-check" and if type errors
appear around icon names/props/exports, upgrade or replace the typings: first
try finding a newer `@types` package or an official types export from
react-native-vector-icons, update the dependency in package.json to the
appropriate package/version, and re-run "npm run type-check" to confirm errors
are resolved (if no newer types exist, add a short follow-up note or open an
issue with the types maintainers).
In `@apps/mobile/screens/MapScreen.tsx`:
- Around line 551-562: The synthesized Stop currently sets stop_name to
stationCode which shows a code instead of a human-friendly name; update the code
that constructs the Stop (the object passed to navigateToStation in
MapScreen.tsx) to use an available full station name from the calling context
(for example from train.intermediateStops or any nearby prop/state) and fall
back to stationCode only if no name is present; if the name must be fetched,
trigger an async lookup before calling navigateToStation (or await it inside the
handler) so navigateToStation receives a Stop with stop_name populated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1f21775-7e1b-4945-a5b4-772039333129
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/mobile/context/RealtimeContext.tsxapps/mobile/hooks/useLiveTrains.tsapps/mobile/hooks/useRealtime.tsapps/mobile/package.jsonapps/mobile/screens/MapScreen.tsxapps/mobile/services/api.tsapps/mobile/services/location-suggestions.tsapps/mobile/services/ws-client.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/mobile/services/ws-client.ts
- apps/mobile/context/RealtimeContext.tsx
- apps/mobile/hooks/useLiveTrains.ts
- apps/mobile/services/location-suggestions.ts
Replace OpenFreeMap liberty styles with custom Apple Maps-inspired color palettes for both light and dark modes. Co-authored-by: Jason Xu <jasonxu@macbookair.lan> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/assets/apple-dark-style.json`:
- Line 1: The boundary_3 layer uses an invalid interpolate expression
["interpolate",["linear",1],["zoom"],7,1,11,2]; remove the extra base parameter
(the numeric 1) so the expression becomes
["interpolate",["linear"],["zoom"],7,1,11,2]; update this in
apple-dark-style.json (layer id "boundary_3") and do the identical fix in
apple-light-style.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72aca922-0d6e-480d-ae1c-b1c86523be35
📒 Files selected for processing (3)
apps/mobile/assets/apple-dark-style.jsonapps/mobile/assets/apple-light-style.jsonapps/mobile/constants/map-styles.ts
…oading - Removed the GTFSParser class and its associated methods, transitioning to a synchronous API-backed cache for stop lookups. - Introduced `lookupStop` and `lookupAgencyTimezone` functions to fetch stop data and agency timezone directly from the API client cache. - Updated `StationLoader` to use the new cache for retrieving station information by code. - Modified utility functions to leverage the new API cache, ensuring consistent behavior across the application. - Cleaned up related imports and removed unused GTFS parsing logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/mobile/components/ui/TrainDetailModal.tsx (2)
186-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry destination weather after the stop cache hydrates.
lookupStop(toCode)is now lazy. If the stop is missing on the first render, this effect returns early and never runs again because its dependency list does not track cache hydration. On a cold start the destination weather stays blank until the modal is reopened.Suggested fix
- useApiCacheVersion(); + const cacheVersion = useApiCacheVersion(); ... - }, [toCode, tempUnit]); + }, [toCode, tempUnit, cacheVersion]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/ui/TrainDetailModal.tsx` around lines 186 - 218, The effect currently exits early when lookupStop(toCode) returns undefined and never re-runs when the stop cache later hydrates; update the useEffect that houses fetchWeather (the effect watching toCode and tempUnit) to also depend on the stop-cache hydration signal so it re-runs after the cache populates — e.g., read the store flag that indicates stops have hydrated (commonly named isStopsHydrated / stopsLoaded / stopsHydrated) and add it to the dependency array, or include the resolved destStop value in deps; ensure you still guard on toCode, call lookupStop(toCode) inside the effect (as in fetchWeather), and only return early when toCode is falsy so fetchWeather runs once the cache is hydrated and then sets weatherData via setWeatherData.
221-268:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't permanently skip stop weather when a cache lookup misses.
This effect snapshots
lookupStop(stop.code)once and storesstopWeatherKeyRef.currentbefore all stop metadata is guaranteed to exist. If any stop is uncached on the first pass, that stop's weather is skipped for the lifetime of the modal because later cache fills do not invalidate this key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/ui/TrainDetailModal.tsx` around lines 221 - 268, The effect currently sets stopWeatherKeyRef.current up-front which freezes out stops that were uncached at that moment; instead defer or make the key reflect cache availability: remove the early assignment of stopWeatherKeyRef.current in the useEffect and only set it after fetchAllStopWeather finishes successfully (inside fetchAllStopWeather after Promise.all and before setStopWeather) or compute the key to include a cached-count marker (e.g. count of lookupStop(...) hits) so that when lookupStop(cache) later fills the key changes and the effect reruns; locate symbols: stopWeatherKeyRef.current, lookupStop, fetchAllStopWeather, setStopWeather and update the logic so the ref is assigned only after verifying current cache state (or includes cached-count) to avoid permanently skipping uncached stops.apps/mobile/components/TwoStationSearch.tsx (1)
329-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild cache-backed suggestions when stop lookups resolve.
This effect only runs once, but it bakes
lookupStop(...)results intopopularSuggestionsandhistorySuggestions. On a cold start those lookups are often still empty, which leaves the history shortcut missing or non-actionable even after the cache fills because the effect never reruns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/components/TwoStationSearch.tsx` around lines 329 - 380, The effect currently bakes synchronous lookupStop(...) results into popularSuggestions and historySuggestions once and never updates when the stop cache fills; update the effect so lookups are resolved asynchronously and the suggestions are rebuilt when lookupStop returns a value (e.g. implement an async helper that retries/awaits lookupStop(code) with a small backoff or subscribes to your stop-cache ready event), then call setPopularSuggestions and setHistorySuggestions again with the resolved stop objects (update the logic around getRoutes/lookupStop/popular construction and the TrainStorageService.getTripHistory handling to use the async-resolved stops instead of assuming lookupStop immediately returns). Ensure useEffect either depends on a cache-ready signal or internally re-runs its lookup logic so popular/history suggestions become actionable once lookupStop values are available.
🧹 Nitpick comments (5)
apps/mobile/hooks/useTravelOverlay.ts (1)
98-102: 💤 Low valueAsync IIFE without error handling.
The async IIFE could throw if
TrainStorageService.getTripHistory()fails. Consider adding a try-catch to prevent unhandled rejection.Proposed fix
(async () => { + try { const history = await TrainStorageService.getTripHistory(); tripHistoryRef.current = history; await resolveTravelOverlay(history, profileYear); + } catch (err) { + console.warn('[useTravelOverlay] Failed to load trip history', err); + } })();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/hooks/useTravelOverlay.ts` around lines 98 - 102, The async IIFE that calls TrainStorageService.getTripHistory() and resolveTravelOverlay(profileYear) can throw and should be wrapped in a try-catch: inside the IIFE call TrainStorageService.getTripHistory(), assign tripHistoryRef.current and call resolveTravelOverlay(profileYear) in a try block, and in the catch block handle/log the error (e.g., use console.error or the app logger) and set a safe fallback for tripHistoryRef.current (like an empty array) or call the error handling path so the hook doesn’t produce an unhandled rejection; keep references to TrainStorageService.getTripHistory, tripHistoryRef.current, and resolveTravelOverlay to locate the code.apps/mobile/hooks/useTripDetail.ts (1)
128-138: 💤 Low valueNon-404 errors logged at debug level may hide issues.
Errors from
getRunStopsthat aren't 404s are logged atdebuglevel, which might be too quiet for unexpected failures. Consider usingwarnfor non-404 errors.Proposed fix
} catch (err) { if (!(err instanceof ApiError && err.status === 404)) { - logger.debug('[useTripDetail] run stops unavailable', err); + logger.warn('[useTripDetail] run stops fetch failed', err); } return { rows: [], available: false }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/hooks/useTripDetail.ts` around lines 128 - 138, In useTripDetail, the delaysPromise async IIFE that calls getRunStops logs non-404 errors at debug level; change the logger call in that catch branch to logger.warn and include the error object so unexpected failures are more visible (keep the existing 404 check and the return { rows: [], available: false } behavior). Target the delaysPromise block and update the logger.debug('[useTripDetail] run stops unavailable', err) to use logger.warn with the same message and error details.apps/mobile/services/api-client.ts (1)
355-362: 💤 Low valueUnhandled promise rejection is silently swallowed.
The
.catch(() => {})innotifyAfterswallows all errors without any logging. While the comment explains the intent, silent failures can make debugging difficult in production.Consider adding debug logging
function notifyAfter<T>(p: Promise<T>): void { p.then(() => { cacheVersion += 1; for (const l of cacheListeners) l(); - }).catch(() => { - // leave un-cached so a later attempt can retry + }).catch((err) => { + // leave un-cached so a later attempt can retry + if (__DEV__) console.debug('[api-client] prefetch failed', err); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/services/api-client.ts` around lines 355 - 362, The catch in function notifyAfter silently swallows all errors; change the .catch handler on the Promise returned by notifyAfter to log the error (e.g., console.error or the module's logger) along with context (mention notifyAfter, cacheVersion and affected operation) while preserving the current behavior of not caching/rethrowing so later attempts can retry; update the catch block associated with notifyAfter (which references cacheVersion and cacheListeners) to emit that diagnostic information.apps/mobile/__tests__/utils/train-helpers.test.ts (1)
44-49: 💤 Low valueTest relies on fallback parsing, not cache — verify this is intentional.
The tests for
'2026-03-10_AMTK_156'and'2026-01-16_AMTK_543'aren't mocked ingetCachedTrip, so they exercise the underscore-parsing fallback path rather than the cache path. This provides good coverage of the fallback logic but doesn't verifyprefetchTripis called on cache miss.Consider adding an assertion that
prefetchTripwas called for unmocked trip IDs if you want to verify the prefetch behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/__tests__/utils/train-helpers.test.ts` around lines 44 - 49, The tests currently exercise the underscore-parsing fallback (via extractTrainNumber) because getCachedTrip isn't mocked for '2026-03-10_AMTK_156' and '2026-01-16_AMTK_543'; update the tests in train-helpers.test.ts to explicitly assert that prefetchTrip was invoked on cache miss by mocking getCachedTrip to return undefined for those trip IDs (or spying on getCachedTrip/prefetchTrip) and adding expect(prefetchTrip).toHaveBeenCalledWith(...) (or equivalent) after calling extractTrainNumber so the test verifies prefetchTrip is triggered when getCachedTrip misses while still covering the fallback parsing logic.apps/mobile/utils/api-stop-cache.ts (1)
24-28: 💤 Low value
splitNamespacedduplicated across files.This exact function also exists in
useTravelOverlay.ts(lines 19-23). Consider extracting to a shared utility to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/utils/api-stop-cache.ts` around lines 24 - 28, The function splitNamespaced is duplicated (same logic in splitNamespaced and also implemented inside useTravelOverlay), so extract it to a shared utility (e.g., utils/splitNamespaced.ts) and export it; then replace the local implementations with an import of the shared splitNamespaced. Update all call sites (including the function named splitNamespaced in api-stop-cache and the implementation inside useTravelOverlay) to use the shared exported function and remove the duplicate definitions to prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mobile/services/api-client.ts`:
- Around line 287-302: getCachedRoute currently looks up `route:${routeId}`
which doesn't match the `route:${provider}:${code}` key used by
getRoute/prefetchRoute; update getCachedRoute to parse routeId (find sep =
routeId.indexOf(':')) and if sep > 0 build the cache key as
`route:${provider}:${code}` (provider = routeId.slice(0,sep), code =
routeId.slice(sep+1)) before calling getCached<ApiRoute>, otherwise fall back to
the existing `route:${routeId}` behavior so both getCachedRoute and
prefetchRoute/getRoute use the same cache key format.
In `@apps/mobile/services/calendar-sync.ts`:
- Around line 65-90: The DISABLED_RESULT constant is being returned by reference
from syncPastTrips and syncFutureTrips which can leak mutable state (notably
addedTrips) across callers; change both functions to return a fresh SyncResult
object per call (e.g., construct a new object that copies the fields from
DISABLED_RESULT but uses a new array for addedTrips) instead of returning the
DISABLED_RESULT reference so consumers cannot mutate shared state.
In `@apps/mobile/services/station-loader.ts`:
- Around line 24-35: The current stationLoader.getStationByCode returns
undefined on a cold miss because lookupStop is lazy; update the contract so
callers don't get silent failures by either (A) prewarming the cache at startup
(call the existing lookup/fetch logic once and populate whatever cache
lookupStop reads before notifications/storage run) or (B) convert
stationLoader.getStationByCode into an async method that awaits the
fetch/populate routine and then returns a StationBounds (i.e., change
getStationByCode to async, await the underlying fetch/populate used by
lookupStop, and have callers (notifications/storage) await it). Locate and
update stationLoader.getStationByCode and the cache-population routine used by
lookupStop (and all call sites of getStationByCode) to follow the chosen
approach so resolution is guaranteed on first call.
In `@README.md`:
- Around line 309-353: The new "API Surface (High Level)" section conflicts with
earlier architecture text describing direct Transitdocs polling, local GTFS
parsing, and a "15-second realtime cache"; update the older architecture
paragraphs to reconcile with the backend+WebSocket model by either (A) replacing
the Transitdocs/GTFS/polling description with a short summary of the centralized
REST+WS approach (referencing the "API Surface (High Level)" and "Realtime
Stream (WebSocket)" headings) or (B) explicitly documenting how the legacy/local
GTFS/15-second cache maps into or is deprecated by the new /v1/* REST endpoints
and WS /ws/realtime stream; ensure references used elsewhere in the README
(search, realtime, storage hooks listed under "Where This Is Consumed in the
App") remain consistent with whichever model you choose.
---
Outside diff comments:
In `@apps/mobile/components/TwoStationSearch.tsx`:
- Around line 329-380: The effect currently bakes synchronous lookupStop(...)
results into popularSuggestions and historySuggestions once and never updates
when the stop cache fills; update the effect so lookups are resolved
asynchronously and the suggestions are rebuilt when lookupStop returns a value
(e.g. implement an async helper that retries/awaits lookupStop(code) with a
small backoff or subscribes to your stop-cache ready event), then call
setPopularSuggestions and setHistorySuggestions again with the resolved stop
objects (update the logic around getRoutes/lookupStop/popular construction and
the TrainStorageService.getTripHistory handling to use the async-resolved stops
instead of assuming lookupStop immediately returns). Ensure useEffect either
depends on a cache-ready signal or internally re-runs its lookup logic so
popular/history suggestions become actionable once lookupStop values are
available.
In `@apps/mobile/components/ui/TrainDetailModal.tsx`:
- Around line 186-218: The effect currently exits early when lookupStop(toCode)
returns undefined and never re-runs when the stop cache later hydrates; update
the useEffect that houses fetchWeather (the effect watching toCode and tempUnit)
to also depend on the stop-cache hydration signal so it re-runs after the cache
populates — e.g., read the store flag that indicates stops have hydrated
(commonly named isStopsHydrated / stopsLoaded / stopsHydrated) and add it to the
dependency array, or include the resolved destStop value in deps; ensure you
still guard on toCode, call lookupStop(toCode) inside the effect (as in
fetchWeather), and only return early when toCode is falsy so fetchWeather runs
once the cache is hydrated and then sets weatherData via setWeatherData.
- Around line 221-268: The effect currently sets stopWeatherKeyRef.current
up-front which freezes out stops that were uncached at that moment; instead
defer or make the key reflect cache availability: remove the early assignment of
stopWeatherKeyRef.current in the useEffect and only set it after
fetchAllStopWeather finishes successfully (inside fetchAllStopWeather after
Promise.all and before setStopWeather) or compute the key to include a
cached-count marker (e.g. count of lookupStop(...) hits) so that when
lookupStop(cache) later fills the key changes and the effect reruns; locate
symbols: stopWeatherKeyRef.current, lookupStop, fetchAllStopWeather,
setStopWeather and update the logic so the ref is assigned only after verifying
current cache state (or includes cached-count) to avoid permanently skipping
uncached stops.
---
Nitpick comments:
In `@apps/mobile/__tests__/utils/train-helpers.test.ts`:
- Around line 44-49: The tests currently exercise the underscore-parsing
fallback (via extractTrainNumber) because getCachedTrip isn't mocked for
'2026-03-10_AMTK_156' and '2026-01-16_AMTK_543'; update the tests in
train-helpers.test.ts to explicitly assert that prefetchTrip was invoked on
cache miss by mocking getCachedTrip to return undefined for those trip IDs (or
spying on getCachedTrip/prefetchTrip) and adding
expect(prefetchTrip).toHaveBeenCalledWith(...) (or equivalent) after calling
extractTrainNumber so the test verifies prefetchTrip is triggered when
getCachedTrip misses while still covering the fallback parsing logic.
In `@apps/mobile/hooks/useTravelOverlay.ts`:
- Around line 98-102: The async IIFE that calls
TrainStorageService.getTripHistory() and resolveTravelOverlay(profileYear) can
throw and should be wrapped in a try-catch: inside the IIFE call
TrainStorageService.getTripHistory(), assign tripHistoryRef.current and call
resolveTravelOverlay(profileYear) in a try block, and in the catch block
handle/log the error (e.g., use console.error or the app logger) and set a safe
fallback for tripHistoryRef.current (like an empty array) or call the error
handling path so the hook doesn’t produce an unhandled rejection; keep
references to TrainStorageService.getTripHistory, tripHistoryRef.current, and
resolveTravelOverlay to locate the code.
In `@apps/mobile/hooks/useTripDetail.ts`:
- Around line 128-138: In useTripDetail, the delaysPromise async IIFE that calls
getRunStops logs non-404 errors at debug level; change the logger call in that
catch branch to logger.warn and include the error object so unexpected failures
are more visible (keep the existing 404 check and the return { rows: [],
available: false } behavior). Target the delaysPromise block and update the
logger.debug('[useTripDetail] run stops unavailable', err) to use logger.warn
with the same message and error details.
In `@apps/mobile/services/api-client.ts`:
- Around line 355-362: The catch in function notifyAfter silently swallows all
errors; change the .catch handler on the Promise returned by notifyAfter to log
the error (e.g., console.error or the module's logger) along with context
(mention notifyAfter, cacheVersion and affected operation) while preserving the
current behavior of not caching/rethrowing so later attempts can retry; update
the catch block associated with notifyAfter (which references cacheVersion and
cacheListeners) to emit that diagnostic information.
In `@apps/mobile/utils/api-stop-cache.ts`:
- Around line 24-28: The function splitNamespaced is duplicated (same logic in
splitNamespaced and also implemented inside useTravelOverlay), so extract it to
a shared utility (e.g., utils/splitNamespaced.ts) and export it; then replace
the local implementations with an import of the shared splitNamespaced. Update
all call sites (including the function named splitNamespaced in api-stop-cache
and the implementation inside useTravelOverlay) to use the shared exported
function and remove the duplicate definitions to prevent drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0b12dbb-46cc-40fc-869c-fa9d434de328
📒 Files selected for processing (31)
README.mdapps/mobile/__tests__/utils/train-helpers.test.tsapps/mobile/app.config.tsapps/mobile/assets/apple-dark-style.jsonapps/mobile/assets/apple-light-style.jsonapps/mobile/components/TrainCardContent.tsxapps/mobile/components/TwoStationSearch.tsxapps/mobile/components/ui/DepartureBoardModal.tsxapps/mobile/components/ui/TrainDetailModal.tsxapps/mobile/components/ui/train-detail/DepartureArrivalBoard.tsxapps/mobile/constants/config.tsapps/mobile/context/GTFSRefreshContext.tsxapps/mobile/hooks/useApiCache.tsapps/mobile/hooks/useShapes.tsapps/mobile/hooks/useStations.tsapps/mobile/hooks/useTravelOverlay.tsapps/mobile/hooks/useTripDetail.tsapps/mobile/package.jsonapps/mobile/screens/MapScreen.tsxapps/mobile/services/api-client.tsapps/mobile/services/calendar-sync.tsapps/mobile/services/gtfs-sync.tsapps/mobile/services/realtime.tsapps/mobile/services/shape-loader.tsapps/mobile/services/station-loader.tsapps/mobile/types/api.tsapps/mobile/utils/api-stop-cache.tsapps/mobile/utils/gtfs-parser.tsapps/mobile/utils/timezone.tsapps/mobile/utils/train-display.tsapps/mobile/utils/train-helpers.ts
💤 Files with no reviewable changes (6)
- apps/mobile/hooks/useShapes.ts
- apps/mobile/services/realtime.ts
- apps/mobile/utils/gtfs-parser.ts
- apps/mobile/hooks/useStations.ts
- apps/mobile/services/gtfs-sync.ts
- apps/mobile/services/shape-loader.ts
✅ Files skipped from review due to trivial changes (2)
- apps/mobile/constants/config.ts
- apps/mobile/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/mobile/app.config.ts
- apps/mobile/assets/apple-dark-style.json
- apps/mobile/context/GTFSRefreshContext.tsx
- apps/mobile/assets/apple-light-style.json
- apps/mobile/types/api.ts
- apps/mobile/screens/MapScreen.tsx
Comment on lines
+65
to
90
| const DISABLED_RESULT: SyncResult = { | ||
| parsed: 0, | ||
| matched: 0, | ||
| added: 0, | ||
| skipped: 0, | ||
| addedTrips: [], | ||
| totalCalendarEvents: 0, | ||
| failReason: 'gtfs_not_loaded', | ||
| }; | ||
|
|
||
| export async function syncPastTrips( | ||
| _calendarIds: string[], | ||
| _scanDays: number, | ||
| _matchGtfs: boolean = false, | ||
| ): Promise<SyncResult> { | ||
| logger.info('Calendar sync (past): disabled during GTFS → API migration'); | ||
| return DISABLED_RESULT; | ||
| } | ||
|
|
||
| export async function syncFutureTrips( | ||
| _calendarIds: string[], | ||
| _matchGtfs: boolean = false, | ||
| ): Promise<SyncResult> { | ||
| logger.info('Calendar sync (future): disabled during GTFS → API migration'); | ||
| return DISABLED_RESULT; | ||
| } |
Contributor
There was a problem hiding this comment.
Return a fresh SyncResult per call (avoid shared mutable state).
Line 81 and Line 89 currently return the same DISABLED_RESULT object reference. If any consumer mutates addedTrips (or stores and reuses this in React state), that shared reference can leak state across sync attempts and cause stale UI updates.
Proposed fix
-const DISABLED_RESULT: SyncResult = {
- parsed: 0,
- matched: 0,
- added: 0,
- skipped: 0,
- addedTrips: [],
- totalCalendarEvents: 0,
- failReason: 'gtfs_not_loaded',
-};
+const createDisabledResult = (): SyncResult => ({
+ parsed: 0,
+ matched: 0,
+ added: 0,
+ skipped: 0,
+ addedTrips: [],
+ totalCalendarEvents: 0,
+ failReason: 'gtfs_not_loaded',
+});
@@
export async function syncPastTrips(
@@
): Promise<SyncResult> {
logger.info('Calendar sync (past): disabled during GTFS → API migration');
- return DISABLED_RESULT;
+ return createDisabledResult();
}
@@
export async function syncFutureTrips(
@@
): Promise<SyncResult> {
logger.info('Calendar sync (future): disabled during GTFS → API migration');
- return DISABLED_RESULT;
+ return createDisabledResult();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mobile/services/calendar-sync.ts` around lines 65 - 90, The
DISABLED_RESULT constant is being returned by reference from syncPastTrips and
syncFutureTrips which can leak mutable state (notably addedTrips) across
callers; change both functions to return a fresh SyncResult object per call
(e.g., construct a new object that copies the fields from DISABLED_RESULT but
uses a new array for addedTrips) instead of returning the DISABLED_RESULT
reference so consumers cannot mutate shared state.
Comment on lines
+24
to
+35
| export const stationLoader = { | ||
| getStationByCode(code: string): StationBounds | undefined { | ||
| const stop = lookupStop(code); | ||
| if (!stop) return undefined; | ||
| return { | ||
| totalStations: this.stations.size, | ||
| id: stop.stop_id, | ||
| name: stop.stop_name, | ||
| lat: stop.stop_lat, | ||
| lon: stop.stop_lon, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Look up a station by its stop_id / code | ||
| */ | ||
| getStationByCode(code: string): StationBounds | undefined { | ||
| return this.stations.get(code); | ||
| } | ||
|
|
||
| /** | ||
| * Clear all data | ||
| */ | ||
| clear(): void { | ||
| this.stations.clear(); | ||
| } | ||
| } | ||
|
|
||
| // Export singleton instance | ||
| export const stationLoader = new StationLoader(); | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
This no longer preserves the old loader contract for service callers.
The previous loader was eager; this facade is lazy. On a cold miss, getStationByCode() now returns undefined and only fills the cache later, but callers like notifications/storage do not re-render to observe that later fill. That means station resolution can silently fail after startup where the old loader succeeded. Please prewarm the cache before these call sites, or make this API async so callers must await the fetch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mobile/services/station-loader.ts` around lines 24 - 35, The current
stationLoader.getStationByCode returns undefined on a cold miss because
lookupStop is lazy; update the contract so callers don't get silent failures by
either (A) prewarming the cache at startup (call the existing lookup/fetch logic
once and populate whatever cache lookupStop reads before notifications/storage
run) or (B) convert stationLoader.getStationByCode into an async method that
awaits the fetch/populate routine and then returns a StationBounds (i.e., change
getStationByCode to async, await the underlying fetch/populate used by
lookupStop, and have callers (notifications/storage) await it). Locate and
update stationLoader.getStationByCode and the cache-population routine used by
lookupStop (and all call sites of getStationByCode) to follow the chosen
approach so resolution is guaranteed on first call.
* Update map styles to Apple Maps-inspired light and dark themes Replace OpenFreeMap liberty styles with custom Apple Maps-inspired color palettes for both light and dark modes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update highway colors from gold to grey in light and dark map styles Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Jason Xu <jasonxu@macbookair.lan> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Jason Xu <jasonxu@Jasons-MacBook-Air.local> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Mr-Technician <26885142+Mr-Technician@users.noreply.github.com>
- Added a new `ids` package to handle encoding and decoding of typed global IDs. - Updated the `Processor` to publish messages using operator IDs (e.g., `o-amtrak`) instead of provider names. - Modified the WebSocket handler to support subscriptions based on typed global IDs. - Refactored API routes to utilize typed global IDs for providers, stops, routes, and trips. - Enhanced tests to cover new encoding/decoding logic and ensure proper functionality with the updated ID structure. - Introduced a new `Hub` struct to manage stops and hubs, with a clear distinction in the API responses.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/gtfs/static.go`:
- Line 400: Replace the panicking ids.MustEncode calls with the error-returning
ids.Encode and propagate a contextual parse error instead of crashing: in the
route parsing assignment that sets RouteID (and similarly in parseStops for
stop_id, parseTrips for trip_id, and parseStopTimes for route_id/stop_id), call
ids.Encode(ids.Kind..., providerID, rawID), check the returned error, and return
a descriptive parse error that includes the providerID, the raw ID value and the
CSV row/context (e.g. function parseRoutes/parseStops/parseTrips/parseStopTimes)
so the caller can handle malformed feed IDs instead of panicking.
In `@apps/api/realtime/process.go`:
- Around line 35-37: The code uses ids.MustEncode(...) with untrusted
snap.ProviderID which can panic; replace the call to ids.MustEncode with
ids.Encode and handle the returned error: call ids.Encode(ids.KindOperator,
snap.ProviderID, ""), check the error, and propagate or log/return an
appropriate error from the realtime ingest path (same error-propagation style
used elsewhere in this file) instead of allowing a panic; update the local
variable (topic) assignment to be set from the encoder result once error
handling succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f93341fd-a7bc-424b-ba34-b751861df5ad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
README.mdapps/api/db/schema.sqlapps/api/db/static_read.goapps/api/gtfs/realtime.goapps/api/gtfs/static.goapps/api/ids/ids.goapps/api/ids/ids_test.goapps/api/realtime/process.goapps/api/realtime/process_test.goapps/api/routes/ingest_test.goapps/api/routes/routes.goapps/api/routes/static.goapps/api/spec/static.goapps/api/ws/handler.goapps/api/ws/hub.goapps/mobile/assets/apple-dark-style.jsonapps/mobile/assets/apple-light-style.json
✅ Files skipped from review due to trivial changes (2)
- apps/api/db/schema.sql
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mobile/assets/apple-light-style.json
- apps/mobile/assets/apple-dark-style.json
| out = append(out, spec.Route{ | ||
| ProviderID: providerID, | ||
| RouteID: providerID + ":" + r["route_id"], | ||
| RouteID: ids.MustEncode(ids.KindRoute, providerID, r["route_id"]), |
Contributor
There was a problem hiding this comment.
Replace ids.MustEncode in feed parsing with error-returning ids.Encode.
Line 400, Line 435, Line 456, and Line 481 currently panic on malformed feed IDs. Since GTFS input is external, this should return contextual parse errors, not crash the process.
Suggested change pattern
- RouteID: ids.MustEncode(ids.KindRoute, providerID, r["route_id"]),
+ routeID, err := ids.Encode(ids.KindRoute, providerID, r["route_id"])
+ if err != nil {
+ return nil, fmt.Errorf("gtfs [%s]: routes.txt: invalid route_id %q: %w", providerID, r["route_id"], err)
+ }
+ RouteID: routeID,Apply the same pattern for stop_id, trip_id, and route_id in parseStops, parseTrips, and parseStopTimes.
Also applies to: 435-435, 456-457, 481-482
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/gtfs/static.go` at line 400, Replace the panicking ids.MustEncode
calls with the error-returning ids.Encode and propagate a contextual parse error
instead of crashing: in the route parsing assignment that sets RouteID (and
similarly in parseStops for stop_id, parseTrips for trip_id, and parseStopTimes
for route_id/stop_id), call ids.Encode(ids.Kind..., providerID, rawID), check
the returned error, and return a descriptive parse error that includes the
providerID, the raw ID value and the CSV row/context (e.g. function
parseRoutes/parseStops/parseTrips/parseStopTimes) so the caller can handle
malformed feed IDs instead of panicking.
Comment on lines
+35
to
+37
| // Topic is the operator's typed global id (o-<provider>) so that future | ||
| // versions can also publish to route/trip/vehicle topics without renaming. | ||
| topic := ids.MustEncode(ids.KindOperator, snap.ProviderID, "") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify ids API surface and panic-style helper usage.
fd -i '^ids\.go$' -x sh -c 'echo "== {} =="; rg -n "func\\s+(Encode|MustEncode)\\s*\\(" "{}" -C2'
# Inspect all MustEncode callsites to confirm whether inputs are trusted.
rg -n --type go '\bMustEncode\s*\(' -C2Repository: RailForLess/tracky
Length of output: 4316
🏁 Script executed:
# Check the function containing line 37 and understand snap.ProviderID
cd apps/api/realtime
head -50 process.go | tail -25
# More context around line 37
sed -n '1,60p' process.go | cat -nRepository: RailForLess/tracky
Length of output: 3330
Use non-panicking ids.Encode in realtime ingest path.
Line 37 uses ids.MustEncode(...) on snap.ProviderID, which comes from ingest sources (HTTP /ingest handler and R2 backfill). The MustEncode function is documented as "for tests and constants only"—not for untrusted input. A malformed provider ID will panic here and crash the entire realtime processor.
Use ids.Encode with error handling instead, consistent with the function's existing error propagation pattern:
Suggested fix
- // Topic is the operator's typed global id (o-<provider>) so that future
- // versions can also publish to route/trip/vehicle topics without renaming.
- topic := ids.MustEncode(ids.KindOperator, snap.ProviderID, "")
+ // Topic is the operator's typed global id (o-<provider>) so that future
+ // versions can also publish to route/trip/vehicle topics without renaming.
+ topic, err := ids.Encode(ids.KindOperator, snap.ProviderID, "")
+ if err != nil {
+ return fmt.Errorf("realtime: invalid provider id %q: %w", snap.ProviderID, err)
+ }
payload, err := json.Marshal(ws.RealtimeUpdate{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Topic is the operator's typed global id (o-<provider>) so that future | |
| // versions can also publish to route/trip/vehicle topics without renaming. | |
| topic := ids.MustEncode(ids.KindOperator, snap.ProviderID, "") | |
| // Topic is the operator's typed global id (o-<provider>) so that future | |
| // versions can also publish to route/trip/vehicle topics without renaming. | |
| topic, err := ids.Encode(ids.KindOperator, snap.ProviderID, "") | |
| if err != nil { | |
| return fmt.Errorf("realtime: invalid provider id %q: %w", snap.ProviderID, err) | |
| } | |
| payload, err := json.Marshal(ws.RealtimeUpdate{ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/realtime/process.go` around lines 35 - 37, The code uses
ids.MustEncode(...) with untrusted snap.ProviderID which can panic; replace the
call to ids.MustEncode with ids.Encode and handle the returned error: call
ids.Encode(ids.KindOperator, snap.ProviderID, ""), check the error, and
propagate or log/return an appropriate error from the realtime ingest path (same
error-propagation style used elsewhere in this file) instead of allowing a
panic; update the local variable (topic) assignment to be set from the encoder
result once error handling succeeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to the mobile app's realtime data handling and map infrastructure. It adds a new
RealtimeContextfor managing live train position updates via WebSocket, replaces polling-based hooks with context-driven ones, and introduces a flexible provider system for map tiles and data sources. Additionally, configuration and provider information are now centralized, and legacy or unnecessary code has been removed or simplified.Key changes include:
Realtime Data Architecture:
RealtimeContext(RealtimeProvideranduseRealtimePositions) to manage live train positions via WebSocket, enabling efficient, context-driven updates across the app. Hooks likeuseLiveTrainsanduseRealtimenow consume this context instead of polling, improving performance and code clarity. [1] [2] [3] [4] [5]Map Tiles and Provider System:
ProviderTilescomponent to render vector map tiles for different transit providers, supporting interactive station and route selection.Providertype and aPROVIDERSconstant list, centralizing provider metadata such as IDs, display names, and zoom levels.Configuration Management:
constants/config.ts, reading API, WebSocket, and tiles URLs from Expo config and environment variables, with sensible defaults. [1] [2] [3]Hook and Service Simplification:
useFrequentlyUsedhook to return a static set of frequently used items instead of dynamically loading all routes and stops, reflecting backend and architecture changes. [1] [2]GTFSRefreshContextto remove unnecessary dependencies and streamline initialization logic. [1] [2] [3]These changes collectively modernize the app’s realtime and mapping infrastructure, improve maintainability, and lay the groundwork for future enhancements.
Summary by CodeRabbit
New Features
Improvements
Chores