[codex] Structure desktop update persistence errors#3261
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved Refactors desktop update error handling to use structured error classes with sanitized messages that don't leak sensitive information. Includes comprehensive test coverage. No behavioral changes beyond improved error presentation and proper interrupt handling. You can customize Macroscope's approvability policy. Learn more. |
Dismissing prior approval to re-evaluate 12714d8
Dismissing prior approval to re-evaluate 22015b0
Dismissing prior approval to re-evaluate b4641b4
b4641b4 to
f5ca17c
Compare
Dismissing prior approval to re-evaluate f5ca17c
a820cc0 to
6e0bb59
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Install failure leaves quitting stuck
- Added Effect.onError after the ElectronUpdaterQuitAndInstallError catchTags to reset updateInstallInFlightRef and desktopState.quitting when backendManager.stop or electronWindow.destroyAll fail with a non-updater error tag, preventing the stuck state.
Or push these changes by commenting:
@cursor push 2a00aa7b08
Preview (2a00aa7b08)
diff --git a/apps/desktop/src/updates/DesktopUpdates.ts b/apps/desktop/src/updates/DesktopUpdates.ts
--- a/apps/desktop/src/updates/DesktopUpdates.ts
+++ b/apps/desktop/src/updates/DesktopUpdates.ts
@@ -435,6 +435,11 @@
},
),
}),
+ Effect.onError(() =>
+ Ref.set(updateInstallInFlightRef, false).pipe(
+ Effect.andThen(Ref.set(desktopState.quitting, false)),
+ ),
+ ),
);
}).pipe(Effect.withSpan("desktop.updates.installDownloadedUpdate"));You can send follow-ups to the cloud agent here.
Dismissing prior approval to re-evaluate 459f9d7
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unexpected failures reject IPC promise
- Replaced Effect.onError with Effect.catchCause in both download and install handlers so unexpected defects are caught and return { accepted: true, completed: false } instead of propagating the failure to the IPC promise.
- ✅ Fixed: Interrupt leaves downloading status stuck
- The download catchCause handler now calls reduceDesktopUpdateStateOnDownloadFailure on interrupt-only causes, rolling back the status from 'downloading' to 'available' so the UI is not stuck.
Or push these changes by commenting:
@cursor push 3e0a9d307a
Preview (3e0a9d307a)
diff --git a/apps/desktop/src/updates/DesktopUpdates.test.ts b/apps/desktop/src/updates/DesktopUpdates.test.ts
--- a/apps/desktop/src/updates/DesktopUpdates.test.ts
+++ b/apps/desktop/src/updates/DesktopUpdates.test.ts
@@ -369,13 +369,13 @@
harness.emit("update-available", { version: "1.2.4" });
yield* flushCallbacks;
- const exit = yield* Effect.exit(updates.download);
- assert.equal(exit._tag, "Failure");
+ const result = yield* updates.download;
+ assert.equal(result.accepted, true);
+ assert.equal(result.completed, false);
- const failedState = yield* updates.getState;
- assert.equal(failedState.status, "available");
- assert.equal(failedState.errorContext, "download");
- assert.equal(failedState.message, "Desktop update download action failed unexpectedly.");
+ assert.equal(result.state.status, "available");
+ assert.equal(result.state.errorContext, "download");
+ assert.equal(result.state.message, "Desktop update download action failed unexpectedly.");
const changedState = yield* updates.setChannel("nightly");
assert.equal(changedState.channel, "nightly");
@@ -396,14 +396,14 @@
harness.emit("update-downloaded", { version: "1.2.4" });
yield* flushCallbacks;
- const exit = yield* Effect.exit(updates.install);
- assert.equal(exit._tag, "Failure");
+ const result = yield* updates.install;
+ assert.equal(result.accepted, true);
+ assert.equal(result.completed, false);
assert.isFalse(yield* Ref.get(desktopState.quitting));
- const failedState = yield* updates.getState;
- assert.equal(failedState.status, "downloaded");
- assert.equal(failedState.errorContext, "install");
- assert.equal(failedState.message, "Desktop update install action failed unexpectedly.");
+ assert.equal(result.state.status, "downloaded");
+ assert.equal(result.state.errorContext, "install");
+ assert.equal(result.state.message, "Desktop update install action failed unexpectedly.");
const changedState = yield* updates.setChannel("nightly");
assert.equal(changedState.channel, "nightly");
diff --git a/apps/desktop/src/updates/DesktopUpdates.ts b/apps/desktop/src/updates/DesktopUpdates.ts
--- a/apps/desktop/src/updates/DesktopUpdates.ts
+++ b/apps/desktop/src/updates/DesktopUpdates.ts
@@ -408,18 +408,22 @@
},
),
}),
- Effect.onError((cause) => {
- if (Cause.hasInterruptsOnly(cause)) {
- return Effect.void;
- }
- const error = new DesktopUpdateUnexpectedActionError({ action: "download", cause });
- return Effect.gen(function* () {
- yield* updateState((current) =>
- reduceDesktopUpdateStateOnDownloadFailure(current, error.message),
- );
- yield* logUpdaterError(error.message, { error });
- });
- }),
+ Effect.catchCause((cause) =>
+ Effect.gen(function* () {
+ if (Cause.hasInterruptsOnly(cause)) {
+ yield* updateState((current) =>
+ reduceDesktopUpdateStateOnDownloadFailure(current, "Download was interrupted."),
+ );
+ } else {
+ const error = new DesktopUpdateUnexpectedActionError({ action: "download", cause });
+ yield* updateState((current) =>
+ reduceDesktopUpdateStateOnDownloadFailure(current, error.message),
+ );
+ yield* logUpdaterError(error.message, { error });
+ }
+ return { accepted: true, completed: false };
+ }),
+ ),
Effect.ensuring(Ref.set(updateDownloadInFlightRef, false)),
);
}).pipe(Effect.withSpan("desktop.updates.downloadAvailableUpdate"));
@@ -459,18 +463,18 @@
},
),
}),
- Effect.onError((cause) =>
+ Effect.catchCause((cause) =>
Effect.gen(function* () {
yield* Ref.set(updateInstallInFlightRef, false);
yield* Ref.set(desktopState.quitting, false);
- if (Cause.hasInterruptsOnly(cause)) {
- return;
+ if (!Cause.hasInterruptsOnly(cause)) {
+ const error = new DesktopUpdateUnexpectedActionError({ action: "install", cause });
+ yield* updateState((current) =>
+ reduceDesktopUpdateStateOnInstallFailure(current, error.message),
+ );
+ yield* logUpdaterError(error.message, { error });
}
- const error = new DesktopUpdateUnexpectedActionError({ action: "install", cause });
- yield* updateState((current) =>
- reduceDesktopUpdateStateOnInstallFailure(current, error.message),
- );
- yield* logUpdaterError(error.message, { error });
+ return { accepted: true, completed: false };
}),
),
);You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 459f9d7. Configure here.
459f9d7 to
a82a80a
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
2c777a9 to
5c21085
Compare


Summary
Validation
Note
Medium Risk
Touches desktop update actions, quit/install flow, and what users see on failure, but behavior is tightened with recovery paths and broad unit tests rather than new external surface area.
Overview
Restructures desktop auto-update error handling so user-visible state and logs use stable, structural messages instead of raw updater or disk failure text, while preserving full cause chains on typed errors for programmatic handling.
Update channel changes now fail with
DesktopUpdateChannelPersistenceError(channel +DesktopSettingsWriteErrorcause),DesktopUpdateActionInProgressErrorincludes the requested channel, andisDesktopUpdateSetChannelErrorguards the set-channel error union. Settings writes drop thewriteErrorhelper and constructDesktopSettingsWriteErrorat each I/O boundary.Check, download, and install use
Effect.catchTagsfor the three Electron updater failure types; unexpected defects during download/install map toDesktopUpdateUnexpectedActionError, with state rollback on interrupt and quitting/install flags reset when install setup fails. Background pollers and malformed updater events get dedicated tagged errors; rawerrorevents becomeDesktopUpdaterReportedErrorwith bounded log annotations (errorTag,channel, etc.).Tests cover cause identity, secret-free messages, download retry after interrupt, and persistence failure wrapping.
Reviewed by Cursor Bugbot for commit 5c21085. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Structure desktop update persistence errors with bounded messages and typed error classes
DesktopUpdates.ts:DesktopUpdatePollerError,DesktopUpdateEventHandlingError,DesktopUpdaterReportedError,DesktopUpdateUnexpectedActionError, andDesktopUpdateChannelPersistenceError, replacing generic catch-all error handling.DesktopUpdateActionInProgressErrornow includes arequestedChannelfield and a more specific message;setChannelmaps persistence failures toDesktopUpdateChannelPersistenceErrorwith channel and underlying cause.{ errorTag, channel }) instead of raw cause strings, preventing secret leakage.Effect.onInterruptandEffect.catchCause.isDesktopUpdateSetChannelErrortype guard and extensive test coverage inDesktopUpdates.test.tsfor error preservation, recovery, and channel persistence behavior.Macroscope summarized 5c21085.