Modernize the trip-plan-change monitor; fix the OTP1→OTP2 "better plan" misfire

[bmander]

Problem

The trip-plan-change monitor could fire a surprise "We found a better trip plan" notification for a trip the user didn't recently plan — the feature that prompted this.

Root cause: itinerary identity was exact ordered-string equality over transit-leg tripId (ItineraryDescription.itineraryMatches), and tripId is the OTP gtfsId verbatim from both adapters. The pinned OTP2 schema documents Trip.gtfsId as colon-delimited FeedId:TripId (graphql/otp2/schema.graphqls); OTP1 emits bare ids (trip_5). A monitor armed under one id vocabulary can never string-match a re-plan under the other, so TripMonitorDecider.decide fell through to ItineraryChanged and fired the "better plan" alert for a trip that never changed. START_REDELIVER_INTENT + a persisted AlarmManager PendingIntent means a stale pre-OTP2 monitor can redeliver after an app update and re-plan under OTP2, reproducing it.

The identity logic is the last piece untouched since the 2016 original; this modernizes it and folds in the adjacent debt found along the way.

Changes

1. Normalized itinerary identity (the fix). Compare on the entity suffix of each trip id, not the raw string. The FeedId: prefix is stripped via a shared gtfsEntitySuffix() extracted from OtpObaIdResolver — the documented OTP2 id contract ("the entity id is identical on both sides, only the feed prefix differs"), reused by both the resolver and ItineraryDescription. So trip_5 (OTP1) and 1:trip_5 (OTP2) compare equal. Colon-only, deliberately (OBA ids are agency_entity and GTFS ids contain underscores). Raw ids stay in the persisted bundle, so the currently-pending pre-OTP2 alarm also compares correctly when it fires.

⚠️ No-heuristics sign-off: per CLAUDE.md, reusing this sanctioned colon transform re-opens the human sign-off gate. It is grounded in the pinned schema + the resolver's existing (unit-tested) rule, not invented. Note: the Otp2PlanDecodeTest fixtures use underscore ids (1_trip_5), which contradict the schema's colon form — those fixtures are wrong; normalization intentionally does not follow them. (Fixing that fixture is out of scope here.)

2. Version-stamped monitor state. New EXTRA_MONITOR_VERSION / MONITOR_STATE_VERSION behind a pure, unit-tested parseMonitorState(). A bundle from a newer/incompatible build (redelivered across an app update) stops the service silently instead of misfiring; an absent version = compatible legacy v0.

3. Honest notification. ItineraryChanged → "Your planned trip changed" (not "better"); the deviation body gets its own string. Retires the old "better plan" strings and their translations (which literally said "better plan" in it/es/pl/fi). The change alert uses a fixed id (single monitored trip) rather than the not-guaranteed-unique tripIds.hashCode().

4. One enable-gate. TripPlanNotifications.isEnabled() = the in-app toggle AND the OS (app-level notifications incl. POST_NOTIFICATIONS, and channel importance). Previously the app pref was ignored on O+, so the Settings switch did nothing on modern devices. Retires the hidden, UI-less live_updates preference. (User-visible: the toggle now gates arming on all SDK levels.)

5. Cleanup. Refreshes stale #1939 comments (restore-into-directions is merged and live).

