Unify stop and route focus banners#1897
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe home map overlay now uses a stop-or-route ChangesFocus banner and arrivals refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
bmander
marked this pull request as ready for review
July 16, 2026 20:03
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalsPanel.kt (1)
80-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear stale short-list height when the next stop is not fully laid out.
HomeScreenretainscontentPxacross stops, but this callback emits nothing when the new list’s last item is off-screen. Switching from a short stop to a long stop therefore leaves the old short height active and keeps the new peek undersized. Report the “content exceeds viewport” case or reset the measurement when the session changes.🤖 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/arrivals/components/ArrivalsPanel.kt` around lines 80 - 92, Update the contentHeightPx measurement and onContentHeight callback in ArrivalsPanel so a new stop clears the previous height when the last item is not fully laid out, while preserving the measured height for fully laid-out short lists. Ensure the measurement is reset or emits the “content exceeds viewport” state when the session/content changes, preventing HomeScreen’s retained contentPx from leaving the peek undersized.
🤖 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/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.kt`:
- Around line 28-30: Resolve the deprecated API warning in the composeRule
declarations for ServiceAlertsDialogTest.kt lines 28-30 and FocusBannerTest.kt
lines 35-37 by replacing the deprecated API where possible; if suppression
remains necessary, add a one-line rationale with a tracking-issue link to each
`@Suppress`("DEPRECATION") declaration.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt`:
- Around line 214-222: Update the focusBannerBottomPx state declaration in the
HomeScreen composable to use unkeyed remember so focus changes preserve the
measured banner height. Reset this state only when the FocusBanner disappears,
while retaining the existing focusBannerTopEdge and setFocusBannerBottomEdge
flow.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.kt`:
- Around line 473-480: Update clearMapFocus() to reset both pending map-focus
fields alongside the existing focus hierarchy state. Ensure pendingMapFocus and
its associated pending stop latch are cleared when the explicit close control
runs, preventing later arrivals from consuming stale focus state.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt`:
- Around line 82-114: Add a BikeStation variant to FocusBannerState and render
it using the appropriate bike-station presentation. In
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt:356-381,
map CurrentFocus.BikeStation to that state. In
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt:37-44,
return the measured banner edge for BikeStation focus alongside the existing
stop and route cases.
- Around line 72-76: Give CompactRouteDismissAction a minimum interactive touch
target instead of using the 22dp dismiss icon bounds as the click area, while
preserving the existing icon appearance and shared 40dp sizing conventions in
FocusBanner.kt. Update the related FocusBannerTest assertion to verify a usable
minimum target size rather than exactly 22dp. Affected sites: FocusBanner.kt
lines 72-76 define the sizing context and require no direct change unless needed
for reuse; FocusBanner.kt lines 300-310 require the dismiss-action container
change; FocusBanner.kt lines 436-449 require no direct change unless they share
the same dismiss-action implementation; FocusBannerTest.kt lines 133-146 require
the expectation update.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBannerViewModel.kt`:
- Around line 38-49: Update toggleRouteFavorite in FocusBannerViewModel to
serialize rapid favorite updates or use an atomic repository-level toggle,
rather than deriving favorite from the potentially stale favoriteRouteIds
snapshot. Ensure consecutive taps are applied in order so two toggles restore
the route to its original state.
In
`@onebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeSheetLogicTest.kt`:
- Around line 44-53: The test around focusBannerTopEdge must treat
CurrentFocus.BikeStation as using the shared focus banner rather than a
bannerless state. Update the bike-station expectation in `stop and route focus
use the measured focus banner edge` and add the corresponding bike-station
banner presentation in the focus UI logic, reusing the existing banner behavior
and measured-edge handling.
---
Outside diff comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalsPanel.kt`:
- Around line 80-92: Update the contentHeightPx measurement and onContentHeight
callback in ArrivalsPanel so a new stop clears the previous height when the last
item is not fully laid out, while preserving the measured height for fully
laid-out short lists. Ensure the measurement is reset or emits the “content
exceeds viewport” state when the session/content changes, preventing
HomeScreen’s retained contentPx from leaving the peek undersized.
🪄 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: 3c8ffce1-bc6c-47f0-b127-a04cfa938cca
📒 Files selected for processing (22)
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/arrivals/ArrivalsPanelHeaderTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.ktonebusaway-android/src/main/AndroidManifest.xmlonebusaway-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/render/MapRenderState.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/ArrivalsScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalRows.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalsPanel.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/arrivals/ArrivalsSheetHost.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialog.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBannerViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/RouteHeaderOverlay.ktonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/test/java/org/onebusaway/android/map/render/MapRenderStatePaddingTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeSheetLogicTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/home/HomeViewModelTest.kt
💤 Files with no reviewable changes (3)
- onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/arrivals/ArrivalsPanelHeaderTest.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/RouteHeaderOverlay.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/components/ArrivalRows.kt
Comment on lines
+28
to
+30
| @Suppress("DEPRECATION") | ||
| @get:Rule | ||
| val composeRule = createComposeRule() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve or properly document both compiler-warning suppressions.
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.kt#L28-L30: fix the deprecated API warning or add the required rationale and issue link.onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt#L35-L37: apply the same treatment.
As per coding guidelines, “Maintain zero Kotlin compiler warnings. Fix warnings at their source; use @Suppress only when necessary, with a one-line rationale and tracking-issue link.”
📍 Affects 2 files
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.kt#L28-L30(this comment)onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt#L35-L37
🤖 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/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.kt`
around lines 28 - 30, Resolve the deprecated API warning in the composeRule
declarations for ServiceAlertsDialogTest.kt lines 28-30 and FocusBannerTest.kt
lines 35-37 by replacing the deprecated API where possible; if suppression
remains necessary, add a one-line rationale with a tracking-issue link to each
`@Suppress`("DEPRECATION") declaration.
Source: Coding guidelines
Comment on lines
+72
to
+76
| // The banner's route action icons (switch-direction / cancel) share one size + tint so they read as | ||
| // one control group: a larger-than-default 36dp icon in a deliberately tightened 40dp touch box | ||
| // (below Material's 48dp default — a conscious trade-off for a compact header banner). | ||
| private val HEADER_ICON_SIZE = 36.dp | ||
| private val HEADER_ICON_BUTTON_SIZE = 40.dp |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first.
git ls-files onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt \
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt
echo
echo "== FocusBanner.kt outline =="
ast-grep outline onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt --view expanded
echo
echo "== FocusBannerTest.kt outline =="
ast-grep outline onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt --view expandedRepository: OneBusAway/onebusaway-android
Length of output: 1441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FocusBanner.kt (header icon + compact dismiss sections) =="
sed -n '60,90p;290,330p;430,470p' onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt
echo
echo "== FocusBannerTest.kt (dismiss target test) =="
sed -n '125,155p' onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.ktRepository: OneBusAway/onebusaway-android
Length of output: 5892
🌐 Web query:
Compose Material3 IconButton modifier size minimum touch target 48dp
💡 Result:
In Material Design 3 and Jetpack Compose, the standard IconButton component is designed with a minimum touch target size of 48dp x 48dp to meet accessibility guidelines [1][2]. Key points regarding touch target sizing: 1. Automatic Handling: Many Material 3 components, including IconButton, automatically enforce this 48dp minimum touch target size [1][2]. Even if an icon appears visually smaller (e.g., 24dp x 24dp), the underlying component adds the necessary padding or expansion to ensure the total interactive area reaches 48dp x 48dp [3][4][5]. 2. Modifier.minimumInteractiveComponentSize: For custom components or when you need to explicitly ensure a component meets the 48dp minimum size requirement in your layout, you can use the Modifier.minimumInteractiveComponentSize modifier [6][5]. This modifier reserves at least 48dp in size for the component, preventing touch target overlap and ensuring accessibility compliance [6]. 3. Important Consideration: While Compose automatically expands the touch target region of interactive elements if they are smaller than 48dp, this can lead to overlapping touch areas if elements are placed too close together [7][5]. To avoid this, it is recommended to ensure that interactive elements are either visually sized to 48dp or spaced with sufficient padding (typically at least 8dp) to prevent touch target overlap [3][7][4].
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/compose/material3/IconButton.composable
- 2: https://developer.android.com/reference/kotlin/androidx/compose/material3/FilledIconButton.composable
- 3: https://support.google.com/accessibility/android/answer/7101858?hl=en
- 4: https://support.google.com/accessibility/android/answer/7101858
- 5: https://github.com/cvs-health/android-compose-accessibility-techniques/blob/main/doc/interactions/MinimumTouchTargetSize.md
- 6: https://developer.android.com/reference/kotlin/androidx/compose/material3/minimumInteractiveComponentSize.modifier
- 7: https://developer.android.com/develop/ui/compose/accessibility/api-defaults
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FocusBanner.kt around CompactRouteDismissAction and its caller =="
sed -n '240,340p' onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt
echo
echo "== Search for minimumInteractiveComponentSize / minimum touch target in the repo =="
rg -n "minimumInteractiveComponentSize|minimumTouchTarget|touch target|minimum touch" onebusaway-android/src/main/java onebusaway-android/src/androidTest -SRepository: OneBusAway/onebusaway-android
Length of output: 4641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '410,440p' onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.ktRepository: OneBusAway/onebusaway-android
Length of output: 1184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '440,460p' onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.ktRepository: OneBusAway/onebusaway-android
Length of output: 975
Give the dismiss icon a minimum touch target.
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L300-L310:CompactRouteDismissActionstill makes the 22dp icon the full click target; wrap it in a minimum interactive-size container.onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt#L133-L146: assert a usable target size instead of exactly 22dp.
📍 Affects 2 files
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L72-L76(this comment)onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L300-L310onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L436-L449onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt#L133-L146
🤖 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/map/FocusBanner.kt`
around lines 72 - 76, Give CompactRouteDismissAction a minimum interactive touch
target instead of using the 22dp dismiss icon bounds as the click area, while
preserving the existing icon appearance and shared 40dp sizing conventions in
FocusBanner.kt. Update the related FocusBannerTest assertion to verify a usable
minimum target size rather than exactly 22dp. Affected sites: FocusBanner.kt
lines 72-76 define the sizing context and require no direct change unless needed
for reuse; FocusBanner.kt lines 300-310 require the dismiss-action container
change; FocusBanner.kt lines 436-449 require no direct change unless they share
the same dismiss-action implementation; FocusBannerTest.kt lines 133-146 require
the expectation update.
Comment on lines
+82
to
+114
| sealed interface FocusBannerState { | ||
| val isFavorite: Boolean | ||
| val favoriteEnabled: Boolean | ||
| @get:DrawableRes val focusIconRes: Int | ||
| @get:StringRes val focusDescriptionRes: Int | ||
|
|
||
| data class SubordinateRoute( | ||
| val shortName: String, | ||
| val color: Int? = null, | ||
| ) | ||
|
|
||
| data class Stop( | ||
| val title: String, | ||
| val direction: String?, | ||
| override val isFavorite: Boolean, | ||
| override val favoriteEnabled: Boolean, | ||
| val hasAlerts: Boolean, | ||
| val subordinateRoutes: List<SubordinateRoute> = emptyList(), | ||
| val subordinateHeadsign: String? = null, | ||
| ) : FocusBannerState { | ||
| override val focusIconRes = R.drawable.stop_flag | ||
| override val focusDescriptionRes = R.string.stop_shortcut | ||
| } | ||
|
|
||
| data class Route( | ||
| val header: RouteHeader, | ||
| override val isFavorite: Boolean, | ||
| ) : FocusBannerState { | ||
| override val favoriteEnabled: Boolean get() = header.routeId != null | ||
| override val focusIconRes = R.drawable.ic_route | ||
| override val focusDescriptionRes = R.string.route_shortcut | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Implement bike-station focus across presentation and geometry.
The linked issue requires bike-share stations, but this flow remains stop-or-route only.
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L82-L114: add aBikeStationpresentation variant and rendering.onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt#L356-L381: construct that state fromCurrentFocus.BikeStation.onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt#L37-L44: return its measured banner edge for bike-station focus.
📍 Affects 3 files
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBanner.kt#L82-L114(this comment)onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt#L356-L381onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt#L37-L44
🤖 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/map/FocusBanner.kt`
around lines 82 - 114, Add a BikeStation variant to FocusBannerState and render
it using the appropriate bike-station presentation. In
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt:356-381,
map CurrentFocus.BikeStation to that state. In
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeSheetLogic.kt:37-44,
return the measured banner edge for BikeStation focus alongside the existing
stop and route cases.
Comment on lines
+44
to
+53
| fun `stop and route focus use the measured focus banner edge`() { | ||
| val stopRoute = StopRouteSelection( | ||
| originHeadsign = null, | ||
| legs = listOf(RouteLeg("route", "40")), | ||
| ) | ||
| assertEquals(120, routeFocusTopEdge(CurrentFocus.Stop(stop, stopRoute), 120, 240)) | ||
| assertEquals(240, routeFocusTopEdge(CurrentFocus.Route(RouteTarget("route")), 120, 240)) | ||
| assertEquals(0, routeFocusTopEdge(CurrentFocus.Stop(stop), 120, 240)) | ||
| assertEquals(0, routeFocusTopEdge(CurrentFocus.None, 120, 240)) | ||
| assertEquals(240, focusBannerTopEdge(CurrentFocus.Stop(stop, stopRoute), 240)) | ||
| assertEquals(240, focusBannerTopEdge(CurrentFocus.Route(RouteTarget("route")), 240)) | ||
| assertEquals(240, focusBannerTopEdge(CurrentFocus.Stop(stop), 240)) | ||
| assertEquals(0, focusBannerTopEdge(CurrentFocus.None, 240)) | ||
| assertEquals(0, focusBannerTopEdge(CurrentFocus.BikeStation("bike"), 240)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not codify bike-station focus as bannerless.
Line 53 expects no measured edge for CurrentFocus.BikeStation, but issue #1894 requires bike stations to use the shared focus banner and reserve map padding. Add the bike-station banner presentation and expect its measured edge here instead.
🤖 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/test/java/org/onebusaway/android/ui/home/HomeSheetLogicTest.kt`
around lines 44 - 53, The test around focusBannerTopEdge must treat
CurrentFocus.BikeStation as using the shared focus banner rather than a
bannerless state. Update the bike-station expectation in `stop and route focus
use the measured focus banner edge` and add the corresponding bike-station
banner presentation in the focus UI logic, reusing the existing banner behavior
and measured-edge handling.
- Document the createComposeRule() @Suppress("DEPRECATION") in the two new test files with the standard #1792 rationale, matching the repo convention. - Keep focusBannerBottomPx unkeyed so switching between two equal-height banners doesn't reset it to 0 (onSizeChanged only fires on size change), which framed the map as if no banner showed. - Clear the pending map-focus latches in clearMapFocus() so a stop closed before its arrivals load can't leak a recenter onto the next stop. - Serialize route-favorite toggles via an optimistic override so rapid double-taps build on the last intent instead of a stale store snapshot; the override is reconciled away once the store catches up. Bike-station focus (the rest of #1894) is deferred to a follow-up; the PR description now references the issue without auto-closing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBannerViewModel.kt (1)
63-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead from the synchronous override state to prevent the rapid-tap race condition.
Because
favoriteRouteIdsis a combinedStateFlow, its value updates asynchronously after an optimistic override. A second rapid tap will read the stalefavoriteRouteIds.valuebefore the flow emits the result of the first tap, effectively ignoring the second toggle.Read directly from the immediate
optimisticFavoritesmap to ensure back-to-back taps compute their intent sequentially, fulfilling the PR objective to serialize rapid updates.🐛 Proposed fix
- val favorite = routeId !in favoriteRouteIds.value - optimisticFavorites.update { it + (routeId to favorite) } + val favorite = !(optimisticFavorites.value[routeId] ?: (routeId in favoriteRouteIds.value)) + optimisticFavorites.update { it + (routeId to favorite) }🤖 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/map/FocusBannerViewModel.kt` around lines 63 - 64, Update the toggle logic in the surrounding ViewModel method to derive the current favorite state from the immediate optimisticFavorites map, falling back to favoriteRouteIds only when no override exists. Compute the next toggle from that synchronous state before updating optimisticFavorites, so consecutive taps are applied sequentially.
🤖 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.
Duplicate comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBannerViewModel.kt`:
- Around line 63-64: Update the toggle logic in the surrounding ViewModel method
to derive the current favorite state from the immediate optimisticFavorites map,
falling back to favoriteRouteIds only when no override exists. Compute the next
toggle from that synchronous state before updating optimisticFavorites, so
consecutive taps are applied sequentially.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f81d16d-f17e-4aec-992d-43fa59bc0b28
📒 Files selected for processing (5)
onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/home/map/FocusBannerViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeViewModel.kt
- onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/arrivals/ServiceAlertsDialogTest.kt
- onebusaway-android/src/androidTest/java/org/onebusaway/android/ui/home/map/FocusBannerTest.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/home/HomeScreen.kt
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.
Summary
Why
Stop focus and route focus previously presented their identity and actions in different parts of the screen. That split made the current focus harder to scan and caused banner actions to resolve at different times. A single focus surface gives both modes consistent placement, loading behavior, and map clearance.
Validation
./gradlew check :onebusaway-android:compileObaGoogleDebugAndroidTestKotlin -x lint -PwarningsAsErrors=trueFocusBannerTestinstrumentation tests on a Pixel 7 ProPart of #1894 — this PR lays the shared stop/route focus-banner groundwork; the bike-station
variant the issue calls for is a follow-up, so it intentionally does not auto-close #1894.
Summary by CodeRabbit
New Features
Improvements
Tests