feat: add ViewModel progress infrastructure [DEV]#20729
Open
criticalAY wants to merge 1 commit intoankidroid:mainfrom
Open
feat: add ViewModel progress infrastructure [DEV]#20729criticalAY wants to merge 1 commit intoankidroid:mainfrom
criticalAY wants to merge 1 commit intoankidroid:mainfrom
Conversation
b6fabdb to
c278581
Compare
Member
david-allison
left a comment
There was a problem hiding this comment.
This looks exceptional. One question on the race condition
Comment on lines
+46
to
+48
| showJob?.cancel() | ||
| showJob = null | ||
| dismissLoadingDialog() |
Member
There was a problem hiding this comment.
Is there a race condition if this and Active are running simultaneously?
Contributor
Author
There was a problem hiding this comment.
Ahh oki, quick fix, here are tests to verify (Used both Gemini and Claude to get the tests)
@Test
fun `Idle during show delay cancels pending dialog without race`() =
runTest {
val manager = ProgressManager()
val operationBlocker = CompletableDeferred<Unit>()
var dialogShown = false
var dialogDismissed = false
// Simulate what observeProgress does: collect state, delay before "showing"
val observer =
launch {
var showJob: kotlinx.coroutines.Job? = null
manager.progress.collect { state ->
when (state) {
is ViewModelProgress.Idle -> {
showJob?.cancel()
showJob = null
dialogDismissed = true
}
is ViewModelProgress.Active -> {
showJob =
launch {
kotlinx.coroutines.delay(600)
dialogShown = true
}
}
}
}
}
// Start an operation — state goes Active, showJob starts its 600ms delay
val operation =
launch {
manager.withProgress(message = "quick op") {
operationBlocker.await()
}
}
// Advance 300ms — halfway through the delay, dialog NOT yet shown
testScheduler.advanceTimeBy(300)
assertEquals(false, dialogShown, "Dialog should not be shown during delay")
// Operation completes — state goes Idle, showJob gets cancelled
operationBlocker.complete(Unit)
testScheduler.advanceUntilIdle()
// Advance well past the 600ms — dialog should STILL not be shown
testScheduler.advanceTimeBy(1000)
assertEquals(false, dialogShown, "Dialog must not show after Idle cancelled the showJob")
assertEquals(true, dialogDismissed, "Idle branch should have run")
assertIs<ViewModelProgress.Idle>(manager.progress.value)
observer.cancel()
operation.cancel()
}
/**
* Verifies that rapid Active state emissions during the 600ms delay
* do not bypass it. Uses the fixed observer logic with a `dialogVisible`
* guard — updates only reach the dialog after the delay has elapsed.
*/
@Test
fun `fixed observer - rapid Active states do not bypass the 600ms delay`() =
runTest {
val progressFlow = MutableStateFlow<ViewModelProgress>(ViewModelProgress.Idle)
var dialogShownCount = 0
val observer =
launch {
var showJob: kotlinx.coroutines.Job? = null
var dialogVisible = false
progressFlow.collect { state ->
when (state) {
is ViewModelProgress.Idle -> {
showJob?.cancel()
showJob = null
dialogVisible = false
}
is ViewModelProgress.Active -> {
if (showJob == null) {
showJob =
launch {
kotlinx.coroutines.delay(600)
dialogVisible = true
dialogShownCount++
}
} else if (dialogVisible) {
// Only update if dialog is actually showing
dialogShownCount++
}
// If delay is still pending, do nothing — no bypass
}
}
}
}
// First Active — starts the 600ms delay
progressFlow.value =
ViewModelProgress.Active(
message = "Loading...",
amount = ProgressContext.Amount(current = 1, max = 100),
cancellable = false,
)
testScheduler.advanceTimeBy(100)
assertEquals(0, dialogShownCount, "Dialog should not show before 600ms")
// Second Active at t=100ms — with the fix, this is ignored during delay
progressFlow.value =
ViewModelProgress.Active(
message = "Loading...",
amount = ProgressContext.Amount(current = 2, max = 100),
cancellable = false,
)
testScheduler.advanceTimeBy(1)
assertEquals(0, dialogShownCount, "Fixed: dialog must not show at 101ms")
// Advance past the delay — now the dialog shows exactly once
testScheduler.advanceTimeBy(600)
assertEquals(1, dialogShownCount, "Dialog should show once after the full 600ms delay")
observer.cancel()
}
```
Contributor
Author
There was a problem hiding this comment.
Not adding them to actual tests, feels too much maybe
Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.
c278581 to
d97aee6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose / Description
Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.
Fixes
withProgressin ViewModels #19460Approach
How Has This Been Tested?
Unit test
Learning (optional, can help others)
NA
Checklist
Please, go through these checks before submitting the PR.