Non-goals: no multi-trip monitoring, no service/alarm re-architecture (#1732/#1733 stays), no OTP1 removal (fix works for both), no "actually better" ranking, no persisted-bundle/TripItinerary JSON schema change.

Tests (JVM, pure)

• OtpGtfsIdsTest — colon split, unprefixed passthrough, underscore preserved, null.
• ItineraryDescriptionTest — cross-scheme matching, order-sensitivity, non-collision.
• TripMonitorDeciderTest — the load-bearing regression (OTP1 armed vs OTP2 re-plan ⇒ KeepMonitoring/Deviation, never ItineraryChanged) + the previously-untested null-delay and threshold-boundary branches.
• TripMonitorStateTest — the version/validity gate (valid / legacy-v0 / missing / incompatible).
• OtpObaIdResolverTest and TripItineraryJsonTest stay green (no persisted-format change).

Both flavors + test sources compile under -PwarningsAsErrors=true; spotlessCheck passes; installed and running on a device.

Summary by CodeRabbit

• New Features

  • Improved trip monitoring across different trip ID formats.
  • Added validation for restored trip-monitoring state to handle missing or unsupported data safely.
  • Trip-plan notifications now respect both in-app and device notification settings.
  • Updated trip-change and service-deviation notification messages.
  • Repeated trip-change alerts now replace older alerts instead of accumulating.

• Bug Fixes

  • Prevented unnecessary monitoring when notifications are disabled.
  • Improved handling of itinerary matching, delays, and incomplete trip data.

• Tests

  • Added coverage for ID normalization, itinerary matching, notification decisions, delay thresholds, and monitor-state validation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9e61eafc-25f5-4dfb-a318-85fb35dba195

📥 Commits

Reviewing files that changed from the base of the PR and between 082ce6d and f8572bd.

📒 Files selected for processing (4)

• onebusaway-android/src/main/java/org/onebusaway/android/directions/model/ItineraryDescription.kt
• onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/MonitorStateParse.kt
• onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.kt
• onebusaway-android/src/test/java/org/onebusaway/android/directions/realtime/TripMonitorStateTest.kt

🚧 Files skipped from review as they are similar to previous changes (3)

• onebusaway-android/src/test/java/org/onebusaway/android/directions/realtime/TripMonitorStateTest.kt
• onebusaway-android/src/main/java/org/onebusaway/android/directions/model/ItineraryDescription.kt
• onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.kt

---

📝 Walkthrough

Walkthrough

Trip monitoring now normalizes OTP/GTFS identifiers, validates persisted monitor state, centralizes notification enablement checks, updates foreground-service handling, and revises trip-change notification navigation and text.

Changes

Trip identity normalization

Layer / File(s)	Summary
GTFS identifier normalization and itinerary matching onebusaway-android/src/main/java/org/onebusaway/android/directions/*, onebusaway-android/src/main/java/org/onebusaway/android/directions/model/ItineraryDescription.kt, onebusaway-android/src/test/java/org/onebusaway/android/directions/*	Colon-delimited GTFS suffix extraction is centralized, resolver usage is updated, and itinerary matching compares normalized trip IDs while retaining raw IDs.
Monitoring decision coverage onebusaway-android/src/test/java/org/onebusaway/android/directions/realtime/TripMonitorDeciderTest.kt	Tests cover cross-scheme matching, delay thresholds, deviations, and missing end dates.

Notification enablement and scheduling

Layer / File(s)	Summary
Centralized notification gate onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanNotifications.kt, onebusaway-android/src/main/java/org/onebusaway/android/directions/util/OTPConstants.kt	Notification availability now checks the preference, app permission, and trip-plan channel state; the obsolete preference constant is removed.
Monitor startup integration onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitor.kt, onebusaway-android/src/main/java/org/onebusaway/android/ui/tripresults/TripResultsScreen.kt	Monitor startup and trip-results initiation use the shared gate, and monitor bundles include a state version.

Validated monitoring service and alerts

Layer / File(s)	Summary
Persisted monitor-state parsing onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/MonitorStateParse.kt, onebusaway-android/src/test/java/org/onebusaway/android/directions/realtime/TripMonitorStateTest.kt	Versioned bundles produce valid, missing, or incompatible parse results, with validation for trip IDs and dates.
Foreground monitoring and notification re-entry onebusaway-android/src/main/java/org/onebusaway/android/directions/realtime/TripPlanMonitorService.kt, onebusaway-android/src/main/res/values/strings.xml	The service consumes validated state, restarts monitoring jobs, reopens HOME with serialized itinerary data, and posts trip-change alerts using a fixed notification ID and updated text.
Localized resource cleanup onebusaway-android/src/main/res/values-es/strings.xml, onebusaway-android/src/main/res/values-fi/strings.xml, onebusaway-android/src/main/res/values-it/strings.xml, onebusaway-android/src/main/res/values-pl/strings.xml	Obsolete new-plan notification resources are removed from localized string files.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TripResultsScreen
  participant TripPlanNotifications
  participant TripPlanMonitor
  participant TripPlanMonitorService
  participant HOME
  TripResultsScreen->>TripPlanNotifications: check notification availability
  TripResultsScreen->>TripPlanMonitor: start monitoring when enabled
  TripPlanMonitor->>TripPlanMonitorService: deliver versioned monitor state
  TripPlanMonitorService->>HOME: reopen directions with updated itineraries

Loading

Possibly related PRs

• OneBusAway/onebusaway-android#1744: Earlier trip-plan monitoring service and state-flow changes.
• OneBusAway/onebusaway-android#1782: Earlier monitoring intent and trip-plan reopening changes.
• OneBusAway/onebusaway-android#1940: Updates HOME to consume notification intent data and restore directions.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 24.39% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main changes: monitor modernization and the OTP1→OTP2 trip-id normalization bug fix.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}