feat(freezer): named freeze profiles (#55a) - #295
Conversation
A profile is a named set of apps the user can freeze or unfreeze in one tap. Two new Room tables (auto-migration 5 -> 6, new tables only, so a shipped database keeps its watchlist), a profiles sheet, an editor sheet reusing the watchlist's app picker, and a "save this selection as a profile" entry in the multi-select toolbox. Profiles are deliberately not a subset of the watchlist. Watchlist membership is a standing instruction the QS tile and both launcher Freeze-all shortcuts act on with no UI, so folding profiles into it would mean creating a "Games" profile silently enlists every game in the tile's Freeze-all. What the two lists share is the runner. That sharing is the point, and it closes all three risks the recon pass named: - Tier gate. Profile runs go through BulkFreezeRunner, so targetsFor -> freezableCandidates applies the list-level FreezeTier.BLOCKED filter. A fourth surface calling setAppDisabled itself would have been the one bulk path able to freeze what every dialog refuses to offer a confirm button for. The editor warns at add time too, because a profile is a standing instruction that later runs act on with no UI. - Coalescing key. The job slot keyed on BulkOp alone would have made "freeze profile A" then "freeze profile B" return the first Deferred and silently never freeze B. The key is now BulkRequest(op, scope): same op / different scope serializes, a conflicting op still replaces. Replacement cancels the whole unsettled chain rather than its tail -- the tail is the run that has not started yet, the head is the one holding packages open. - Launcher restore. FreezerBridgeProvider now gates on the watchlist union every profile's membership, so an app frozen only by a profile is not a dead launcher tap. 17 unit tests cover the name rule (trimmed, capped, NOCASE-duplicate to match the index the database enforces) and the request identity.
Only the #55a rows and its detail section, per the convention the permission-filter branch wrote down: several open branches edit this file, so each touches its own rows and leaves the shared sequencing and "in review" paragraphs alone. The detail section now says which piece exists because of which risk, rather than listing the risks as open questions.
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Room-backed freeze profiles, profile-scoped bulk freeze/unfreeze execution, expanded restore authorization, Compose profile management, typed bulk outcomes, localized strings, tests, and related documentation. ChangesFreeze profile persistence
Scoped bulk execution
Profile UI and ViewModel flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfileEditorSheet.kt (1)
116-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApp-picker filtering/toggle logic duplicated between the profile editor and the watchlist manage sheet. Both files copy-paste the same type+search filtering
rememberblock, and separately duplicate the sameAppListTypeConnectedButtonGrouptoggle block (FreezeProfileEditorSheet.ktL204-L218 vsManageFreezerSheet.ktL119-L131) — the leaf tile (FreezerAppPickerItem) was correctly shared, but the surrounding grid controls were not.
app/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfileEditorSheet.kt#L116-L123: extract this filter block (and the L204-L218 type-toggle) into a shared helper/composable reused fromManageFreezerSheet.app/src/main/java/com/valhalla/thor/presentation/freezer/ManageFreezerSheet.kt#L83-L90: source of the duplicated filter logic (paired with L119-L131 for the toggle); consolidate both sheets onto the same shared implementation instead of two copies that can silently drift.🤖 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 `@app/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfileEditorSheet.kt` around lines 116 - 123, Extract the duplicated app type/search filtering and AppListType ConnectedButtonGroup toggle into shared helper/composable symbols, then update FreezeProfileEditorSheet.kt lines 116-123 and 204-218 and ManageFreezerSheet.kt lines 83-90 and 119-131 to reuse them. Preserve the existing filtering behavior, selected-type state, and toggle interactions in both sheets.
🤖 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 `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt`:
- Around line 269-298: Move the _runningRequest.value = request assignment out
of the synchronous launch() setup and into the async job after the previous job
has fully settled, immediately before this run begins package work. Keep
activeRequest and coalescing behavior unchanged, and ensure the running request
remains the previous request until the handoff completes.
In `@app/src/main/java/com/valhalla/thor/domain/model/FreezeProfile.kt`:
- Around line 48-54: The duplicate check in profileNameError must match SQLite
NOCASE rather than Kotlin Unicode-insensitive comparison. Update
FreezeProfile.kt’s duplicate validation to use an ASCII-only NOCASE-equivalent
comparison, and add a regression test in FreezeProfileTest.kt covering distinct
non-ASCII names such as Ä and ä; list both affected sites as required by the
consolidated review.
In
`@app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt`:
- Around line 338-367: Update the profile-write error handling so
SQLiteConstraintException uses an operation-appropriate toast: retain
error_profile_name_taken for create/update name conflicts, but use a
non-name-conflict error for deleteProfile failures. Adjust runProfileWrite or
provide distinct create/update and delete handling while preserving cancellation
propagation and logging.
---
Nitpick comments:
In
`@app/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfileEditorSheet.kt`:
- Around line 116-123: Extract the duplicated app type/search filtering and
AppListType ConnectedButtonGroup toggle into shared helper/composable symbols,
then update FreezeProfileEditorSheet.kt lines 116-123 and 204-218 and
ManageFreezerSheet.kt lines 83-90 and 119-131 to reuse them. Preserve the
existing filtering behavior, selected-type state, and toggle interactions in
both sheets.
🪄 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 Plus
Run ID: 0a940e65-d06a-4ddb-8d03-2791086bf15c
📒 Files selected for processing (26)
app/schemas/com.valhalla.thor.data.source.local.room.AppDatabase/6.jsonapp/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.ktapp/src/main/java/com/valhalla/thor/data/provider/FreezerBridgeProvider.ktapp/src/main/java/com/valhalla/thor/data/repository/FreezeProfileRepositoryImpl.ktapp/src/main/java/com/valhalla/thor/data/source/local/room/AppDatabase.ktapp/src/main/java/com/valhalla/thor/data/source/local/room/FreezeProfileDao.ktapp/src/main/java/com/valhalla/thor/data/source/local/room/FreezeProfileEntity.ktapp/src/main/java/com/valhalla/thor/di/Modules.ktapp/src/main/java/com/valhalla/thor/domain/model/BulkFreeze.ktapp/src/main/java/com/valhalla/thor/domain/model/FreezeProfile.ktapp/src/main/java/com/valhalla/thor/domain/model/RestoreRequest.ktapp/src/main/java/com/valhalla/thor/domain/repository/FreezeProfileRepository.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfileEditorSheet.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfilesSheet.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerScreen.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerSelectToolBox.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/ManageFreezerSheet.ktapp/src/main/java/com/valhalla/thor/presentation/tile/FreezerTileService.ktapp/src/main/res/values-ar/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/valhalla/thor/domain/model/FreezeProfileTest.ktdocs/feature-request-roadmap.md
trinadhthatakula
left a comment
trinadhthatakula
left a comment
There was a problem hiding this comment.
Code review summary
The profile schema, repository transactions, tier filtering, launcher restore gate, and request identity are internally consistent. I found no blocking correctness issue.
Non-blocking suggestions
-
Test runner behavior, not only request equality. The central risk in this PR is the new same-request coalescing, same-op serialization, and conflicting-op chain cancellation in
BulkFreezeRunner. Current profile tests proveBulkRequestidentity but do not execute those coroutine handoffs. Deterministic runner tests would protect the behavior that closes the roadmap risk. -
Keep editor state until persistence succeeds.
FreezerScreencloses the editor immediately after dispatching the asynchronous create/update. A constraint or I/O failure shows a toast after the draft has been discarded. Consider returning save state/result to the UI and dismissing only on success.
Verification
- Reviewed commit
c93472be89e06207a1ca0fc818abcde5b7d4a438in an isolated clone. FreezeProfileTestpassed.assembleFossDebugsucceeded.- GitHub checks are green.
- Room schema 6 contains the expected profile, membership, foreign-key, and index definitions.
Verdict
Approve with non-blocking suggestions.
Model: OpenAI GPT-5 (Codex)
Three review findings on the freeze-profiles branch. `runningRequest` was one slot, so a newer launch overwrote an older one — and on the serialize path the older one is the run still mutating packages. Running a profile freeze during a watchlist freeze therefore made the QS tile paint itself idle, and surface a stale "Froze N apps" subtitle, while the watchlist freeze it started was still going. The profiles sheet had the mirror bug: the first profile's spinner moved to the second row. It is now `runningRequests`, the whole chain in launch order, derived from one `unsettled` list that also carries the coalescing key — `activeJob` and `activeRequest` were a second copy of the chain's tail and are gone. The tile asks for membership, the sheet asks per row, and both are true while two runs are in flight. Retirement moves to `invokeOnCompletion`, because a job cancelled before its body ever ran never reaches the coroutine's `finally`: that job used to sit in `unsettled` for the process lifetime, which under the new flow would have pinned the tile on "Freezing…". `profileNameError` folded case the Kotlin way, which is Unicode-aware, while the `name COLLATE NOCASE` unique index folds ASCII only. It rejected "Ärger" against an existing "ärger" — a name the database would have taken, refused by an inline error the user cannot argue with. Now folded to match the index, with the constraint catch still there as the real backstop. `runProfileWrite` reported "that profile name is already taken" for every `SQLiteConstraintException`, including the foreign-key failure a delete can raise, which its own KDoc calls out. The message is now the caller's, since only the caller knows which constraint was reachable. assembleFossDebug + testFossDebugUnitTest + lintFossDebug --rerun-tasks: 228 tests, 0 failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt (1)
271-282: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA single Room failure permanently stops profile updates.
catchterminates the flow, so after one transient failurestate.profilesis frozen for the rest of the process — the profiles sheet keeps showing a stale list with no way to recover short of app restart.observeAppsalready uses a boundedretryWhenfor exactly this; consider the same here before falling through to the toast.♻️ Proposed change
freezeProfileRepository.observeProfiles() + .retryWhen { cause, attempt -> + if (cause is CancellationException || attempt >= 2) false + else { delay(500); true } + } // A Room read failure must not take the whole Freezer screen down with it: the // watchlist is a separate flow and stays perfectly usable without profiles. .catch { e ->🤖 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 `@app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt` around lines 271 - 282, Update observeProfiles to add the same bounded retryWhen recovery used by observeApps before the existing catch block, so transient Room failures retry and profile updates resume without restarting the app. Preserve the current logging, toast emission, and profile state collection after retries are exhausted.
🤖 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 `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt`:
- Around line 284-299: Update the coalescing logic in the surrounding runner
method to search all entries in unsettled for an active job with the same
request, rather than checking only newest. Return the matching in-flight job
before computing previous, cancelPrevious, or doomed, while preserving the
existing chain handoff behavior when no identical active request exists.
---
Nitpick comments:
In
`@app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt`:
- Around line 271-282: Update observeProfiles to add the same bounded retryWhen
recovery used by observeApps before the existing catch block, so transient Room
failures retry and profile updates resume without restarting the app. Preserve
the current logging, toast emission, and profile state collection after retries
are exhausted.
🪄 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 Plus
Run ID: a3edba58-ddc4-4e62-a04e-72253ef98238
📒 Files selected for processing (13)
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.ktapp/src/main/java/com/valhalla/thor/domain/model/FreezeProfile.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezeProfilesSheet.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerScreen.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.ktapp/src/main/java/com/valhalla/thor/presentation/tile/FreezerTileService.ktapp/src/main/res/values-ar/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/valhalla/thor/domain/model/FreezeProfileTest.ktdocs/follow-ups/bulk-freeze-runner-concurrency-tests.md
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/res/values-ar/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values/strings.xml
…aunch Fallout from the previous commit. Extending the runner from a single job slot to a serialized chain fixed what `runningRequests` publishes, but left the coalescing check reading only the tail. With a watchlist freeze running and a profile freeze queued behind it, a second watchlist freeze — a tile tap, a launcher shortcut — no longer matched the tail. It took the serialize path instead and enqueued a *second* full watchlist batch that re-acts on every package and posts its own result after the first one had already reported. That is exactly the double-act the method's contract says cannot happen, and the chain is the state that made it reachable. Match any unsettled entry, guarded on the chain being uniform in op. A conflicting-op launch cancels every entry from inside its own body, so a doomed entry still reports `isActive` for a moment; coalescing onto one would hand a caller a run about to be cancelled and its freeze would never happen. If every entry shares the incoming op, no such launch has happened — it would still be in the chain, carrying the op that fails the check. Found by CodeRabbit on the previous commit.
…lure `catch` ends the flow it is attached to, so one failed read left `profiles` frozen for the rest of the process: the sheet kept showing a stale list and nothing short of restarting the app brought it back. The app-list flow in this same view model already retries twice before falling through to its toast — this is the same bound, for the same reason.
Non-blocking suggestion from #295's review, recorded rather than fixed: the shape of the fix is a UX call (dismiss-on-success, optimistic-with-restore, or validate-first), and the branch's own risk was the bulk-run coalescing a profile freeze goes through, not this.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt (1)
372-379: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep bulk-run failures distinct from intentional no-ops.
BulkFreezeRunnerreturnsnullfor both “nothing to do” and caught Room/binder failures.FreezerViewModelthen reportsprofile_nothing_to_do; a failed run, or a successful package mutation followed by notifier failure, can therefore be presented as an empty profile.
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt#L372-L379: return a distinct failure outcome or propagate failures; do not rewrite successful execution asnull.app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt#L325-L330: map only the intentional no-op outcome toprofile_nothing_to_do.🤖 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 `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt` around lines 372 - 379, Keep intentional no-ops distinct from failures in BulkFreezeRunner: at app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt:372-379, return a dedicated failure outcome or propagate the exception instead of converting caught failures to null, while preserving successful mutation results. At app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt:325-330, map only the explicit no-op outcome to profile_nothing_to_do and handle failure outcomes separately.
🤖 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 `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt`:
- Around line 295-303: Update the coalescing logic in the unsettled-run lookup
to inspect only the current same-operation suffix, rather than requiring every
unsettled entry to have request.op. Start after the most recent entry with a
different operation, then reuse an active job matching request from that suffix
so queued identical operations share the existing run while older conflicting
operations remain excluded.
---
Outside diff comments:
In `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt`:
- Around line 372-379: Keep intentional no-ops distinct from failures in
BulkFreezeRunner: at
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt:372-379,
return a dedicated failure outcome or propagate the exception instead of
converting caught failures to null, while preserving successful mutation
results. At
app/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.kt:325-330,
map only the explicit no-op outcome to profile_nothing_to_do and handle failure
outcomes separately.
🪄 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 Plus
Run ID: 6abd6ab7-29a3-481c-b237-5d096c8eba4e
📒 Files selected for processing (5)
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.ktdocs/follow-ups/README.mddocs/follow-ups/bulk-freeze-runner-concurrency-tests.mddocs/follow-ups/profile-editor-dismisses-before-the-save-lands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/follow-ups/bulk-freeze-runner-concurrency-tests.md
BulkFreezeRunner returned null both for "there was nothing to act on" and for "this raised and was caught", so every surface that awaited a run reported a Room or binder failure as a no-op. That is the one wrong thing to say about a failure: it tells the user their apps are fine when nobody knows whether they are, on the run most likely to have left the watchlist half-frozen. The awaited value is now a BulkOutcome — Completed, NothingToDo or Failed. The profile toast and the launcher trampoline each say which of the three it was; the failure wording deliberately does not claim how far the batch got, because the runner does not know either. A notifier.post that throws no longer rewrites a completed run as a failure. The packages are already mutated by then, and failing to describe a freeze is not failing to perform it.
|
Latest CodeRabbit finding applied in "Keep bulk-run failures distinct from intentional no-ops" (Major) — confirmed, and it is worse than the report suggests, because it is the only thing the surface can say.
The awaited value is now sealed interface BulkOutcome {
data class Completed(val result: BulkResult) : BulkOutcome
data object NothingToDo : BulkOutcome
data class Failed(val cause: Throwable) : BulkOutcome
}
Both awaiting surfaces were fixed, not just the profile one — One thing the finding did not name. Strings added in all five locales. No test covers the new
|
Lint's AndroidGradlePluginVersion check ships at severity ERROR, not warning, so `warningsAsErrors` was never what promoted it and the GradleDependency / NewerVersionAvailable overrides in app/lint.xml never covered it. The moment alpha07 published upstream, lintStoreRelease began failing on gradle/libs.versions.toml:6 for every open PR at once, on a line none of them had touched. Taking the version the check is asking for clears it. Verified locally: lintFossDebug, lintStoreRelease and the unit tests all pass on alpha07 with --rerun-tasks.
github-actions
Bot
added
the
dependencies
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt (1)
286-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCoalescing check still doesn't cover a same-op suffix — this is a previously-flagged, still-unresolved issue.
unsettled.all { it.request.op == request.op }requires the entire chain to share the incoming op before attempting to coalesce. Once a conflicting-op replacement is underway (a doomed head entry hasn't retired yet, but a new tail entry with a different op is active), a repeat of that tail's request fails thisallcheck and falls through to the serialize path instead of reusing the active tail job — spawning a duplicate run of the same request (double package mutation report, double notification, double_completionsemission).Example:
unsettled = [A(FREEZE, cancelling), B(UNFREEZE, active)]. A secondUNFREEZErequest matchingBfailsunsettled.all { op == UNFREEZE }(A is still FREEZE), skips coalescing, and — sincecancelPreviousonly compares against the tail (B, same op) — takes the serialize path and creates a new runCthat duplicatesB.This is the same gap raised in a prior review round on this PR (
Coalesce against the current same-operation suffix), which was not marked as addressed.🔧 Proposed fix: match only the current same-op suffix
- if (unsettled.all { it.request.op == request.op }) { - unsettled.firstOrNull { it.job.isActive && it.request == request } - ?.let { return it.job } - } + val sameOpSuffix = unsettled.asReversed().takeWhile { it.request.op == request.op } + sameOpSuffix.firstOrNull { it.job.isActive && it.request == request } + ?.let { return it.job }🤖 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 `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt` around lines 286 - 306, Update the coalescing logic in launch so it considers only the contiguous same-operation suffix of unsettled, not the entire chain. For an incoming request, identify the current tail segment whose entries have request.op equal to request.op, then reuse an active matching job from that suffix; preserve the existing behavior for conflicting operations and avoid coalescing with entries outside the suffix.
🤖 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.
Duplicate comments:
In `@app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt`:
- Around line 286-306: Update the coalescing logic in launch so it considers
only the contiguous same-operation suffix of unsettled, not the entire chain.
For an incoming request, identify the current tail segment whose entries have
request.op equal to request.op, then reuse an active matching job from that
suffix; preserve the existing behavior for conflicting operations and avoid
coalescing with entries outside the suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6205fde1-f29e-4cba-8561-db21dd0f321f
📒 Files selected for processing (11)
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.ktapp/src/main/java/com/valhalla/thor/data/launcher/FreezerShortcutManager.ktapp/src/main/java/com/valhalla/thor/domain/model/BulkFreeze.ktapp/src/main/java/com/valhalla/thor/presentation/freezer/FreezerViewModel.ktapp/src/main/java/com/valhalla/thor/presentation/launcher/FreezerLaunchActivity.ktapp/src/main/res/values-ar/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xmlgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/res/values-ar/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values/strings.xml
trinadhthatakula
left a comment
trinadhthatakula
left a comment
There was a problem hiding this comment.
Review summary
This PR adds named freeze profiles, Room persistence and normalization, profile management UI, bulk freeze/unfreeze execution, result reporting, and supporting tests. The persistence and profile-flow changes are internally consistent, and the full Foss debug unit suite plus git diff --check pass at 4b2291091180f8f70914f6ad47e5b222bb7e11ca.
Blocking finding
[P1] Coalesce against the matching operation suffix, not only a completely uniform unsettled chain
app/src/main/java/com/valhalla/thor/data/freezer/BulkFreezeRunner.kt:303-305
The unsettled.all { it.request.op == request.op } guard misses a valid duplicate of the newest active request while a cancelled opposite-operation predecessor is still unwinding. For example:
- A
FREEZErequest is active. - B
UNFREEZEarrives, cancels A, and becomes the active replacement. - Before A leaves
unsettled, the sameUNFREEZErequest arrives again. - The chain is temporarily
[A(FREEZE), B(UNFREEZE)], so theallcheck fails and C is enqueued behind B. - B and C both execute the same unfreeze batch and report twice.
Please search only the contiguous same-operation suffix of unsettled for an active identical request (for example, iterate from the end while request.op matches), so stale cancelling predecessors do not prevent coalescing while an earlier same-op request across a conflicting operation is still excluded. A focused regression test for the A/B/repeated-B timing would protect the intended invariant.
Verdict
Changes requested. The feature is otherwise in good shape, but the duplicate bulk action can repeat package operations and user-visible results.
Model used: OpenAI GPT-5 (Codex)
Note: GitHub does not allow the PR author to submit a formal Request changes review, so this verdict is posted as a review comment.
The previous rule asked whether the *whole* unsettled chain shared the incoming op and only searched it for a match when it did. That is the right answer for a uniform chain and the wrong one for a mixed chain, which is exactly the shape a conflicting-op replacement leaves behind: 1. A(FREEZE) is in flight. 2. B(UNFREEZE) arrives, cancels A and becomes the replacement. 3. The same UNFREEZE arrives again before A has left the chain. 4. The chain is [A(FREEZE), B(UNFREEZE)], so the uniformity check fails and the repeat does not coalesce. 5. It falls through to the serialize path — same op as B, so nothing is cancelled — and queues a second identical unfreeze behind B. Both run the same batch, both report. The eligible set is the chain's trailing same-op run, which is not an approximation of "not doomed" but equal to it: an entry E is doomed by some later entry X whose op differs from the entry before X, so if everything from E to the end shares E's op, no such X can exist after E. Scanning back, the first op change marks a launch that cancelled everything before it. So the suffix answers the repeat above with B while still refusing to hand back A — which is the case the uniformity check was added for and which a plain search over the chain would get wrong in the other direction. Lifted out of launch() as `coalesceTargetIndex` so it can be tested at all. BulkFreezeRunner takes four final collaborators over Context/PackageManager and cannot be built on a JVM (see the BulkFreezeWorkerTest KDoc), but this part of it is a pure function of the chain's shape — and both ways of getting it wrong are silent. Coalescing too little runs the batch twice; coalescing too much hands back a run that is about to be cancelled, so the caller's freeze never happens. Indices rather than entries keeps InFlight private and copies nothing under the monitor. Nine cases in CoalesceTargetIndexTest, including both directions of the mixed-chain case and the scope-identity rule. Verified: 245 unit tests, assembleFossDebug, lintFossDebug, lintStoreRelease.
|
P1 fixed in
So the suffix answers the repeated-B case with B while still refusing to hand back the cancelled A — which is the case the uniformity check was added for, and the one a plain unconditional search over the chain would get wrong in the other direction. Both failure modes are silent, which is why it now has its own tests. On the regression test: 245 tests / 0 failures; |
Two of the three were fixed by this branch's feature work rather than by separate commits, which is what "fold into the #30 branch — same files" meant — but the doc still read as OPEN, so the closure was not checkable. (1) share now carries the bundle's own MIME from BundleFormat, so a .apks or .xapk is no longer announced to receivers as an installable package. (3) exportInto deletes its staged copy in a finally, under a run-owned staging scope, behind a getAllocatableBytes pre-flight — the export/share split the doc asked for. (2) is a decision, not a fix: the OBB field stays for phase 2. Option 1's case for deleting it rested on "removes a File.exists() off the app-list mapping path", and that is simply not true — the app list maps with isLightweight = true and skips the whole block. Deleting it would buy one stat per details-screen open at the price of a Room migration, with #295 already taking the schema to 6.
Both would have cost whoever picked them up real time. The ViewModel-behavior-tests row still reads "unblocked — the tests are just unwritten". They are written and merged: 8 tests on virtual time, plus the four ports it turned out to need because AppListViewModel was not constructible on a JVM at all. Its own doc has said so since 2026-07-30; only the index lagged. The BulkFreezeRunner row names four collaborators that block a JVM test. One of them stopped blocking when those ports shipped: the runner's sole use of PrivilegeManager is `state` (BulkFreezeRunner.kt:361), which is precisely PrivilegeStateProvider. Three left, and the port pattern is the template for them. Noted that this waits on #295, which rewrites the file.
# Conflicts: # docs/feature-request-roadmap.md
# Conflicts: # docs/follow-ups/README.md # docs/follow-ups/bulk-freeze-runner-concurrency-tests.md
It merged. The paragraph is deliberately byte-identical across every open branch so the merges stay clean, so the correction has to land on each of them in the same words — a differently-worded fix is the conflict it was written to avoid.
# Conflicts: # docs/follow-ups/README.md
The byte-identical-across-branches rule that governed this file only bought anything while several branches edited it at once. With one left, the note now says when to reinstate it rather than reading as a standing constraint on a file only this branch touches.
1 participant
Closes #55 (the #55a half — the process manager, #55b, stays declined).
A freeze profile is a named set of apps you can freeze or unfreeze in one tap.
freeze_profiles+freeze_profile_apps, via auto-migration 5 → 6. New tables only, nothing existing touched, so a shipped database gets them and keeps its watchlist. Schema export committed.Profiles are not a subset of the watchlist
Watchlist membership is a standing instruction that the QS tile and both launcher Freeze-all shortcuts act on with no UI at all. Folding profiles into it would mean creating a "Games" profile silently enlists every game in the tile's Freeze-all. The two lists stay independent; what they share is
BulkFreezeRunner.The three risks the roadmap named are closed
Tier gate. Profile runs route through
BulkFreezeRunner, sotargetsFor→freezableCandidates(...)applies the list-levelFreezeTier.BLOCKEDfilter for free. Routing through the runner is what earns the gate — a fourth surface callingsetAppDisableditself would have been the one bulk path able to freeze what every dialog in the app refuses to render a confirm button for. The editor additionally raisesAppRiskDialogat add time, because a profile is a standing instruction that later runs act on with no UI.Coalescing key. The runner's job slot keyed on
BulkOpalone would have made "freeze profile A" then "freeze profile B" return the firstDeferredand silently never freeze B. The key is nowBulkRequest(op, scope):join()), because two freezes do not contradict each other and cancelling A to start B would leave A half-frozen with nothing said about it;cancelAndJoin()), unchanged in spirit but now cancelling the whole unsettled chain rather than just its tail. Found in self-review: the tail of a serialized chain is the run that has not started yet, the head is the one holding packages open, so cancelling the tail alone left a freeze running alongside the replacing unfreeze.Launcher restore.
FreezerBridgeProvidernow gates on the watchlist ∪ every profile's membership. An app frozen only by a profile it is the sole member of used to be un-unfreezable from the launcher — a dead icon tap with nothing said about why.Tests
FreezeProfileTest— 17 cases over the two pure pieces: the name rule (trimmed, capped at 40, duplicate detection case-insensitive to match theCOLLATE NOCASEunique index, so the inline error and the database cannot disagree) andBulkRequestidentity, including a regression test that two profiles under the same op are not the same request.Suite is 226 tests, 0 failures (was 209 on
dev).assembleFossDebug,testFossDebugUnitTest --rerun-tasksandlintFossDebugall green — lint runs withwarningsAsErrors, so all five locales are translated.Notes for review
BulkFreezeRunner's outer catch returnsnullfor a genuinely failed run, which is the same value it uses for "no-op". The profiles sheet therefore reports a rare hard failure as "Nothing to do". Changing it would alter what the tile and both shortcuts report, so it did not belong in this branch.runningRequestalready names the queued one, so the spinner moves to the pending row a moment early. Self-healing (the tile re-sweeps ononStartListening) and left alone deliberately.Summary by CodeRabbit
New Features
Bug Fixes