feat(updater): add update install mode preference (#4467) - #4543
feat(updater): add update install mode preference (#4467)#4543cbeaulieu-gt wants to merge 9 commits into
Conversation
Add a user-facing Automatic vs Manual update-install preference, persist it through /api/app-config, wire it into updater candidate selection, and add clearer manual-install copy for the DMG/installer case. - daemon: `updateInstallMode` config field (enum-validated, defaults to automatic; reachable via `od config set updateInstallMode manual`). - desktop: lazy-read the pref at check time and force installer/DMG selection when manual; `readUpdateInstallMode` helper wired into createDesktopUpdater. - web: Settings toggle (gated on canApplyInPlace), `requiresManualInstall` surfaced on UpdaterModel, and a manual-install copy branch in UpdaterPopup. - i18n: new updater/settings keys across all locales. Fix the dead manual-install copy branch: capabilitiesFor couples canOpenInstaller and requiresManualInstall, so `requiresManualInstall && !canOpenInstaller` was always false and the manual-install copy never rendered. Gate on requiresManualInstall alone. The frozen test now uses the real capability combo capabilitiesFor emits (canOpenInstaller: true) instead of an impossible one that masked the bug. Note: this is cross-platform PR1. macOS-native end-to-end acceptance (payload extraction, DMG drag-install, signed-bundle identity) defers to PR2. Fixes nexu-io#4467
|
Hi @cbeaulieu-gt! The PR body here is very easy to follow — the PR1/PR2 split and the updater UX tradeoff are laid out clearly. I've queued the reviewer routing and the follow-up checks this change needs on our side.
|
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we'll loop QA in once it's merge-ready (and design/product have signed off, where applicable). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92509be316
ℹ️ 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".
PerishCode
left a comment
There was a problem hiding this comment.
I found two blockers in the install-mode wiring that need to be fixed before this can satisfy #4467: the new app-config field is not part of the shared contract/config sync path, and the Settings control is hidden unless the current selected update artifact is already a payload.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.|
Hey @cbeaulieu-gt — following up on the review loop here: @PerishCode's blocking review covers the two changes that need to land on this head before this can move forward. The current blockers are:
Once those are addressed, the product/design/QA routing that's already queued on this PR can continue from there. |
… gate (nexu-io#4543) Resolves the two blockers from PerishCode's review on nexu-io#4543: - Contract/sync: add `updateInstallMode` to the shared `AppConfigPrefs` (packages/contracts), merge it from daemon config in `mergeDaemonConfig`, and include it in `syncConfigToDaemon`. Previously the field lived only on the daemon-local shape and the Settings dialog's direct PUT, so the normal web merge/sync path dropped it — leaving the toggle and the daemon updater able to disagree after a daemon-hydrated load. - Toggle visibility: gate the Settings "Update install mode" control on a new artifact-independent `payloadCapable` signal (derived in `deriveUpdaterModel` as host + supported + package-launcher) instead of `canApplyInPlace`. `canApplyInPlace` is artifact-aware and false until a payload candidate is selected, which hid the preference exactly when the user wants to opt out of payload updates before one is picked. Tests: contract/sync round-trip in web config-state tests; SettingsDialog visibility on idle and installer-pending payload-capable runtimes (toggle shown) vs non-package-launcher runtimes (hidden).
PerishCode
left a comment
There was a problem hiding this comment.
I found one remaining blocker in the install-mode config surface: the new preference can be set, but the advertised CLI unset path does not actually clear the stored value.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.|
Hey @cbeaulieu-gt — quick follow-up on the new head: @PerishCode has narrowed this down to one remaining blocker on the current commit. The remaining item is the advertised Once that unset path is fixed on this head, the review can move forward from there. |
…nexu-io#4543) Addresses PerishCode's third review blocker on nexu-io#4543. `od config unset <key>` deleted the key client-side then PUT the remaining object, but the daemon's writeAppConfig is merge-style (only applies keys present in the body), so the stored value survived — `od config unset updateInstallMode` could not return to the documented unset⇒automatic state. Send `{ ...cfg, [key]: null }` instead of omitting the key, matching the daemon's merge-patch + null-clears convention (applyConfigValue already deletes updateInstallMode on null; existing writeAppConfig({ agentId: null }) behaviour relies on the same idiom). Regression test exercises the real CLI unset path against a stub daemon and asserts the PUT body carries `{ updateInstallMode: null }`.
PerishCode
left a comment
There was a problem hiding this comment.
I found one remaining blocker in the install-mode config merge path: clearing the preference through the CLI can still be undone by the web app's persisted local state.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234b70ae66
ℹ️ 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".
| setCfg((prev) => ({ ...prev, updateInstallMode: mode })); | ||
| await fetch('/api/app-config', { | ||
| method: 'PUT', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify({ updateInstallMode: mode }), | ||
| }).catch(() => {/* best-effort */}); |
There was a problem hiding this comment.
Reconcile pending payload when switching to manual
When a payload update has already been selected or downloaded, changing this setting to manual only updates React state and writes the daemon config. The desktop updater reads updateInstallMode only during checkForCandidate, while installUpdate still applies the current activeRelease, so a user who switches to Manual after an automatic payload download can still click the existing update prompt and get the in-place payload relaunch instead of the installer path. Please trigger/reconcile a new updater selection here, clear the active payload, or make install re-check the preference before applying a payload.
Useful? React with 👍 / 👎.
|
Hey @cbeaulieu-gt — one more follow-up on the latest head: @PerishCode has reduced the current review to a single remaining blocker in the config merge path. The remaining issue is that clearing Once that local-state merge case is fixed on this head, the review can move forward from there. |
mergeDaemonConfig preserved the local updateInstallMode when the daemon config omitted the field, so a browser holding 'manual' in localStorage kept it. App.tsx bootstrap then re-synced that stale value back to the daemon, undoing `od config unset updateInstallMode` and breaking the absent/unset => automatic contract. Treat omission from a present daemon config as a clear: the daemon is authoritative for this key. Flip the merge round-trip test to assert the local manual value is removed when the daemon omits the field.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PerishCode
left a comment
There was a problem hiding this comment.
I found one blocker in the install-mode lifecycle: switching to Manual after a payload is already downloaded can still apply the payload instead of moving the user back to the installer path.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.|
Hey @cbeaulieu-gt — another quick follow-up on the latest head: @PerishCode has the review narrowed to one remaining lifecycle blocker on this commit. The remaining issue is the Manual-mode switch after a payload is already downloaded: changing the preference in Settings still needs to force the updater back onto the installer/manual path instead of letting an already-prepared payload continue through the in-place apply flow. Once that downloaded-payload handoff is reconciled on this head, the review can continue from there. |
|
Hey @cbeaulieu-gt, @lefarcen, I picked up the remaining manual-mode lifecycle blocker from the latest review. I could not push directly to cbeaulieu-gt/open-design:feat/4467-update-install-mode from @TheAngryPit's auth because GitHub returned a 403, so I pushed the validated patch here instead:
What this patch changes:
Proof run locally:
Aegis, on behalf of @TheAngryPit |
|
Hey @TheAngryPit — thanks for doing the legwork on this and for dropping a validated branch/commit with the exact proof run. That’s very helpful. @cbeaulieu-gt, the current blocker looks well-scoped to the patch linked above. If you cherry-pick or port |
|
Both review blockers on the latest head are addressed. F1 — stale local state could undo the CLI unset ( F2 — switching to Manual after a payload was already downloaded ( Verified locally on Windows:
Current head: 🤖 Generated by Claude Code on behalf of @cbeaulieu-gt |
|
Hey @TheAngryPit — the part that still needed movement here wasn't another code change so much as the product call on whether the Manual path should ship at all. This PR already had the Settings → About install-mode toggle, the clearer Open installer updater copy, and the daemon/CLI/API wiring in place; design sign-off landed on the same head, but the PR was then auto-closed while waiting on that last product-direction decision. The most helpful next step is to carry the concrete user cases you outlined back into #4467 and ask whether the team wants to reopen this exact PR or take a smaller follow-up from there. If you and @cbeaulieu-gt still want to push it forward, that issue is the clean place to anchor the reopen / next-PR decision. |
|
Closing this PR for now because it has been waiting on an author response for more than 5 days after reviewer or maintainer feedback. This is only a queue-management step, not a rejection of the work. If you would like to continue, please leave a comment or push an update and reopen the PR when ready. |
|
Seriously actions-bot. It was just reopened. |
|
@cbeaulieu-gt You're right — that auto-close comment landed at the wrong moment. The code-side review on If that decision lands positive, reopening this PR (or spinning the narrower follow-up from #4467) is still the right next step. |
…all-mode # Conflicts: # apps/web/src/i18n/locales/zh-CN.ts # apps/web/src/i18n/locales/zh-TW.ts
Visual regression reviewHead:
0 changed · 0 unchanged · 0 new without baseline · 1 failed Capture or diff failures
Visual diff is advisory only and does not block merging. |
|
@PerishCode This PR's current head c2c9f65 is waiting on your review. |
reconcileManualModeBeforePayloadInstall only reselected the payload path on Automatic->Manual. Switching Manual->Automatic after an installer/DMG was already downloaded left installUpdate() opening the stale installer until the next update check, per chatgpt-codex-connector review on PR nexu-io#4543. Renamed to reconcileInstallModeBeforeInstall and generalized the gate to a symmetric mismatch check (wantsManual !== activeIsPayload) so both directions reconcile. Added hasValidLauncherPayloadContext guard on the new Automatic direction so an offline/no-launcher-context install still falls back to the installer instead of forcing a network reselect that could error. Added regression test covering the Manual->Automatic reselect case.
|
@PerishCode This PR's current head 217086d is waiting on your review. |
# Conflicts: # apps/desktop/src/main/updater.ts # apps/web/src/components/SettingsDialog.tsx # apps/web/src/components/UpdaterPopup.tsx # apps/web/src/i18n/locales/ar.ts # apps/web/src/i18n/locales/de.ts # apps/web/src/i18n/locales/en.ts # apps/web/src/i18n/locales/es-ES.ts # apps/web/src/i18n/locales/fa.ts # apps/web/src/i18n/locales/fr.ts # apps/web/src/i18n/locales/hu.ts # apps/web/src/i18n/locales/id.ts # apps/web/src/i18n/locales/it.ts # apps/web/src/i18n/locales/ja.ts # apps/web/src/i18n/locales/ko.ts # apps/web/src/i18n/locales/pl.ts # apps/web/src/i18n/locales/pt-BR.ts # apps/web/src/i18n/locales/ru.ts # apps/web/src/i18n/locales/th.ts # apps/web/src/i18n/locales/tr.ts # apps/web/src/i18n/locales/uk.ts # apps/web/src/i18n/locales/zh-CN.ts # apps/web/src/i18n/locales/zh-TW.ts # apps/web/src/i18n/types.ts # apps/web/tests/components/UpdaterPopup.test.tsx # apps/web/tests/state/config.test.ts
Follow-up to the origin/main merge commit (9c52816): the hand-resolved SettingsDialog.tsx carried forward a pre-existing duplicate import block from '../lib/updater' (TS2300 duplicate identifiers under tsc -b), and updater.ts's reconcileInstallModeBeforeInstall did not surface a failed metadata reselect as an error, silently returning the stale downloaded snapshot instead of the ERROR state the existing updater-update-install-mode.test.ts test expects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes #4467
Why
Following up on #4467 after #4471 landed the payload-capability foundation. #4471 made
capabilitiesForartifact-aware (payload ⇒ in-app install) but intentionally deferred the user-facing preference. This PR adds that preference and the clearer manual-install copy, as scoped with @lefarcen in the issue thread.The pain: on a packaged install there was no way to opt out of in-app payload updates back to the installer/DMG flow, and the "update ready" copy for the installer case read like a generic prompt rather than telling the user they need to run the installer themselves.
Who Manual mode is for
Manual is an escape hatch, not the happy path — most users should stay on Automatic. But since the app already has to understand the installer-vs-payload paths, leaving Manual wired gives power users and maintainers a controlled way to opt out of automatic apply when that is safer or easier to debug. Concrete cases (raised by @TheAngryPit in the thread):
Importantly, unset/absent still maps back to Automatic, so this does not change the default experience.
This is the cross-platform first slice (PR1). Per the agreement in #4467, macOS-native end-to-end acceptance (real payload extraction, DMG mount/Finder drag-install, signed-bundle identity) is deferred to a follow-up (PR2) that needs a Mac.
What users will see
od config set updateInstallMode manual(andget/unset) via the existing genericod configcommand — no new subcommand.Surface area
od config set updateInstallMode manual(rides the existing genericod configcommand; key added to the app-config allowlist — no new subcommand)/api/app-configgains an optionalupdateInstallMode: 'automatic' | 'manual'field (additive, backward-safe)Screenshots
Bug fix verification
This PR also fixes a dead-code branch found while wiring the manual-install copy:
capabilitiesForcouplescanOpenInstallerandrequiresManualInstall(both equalmanualInstaller), so the popup'sisManualInstallCaseguardrequiresManualInstall && !canOpenInstallerwas always false — the manual-install copy never rendered. The guard now keys onrequiresManualInstallalone.apps/web/tests/components/UpdaterPopup-update-install-mode.test.tsx安装更新instead of打开安装程序), and passes with the fix. The earlier version of this test used an impossible capability combo (canOpenInstaller: false+requiresManualInstall: true) thatcapabilitiesFornever emits, which masked the bug; it now uses the real combo (canOpenInstaller: true).Validation
pnpm typecheck— cleanpnpm --filter @open-design/web test— green (UpdaterPopup 12/12; pre-existing unrelated failures only: design-system style fixtures, sidecar-proxy socket)pnpm --filter @open-design/desktop test(run locally; not in PR CI gate) — update-install-mode suites 9/9pnpm --filter @open-design/daemon test— app-config round-trip greenpnpm guard— failures are pre-existing design-system component-fixture coverage only; zero overlap with the files in this PR🤖 Generated by Claude Code on behalf of @cbeaulieu-gt