feat(updater): add update install mode preference (#4467)#4543
feat(updater): add update install mode preference (#4467)#4543cbeaulieu-gt wants to merge 4 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.
|
lefarcen
added
size/XL
|
🧪 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). |
![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: 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
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
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.| if (value === 'automatic' || value === 'manual') { | ||
| target[key] = value; | ||
| } else { | ||
| delete target[key]; |
There was a problem hiding this comment.
Make the advertised od config unset updateInstallMode path clear this value, not only direct null writes. This branch deletes updateInstallMode when the key reaches writeAppConfig with an invalid/null value, but the existing generic CLI unset flow fetches the config, deletes the key from the object, and PUTs the remaining object. Because writeAppConfig is merge-style and only calls applyConfigValue for keys present in the request, the stored manual value survives and users cannot get back to the documented absent/unset => automatic state via the CLI. Please either change the CLI unset path to send { updateInstallMode: null } for this merge-patch API, or add equivalent daemon/API handling plus a CLI-level regression test for od config unset updateInstallMode.
|
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
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.
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
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.| if (root != null) scheduleBackCleanup(root.realRoot, logger); | ||
| const selected = selectUpdateCandidateWithFallback(body, config, await hasValidLauncherPayloadContext(config)); | ||
| const pref = readUpdateInstallMode ? await readUpdateInstallMode() : undefined; | ||
| const preferPayload = pref === 'manual' ? false : await hasValidLauncherPayloadContext(config); |
There was a problem hiding this comment.
Reconcile the active release when the install mode changes to manual. This new logic only reads updateInstallMode while checkForCandidate selects a candidate, but the Settings dropdown writes the daemon preference without triggering a new selection or clearing an existing activeRelease. If a payload was already selected/downloaded while Automatic was active, installUpdate() still branches on activeRelease.ref.artifact.type === "payload" and applies the prepared payload, so the user can opt out in Settings and still get the in-place payload relaunch from the existing prompt. That breaks the promised Manual (installer) behavior. Please either re-run/reconcile updater selection when the setting is changed to Manual, clear/reselect a payload active release as an installer release, or make installUpdate() re-read the preference before applying a payload; add a regression test for switching to Manual after a payload has already reached DOWNLOADED.
|
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. |
3 participants
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.
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