Skip to content

Review triggers were lost when the user left the screen immediately - #2200

Merged
mpretty-cyro merged 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/review-events-survive-navigation
Aug 31, 2026
Merged

Review triggers were lost when the user left the screen immediately#2200
mpretty-cyro merged 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/review-events-survive-navigation

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

A review trigger emitted from a screen the user immediately leaves never arrives, so the prompt never shows.

What happens

Change the theme in Appearance and press back, and no review prompt appears — not late, never. Found by Review prompt Appearance trigger in the Appium suite, which had never passed.

Why

InAppReviewManager's event channel was a rendezvous channel (capacity 0). On a rendezvous channel send does not complete when it is called — it suspends until the collector receives. Every call site wrapped it in the screen's own scope:

PathActivity.kt:114              lifecycleScope.launch
SettingsViewModel.kt:642         viewModelScope.launch      <- Donate
AppearanceSettingsViewModel:33   viewModelScope.launch      <- setNewAccent
AppearanceSettingsViewModel:45   viewModelScope.launch      <- setNewStyle

So the emission had to outlive the screen that triggered it. Changing a theme is the one interaction a user reliably follows with "back" within milliseconds, which is why Theme lost it and Path and Donate did not — all four were equally exposed; three survived on user behaviour rather than on structure. A fast enough Donate tap has the same defect.

The change

The channel becomes UNLIMITED and onEvent becomes non-suspending (trySend), so no caller needs a coroutine and the four call sites reduce to a plain call. Fixing AppearanceSettingsViewModel alone would have left Donate and Path racing and looked fixed, because those two are hard to lose by hand.

A dropped trySend is logged rather than swallowed. On an unlimited channel it is unreachable short of the manager being closed, but silence there would look exactly like the bug this replaces.

setNewAccent is covered by the same change; it had the identical shape and no coverage.

Verification

Two new tests in InAppReviewManagerEventDeliveryTest. The first fires an event before the collector has started — the precise moment a rendezvous channel had nowhere to put it — and waits with first { it } rather than sampling, so a lost event fails by timing out instead of by reading a stale false.

Mutated by setting the capacity back to RENDEZVOUS while keeping trySend: both tests fail. That isolates the buffer as the thing doing the work rather than the non-suspending signature.

End to end: all six Review prompt * Appium specs pass against this build, including Appearance trigger, which was previously unreachable rather than flaky.

Note

Touches InAppReviewManagerTest.kt, which qa/review-prompt-app-updated-extra also adds a class to — the two conflict on that file and want stacking or a merge order.

@mpretty-cyro
mpretty-cyro marked this pull request as ready for review August 31, 2026 06:11
Change the theme and press back straight away and no review prompt ever
appears -- not just in a test, for anyone. The same is true of the accent
colour.

The defect is the channel's capacity, not the call sites. `Channel<Event>()`
is rendezvous, so `send` does not complete when it is called: it suspends until
the collector receives. The emission therefore had to outlive the screen that
triggered it, and a theme change is precisely the interaction a user follows with
"back" within milliseconds -- the ViewModel is cleared, its scope is cancelled,
and the event is gone.

All four emit sites were lifecycle-scoped: path visited on `lifecycleScope`,
donate and both appearance setters on `viewModelScope`. Three of them survived on
user behaviour rather than on structure -- visiting Path leaves you on Path, and
tapping Donate opens a link and leaves Settings alive -- so they looked correct
while carrying the same race. Repairing only the appearance screen would have
left them racing and looked like a fix.

So the fix is at the manager: the channel is UNLIMITED and `onEvent` is no longer
suspending, handing off via `trySend`. No caller needs a coroutine, and no future
caller can reintroduce this by choosing the wrong scope. The four call sites lose
their launches.

`trySend` cannot fail on an unlimited channel short of the manager being closed,
but the failure is logged rather than discarded: a silently dropped event would
look exactly like the bug being fixed.

Both appearance setters are covered -- setNewAccent had the identical shape and
the same defect, untested until now.
@mpretty-cyro
mpretty-cyro force-pushed the fix/review-events-survive-navigation branch from 6fbe394 to 462fe77 Compare August 31, 2026 06:21
@mpretty-cyro
mpretty-cyro merged commit c7dc2c8 into session-foundation:dev Aug 31, 2026
5 checks passed
@mpretty-cyro
mpretty-cyro deleted the fix/review-events-survive-navigation branch August 31, 2026 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant