Directions drawer: expandable legs, tap-to-frame, and a directions→route focus#1961
Merged
Conversation
…vrons Two UX tweaks to the directions drawer's option-card picker: - Fold the picker into the directions LazyColumn as its first item so it scrolls out of sight as you read down the steps, instead of staying pinned above them. - Flank the horizontally-scrollable picker with the ETA strip's overflow chevrons so options hanging off either edge are discoverable and one tap jumps a viewport. Extracted EtaStrip's private ScrollChevronGutter into a shared ui/compose/components composable and reused it in both places.
Restructure the directions list so each itinerary leg is a single card instead of a flat step list (transit legs previously split into separate board/alight rows). Tapping a card body frames the whole leg on the map; a distinct expand button reveals the leg's steps (turn-by-turn for a walk leg; intermediate stops + alight for a transit leg), each of which recenters on its own point. - Re-group the generator's flat, leg-ordered Direction list into one card per leg via a pure, JVM-tested DirectionCardGrouping.groupByLeg. - Extend DirectionItem with legPoints (decoded leg polyline, for framing) and routeLeg identity (routeId + board/alight stop, threaded now for the upcoming route-focus phase). Add TripPlace.stopId from OTP2 stop.gtfsId. - Frame a leg via a new FocusItineraryLeg map directive -> MapViewModel.focusItineraryLeg -> FramingIntent.Points (padding-aware, no per-flavor code).
…olution)
Tapping a transit leg card now drills into a route-subordinate-to-directions
focus: the map recontextualizes onto that leg's route (anchored to the
boarding stop's direction), and a map-background tap or Back returns to the
itinerary overview. CurrentFocus.Directions gains an optional routeFocus
sub-state carrying that; the shown itinerary is cached so the overview
redraws when the route sub-focus is popped.
OTP and OBA use different id namespaces (e.g. Puget Sound OTP `kcm:102574`
vs OBA `1_102574`), so a client-side OtpObaIdResolver maps them: OBA id =
`{agency}_{entity}`, where the entity number is taken from the OTP id and the
agency is derived from OTP's agency.gtfsId suffix, verified against
agencies-with-coverage, and matched by agency name on a miss (covers feeds
whose GTFS agency_id diverges from OBA's). Unresolvable legs degrade to plain
leg framing. Added agency.gtfsId to the OTP query; threaded agency id/name
through TripLeg -> RouteLegRef. Resolution is async on tap in HomeViewModel.
While a transit leg's route is in focus, swap the directions step list for the departing stop's live arrivals board over the highlighted route, with a back control (and map-background tap) returning to the itinerary overview. DirectionsRouteFocus now carries a FocusedStop for the departing stop (built from the resolved OBA stop id + the leg's board-stop name/code/point). A new DirectionsRouteFocusSheet reuses the same per-stop arrivals session + panel as the home focused-stop drawer (rememberArrivalsSession / ArrivalsSheetHost), keyed to that stop, so it polls and renders identically; HomeScreen shows it in place of the results sheet whenever routeFocus.boardStop is set. Route-row taps in this board are inert for now.
While a transit leg's route is in focus, overlay the leg's own polyline (the exact board→alight path from the OTP leg geometry) as a markedly thicker line (2.4x vs the route's 1.5x) in the route's colour, so the portion the user rides stands out along the full route. Threaded via ShowRouteRequest.highlightedSegment -> MapViewModel.enterRoute -> RouteMapController, which overlays it last in publishMapPresentation (so it sits on top and survives re-publishes from vehicle polls / direction changes) and resets it on stop(). New HIGHLIGHTED_SEGMENT_WIDTH_PROFILE in DetailZoomRamp.
Expanding a transit leg now shows, under each of its Get on / Get off rows, that stop's live ETA strip — the same scrollable pill component as the arrivals drawer. Tapping a pill focuses/animates/pings that trip's live vehicle by riding the exact arrivals path (the shared row callbacks' onFocusVehicleOnMap → a focusTripId ShowRouteRequest → focusDirectionsRouteVehicle → RouteMapController), adding the leg's traveled segment; long-press opens the trip menu. The leg row body still highlights the route on the map. Replaces the earlier route-focus arrivals board (DirectionsRouteFocusSheet), which is removed. OTP→OBA id resolution moves to build time: TripResultsRepository resolves each transit leg's route/stop ids via OtpObaIdResolver so RouteLegRef/RouteStopRef carry OBA ids synchronously (agency fields dropped, headsign added); the pure grouping takes pre-resolved refs; HomeViewModel's route-leg focus is now sync.
The trip planner formatted arrival/departure times against the agency's GMT offset (ConversionUtils' agencyWallClock + TripLeg.agencyTimeZoneOffset), so a trip in another agency's zone rendered in that zone rather than on the phone's clock. Format against ZoneId.systemDefault() / the device-default TimeZone instead, and drop the now-unused agencyTimeZoneOffset field and its adapter/test plumbing.
Tapping a transit leg recontextualizes the map onto just the ridden board→alight span: the full route is de-emphasized, the segment is drawn at normal weight on top, only the segment's stops are kept, and the camera frames the segment rather than the whole route. The route is narrowed to the ridden direction via the boarding stop, so only the relevant direction shows. Extract the pure segment geometry (routePolylinesWithSegment / onSegment / isDrawableSegment) into RouteSegmentHighlight with JVM tests so it stays out of RouteMapController's state plumbing, and hold the originating ShowRouteRequest directly in DirectionsRouteFocus so a back-press replays the focus faithfully (including a followed vehicle's focusTripId).
Board/Alight rows become their own tap targets (titleMedium labels that zoom to the stop), the inline ETA strip always shows while a transit leg is expanded, and the expand chevrons are 1.75x larger and tinted like the row icons. Thread the leg's OBA route/stop ids and points for route focus through the shared geoPointOrNull helper and TripLegGeometry.decodedPoints().
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughOTP2 GraphQL and adapter mappings now fetch and populate agency GTFS IDs and stop IDs; these flow into domain models with new optional fields and are resolved to OBA identifiers by a client-side resolver. Leg geometry is decoded on demand, grouped into leg-level direction cards with transit references, and used for route highlighting and map framing. Time formatting switched to device-local wall-clock calculations without agency offsets. Directions focus state carries optional route sub-focus for restoration. Trip-results UI was refactored to embed the picker in the list, split expand button from leg-body click, show Board/Alight stops with inline ETA strips, and add scrollable option navigation. A shared ChangesDirections leg focus and transit identity
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (2)
onebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsRepository.kt (1)
99-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize per-leg route/stop resolution.
itinerary.legs.map { resolveRouteLeg(leg) }resolves each leg sequentially, andresolveRouteLegitself awaitsobaRouteId, thenboard, thenalightone after another (3 sequential suspend calls per leg). For a multi-leg itinerary this serializes what are described as network-backed lookups, adding up latency each time a user opens directions for an option.♻️ Suggested parallelization
- val flat = DirectionsGenerator(itinerary.legs, context).directions - val routeLegRefs = itinerary.legs.map { leg -> - if (leg.mode?.isOnStreetNonTransit == true) null else resolveRouteLeg(leg) - } - DirectionCardGrouping.groupByLeg(itinerary.legs, flat, routeLegRefs) + val flat = DirectionsGenerator(itinerary.legs, context).directions + val routeLegRefs = coroutineScope { + itinerary.legs.map { leg -> + async { + if (leg.mode?.isOnStreetNonTransit == true) null else resolveRouteLeg(leg) + } + }.awaitAll() + } + DirectionCardGrouping.groupByLeg(itinerary.legs, flat, routeLegRefs)🤖 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/tripresults/TripResultsRepository.kt` around lines 99 - 124, Parallelize the network-backed lookups in the directions flow: update the per-leg mapping around routeLegRefs to resolve transit legs concurrently, and update resolveRouteLeg so obaRouteId, board.resolveStop, and alight.resolveStop run concurrently before constructing RouteLegRef. Preserve null entries for non-transit legs and the existing output order.onebusaway-android/src/main/java/org/onebusaway/android/ui/home/directions/DirectionsFeature.kt (1)
336-388: 🚀 Performance & Scalability | 🔵 TrivialConcurrent arrivals polling from expanded legs. Each expanded transit leg mounts two
rememberArrivalsSessioninstances (Board + Alight), each polling independently. With several legs expanded at once this fans out into multiple concurrent arrivals polls against the API. LazyColumn disposal caps this to on-screen legs, so it's bounded in practice, but consider collapsing sibling legs on expand (accordion behavior) or de-duplicating polls per route to reduce load if telemetry shows many legs expanded together.🤖 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/directions/DirectionsFeature.kt` around lines 336 - 388, Limit expanded transit legs to one at a time so expanding a leg collapses its siblings before DirectionStopEtaStrip mounts. Update the expansion-state owner rather than changing rememberArrivalsSession, preserving the existing Board/Alight behavior for the active leg while preventing multiple legs from polling concurrently.
🤖 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/directions/util/ConversionUtils.kt`:
- Around line 179-183: Update getTimeWithContext and getTimeUpdated to accept
today as a parameter and remove their internal LocalDate.now(zone) calls. Thread
the supplied today value through the existing today/tomorrow branching and
preserve the current zone-based behavior.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.kt`:
- Around line 288-291: Update RouteMapController.reframe so it assigns the
controller’s highlightedSegment from request.highlightedSegment before applying
direction, focus-trip, or framing behavior. Preserve the existing reframe
branching while ensuring empty or changed request values replace any stale
highlighted segment used by publishMapPresentation and showDirectionStops.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/directions/DirectionsFeature.kt`:
- Around line 336-388: Limit expanded transit legs to one at a time so expanding
a leg collapses its siblings before DirectionStopEtaStrip mounts. Update the
expansion-state owner rather than changing rememberArrivalsSession, preserving
the existing Board/Alight behavior for the active leg while preventing multiple
legs from polling concurrently.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsRepository.kt`:
- Around line 99-124: Parallelize the network-backed lookups in the directions
flow: update the per-leg mapping around routeLegRefs to resolve transit legs
concurrently, and update resolveRouteLeg so obaRouteId, board.resolveStop, and
alight.resolveStop run concurrently before constructing RouteLegRef. Preserve
null entries for non-transit legs and the existing output order.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a1450c5-e3c7-4f81-9d07-8d04dfa98a37
📒 Files selected for processing (37)
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/tripresults/DirectionRowFocusTest.ktonebusaway-android/src/main/graphql/otp2/Plan.graphqlonebusaway-android/src/main/java/org/onebusaway/android/api/adapters/Otp2PlanAdapters.ktonebusaway-android/src/main/java/org/onebusaway/android/api/adapters/TripPlanAdapters.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/OtpObaIdResolver.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/model/TripItinerary.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/util/ConversionUtils.ktonebusaway-android/src/main/java/org/onebusaway/android/directions/util/DirectionsGenerator.ktonebusaway-android/src/main/java/org/onebusaway/android/map/DirectionsMapController.ktonebusaway-android/src/main/java/org/onebusaway/android/map/MapHost.ktonebusaway-android/src/main/java/org/onebusaway/android/map/MapViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/map/RouteMapController.ktonebusaway-android/src/main/java/org/onebusaway/android/map/RouteSegmentHighlight.ktonebusaway-android/src/main/java/org/onebusaway/android/map/ShowRouteRequest.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/EtaStrip.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/compose/components/ScrollChevronGutter.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/CurrentFocus.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/CurrentFocusPersistence.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/directions/DirectionsFeature.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/MapFeature.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/DirectionCardGrouping.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsRepository.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsUiState.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/util/GeoPoint.ktonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/test/java/org/onebusaway/android/api/Otp2PlanDecodeTest.ktonebusaway-android/src/test/java/org/onebusaway/android/api/OtpPlanDecodeTest.ktonebusaway-android/src/test/java/org/onebusaway/android/directions/OtpObaIdResolverTest.ktonebusaway-android/src/test/java/org/onebusaway/android/directions/util/ConversionUtilsTest.ktonebusaway-android/src/test/java/org/onebusaway/android/map/RouteSegmentHighlightTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeViewModelTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/tripresults/DirectionCardGroupingTest.kt
💤 Files with no reviewable changes (2)
- onebusaway-android/src/test/java/org/onebusaway/android/api/OtpPlanDecodeTest.kt
- onebusaway-android/src/main/java/org/onebusaway/android/api/adapters/TripPlanAdapters.kt
Comment on lines
+179
to
+183
| val zone = ZoneId.systemDefault() | ||
| val local = wallClock(time, zone) | ||
| val displayDate = Date.from(local.toInstant()) | ||
| val localDay = local.toLocalDate() | ||
| val today = LocalDate.now(zone) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Read today from a parameter, not the clock, to keep these helpers pure.
getTimeWithContext and getTimeUpdated both call LocalDate.now(zone) internally for their today/tomorrow branching, while this same refactor extracted wallClock/isToday/isTomorrow as pure, parameterized (and directly unit-tested) helpers. These two entry points remain the one place still reading the clock inside the helper, and so remain untestable for their day-boundary branches.
♻️ Thread `today` through as a parameter
fun getTimeWithContext(
applicationContext: Context,
time: Long,
inLine: Boolean,
color: Int = -1,
+ today: LocalDate = LocalDate.now(ZoneId.systemDefault()),
): CharSequence {
...
val zone = ZoneId.systemDefault()
val local = wallClock(time, zone)
val displayDate = Date.from(local.toInstant())
val localDay = local.toLocalDate()
- val today = LocalDate.now(zone)Apply the same pattern to getTimeUpdated's val today = LocalDate.now(zone) at line 245.
As per path instructions, **/*.{kt,java}: "Keep ETA and active-window helpers pure by passing the current time as a parameter instead of reading the clock inside the helper."
Also applies to: 240-245
🤖 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/directions/util/ConversionUtils.kt`
around lines 179 - 183, Update getTimeWithContext and getTimeUpdated to accept
today as a parameter and remove their internal LocalDate.now(zone) calls. Thread
the supplied today value through the existing today/tomorrow branching and
preserve the current zone-based behavior.
Source: Path instructions
Resolve conflicts in DirectionsFeature.kt (keep both imports) and TripResultsScreen.kt: adopt main's vendored AppIcons for the leg expand/collapse chevron (main dropped the material-icons-core dependency), keeping the PR's expandable board/alight card structure.
- RouteMapController.reframe(): update highlightedSegment from the request when it changes (re-emphasize polyline + re-filter stops), so tapping a different leg of the already-shown route doesn't keep the stale segment. start() already did this unconditionally; reframe() was the gap. - ConversionUtils.getTimeWithContext/getTimeUpdated: take 'today' as a defaulted parameter instead of reading LocalDate.now() inside, matching the pure-helper rule the wallClock/isToday/isTomorrow extraction follows.
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.
What this does
Reworks the trip-planner directions drawer to make the map a first-class part of reading directions, building on the sheet rework from #1951.
Commits (phased, drawer-first)
RouteSegmentHighlightOTP → OBA id resolution
OTP ids (
kcm:102574) and OBA ids (1_102574) use different schemes, so tapping a leg's route needs a mapping.OtpObaIdResolverderives it client-side and deterministically — feed id → agency id from theagency.gtfsIdsuffix, verified against agencies-with-coverage, with a name-based fallback — so no human-maintained OTP↔OBA directory is required for the Puget Sound (OTP2) region today. A proposed upstream extension to the OTP regions directory would make this explicit later.Per CLAUDE.md's "no unsanctioned heuristics" rule, calling this out for a human OK before merge:
SEGMENT_STOP_TOLERANCE_METERS = 50.0(RouteSegmentHighlight.onSegment): OTP2 does not return a transit leg's intermediate stop ids, so "which stops are on the ridden segment" can't be answered by exact id match. Stops are instead kept when they fall within 50 m of the clipped board→alight polyline. 50 m sits well below transit stop spacing, so the nearest off-segment stop isn't wrongly included. Failure mode: a stop within 50 m of the ridden path but not actually served by this leg could be drawn (or, conversely, a served stop set back >50 m from the decoded line dropped). The tolerance is documented at the call site.Testing
-PwarningsAsErrors=true) on bothobaGoogleandobaMaplibre.RouteSegmentHighlightTestand theDirectionCardGrouping/OtpPlanDecode/ConversionUtilssuites.Summary by CodeRabbit
New Features
Bug Fixes