Show every bay's departures at transit-centre zoom (#2107) - #2198
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughAdds nearby-arrivals API models, endpoint loading, settled-viewport polling, route grouping, Compose sheet rendering, map actions, and home-screen integration. Tests cover decoding, grouping, sheet behavior, endpoint classification, polling, failures, and region changes. ChangesNearby arrivals
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MapFeature
participant NearbyArrivalsViewModel
participant NearbyArrivalsDataSource
participant HomeScreen
MapFeature->>NearbyArrivalsViewModel: send settled viewport and stop band
NearbyArrivalsViewModel->>NearbyArrivalsDataSource: request nearby arrivals
NearbyArrivalsDataSource-->>NearbyArrivalsViewModel: publish loaded or unavailable state
NearbyArrivalsViewModel-->>HomeScreen: expose nearby UI state
HomeScreen->>HomeScreen: build and render nearby route rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt (1)
71-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the transit-centre band predicate with the ViewModel.
NearbyArrivalsViewModelapplies the same rule at its gate (band >= StopBand.ROUTES). The query and the sheet decision must agree, or the drawer can gate on a band the query does not serve. Extract one predicate, for exampleStopBand.showsNearbyArrivals, and call it from both places.🤖 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 `@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt` around lines 71 - 80, Extract the shared transit-centre band check from homeSheetContent into a StopBand.showsNearbyArrivals predicate, then replace the direct band >= StopBand.ROUTES checks in both homeSheetContent and NearbyArrivalsViewModel with that predicate so the sheet and query use identical gating.onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt (1)
249-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stop-band derivation is duplicated.
MapFeature.ktlines 311-316 build the same flow frommapViewModel.renderState.snapshotand push it intoNearbyArrivalsViewModel. This screen builds it again for its own sheet decision. Two derivations of one value can drift if the source field changes.One option: expose the band from
NearbyArrivalsViewModel(it already receives it throughonStopBand) and read it here, soMapFeaturestays the single producer.🤖 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 `@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt` around lines 249 - 253, Remove the duplicate stop-band flow derived in HomeScreen around mapViewModel.renderState.snapshot. Expose or reuse the stop-band state already received by NearbyArrivalsViewModel through onStopBand, and update the sheet decision to consume that value while keeping MapFeature as the single producer.onebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsViewModel.kt (1)
118-138: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKey the nearby arrivals query on region as well as viewport.
combine(viewport, band, active)does not includeregionRepo.region, so switching regions with the same viewport keeps the current state inflatMapLatest. AddregionRepo.regionto the combine key; clearlastLoadedand pass the selected region id throughpoll, using the id already available inside the poll loop.🤖 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 `@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsViewModel.kt` around lines 118 - 138, The NearbyArrivalsViewModel state flow currently keys flatMapLatest only on viewport, band, and active, so region changes with the same viewport are ignored. Include regionRepo.region in the combine inputs, clear lastLoaded when the selected region changes or the gate closes, and pass the selected region ID into poll, reusing the region ID already available within the poll loop.
🤖 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
`@onebusaway-android/src/main/java/org/onebusaway/android/api/data/NearbyArrivalsSupport.kt`:
- Around line 44-54: Update NearbyArrivalsSupport.isKnownUnsupported and
recordAbsent to key the unsupported cache by the full Region.sidecarTarget
rather than nullable regionId or sidecarId alone. Include both the resolved
endpoint host and region ID in the key, and update callers to pass the
Region-derived sidecarTarget so 404 state remains isolated per deployment.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.kt`:
- Around line 548-577: Clear the stale pendingFocus latch at the start of
showNearbyRouteOnMap, before pushFocus and the route directive are emitted.
Ensure subsequent onArrivalsLoaded handling cannot consume an unrequested
pending focus and recenter the map.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyRowCallbacks.kt`:
- Around line 49-55: Update the nearby row callback setup to wrap undoViewport
with rememberUpdatedState and have the retained callbacks read that current
state instead of capturing the initial lambda. Add a recomposition test that
replaces undoViewport, then invokes onShowVehiclesOnMap, onEtaClick, and badge
onShowRouteOnMap to verify each uses the updated viewport.
In
`@onebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeSheetLogicTest.kt`:
- Around line 100-105: Update the test `the nearby key is stable while the stop
key is per stop` so the nearby key is captured in separate evaluations and those
two values are compared, rather than comparing the same property access
expression. Preserve the existing stop-key and null assertions.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt`:
- Around line 249-253: Remove the duplicate stop-band flow derived in HomeScreen
around mapViewModel.renderState.snapshot. Expose or reuse the stop-band state
already received by NearbyArrivalsViewModel through onStopBand, and update the
sheet decision to consume that value while keeping MapFeature as the single
producer.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt`:
- Around line 71-80: Extract the shared transit-centre band check from
homeSheetContent into a StopBand.showsNearbyArrivals predicate, then replace the
direct band >= StopBand.ROUTES checks in both homeSheetContent and
NearbyArrivalsViewModel with that predicate so the sheet and query use identical
gating.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsViewModel.kt`:
- Around line 118-138: The NearbyArrivalsViewModel state flow currently keys
flatMapLatest only on viewport, band, and active, so region changes with the
same viewport are ignored. Include regionRepo.region in the combine inputs,
clear lastLoaded when the selected region changes or the gate closes, and pass
the selected region ID into poll, reusing the region ID already available within
the poll loop.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bcb91157-bc5f-4f9b-a54b-30075bab0890
📒 Files selected for processing (23)
onebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaApiModels.ktonebusaway-android/src/main/java/org/onebusaway/android/api/contract/ObaWebService.ktonebusaway-android/src/main/java/org/onebusaway/android/api/data/NearbyArrivalsDataSource.ktonebusaway-android/src/main/java/org/onebusaway/android/api/data/NearbyArrivalsSupport.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/RepositoryModule.ktonebusaway-android/src/main/java/org/onebusaway/android/map/BikeLayerController.ktonebusaway-android/src/main/java/org/onebusaway/android/map/MapDecisions.ktonebusaway-android/src/main/java/org/onebusaway/android/map/StopsMapController.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/RouteRowGroup.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalRows.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/MapFeature.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsSheetHost.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyRouteRows.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/nearby/NearbyRowCallbacks.ktonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/test/java/org/onebusaway/android/api/NearbyArrivalsDecodeTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/arrivals/NearbyRouteGroupingTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeSheetLogicTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/home/nearby/NearbyArrivalsViewModelTest.kt
Item (1) of #2107 put route *names* on each stop marker from zoom 17.5. What a label can't say is *when*. This is item (2): at that same zoom, with nothing focused, the arrivals drawer engages at its peek listing every route leaving every bay in view — so a rider standing in a transit centre reads the whole place at once instead of opening bay after bay. Route-first, not stop-grouped. One row per (route, direction, bay), each row naming where to stand. Grouping by stop would only reproduce the bay-by-bay scan the feature exists to remove. A route+direction boarding from two bays is two rows: merging them would either name both (the same scan) or pick one, which is a claim that can be false on the ground. `arrivals-and-departures-for-location` takes the viewport parameters the map already computes for `stops-for-location` and answers with every arrival at every stop in the box, each carrying its own `stopId`. It is missing from developer.onebusaway.org (docs PR onebusaway-docs#166 is open) but has been in onebusaway-application-modules since 2022 as `ArrivalsAndDeparturesForLocationAction`. Measured at 3rd & Pine, Seattle, in a zoom-17.5-sized box: 8 bays, 40 rows, ~29 KB gzipped. One single-stop request at the same window is 6.8 KB, so this is about half the bytes of an eight-way fan-out and an eighth of the requests — the same cost as the one focused stop the drawer already polls. Two things the endpoint does that the code has to know: - **The empty box answers a different shape.** With stops it is `{entry, references}`; with none it is the raw bean, no `entry` at all (`emptyResponse()` skips `factory.getResponse`). Identical on all four regions that serve it. `entry` is nullable for exactly this reason — otherwise panning onto water throws instead of showing an empty list. - **`maxCount` truncates arrivals, not stops.** At 10 the response covers one bay, not ten; the nearest bay's departures eat the budget and the rest vanish. So it is left at the server default and `minutesAfter` is the size lever, with `limitExceeded` surfaced in the drawer rather than silently dropping bays a rider is looking straight at. Four of the seven regions in the directory serve it (Puget Sound, Tampa, WMATA, Davis); MTA New York, San Diego and Adelaide answer 404. No directory field records this, so the first query is the probe and an explicit HTTP 404 — read off the status line, since San Diego replies with a Tomcat HTML page — is believed. Nothing else: a timeout, a 5xx, or a non-OK OBA envelope code stays transient, because those happen to regions that do serve it. The verdict is in-memory per region id, so a deployment that upgrades is picked up on the next launch instead of being pinned off until a reinstall. - `HomeSheetLogic` grows a three-way `HomeSheetContent` in place of `shouldShowSheet`. No new `CurrentFocus` variant: that type is the map's *subject*, persisted and undoable, and this list is what shows when there isn't one. It gates on having rows, so the drawer never opens empty, never opens mid-load, and never opens at all where the server can't answer. - Back collapses an expanded list to peek, but at peek it passes to the system — ambient, with nothing behind it to return to. - A pan updates the list in place. The previous response is held while the new viewport loads; re-emitting Loading would empty the rows, and since the sheet gates on rows that would retract and re-reveal the drawer once per pan. - `settledCamera()` is extracted from the byte-identical debounce + `cameraInteracting` gate that `StopsMapController` and `BikeLayerController` each carried, so all three loaders share one definition of "settled". - ETAs are measured against the response's own `currentTime`, minted to `ServerTime` at the wire boundary (#1612/#1620). Row tap shows that route on the map scoped to the row's bay, in one focus push so one Back returns to the list. The long-press menu's per-stop actions (star, reminder, tracking, report) need the stop-scoped `ArrivalsViewModel` this many-bay list doesn't create; rather than no-op them they focus the row's bay, landing the rider on the panel where they're wired. Worth a follow-up. Not yet exercised on device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four inline findings and three nitpicks from the PR 2198 review.
**The unsupported verdict is keyed by the OBA base URL.** `NearbyArrivalsSupport`
held it under `Region.id`, our own primary key. The verdict is a statement about
the *server* that answered 404, and the region is only how the app reaches one: a
directory refresh can repoint an existing region's `obaBaseUrl` at a different
deployment under the same id, and a deep-link-added custom region is a host the
directory never named. Either would inherit the other's 404 and lose the drawer
where it in fact works. Keyed on the endpoint, an unknown host is simply
un-probed, so a region switch still needs no reset. The one case the key does not
separate — a user-entered custom API URL applied ahead of the region — is stated
at the site rather than left implicit.
**The query is keyed on the region too.** `combine(viewport, band, active)` left
the standing poll pointed at the old host when the rider switched regions without
moving the camera. The endpoint is now a fourth input, distinct on the endpoint
alone so an unrelated region field can't re-fire it. Rows are held across a *pan*
so the drawer updates in place, but rows from another deployment describe another
city, so a region switch drops them and starts from Loading.
**One band predicate.** `StopBand.showsNearbyArrivals` replaces the `>= ROUTES`
comparison that `HomeSheetLogic` and `NearbyArrivalsViewModel` each carried, so
the sheet cannot decide to show a band the query never asked for. `HomeScreen`
likewise stops deriving the band a second time off the render snapshot and reads
back the one `MapFeature` already pushes into the query — one producer.
**A stale `pendingFocus` latch could recenter an unrequested camera.**
`showNearbyRouteOnMap` now clears it first, the way `clearMapFocus` and
`enterDirections` do: a restore that armed the latch and then had its stop
unfocused before arrivals landed leaves it set with no focus — exactly the state
that puts this list on screen — and the tapped bay's own load would consume it.
**`undoViewport` reads through `rememberUpdatedState`**, like `rows` and
`onShowTrip` beside it. Today's call site passes `{ mapViewModel.viewport }`,
which re-reads on each invocation, so no stale viewport is reachable now; the
guard is what keeps that true of a caller whose lambda closes over a value.
Also fixed a tautological assertion in `HomeSheetLogicTest` that compared
`NearbyRoutes.sheetKey` with itself and so held for any implementation.
Rebased onto main. `BikeLayerController` was deleted by #2168's move to
`RentalLayerController`; the `settledCamera()` extraction applies there instead,
and the two doc comments naming the bike loader now name the rental one.
- 1,998 unit tests, 0 failures, both flavors
- `compileObaGoogleDebugKotlin` + `compileObaMaplibreDebugKotlin` with
`-PwarningsAsErrors=true`, and `spotlessCheck` — clean
- 3 new tests: the endpoint-scoped verdict, the region-switch requery (and that a
non-endpoint region field does not requery), and the region-switch row drop
Still not exercised on device.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bmander
force-pushed
the
nearby-arrivals-drawer-2107
branch
from
August 8, 2026 23:49
0aa972b to
a56263f
Compare
Contributor
Author
|
Rebased onto Conflicts. Two, both from #2168's dockless-rentals work landing first:
Review items. All four actionable, plus the three nitpicks:
Testing: 1,998 unit tests, 0 failures, both flavors; both The three items under "Needs a human decision before merge" in the description are unchanged and still open. Still not exercised on device. |
This was referenced Aug 9, 2026
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
1 participant
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.
Closes the second half of #2107. Item (1) (#2136) put route names on each stop
marker from zoom 17.5. What a label can't say is when. At that same zoom, with
nothing focused, the arrivals drawer now engages at its peek listing every route
leaving every bay in view — so a rider standing in a transit centre reads the
whole place at once instead of opening bay after bay.
Route-first, not stop-grouped: one row per (route, direction, bay), each naming
where to stand. Grouping by stop would only reproduce the bay-by-bay scan the
feature exists to remove. A route+direction boarding from two bays is two rows —
merging them would either name both (the same scan) or pick one, which is a claim
that can be false on the ground.
One request per viewport, not one per stop
arrivals-and-departures-for-locationtakes the viewport parameters the mapalready computes for
stops-for-locationand answers with every arrival at everystop in the box, each carrying its own
stopId.It is missing from developer.onebusaway.org — docs PR onebusaway-docs#166
is open to add it — but it has been in onebusaway-application-modules since 2022
as
ArrivalsAndDeparturesForLocationAction,with docs in that repo's own
src/site/markdown/. Worth stating plainly so afuture reader doesn't conclude it was invented.
Measured at 3rd Ave & Pine, Seattle, in a zoom-17.5-sized box: 8 bays, 40 rows,
~29 KB gzipped. One single-stop request at the same window is 6.8 KB — so this
is about half the bytes of an eight-way fan-out and an eighth of the requests,
i.e. the same cost as the one focused stop the drawer already polls at 60s.
Two behaviours found by probing the live servers that the code has to know:
{entry, references}; with none it is the raw bean with noentryat all(
emptyResponse()skipsfactory.getResponse). Identical on all four regionsthat serve it.
entryis nullable for exactly this reason — otherwise panningonto water throws instead of showing an empty list. Both shapes are pinned by
NearbyArrivalsDecodeTest.maxCounttruncates arrivals, not stops. AtmaxCount=10the responsecovered one bay, not ten: the nearest bay's departures eat the budget and the
rest vanish. Using it as a cost lever would silently drop bays the rider is
looking straight at. It is left at the server default;
minutesAfteris thesize lever, and
limitExceededis surfaced in the drawer.Support is discovered, not configured
That is every region in the live directory, probed with the app's own key. No
directory field records this, so the first query is the probe and an explicit
HTTP 404 is believed — read off the status line, since San Diego replies with
a raw Tomcat HTML page no JSON decode could classify. Nothing else counts: a
timeout, a 5xx, or a non-OK OBA envelope code stays transient, because those
happen to regions that do serve it.
The verdict is in-memory per region id, deliberately not persisted: a deployment
that upgrades is then picked up on the next launch rather than pinned off until a
reinstall, at a cost of one wasted request per launch per unsupported region.
The rest
HomeSheetLogicgrows a three-wayHomeSheetContentin place ofshouldShowSheet. No newCurrentFocusvariant — that type is the map'ssubject, persisted and undoable, and this list is precisely what shows when
there isn't one. It gates on having rows, so the drawer never opens empty,
never opens mid-load, and never opens at all where the server can't answer.
since the list is ambient with nothing behind it to return to.
viewport loads — re-emitting
Loadingwould empty the rows, and since thesheet gates on rows that would retract and re-reveal the drawer once per pan.
settledCamera()is extracted from the byte-identical debounce +cameraInteractinggate thatStopsMapControllerandBikeLayerControllereach carried, so all three loaders share one definition of "settled".
currentTime, minted toServerTimeat the wire boundary (Use the server clock (currentTime) as the ETA baseline everywhere; audit the app for time-domain mixing #1612 / Typed time: make clock-domain/unit mixing impossible by construction #1620).Row tap shows that route on the map scoped to the row's bay, in one focus
push so one Back returns straight to the list.
Needs a human decision before merge
minutesAfter = 35is a new product threshold (the starred-stops list'svalue, against the focused-stop default of 65) — ~29 KB vs ~40 KB gzipped, for
departures further out than anyone standing at the stop is waiting for.
CLAUDE.md wants thresholds called out for sign-off; this is that.
report-a-problem each need the stop-scoped
ArrivalsViewModelthat thismany-bay list deliberately does not create. Rather than no-op them they focus
the row's bay, landing the rider on the panel where they are wired. Coherent,
but not what a rider expects from a menu item — a follow-up issue is the
honest fix, and service alerts on rows are deferred with it.
(
PEEK_HEIGHT_FRACTION = 0.30f) is untuned for it — a device-tuning job, theway Name the routes on a stop marker at transit-centre zoom (#2107) #2136 tuned the zoom threshold itself.
Also worth noting: three of seven regions, including MTA New York, get nothing.
That was a deliberate call over carrying a second fan-out data path.
Testing
obaGoogle+obaMaplibre)compileObaGoogleDebugKotlin+compileObaMaplibreDebugKotlinwith-PwarningsAsErrors=true— cleanspotlessCheck— cleanand that row order never depends on ETA; the three-way sheet mode and back
behaviour; and the query's band gating, requery-on-pan, poll cadence,
hold-rows-through-a-pan, and the 404-vs-transient split.
Not yet exercised on device.
🤖 Generated with Claude Code
Summary by CodeRabbit