Register devices for OBACloud service-alert push notifications (#1957) - #1958
Conversation
OBACloud can deliver service-alert push notifications, but today it only learns a device's FCM token as a side effect of the rider creating a trip alarm. Riders who enable notifications but never set an alarm go unregistered, device locale is never captured (alerts default to English), and tokens age out after 180 days without re-registration. Register the device proactively against the new push_registrations API: - PushRegistrationWebService: form POST register (token, locale as-is, test_device, operating_system=android) and DELETE unregister, on the region sidecar host via @url, mirroring ReminderWebService. - PushRegistrationManager: derives the desired registration from OS notifications-enabled + region + FCM token + locale + a hidden test-device flag, reconciles it against the last-sent registration via a pure decide() (register / re-register / unregister / no-op), and persists the last registration so opt-out can DELETE the right token and redundant POSTs are skipped (30 req/min/IP limit). Driven reactively off the region flow + the token/test-device prefs, plus a ProcessLifecycleOwner ON_START resync to catch OS-permission changes made in system settings. - Hidden "Register as test device" toggle on the Advanced settings screen (OBACloud "Test users only" preview audience). Opt-in is the OS notification permission; there is no separate in-app toggle. decide() is a pure, JVM-tested function (PushRegistrationDecisionTest). Note: the exact way the token is conveyed to the DELETE (form body vs query param) is not pinned down by the issue; it is sent as a form field mirroring register, flagged at the call site to confirm against the OBACloud spec before release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds OBACloud push registration with Retrofit, persisted reconciliation state, lifecycle-triggered synchronization, notification-permission resync, test-device settings, batched preference persistence, and comprehensive tests. ChangesPush registration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Application
participant PushRegistrationManager
participant PushRegistrationStore
participant PushRegistrationClient
participant PushRegistrationWebService
Application->>PushRegistrationManager: start()
PushRegistrationManager->>PushRegistrationStore: read registration state
PushRegistrationManager->>PushRegistrationClient: register or unregister
PushRegistrationClient->>PushRegistrationWebService: POST or DELETE registration
PushRegistrationClient-->>PushRegistrationManager: return success or failure
PushRegistrationManager->>PushRegistrationStore: record or clear successful state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Code review by Claude (Fable 5)Automated review generated with Claude Code. OverviewAdds proactive device registration against OBACloud's new I verified the integration points against the codebase: Strengths
Issues1. In-app notification-permission grant doesn't trigger registration ( 2. is PushRegistrationAction.Reregister -> {
val cleaned = unregister(action.previous)
if (register(action.target)) persist(action.target)
else if (cleaned) clearLast() // else keep `previous` so the next sync retries the full Reregister
}3. DELETE token conveyance — already flagged in the PR body and at the call site; agreeing this is the one hard pre-merge blocker. While verifying against the OBACloud spec, also confirm the boolean form encoding: Minor / nits
Test coverageThe pure decision core is fully covered, and SecurityNothing concerning client-side: the token travels over TLS to the region's sidecar, and "possession of token = ownership" on an unauthenticated endpoint is the server's documented model. The token was already persisted in prefs pre-PR. The VerdictWell-built and convention-faithful; the decision/manager split is the right architecture. Before merge: confirm the DELETE shape (author-flagged), and I'd recommend fixing #1 (in-app permission grant is a missed registration trigger for the feature's primary audience) and #2 (orphaned registration on double failure) — both are small, contained changes. On-device verification of the actual request shapes, as the PR body notes, remains pending. |
The two in-app POST_NOTIFICATIONS requests (destination-reminder and trip-info save) called ActivityCompat.requestPermissions() fire-and-forget — there was no result handler at all. The system permission dialog only pauses the activity, so no ON_STOP->ON_START cycle occurs and PushRegistrationManager's ProcessLifecycleOwner resync never fires. An in-flow opt-in — the highest-value moment for this feature — wasn't picked up until the next app foreground/launch. Convert both sites to a Compose RequestPermission launcher and call PushRegistrationManager.resync() (via the entry point) from the result callback. resync() is a no-op when nothing changed and currentTarget() re-checks areNotificationsEnabled(), so firing on both grant and deny is safe. Drop the now-unused ActivityCompat / NOTIFICATION_PERMISSION_REQUEST / PermissionUtils imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The resync-on-grant fix had duplicated the RequestPermission launcher, its resync callback, the explanatory comment, and the API-33 gate across both opt-in sites (DestinationReminder, TripDestinations) — copy-paste that would silently reintroduce the fire-and-forget bug at any future opt-in site. Factor it into rememberNotificationPermissionRequest() in ui.compose (beside findActivity), returning a () -> Unit that requests POST_NOTIFICATIONS, gates on API 33 internally, and resyncs the push registration on the result. Both sites now call it unconditionally. Also delete the now-orphaned PermissionUtils.NOTIFICATION_PERMISSION_REQUEST constant and prune the imports the extraction freed up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In the Reregister arm of sync(), unregister(previous)'s result was discarded and clearLast() ran whenever the POST failed. If both the DELETE and the POST failed (e.g. offline during a region switch), the still-registered `previous` was forgotten: the next sync saw last == null, did a plain Register, and the old region/token was never DELETEd — it keeps receiving that region's alerts until the 180-day age-out. Capture the DELETE result and only clearLast() when the DELETE actually removed `previous`; otherwise keep `previous` on record so the next sync retries the full Reregister (DELETE + POST). Persisting the new record still happens only once the POST lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The originating issue (#1957) is the authoritative OBACloud contract (its author is the obacloud maintainer), and it resolves both open questions the PR flagged: - test_device is documented as the literal true/false, upserted on every call (an omitted value resets it to false) — so the existing Boolean @field is correct as-is; no change needed there. - unregister is DELETE …/push_registrations with a `token` param. The contract doesn't pin body-vs-query, but a query param avoids sending a body on a DELETE (which some HTTP stacks/proxies drop) and a Rails `params` backend reads it either way. Switch @Field-in-body to @query("token") and drop the now-needless @FormUrlEncoded / hasBody on that call. Also replace the "verify before release" doc comments with the confirmed contract details. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Rename the package-level decide() to decidePushRegistration() so call sites outside PushRegistrationDecision.kt (the manager, the tests) carry context; the bare name read as too generic. Updated the one main call site, the eight test call sites, and the KDoc references. - Make PushRegistrationManager.sync() private — nothing outside the class calls it (start()'s collector and resync() are the only callers), so resync() is the sole public trigger. Left as-is per review: no retry/backoff (foreground/pref-change triggers give an acceptable eventual-consistency story) and loadLast()'s unreachable regionId=-1 / locale="" defaults (they self-heal via inequality → refresh). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…M test The pure decision (decidePushRegistration) was well covered, but the manager that applies it — which endpoint calls fire and, crucially, what the persisted "last registration" record becomes after each success/failure — had no test. That's the layer where the Reregister-orphan bug lived. Add a small test seam so sync() is JVM-testable without Robolectric: the two Android-only reads (OS notifications-enabled + the endpoint-path string) move behind a delegating @Inject constructor that resolves them from Context, while a package-visible primary constructor takes them as a `() -> Boolean` supplier and a plain String. Every other dependency was already injectable (and FirebaseMessagingManager.userPushId() only touches prefs, so the real object works over a fake). PushRegistrationManagerTest drives sync() through the public resync() trigger over hand fakes (reusing FakePreferencesRepository / FakeRegionRepository), covering: register+persist then no-op, disabled-notifications no-op, opt-out unregister+clear, failed-register no-persist+retry, token-rotation reregister, delete-succeeds/post-fails clears, 404-as-already-gone, and the headline case — a fully-failed Reregister must keep `previous` so the next sync retries the DELETE rather than orphaning the old token. Mutation-checked: reverting the arm to the old `clearLast()` fails exactly that test (expected 2 DELETEs, got 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collapse the ~20 repeated `resync() + advanceUntilIdle()` pairs into a single
Fixture.sync() (Fixture now holds the TestScope), and give the five tests that
open from an already-registered device a Fixture.registered("T1") baseline
instead of copy-pasting the setToken+sync preamble. Also trim the @Inject
constructor's KDoc, which restated the seam rationale already on the primary
constructor's params.
Behaviour-identical; the 8 tests still pass. From /simplify (the reuse,
efficiency, and altitude passes found nothing to change — the two-constructor
seam matches the FocusedTripRepository/StopsForRouteRepository house style).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR review on the Reregister quadrant where the DELETE of the stale registration fails but the POST of the new one succeeds. Previously persist() overwrote the record and the owed DELETE was never retried, so on a region change the old region kept pushing to a still-valid token until the 180-day server prune. - Add a single push_pending_del_* prefs slot: on the DELETE-fail/POST-ok quadrant, persist(target) AND queue `previous`, then drainPendingDelete() retries the owed DELETE at the top of every sync. persist() drops a queued delete whose endpoint matches the just-committed live record, keeping the single slot safe against a return-to-the-old-endpoint. The sync() comment now enumerates all four failure quadrants. - Surface register/unregister failures via a Log.w instead of swallowing them, logging the region + host (never the token). Injected as a logWarning seam so the failure paths stay exercisable from the plain-JVM unit test. - Hoist the endpoint comparison into a shared PushRegistration.sameEndpoint in the pure-data decision file, used by both decidePushRegistration and persist(). - Cover the new behavior with tests: the DELETE-fail/POST-ok quadrant queues and later drains the DELETE, and a return-to-old-token never deletes the live row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esh registrations Device testing against the live OBACloud sidecar turned up two bugs, and a comparison with the iOS client (OneBusAway/onebusaway-ios) turned up two more gaps. All four are fixed here. Bugs found on device: - A test-device registration was rejected with 422 "Description can't be blank". The server requires a `description` field when `test_device=true`, which the contract never sent, so the test-device toggle could not work at all. The requirement landed server-side on 2026-07-19 and postdates the contract table in #1957; it is documented in the iOS client's docs/push-notifications.md. - HTTP error responses were silently swallowed. `runCatching { ... }.onFailure` only fires for thrown exceptions, but a non-2xx is a successful call returning an unsuccessful response, so the 422 produced no log output at all — and because a failed call persists nothing, the same doomed request repeated on every foreground, invisibly. Failures are now branched on explicitly and logged with the status and server error body (never the token, and never the URL: the unregister URL carries the token as a query param). Parity with the iOS client: - Re-register an otherwise-unchanged registration every 24h. Previously we re-posted only when an input changed, so a device whose token, region and locale never change would go untouched until the server's 180-day last_seen_at prune dropped it and alerts silently stopped — defeating the "freshness" goal of #1957. An unknown timestamp counts as stale, so existing records refresh once on upgrade. - Stop mistaking an unresolved region for an opt-out. RegionRepository seeds its region flow to null and fills it asynchronously, so a cold-start sync could DELETE the registration and re-POST it seconds later. Notifications being off is now the only definitive opt-out; unresolved inputs bail and wait. - Take the test-device description from a new "Test device name" setting rather than deriving it from Build.MODEL, which could not distinguish two identical handsets in the admin UI. An unnamed device registers as an ordinary device instead of sending a request the server is guaranteed to reject. - Report server rejections to Crashlytics. Registrations are the server's only audience source for service alerts, so a systematic failure must not be visible only in one developer's logcat. Transport failures stay local. Verified on a Pixel 7 Pro against the live sidecar: first registration, opt-in via both the in-app permission dialog and system settings, opt-out, the test-device round trip with a description (204), the no-op path, and the upgrade refresh.
…sk-best-effort Each prefs slot persists as its own async DataStore edit, so the "token written last" ordering is not guaranteed across a process death mid-persist. The recovery paths don't depend on it — a torn record re-registers via the idempotent upsert, and a stale one DELETEs into a tolerated 404 — but the comments previously implied a durability the writes don't have.
PushRegistrationManager was ~65% plumbing: of its 227 code lines, roughly 100 were prefs slots and commit-sentinel bookkeeping and 65 were HTTP calls and failure classification, leaving only ~90 for the reconciliation it is named for. Two mechanisms move out: - PushRegistrationStore owns the durable record — every push_reg_*/ push_pending_del_* slot, the commit-sentinel semantics and their torn-write reasoning, the refresh timestamp, and the pending-delete queue. - PushRegistrationClient owns talking to the endpoint and making failures visible — URL assembly, which non-2xx codes still count as success (DELETE 404), the redaction rule, and the local-vs-remote reporting split. The manager keeps what is actually policy: the triggers, deriving the desired registration, and the rule for what to record given which of a Reregister's two calls succeeded. That four-quadrant logic deliberately stays inline and is now its own named function — it is the least obvious part of the feature and hiding it behind an abstraction would be worse than the plumbing was. Behaviour is unchanged; the 28 existing tests pass untouched, exercising the real store and client over the same fakes. The manager drops from 227 to 114 code lines and from 9 constructor parameters to 7, with one Android seam left instead of four.
…istrations # Conflicts: # onebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.kt # onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java
The merge conflict resolution kept this request-code constant, but the PR deliberately removed it when it replaced the request-code notification-permission flow with the Compose rememberNotificationPermissionRequest() helper. Both former call sites (DestinationReminder, TripDestinations) now use the helper, so the constant is referenced nowhere and is dead code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
OBACloud's newly published push-notifications documentation (https://developer.onebusaway.org/projects/obacloud) specifies the test-device description as free text of at most 255 characters, server-enforced when test_device=true. The rider-entered name was unbounded, so a long one would 422 — and because a failed registration persists nothing, the doomed POST would repeat on every app foreground against a rate-limited endpoint. Bound it in two places: at the input field (so the limit is felt while typing) and in buildTarget() (so a value stored before that bound existed, or arriving from anywhere else, still cannot reach the wire). EditTextPreferenceItem gains an optional maxLength that defaults to the previous unbounded behaviour. Also refresh two KDoc blocks that described the contract as unresolved. The description requirement was originally established by probing the deployed server and noted as awaiting maintainer confirmation; the DELETE body-vs-query question was noted as unstated. Both are now specified upstream, so cite the documentation instead of the probe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s write Quality cleanup from a four-angle review of the branch. No behaviour change. Derive PushRegistration.testDevice from description instead of storing it. Every construction already set testDevice == (description != null) — the server requires a description exactly when test_device=true — so the separate field bought a redundant preference slot, an extra write and read, and a representable state (flag set, no name) that can only ever be rejected. A computed property makes the rule hold at the type level. Move the 255-char description cap to the settings write boundary, the one place the value enters persistence, per CLAUDE.md's normalize-at-the-boundary rule. It was previously clamped in the shared EditTextPreferenceItem, which gave that component a second validation mechanism competing with its existing reject-on-invalid contract, leaked an OBACloud constant into UI code, and let the stored name differ from the one sent. Extracted as applyPushTestDeviceName alongside applyMapStopCacheSize so it is unit-tested directly; the clamp in buildTarget stays as a cheap guard on the wire boundary itself. Remember the lambda returned by rememberNotificationPermissionRequest. It captured launcher and was reallocated on every recomposition despite the remember* name, so the onSave lambda wrapping it at TripDestinations changed identity each time and TripInfoRoute could never skip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gs legible Review follow-ups on #1958: - sinceLastSent() treats a negative elapsed time (device wall clock set backwards past the record's stamp) as unknown, i.e. stale: the next sync re-POSTs once and restamps at the new clock, instead of the keep-alive staying silent until the clock catches back up — within reach of the server's 180-day prune for a large jump. - clear() drops the sent-at stamp with the record it stamped, so the store can no longer report an elapsed-since-sent for a registration last() says doesn't exist (previously harmless only because the decision layer ignores the stamp when there is no record). - Advanced settings: the test-device name field is enabled only while the toggle is on, and the toggle's summary calls out the one silent state — on but unnamed, where the device keeps registering normally until a name is set.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
onebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.kt (1)
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
@DELETEover@HTTP(method = "DELETE").Since this request does not include a body, you can use the more concise and idiomatic
@DELETEannotation from Retrofit instead of the generic@HTTPannotation.♻️ Proposed refactor
Make sure to add
import retrofit2.http.DELETEif you apply this change:- `@HTTP`(method = "DELETE") + `@DELETE` suspend fun unregister(`@Url` url: String, `@Query`("token") token: String): Response<Unit>🤖 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/api/contract/PushRegistrationWebService.kt` around lines 80 - 81, Update the unregister method annotation to use Retrofit’s `@DELETE` instead of `@HTTP`(method = "DELETE"), and add the retrofit2.http.DELETE import while removing the no-longer-needed generic annotation import.onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt (1)
126-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
pushTestDevice/pushTestDeviceNamepass-through.The baseline snapshot now carries these two new fields, but no test in this file asserts how they map into
AdvancedSettingsUiState(e.g., row visibility or a summary string), unlike the pattern used for other fields (see the "values pass through" test forSettingsPrefSnapshot). Worth a small assertion to lock down the wiring for this new setting.🤖 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/settings/SettingsUiStateTest.kt` around lines 126 - 135, Extend the settings UI state tests using the existing advPrefs fixture to cover pushTestDevice and pushTestDeviceName propagation into AdvancedSettingsUiState. Add assertions for the resulting setting row visibility and summary, following the established “values pass through” test pattern used for SettingsPrefSnapshot.
🤖 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.
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.kt`:
- Around line 80-81: Update the unregister method annotation to use Retrofit’s
`@DELETE` instead of `@HTTP`(method = "DELETE"), and add the retrofit2.http.DELETE
import while removing the no-longer-needed generic annotation import.
In
`@onebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt`:
- Around line 126-135: Extend the settings UI state tests using the existing
advPrefs fixture to cover pushTestDevice and pushTestDeviceName propagation into
AdvancedSettingsUiState. Add assertions for the resulting setting row visibility
and summary, following the established “values pass through” test pattern used
for SettingsPrefSnapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73359b46-ff48-4d2c-95f2-b2d41ba76970
📒 Files selected for processing (24)
gradle/libs.versions.tomlonebusaway-android/build.gradle.ktsonebusaway-android/src/main/java/org/onebusaway/android/api/contract/PushRegistrationWebService.ktonebusaway-android/src/main/java/org/onebusaway/android/app/Application.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/NetworkModule.ktonebusaway-android/src/main/java/org/onebusaway/android/app/di/PushRegistrationEntryPoint.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationDecision.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationManager.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationStore.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/compose/NotificationPermissionRequest.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsScreen.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/SettingsUiState.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/DestinationReminder.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripdetails/TripDestinations.ktonebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.javaonebusaway-android/src/main/res/values/donottranslate.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationDecisionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationManagerTest.ktonebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationStoreTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModelTest.ktonebusaway-android/src/test/java/org/onebusaway/android/ui/settings/SettingsUiStateTest.kt
💤 Files with no reviewable changes (1)
- onebusaway-android/src/main/java/org/onebusaway/android/util/PermissionUtils.java
| // two requests against a rate-limited endpoint and leaving a window with no registration at all. | ||
| val target = if (notificationsEnabled()) { | ||
| // Inputs still settling — bail rather than decide; a later trigger re-runs us. | ||
| buildTarget() ?: return@withLock |
There was a problem hiding this comment.
🔴 Confirmed correctness bug — stale registration leaks when the new region has no sidecar host
When notifications are enabled but buildTarget() returns null because the current region's sidecarBaseUrl is blank, sync() bails via ?: return@withLock here — before decidePushRegistration runs. A null target on this path is treated as "inputs not resolved yet", not opt-out, so a registration stored for a different region is never Unregistered.
Scenario: rider registers in region A (has a sidecar), then switches to region B whose Region.sidecarBaseUrl is blank (the field defaults to ""). On next foreground, notificationsEnabled() is true, buildTarget() returns null at the takeIf { it.isNotBlank() } guard (~line 210), and sync() returns. The region-A registration is never deleted — the device keeps getting region A's alerts and no region-B alerts until the server's 180-day prune.
Consider distinguishing "no sidecar for this region" (which should still reconcile away a stale registration) from "inputs genuinely not ready yet."
There was a problem hiding this comment.
Fixed in 20a051b, and refined in 57c5d39. The null-target bail is gone: deriveDesiredRegistration now returns a sealed DesiredRegistration in which "the region hasn't loaded yet" (Unresolved — freezes reconciliation) and "this region has no sidecar" (NoSidecar — a resolved fact) are distinct types, so a blank sidecarBaseUrl can no longer masquerade as not-ready. decidePushRegistration only accepts resolved states, and NoSidecar with a record on file yields Unregister — your exact scenario (region A → sidecar-less region B) is pinned by the switching to a region without a sidecar unregisters the stale registration test, which asserts the DELETE targets region A's host.
| val base = prefs.getString(KEY_PENDING_DEL_BASE, null) ?: return null | ||
| val token = prefs.getString(KEY_PENDING_DEL_TOKEN, null) ?: return null | ||
| return PushRegistration( | ||
| regionId = prefs.getLong(KEY_PENDING_DEL_REGION_ID, -1L), |
There was a problem hiding this comment.
🔴 Confirmed correctness bug — torn pending-delete write DELETEs against region -1 and treats the 404 as success
pendingDelete() reconstructs regionId with a default of -1L. queuePendingDelete issues three independent async DataStore writes, with the token written last as the presence sentinel — but the sentinel guards record presence, not regionId correctness. If the process is killed after KEY_PENDING_DEL_TOKEN flushes but before KEY_PENDING_DEL_REGION_ID, the next launch reads regionId = -1L, drainPendingDelete issues DELETE …/regions/-1/push_registrations?token=…, the server returns 404, the client counts 404 as success, and clearPendingDelete() runs.
Net effect: the genuinely-stale registration is never deleted and keeps pushing to the still-valid token until the 180-day prune. Consider persisting the pending-delete record atomically (single serialized value) rather than as three separate keys, and/or refusing to drain when regionId < 0.
There was a problem hiding this comment.
Fixed at the root in 8a8f67a by implementing #1978's option 1: PreferencesRepository gained an atomic batch edit {} (one cache swap + one dataStore.edit, so a multi-slot record persists whole or not at all across process death), and every multi-slot store write goes through it. A record whose presence marker exists but whose regionId didn't flush can no longer exist, so no regionId < 0 drain guard was needed — nothing shipped the old layout, so no legacy torn state exists in the wild. A bonus fix fell out: record() folds the same-endpoint pending-delete drop into the same commit, closing the sibling crash window where a fresh registration and a stale queued DELETE for the same endpoint could both survive and delete the row just POSTed.
(Since then, the pending-delete record itself was removed entirely per the over-engineering thread below — but the atomicity fix stands for the six-slot live record, and closes #1978.)
| description = target.description | ||
| ) | ||
| val registered = response.isSuccessful | ||
| if (!registered) reportHttpFailure("register", target, response) |
There was a problem hiding this comment.
🟠 Confirmed correctness concern — Crashlytics retry storm on rate-limited (429) responses
reportHttpFailure records every non-2xx register response to Crashlytics, gating only on response.isSuccessful with no status-code carve-out — contrast unregister() (line ~90), which does carve out 404. The unauthenticated endpoint is rate-limited (30 req/min/IP). On a shared-NAT network (public wifi, carrier CGNAT) a 429 gets reported like a systematic 422. Because a failed register persists nothing, the next foreground retries and reports again, flooding the error channel with transient throttling noise and drowning the genuine systematic-rejection signal the reporting was meant to surface.
Consider treating 429 (and other transient statuses) as non-reportable, mirroring the 404 carve-out already in unregister().
There was a problem hiding this comment.
Fixed in 2812ca2. reportHttpFailure still logs every non-2xx locally, but a new isTransient() (429, plus the 5xx range — a server outage has the same fleet-wide flood shape) skips the Crashlytics report, mirroring the transport-failure split that already existed. Contract-shaped 4xx like the 422 still report. The boolean return is untouched, so the retry-on-next-foreground behavior is identical — tests pin that a 429 logs without reporting and succeeds on the next sync once the throttle lifts.
One deliberate trade worth your eyes: a chronic 5xx (server-side register handler broken indefinitely) is now invisible to Crashlytics too. We judged that acceptable since a fleet-wide 5xx shows up in server-side monitoring, but if you'd rather catch it client-side, a report-after-N-consecutive-failures counter would cover it.
| PushRegistrationAction.NoOp -> Unit | ||
| is PushRegistrationAction.Register -> | ||
| if (client.register(action.target)) store.record(action.target) | ||
| is PushRegistrationAction.Unregister -> |
There was a problem hiding this comment.
🟠 Correctness / contract divergence — Android DELETEs on OS-level opt-out; iOS never does
When notifications are disabled at the OS level, target becomes null, decidePushRegistration returns Unregister(last), and the client DELETEs …/push_registrations?token={token} (plus store.clear()). Issue #1957 explicitly says an OS-level disable doesn't need a DELETE — "FCM bounces feed back to the server and it cleans up unregistered tokens itself" — and the iOS PushRegistrationManager only ever POSTs, never DELETEs.
Beyond the divergence: if the server couples trip-alarm delivery to the push_registration row for that token, this Android-only DELETE can silently drop the delivery target of an already-scheduled alarm when a rider toggles notifications off. Recommend matching the iOS contract — don't DELETE on opt-out; let FCM-bounce/server prune handle it.
There was a problem hiding this comment.
Fixed in 57c5d39, verified against all three sources: #1957's "OS-level disable doesn't need a DELETE", the iOS manager (no DELETE call exists at all), and the iOS docs ("OS-level permission revocation does not require a DELETE"). One nuance: the old DesiredRegistration.None conflated two cases with opposite correct behaviors, so it's split rather than removed — OptedOut (notifications off) is now a pure NoOp that keeps both the server row and the local record (so re-enabling inside the refresh window reads as "unchanged", with no spurious re-POST), while NoSidecar still Unregisters, since your docs sanction exactly that old-row DELETE on a region change. The trip-alarm delivery-target risk you raised is recorded in OptedOut's KDoc as part of the rationale.
| val token = firebaseMessagingManager.userPushId().takeIf { it.isNotEmpty() } ?: return null | ||
| val description = prefs.getString(R.string.preference_key_push_test_device_name, null) | ||
| ?.trim() | ||
| ?.take(PUSH_DESCRIPTION_MAX_LENGTH) |
There was a problem hiding this comment.
🟡 Plausible correctness — take(255) counts UTF-16 code units and can split a surrogate pair
The test-device name cap uses String.take(PUSH_DESCRIPTION_MAX_LENGTH), which counts UTF-16 code units. A description near the boundary containing emoji / astral characters can be truncated mid-surrogate-pair, leaving a lone surrogate (mangling the last char), and the code-unit count may not match the server's char/byte limit.
If the server then 422-rejects, nothing is persisted, so buildTarget rebuilds the identical doomed request and re-POSTs on every foreground — the exact repeat loop the cap was meant to prevent, plus a Crashlytics report each time. Consider capping on Unicode code points / grapheme boundaries, and confirming whether the server limit is chars or bytes.
There was a problem hiding this comment.
Fixed in 6381afa. Both cap sites (the settings persistence boundary and the wire-boundary clamp) now share truncatedToDescriptionCap(), which keeps the 255-UTF-16-unit budget but drops a straddling pair whole — a lone surrogate can no longer be stored or sent. Tests cover the straddling and exact-fit boundary cases.
On your chars-vs-bytes question: we couldn't settle it at the source — the OBACloud repo is private — so the cap deliberately stays at 255 UTF-16 units as the strictest plausible reading of the documented "≤255 chars" (≤N units implies ≤N code points, so it's legal under either counting; only a byte-count limit could differ, and nothing in the docs suggests bytes). Since you have OBACloud access: if the Rails validation counts characters/code points, this could safely loosen to 255 code points later — purely a permissiveness change. Happy to file that as a follow-up if you can confirm.
| }.onFailure { logTransportFailure("unregister", previous, it) }.getOrDefault(false) | ||
|
|
||
| /** `{sidecarBaseUrl}/api/v2/regions/{regionId}/push_registrations` — mirrors the alarms URL build. */ | ||
| private fun registrationUrl(registration: PushRegistration): String = registration.sidecarBaseUrl + |
There was a problem hiding this comment.
🔵 Cleanup (DRY) — sidecar registration-URL assembly duplicates TripInfoRepository.requestAlarm
registrationUrl() hand-concatenates sidecarBaseUrl + arrivals_reminders_api_endpoint + regionId + "/push_registrations", which is the same sidecar-path shape TripInfoRepository.requestAlarm (~line 263) builds for "/alarms". Both independently re-derive the /api/v2/regions/{id} path, so a future change to the sidecar path scheme must be found and edited in two places. Consider extracting one helper (e.g. Region.sidecarUrl(endpoint, suffix)) as the single source of truth for the sidecar URL shape.
There was a problem hiding this comment.
Fixed in 9b6c973 (polished in 7efe9f5): both sites now call sidecarRegionUrl() in api/contract, next to the two sidecar Retrofit interfaces it serves. Output is byte-identical (the test pinning the exact registration URL passes untouched). Two deviations from the Region.sidecarUrl() sketch, both forced: it's not a Region method because the push client addresses DELETEs from its recorded registration, whose region may no longer be current; and the /api/v2/regions/ segment stays a parameter because it lives in the arrivals_reminders_api_endpoint string resource. Open question for you: no flavor overrides that resource today — if the override affordance isn't wanted, the segment could be hardcoded in the helper and the resource deleted, which would also let the client drop its constructor param. (Also noted in the helper's KDoc: the v1 sidecar endpoints — weather, surveys — use a separate regionID-placeholder idiom; this helper owns only the v2 shape.)
| * dropping it — otherwise the old region keeps pushing to a still-valid token until the server's | ||
| * 180-day prune. | ||
| */ | ||
| private suspend fun applyReregister(action: PushRegistrationAction.Reregister) { |
There was a problem hiding this comment.
🔵 Cleanup / over-engineering vs. the ticket + iOS — the whole Reregister + persisted pending-delete machinery
Issue #1957 marks deleting the old region's registration as optional ("optionally DELETE from the old one"), and iOS PushRegistrationManager never DELETEs at all — it re-POSTs and relies on the server's 180-day prune + FCM-bounce cleanup. This branch instead adds a full Reregister action, applyReregister's 4-quadrant handling, three persisted pref slots (KEY_PENDING_DEL_*), the queuePendingDelete/pendingDelete/clearPendingDelete/drainPendingDelete cycle drained on every sync, and a large slice of the manager test — all to robustly recover the one DELETE-fail/POST-ok quadrant of a behavior the sister platform chose not to implement.
That's permanent maintenance + test surface for an optional cleanup the server already performs, and it silently diverges from iOS. Worth confirming with a human whether the DELETE-on-region-change path is wanted at all, or whether matching iOS (POST-only) is the simpler, intended contract.
There was a problem hiding this comment.
Confirmed with a human as requested, and resolved down the middle in 6d8e8e0 (+7efe9f5b): the Reregister DELETE stays, but it's now best-effort — the persisted pending-delete queue (three pref slots, queue/drain on every sync, and its test surface, net −110 lines) is gone. The reasoning: on a region change the old row holds a live token, so nothing bounces and the rider who moved keeps getting the old region's alerts until the 180-day prune — your own iOS docs flag that consequence and sanction the DELETE, so we kept it. The queue, though, only insured the DELETE-fails/POST-lands corner, and its failure mode without the queue (old row lingers until prune) is precisely iOS's baseline for every region change — disproportionate machinery for that corner, exactly as you argued. applyReregister is now the plain two-step composition of the Unregister/Register rules, with the best-effort choice documented on the Reregister action. If you'd rather go full POST-only (drop the region-change DELETE too), that's now a five-line change — but we'd argue the wrong-region-alerts window makes the best-effort DELETE worth keeping.
…ime property Review of #1958 found a stale-registration leak: with notifications enabled, a region whose sidecarBaseUrl is blank (the default) made buildTarget() return null, which sync() read as "inputs not resolved yet" and bailed — so a registration recorded for a previous sidecar region was never unregistered, and the device kept receiving the old region's alerts until the server's 180-day prune. Rather than patching the guard, remove the representation that allowed the bug: buildTarget(): PushRegistration? collapsed three answers ("inputs still settling", "definitively no registration should exist", "this registration should exist") into two values. Desired state is now the sealed DesiredRegistration type — Unresolved, plus Resolved { None, Wanted } — with a pure deriveDesiredRegistration() in which every early exit must name its classification: a null region or missing FCM token is Unresolved (freeze reconciliation; a later trigger re-runs it), while notifications-off or a sidecar-less region is None (reconcile the stale record away). decidePushRegistration() accepts only DesiredRegistration.Resolved, so an unresolved state cannot reach the decision and be mistaken for an opt-out — and None cannot skip it. The original bug now fails to compile in every form. Covered by new derivation unit tests (the None-vs-Unresolved classification, test-device description gating) and a manager regression test proving a switch to a sidecar-less region DELETEs the old registration against the old region's host, then settles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y batch edit Fixes the confirmed review finding on #1958: queuePendingDelete() persisted its three slots as three independent async DataStore edits, with the token written last as the presence sentinel. The sentinel guarded record presence, not field completeness — a process death after the token flushed but before the region id left a durable record whose regionId read back as the -1 default. The next launch's drain then issued DELETE …/regions/-1/… , counted the resulting 404 as success (the documented already-gone contract), and cleared the queue — permanently leaking the genuinely-stale registration until the server's 180-day prune. record() had the same torn-write exposure. Fixed at the root by implementing #1978 (option 1): PreferencesRepository gains edit {}, which stages writes through a PreferencesEditor receiver and commits them as ONE cache swap + ONE dataStore.edit — atomic on disk, and one file rewrite/fsync/emission instead of one per key. PreferencesEditor holds the String-keyed setters and PreferencesRepository extends it, so the batch receiver and the repository's own one-shot setters are the same five methods rather than parallel declarations. edit has deliberately NO default body — the atomicity is the contract, so each implementation must decide how it honors it; the in-memory fakes (which have no commit to tear) implement it honestly as block(this), while DefaultPreferencesRepository stages ops and funnels both paths through a shared single-op commit() (put() is now just the one-op case). PushRegistrationStore now writes each record as a single edit {}. That also lets record() fold the same-endpoint pending-delete drop into the same commit, closing the sibling crash window where a recorded registration and a not-yet-cleared pending DELETE for the same endpoint could survive together and delete the row just POSTed; the drop shares one staging helper with clearPendingDelete() so the clear semantics live in one place. The store KDoc's "best-effort on disk" caveat is gone — a torn record can no longer exist — so no drain guard against regionId < 0 is needed (nothing shipped the old layout, so no legacy torn state exists in the wild). The read-your-writes instrumented test grows a case pinning the contract: one updateData commit carrying every staged key (including null-removes), with synchronous read-your-writes across the batch. Closes #1978. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the confirmed review finding on #1958: reportHttpFailure() sent every non-2xx response to Crashlytics. The push_registrations endpoint is unauthenticated and rate-limited at 30 req/min/IP, so on a shared-NAT network (public wifi, carrier CGNAT) other clients spend this device's budget and a 429 says nothing about this client. Because a failed register persists nothing, every app foreground retried and re-reported it — flooding the error channel with throttling noise and drowning the systematic-rejection signal (a 422 contract failure) the reporting exists to carry. Transient statuses — 429 and 5xx server-side trouble — are now local-log only, the same treatment transport failures already get: they heal on a later reconcile pass, which retries by design. Only statuses that say *this request* can never succeed (contract-shaped 4xx like the 422) still report remotely. The return value is unchanged, so retry behavior is untouched. Tests: a 429 register logs but does not report and succeeds on the next sync once the throttle lifts; the 500-unregister case now also pins the not-reported half. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1957) Fixes the confirmed review finding on #1958: disabling notifications at the OS level produced Unregister(last), DELETEing the server row and clearing the record. Issue #1957 explicitly reserves DELETE for an *in-app* opt-out (which this app doesn't have): "Notifications disabled at the OS level doesn't need a DELETE — FCM bounces feed back to the server and it cleans up unregistered tokens itself." The iOS PushRegistrationManager confirms the contract — it never DELETEs at all; on a denied permission it simply no-ops. Beyond the divergence, an Android-only DELETE here could silently drop the delivery target of an already-scheduled trip alarm if the server couples alarm delivery to the registration row. DesiredRegistration.None conflated two cases with opposite consequences, so it is split rather than removed: - OptedOut (notifications off) → NoOp. The record is kept alongside the server row, so re-enabling within the refresh window reads as "unchanged" — no spurious re-POST — and a stale record refreshes via the ordinary keep-alive on re-enable. - NoSidecar (the region has no sidecar host) → still Unregister. The iOS docs sanction exactly this old-row DELETE on a region change: a rider who moves to a sidecar-less region must stop receiving the old region's alerts rather than waiting out the 180-day prune. The Reregister DELETE (token/region changed while enabled) is untouched — also doc-sanctioned cleanup of a stale row. Tests: the decision matrix covers both new states (including no keep-alive POST while opted out); the manager test asserts opt-out issues no traffic and re-enable is a NoOp; the unregister 404/500 cases now drive the DELETE via a region switch, the path that still owns it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the plausible review finding on #1958: both description cap sites (applyPushTestDeviceName at the persistence boundary, and the wire-boundary clamp in deriveDesiredRegistration) used String.take(255), which counts UTF-16 code units and can cut between the halves of a surrogate pair. A name with emoji near the boundary would end in a lone surrogate — mangled to '?'/U+FFFD by the form encoding — as its final character. Both sites now share truncatedToDescriptionCap(), which keeps the 255-unit budget but drops a straddling pair whole instead of splitting it. On the counting-semantics half of the finding: the server's published contract says "Free text ≤255 chars" with no byte qualifier, the iOS client does not truncate at all, and the OBACloud source is private, so whether "chars" means code points or something else cannot be read at the source. The cap therefore deliberately stays in UTF-16 units, the strictest of the plausible readings: a string of at most N UTF-16 units also has at most N code points, so the result is within a 255-char limit under either interpretation — no new assumption is introduced, and the choice plus its rationale are documented on the helper. Grapheme clusters (ZWJ sequences, combining marks) can still be cut between code points; that renders imperfectly but is valid Unicode and within the server limit. Tests: the derive-level matrix gains a boundary-straddling emoji case (pair dropped whole) and an exact-fit case (pair kept); the settings-boundary test now pins the same behavior at the persistence site. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ighten the cap helper
Cleanup pass over the transient-status carve-out, the OptedOut/NoSidecar
split, and the surrogate-safe cap:
- PushRegistrationManager's overview KDoc still described the derivation as
"wanted / none / not-yet-resolved"; now names the four states. Its
Unresolved comment (and the missing-region tests) said misreading it as
an "opt-out" fires spurious DELETEs — stale since an opt-out no longer
DELETEs; the dangerous misreading is no-sidecar.
- The OptedOut rationale (FCM bounces / iOS parity / trip-alarm risk) was
copied near-verbatim into three test comments; they now carry one line
plus a pointer to DesiredRegistration.OptedOut, the canonical home. Same
for the 429 shared-NAT rationale, consolidated into isTransient's KDoc
with reportHttpFailure and the manager test pointing at it.
- truncatedToDescriptionCap: take(n).dropLastWhile { isHighSurrogate } in
place of the index-arithmetic substring form. (On already-malformed input
with consecutive trailing lone high surrogates it drops them all rather
than one — at least as correct.)
- Dropped the duplicate surrogate-boundary assertion from
AdvancedSettingsViewModelTest: the branch behavior of the one shared
helper is pinned in DeriveDesiredRegistrationTest, and the settings-side
test's 300-char case already proves that site applies the cap.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the DRY review finding on #1958: PushRegistrationClient's registrationUrl() and TripInfoRepository.requestAlarm() each hand- concatenated {sidecarBaseUrl}{/api/v2/regions/}{regionId}/{resource}, so a change to the sidecar path scheme had to be found and edited in two places. Both now call sidecarRegionUrl() in api/contract — a pure function, so each output is unchanged (the manager test pinning the exact registration URL passes untouched). Two deliberate deviations from the review's Region.sidecarUrl() sketch, both forced by existing constraints: the helper is not a Region method because the push client addresses URLs from its *recorded* registration (whose region may no longer be current), not from a live Region; and the /api/v2/regions/ segment stays a parameter rather than a constant because it is a string resource (a brand override point) that the push client already injects for JVM testability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t-effort Resolves the last review item on #1958 (over-engineering vs. the ticket and iOS), per maintainer decision: keep the old-row DELETE on a token/region change, but stop persisting a missed one for retry. The split follows what the DELETE is actually worth in each case. On a region change the old row holds a live token — nothing bounces, so without a DELETE the rider who moved keeps receiving the old region's alerts until the 180-day prune; the iOS docs sanction exactly this cleanup, so the Reregister DELETE stays. The persisted queue, however, existed only for the corner where that DELETE fails while the POST succeeds (old host unreachable at the moment of a region switch, new host reachable). Its failure mode without the queue — the old row lingers until the prune — is precisely the iOS client's baseline for every region change, and #1957 marks the old-row DELETE as optional. Three pref slots, a queue/drain cycle on every sync, and a slice of test surface were disproportionate insurance for that corner. Removed: queuePendingDelete/pendingDelete/clearPendingDelete and the KEY_PENDING_DEL_* slots from the store (record() no longer needs the same-endpoint drop — with no queue there is nothing to resurrect a stale DELETE from), drainPendingDelete and the sync-top drain from the manager, and the queued-delete manager tests. applyReregister's fourth quadrant (DELETE fail + POST ok) now just records the target, with the best-effort rationale documented at the site; the other three quadrants are unchanged, so a full Reregister failure still retries via the kept record. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ls KDoc Cleanup pass over the last two commits: - applyReregister: with the pending-delete queue gone, the cleaned-flag quadrant logic was provably equivalent to composing the standalone Unregister/Register apply rules — DELETE settles and persists, then POST settles and persists (record() overwrites the intermediate clear() in the both-ok case; every end state matches, pinned by the existing quadrant tests). The four-row truth-table KDoc shrinks accordingly, and the best-effort rationale moves to PushRegistrationAction.Reregister's KDoc in the decision layer, where the sibling policy notes (OptedOut, NoSidecar) already live. - SidecarUrls: scope the "single source" claim to the v2 URL shape and name the pre-existing v1 idiom (weather/surveys embed the region id in their own string resources via a regionID placeholder) instead of implying this helper covers all sidecar URLs; stop asserting the /api/v2/regions/ resource IS a brand override point — no flavor overrides it today — and say only that it stays overridable. Rename the client's registrationsEndpointPath param to regionsPath: it carries the shared regions segment, not anything registrations-specific. - The best-effort reregister test no longer resets onUnregister to success before the settled-state sync — leaving it throwing pins that no DELETE is even attempted, a strictly stronger assertion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.kt`:
- Around line 228-262: Serialize the read-modify-write section of
PreferencesRepository.commit so concurrent callers cannot overwrite each other’s
cache updates before launching the asynchronous DataStore edit. Protect the
cache snapshot update using the repository’s existing synchronization mechanism
or add a suitable lock, while preserving the single async edit for each
operation and keeping the edit method’s batching behavior unchanged.
🪄 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: 29ec51d8-6b0e-46f2-938e-e6576ecc2d7c
📒 Files selected for processing (14)
onebusaway-android/src/androidTest/java/org/onebusaway/android/database/oba/LegacyDataImporterTest.ktonebusaway-android/src/androidTest/java/org/onebusaway/android/preferences/PreferencesRepositoryReadYourWritesTest.ktonebusaway-android/src/main/java/org/onebusaway/android/api/contract/SidecarUrls.ktonebusaway-android/src/main/java/org/onebusaway/android/preferences/PreferencesRepository.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationDecision.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationManager.ktonebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationStore.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.ktonebusaway-android/src/main/java/org/onebusaway/android/ui/tripinfo/TripInfoRepository.ktonebusaway-android/src/test/java/org/onebusaway/android/push/DeriveDesiredRegistrationTest.ktonebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationDecisionTest.ktonebusaway-android/src/test/java/org/onebusaway/android/push/PushRegistrationManagerTest.ktonebusaway-android/src/test/java/org/onebusaway/android/testing/FakePreferencesRepository.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- onebusaway-android/src/test/java/org/onebusaway/android/push/DeriveDesiredRegistrationTest.kt
- onebusaway-android/src/main/java/org/onebusaway/android/push/PushRegistrationClient.kt
- onebusaway-android/src/main/java/org/onebusaway/android/ui/settings/AdvancedSettingsViewModel.kt
Resolves the CodeRabbit finding on #1958: commit()'s optimistic cache update (cache = cache.toMutablePreferences().also(op).toPreferences()) was an unsynchronized read-modify-write on the @volatile cache — two threads committing concurrently could snapshot the same cache, and the later swap would silently drop the earlier writer's keys from every subsequent synchronous read for the life of the process (the cache is deliberately never re-mirrored from DataStore, so the loss never heals). @volatile provides visibility, not atomicity. The race predates this branch (the pre-refactor put() had the same shape); the shared commit() is now where it is fixed. The swap is now under a private lock. Reads stay lock-free (volatile read of an immutable Preferences), and the async persist stays outside the lock — DataStore already serializes its own edits, and each op is self-contained, so only the in-memory side needed guarding. No deterministic test: the interleaving cannot be forced without hooks into the critical section, and a stress test would be flaky-by-design; the lock's rationale is documented at the site instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Registers devices proactively with OBACloud's push_registrations API (#1957) so service-alert push notifications reach every rider who has notifications enabled — not just those who happen to create a trip alarm.
Today OBACloud only learns a device's FCM token as a side effect of creating a trip alarm, which means:
What's included
PushRegistrationWebService— formPOSTregister (token,localeas-is,test_device,description,operating_system=android) andDELETEunregister, on the region's sidecar host via@Url, mirroringReminderWebService. Provided throughNetworkModule.plainRetrofit.PushRegistrationDecision(decidePushRegistration) — a pure, Android-free function that reconciles the desired registration against the last-sent one → register / re-register / unregister / no-op, including the 24h keep-alive. JVM-unit-tested.PushRegistrationManager— derives the desired registration from OS notifications-enabled + region + FCM token + locale + the test-device settings; persists the last registration and when it was sent (so opt-out DELETEs the right token, redundant POSTs are skipped given the 30 req/min/IP limit, and the server'slast_seen_atstays fresh). Driven reactively off the region flow and the preferences it reads, plus aProcessLifecycleOwnerON_STARTresync to catch OS-permission changes made in system settings.Opt-in is the OS notification permission — there is no separate in-app toggle.
Triggers covered
App launch, app foreground, FCM token rotation, region change, the test-device settings, and notification opt-in/opt-out.
Two bugs found during device testing
Both were found by exercising the real endpoint against the live sidecar, and both are fixed here.
1. Test-device registration was rejected outright. A
test_device=truePOST returns422 {"error":"Unable to register device","messages":["Description can't be blank"]}— the server requires adescriptionfield that the contract table in #1957 never sent, so the toggle could not work at all. The requirement landed server-side after that table was written, which is why it wasn't implemented initially. It is now specified in OBACloud's published documentation: free text ≤255 chars identifying the device to admins, required only whentest_device=true, and cleared server-side when a device is demoted.2. HTTP error responses were silently swallowed.
runCatching { … }.onFailureonly fires for thrown exceptions, but a non-2xx is a successful call returning an unsuccessful response — so the 422 above produced no log output whatsoever. Worse, because a failed call persists nothing, the same doomed request repeated on every single app foreground, invisibly, against a rate-limited endpoint. Failures are now branched on explicitly and logged with the status code and the server's error body (never the token, and never the URL — the unregister URL carries the token as a query param).Conformance with the published OBACloud documentation
OBACloud's push-notifications documentation was published after this PR was opened. Checking the implementation against it found it already conformant on every point below — notably the dedupe rule, which was derived independently from the iOS client's source and matches the documented behaviour exactly.
POST /api/v2/regions/{id}/push_registrationsregistrationUrl()builds exactly thisDELETE …?token={token}404on DELETE = treat as successHTTP_NOT_FOUNDcheckoperating_systemmust beios/android"android"localeBCP-47, do not normalizeLocale.getDefault().toLanguageTag(), sent as-istest_devicesent on every call (omission resets it to false)Boolean, always sentdescriptionrequired ifftest_device=truetestDevice = description != null— holds by constructiondescription≤255 chars(token, region, locale, test_device, timestamp); re-POST on change or >24hPUSH_REFRESH_INTERVAL = 24.hours, identical semanticsThe one gap this surfaced was the 255-character cap, now fixed. The rider-entered name was unbounded, so a long one would 422 — and since a failed registration persists nothing, that doomed POST would have repeated on every foreground. It is bounded in two places: at the input field, so the limit is felt while typing, and in
buildTarget(), so a value stored before that bound existed cannot reach the wire.Two deliberate divergences, both worth a maintainer's eye:
key/app_uid/app_ver/version=2. This client sends none, matching the siblingReminderWebServiceon the same sidecar, which already ships that way. Device testing returns204without it and the rate limit is per-IP rather than per-key, so there is no functional impact today — but if OBACloud intends to enforce or attribute by key, both sidecar services should change together.One documentation ambiguity worth flagging upstream:
tokenis described as "APNs device token, lowercase hex", which is iOS-specific — FCM tokens are neither. Sinceoperating_systemacceptsandroidand the live server returns204, this is evidently phrasing rather than a constraint.Parity with the iOS client
Comparing against
OneBusAway/onebusaway-ios(whosedocs/push-notifications.md§5.2 is a conformance checklist for non-OBAKit clients) surfaced two more gaps and improved a third:last_seen_atprune dropped it and alerts silently stopped — defeating the "freshness" goal of Register for OBACloud service-alert push notifications (new push_registrations API) #1957 that this PR's own KDoc claimed to address.decidePushRegistrationnow takes elapsed-time and re-registers afterPUSH_REFRESH_INTERVAL(24h, matching iOS). An unknown timestamp counts as stale, so existing records refresh once on upgrade. NoWorkManagerjob: every foreground already runs the reconcile, and a rider who hasn't opened the app in 180 days has no reach to preserve.RegionRepositoryseeds its region flow to null and fills it asynchronously, so a cold-start reconcile could read "no region" as "the rider opted out",DELETEthe registration, and re-POSTit seconds later. Notifications being off is now the only definitive opt-out; unresolved inputs bail and wait for the collector to re-fire. (The underlyingRegionStatelimitation affects other consumers too — filed as RegionState cannot express "not resolved yet", so a transient null region is indistinguishable from "no region" #1969.)Build.MODEL, which could not distinguish two identical handsets in the admin UI. An unnamed device registers as an ordinary device rather than sending a request the server is guaranteed to reject — the same gating iOS uses.Device verification
On a Pixel 7 Pro (API 36) against the live
sidecar.onebusaway.org, region 1:POST→ 204POST→ 204, fired from the result callbackDELETE …?token=…→ 204, record clearedPOST→ 204POST→ 204, then quietTwo details worth recording. The in-app dialog case is the one the shared permission helper exists for: the system dialog launches as a transparent activity in the app's own task, so the host activity is only paused and
ProcessLifecycleOwner'sON_STARTnever fires — logs confirm no stop/start cycle, so without the result-callback resync the device would have stayed unregistered until the next cold launch. And the DELETE contract question raised in an earlier revision of this description is settled: the token as a query param is both accepted (204) and now the documented form.Not yet device-exercised (unit-tested only): the
Reregisterpath via a region change, the unnamed-test-device downgrade, renaming a test device, and the 255-character truncation.Notes
mainhas been merged in. The notification-permission conflict resolved toward this branch's ComposerememberNotificationPermissionRequest()helper, which requestsPOST_NOTIFICATIONSbehind the same API-33 guard as the request-code flow it replaces and resyncs the registration from the result callback. The now-unreferencedPermissionUtils.NOTIFICATION_PERMISSION_REQUESTconstant is removed. The branch's files are also reformatted for the Spotless gate (Automate formatting with Spotless; retire AndroidStyle.xml (wiring) #1970–Turn on the Spotless formatting gate #1972) that landed onmainwhile this was open.descriptionsemantics are documented upstream and the value is rider-supplied rather than inferred.CLAUDE.mddocuments a OneSignal push setup that doesn't exist in code (the app is FCM-only). Left untouched here; worth a separate cleanup.Testing
./gradlew :onebusaway-android:compileObaGoogleDebugKotlin -PwarningsAsErrors=true— clean../gradlew spotlessCheck— clean../gradlew :onebusaway-android:testObaGoogleDebugUnitTest— 29 push-registration tests pass (11 decision + 18 manager), covering the decision matrix, the persist/retry semantics across all four re-register failure quadrants, the keep-alive window, the region guard, the test-device gating, the description truncation, and the failure-reporting split.🤖 Generated with Claude Code
Summary by CodeRabbit