fix(updater): suppress auto-update error dialogs (404 on latest-mac.y…#3012
fix(updater): suppress auto-update error dialogs (404 on latest-mac.y…#3012vanshx999 wants to merge 2 commits into
Conversation
…ml etc.)
Update-check failures (e.g. missing latest-mac.yml 404) are now logged
instead of broadcast as error dialogs to users. The error message from
electron-updater is misleading ('check your auth token') and not actionable.
Closes AgentWrapper#2270
|
|
||
| expect(BrowserWindow.getAllWindows).toHaveBeenCalled(); | ||
| expect(module.getUpdateStatus()).toEqual({ state: "error", message: "manual feed failed" }); | ||
| expect(module.getUpdateStatus()).toEqual({ state: "checking" }); |
There was a problem hiding this comment.
This test's name now contradicts its assertion. It is still called "keeps manual updater error events visible to the renderer", but after this change it asserts state: "checking", meaning the error is no longer surfaced. Either the title should be updated to describe the new behavior (error is swallowed and status restored), or, if manual errors are meant to stay visible, the assertion is wrong. As written a future reader will trust a title that is no longer true.
There was a problem hiding this comment.
Addressed in latest push the test name now accurately reflects the behavior since this error (non-404) is still surfaced to the renderer. The assertion has been reverted to expect { state: "error" }.
| broadcast(withActiveRequest({ state: "error", message: err?.message ?? String(err) })); | ||
| // Manual check failures (e.g. 404 on latest-mac.yml) are logged, not | ||
| // shown to users — the error message is misleading and not actionable. | ||
| console.error("update check failed:", err); |
There was a problem hiding this comment.
This suppresses every manual error, not just the misleading 404 on latest-mac.yml that #2270 is about. activeUpdaterOperation here is anything other than automatic-check, so this branch also swallows genuine manual-check and manual-download failures (network down, server 5xx, a real download failure) and restores the status silently.
For a user-initiated action that is a UX regression: the user clicks "Check for updates" / "Download" and on any failure sees nothing at all, arguably worse than the scary dialog. Recommend narrowing the suppression to the specific missing-manifest / 404 case (match on the electron-updater error code or message) and continuing to surface genuinely actionable errors, at least on the manual-download path.
Also note the two branches of this handler are now nearly identical (both console.error + restoreAutomaticCheckPreviousStatus), so the automatic-check split no longer does anything distinct.
There was a problem hiding this comment.
Addressed narrowed to only suppress errors matching isManifest404Error() (containing "HttpError: 404"). Other manual errors still broadcast to the renderer.
| ); | ||
| } catch (err) { | ||
| broadcast({ state: "error", message: (err as Error)?.message ?? "Download failed", requestId }); | ||
| console.error("update download failed:", err); |
There was a problem hiding this comment.
restoreAutomaticCheckPreviousStatus restores the automatic-check baseline, but this catch also covers a manual download the user explicitly triggered. Silently swallowing a download failure means the user waits and sees neither an error nor a completion. Previously this broadcast carried requestId; now nothing terminal is emitted for that request. How does the renderer resolve a manual request whose operation errors here? If it is waiting on a terminal state keyed by requestId, it may sit in "checking"/"downloading" indefinitely. Worth emitting a terminal (non-scary) status for manual, user-initiated requests rather than only logging.
There was a problem hiding this comment.
Addressed downloadUpdateNow catch now emits a terminal { state: "not-available" } for 404 manifest errors, and broadcasts the error for other failures, so the renderer always resolves its request.
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
vanshx999
left a comment
vanshx999
left a comment
There was a problem hiding this comment.
Thanks for the review @Pulkit7070! Pushed changes to narrow the suppression to only HttpError: 404 manifest errors via an isManifest404Error() helper other manual errors still surface. The downloadUpdateNow catch now also emits a terminal "not-available" state so the renderer doesn't hang. Tests updated and all 26 pass.
4 participants
What
Suppress misleading auto-updater error dialogs when
electron-updater404s onlatest-mac.yml(or similar release manifest files).Why
Closes #2270. The Electron auto-updater surfaces a raw
electron-updatererrorto users when it can't find
latest-mac.ymlin a release's artifacts. This is ascary, technical 404 that looks like an auth/security failure ("Please double
check that your authentication token is correct"), but it's really just a
missing/incomplete release manifest. It should not be propagated to users.
How
Three catch/error paths in
frontend/src/main/auto-updater.ts:autoUpdater.on("error")— manual check errors (automatic ones were alreadysilent) now log and restore previous status instead of broadcasting an error
state to the renderer.
checkForUpdatesNow()catch — logs errors instead of broadcasting.downloadUpdateNow()catch — logs errors instead of broadcasting.Updated test expectations in
auto-updater.test.tsto match the newsilent-failure behaviour.
Testing
npm run typecheck✓npm run test— 26/26 tests pass ✓Checklist
main(or continuing an existing PR branch)