fix(pages): resync congrats screen after filtered deck rebuild

[Ayush-Patel-56]

Purpose / Description

The congrats screen for a filtered deck stays stuck on "Congratulations" after rebuilding, even when the rebuild pulls cards back in.

Fixes

• Fixes Filtered deck overview doesn't update properly after rebuilding #21393

Approach

CongratsPage reloaded its own congrats WebView route on every studyQueues change without checking if the deck was actually still empty. CongratsViewModel now checks sched.counts() first and navigates to StudyOptionsActivity instead of reloading when the deck has cards due. Desktop's Overview already does this same check on every refresh, AnkiDroid's split-screen version just never ported it over.

How Has This Been Tested?

Added CongratsViewModelTest, reproducing the repro steps from the issue (fill deck, empty it, rebuild). Ran the full unit test suite locally. Manually reproduced and verified the fix on a Pixel 7 emulator, API 33.

Learning (optional, can help others)

Compared against desktop's qt/aqt/overview.py to see how it avoids this class of bug: it re-checks sched._is_finished() on every refresh instead of assuming the previous state still holds.

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

[lukstbit]

A fix for the main branch lint failure was merged. Rebase on top of main to clear CI failures(if your PR doesn't introduce any).

[david-allison]

I don't see how this would work. We wouldn't be subscribed when the event is emitted as the screen wouldn't be active/visible.

[Ayush-Patel-56]

emit() suspends until something's collecting, it doesn't drop the value. flowWithLifecycle just cancels/restarts the collector on stop/start, so a rebuild while we're stopped just parks until we resume. can add a test for that if you want it nailed down.

[david-allison]

emit() suspends until something's collecting

I don't believe this is the case, could you confirm empirically. In your test, you have a subscriber, driven by turbine.

We are calling MutableSharedFlow.emit: https://github.com/Kotlin/kotlinx.coroutines/blob/b157ff07152dc2adcfdb2e7e45ff2a1fd4dca0bd/kotlinx-coroutines-core/common/src/flow/SharedFlow.kt#L418-L421

We built the MutableStateFlow as: MutableSharedFlow<CongratsRefreshState>() which defines replay = 0

See the implementation of tryEmitNoCollectorsLocked: https://github.com/Kotlin/kotlinx.coroutines/blob/b157ff07152dc2adcfdb2e7e45ff2a1fd4dca0bd/kotlinx-coroutines-core/common/src/flow/SharedFlow.kt#L447-L455

     * If there are no subscribers, the buffer is not used.
     * Instead, the most recently emitted value is simply stored into
     * the replay cache if one was configured, displacing the older elements there,
     * or dropped if no replay cache was configured.

https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow/emit.html

[Ayush-Patel-56]

you're right, with replay=0 and no active subscriber the emit is dropped, confirmed via the docs you linked. real fix: added an onResume() recheck in CongratsPage so it re-evaluates deck state every time the screen becomes visible again, not just when it catches the live event. wrote a test that rebuilds the deck while the fragment is stopped, then resumes it, failed without the fix, passes now.

[david-allison]

Ok, I've looked into this more deeply:

This works, but only because finish() is called implicitly via:

            undoableOp<OpChangesWithId> {
                val safeAddUpdateResult = safeAddOrUpdateFilteredDeck(newFilterForUpdate)
                if (safeAddUpdateResult.isSuccess) {
                    state.update { DeckBuilt } // this calls through to `finish()`

Instead, could we explicitly perform a refresh on STARTED, rather than relying on an implicit ordering.

[github-actions]

Snapshot diff report vs main. Open screenshot-diff for diffs.

• DeckPickerScreenshotTest: 1 change
• PreferencesScreenshotTest: 1 change

All 2 changed screenshots

DeckPickerScreenshotTest

• hierarchy_lines_many_siblings_compare.png

PreferencesScreenshotTest

• ReviewerOptionsFragment_compare.png

[Ayush-Patel-56]

Ok, I've looked into this more deeply:

This works, but only because finish() is called implicitly via:

            undoableOp<OpChangesWithId> {
                val safeAddUpdateResult = safeAddOrUpdateFilteredDeck(newFilterForUpdate)
                if (safeAddUpdateResult.isSuccess) {
                    state.update { DeckBuilt } // this calls through to `finish()`

Instead, could we explicitly perform a refresh on STARTED, rather than relying on an implicit ordering.

plain onStart() has the same race in a different spot (fires before the collector resubscribes). moved the refresh inside the same repeatOnLifecycle(STARTED) block as the collector so ordering is guaranteed. pushed just now.

[david-allison]

Could you add a Timber.d here, I suspect this is going to be called twice. In the case of reload(), this will cause noticeable latency (and we'll want to resolve this).

[Ayush-Patel-56]

good catch, was scoped to the fragment's own lifecycle - onViewCreated can run more than once (webview gets torn down/recreated), so old collectors stuck around. switched to viewLifecycleOwner, also fixes the lint failure. added the Timber.d.

[david-allison]

onViewCreated can run more than once (webview gets torn down/recreated)

I don't believe this is the case, and the fix adds complexity to the code (with a verbose comment).

Please revert this (unless there's a case I haven't thought about): SafeWebView is the only 'refresh' we have, and this works on the WebView component.

Note: My 'double reload' was referring to the case where the opChanges occurred from a rebuild after the screen moved back to STARTED - I don't think I was correct here, but the line is useful as reload is expensive.

[david-allison]

Thanks!! One more round I feel

[david-allison]

onViewCreated can run more than once (webview gets torn down/recreated)

I don't believe this is the case, and the fix adds complexity to the code (with a verbose comment).

Please revert this (unless there's a case I haven't thought about): SafeWebView is the only 'refresh' we have, and this works on the WebView component.

Note: My 'double reload' was referring to the case where the opChanges occurred from a rebuild after the screen moved back to STARTED - I don't think I was correct here, but the line is useful as reload is expensive.