Repository navigation
fix: retain library navigation context and review flows - #180
Conversation
cnhy-nero-diskard
left a comment
There was a problem hiding this comment.
Consolidated finding is inline.
cnhy-nero-diskard
left a comment
There was a problem hiding this comment.
Implementation finding is inline.
cnhy-nero-diskard
left a comment
There was a problem hiding this comment.
Consolidated implementation finding is inline.
| @@ -287,9 +254,7 @@ class HltbReviewViewModel @Inject constructor( | |||
| */ | |||
| fun resolve(appId: Long, candidate: HltbCandidate) = viewModelScope.launch { | |||
| hltbRepository.resolveMatch(appId, candidate) | |||
There was a problem hiding this comment.
!RESOLVE The reviewer protects stale-frame Skip/Next/Previous actions with expectedAppId, but match confirmation is still an unconditional persistence write, so an old UI frame can overwrite newer HLTB state. HltbRepository.resolveMatch() does a read followed by upsert() with no commit-time eligibility/version check. Repro: game A is visible as NEEDS_REVIEW, then another in-flight refresh/manual path updates A (for example resolves it automatically or replaces its candidate set) before Compose renders the queue emission; tapping a candidate from the stale frame still calls this method with A and overwrites the newer row. The same problem exists in confirmManualLink(), which eventually calls the same unconditional resolver. This is exactly the class of race the new session code already guards for stale Skip/Next/Previous, and broader search already avoids with a conditional DAO update. Make manual/candidate resolution atomic and conditional on the row version/state the UI actually reviewed (status alone is insufficient if NEEDS_REVIEW can be replaced with a newer NEEDS_REVIEW candidate set), return whether the write was accepted, and let a rejected stale action follow the current queue instead of overwriting it. Add a regression test where the row changes after the frame is produced but before candidate/manual confirmation.
There was a problem hiding this comment.
Resolved in commit cb7c4dd. Applied the requested review fix. Validation: configured commands passed.
cnhy-nero-diskard
left a comment
There was a problem hiding this comment.
Follow-up implementation finding is inline.
| ): LazyListState { | ||
| val items = remember(state, wishlist) { libraryScrollItems(state, wishlist) } | ||
| // No rememberSaveable: a newly launched process must never resurrect navigation's saved scroll. | ||
| val listState = remember(visit, generation, state.density) { |
There was a problem hiding this comment.
!RESOLVE [P2] Restoring a retained Wishlist scroll anchor can silently land on (and then save) the wrong Library row when Wishlist state arrives after the owned-library state. LibraryScreen only waits for LibraryUiState to finish loading, but WishlistViewModel.uiState starts as WishlistUiState(configured=false, entries=[]) and hydrates independently. If the saved visit.scrollAnchor.gameId belongs to a wishlist-only search result, the first libraryScrollItems(state, wishlist) omits that entire section. resolveLibraryScroll() falls back to the old numeric index, and the initial snapshotFlow capture overwrites the correct wishlist identity with whatever row occupies that index. When Wishlist emits its cached entries, remember(visit, generation, state.density) keeps the already-created LazyListState, so the original game is never re-anchored. Repro: search, scroll to a wishlist result, leave Library (e.g. Back to Home), reopen within the 5-minute visit while delaying the fresh Wishlist state emission; the return is at an unrelated row. Defer/reconcile restoration until the relevant Wishlist rows are hydrated, and do not capture a fallback as a new anchor before that reconciliation. Add a Compose regression test with a retained wishlist-only anchor and initially unconfigured/empty Wishlist state followed by cached results.
There was a problem hiding this comment.
Resolved in commit fb0ae39. Applied the requested review fix. Validation: configured commands passed.
Summary
Retains Library discovery context across detail/review returns, tab switches, and short background departures (5-minute visit), fixes Library bottom insets and top-level bar visibility, extends Library discovery (wishlist search, Added recently sort, concise Hide dialog), and makes HLTB review progression forward-only with session Skip plus grid and Settings entry points.
Covers feedback threads #157 (navigation/insets), #167 (library visit + discovery), and #163 (HLTB review jump) from index #174.
Changes
Release note
Testing
Notes