Android driver app v1 + driver-facing vehicles endpoint - #89
Merged
Conversation
- background location permission required for post-kill tracking resume (Android 14+ FGS rules) - explicit manifest permission checklist incl. FOREGROUND_SERVICE_LOCATION - fine+coarse requested together; approximate-only grant handling - pre-start location-services check via SettingsClient - clock-skew (±5min) distinct error status; dual trip-ID persistence - edge-to-edge/target-36 notes, notification channel + swipe-away edge cases
…orts - Add tests for HTTP 500 and 400-without-timestamp, asserting status and fixesSent are unchanged (previously uncovered else branch). - Add test proving success resets the consecutive timestamp-reject streak. - Log.w dropped reports (other 4xx/5xx, and network failures) per the "log-and-drop" spec language. - Guard errorBody().string() with runCatching so a truncated/unreadable body can't throw an uncaught IOException out of report(); treated as a non-timestamp 400 (logged and dropped) instead.
…network-backed ViewModelsTest cases The brief's single advanceUntilIdle() call races real MockWebServer/OkHttp async I/O, which resolves on a background thread pool outside the test dispatcher's control. Add a bounded, real-time poll helper (advanceUntilIdle + tiny sleep, repeated up to a 5s timeout, fails loudly on expiry) and use it only in the two tests that were flaky. Assertions and outcomes are unchanged; the other two tests are untouched.
… trip start
- LoginViewModel now injects SessionStore and prefills the server URL field
from the persisted session in init, guarded so it never clobbers text the
user has already typed. Adds a ViewModelsTest case proving the prefill.
- AppNav's Trip Setup -> Tracking transition used popUpTo(ROUTE_LOGIN), a
no-op since "login" was already popped at Login -> Vehicles, leaving Trip
Setup/Vehicle Select reachable via system back mid-trip. Switched to
popUpTo(0) { inclusive = true }, matching the Tracking -> Login transition
already in the file, so Tracking becomes the sole back-stack root.
PermissionFlow's needsBackgroundLocationStage gated on fine-OR-coarse location, so a user who declined precise location at the APPROX_WARNING dialog was still funneled into the background-location explain/request stage. Per spec, background location is a settings-directed step that should only follow a FINE grant. Both call sites route through this one predicate, so tightening it to FINE-only fixes the flow end-to-end.
ApiHolder.api() read a serverUrl populated asynchronously by collecting
SessionStore.session; on a cold start straight into VehicleScreen (fresh
persisted token, no active trip), the ViewModel could resolve TrackerApi
before that collector's first emission landed, so Retrofit.Builder()
.baseUrl("") threw uncaught and crashed the app. Reproduced reliably on
emulator prior to this fix (see task-8-report.md).
Two changes:
- ApiHolder now synchronously seeds its cache from the store on first
use (guarded runBlocking, one-time) instead of relying solely on the
background collector.
- VehicleRepository/TripRepository now take a TrackerApiProvider and
resolve TrackerApi lazily inside their own try/catch, so a genuinely
absent server URL surfaces as Result.failure(ApiError.Other) instead
of throwing during Hilt-graph construction.
Adds JVM tests proving the holder seeds without waiting on the async
collector and that both repositories fail gracefully with no server
URL. Re-verified cold-start-to-VehicleScreen on an API 35 emulator: 0/3
crashes (previously 3/3).
TripReporter now takes TrackerApiProvider and resolves TrackerApi lazily inside report()'s existing try block, matching the Task 8 cold-start fix and removing a stale-URL pin across re-login at a different server. AppModule's now-unused eager provideTrackerApi binding is deleted. After ending a trip, the driver lands on Vehicle Select instead of Login (the session token is still fresh) per spec design intent; updates AppNav's onTripEnded and the smoke-test doc's Check 5.
- Resume tracking notification no longer sets setOngoing(true); only the active tracking notification does. - AuthRepository/TripRepository/VehicleRepository rethrow CancellationException before the general catch so coroutine cancellation doesn't surface as a user-visible error. - Differentiate tracking_status_auth_expired (banner) from tracking_reauth_button (button label "Log in") — they were identical text. - LocationTrackingService returns START_NOT_STICKY (not START_STICKY) when it stops itself due to a null active trip. - Remove the unreferenced okhttp-logging library entry from libs.versions.toml.
…ase in CI Add a JVM test that builds a TrackerApi from a base URL lacking a trailing slash and asserts the request still hits the correct path. Add an assembleRelease step to the Android CI workflow after unit tests, so a release-build regression is caught in CI.
📝 WalkthroughWalkthroughAdds an Android driver tracking app with Compose screens, persisted authentication and trip state, foreground location reporting, permissions, notifications, CI, and tests. Adds an authenticated Go endpoint that lists active vehicles assigned to the current driver. ChangesDriver vehicle API
Android foundation
API and persistence
Foreground tracking
Compose workflows
Validation and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Driver
participant AndroidApp
participant VehicleApi
participant LocationService
participant TrackerApi
Driver->>AndroidApp: Log in and select vehicle
AndroidApp->>VehicleApi: GET /api/v1/vehicles
VehicleApi-->>AndroidApp: Assigned vehicle list
Driver->>AndroidApp: Start trip
AndroidApp->>TrackerApi: Start trip
TrackerApi-->>AndroidApp: Active trip
AndroidApp->>LocationService: Start foreground tracking
LocationService->>TrackerApi: Report location fixes
TrackerApi-->>LocationService: Report result
LocationService-->>AndroidApp: Tracking status updates
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 |
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (8)
.github/workflows/android.yml (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRun the instrumentation test suite in CI.
This workflow runs
assembleDebugandtestDebugUnitTest, but neither command executes tests underandroidTest. Add an emulator-backedconnectedDebugAndroidTestjob so Compose and device-level flows are release gates.🤖 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 @.github/workflows/android.yml around lines 24 - 29, Add an emulator-backed CI step or job alongside the existing Android workflow steps that runs ./gradlew :app:connectedDebugAndroidTest, ensuring the emulator is configured and started before execution so androidTest instrumentation and Compose device flows become release gates.android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt (1)
62-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
"network"string sentinel with a typed error.Line 66 detects a connectivity failure by comparing
error.msgto the literal"network". The check depends on an exact string produced bymapHttpError. A wording change in the mapper silently downgrades every network failure toTripError.OTHER, and the compiler cannot catch it.LoginViewModelline 70 repeats the same comparison. Add anApiError.Networkcase and match on the type.🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt` around lines 62 - 68, Add a typed ApiError.Network variant and update mapHttpError to return it for connectivity failures instead of embedding "network" in error.msg. In TripSetupViewModel’s onFailure mapping, match ApiError.Network directly to TripError.NETWORK, and apply the same replacement in LoginViewModel, removing the string comparison.android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginViewModel.kt (1)
53-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject a second submit while a login is in flight.
onLogindoes not inspectstate.loading.LoginScreendisables the button through recomposition, so two fast taps can both reachonLoginand start twoauthRepository.logincalls. The second response then overwrites the first. Return early whenloadingis already true.♻️ Proposed fix
fun onLogin(onSuccess: () -> Unit) { val state = _uiState.value + if (state.loading) return if (state.serverUrl.isBlank() || state.email.isBlank() || state.password.isBlank()) {🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginViewModel.kt` around lines 53 - 59, Update LoginViewModel.onLogin to return immediately when the current UI state has loading already set, before validation or starting another login request. Preserve the existing validation and loading behavior for submissions made when no login is in flight.android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginScreen.kt (1)
62-77: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
KeyboardOptionson the email and password fields.Both fields use the default keyboard configuration. The email field does not show the email keyboard layout. The password field allows autocorrect and word suggestions, so the IME can store the password in the user dictionary and suggest it in other apps. Set
KeyboardType.EmailandKeyboardType.Password.♻️ Proposed fix
OutlinedTextField( value = state.email, onValueChange = onEmailChange, label = { Text(stringResource(R.string.login_email_label)) }, singleLine = true, + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Email), modifier = Modifier.fillMaxWidth().heightIn(min = 48.dp), ) Spacer(Modifier.height(12.dp)) OutlinedTextField( value = state.password, onValueChange = onPasswordChange, label = { Text(stringResource(R.string.login_password_label)) }, singleLine = true, visualTransformation = PasswordVisualTransformation(), + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Password), modifier = Modifier.fillMaxWidth().heightIn(min = 48.dp), )Add the imports:
+import androidx.compose.foundation.text.KeyboardOptions +import androidx.compose.ui.text.input.KeyboardType🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginScreen.kt` around lines 62 - 77, Update the email and password OutlinedTextField declarations in LoginScreen to provide KeyboardOptions with KeyboardType.Email and KeyboardType.Password respectively, preserving the existing field behavior while disabling password autocorrect and suggestions through the password keyboard configuration.android/app/src/test/kotlin/org/onebusaway/vehicletracker/service/TripReporterTest.kt (1)
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShut down
MockWebServerin a teardown block.Every test calls
server.shutdown()as the last statement. If an assertion fails before that line, the shutdown never runs and the server thread and socket leak for the rest of the JVM test run. Hold the server in a field and shut it down from an@Aftermethod, or wrap each server inuse { }.♻️ Sketch of the teardown approach
class TripReporterTest { + private val servers = mutableListOf<MockWebServer>() + + `@After` fun tearDown() = servers.forEach { runCatching { it.shutdown() } } + + private fun newServer() = MockWebServer().apply { start() }.also { servers += it }🤖 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 `@android/app/src/test/kotlin/org/onebusaway/vehicletracker/service/TripReporterTest.kt` around lines 26 - 34, Move MockWebServer cleanup out of individual test bodies: store each test server in a test field and shut it down from an `@After` teardown method, removing direct server.shutdown() calls so cleanup still runs when assertions fail.android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TrackingNotification.kt (1)
64-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
setAutoCancel(true)to the resume notification.
buildResumeNotificationis not ongoing, so the user can swipe it away. After a tap, the notification stays in the shade becausesetAutoCancelis not set. The tap opensMainActivityand the stale "tap to resume" notification remains visible.♻️ Proposed fix
fun buildResumeNotification(): Notification = baseBuilder() + .setAutoCancel(true) .setContentTitle(context.getString(R.string.tracking_notification_resume_title)) .setContentText(context.getString(R.string.tracking_notification_resume_text)) .build()🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TrackingNotification.kt` around lines 64 - 68, Update buildResumeNotification to call setAutoCancel(true) on its notification builder before build(), ensuring the resume notification is removed from the notification shade after the user taps it.android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt (1)
62-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTie the
MockWebServerlifetime to the test lifecycle.Three tests create a
MockWebServerand callserver.shutdown()only after their assertions. If an assertion fails, or ifawaitConditioncallsfail(...), the shutdown never runs. Each failing test then leaks a server thread and a bound port, which can make later failures in the same run harder to diagnose. The shared root cause is that server cleanup is not registered with JUnit.
android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt#L62-L74: use a single class-level server shut down intearDown, and remove the localMockWebServer()and trailingserver.shutdown().android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt#L89-L100: use the same shared server and remove the trailingserver.shutdown().android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt#L102-L115: use the same shared server and remove the trailingserver.shutdown().♻️ Proposed shared server fixture
class ViewModelsTest { private val dispatcher = StandardTestDispatcher() + private val server = MockWebServer() - `@Before` fun setUp() = Dispatchers.setMain(dispatcher) - `@After` fun tearDown() = Dispatchers.resetMain() + `@Before` fun setUp() { + Dispatchers.setMain(dispatcher) + server.start() + } + + `@After` fun tearDown() { + Dispatchers.resetMain() + server.shutdown() + }Then in each affected test, delete the local
val server = MockWebServer().apply { start() }line and the trailingserver.shutdown()line.🤖 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 `@android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt` around lines 62 - 74, In ViewModelsTest.kt, add a class-level MockWebServer fixture started for the test lifecycle and shut down from tearDown. Update the affected tests at lines 62-74, 89-100, and 102-115 to reuse that shared server, removing each local server creation and trailing shutdown call; no direct changes are needed beyond replacing those local usages.android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/theme/Theme.kt (1)
10-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme-aware background colors for the tracking status banner.
TrackingScreen.ktdrawsStatusRed/StatusGreenon full-width status banners with white text, but both values are fixed and do not vary byAppTheme. The fixedStatusGreenis below 4.5:1 against the Material 3 dark surface, so the dark-status connected state does not meet normal text contrast. Choose lighter dark-mode status backgrounds or provide theme-aware colors and apply them inStatusBanner,TrackingScreenContent, and the reauth/end-trip buttons.🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/theme/Theme.kt` around lines 10 - 20, Make the tracking status colors theme-aware instead of using fixed StatusGreen and StatusRed values. Define accessible light- and dark-theme variants, then apply the selected colors consistently in StatusBanner, TrackingScreenContent, and the reauth/end-trip buttons so white text maintains sufficient contrast in dark mode.
🤖 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 @.github/workflows/android.yml:
- Line 18: Update the actions/checkout@v4 step in the Android workflow to set
persist-credentials to false, preventing the GitHub token from being stored in
local Git configuration before Gradle runs.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/SessionStore.kt`:
- Around line 22-28: Update DataStoreSessionStore and its sessionDataStore so
bearer TOKEN is stored in a protected credential storage facility rather than
the default Preferences DataStore. Exclude the session store from cloud and
device-transfer backups, and ensure restored state cannot reuse the token by
requiring re-authentication after restore.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.kt`:
- Around line 18-43: Update TripRepository.start and end so successful server
operations are not converted to failure by subsequent DataStore errors. Make
addRecentRoute non-blocking/non-critical, and give saveActiveTrip and
clearActiveTrip dedicated recovery handling that preserves the server-success
result and lets MainActivity.onResume restore or reconcile active-trip state.
Continue propagating CancellationException and mapping genuine API failures as
before.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt`:
- Around line 76-81: Update addRecentRoute() and the corresponding recent-route
read logic to persist RECENT_ROUTES as a serialized List<String> rather than a
pipe-delimited string, preserving route IDs exactly—including embedded “|”
characters—while retaining deduplication and the five-route limit. Align
FakeTripStateStore with this behavior without sanitizing route IDs, and add a
trip-start test covering a route containing “|”.
In `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/di/AppModule.kt`:
- Around line 70-79: Update ensureSeeded() so API cache seeding does not call
runBlocking or synchronously await sessionStore.session.first() during api()
resolution. Launch the session read asynchronously while preserving token and
serverUrl assignment and ensuring concurrent callers do not start duplicate
seeding; expose or use readiness separately if needed without blocking the
caller.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/LocationTrackingService.kt`:
- Around line 117-131: Update onStartCommand so the activeTrip load from
tripStateStore.activeTrip.first() runs asynchronously off the main thread using
withContext, removing the blocking runBlocking call and its unused import.
Preserve the existing null-trip behavior by calling stopSelf(), but return
START_STICKY after the asynchronous initialization path as requested.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TripReporter.kt`:
- Around line 30-37: Guard TripReporter’s shared mutable state, including
gpsAvailable, consecutiveTimestampRejects, and currentSendProblem, against
concurrent report and gpsAvailable calls. Prefer enforcing a single-thread
coroutine execution context for TripReporter, or synchronize all accesses and
ensure cross-thread visibility while preserving the existing counter and
problem-refresh behavior.
- Around line 55-70: Update the HTTP 400 handling in the TripReporter catch
block to classify clock-skew responses using a stable server-provided error code
or field parsed from the response payload, rather than searching for the
substring “timestamp”. Preserve the existing consecutiveTimestampRejects
threshold and CLOCK_SKEW handling, while treating other 400 responses through
the existing fallback path.
In `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/AppNav.kt`:
- Around line 98-102: Update the onVehicleSelected callback in the
ROUTE_VEHICLES composable to URI-encode vehicleId before constructing the
navigation route, and build the destination from ROUTE_TRIP rather than a
hard-coded "trip" prefix. Preserve the encoded ID as the route argument so IDs
containing reserved characters navigate correctly.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginScreen.kt`:
- Around line 48-51: Update the login form Column in LoginScreen to add a
remembered verticalScroll state and imePadding to its modifier, importing the
required Compose APIs. Preserve the existing fillMaxSize, padding, and centered
arrangement so the complete form remains reachable when content exceeds the
available height or the keyboard is visible.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/permissions/PermissionFlow.kt`:
- Around line 153-169: Update the Stage.REQUEST_BATTERY branch in PermissionFlow
to catch ActivityNotFoundException from batteryLauncher.launch and advance
directly to Stage.CHECK_LOCATION_SETTINGS. Add the required
ActivityNotFoundException and Log imports plus the PermissionFlow TAG constant,
and log the caught IntentSender.SendIntentException in the
settingsResolutionLauncher failure path before setting
Stage.LOCATION_SETTINGS_OFF.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/tracking/TrackingScreen.kt`:
- Around line 145-161: Prevent duplicate end-trip submissions by disabling the
retry TextButton in the end-trip error dialog while state.ending is true, and
add an early return at the start of TrackingViewModel.onEndTrip when
_uiState.value.ending is already true. Preserve the existing active-trip guard
and end-trip behavior for the first submission.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt`:
- Around line 47-53: Update TripSetupViewModel.onStartTrip to return immediately
when the current UI state has loading already true, before starting another
request; preserve the existing route validation and loading transition for idle
starts. Apply the same guard in LoginViewModel.onLogin so repeated submissions
cannot run concurrently.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleScreen.kt`:
- Around line 35-40: Update the single-vehicle auto-selection logic in
VehicleScreen’s LaunchedEffect(state) so it executes only once across
recomposition and back-navigation, using a rememberSaveable guard or a consumed
one-shot event from VehicleViewModel. Preserve selection for the initially
loaded single-vehicle state while preventing repeated navigation after returning
from trip setup.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleViewModel.kt`:
- Around line 40-43: Update the unauthorized handling in the vehicle-loading
flow around the ViewModel’s onFailure callback so ApiError.Unauthorized does not
produce the non-retryable generic VehiclesUiState.Error dead end. Surface a
distinct unauthorized state that VehicleScreen can use to navigate to login, or
clear the session so AppNavViewModel routes to the login screen on the next
launch; preserve retry behavior for other errors.
In `@docs/android-smoke-test.md`:
- Around line 157-163: Update the physical-device guidance near the Android
connectivity instructions so it uses HTTPS, or consistently extend the debug
network-security configuration and its documentation to allow the intended LAN
development host. Ensure the documented login path matches the actual cleartext
hosts permitted by the debug configuration.
- Around line 20-29: Add Python 3 to the Tools prerequisites list in the
smoke-test documentation, since the token extraction commands use python3. Keep
the existing extraction commands unchanged.
In `@docs/superpowers/specs/2026-08-04-android-driver-app-design.md`:
- Around line 32-40: Update the fenced directory-tree block in the Android
project structure documentation to declare the `text` language, preserving the
existing tree content.
---
Nitpick comments:
In @.github/workflows/android.yml:
- Around line 24-29: Add an emulator-backed CI step or job alongside the
existing Android workflow steps that runs ./gradlew
:app:connectedDebugAndroidTest, ensuring the emulator is configured and started
before execution so androidTest instrumentation and Compose device flows become
release gates.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TrackingNotification.kt`:
- Around line 64-68: Update buildResumeNotification to call setAutoCancel(true)
on its notification builder before build(), ensuring the resume notification is
removed from the notification shade after the user taps it.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginScreen.kt`:
- Around line 62-77: Update the email and password OutlinedTextField
declarations in LoginScreen to provide KeyboardOptions with KeyboardType.Email
and KeyboardType.Password respectively, preserving the existing field behavior
while disabling password autocorrect and suggestions through the password
keyboard configuration.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginViewModel.kt`:
- Around line 53-59: Update LoginViewModel.onLogin to return immediately when
the current UI state has loading already set, before validation or starting
another login request. Preserve the existing validation and loading behavior for
submissions made when no login is in flight.
In `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/theme/Theme.kt`:
- Around line 10-20: Make the tracking status colors theme-aware instead of
using fixed StatusGreen and StatusRed values. Define accessible light- and
dark-theme variants, then apply the selected colors consistently in
StatusBanner, TrackingScreenContent, and the reauth/end-trip buttons so white
text maintains sufficient contrast in dark mode.
In
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt`:
- Around line 62-68: Add a typed ApiError.Network variant and update
mapHttpError to return it for connectivity failures instead of embedding
"network" in error.msg. In TripSetupViewModel’s onFailure mapping, match
ApiError.Network directly to TripError.NETWORK, and apply the same replacement
in LoginViewModel, removing the string comparison.
In
`@android/app/src/test/kotlin/org/onebusaway/vehicletracker/service/TripReporterTest.kt`:
- Around line 26-34: Move MockWebServer cleanup out of individual test bodies:
store each test server in a test field and shut it down from an `@After` teardown
method, removing direct server.shutdown() calls so cleanup still runs when
assertions fail.
In
`@android/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.kt`:
- Around line 62-74: In ViewModelsTest.kt, add a class-level MockWebServer
fixture started for the test lifecycle and shut down from tearDown. Update the
affected tests at lines 62-74, 89-100, and 102-115 to reuse that shared server,
removing each local server creation and trailing shutdown call; no direct
changes are needed beyond replacing those local usages.
🪄 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: c3b5af01-4c11-4224-8e32-a9d16431b60a
⛔ Files ignored due to path filters (1)
android/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (68)
.github/workflows/android.yml.gitignoreREADME.mdandroid/app/build.gradle.ktsandroid/app/proguard-rules.proandroid/app/src/androidTest/kotlin/org/onebusaway/vehicletracker/ui/ScreenFlowTest.ktandroid/app/src/debug/AndroidManifest.xmlandroid/app/src/debug/res/xml/network_security_config.xmlandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/MainActivity.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/TrackerApp.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/ApiError.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/AuthRepository.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/SessionStore.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TrackingRepository.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/VehicleRepository.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/api/ApiFactory.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/api/ApiModels.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/data/api/TrackerApi.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/di/AppModule.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/service/LocationTrackingService.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/service/ServiceController.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/service/ServiceControllerImpl.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TrackingNotification.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/service/TripReporter.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/AppNav.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginScreen.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/login/LoginViewModel.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/permissions/PermissionFlow.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/theme/Theme.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/tracking/TrackingScreen.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/tracking/TrackingViewModel.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupScreen.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleScreen.ktandroid/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleViewModel.ktandroid/app/src/main/res/drawable/ic_launcher_foreground.xmlandroid/app/src/main/res/drawable/ic_tracking_notification.xmlandroid/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlandroid/app/src/main/res/values/colors.xmlandroid/app/src/main/res/values/strings.xmlandroid/app/src/main/res/values/themes.xmlandroid/app/src/test/kotlin/org/onebusaway/vehicletracker/data/Fakes.ktandroid/app/src/test/kotlin/org/onebusaway/vehicletracker/data/RepositoriesTest.ktandroid/app/src/test/kotlin/org/onebusaway/vehicletracker/data/api/TrackerApiTest.ktandroid/app/src/test/kotlin/org/onebusaway/vehicletracker/service/TripReporterTest.ktandroid/app/src/test/kotlin/org/onebusaway/vehicletracker/ui/ViewModelsTest.ktandroid/build.gradle.ktsandroid/gradle.propertiesandroid/gradle/libs.versions.tomlandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/gradlewandroid/gradlew.batandroid/settings.gradle.ktsdb/models.godb/query.sqldb/query.sql.godocs/android-smoke-test.mddocs/superpowers/plans/2026-08-04-android-driver-app.mddocs/superpowers/specs/2026-08-04-android-driver-app-design.mddriver_vehicle_handlers.godriver_vehicle_handlers_test.gomain.goroute_wiring_test.gostore_vehicles.gostore_vehicles_test.go
| run: | ||
| working-directory: android | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Disable persisted checkout credentials.
actions/checkout stores the GitHub token in local Git configuration by default. Gradle runs after checkout and can read this credential. Set persist-credentials: false.
Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/android.yml at line 18, Update the actions/checkout@v4
step in the Android workflow to set persist-credentials to false, preventing the
GitHub token from being stored in local Git configuration before Gradle runs.
Source: Linters/SAST tools
Comment on lines
+22
to
+28
| private val Context.sessionDataStore by preferencesDataStore(name = "session") | ||
|
|
||
| class DataStoreSessionStore(private val context: Context) : SessionStore { | ||
| private object Keys { | ||
| val SERVER_URL = stringPreferencesKey("server_url") | ||
| val TOKEN = stringPreferencesKey("token") | ||
| val TOKEN_ISSUED_AT = longPreferencesKey("token_issued_at") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f . android/app/src | rg 'AndroidManifest\.xml$|/res/xml/.*(backup|data).*\.xml$'
rg -n -C 3 'allowBackup|dataExtractionRules|fullBackupContent|cloud-backup|device-transfer|<exclude|session' android/app/srcRepository: OneBusAway/vehicle-positions
Length of output: 29172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Manifests ---\n'
for f in $(fd -HI -t f 'AndroidManifest.xml$' android/app/src); do
echo "### $f"
cat -n "$f" | sed -n '1,220p'
done
printf '\n--- AndroidManifest snippets ---\n'
rg -n -C 4 'allowBackup|fullBackupContent|dataExtractionRules|backup="false"|android:backup' android/app/src/*/AndroidManifest.xmlRepository: OneBusAway/vehicle-positions
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Manifests ---'
while IFS= read -r f; do
echo "### $f"
cat -n "$f" | sed -n '1,220p'
done < <(fd -HI -t f 'AndroidManifest.xml$' android/app/src)
printf '%s\n' ''
printf '%s\n' '--- AndroidManifest snippets ---'
rg -n -C 4 'allowBackup|fullBackupContent|dataExtractionRules|backup="false"|android:backup' android/app/src/*/AndroidManifest.xml || trueRepository: OneBusAway/vehicle-positions
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AndroidManifest files ---'
fd -HI -t f 'AndroidManifest.xml$' android/app/src
printf '%s\n' ''
printf '%s\n' '--- Manifest snippets ---'
for f in $(fd -HI -t f 'AndroidManifest.xml$' android/app/src | sort); do
echo "### $f"
sed -n '1,220p' "$f"
done
printf '%s\n' ''
printf '%s\n' '--- Backup configuration snippets ---'
rg -n -C 4 'allowBackup|fullBackupContent|dataExtractionRules|backup="false"|android:backup' android/app/src || true
printf '%s\n' ''
printf '%s\n' '--- DataStore definition in SessionStore.kt ---'
sed -n '1,70p' android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/SessionStore.ktRepository: OneBusAway/vehicle-positions
Length of output: 4238
Store the bearer token outside the default Preferences DataStore.
DataStoreSessionStore saves TOKEN via preferencesDataStore(name = "session"), and the manifest enables standard backup behavior. A restored or transferred backup can replay this bearer credential while it remains valid. Use a protected credential/token storage facility, exclude this store from cloud and device-transfer backups, and require re-authentication after restore.
🤖 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
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/SessionStore.kt`
around lines 22 - 28, Update DataStoreSessionStore and its sessionDataStore so
bearer TOKEN is stored in a protected credential storage facility rather than
the default Preferences DataStore. Exclude the session store from cloud and
device-transfer backups, and ensure restored state cannot reuse the token by
requiring re-authentication after restore.
Comment on lines
+18
to
+43
| suspend fun start(vehicleId: String, routeId: String, gtfsTripId: String): Result<ActiveTrip> = try { | ||
| val trip = apiProvider.get().startTrip(StartTripRequest(vehicleId, routeId, gtfsTripId)) | ||
| val activeTrip = ActiveTrip( | ||
| tripDbId = trip.id, | ||
| locationTripId = gtfsTripId.ifBlank { routeId }, | ||
| vehicleId = vehicleId, | ||
| routeId = routeId, | ||
| startedAtEpochSec = clock(), | ||
| ) | ||
| tripStateStore.saveActiveTrip(activeTrip) | ||
| tripStateStore.addRecentRoute(routeId) | ||
| Result.success(activeTrip) | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Result.failure(mapHttpError(e)) | ||
| } | ||
|
|
||
| suspend fun end(tripDbId: Long): Result<Unit> = try { | ||
| apiProvider.get().endTrip(EndTripRequest(tripDbId)) | ||
| tripStateStore.clearActiveTrip() | ||
| Result.success(Unit) | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Result.failure(mapHttpError(e)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.kt --items all
fd -t f 'TripSetupViewModel.kt|TrackingViewModel.kt' android/app/src/main/kotlin \
-x ast-grep outline {} --items all
rg -n -C 4 'Result<ActiveTrip>|\.start\(|\.end\(|TripAlreadyActive|clearActiveTrip|saveActiveTrip' \
android/app/src/main/kotlin \
android/app/src/test/kotlinRepository: OneBusAway/vehicle-positions
Length of output: 24927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- TripRepository.kt ---\n'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.kt
printf '\n--- TrackingViewModel start/end handling ---\n'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/tracking/TrackingViewModel.kt
printf '\n--- TripSetupViewModel start handling ---\n'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt
printf '\n--- TripStateStore implementation ---\n'
sed -n '1,95p' android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt | cat -n
printf '\n--- StartService / state restore references ---\n'
rg -n -C 3 'ActiveTrip|recentRoutes|activeTrip|Track|startService|TripSetup|restore|load' android/app/src/main/kotlin -g '*.kt'Repository: OneBusAway/vehicle-positions
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- TripRepository.kt ---'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.kt
echo
echo '--- TrackingViewModel start/end handling ---'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/tracking/TrackingViewModel.kt
echo
echo '--- TripSetupViewModel start handling ---'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/trip/TripSetupViewModel.kt
echo
echo '--- TripStateStore implementation ---'
sed -n '1,95p' android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt | cat -n
echo
echo '--- State restore and service references ---'
rg -n -C 3 'ActiveTrip|recentRoutes|activeTrip|Track|startService|TripSetup|restore|load' android/app/src/main/kotlin -g '*.kt'Repository: OneBusAway/vehicle-positions
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- service files ---'
git ls-files android/app/src/main/kotlin/org/onebusaway/vehicletracker/service | sort
echo
echo '--- ServiceControllerImpl ---'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/ServiceControllerImpl.kt
echo
echo '--- ServiceController interface ---'
cat -n android/app/src/main/kotlin/org/onebusaway/vehicletracker/service/ServiceController.kt
echo
echo '--- startTracking / stopTracking implementation references ---'
rg -n -C 5 'startTracking|stopTracking|bindService|startForeground|stopForeground|Intent|ServiceControllerImpl' android/app/src/main/kotlin/org/onebusaway/vehicletracker/service -g '*.kt'Repository: OneBusAway/vehicle-positions
Length of output: 22810
Reconcile server success when DataStore writes fail.
start() calls startTrip before saveActiveTrip() and addRecentRoute(), so a DataStore failure after server success still returns Result.failure. The user can retry and hit TripAlreadyActive, even though the server trip exists. end() has the same inversion: endTrip() succeeds before clearActiveTrip(), so a local clear failure leaves the app tracking an ended trip.
Make recent-route writes non-blocking/non-critical, and give the active-trip write its own recovery path, such as returning to MainActivity.onResume()’s active-trip restore state.
🤖 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
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripRepository.kt`
around lines 18 - 43, Update TripRepository.start and end so successful server
operations are not converted to failure by subsequent DataStore errors. Make
addRecentRoute non-blocking/non-critical, and give saveActiveTrip and
clearActiveTrip dedicated recovery handling that preserves the server-success
result and lets MainActivity.onResume restore or reconcile active-trip state.
Continue propagating CancellationException and mapping genuine API failures as
before.
Comment on lines
+76
to
+81
| override suspend fun addRecentRoute(routeId: String) { | ||
| val cleaned = routeId.replace("|", "") | ||
| context.tripStateDataStore.edit { prefs -> | ||
| val current = prefs[Keys.RECENT_ROUTES]?.split("|")?.filter { it.isNotEmpty() } ?: emptyList() | ||
| val updated = (listOf(cleaned) + current.filter { it != cleaned }).take(5) | ||
| prefs[Keys.RECENT_ROUTES] = updated.joinToString("|") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt --items all
rg -n -C 3 'addRecentRoute|recentRoutes|replace\("\\|"' \
android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt \
android/app/src/test/kotlin/org/onebusaway/vehicletracker/data/Fakes.kt \
android/app/src/test/kotlin/org/onebusaway/vehicletracker/data/RepositoriesTest.ktRepository: OneBusAway/vehicle-positions
Length of output: 22003
Preserve route IDs when persisting recent routes.
addRecentRoute() removes | from route IDs, so a valid route such as A|B is stored and later returned as AB. Store a serialized List<String> instead of joining/splitting on |, update FakeTripStateStore to avoid deleting or altering route IDs, and add a trip-start test with a route containing |.
🤖 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
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/data/TripStateStore.kt`
around lines 76 - 81, Update addRecentRoute() and the corresponding recent-route
read logic to persist RECENT_ROUTES as a serialized List<String> rather than a
pipe-delimited string, preserving route IDs exactly—including embedded “|”
characters—while retaining deduplication and the five-route limit. Align
FakeTripStateStore with this behavior without sanitizing route IDs, and add a
trip-start test covering a route containing “|”.
Comment on lines
+70
to
+79
| private fun ensureSeeded() { | ||
| if (seeded) return | ||
| synchronized(seedLock) { | ||
| if (seeded) return | ||
| runCatching { runBlocking { sessionStore.session.first() } } | ||
| .onSuccess { session -> | ||
| token = session.token | ||
| serverUrl = session.serverUrl | ||
| } | ||
| seeded = true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline android/app/src/main/kotlin/org/onebusaway/vehicletracker/di/AppModule.kt --items all
rg -n -C 4 '\brunBlocking\b|ensureSeeded\s*\(|\.api\s*\(' \
android/app/src/main/kotlin \
android/app/src/test/kotlinRepository: OneBusAway/vehicle-positions
Length of output: 9372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AppModule relevant section =="
sed -n '35,135p' android/app/src/main/kotlin/org/onebusaway/vehicletracker/di/AppModule.kt | cat -n | sed 's/^/ /'
echo
echo "== ApiHolder usages across repository/data files =="
rg -n -C 3 'ApiHolder|provideApiProvider|providesApiProvider|api\(\)|TrackerApiProvider|injectApiHolder' android/app/src/main/kotlin/org/onebusaway/vehicletracker
echo
echo "== TrackerApiProvider usages =="
rg -n -C 3 'TrackerApiProvider\s|apiProvider\s|apiProvider' android/app/src/main/kotlin/org/onebusaway/vehicletracker android/app/src/test/kotlin
echo
echo "== runBlocking occurrences with relevant context =="
rg -n -C 5 'runBlocking' android/app/src/main/kotlin android/app/src/test/kotlinRepository: OneBusAway/vehicle-positions
Length of output: 38629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SessionStore implementation =="
fd -a 'SessionStore|DataStoreSessionStore' android/app/src/main/kotlin android/app/src/test/kotlin | sed -n '1,20p'
rg -n -C 5 'interface SessionStore|class DataStoreSessionStore|override val session|fun.*session|DataStore.*session|session.*DataStore' android/app/src/main/kotlin/org/onebusaway/vehicletracker/data android/app/src/test/kotlin
echo
echo "== Repository call sites in UI/service =="
rg -n -C 4 'VehicleRepository|MyVehicles|start\(|end\(|reportTrack|apiProvider\.get\(\)|TrackerApiProvider|TripReporter\(' android/app/src/main/kotlin/org/onebusaway/vehicletracker
echo
echo "== Suspend context around repository resolution =="
rg -n -C 6 'myVehicles\(\)|start\(|end\(\)|getLocationReport|report\(|reportTrack\(' android/app/src/main/kotlin/org/onebusaway/vehicletracker -g '*.kt'
echo
echo "== AndroidManifest service lifecycle =="
fd -a 'AndroidManifest.xml' android/app/src/main | xargs rg -n -C 4 'LocationTrackingService|ServiceController|android:process|android:exported|startService|bindService' || trueRepository: OneBusAway/vehicle-positions
Length of output: 48577
Do not block during API cache seeding.
api() can run before SessionStore.session emits and reaches runBlocking { sessionStore.session.first() }; VehicleRepository.myVehicles() wraps this call but does not change the blocking or DataStore main-thread path. Seed the API cache asynchronously or expose readiness separately so DataStore initialization cannot block the API resolution caller.
🤖 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 `@android/app/src/main/kotlin/org/onebusaway/vehicletracker/di/AppModule.kt`
around lines 70 - 79, Update ensureSeeded() so API cache seeding does not call
runBlocking or synchronously await sessionStore.session.first() during api()
resolution. Launch the session read asynchronously while preserving token and
serverUrl assignment and ensuring concurrent callers do not start duplicate
seeding; expose or use readiness separately if needed without blocking the
caller.
Comment on lines
+35
to
+40
| LaunchedEffect(state) { | ||
| val loaded = state as? VehiclesUiState.Loaded | ||
| if (loaded != null && loaded.vehicles.size == 1) { | ||
| onVehicleSelected(loaded.vehicles.first().id) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The single-vehicle auto-select traps the user on back navigation.
LaunchedEffect(state) calls onVehicleSelected whenever the state is Loaded with exactly one vehicle. onVehicleSelected navigates to the trip setup route. If the user presses Back from trip setup before starting a trip, VehicleScreen re-enters composition, the ViewModel state is unchanged, and the effect runs again. The app navigates forward immediately, so the user cannot stay on the vehicle screen.
Make the auto-select fire once. Guard it with a rememberSaveable flag, or expose it from VehicleViewModel as a one-shot event that the ViewModel marks as consumed.
🐛 Sketch of the guard
+ var autoSelected by rememberSaveable { mutableStateOf(false) }
LaunchedEffect(state) {
val loaded = state as? VehiclesUiState.Loaded
- if (loaded != null && loaded.vehicles.size == 1) {
+ if (!autoSelected && loaded != null && loaded.vehicles.size == 1) {
+ autoSelected = true
onVehicleSelected(loaded.vehicles.first().id)
}
}📝 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
| LaunchedEffect(state) { | |
| val loaded = state as? VehiclesUiState.Loaded | |
| if (loaded != null && loaded.vehicles.size == 1) { | |
| onVehicleSelected(loaded.vehicles.first().id) | |
| } | |
| } | |
| var autoSelected by rememberSaveable { mutableStateOf(false) } | |
| LaunchedEffect(state) { | |
| val loaded = state as? VehiclesUiState.Loaded | |
| if (!autoSelected && loaded != null && loaded.vehicles.size == 1) { | |
| autoSelected = true | |
| onVehicleSelected(loaded.vehicles.first().id) | |
| } | |
| } |
🤖 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
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleScreen.kt`
around lines 35 - 40, Update the single-vehicle auto-selection logic in
VehicleScreen’s LaunchedEffect(state) so it executes only once across
recomposition and back-navigation, using a rememberSaveable guard or a consumed
one-shot event from VehicleViewModel. Preserve selection for the initially
loaded single-vehicle state while preventing repeated navigation after returning
from trip setup.
Comment on lines
+40
to
+43
| onFailure = { error -> | ||
| val retryable = error !is ApiError.Unauthorized | ||
| _uiState.value = VehiclesUiState.Error(retry = retryable) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
An unauthorized vehicle load leaves the user with no recovery path.
ApiError.Unauthorized maps to Error(retry = false). VehicleScreen then renders only vehicles_error_message and no button, because state.retry is false. The screen exposes no re-login callback, so the user cannot reach the login screen.
The local session can still look valid: AppNavViewModel routes to ROUTE_VEHICLES when session.hasFreshToken(clock()) is true. If the server rejects the token while the local expiry check passes, a restart returns to the same dead end. Surface a distinct unauthorized state and navigate to the login route, or clear the session so the next launch routes to login.
🤖 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
`@android/app/src/main/kotlin/org/onebusaway/vehicletracker/ui/vehicles/VehicleViewModel.kt`
around lines 40 - 43, Update the unauthorized handling in the vehicle-loading
flow around the ViewModel’s onFailure callback so ApiError.Unauthorized does not
produce the non-retryable generic VehiclesUiState.Error dead end. Surface a
distinct unauthorized state that VehicleScreen can use to navigate to login, or
clear the session so AppNavViewModel routes to the login screen on the next
launch; preserve retry behavior for other errors.
Comment on lines
+20
to
+29
| ### Tools | ||
|
|
||
| - Docker + Docker Compose (to run Postgres + the server) | ||
| - `curl` | ||
| - `psql` (or `docker compose exec db psql`, used below — no local Postgres | ||
| client install required) | ||
| - Android Studio with an emulator image (API 26+; API 35 was used for the | ||
| reference run), **or** a physical Android 8.0+ device on the same network | ||
| as the server | ||
| - `adb` on your `PATH` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add Python 3 to the prerequisites.
The token extraction commands at Line 88 and Line 135 invoke python3, but the Tools list does not require Python 3. A clean environment following this guide will fail at both login steps. Add Python 3 to the Tools list, or replace the extraction command with a listed tool.
🤖 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 `@docs/android-smoke-test.md` around lines 20 - 29, Add Python 3 to the Tools
prerequisites list in the smoke-test documentation, since the token extraction
commands use python3. Keep the existing extraction commands unchanged.
Comment on lines
+157
to
+163
| The app talks to the server at whatever URL you enter on the login screen. | ||
| From an emulator, the host machine's `localhost:8080` is reachable at | ||
| **`http://10.0.2.2:8080`**. From a physical device on the same LAN, use the | ||
| host machine's LAN IP instead. Debug builds ship a network-security config | ||
| that permits cleartext HTTP to `10.0.2.2` / `localhost` / `127.0.0.1` only | ||
| (`android/app/src/debug/`) — login over plain HTTP to any other host, or from | ||
| a release build, will be blocked by Android's default cleartext policy. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the physical-device cleartext instructions.
The guide directs physical-device users to use the host LAN IP. It also states that debug cleartext is allowed only for 10.0.2.2, localhost, and 127.0.0.1. A LAN IP is not in that allowlist, so the documented physical-device login path will fail. Document HTTPS for physical devices, or update the debug network-security configuration and this example to allow the intended development host.
🤖 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 `@docs/android-smoke-test.md` around lines 157 - 163, Update the
physical-device guidance near the Android connectivity instructions so it uses
HTTPS, or consistently extend the debug network-security configuration and its
documentation to allow the intended LAN development host. Ensure the documented
login path matches the actual cleartext hosts permitted by the debug
configuration.
Comment on lines
+32
to
+40
| ``` | ||
| android/ # Gradle root (open this in Android Studio) | ||
| app/ | ||
| src/main/kotlin/org/onebusaway/vehicletracker/ | ||
| ui/ # Compose screens + ViewModels (login, vehicles, trip, tracking) | ||
| data/ # Repositories, Retrofit API, DTOs, DataStore prefs | ||
| service/ # LocationTrackingService + notification | ||
| di/ # Hilt modules | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint reports MD040 for this block. Use text for the directory tree.
📝 Proposed fix
-```
+```text
android/ # Gradle root (open this in Android Studio)📝 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
| ``` | |
| android/ # Gradle root (open this in Android Studio) | |
| app/ | |
| src/main/kotlin/org/onebusaway/vehicletracker/ | |
| ui/ # Compose screens + ViewModels (login, vehicles, trip, tracking) | |
| data/ # Repositories, Retrofit API, DTOs, DataStore prefs | |
| service/ # LocationTrackingService + notification | |
| di/ # Hilt modules | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/superpowers/specs/2026-08-04-android-driver-app-design.md` around lines
32 - 40, Update the fenced directory-tree block in the Android project structure
documentation to declare the `text` language, preserving the existing tree
content.
Source: Linters/SAST tools
diveshpatil9104
added a commit
to diveshpatil9104/vehicle-positions
that referenced
this pull request
Aug 13, 2026
…ebase Rebased onto 08cc246. Two routes landed on main since the approval and the guard failed on both, which is the guard doing its job: GET /api/v1/admin/vehicles/{vehicleID}/locations (OneBusAway#86) GET /api/v1/vehicles (OneBusAway#89) The review note only mentioned the first. OneBusAway#89 merged two days later and added the second, a driver-facing listing wrapped in authMiddleware but not adminMiddleware — so it documents 401 without 403, and the auth guard enforces that difference rather than leaving it to review. Location history is documented from location_history_handlers.go: the from/to/limit/format query parameters, the 24h default window that hangs off `to` rather than now, has_more derived from reading one row past the limit, the 404 for an unknown vehicle, and both the JSON and text/csv representations including the CSV header and the formula-injection prefix on trip_id. LocationHistoryEntry marks bearing, speed, and accuracy required and nullable, matching the Go struct's pointers without omitempty — a bearing of 0 is due north, not a missing reading. limit's bounds moved into a named HistoryLimit schema so the constants guard has a stable address, and it now pins maxHistoryLimit and defaultHistoryLimit alongside the vehicle-id and field-length constants. Verified by mutation, as with the others. AssignmentVehicleIDPath is renamed VehicleIDPathNamed, since location history spells its path variable {vehicleID} too and the component is no longer assignment-specific. Docs and tests only; no production Go changed.
diveshpatil9104
added a commit
to diveshpatil9104/vehicle-positions
that referenced
this pull request
Sep 3, 2026
…ebase Rebased onto 08cc246. Two routes landed on main since the approval and the guard failed on both, which is the guard doing its job: GET /api/v1/admin/vehicles/{vehicleID}/locations (OneBusAway#86) GET /api/v1/vehicles (OneBusAway#89) The review note only mentioned the first. OneBusAway#89 merged two days later and added the second, a driver-facing listing wrapped in authMiddleware but not adminMiddleware — so it documents 401 without 403, and the auth guard enforces that difference rather than leaving it to review. Location history is documented from location_history_handlers.go: the from/to/limit/format query parameters, the 24h default window that hangs off `to` rather than now, has_more derived from reading one row past the limit, the 404 for an unknown vehicle, and both the JSON and text/csv representations including the CSV header and the formula-injection prefix on trip_id. LocationHistoryEntry marks bearing, speed, and accuracy required and nullable, matching the Go struct's pointers without omitempty — a bearing of 0 is due north, not a missing reading. limit's bounds moved into a named HistoryLimit schema so the constants guard has a stable address, and it now pins maxHistoryLimit and defaultHistoryLimit alongside the vehicle-id and field-length constants. Verified by mutation, as with the others. AssignmentVehicleIDPath is renamed VehicleIDPathNamed, since location history spells its path variable {vehicleID} too and the component is no longer assignment-specific. Docs and tests only; no production Go changed.
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
Implements the Android driver tracking app from the approved design spec (
docs/superpowers/specs/2026-08-04-android-driver-app-design.md), plus the one server-side addition it needs.Server (Go)
GET /api/v1/vehiclesreturning the calling driver's assigned, active vehicles (new sqlc query +DriverVehicleListerstore method + handler + route-wiring tests)Android app (
android/)LocationTrackingService: foregroundlocationservice, 10s fused updates, START_STICKY with DataStore trip rehydration, degraded "tap to resume" path when background-location is deniedTripReporterstatus machine: green/red states for network, GPS, auth-expiry, and clock-skew (±5 min server rejection surfaced distinctly); 429s dropped silently; bearing/speed clamped to the server contractDisallowUnknownFieldsvalidationstrings.xml, light/dark themes, edge-to-edge insetsTests & CI
android.ymlworkflow: assembleDebug, unit tests, assembleRelease (R8)docs/android-smoke-test.mdDocs
docs/superpowers/JWT_SECRETdocker-compose override recipeTest plan
go test ./...green (with local Postgres)./gradlew :app:testDebugUnitTest33/33./gradlew :app:assembleDebug :app:assembleReleasegreen./gradlew :app:connectedDebugAndroidTest3/3 on API 35 emulatordocs/android-smoke-test.mdSummary by CodeRabbit
New Features
Documentation
Tests
Chores