fix: potential skip in repeatOnLifecycle

[HS0204]

Purpose / Description

A potential issue was discussed in the review of #20192.
If the logic inside the collection block updates lastValue before the UI is actually updated, or if there is a mismatch between how lastValue is persisted and how the flow emits values, a critical UI update may be skipped because the check assumes the state has not changed since the last emission.
This PR reproduces the scenario discussed in the review with tests and adds a fix that ensures the corrected behaviour.

Fixes

• Fixes Potential UI update skip in repeatOnLifecycle #20394

Approach

1. Duplicated the existing logic with lifecycle transitions (STARTED -> CREATED -> STARTED) to reproduce the reviewer's concern like if the app is in background and value is updated.
2. Confirmed the potential issue occur. When StateFlow.value changes during collection, the update for that value was skipped.
3. Added a second test using the collected emission it instead of StateFlow.value under the same scenario.
4. Verified the fix ensures all updates are delivered.
5. Based on these testing process, I finally wrote an executable test directly using launchCollectionInLifecycleScope.

How Has This Been Tested?

Test reproducing the potential scenario passed.

Learning (optional, can help others)

This also helped me better understand the behaviour of StateFlow.value, especially how the current value can diverge from the collected emission when another updates the StateFlow during collection.
Before this PR, I assumed that StateFlow.value would always correspond to the value being collected. While looking into this issue, I gained a clearer understanding of the "most recently emitted value" behaviour described in the official docs.

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[HS0204]

I posted this PR before assigning it. Of course I already left the comment that I'd like to figure out this issue, but If I happened to violate Anki's assigning system, please tell me. I'll follow that 😉

[david-allison]

Is it feasible to test launchCollectionInLifecycleScope itself, rather than the logic?

[HS0204]

Sure I'll work on that. I'll remove the logic tests and try to add comments in the new test explaining the background of this change. If you want to leave these tests, please let me know. Thanks!

[david-allison]

I don't think they produce value if they can't be broken by changes to the app.

[HS0204]

I replaced the logic tests with a test directly using launchCollectionInLifecycleScope. At first, I focused on proving why it should be used instead of value, so I wrote off-topic tests. Hopefully this new one better reflects the actual purpose!

I'm a little concerned about using opt-in API, but without it, reproducing this scenario would require adding test specific code (e.g. yield). So I chose this approach to avoid changing origin code.

@david-allison when you have time, could you take another look? Thank you!

[david-allison]

Sorry this one sat. @criticalAY FYI

I'm struggling conceptually here: queuing up state updates shouldn't be the cause of a bug: UI shouldn't have dependencies between states. Each state update should describe the 'full' state of the screen.

We should be able to collectLatest without issues.

If the intermediate updates matter, then we should be treating this as a stream of events, rather than state (which IMO should be a SharedFlow/Channel, not a StateFlow)

Feel free to use AI for this one: let me know if I'm wrong, or if there are instances in our codebase where this would make a difference. I'm not certain in my viewpoint here, I feel I'm missing something and facts trump opinion :D

[github-actions]

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

[david-allison]

@criticalAY thoughts welcome

[criticalAY]

Will get back in few hours, thanks!

[criticalAY]

The problem isn't about needing intermediate values. It's that the dedup logic reads StateFlow.value inside collect, and .value can race ahead of what's actually being collected. This means our own code can skip the final latest state too, not just intermediates. So the UI ends up stuck on a stale value.

I used AI to get an example :

Example

Let me walk through a concrete scenario.

Say you have a screen showing a counter value, and your StateFlow dedup logic looks like this:

if (lastValue == value) return@collect  // value = StateFlow.value
lastValue = value

Here's what happens step by step:

1. App is in foreground, StateFlow emits 5, UI shows 5, lastValue = 5
2. User switches to another app (lifecycle drops to CREATED, collection pauses)
3. While backgrounded, something updates StateFlow from 5 → 6 → 7 (two quick updates)
4. User comes back (lifecycle goes to STARTED, collection resumes)
5. The collector receives the emission 6, but by the time it runs the check, StateFlow.value has already raced ahead to 7
6. So the check does: if (lastValue == value) → if (5 == 7) → false, passes through
7. Then lastValue = 7 (set from .value, not the collected 6)
8. Now the collector receives emission 7, checks if (lastValue == value) → if (7 == 7) → true, skipped!

The UI never shows 7. It jumped from 5 to 6 and skipped the final actual state. The collector did receive 7, but the dedup logic threw it away because lastValue was set from the racy .value read instead of from the actual collected emission.

If you use it (the collected value) instead of .value, then at step 7 lastValue would be 6, and when 7 arrives, if (6 == 7) is false, so it passes through correctly.

(You are right about, that StateFlow is a state holder and well-designed UI state shouldn't depend on intermediate emissions)

[david-allison]

@HS0204 Please update the commit message given the above, then let's get this in

[criticalAY]

Thanks

[criticalAY]

Feel free to merge once David's comment is resolved