feat(android): add project foundation

[Vedd-Patel]

Summary

This PR lays the foundational Android project scaffold for the driver companion app (Issue #36). It includes the build system setup, dependency catalog, core architecture, and all supporting infrastructure needed for subsequent PRs to build upon.

What's Included

Build System

• Gradle build scaffold with version catalog (libs.versions.toml)
• AGP 8.x + Kotlin 2.x + KSP configuration
• buildConfig = true for BASE_URL injection via local.properties

AndroidManifest

• Location permissions (ACCESS_FINE_LOCATION, ACCESS_COARSE_LOCATION, ACCESS_BACKGROUND_LOCATION)
• FOREGROUND_SERVICE_LOCATION for Android 14+
• POST_NOTIFICATIONS for Android 13+
• usesCleartextTraffic="false" - HTTPS enforced in production

Architecture

MVVM + Repository pattern with Hilt DI, per project README:

• Data layer - Room entities + DAO, Retrofit API service, VehicleRepository
• DI modules - AppModule (Retrofit + OkHttp), DatabaseModule (Room)
• Utility classes - TokenManager (EncryptedSharedPreferences + Android Keystore), ShiftStateManager (DataStore), LocationStateHolder (singleton location cache)

Security

• JWT stored in EncryptedSharedPreferences backed by Android Keystore - never logged
• BASE_URL injected via BuildConfig from local.properties - not hardcoded

Testing

• Added foundational unit tests for core Android utility classes (e.g., LocationStateHolder, ServiceEventBus, ShiftStateManager).

How to Build Locally

1. Copy local.properties.example → local.properties
2. Fill in sdk.dir, MAPS_API_KEY, and BASE_URL
3. Run ./gradlew assembleDebug

Notes

• local.properties is gitignored - see local.properties.example for required fields
• JWT injection is handled by the login flow (separate PR). For local testing, obtain a JWT via POST /api/v1/auth/login and temporarily set it via TokenManager.saveToken()
• This PR contains no UI - subsequent PRs (Screen 1, Screen 2 & 3) build on top of this foundation

Related

• Closes Android: Driver post-login experience — vehicle selection & active tracking #36 (partially - foundation only)
• Subsequent PRs: Screen 1 (empty map + permissions), Screen 2 & 3 (vehicle setup + active tracking)

[aaronbrethorst]

Ved, the architecture choices here are strong — MVVM + Repository with Hilt DI, EncryptedSharedPreferences for token storage, DataStore for shift state, and a clean separation between local (Room) and remote (Retrofit) data layers. The network security config enforcing HTTPS-only, the FOREGROUND_SERVICE_LOCATION declaration for Android 14+, and the BuildConfig.BASE_URL injection pattern all show careful attention to production readiness. There are a few things that need to be fixed before this can merge.

Critical

1. Five empty (0-byte) files make the project un-buildable (MainActivity.kt, LocationForegroundService.kt, ActiveTrackingScreen.kt, HomeScreen.kt, HomeViewModel.kt). The manifest declares MainActivity as the launcher activity and LocationForegroundService as a foreground service, but both files are completely empty — no package declaration, no class definition. The app will not compile at this commit. A foundation PR should leave the project in a buildable state. Please either add minimal stub implementations (e.g. an empty ComponentActivity subclass) or remove the empty files and their manifest references, adding them in the subsequent screen PRs.

2. build.gradle.kts crashes on fresh clones without local.properties (build.gradle.kts:3-5). The load(rootProject.file("local.properties").inputStream()) call will throw FileNotFoundException if the file doesn't exist. CI environments and fresh clones won't have this file. Please guard the load:

   val localProperties = Properties().apply {
       val file = rootProject.file("local.properties")
       if (file.exists()) load(file.inputStream())
   }

Important

3. HTTP body logging in debug contradicts the security comment (AppModule.kt:23-31). The comment says "avoid leaking JWTs or GPS coordinates into logcat", but Level.BODY in debug mode logs the full Authorization: Bearer <token> header and all request/response bodies including GPS coordinates. For local debugging this is common, but the comment creates a false sense of security. Either downgrade to Level.HEADERS and redact the Authorization header, or update the comment to honestly say "debug builds log everything including credentials — do not use debug builds with production tokens."

4. MasterKeys API is deprecated (TokenManager.kt:25). MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC) is deprecated in favor of MasterKey.Builder:

   private val masterKey = MasterKey.Builder(context)
       .setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
       .build()

   This will generate a compile warning and the deprecated API may be removed in a future AndroidX release.

