TASK-034 third round: make favorite star/count update optimistic
Every state/count change in toggleFavorite was applied only after the network round trip to this app's home-NAS-hosted Gitea completed, so a tap visibly did nothing until that finished -- reported as feeling laggy. The star/count now update synchronously on tap, before the coroutine launches; a failed request rolls the optimistic change back instead of leaving the display stuck at it. FAVORITES-list unstar (row removal) stays non-optimistic, since reverting a removed row is more jarring than reverting a color/count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
2e8c9e7987
commit
bec12e4f46
@ -286,29 +286,30 @@ class RepositoryListViewModel(
|
|||||||
val mode = content.mode
|
val mode = content.mode
|
||||||
// Once this session has confirmed a star succeeded (favoriteAction == Starred), a repeat
|
// Once this session has confirmed a star succeeded (favoriteAction == Starred), a repeat
|
||||||
// tap on the ALL list now un-stars instead of starring again -- a real toggle, matching
|
// tap on the ALL list now un-stars instead of starring again -- a real toggle, matching
|
||||||
// what tapping an already-filled star obviously should do. Before this, a repeat tap either
|
// what tapping an already-filled star obviously should do.
|
||||||
// re-called star() and double-incremented the displayed count, or (briefly) was a dead-end
|
|
||||||
// no-op with no way back to unstarred without switching to the FAVORITES tab.
|
|
||||||
val alreadyStarredThisSession = mode == RepositoryListMode.ALL && repository.favoriteAction == FavoriteActionUiState.Starred
|
val alreadyStarredThisSession = mode == RepositoryListMode.ALL && repository.favoriteAction == FavoriteActionUiState.Starred
|
||||||
updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.InProgress) }
|
|
||||||
|
// Applied immediately, before the network call -- this app's Gitea host is a home NAS, not
|
||||||
|
// a datacenter, so waiting for the full round trip before moving the star/count at all made
|
||||||
|
// every tap feel laggy ("좀 늦게 카운팅이 변하는 느낌"). The ALL-list star/count now update
|
||||||
|
// optimistically on tap and only get rolled back in the catch block below if the request
|
||||||
|
// actually turns out to have failed; the FAVORITES-list unstar (which removes the whole
|
||||||
|
// row, a more disruptive thing to have to undo visually) still waits for confirmation.
|
||||||
|
when {
|
||||||
|
alreadyStarredThisSession -> updateRow(repository.coordinates) {
|
||||||
|
it.copy(favoriteAction = FavoriteActionUiState.Idle, starsCount = (it.starsCount - 1).coerceAtLeast(0))
|
||||||
|
}
|
||||||
|
mode == RepositoryListMode.ALL -> updateRow(repository.coordinates) {
|
||||||
|
it.copy(favoriteAction = FavoriteActionUiState.Starred, starsCount = it.starsCount + 1)
|
||||||
|
}
|
||||||
|
else -> updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.InProgress) }
|
||||||
|
}
|
||||||
|
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
try {
|
try {
|
||||||
when {
|
when {
|
||||||
alreadyStarredThisSession -> {
|
alreadyStarredThisSession -> favoritesApi.unstar(repository.coordinates)
|
||||||
favoritesApi.unstar(repository.coordinates)
|
mode == RepositoryListMode.ALL -> favoritesApi.star(repository.coordinates)
|
||||||
updateRow(repository.coordinates) {
|
|
||||||
it.copy(favoriteAction = FavoriteActionUiState.Idle, starsCount = (it.starsCount - 1).coerceAtLeast(0))
|
|
||||||
}
|
|
||||||
}
|
|
||||||
mode == RepositoryListMode.ALL -> {
|
|
||||||
favoritesApi.star(repository.coordinates)
|
|
||||||
// starsCount only ever reflects what the last full list load returned
|
|
||||||
// otherwise -- a successful star here needs its own +1, or the row keeps
|
|
||||||
// showing the pre-star count until the next reload.
|
|
||||||
updateRow(repository.coordinates) {
|
|
||||||
it.copy(favoriteAction = FavoriteActionUiState.Starred, starsCount = it.starsCount + 1)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
else -> {
|
else -> {
|
||||||
favoritesApi.unstar(repository.coordinates)
|
favoritesApi.unstar(repository.coordinates)
|
||||||
removeRow(repository.coordinates)
|
removeRow(repository.coordinates)
|
||||||
@ -329,7 +330,17 @@ class RepositoryListViewModel(
|
|||||||
failure,
|
failure,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.Failed) }
|
if (mode == RepositoryListMode.ALL) {
|
||||||
|
// Roll back the optimistic update above -- the tap turned out not to have
|
||||||
|
// actually succeeded server-side, so both the icon and the count need to
|
||||||
|
// revert to what they were before this attempt.
|
||||||
|
updateRow(repository.coordinates) {
|
||||||
|
val revertedCount = if (alreadyStarredThisSession) it.starsCount + 1 else (it.starsCount - 1).coerceAtLeast(0)
|
||||||
|
it.copy(favoriteAction = FavoriteActionUiState.Failed, starsCount = revertedCount)
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.Failed) }
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -353,6 +353,28 @@ class RepositoryListViewModelTest {
|
|||||||
assertEquals(5, updated.starsCount)
|
assertEquals(5, updated.starsCount)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `toggleFavorite updates the star and count immediately, before the network call resolves`() = runTest(dispatcher.scheduler) {
|
||||||
|
val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo", starsCount = 2))))
|
||||||
|
val viewModel = viewModel(browser)
|
||||||
|
advanceUntilIdle()
|
||||||
|
|
||||||
|
val row = (viewModel.state.value as RepositoryListUiState.Content).repositories.single()
|
||||||
|
viewModel.toggleFavorite(row)
|
||||||
|
|
||||||
|
// Deliberately no advanceUntilIdle() here yet: the launched coroutine that actually calls
|
||||||
|
// favoritesApi.star() hasn't run on the StandardTestDispatcher at all, but the row's
|
||||||
|
// favoriteAction/starsCount should already reflect the tap -- that immediacy is the whole
|
||||||
|
// point of the fix ("좀 늦게 카운팅이 변하는 느낌").
|
||||||
|
val immediatelyAfterTap = (viewModel.state.value as RepositoryListUiState.Content).repositories.single()
|
||||||
|
assertEquals(FavoriteActionUiState.Starred, immediatelyAfterTap.favoriteAction)
|
||||||
|
assertEquals(3, immediatelyAfterTap.starsCount)
|
||||||
|
assertEquals(emptyList<RepositoryCoordinates>(), browser.starredCalls)
|
||||||
|
|
||||||
|
advanceUntilIdle()
|
||||||
|
assertEquals(listOf(RepositoryCoordinates.of("owner", "repo")), browser.starredCalls)
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `a repeat tap on an already-starred row from the all-repositories list un-stars it`() = runTest(dispatcher.scheduler) {
|
fun `a repeat tap on an already-starred row from the all-repositories list un-stars it`() = runTest(dispatcher.scheduler) {
|
||||||
val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo", starsCount = 4))))
|
val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo", starsCount = 4))))
|
||||||
@ -429,6 +451,9 @@ class RepositoryListViewModelTest {
|
|||||||
val rows = (viewModel.state.value as RepositoryListUiState.Content).repositories.associateBy { it.name }
|
val rows = (viewModel.state.value as RepositoryListUiState.Content).repositories.associateBy { it.name }
|
||||||
assertEquals(FavoriteActionUiState.Failed, rows.getValue("a").favoriteAction)
|
assertEquals(FavoriteActionUiState.Failed, rows.getValue("a").favoriteAction)
|
||||||
assertEquals(FavoriteActionUiState.Idle, rows.getValue("b").favoriteAction)
|
assertEquals(FavoriteActionUiState.Idle, rows.getValue("b").favoriteAction)
|
||||||
|
// The optimistic +1 applied on tap must be rolled back once the star turned out to have
|
||||||
|
// failed -- otherwise a failed attempt would still leave the count permanently off by one.
|
||||||
|
assertEquals(0, rows.getValue("a").starsCount)
|
||||||
|
|
||||||
browser.favoriteActionFailure = null
|
browser.favoriteActionFailure = null
|
||||||
viewModel.toggleFavorite(rows.getValue("a"))
|
viewModel.toggleFavorite(rows.getValue("a"))
|
||||||
@ -436,6 +461,28 @@ class RepositoryListViewModelTest {
|
|||||||
|
|
||||||
val retried = (viewModel.state.value as RepositoryListUiState.Content).repositories.single { it.name == "a" }
|
val retried = (viewModel.state.value as RepositoryListUiState.Content).repositories.single { it.name == "a" }
|
||||||
assertEquals(FavoriteActionUiState.Starred, retried.favoriteAction)
|
assertEquals(FavoriteActionUiState.Starred, retried.favoriteAction)
|
||||||
|
assertEquals(1, retried.starsCount)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a failed un-star rolls back the optimistic decrement and returns the row to Starred`() = runTest(dispatcher.scheduler) {
|
||||||
|
val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo", starsCount = 4))))
|
||||||
|
val viewModel = viewModel(browser)
|
||||||
|
advanceUntilIdle()
|
||||||
|
|
||||||
|
val row = (viewModel.state.value as RepositoryListUiState.Content).repositories.single()
|
||||||
|
viewModel.toggleFavorite(row)
|
||||||
|
advanceUntilIdle()
|
||||||
|
val starred = (viewModel.state.value as RepositoryListUiState.Content).repositories.single()
|
||||||
|
assertEquals(5, starred.starsCount)
|
||||||
|
|
||||||
|
browser.favoriteActionFailure = ApiFailure.Network(java.io.IOException())
|
||||||
|
viewModel.toggleFavorite(starred)
|
||||||
|
advanceUntilIdle()
|
||||||
|
|
||||||
|
val afterFailedUnstar = (viewModel.state.value as RepositoryListUiState.Content).repositories.single()
|
||||||
|
assertEquals(FavoriteActionUiState.Failed, afterFailedUnstar.favoriteAction)
|
||||||
|
assertEquals(5, afterFailedUnstar.starsCount)
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|||||||
@ -593,3 +593,15 @@ This is the append-only implementation and handoff record shared by Codex and Cl
|
|||||||
- Known limitations: still optimistic (not re-fetched from the server) in both directions; a failed *unstar* attempt still collapses to the same flat `Failed` UI state as a failed star attempt, which can render as not-starred even if the repo is still actually starred server-side — accepted as a minor, harmlessly-retryable gap rather than adding more state to track which action was in flight.
|
- Known limitations: still optimistic (not re-fetched from the server) in both directions; a failed *unstar* attempt still collapses to the same flat `Failed` UI state as a failed star attempt, which can render as not-starred even if the repo is still actually starred server-side — accepted as a minor, harmlessly-retryable gap rather than adding more state to track which action was in flight.
|
||||||
- Next recommended task: get live confirmation that both starring (count up) and un-starring (count back down, tappable again) now work from the ALL list.
|
- Next recommended task: get live confirmation that both starring (count up) and un-starring (count back down, tappable again) now work from the ALL list.
|
||||||
- Next owner: Claude, continuing per the standing direction.
|
- Next owner: Claude, continuing per the standing direction.
|
||||||
|
|
||||||
|
## 2026-08-29 (continued) — TASK-034 third round: make the star/count update optimistic
|
||||||
|
|
||||||
|
- Agent and role: Claude, direct continuation of the same live session — with starring and un-starring both working correctly, the human product owner reported it still felt slow: "좋은데 바로바로 반응이 안되네 좀 늦게 카운팅이 변하는 느낌" (good, but doesn't respond immediately — the count feels like it changes a bit late).
|
||||||
|
- Root cause: every state/count update in `toggleFavorite` was applied only *inside* the coroutine, after `favoritesApi.star()/unstar()` had already returned — meaning the star/count visibly did nothing until the full network round trip to this app's home-NAS-hosted Gitea completed, the same underlying "this host is not a datacenter" latency class as TASK-031's HTTP timeout fix.
|
||||||
|
- How it was implemented: moved the `favoriteAction`/`starsCount` update to run synchronously in `toggleFavorite`, before `viewModelScope.launch { ... }` even starts, so the tap feels instant; the coroutine itself now only performs the actual `star()/unstar()` call. The catch block was extended to roll the optimistic change back (restore the pre-tap count, mark `Failed`) if the request actually turns out to have failed, so a failure doesn't leave the display stuck showing an action that never really happened. The FAVORITES-list unstar (which removes the whole row) deliberately stays non-optimistic, since reverting a *removed* row on failure is more jarring than reverting a color/count and wasn't part of what was reported as slow.
|
||||||
|
- Files changed: `RepositoryListViewModel.kt`, `RepositoryListViewModelTest.kt`, `tasks/034-favorite-star-count-not-incrementing.md`.
|
||||||
|
- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` BUILD SUCCESSFUL, all unit tests passing (new coverage: state/count updates immediately on tap, before the queued coroutine runs on the `StandardTestDispatcher`; a failed star and a failed unstar each roll the count back to its pre-tap value), 0 lint errors. Installed on the human product owner's device over wireless `adb`.
|
||||||
|
- Review findings and resolutions: None from a separate review pass.
|
||||||
|
- Known limitations: unchanged from earlier in this task — still optimistic rather than server-authoritative in both directions (accepted tradeoff), and the underlying "can't tell if already starred on load" limitation from TASK-018/TASK-030 is untouched.
|
||||||
|
- Next recommended task: get live confirmation that starring/un-starring now feels instant.
|
||||||
|
- Next owner: Claude, continuing per the standing direction.
|
||||||
|
|||||||
@ -1,4 +1,4 @@
|
|||||||
# TASK-034: Star count doesn't increment, then repeat tap doesn't un-star
|
# TASK-034: Star count doesn't increment, repeat tap doesn't un-star, then feels laggy
|
||||||
|
|
||||||
Status: implemented; all required checks pass; live-installed, awaiting the human product owner's confirmation
|
Status: implemented; all required checks pass; live-installed, awaiting the human product owner's confirmation
|
||||||
Owner: Claude
|
Owner: Claude
|
||||||
@ -6,7 +6,7 @@ Reviewer: Codex
|
|||||||
|
|
||||||
## Outcome
|
## Outcome
|
||||||
|
|
||||||
Two rounds of live feedback on the same feature, following TASK-033's OAuth scope fix. First: the
|
Three rounds of live feedback on the same feature, following TASK-033's OAuth scope fix. First: the
|
||||||
human product owner confirmed starring itself now works ("별표 눌러서 즐겨찾기는 되는데") but the
|
human product owner confirmed starring itself now works ("별표 눌러서 즐겨찾기는 되는데") but the
|
||||||
displayed star count didn't go up ("카운팅은 안올라가네") — `toggleFavorite`'s ALL-list success
|
displayed star count didn't go up ("카운팅은 안올라가네") — `toggleFavorite`'s ALL-list success
|
||||||
branch updated only `favoriteAction` (to `Starred`) and never touched `starsCount`. Fixed by
|
branch updated only `favoriteAction` (to `Starred`) and never touched `starsCount`. Fixed by
|
||||||
@ -14,7 +14,12 @@ incrementing `starsCount` in the same state update. Second, immediately after: w
|
|||||||
repeat-tap guard in place, tapping an already-starred row no longer un-starred it either
|
repeat-tap guard in place, tapping an already-starred row no longer un-starred it either
|
||||||
("이제 올라가는데 다시 눌렀을때 취소가 안되네") — the first fix's guard made a repeat tap a dead
|
("이제 올라가는데 다시 눌렀을때 취소가 안되네") — the first fix's guard made a repeat tap a dead
|
||||||
no-op instead of a real toggle. Replaced that guard with an actual toggle: a repeat tap on a
|
no-op instead of a real toggle. Replaced that guard with an actual toggle: a repeat tap on a
|
||||||
row this session knows is starred now calls `unstar()` and decrements the count back down.
|
row this session knows is starred now calls `unstar()` and decrements the count back down. Third:
|
||||||
|
with the toggle working correctly, the human product owner reported the response still felt slow
|
||||||
|
("좀 늦게 카운팅이 변하는 느낌") — every state/count update was applied only *after* the full
|
||||||
|
network round trip to this app's home-NAS-hosted Gitea completed, so a tap visibly did nothing for
|
||||||
|
however long that took. Made the star/count update optimistically, on tap, before the network call
|
||||||
|
starts; the update is only rolled back if the request actually turns out to have failed.
|
||||||
|
|
||||||
## Scope
|
## Scope
|
||||||
|
|
||||||
@ -45,6 +50,17 @@ row this session knows is starred now calls `unstar()` and decrements the count
|
|||||||
only tracks what happened *within this session's own interaction* with a row, same as
|
only tracks what happened *within this session's own interaction* with a row, same as
|
||||||
`favoriteAction` always has (TASK-018/TASK-030). A repository already starred before the app
|
`favoriteAction` always has (TASK-018/TASK-030). A repository already starred before the app
|
||||||
session started still shows outlined/untapped until acted on directly.
|
session started still shows outlined/untapped until acted on directly.
|
||||||
|
- **Optimistic on tap, not after the network call, for the ALL list specifically**: `updateRow(...)`
|
||||||
|
for the star/unstar case now runs synchronously in `toggleFavorite`, before
|
||||||
|
`viewModelScope.launch { ... }` even starts, instead of inside the coroutine after
|
||||||
|
`favoritesApi.star()/unstar()` returns. This app's Gitea host is a home NAS (the same root cause
|
||||||
|
category as TASK-031's timeout fix), so waiting for a full round trip before moving the star or
|
||||||
|
count at all made every tap feel delayed rather than instant. The catch block now explicitly
|
||||||
|
rolls the optimistic change back (`Failed` state, count restored to what it was before the tap)
|
||||||
|
if the request turns out to have actually failed, rather than only ever moving the display forward
|
||||||
|
on success as the first two rounds of this fix did. The FAVORITES-list unstar (which removes the
|
||||||
|
whole row) deliberately stays non-optimistic — reverting a *removed* row is a more jarring failure
|
||||||
|
case than reverting a color/count, and wasn't part of what was reported as feeling slow.
|
||||||
|
|
||||||
## Acceptance criteria
|
## Acceptance criteria
|
||||||
|
|
||||||
@ -52,11 +68,14 @@ row this session knows is starred now calls `unstar()` and decrements the count
|
|||||||
- [x] Tapping an already-`Starred` row again un-stars it: calls `unstar()`, decrements the count by
|
- [x] Tapping an already-`Starred` row again un-stars it: calls `unstar()`, decrements the count by
|
||||||
1, and returns the row to `Idle` (outlined, tappable to star again).
|
1, and returns the row to `Idle` (outlined, tappable to star again).
|
||||||
- [x] The count never displays below 0.
|
- [x] The count never displays below 0.
|
||||||
- [x] Unstarring from the FAVORITES list is unaffected (row removal, no count to update).
|
- [x] The star icon and count update immediately on tap, not after the network call resolves; a
|
||||||
|
failed request rolls the display back to its pre-tap state instead of leaving it stuck at the
|
||||||
|
optimistic value.
|
||||||
|
- [x] Unstarring from the FAVORITES list is unaffected (row removal, no count to update, still
|
||||||
|
waits for confirmation as before).
|
||||||
- [x] Build, unit tests, and lint pass.
|
- [x] Build, unit tests, and lint pass.
|
||||||
- [ ] Live confirmation from the human product owner that both starring (count up) and un-starring
|
- [ ] Live confirmation from the human product owner that starring/un-starring now feels instant —
|
||||||
(count back down) now work as expected from the ALL list — installed on the device but not
|
installed on the device but not yet re-tested in this exact session.
|
||||||
yet re-tested in this exact session.
|
|
||||||
|
|
||||||
## Required checks
|
## Required checks
|
||||||
|
|
||||||
@ -68,20 +87,23 @@ row this session knows is starred now calls `unstar()` and decrements the count
|
|||||||
|
|
||||||
## Evidence
|
## Evidence
|
||||||
|
|
||||||
- `RepositoryListViewModel.kt`: `toggleFavorite` now branches on whether the row is already
|
- `RepositoryListViewModel.kt`: `toggleFavorite`'s ALL-list `favoriteAction`/`starsCount` update
|
||||||
`Starred` this session — un-star path (`unstar()`, `starsCount - 1` coerced at 0, → `Idle`) and
|
now happens synchronously before the coroutine launches (optimistic), with the network call
|
||||||
star path (`star()`, `starsCount + 1`, → `Starred`) are both explicit `when` branches, replacing
|
itself reduced to just `favoritesApi.star()/unstar()` inside `viewModelScope.launch`; the catch
|
||||||
the first pass's early-return no-op guard.
|
block now rolls the optimistic change back on failure (restoring the pre-tap count, marking
|
||||||
|
`Failed`) instead of only ever advancing the display on success.
|
||||||
- `RepositoryListViewModelTest.kt`: `repo()` test helper gained a `starsCount` parameter; tests
|
- `RepositoryListViewModelTest.kt`: `repo()` test helper gained a `starsCount` parameter; tests
|
||||||
cover starring incrementing the count by 1, a repeat tap un-starring (calls both `star()` and
|
cover starring incrementing the count by 1, a repeat tap un-starring (calls both `star()` and
|
||||||
`unstar()`, decrements the count, returns to `Idle`), and a full star→unstar round trip landing
|
`unstar()`, decrements the count, returns to `Idle`), a full star→unstar round trip landing back
|
||||||
back at the original count.
|
at the original count, the state/count updating immediately on tap *before* the queued coroutine
|
||||||
|
has run on the `StandardTestDispatcher` (the optimism itself), and a failed star/unstar each
|
||||||
|
rolling the count back to its pre-tap value.
|
||||||
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
||||||
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing.
|
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing.
|
||||||
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
||||||
- Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed
|
- Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed
|
||||||
install succeeded) twice across this task's two rounds; not yet re-tested live against the final
|
install succeeded) three times across this task's three rounds; not yet re-tested live against
|
||||||
toggle behavior in this exact session.
|
the optimistic-update behavior in this exact session.
|
||||||
|
|
||||||
## Known limitations
|
## Known limitations
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user