[codex] structure desktop preview failures#3244
Conversation
Co-authored-by: codex <codex@users.noreply.github.com>
|
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 |
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: Timeline omits nested failure messages
- Restored cause-unwrapping logic in finalizeControlAction so that when a PreviewOperationError occurs, the timeline records the underlying error.cause.message instead of the generic wrapper message.
Or push these changes by commenting:
@cursor push 012e32aa35
Preview (012e32aa35)
diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts
--- a/apps/desktop/src/preview/Manager.ts
+++ b/apps/desktop/src/preview/Manager.ts
@@ -851,11 +851,13 @@
} else {
const error = Option.getOrNull(Cause.findErrorOption(exit.cause));
const interrupted = isPreviewAutomationControlInterruptedError(error);
+ const underlying =
+ isPreviewOperationError(error) && error.cause instanceof Error ? error.cause : error;
yield* replaceAction(tabId, {
...actionEvent,
status: interrupted ? "interrupted" : "failed",
completedAt,
- error: error instanceof Error ? error.message : String(error),
+ error: underlying instanceof Error ? underlying.message : String(underlying),
});
}
const tabs = yield* SynchronizedRef.get(tabsRef);
@@ -2520,6 +2522,7 @@
export type PreviewManagerError = typeof PreviewManagerError.Type;
export const isPreviewManagerError = Schema.is(PreviewManagerError);
+export const isPreviewOperationError = Schema.is(PreviewOperationError);
export const isPreviewAutomationControlInterruptedError = Schema.is(
PreviewAutomationControlInterruptedError,
);You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Approved This PR refactors error handling by replacing a generic error class with specific tagged error types, adding structured metadata to errors. Runtime behavior is unchanged - errors occur at the same points with more informative types. The test updates verify the new structure. You can customize Macroscope's approvability policy. Learn more. |
Co-authored-by: codex <codex@users.noreply.github.com>
Dismissing prior approval to re-evaluate 6168586
Co-authored-by: codex <codex@users.noreply.github.com>
Dismissing prior approval to re-evaluate 2431a34
Co-authored-by: codex <codex@users.noreply.github.com>
Dismissing prior approval to re-evaluate 09f6a22
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: Automation timeline omits cause details
- Added toTimelineMessage() static methods to PreviewAutomationEvaluationError and PreviewAutomationInvalidSelectorError that extract cause details, and updated finalizeControlAction to use them instead of falling through to the generic error.message.
- ✅ Fixed: detailKind mismatches detailLength
- Changed detailKind checks from
!== undefinedto truthiness checks and detailLength from??to||so that null and empty-string descriptions correctly fall through to the text field, keeping kind and length aligned.
- Changed detailKind checks from
Or push these changes by commenting:
@cursor push 24ffcaf9ae
Preview (24ffcaf9ae)
diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts
--- a/apps/desktop/src/preview/Manager.ts
+++ b/apps/desktop/src/preview/Manager.ts
@@ -876,9 +876,13 @@
const interrupted = isPreviewAutomationControlInterruptedError(error);
const errorMessage = isPreviewOperationError(error)
? PreviewOperationError.toTimelineMessage(error)
- : error instanceof Error
- ? error.message
- : String(error);
+ : isPreviewAutomationEvaluationError(error)
+ ? PreviewAutomationEvaluationError.toTimelineMessage(error)
+ : isPreviewAutomationInvalidSelectorError(error)
+ ? PreviewAutomationInvalidSelectorError.toTimelineMessage(error)
+ : error instanceof Error
+ ? error.message
+ : String(error);
yield* replaceAction(tabId, {
...actionEvent,
status: interrupted ? "interrupted" : "failed",
@@ -915,13 +919,8 @@
return Effect.fail(
new PreviewAutomationEvaluationError({
tabId,
- detailKind:
- description !== undefined
- ? "exception-description"
- : text !== undefined
- ? "exception-text"
- : "unknown",
- detailLength: description?.length ?? text?.length ?? 0,
+ detailKind: description ? "exception-description" : text ? "exception-text" : "unknown",
+ detailLength: description?.length || text?.length || 0,
cause: response.exceptionDetails,
}),
);
@@ -2467,6 +2466,13 @@
cause: Schema.Defect(),
},
) {
+ static toTimelineMessage(error: PreviewAutomationEvaluationError): string {
+ const cause = error.cause as
+ | { text?: string; exception?: { description?: string } }
+ | undefined;
+ return cause?.exception?.description || cause?.text || error.message;
+ }
+
override get message(): string {
return `Preview JavaScript evaluation failed in tab ${this.tabId}`;
}
@@ -2513,6 +2519,11 @@
cause: Schema.Defect(),
},
) {
+ static toTimelineMessage(error: PreviewAutomationInvalidSelectorError): string {
+ const cause = error.cause as { message?: string } | undefined;
+ return cause?.message || error.message;
+ }
+
get detail(): {
readonly selectorKind: PreviewAutomationSelectorKind;
readonly selectorLength?: number;
@@ -2595,6 +2606,10 @@
export const isPreviewAutomationControlInterruptedError = Schema.is(
PreviewAutomationControlInterruptedError,
);
+export const isPreviewAutomationEvaluationError = Schema.is(PreviewAutomationEvaluationError);
+export const isPreviewAutomationInvalidSelectorError = Schema.is(
+ PreviewAutomationInvalidSelectorError,
+);
export class PreviewManager extends Context.Service<
PreviewManager,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 09f6a22. Configure here.
Co-authored-by: codex <codex@users.noreply.github.com>


Summary
Validation
Note
Medium Risk
Broad refactor of error paths across preview automation, CDP control, and artifact I/O; callers still type as the union but behavior and failure shapes change. Privacy-sensitive encoding rules need to stay correct for IPC/logging.
Overview
Desktop preview and Playwright injection stop funneling failures through a single wrapped
PreviewManagerError/ genericautomationErrorhelpers. Failures are now typedSchema.TaggedErrorvariants united asPreviewManagerErrorandPlaywrightInjectedRuntimeError, withoperation,tabId,webContentsId, and artifact paths on operational errors.Automation and artifacts get dedicated tags (evaluation, invalid selector, target not found, control interrupted, path outside directory, image load, recording conflict, etc.). User-facing
messagestays generic;toTimelineMessageand similar helpers supply detail fromcausewithout baking browser/selector text into structural fields. Diagnostics use kinds and lengths (e.g.detailKind,selectorLength) so encoded payloads avoid raw selectors, locators, and exception bodies where tests require it; fullcauseis still retained for debugging.attempt/failwrappers are removed in favor of failing with the specific error class. Tests assert_tagshapes, capture failures, artifact sandboxing, clipboard empty image, interruption, and encoding privacy.Reviewed by Cursor Bugbot for commit d1301b2. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace generic
PreviewManagerErrorwith a discriminated union of structured error typesPreviewManagerErrorwrapper class in Manager.ts with aSchema.Unionof specific tagged error classes (e.g.PreviewOperationError,PreviewAutomationEvaluationError,PreviewArtifactPathOutsideDirectoryError, etc.), each carrying structured diagnostic fields.toTimelineMessagehelpers on relevant error classes so timeline messaging derives from structured error data rather than generic strings.attempt/attemptPromisehelpers to attach operation name,tabId, andwebContentsIdto everyPreviewOperationError, replacing prior ad-hoc error construction throughout the manager.PreviewManagerAPI callers now receive specific tagged errors instead of a generic wrapper; any code pattern-matching on the oldPreviewManagerErrorclass will need updating.Macroscope summarized d1301b2.