From 2e8c9e7987496374ceef53620aea873714b0b829 Mon Sep 17 00:00:00 2001 From: choiminyeong Date: Sat, 29 Aug 2026 16:42:17 +0900 Subject: [PATCH] TASK-034 follow-up: repeat tap on a starred row now un-stars instead of no-op The prior fix for double-counting used an early-return guard that made a repeat tap on an already-Starred row do nothing -- fixed the count bug but also made the star impossible to undo from the ALL list. Replaced it with a real toggle: an already-starred-this-session tap now calls unstar() and decrements the count back down instead of being inert. Co-Authored-By: Claude Sonnet 5 --- .../repositories/RepositoryListViewModel.kt | 24 +++-- .../RepositoryListViewModelTest.kt | 28 +++++- docs/development-log.md | 13 +++ ...34-favorite-star-count-not-incrementing.md | 98 +++++++++++-------- 4 files changed, 110 insertions(+), 53 deletions(-) diff --git a/app/src/main/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModel.kt b/app/src/main/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModel.kt index b92af21..90511c2 100644 --- a/app/src/main/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModel.kt +++ b/app/src/main/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModel.kt @@ -284,17 +284,23 @@ class RepositoryListViewModel( fun toggleFavorite(repository: RepositoryRowUiModel) { val content = mutableState.value as? RepositoryListUiState.Content ?: return val mode = content.mode - // On the ALL list, a repeat tap after a successful star would otherwise re-call star() - // (harmless server-side -- PUT is idempotent) and, before this guard, double-increment the - // client-displayed count each time. Once this session has confirmed a star succeeded, - // further taps here are a no-op; unstarring from the ALL list isn't supported (see the - // FavoriteStar doc comment in RepositoryListScreen.kt for why). - if (mode == RepositoryListMode.ALL && repository.favoriteAction == FavoriteActionUiState.Starred) return + // 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 + // what tapping an already-filled star obviously should do. Before this, a repeat tap either + // 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 updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.InProgress) } viewModelScope.launch { try { - when (mode) { - RepositoryListMode.ALL -> { + when { + alreadyStarredThisSession -> { + favoritesApi.unstar(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 @@ -303,7 +309,7 @@ class RepositoryListViewModel( it.copy(favoriteAction = FavoriteActionUiState.Starred, starsCount = it.starsCount + 1) } } - RepositoryListMode.FAVORITES -> { + else -> { favoritesApi.unstar(repository.coordinates) removeRow(repository.coordinates) } diff --git a/app/src/test/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModelTest.kt b/app/src/test/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModelTest.kt index 4be2086..c66b104 100644 --- a/app/src/test/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModelTest.kt +++ b/app/src/test/java/com/ubisam/ubgitea/feature/repositories/RepositoryListViewModelTest.kt @@ -354,8 +354,8 @@ class RepositoryListViewModelTest { } @Test - fun `a repeat tap after a successful star is a no-op, not a second star call or count increment`() = runTest(dispatcher.scheduler) { - val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo")))) + 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 viewModel = viewModel(browser) advanceUntilIdle() @@ -363,13 +363,35 @@ class RepositoryListViewModelTest { viewModel.toggleFavorite(row) advanceUntilIdle() val afterFirstTap = (viewModel.state.value as RepositoryListUiState.Content).repositories.single() + assertEquals(FavoriteActionUiState.Starred, afterFirstTap.favoriteAction) + assertEquals(5, afterFirstTap.starsCount) viewModel.toggleFavorite(afterFirstTap) advanceUntilIdle() val afterSecondTap = (viewModel.state.value as RepositoryListUiState.Content).repositories.single() - assertEquals(1, afterSecondTap.starsCount) + assertEquals(FavoriteActionUiState.Idle, afterSecondTap.favoriteAction) + assertEquals(4, afterSecondTap.starsCount) assertEquals(listOf(RepositoryCoordinates.of("owner", "repo")), browser.starredCalls) + assertEquals(listOf(RepositoryCoordinates.of("owner", "repo")), browser.unstarredCalls) + } + + @Test + fun `starring then un-starring from the all-repositories list round-trips the count back to zero`() = runTest(dispatcher.scheduler) { + val browser = FakeBrowser(repositories = listOf(page1Of(repo("repo", starsCount = 0)))) + 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() + + viewModel.toggleFavorite(starred) + advanceUntilIdle() + + val unstarred = (viewModel.state.value as RepositoryListUiState.Content).repositories.single() + assertEquals(0, unstarred.starsCount) } @Test diff --git a/docs/development-log.md b/docs/development-log.md index 351c78a..25a2ef8 100644 --- a/docs/development-log.md +++ b/docs/development-log.md @@ -580,3 +580,16 @@ This is the append-only implementation and handoff record shared by Codex and Cl - Known limitations: still optimistic, not authoritative — if Gitea's real count ever differs from this app's local +1 (e.g. a concurrent star from another client), the display stays off until the next full list reload. The underlying "can't tell if already starred on load" limitation is unchanged; this only stops it from causing a visible count bug. - Next recommended task: get live confirmation that the count now increments on tap; otherwise continue per the standing direction on everything else still queued from this session. - Next owner: Claude, continuing per the standing direction. + +## 2026-08-29 (continued) — TASK-034 follow-up: repeat tap now un-stars instead of doing nothing + +- Agent and role: Claude, direct continuation of the same live session — immediately after confirming the count now increments, the human product owner reported "이제 올라가는데 다시 눌렀을때 취소가 안되네" (it goes up now, but tapping again doesn't un-star it). +- Bug found: the first pass's fix for the double-count bug added an early-return guard that made a repeat tap on an already-`Starred` row a complete no-op — stopped the double-count, but also made the star impossible to undo from the ALL list without switching to the FAVORITES tab, which is not what a filled/tappable star should do. +- How it was implemented: replaced the no-op guard with a real toggle — `toggleFavorite` now branches on the row's own `favoriteAction`: not-yet-starred-this-session calls `star()` (`+1`, → `Starred`), already-starred-this-session calls `unstar()` (`-1`, coerced at 0, → `Idle`). Tests updated to match: the old "repeat tap is a no-op" test was replaced with one asserting the repeat tap actually un-stars (both `star()` and `unstar()` get called, count and state both revert), plus a new round-trip test (star then unstar lands back at the original count). +- Files changed: `RepositoryListViewModel.kt`, `RepositoryListViewModelTest.kt`, `tasks/034-favorite-star-count-not-incrementing.md` (updated throughout to describe both rounds of this fix). +- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. Installed on the human product owner's device over wireless `adb`. +- Also this turn: the `task-015-notifications` worktree had been removed from disk between sessions (branch/commits were intact in the main repo); recreated via `git worktree add`, plus a fresh untracked `local.properties` pointing at the local Android SDK. +- Review findings and resolutions: None from a separate review pass. +- 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 owner: Claude, continuing per the standing direction. diff --git a/tasks/034-favorite-star-count-not-incrementing.md b/tasks/034-favorite-star-count-not-incrementing.md index 241bac1..4473572 100644 --- a/tasks/034-favorite-star-count-not-incrementing.md +++ b/tasks/034-favorite-star-count-not-incrementing.md @@ -1,4 +1,4 @@ -# TASK-034: Star count doesn't increment after a successful favorite +# TASK-034: Star count doesn't increment, then repeat tap doesn't un-star Status: implemented; all required checks pass; live-installed, awaiting the human product owner's confirmation Owner: Claude @@ -6,15 +6,15 @@ Reviewer: Codex ## Outcome -Following TASK-033's OAuth scope fix, the human product owner confirmed starring itself now works -("별표 눌러서 즐겨찾기는 되는데") but reported the displayed star count doesn't go up -("카운팅은 안올라가네"). `toggleFavorite`'s ALL-list success branch updated only `favoriteAction` -(to `Starred`) and never touched `starsCount`, so a row kept showing whatever count the last full -list load had returned, even after a star that the server had actually accepted. Fixed by -incrementing the row's `starsCount` in the same state update that marks it `Starred`. While fixing -this, found and closed a related gap: a repeat tap on an already-`Starred` row (the ALL list has no -way to know a repository's already-starred state on initial load, so the star stays tappable) would -have re-called `star()` and double-incremented the client-displayed count each time — now a no-op. +Two 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 +displayed star count didn't go up ("카운팅은 안올라가네") — `toggleFavorite`'s ALL-list success +branch updated only `favoriteAction` (to `Starred`) and never touched `starsCount`. Fixed by +incrementing `starsCount` in the same state update. Second, immediately after: with a naive +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 +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. ## Scope @@ -23,32 +23,40 @@ have re-called `star()` and double-incremented the client-displayed count each t - Excluded: No change to the FAVORITES-list unstar path (removing the row entirely already made its count irrelevant, no bug there). No change to how the initial list load populates `starsCount` (that already came from Gitea's own response, unaffected). No attempt to fetch the - server's authoritative post-star count instead of incrementing client-side (see Design notes). + server's authoritative post-star/unstar count instead of adjusting it client-side (see Design + notes). ## Design notes -- **Optimistic +1, not a re-fetch**: Gitea's star endpoint (`PUT`, 204 No Content) doesn't return - the new count, so getting the authoritative number back would mean a second network call - (re-fetching the repository) purely to update one number — not worth it for a value the app - already knows how to derive from something it just confirmed itself (the star succeeded, so the - count is exactly one higher than what was last displayed). -- **The repeat-tap guard closes a real double-count bug, not just a hypothetical one**: because the - ALL list can't determine a repository's already-starred state on load (an existing, accepted - limitation from TASK-018/TASK-030), a `Starred` row's star icon stays enabled and tappable — - nothing before this fix stopped a second tap from calling `star()` again and incrementing the - displayed count a second time for a repository the user only starred once. The guard checks the - row's own `favoriteAction` before doing anything, so it also saves a redundant network call, not - just a redundant `+1`. +- **Optimistic ±1, not a re-fetch**: Gitea's star/unstar endpoints (`PUT`/`DELETE`, 204 No Content) + don't return the new count, so getting the authoritative number back would mean a second network + call purely to update one number — not worth it for a value the app already knows how to derive + from something it just confirmed itself (the action succeeded, so the count is exactly one + higher or lower than what was last displayed). `starsCount` is floored at 0 via + `coerceAtLeast(0)` as a defensive guard, not because normal use can reach it. +- **A real toggle, not a guard**: the first pass at this fix added an early-return guard that made + a repeat tap on an already-`Starred` row do nothing — that stopped the double-count bug but also, + as immediately reported, made the star impossible to undo from the ALL list without switching to + the FAVORITES tab. The actual fix branches on the row's own `favoriteAction`: not-yet-starred-this- + session taps call `star()` (`+1`, → `Starred`), already-starred-this-session taps call + `unstar()` (`-1`, → `Idle`) — this both fixes the double-count and restores the obviously-expected + behavior of tapping a filled star to unfill it. +- **The underlying "can't tell if already starred on load" limitation is untouched**: this toggle + 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 + session started still shows outlined/untapped until acted on directly. ## Acceptance criteria - [x] Starring a repository from the ALL list increments its displayed star count by exactly 1. -- [x] Tapping an already-`Starred` row again does not call `star()` a second time and does not - change the displayed count further. +- [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). +- [x] The count never displays below 0. - [x] Unstarring from the FAVORITES list is unaffected (row removal, no count to update). - [x] Build, unit tests, and lint pass. -- [ ] Live confirmation from the human product owner that the count now visibly increments on tap — - installed on the device but not yet re-tested in this exact session. +- [ ] Live confirmation from the human product owner that both starring (count up) and un-starring + (count back down) now work as expected from the ALL list — installed on the device but not + yet re-tested in this exact session. ## Required checks @@ -60,28 +68,36 @@ have re-called `star()` and double-incremented the client-displayed count each t ## Evidence -- `RepositoryListViewModel.kt`: `toggleFavorite`'s ALL-list success branch now updates - `starsCount = it.starsCount + 1` alongside `favoriteAction = Starred`; new early-return guard - when the row's `favoriteAction` is already `Starred`. -- `RepositoryListViewModelTest.kt`: `repo()` test helper gained a `starsCount` parameter; two new - tests — starring increments the displayed count by 1; a repeat tap after a successful star is a - no-op (no second `star()` call, count stays at 1). +- `RepositoryListViewModel.kt`: `toggleFavorite` now branches on whether the row is already + `Starred` this session — un-star path (`unstar()`, `starsCount - 1` coerced at 0, → `Idle`) and + star path (`star()`, `starsCount + 1`, → `Starred`) are both explicit `when` branches, replacing + the first pass's early-return no-op guard. +- `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 + `unstar()`, decrements the count, returns to `Idle`), and a full star→unstar round trip landing + back at the original count. - `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL. -- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing (including the two new - ones above). +- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing. - `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors. - Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed - install succeeded); not yet re-tested live in this exact session. + install succeeded) twice across this task's two rounds; not yet re-tested live against the final + toggle behavior in this exact session. ## Known limitations -- Still optimistic, not authoritative: if Gitea's real count differs from this app's local - increment for any reason (e.g. another client starred the same repo around the same time), the +- Still optimistic, not authoritative: if Gitea's real count differs from this app's local ±1 for + any reason (e.g. another client starred/unstarred the same repo around the same time), the displayed number stays off until the next full list reload. Considered an acceptable tradeoff - given the alternative (a second network call per star) for a rarely-contended personal count. + given the alternative (a second network call per action) for a rarely-contended personal count. - The underlying "can't tell if already starred on initial load" limitation (TASK-018/TASK-030) - is unchanged — this task only stops it from causing a *visible count* bug, it doesn't resolve the - limitation itself. + is unchanged — a repository starred before the current app session still shows outlined until + acted on directly in this session, same as before this task. +- A failed *un*star attempt (network error mid-toggle) still collapses to the same flat `Failed` + state as a failed star attempt, which renders as outlined/not-starred on the ALL list even though + the repository may still actually be starred server-side — `FavoriteActionUiState.Failed` carries + no memory of which action was being attempted. Accepted as a minor, retryable rendering gap + rather than adding state to track it, since a retry from that visual state is harmless either way + (an unwanted second `star()` on an already-starred repo is a no-op server-side). ## Review findings