feat(ENG-671): user-facing "Check for updates" control - #449
Conversation
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT (would request changes)
Two failure-path issues should be addressed before merge. The combined check can report "You're up to date" when one update channel actually failed, and the apply control can remain permanently busy when the updater resolves false. This is submitted as COMMENT because it is a self-review.
Summary of findings
| Severity | File | Issue |
|---|---|---|
| Major | src/main/updater.ts:148 |
Partial check failures are collapsed into a successful up-to-date result |
| Major | src/renderer/cowork/views/SettingsView.jsx:770 |
A resolved apply failure leaves the UI stuck on Updating |
What's working well
The unified result shape is clear, the pure summary cases are well covered, and the existing test/build/type checks pass. The remaining test gap is orchestration and UI failure behavior, which would directly exercise these two findings.
Reviewed by Codex · paul/eng-671-add-a-user-facing-check-for-updates-control → staging · 2026-07-22
|
Addressed both findings in 073799d: checkForUpdates() partial-failure collapse (updater.ts:148) — Stuck "Updating…" on a resolved Both fixes are backed by directly-tested pure decision functions ( |
pnewsam
left a comment
There was a problem hiding this comment.
Follow-up Code Review
Verdict: COMMENT (would request changes)
The prior stuck Updating… issue is resolved with a retryable failure state, and partial failures no longer produce a false You're up to date result. One material issue remains: the new summary discards an independently confirmed update whenever the other channel fails, so the manual check still diverges from the periodic poll and withholds the Update now action during a partial outage.
Summary of findings
| Severity | File | Issue |
|---|---|---|
| Major | src/main/update-logic.ts:248 |
A confirmed update is discarded when the other channel is inconclusive |
What's working well
The resolved-false apply path now returns to idle, displays a clear retry state, and is directly tested. The updated targeted tests, type checks, purity validation, and CI all pass.
Reviewed by Codex · paul/eng-671-add-a-user-facing-check-for-updates-control → staging · 2026-07-22
| ui: { updateAvailable: boolean; newVersion?: string; error?: boolean }; | ||
| server: { updateAvailable: boolean; latestVersion?: string; error?: boolean }; | ||
| }): UpdateCheckSummary { | ||
| if (input.offline || input.ui.error || input.server.error) { |
There was a problem hiding this comment.
Major: This still suppresses an independently confirmed update whenever the other channel fails.
For example, if the UI manifest is unavailable but the server check successfully finds an update, this branch returns updateAvailable: false; Settings renders only Couldn't check and provides no Update now action. The periodic poll continues with and surfaces/applies that same confirmed server update, so the manual check still does not mirror it. This also makes an unavailable manifest relevant in builds where UI OTA is disabled.
Please let a confirmed positive result win. The error outcome should apply when no channel found an update and at least one channel was inconclusive, or the result shape should represent both updateAvailable and a partial-check warning.
|
Addressed in c9df8ba: A channel's own error no longer invalidates a different channel's confirmed positive result — Also removed the
|
pnewsam
left a comment
There was a problem hiding this comment.
Follow-up Code Review
Verdict: COMMENT (would request changes)
The cross-channel partial-success case is fixed, but the same confirmed-positive rule is not yet applied within an aggregated channel result. checkForServerUpdate() can return both updateAvailable: true and error: true when one Git remote confirms a change while the sibling remote lookup fails; the summary currently discards that confirmed update.
Summary of findings
| Severity | File | Issue |
|---|---|---|
| Major | src/main/update-logic.ts:260 |
A positive result carrying a partial-error flag is still suppressed |
What's working well
The manifest pre-gate has been removed, cross-channel confirmed updates now win, the retryable apply failure is resolved, and current CI passes.
Reviewed by Codex · paul/eng-671-add-a-user-facing-check-for-updates-control → staging · 2026-07-22
Adds a "Check for updates" action to Settings → Updates (desktop only) that
runs an on-demand check and reports the result — checking / up to date /
update available (with version) / couldn't check (offline or error). When an
update is available it offers "Update now", which applies via the existing
server-first + UI apply + reload path.
Updates auto-apply at boot (ENG-858) and the mid-session periodic check never
auto-applies, so this is how a user acts on a detected update immediately
instead of waiting for the next launch.
The check reports UI (OTA) and server together, mirroring exactly what the
4-hour poll detects (minus the apply): the apply path updates both, so a
UI-only check would falsely report "up to date" while a server update was
pending. A connectivity probe first turns an unreachable network into an
explicit "couldn't check" rather than a misleading "up to date".
- update-logic.ts: pure summarizeUpdateCheck({offline, ui, server}) → summary
(+ 6 unit tests). Fail-closed: a failed check can't surface a spurious update.
- updater.ts: checkForUpdates() orchestrator (probe → parallel UI+server
detection → summarize). Repointed the existing UI_UPDATE_CHECK handler at it
(it was UI-only and unused by the renderer — no channel rename, IPC snapshot
untouched).
- host.ts / global.d.ts: surface checkForUpdates() through the bridge; web
resolves to a benign "up to date".
- SettingsView.jsx: the control, gated behind isElectron, styled to match the
existing sidebar update banner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…; fix stuck "Updating…" Addresses the two review findings on this PR: - checkForUpdates() now threads an explicit error state per channel (UI/OTA, server) instead of letting a swallowed network/prerequisite failure read as a clean "no update". summarizeUpdateCheck only reports "up to date" when both applicable channels completed successfully. The manifest host being unreachable now only skips the UI check — it no longer suppresses the independent server check, matching the existing periodic poll's behavior. - handleApplyUpdateNow now inspects applyUpdate()'s resolved boolean instead of only resetting state in `catch`. A resolved `false` (failed download, compat rejection, update disappeared between check and apply) is a normal, expected outcome, not an exception — previously it left the control stuck on "Updating…" with no way to retry short of remounting the view. Both fixes are backed by pure, directly-tested decision functions (summarizeUpdateCheck, nextApplyUpdateState) per this repo's existing convention, rather than testing the orchestration/JSX directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… inconclusive Follow-up to the previous fix. summarizeUpdateCheck was still discarding a genuinely-confirmed update on one channel whenever the other channel merely failed to check — e.g. an unreachable UI manifest host suppressed a server update the periodic poll would still surface (and could apply). A channel's own error should never invalidate a *different* channel's positive result, so a confirmed update now always wins: "couldn't check" only applies when neither channel found anything and at least one was inconclusive. `offline` now requires both channels to have failed (or an explicit override). Also dropped checkForUpdates()'s hasInternet() pre-gate entirely: it made an unrelated manifest-host outage relevant even in builds where UI OTA is disabled (checkForUIUpdate() already returns instantly with no network call in that case). The two channels now run fully independently with no gating at all. update-logic.ts remains at 100% coverage (5 new/updated cases). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Let a confirmed update remain actionable when its aggregated channel also reports a sibling lookup failure. Add a regression test for a server result carrying both updateAvailable and error.
37739e7 to
23284f9
Compare
…ell-aware Combines PR #449 (ENG-671) into this PR and integrates it with the shell notice so the two update paths agree (previously #449's manual check reported only UI + server, and could say "up to date" while a newer shell was available). - checkForUpdates() now runs the shell check alongside UI/server; a shell-check failure is swallowed to "no update" so it can't sink the whole check. - summarizeUpdateCheck folds the shell in: it counts toward updateAvailable (so "up to date" can't lie) but stays a distinct shellUpdateAvailable flag, since the shell is download-applied, not apply-in-place. - Settings: one unified "Software updates" section replaces the two separate ones. UI/server show an "Update now" apply card; a pending shell shows a "Download update" card, sourced from the fresh check or the background poll (so it still surfaces on a plain Settings visit, ENG-849). handleDownloadShell Update takes an optional explicit URL for the check-sourced case. update-logic.ts stays at 100%; added shell coverage to summarizeUpdateCheck and updated the exact-match assertions for the new field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Superseded by #453. All of this PR's commits have been folded into #453, and the manual "Check for updates" control is now integrated with the shell reinstall notice there (the check reports UI + server and shell, so it can't say "up to date" while a newer app version is available). Closing this and deleting the branch; ENG-671 will be closed by #453. |
Description
Adds a user-facing Check for updates control to Settings → Updates (desktop only), closing the last gap in the update UX: today an update check only happens on boot and the 4-hour periodic poll, surfaced via the sidebar banner — there is no explicit way for a user to check or trigger one.
The button runs an on-demand check and reports the result:
Why "Update now" still matters after ENG-858: updates now always auto-apply at boot, but the mid-session periodic (4h) check still never auto-applies — it only shows a banner. So this control is how a user acts on a detected update immediately, instead of waiting for the next launch. "Update now" runs the same server-first + UI apply + reload path as the boot check.
Why the check reports UI and server together: it mirrors exactly what the periodic poll detects (minus the apply). The apply path (
UI_UPDATE_APPLY) updates both server and UI, so a UI-only check would have reported "up to date" while a server update was pending. A connectivity probe runs first so an unreachable network becomes an explicit "couldn't check" rather than a misleading "up to date".Implementation
update-logic.ts— new puresummarizeUpdateCheck({offline, ui, server})→ display summary (ok/offline/updateAvailable/ per-component flags + versions). Fail-closed: a failed check can't surface a spurious update. Covered by 6 new unit tests.updater.ts— newcheckForUpdates()orchestrator: connectivity probe → parallelcheckForUIUpdate+checkForServerUpdate→ summarize, applying nothing. Repointed the existingUI_UPDATE_CHECKIPC handler at it (it was UI-only and unused by the renderer — no channel rename, IPC snapshot untouched).host.ts/global.d.ts— surfacecheckForUpdates()through the bridge (host.ts didn't expose it before); web resolves to a benign "up to date".SettingsView.jsx— the control, gated behindisElectron, styled to match the existing sidebar update banner.Fixes ENG-671.
Screenshots
Pending — desktop smoke on a packaged prod/staging build (see below).
Type of change
Checklist:
Verification
npm run typecheck(main + renderer + tests)npm test— 363 passed, incl. 6 newsummarizeUpdateCheckcases (offline, up-to-date, server-only, UI-only, both, missing-version)check:cowork-purity· ✅npm run build:rendererdev), so the interesting paths only light up in a packaged prod/staging build against the manifest host (or via theCOWORK_OTA_MANIFEST_URLQA seam). Left as draft until that smoke + screenshots are attached.🤖 Generated with Claude Code