fix(characters:presentation): pagination race + silent restore failure (review)
- Race: isLoadingNextPage was set inside the launched coroutine, so a rapid second OnLoadNextPage passed the guard before the flag flipped -> the same page loaded twice and items were appended twice. Set the loading flag synchronously before launching. - Restore: when a middle page failed after earlier pages loaded, the error was swallowed (error=null, no event). Now any restore failure emits a ShowSnackbar; partial restores show the loaded list + snackbar, full failures show the error state. Found by the milestone review.
This commit is contained in:
@@ -52,9 +52,9 @@ class CharacterListViewModel(
|
|||||||
}
|
}
|
||||||
|
|
||||||
private fun restore(targetPage: Int) {
|
private fun restore(targetPage: Int) {
|
||||||
|
// Flip the flag synchronously so a guard reading state sees it immediately.
|
||||||
|
_state.update { it.copy(isLoading = true, error = null) }
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
_state.update { it.copy(isLoading = true, error = null) }
|
|
||||||
|
|
||||||
val accumulated = mutableListOf<CharacterUi>()
|
val accumulated = mutableListOf<CharacterUi>()
|
||||||
var lastLoadedPage = 0
|
var lastLoadedPage = 0
|
||||||
var endReached = false
|
var endReached = false
|
||||||
@@ -77,9 +77,14 @@ class CharacterListViewModel(
|
|||||||
page++
|
page++
|
||||||
}
|
}
|
||||||
|
|
||||||
if (accumulated.isEmpty() && error != null) {
|
// Always surface a failure — even a partial one where earlier pages loaded.
|
||||||
_state.update { it.copy(isLoading = false, error = error) }
|
if (error != null) {
|
||||||
_events.send(CharacterListEvent.ShowSnackbar(error))
|
_events.send(CharacterListEvent.ShowSnackbar(error))
|
||||||
|
}
|
||||||
|
|
||||||
|
if (accumulated.isEmpty()) {
|
||||||
|
// Nothing loaded → full-screen error (or an empty list if the API simply had none).
|
||||||
|
_state.update { it.copy(isLoading = false, error = error) }
|
||||||
} else {
|
} else {
|
||||||
val loadedPage = lastLoadedPage.coerceAtLeast(1)
|
val loadedPage = lastLoadedPage.coerceAtLeast(1)
|
||||||
_state.update {
|
_state.update {
|
||||||
@@ -88,6 +93,7 @@ class CharacterListViewModel(
|
|||||||
isLoading = false,
|
isLoading = false,
|
||||||
currentPage = loadedPage,
|
currentPage = loadedPage,
|
||||||
endReached = endReached,
|
endReached = endReached,
|
||||||
|
// The list is shown; the snackbar already surfaced any partial failure.
|
||||||
error = null,
|
error = null,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -112,8 +118,11 @@ class CharacterListViewModel(
|
|||||||
}
|
}
|
||||||
|
|
||||||
private fun loadPage(page: Int) {
|
private fun loadPage(page: Int) {
|
||||||
|
// Flip the loading flag SYNCHRONOUSLY (before launching) so a rapid second OnLoadNextPage is
|
||||||
|
// guarded out before its coroutine starts — otherwise the same page loads twice and items
|
||||||
|
// get appended twice.
|
||||||
|
_state.update { it.copy(isLoadingNextPage = true, error = null) }
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
_state.update { it.copy(isLoadingNextPage = true, error = null) }
|
|
||||||
characterRepository.getCharacters(page)
|
characterRepository.getCharacters(page)
|
||||||
.onSuccess { pageData ->
|
.onSuccess { pageData ->
|
||||||
_state.update { state ->
|
_state.update { state ->
|
||||||
|
|||||||
Reference in New Issue
Block a user