fix(apps): re-check on Apps screen resume (#514)#573
Merged
Conversation
Contributor
WalkthroughAdds OnLifecycleResume action dispatched from AppsRoot on ON_RESUME; AppsViewModel invokes autoCheckForUpdatesIfNeeded(). checkAllForUpdates() now stamps lastAutoCheckTimestamp and sets isCheckingForUpdates at coroutine start; lastCheckedTimestamp still advances only after successful work. ChangesLifecycle-Driven Update Mechanism
Sequence DiagramsequenceDiagram
participant Lifecycle
participant AppsRoot
participant LifecycleObserver
participant AppsViewModel
participant UpdateCheck
Lifecycle->>AppsRoot: composition starts / provide LocalLifecycleOwner
AppsRoot->>LifecycleObserver: register DisposableEffect observer
Lifecycle->>LifecycleObserver: ON_RESUME event
LifecycleObserver->>AppsViewModel: onAction(OnLifecycleResume)
AppsViewModel->>UpdateCheck: autoCheckForUpdatesIfNeeded()
UpdateCheck->>UpdateCheck: check cooldown (lastAutoCheckTimestamp)
UpdateCheck->>UpdateCheck: trigger checkAllForUpdates() if allowed
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 301-303: autoCheckForUpdatesIfNeeded() can start multiple
concurrent checkAllForUpdates() runs when resumes happen quickly because
lastAutoCheckTimestamp is only updated after completion; add a single-inflight
guard (e.g., an atomic boolean or mutex) inside autoCheckForUpdatesIfNeeded() or
around the call site so that if a check is already running it returns early, and
ensure the guard is cleared when the running check completes or fails; reference
AppsAction.OnLifecycleResume, autoCheckForUpdatesIfNeeded(),
checkAllForUpdates(), and lastAutoCheckTimestamp when adding the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fefdd984-0782-4d13-bc21-c4cd946c7335
📒 Files selected for processing (3)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
Greptile Summary
Confidence Score: 5/5Safe to merge — changes are targeted, well-guarded by the existing 30-minute cooldown, and both previously-flagged issues appear resolved. No new P1 or P0 issues found. The inflight guard (stamping No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant LC as Lifecycle (ON_RESUME)
participant AR as AppsRoot
participant VM as AppsViewModel
participant ACFI as autoCheckForUpdatesIfNeeded()
participant CAU as checkAllForUpdates()
participant Net as Network
AR->>LC: addObserver (DisposableEffect)
LC-->>AR: ON_RESUME event
AR->>VM: onAction(OnLifecycleResume)
VM->>ACFI: call
alt "lastAutoCheckTimestamp < 30 min ago"
ACFI-->>VM: skip (cooldown active)
else cooldown expired
ACFI->>CAU: call
CAU->>CAU: stamp lastAutoCheckTimestamp (inflight guard)
CAU->>Net: syncInstalledAppsUseCase()
CAU->>Net: checkAllForUpdates()
alt success
CAU->>VM: update lastCheckedTimestamp
else failure
CAU->>VM: log error (lastCheckedTimestamp unchanged)
end
end
AR->>LC: removeObserver (onDispose)
Reviews (2): Last reviewed commit: "fix(apps): stamp lastCheckedTimestamp on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt (1)
206-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve coroutine cancellation in
checkAllForUpdates().
catch (e: Exception)currently swallowsCancellationException, which can break structured cancellation behavior in this ViewModel path.Suggested fix
- } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { logger.error("Check all for updates failed: ${e.message}") } finally { _state.update { it.copy(isCheckingForUpdates = false) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt` around lines 206 - 210, In checkAllForUpdates(), the broad catch (e: Exception) swallows CancellationException and breaks structured coroutine cancellation; change the error handling to preserve cancellation by rethrowing CancellationException (either add a specific catch(e: CancellationException) { throw e } before the general catch, or in the existing catch check if (e is CancellationException) throw e) and keep logging other exceptions via logger.error("Check all for updates failed: ${e.message}"), ensuring the finally block that updates _state.update { it.copy(isCheckingForUpdates = false) } still runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 206-210: In checkAllForUpdates(), the broad catch (e: Exception)
swallows CancellationException and breaks structured coroutine cancellation;
change the error handling to preserve cancellation by rethrowing
CancellationException (either add a specific catch(e: CancellationException) {
throw e } before the general catch, or in the existing catch check if (e is
CancellationException) throw e) and keep logging other exceptions via
logger.error("Check all for updates failed: ${e.message}"), ensuring the finally
block that updates _state.update { it.copy(isCheckingForUpdates = false) } still
runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46d1947f-ed91-4285-a175-155d69b655af
📒 Files selected for processing (2)
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.kt
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
1 participant
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.
Apps row could stay stuck at the old version when an external install (file-manager sideload, update via another app) landed while GHS was background-killed by an aggressive OEM ROM —
PackageEventReceivernever receivedPACKAGE_REPLACED, andSyncInstalledAppsUseCaseonly ran on cold start (hasLoadedInitialDataflag, once per VM lifetime). Result: GHS kept prompting for a version the user already installed (NextPlayer case in #514).Wire
AppsRootto fireAppsAction.OnLifecycleResumeonON_RESUME. VM routes throughautoCheckForUpdatesIfNeededwhich honors the existing 30-min cooldown — so rapid resumes don't burn GitHub rate limits, but the first resume after a missed external install catches the drift. Closes #514.Summary by CodeRabbit
New Features
Bug Fixes