5. No tests for utility classes with real logic (TokenManager, ShiftStateManager, LocationStateHolder, ServiceEventBus). These classes manage security credentials, shift lifecycle state, and inter-component communication — all critical infrastructure. LocationStateHolder and ServiceEventBus are pure Kotlin with no Android dependencies and could easily have JVM unit tests. ShiftStateManager could use a fake DataStore. I understand this is a foundation PR, but untested infrastructure tends to stay untested. At minimum, LocationStateHolder and ServiceEventBus should have unit tests since they're trivial to test.

Suggestions

6. postLocation silently succeeds when there's no token (VehicleRepository.kt:53-56). Returning Result.success(Unit) when the JWT is null means the caller has no way to know that location data was silently dropped. This may be intentional for the foundation phase, but consider returning Result.failure so the foreground service (when implemented) can surface this to the driver or attempt a token refresh.

7. RefreshTokenResponse field names may not match the server (LocationRequest.kt:19-22). The response uses @SerializedName("token") and @SerializedName("refresh_token"), but the current server returns {"token": "..."} from login (no refresh_token field). If this targets the refresh token PR (#62), that PR changes the login response to {"access_token": "...", "refresh_token": "..."} — note access_token, not token. Make sure the client and server agree on field names before the integration PR.

Strengths

• Security-conscious architecture: EncryptedSharedPreferences for JWT storage, HTTPS enforcement via network security config, cleartext traffic disabled, and logging disabled in release builds.
• Clean dependency injection: Hilt modules are well-scoped — AppModule handles networking, DatabaseModule handles persistence, and both provide singletons through the standard @Provides pattern.
• Thoughtful local data model: The VehicleEntity with favorites and recents, the VehicleDao with capped recents (limit 10), and the search query that floats favorites to the top show good UX thinking at the data layer.
• DataStore for shift state: Using Jetpack DataStore instead of SharedPreferences for the shift lifecycle is the modern, correct choice — it's coroutine-safe and handles concurrent writes properly.

Recommended Action

Request changes. The empty files and the local.properties crash are the blockers — the app must compile at every commit. The deprecated API and the logging concern should also be addressed.

[aaronbrethorst]

Ved, nice work addressing the feedback from the first round — the local.properties guard, the MasterKey.Builder migration, the HEADERS-only logging, the stub implementations, and the postLocation token-null fix all look good. The architecture here is solid: clean MVVM + Repository with Hilt DI, well-scoped modules, and thoughtful data layer design (favorites floated to top, capped recents). A few more things to address before this can merge.

Critical Issues (4 found)

1. refreshToken() API return type prevents HTTP error inspection — ApiService.refreshToken() returns RefreshTokenResponse directly instead of Response<RefreshTokenResponse>. This means Retrofit throws HttpException on any non-2xx response, and the catch block in VehicleRepository.refreshToken() can't distinguish a 401 (user must re-login) from a 500 (transient, should retry). On a non-JSON error body (HTML 502 from a gateway), Gson will throw JsonSyntaxException before the code can inspect the HTTP status. Compare with postLocation, which correctly uses Response<Unit>. Fix: change to Response<RefreshTokenResponse> and handle status codes explicitly. [ApiService.kt:18-19]

2. LocationRequest sends non-nullable fields where the server expects optional — The Go server defines bearing, speed, and accuracy as pointer types (*float64) with omitempty. The Android model declares them as non-nullable Float, so they always serialize — including 0.0f for bearing, which means "due north" rather than "unknown." Fix: make these Float? so Gson omits nulls, matching the server's omitempty behavior. [LocationRequest.kt:9-11]

3. LocationRequest is missing trip_id — The server's LocationReport struct includes trip_id. GTFS-RT vehicle positions are associated with trips, and this foundation model should include the field as nullable so future PRs can populate it: @SerializedName("trip_id") val tripId: String? = null. [LocationRequest.kt]

4. Both postLocation() and refreshToken() catch CancellationException — The catch (e: Exception) blocks in both methods will intercept kotlin.coroutines.cancellation.CancellationException, breaking structured concurrency. When a coroutine scope is cancelled (e.g., ViewModel cleared on navigation), the cancellation won't propagate, causing coroutine leaks. Fix: add catch (e: CancellationException) { throw e } before each generic catch. [VehicleRepository.kt:84, VehicleRepository.kt:99]

Important Issues (4 found)

5. Backup rules don't exclude encrypted preferences or DataStore — The manifest sets allowBackup="true", but neither backup_rules.xml nor data_extraction_rules.xml excludes secure_prefs or shift_prefs. When Android restores a backup to a new device, the Keystore keys are different, so EncryptedSharedPreferences will throw an unrecoverable GeneralSecurityException on first access, crashing the app. Fix: add <exclude domain="sharedpref" path="secure_prefs.xml"/> and <exclude domain="file" path="datastore/shift_prefs.preferences_pb"/> to both backup rule files. [backup_rules.xml, data_extraction_rules.xml]

6. ServiceEventBus.tryEmit() return value is ignored — tryEmit() returns a Boolean indicating success, but it's discarded. With extraBufferCapacity = 4, the 5th unprocessed event is silently dropped. The two event types (NavigateToLogin, LocationPermissionRevoked) are critical navigation/security events — dropping them means the driver continues operating with an expired token or without location permissions. Fix: at minimum, log when tryEmit returns false. [ServiceEventBus.kt:24-26]

7. ActiveTrackingScreen.kt contains a ViewModel, not a Screen — The file is named ActiveTrackingScreen.kt but contains class ActiveTrackingViewModel. This breaks the naming convention used in ui/home/ and ui/vehiclesetup/ (separate Screen and ViewModel files). Fix: rename to ActiveTrackingViewModel.kt. [ActiveTrackingScreen.kt]

8. LocationStateHolderTest has a copy-paste bug — The second test (hasLocation returns false when no location set) asserts assertNull(holder.lastLocation.value) instead of actually calling hasLocation(). It's identical to the first test. Fix: assertFalse(holder.hasLocation()). [LocationStateHolderTest.kt:16-18]

Suggestions (3 found)

9. 429 rate-limit handling returns Result.success(Unit) — The caller has no signal that the location was dropped, so it won't implement backoff and will keep posting at the same cadence (making rate limiting worse). Consider returning Result.failure with a recognizable exception type so the future foreground service can delay its next POST. [VehicleRepository.kt:70-73]

10. TokenManager constructor-time Keystore init can crash the app — MasterKey.Builder.build() and EncryptedSharedPreferences.create() run during Hilt DI. If the Android Keystore is corrupted (a well-documented issue on many devices), the app will crash on startup with an opaque Hilt injection error. Consider lazy initialization with a try-catch and Keystore recovery fallback. [TokenManager.kt:25-35]

11. ServiceEventBusTest only tests construction — The single test verifies assertNotNull(bus), which has no behavioral value. Consider replacing with a test that verifies emit/collect actually works — it's trivial with runTest and Turbine. [ServiceEventBusTest.kt]

Strengths

• Responsive to feedback: Every item from the first review was addressed thoughtfully — stub implementations, guarded property loading, deprecated API migration, logging downgrade, token-null failure propagation, and field name alignment.
• Clean architecture: MVVM + Repository with Hilt DI is well-scoped — AppModule for networking, DatabaseModule for persistence, both providing singletons through standard @Provides.
• Security-conscious design: EncryptedSharedPreferences for JWT storage, HTTPS enforcement via network security config, cleartext disabled, token values never logged.
• Thoughtful data layer: VehicleDao with capped recents (limit 10), favorites floated to top in search, and upsert semantics in saveVehicleToRecents.
• Well-structured VehicleRepository: postLocation() handles distinct HTTP status codes with appropriate logging and avoids logging PII.

Recommended Action

1. Fix critical issues 1–4 (API contract alignment, CancellationException)
2. Address important issues 5–8 (backup rules, event bus, file naming, test bug)
3. Consider suggestions 9–11
4. Re-run review after fixes

[aaronbrethorst]

Ved, this is in great shape and the responsiveness across rounds has been excellent — every one of the eleven items from the last review is correctly resolved: refreshToken() now returns Response<RefreshTokenResponse>, bearing/speed/accuracy are nullable to match the server's omitempty, trip_id is present, CancellationException is rethrown in both repo methods, the backup/data-extraction rules exclude secure_prefs and the DataStore file, the ServiceEventBus drop is logged, the ActiveTracking files are split correctly, the LocationStateHolderTest copy-paste bug is fixed, and the Keystore init is now lazy with a recovery fallback. The architecture remains a strength.

A few things still need attention before this merges — one of them is a security item from the first round that was only half-addressed.

Critical Issues (1 found)

1. The JWT bearer token is still logged to logcat in debug builds [di/AppModule.kt:23-28]. Round one asked you to "downgrade to Level.HEADERS and redact the Authorization header." The downgrade happened, but the redaction didn't — HttpLoggingInterceptor.Level.HEADERS logs all request headers, including Authorization: Bearer <jwt>. So every debug build still writes the full token to logcat. Worse, the comment ("BODY level logs Authorization headers… use HEADERS only") now implies HEADERS keeps the token out of logs, which is false — it only stops the GPS body from being logged. There is no redactHeader call anywhere in android/. Fix:

   val logging = HttpLoggingInterceptor().apply {
       level = if (BuildConfig.DEBUG) HttpLoggingInterceptor.Level.HEADERS
       else HttpLoggingInterceptor.Level.NONE
       redactHeader("Authorization")
   }

   …and correct the comment to say HEADERS still logs the Authorization header unless redacted.

Important Issues (3 found)

2. local.properties-less release builds silently ship a dead placeholder URL [app/build.gradle.kts:30-33]. The fallback "https://your-production-server.com/" is applied in defaultConfig with no release override, and the comment calls it a "production HTTPS URL." A release or CI build on a machine without BASE_URL in local.properties compiles cleanly and then points every network call at a non-existent host — indistinguishable at runtime from "server down." Either fail the build when BASE_URL is unset for the release variant, or default the placeholder only for debug. And fix the comment — it's a placeholder, not a production endpoint.

3. The Keystore recovery path can both silently log the user out and crash at a random call site [util/TokenManager.kt:44-58]. Two concerns: (a) catch (e: Exception) deletes secure_prefs on any exception (disk/IO/SecurityException, not just corruption), wiping the JWT and refresh token with no signal — the driver is silently logged out and postLocation just starts returning failures. (b) The retry block (the second MasterKey.Builder + EncryptedSharedPreferences.create) is itself unguarded inside the by lazy {}; a genuinely broken Keystore throws again and propagates out of lazy init at whatever call site touched prefs (e.g. inside postLocation on the IO dispatcher), producing a crash far from the cause. Consider narrowing the catch, wrapping the retry so a second failure surfaces an explicit error, and emitting ServiceEventBus.emitNavigateToLogin() so a wiped session becomes a visible, recoverable event.

4. refreshToken() field names contradict the only auth response the server actually produces [data/remote/LocationRequest.kt:20-22, data/remote/ApiService.kt:16-19]. The server today exposes only POST /api/v1/auth/login returning {"token": ...} — there's no /api/v1/auth/refresh route and no access_token/refresh_token fields server-side. RefreshTokenResponse expects @SerializedName("access_token") and @SerializedName("refresh_token"). I understand the refresh path is forward-looking (login lands in a later milestone), but the field names should agree with whatever the refresh-token PR (#62) actually returns before this dependent code ships. Please confirm intent or align the names now so the integration PR doesn't inherit a silent mismatch.

Suggestions (4 found)

5. VehicleRepository has the densest logic in the PR and zero tests [data/repository/VehicleRepository.kt]. The util layer is well covered, but postLocation()'s six status branches and refreshToken()'s token-persistence side effects are exactly the contract later milestones will branch on. A plain JVM VehicleRepositoryTest with a mocked ApiService/TokenManager/VehicleDao (no Android runtime needed) covering refresh-success-persists-both-tokens, refresh 401, postLocation null-token, and postLocation 401-vs-429 would lock this down cheaply.

6. Leftover authoring marker in a shipped log line [data/repository/VehicleRepository.kt:63-64]. // No GPS coordinates in logs (PII) (intentionally added emoji) — the PII rationale is worth keeping, but drop the (intentionally added emoji) marker and the ✅ in the log string (no other log line in the file uses emoji).

7. Consider guaranteeing delivery of security events [util/ServiceEventBus.kt:24-28]. NavigateToLogin and LocationPermissionRevoked are dropped (with only a Log.e) if the 4-slot buffer fills. For security-relevant signals, BufferOverflow.SUSPEND with a suspending emit would guarantee delivery. (Also: thiss → this typo on line 26.)

8. Prefer a typed sealed error over string sentinels [data/repository/VehicleRepository.kt]. Result.failure(Exception("401")) / Exception("429: rate limited") work, but the consumer that wires postLocation in will have to string-match e.message to branch 401→refresh vs 429→backoff. A small sealed error type would make that robust.

Strengths

• Thorough follow-through: all eleven items from the prior round are correctly resolved, and the fixes are real fixes, not band-aids — the nullable DTO fields match the Go omitempty semantics, and the backup-rule exclusions are in both files.
• Security-conscious design: EncryptedSharedPreferences-backed token storage, HTTPS enforced via network security config, cleartext disabled, token values never logged in the repository.
• Clean, well-scoped DI and data layer: AppModule/DatabaseModule separation, capped recents, favorites floated to top, upsert semantics in saveVehicleToRecents.
• Good test discipline where tests exist: ServiceEventBusTest correctly uses backgroundScope + UnconfinedTestDispatcher for the hot SharedFlow, and the instrumented TokenManager/ShiftStateManager tests are placed correctly in androidTest.

Recommended Action

1. Fix critical issue 1 (redact the Authorization header + fix the comment) — this is the half-done item from round one.
2. Address important issues 2–4 (release BASE_URL guard, Keystore recovery hardening, refresh field-name alignment).
3. Consider suggestions 5–8.
4. Re-run review after fixes.

Verdict: Request Changes.