Guard ArrivalsViewModel.refresh() against out-of-order overlapping refreshes (#1933)

[bmander]

Fixes #1933 (split out from a CodeRabbit finding on #1932, where it was out of scope).

Problem

ArrivalsViewModel.refresh() applied every completed result unconditionally (loaded.value = data). The 60s poll loop awaits its suspend refresh() sequentially, so it never overlaps itself — but manualRefresh() and loadMore() each viewModelScope.launch { refresh() }. So a user refresh (toolbar button, or the alert-dialog dismiss in ArrivalActionHandlers) can run concurrently with an in-flight poll and finish out of order: the older-started completion's loaded.value = data then clobbers the fresher result.

Impact (low, self-correcting): a spurious isStale flag (an overlapping call whose own network failed returns the repo's stale fallback) or momentarily older arrivals, until the next poll corrects it (≤60s). The map snapshot lastLoaded() was already protected by the repository's compareAndSet (#1932); this closes the same gap one layer up, in the ViewModel's loaded StateFlow.

Fix

A monotonic refreshGeneration, bumped at the start of each refresh(). A completion applies its result only while it's still the latest; otherwise it drops out — no state write, no map-snapshot emit:

suspend fun refresh(): Boolean {
    val generation = ++refreshGeneration
    val result = repository.getArrivals(stopId, minutesAfter)
    lastResponseTime = WallTime.now()
    if (generation != refreshGeneration) return false   // superseded — drop
    return result.fold( ... )
}

All reads/writes are confined to the ViewModel dispatcher (the only suspension in refresh() is the fetch), so a plain Int needs no synchronization. Chose the generation counter over serializing/cancelling refreshes because it preserves the existing parallelism and is the least behavior-changing — only the superseded completion's application is dropped.

Test

Drives two overlapping refreshes (per-call gates added to the fake repository) completing out of order and asserts the older-started stale completion cannot overwrite the fresher one (checks both isStale and windowEnd). Confirmed red without the guard (only that test fails) and green with it.

Verification

• New + all existing ArrivalsViewModelTest cases pass; full unit suites green on obaGoogle and obaMaplibre with -PwarningsAsErrors=true; compiles clean.
• No runtime UI surface beyond the covered logic; the common sequential-refresh path is unchanged (existing tests) — the guard only affects the overlapping case.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved arrivals refresh behavior when multiple refreshes overlap, ensuring only the most recent completed request can update the UI.
  • Prevented stale in-flight results from overwriting fresher arrival data.
• Tests
  • Added a regression test to confirm out-of-order, overlapping refresh responses can’t clobber the latest results.
  • Enhanced the arrivals test double to simulate per-call completion ordering for reliable coverage.

[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: 93072edc-d720-4c44-a7ed-f12c035995d9

📥 Commits

Reviewing files that changed from the base of the PR and between 7fb26da and 1c0dd56.

📒 Files selected for processing (1)

• onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModel.kt

---

📝 Walkthrough

Walkthrough

ArrivalsViewModel.refresh() now uses a generation counter to ignore superseded asynchronous results. Tests configure overlapping repository calls to complete out of order and verify that the newer arrivals state is preserved.

Changes

Arrivals refresh latest-wins handling

Layer / File(s)	Summary
Refresh generation guard onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModel.kt	refresh() captures a monotonically increasing generation and returns false without applying results when a newer refresh has started.
Out-of-order refresh regression coverage onebusaway-android/src/test/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModelTest.kt	The fake repository supports per-call gates and results, and a regression test verifies that a stale earlier completion cannot overwrite the fresher result.

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

Sequence Diagram(s)

sequenceDiagram
  participant RefreshOne
  participant RefreshTwo
  participant ArrivalsRepository
  participant ArrivalsViewModel
  RefreshOne->>ArrivalsViewModel: start older refresh
  RefreshTwo->>ArrivalsViewModel: start newer refresh
  ArrivalsRepository-->>RefreshTwo: return fresh result
  ArrivalsViewModel->>ArrivalsViewModel: apply newer generation
  ArrivalsRepository-->>RefreshOne: return stale result
  ArrivalsViewModel->>ArrivalsViewModel: drop superseded completion

Loading

Possibly related PRs

• OneBusAway/onebusaway-android#1746: Both changes touch ArrivalsViewModel refresh and completion timestamp handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change and references the linked issue.
Linked Issues check	✅ Passed	The refresh generation guard and out-of-order regression test satisfy #1933's latest-wins requirement.
Out of Scope Changes check	✅ Passed	Only ArrivalsViewModel and its test were changed, matching the issue scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/arrivals-viewmodel-latest-wins-1933

---

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.}

[coderabbitai]

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/ui/arrivals/ArrivalsViewModel.kt`:
- Around line 127-130: Move the lastResponseTime = WallTime.now() assignment in
the refresh completion flow to after the generation != refreshGeneration
early-return guard. Ensure superseded requests do not update the poll timer,
while valid completions still record their completion time.

🪄 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: 8f967462-1071-45d4-9d63-14908e66e5de

📥 Commits

Reviewing files that changed from the base of the PR and between c4e00e0 and 7fb26da.

📒 Files selected for processing (2)

• onebusaway-android/src/main/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModel.kt
• onebusaway-android/src/test/java/org/onebusaway/android/ui/arrivals/ArrivalsViewModelTest.kt