Every stock-launcher path disables or restores its paired setup helper (#157, #158) - #159
Conversation
#157, #158) #157: disabling Google TV's stock launcher from the App List, Optimize or the Launcher tab's Disable left Setup Wraith on, and it took the Home button back - the #122 state through a different door. All three go through disable_package_impl, which now reads the Home apps first and, once stock is off, turns the paired helper off through the takeover's guarded, verified path when another launcher already holds Home. A failure turns stock back on. With no other launcher on Home the helper is left on, since it may be the only Home left, and the message says how to finish. #158: Enable & set default on a disabled stock launcher now re-enables its paired helper, decided from the package state before the switch rather than the switch's transcript, and a helper that can't be confirmed back on is reported.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41291de36c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let stock_was_disabled = | ||
| if launchers().is_stock(package) && !launchers().disable_with_for(package).is_empty() { |
There was a problem hiding this comment.
When Google TV Home is restored from the App List or Optimize Restore on desktop or mobile, those callers invoke apps::enable_package, not set_default_launcher_impl; that command still only runs pm enable for the requested package. Consequently these common restore routes turn stock back on while leaving Setup Wraith disabled, despite the changelog's “any route” guarantee. Move the paired-helper restoration into the shared enable path or invoke it from every such caller.
Useful? React with 👍 / 👎.
| if stock_was_disabled { | ||
| let adb = state.adb_snapshot().await; | ||
| if let Some(warning) = | ||
| reenable_paired_helpers(&*adb, serial, package, &mut result.diagnostics).await |
There was a problem hiding this comment.
If the core switch returns after pm enable <stock> fails, stock_was_disabled is still true, so this block can successfully enable Setup Wraith while the stock launcher remains off. That recreates the dangerous state where the setup helper can take the Home button away from the user's launcher. Restore the helper only after the stock-enable command is known to have succeeded.
Useful? React with 👍 / 👎.
| result.last_error = Some(match result.last_error.take() { | ||
| Some(e) => format!("{e} {warning}"), | ||
| None => warning, | ||
| }); |
There was a problem hiding this comment.
When the stock launcher switch succeeds but the helper state is unreadable or pm enable for Setup Wraith fails, this stores the warning in last_error without clearing result.ok. Both desktop and mobile launcher screens display last_error only when ok is false, so the user sees an unconditional success message and never receives the retry guidance even though the helper remains off. Return a visible warning field or otherwise make this partial failure observable to the callers.
Useful? React with 👍 / 👎.
…is on, and show its warning on success Codex on #159: - App List and Optimize Restore re-enable stock through apps::enable_package, which never restored Setup Wraith. The shared enable path now does. - Enable & set default could turn the helper on after its stock enable had failed, recreating the state that steals the Home button. The helper is now restored only once stock is confirmed back on. - A helper that couldn't be re-enabled was stored in last_error on a successful switch, which neither UI shows. Both now append it to the success message.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60233e3931
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Outcome::LeftOn => result.message.push_str( | ||
| " Google TV's setup helper (Setup Wraith) is still on and can take the Home button. \ | ||
| Set another launcher as Home, then use Turn it off on the Launcher tab.", |
There was a problem hiding this comment.
When settle_paired_helpers_after_stock_disable returns LeftOn—for example, while Android still reports Setup Wraith rather than the enabled replacement after stock is disabled—this leaves result.ok true and puts the required recovery steps only in message. The desktop Launcher handler replaces that message with “disabled,” while desktop/mobile Optimize and mobile Apps likewise discard messages for successful actions, so these inspected flows report success even though the helper remains able to capture Home and the user is never told to finish the switch. Return a structured warning that every caller renders, or otherwise make this partial outcome non-successful.
Useful? React with 👍 / 👎.
| if let Some(warning) = | ||
| super::launcher::reenable_paired_helpers(&*adb, serial, package, &mut diagnostics).await | ||
| { | ||
| result.message.push_str(&format!(" {warning}")); |
There was a problem hiding this comment.
If enabling stock succeeds but reading or enabling its paired helper fails, this appends a warning while leaving ok true. Desktop and mobile Optimize only count successful results, and mobile Apps replaces a successful result with its own “Enabled …” toast, so those restore routes silently claim completion while Setup Wraith remains disabled. The launcher-result UI fix does not cover this newly added ActionResult path; expose a warning field consumed by these callers or make the partial restoration observable as a failure.
Useful? React with 👍 / 👎.
| // App List and Optimize Restore bring a disabled stock launcher back | ||
| // through here, so its paired setup helper (Setup Wraith) follows (#158). | ||
| if result.ok | ||
| && !super::loader::launchers() | ||
| .disable_with_for(package) | ||
| .is_empty() |
There was a problem hiding this comment.
This restoration hook only covers enable_package_impl, but the desktop Advanced picker includes every installed package and calls set_home_any_impl, which issues its own direct pm enable at launcher.rs:244. Selecting a disabled Google TV Home entry there therefore re-enables stock and can make it Home while leaving Setup Wraith disabled, despite the new any-route behavior. Reuse the paired-helper restoration after that enable succeeds.
Useful? React with 👍 / 👎.
…Advanced picker Codex on #159: - A stock disable that had to leave Setup Wraith on, and a stock enable whose helper couldn't follow, came back ok with the detail in the message. The Launcher tab, Optimize and mobile Apps replace successful messages, so both read as plain success. They now return not-ok with a message that says what happened and how to finish, which every caller shows. - The Advanced picker enables a disabled stock launcher itself and skipped the helper restore. It now shares the same before/after check as Enable & set default.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64cff76dcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| matches!( | ||
| adb.shell(serial, &format!("pm list packages -d {package}")).await, | ||
| Ok(out) if out.success() | ||
| && out.stdout.lines().any(|l| l.trim() == format!("package:{package}")) | ||
| ) |
There was a problem hiding this comment.
When the initial pm list packages -d call transiently fails, this collapses the unreadable state to false. Both launcher wrappers still proceed into their core operation and can successfully run pm enable <stock>, but they then skip restore_helper_if_stock_enabled, leaving Setup Wraith disabled with no warning. Preserve an unknown state and either restore the helper after confirming stock is enabled or report that its prior state could not be determined.
Useful? React with 👍 / 👎.
| result.last_error = Some(match result.last_error.take() { | ||
| Some(e) => format!("{e} {warning}"), | ||
| None => warning, |
There was a problem hiding this comment.
When a snapshot switches to a disabled Google TV stock launcher and the paired helper cannot be read or re-enabled, this keeps ok true and stores the warning only in last_error. The fresh uncovered caller beyond the fixed launcher UIs is apply_snapshot: snapshot.rs:445-454 replaces the message with a success string whenever r.ok is true, while the mobile snapshot screen reports the summary as success, so users are told the snapshot applied cleanly even though Setup Wraith remains off. Include successful warnings in the snapshot result or make this partial outcome non-successful.
Useful? React with 👍 / 👎.
…ote through snapshot apply Codex on #159: - A failed "was stock disabled?" read collapsed to no, so the helper was never restored and nothing said so. Unknown now counts as "may need it", and the helper is still restored only once stock is confirmed enabled. - Snapshot apply replaced a successful launcher result with its own success text, dropping a helper re-enable warning. The warning now rides along in the launcher line of the summary.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5ec864aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(match r.last_error { | ||
| Some(note) => format!( | ||
| "{launcher_pkg} via {}. {note}", | ||
| r.strategy.unwrap_or_default() | ||
| ), |
There was a problem hiding this comment.
When a snapshot successfully enables a disabled stock launcher but Setup Wraith cannot be restored, this puts the warning only in launcher_message. The fresh evidence against the earlier “fixed” response is that v2/mobile/src/screens/Snapshots.svelte:121-123 still displays only res.summary, which never incorporates this field, and selects a success toast when package/settings failures are empty. Mobile users therefore still see a clean success despite the requested recovery step; include the note in the summary or have that caller render launcher_message.
Useful? React with 👍 / 👎.
| // Reported as not-ok so every caller shows it: a success toast | ||
| // would hide that the helper can still take the Home button. | ||
| Outcome::LeftOn => { | ||
| result.ok = false; | ||
| result.message = format!( |
There was a problem hiding this comment.
When another launcher is installed but Android still resolves Home to Setup Wraith after pm disable succeeds, LeftOn is returned after the stock launcher has actually been disabled. Marking that outcome ok = false makes existing callers treat the mutation as rolled back: mobile Apps restores its optimistic row to enabled at v2/mobile/src/screens/Apps.svelte:437-439, while the desktop App List returns before updating or invalidating state at +page.svelte:1049-1053. The screens consequently contradict the TV precisely in this recovery scenario; represent the warning separately from whether the package-state change landed, or force callers to reload the actual state.
Useful? React with 👍 / 👎.
…nto the snapshot summary Codex on #159: - A stock disable that had to leave Setup Wraith on is reported not-ok, but the stock disable did land; the desktop App List kept its old state and mobile Apps flipped the row back to enabled. Both now re-read the device after any not-ok result instead of assuming nothing changed. - The mobile snapshot toast shows only the summary, which didn't include a launcher follow-up. The summary now carries it, and that apply is shown as a notice rather than a clean success.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fea28edf1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match super::launcher::settle_paired_helpers_after_stock_disable( | ||
| &*adb, | ||
| serial, | ||
| package, | ||
| inventory.as_deref(), | ||
| &mut diagnostics, | ||
| ) |
There was a problem hiding this comment.
When applying a snapshot whose disabled_packages contains Google TV Home while Setup Wraith is enabled—such as a snapshot captured before this fix—apply_snapshot still calls the direct disable_from_plan loop at snapshot.rs:348-370, bypassing this new settlement logic. If the current Home app already matches the snapshot, launcher_to_set is None, so nothing later disables the helper; stock is turned off while Setup Wraith remains able to capture Home. Route snapshot stock-launcher disables through the paired-helper safety flow as well.
Useful? React with 👍 / 👎.
| // Not-ok so Optimize and the App List show it rather than a plain | ||
| // "Enabled": stock is back, the helper is not. | ||
| result.ok = false; | ||
| result.message = format!("{package} was enabled. {warning}"); |
There was a problem hiding this comment.
When pm enable successfully turns stock back on but restoring Setup Wraith fails, this changes the result to ok = false even though the stock package state already changed. Fresh evidence after the warning-propagation fix is that mobile Apps reverts its optimistic row to disabled (Apps.svelte:298-303), while the desktop App List and Launcher return without refreshing (+page.svelte:1102-1109 and 1427-1430), leaving their rows inconsistent with the TV and even saying stock could not be enabled. Return mutation state separately or make every not-ok caller reload the device.
Useful? React with 👍 / 👎.
| Outcome::LeftOn => { | ||
| result.ok = false; | ||
| result.message = format!( |
There was a problem hiding this comment.
When stock is disabled but no replacement currently holds Home, LeftOn deliberately returns ok = false after the disable has landed. Fresh evidence beyond the earlier partial-outcome fix is that both launcher-specific callers still reload only on success: mobile Launcher.svelte:152-157 and desktop +page.svelte:1542-1550. Disabling stock from either Launcher screen therefore leaves the row shown as enabled until a manual refresh, despite the warning saying it was disabled; these callers should re-read state on this partial outcome too.
Useful? React with 👍 / 👎.
…lper during snapshot apply Codex on #159: - Not-ok results from stock enables and Launcher-screen disables can still have landed. The desktop App List enable rows, the desktop Launcher tab's enable and disable, mobile Apps and the mobile Launcher now all re-read the device instead of keeping or reverting their old state. - Snapshot apply disabled stock directly, so a snapshot captured before this fix could turn stock off and leave Setup Wraith on. It now reads the Home apps first and, after the launcher step, settles each disabled stock launcher's helper through the shared guarded path, reporting a left-on or rolled-back helper in the summary.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8a9118efc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .await?; | ||
| if pairs_helper && result.ok { |
There was a problem hiding this comment.
When ADB disconnects after the TV executes pm disable-user, run(...).await? returns early; similarly, a failure marker leaves result.ok false. In both cases this guard skips paired-helper settlement entirely, even though this module's takeover path explicitly accounts for an errored disable having landed. App List, Optimize, or Launcher can therefore leave stock off with Setup Wraith still on—the exact unsafe state this change is intended to prevent. Probe the resulting package state or conservatively re-enable stock before returning.
Useful? React with 👍 / 👎.
| Outcome::LeftOn => helper_notes.push(format!( | ||
| "{stock} is off but Google TV's setup helper (Setup Wraith) is still on and can \ | ||
| take the Home button. Set another launcher as Home, then use Turn it off on the \ | ||
| Launcher tab." | ||
| )), |
There was a problem hiding this comment.
Fresh evidence after the earlier snapshot-settlement fix is that a snapshot captured after a normal Google TV takeover contains both stock and Setup Wraith in disabled_packages, so disable_from_plan disables both before the launcher switch. If that switch fails or leaves current Home unreadable, settlement returns LeftOn; this branch then claims Wraith is still on and leaves stock off, although both paired packages are actually disabled and the TV may have no Home screen. Exclude paired helpers from the generic disable pass or track their actual state so this outcome restores stock instead of reporting the opposite state.
Useful? React with 👍 / 👎.
… when a snapshot leaves no Home Codex on #159: - A stock disable whose reply was lost or failure-marked can still have landed, and the helper step was skipped, leaving stock off with Setup Wraith on. The device is now asked, and the helper is settled whenever stock is actually off. - A snapshot captured after a takeover disables stock and Setup Wraith. If the launcher switch then leaves no other launcher on Home, the TV may have no Home screen; stock is now turned back on (or Emergency Recovery is named) instead of reporting the helper as still on.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b83516a45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let target = read_current_home(adb, serial) | ||
| .await | ||
| .ok() | ||
| .and_then(|r| r.package) | ||
| .filter(|p| p != stock && !catalog.is_stock(p) && !catalog.is_transient_home_holder(p)); |
There was a problem hiding this comment.
When the stock disable has landed and the preflight found Setup Wraith enabled, a transient failure of both Home-reading commands is collapsed to None here and then treated as though no other launcher holds Home. This leaves stock disabled and Setup Wraith enabled—the unsafe state this patch is intended to prevent—instead of rolling back. Fresh evidence after 7b83516 is that the landed-disable probe now reaches settlement, but a subsequent unreadable read_current_home result still needs to be distinguished from an affirmative no-target result and should cause stock to be re-enabled.
Useful? React with 👍 / 👎.
| result.ok = false; | ||
| result.message = format!( | ||
| "{package} was disabled, but Google TV's setup helper (Setup Wraith) is still \ | ||
| on and can take the Home button. Set another launcher as Home, then use Turn \ | ||
| it off on the Launcher tab." |
There was a problem hiding this comment.
When this branch reports that stock was actually disabled but Setup Wraith remains able to capture Home, the inspected mobile Optimize flow still discards result.message: v2/mobile/src/screens/Optimize.svelte:569 records only the app name, and lines 624-626 show only a generic failure count. Fresh evidence after the claimed warning fix is therefore that users running this action through mobile Optimize are still not told that the package state changed or that they must set another launcher and use Turn it off.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #156. Disabling Google TV's stock launcher from the App List, Optimize or the Launcher tab's Disable now turns Setup Wraith off with it once another launcher holds Home, through the takeover's guarded, verified path (failure turns stock back on; no other Home leaves the helper on and says so). Enable & set default on a disabled stock launcher re-enables the helper, which also covers mobile's stock restore. Three new tests, each shown to fail without its fix.
Closes #157
Closes #158
🤖 Generated with Claude Code