[codex] Structure native view resolution failures - #3353
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.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Uncached failures repeat logging
- Added a module-level boolean flag in each resolver that is set to true in the catch block, causing subsequent calls to short-circuit and return null without retrying or re-logging the error.
Or push these changes by commenting:
@cursor push cf34a749ca
Preview (cf34a749ca)
diff --git a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts
--- a/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts
+++ b/apps/mobile/src/features/diffs/nativeReviewDiffSurface.ts
@@ -130,6 +130,7 @@
}
let cachedNativeReviewDiffView: ComponentType<NativeReviewDiffViewProps> | undefined;
+let reviewDiffResolutionFailed = false;
function getExpoViewConfig(moduleName: string) {
return (globalThis as typeof globalThis & ExpoGlobalWithViewConfig).expo?.getViewConfig?.(
@@ -142,6 +143,10 @@
return cachedNativeReviewDiffView;
}
+ if (reviewDiffResolutionFailed) {
+ return null;
+ }
+
if (getExpoViewConfig(NATIVE_REVIEW_DIFF_MODULE_NAME) == null) {
return null;
}
@@ -151,6 +156,7 @@
NATIVE_REVIEW_DIFF_MODULE_NAME,
);
} catch (cause) {
+ reviewDiffResolutionFailed = true;
console.error(
new NativeViewResolutionError({
nativeModuleName: NATIVE_REVIEW_DIFF_MODULE_NAME,
diff --git a/apps/mobile/src/features/terminal/nativeTerminalModule.ts b/apps/mobile/src/features/terminal/nativeTerminalModule.ts
--- a/apps/mobile/src/features/terminal/nativeTerminalModule.ts
+++ b/apps/mobile/src/features/terminal/nativeTerminalModule.ts
@@ -35,6 +35,7 @@
}
let cachedNativeTerminalSurfaceView: ComponentType<NativeTerminalSurfaceProps> | undefined;
+let terminalResolutionFailed = false;
function getExpoViewConfig(moduleName: string) {
return (globalThis as typeof globalThis & ExpoGlobalWithViewConfig).expo?.getViewConfig?.(
@@ -47,6 +48,10 @@
return cachedNativeTerminalSurfaceView;
}
+ if (terminalResolutionFailed) {
+ return null;
+ }
+
if (getExpoViewConfig(NATIVE_TERMINAL_MODULE_NAME) == null) {
return null;
}
@@ -56,6 +61,7 @@
NATIVE_TERMINAL_MODULE_NAME,
);
} catch (cause) {
+ terminalResolutionFailed = true;
console.error(
new NativeViewResolutionError({
nativeModuleName: NATIVE_TERMINAL_MODULE_NAME,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit fb0588b. Configure here.
ApprovabilityVerdict: Approved This PR adds structured error handling for native view resolution failures. The changes are self-contained, well-tested, and don't alter core functionality - they improve error observability and add a minor optimization to track failed resolution attempts. You can customize Macroscope's approvability policy. Learn more. |
Co-authored-by: codex <codex@users.noreply.github.com>
Dismissing prior approval to re-evaluate 4b5ceb1


What Changed
nullfallbackWhy
Both resolvers silently discarded
requireNativeViewfailures. The UI fallback is intentional, but the failure now remains diagnosable without changing availability behavior.Checklist
Validation
vp test apps/mobile/src/features/terminal/nativeTerminalModule.test.ts apps/mobile/src/features/diffs/nativeReviewDiffSurface.test.tsvp check(passes with 20 pre-existing warnings)vp run typecheckvp run lint:mobileNote
Low Risk
Observability and retry-avoidance only; public resolver return values and UI fallback paths are unchanged.
Overview
Native Expo view resolvers for review diff and terminal surfaces no longer swallow
requireNativeViewfailures. On error they log aNativeViewResolutionError(module name + original cause) viaconsole.error, set a one-shot failure flag so later calls skip re-invokingrequireNativeView, and still returnnullfor the existing JS fallback.Adds
nativeViewResolutionError.tsas a shared Effect Schema tagged error. Resolver tests now assert structured logging, preserved cause, and that a second resolve attempt does not callrequireNativeViewagain.Reviewed by Cursor Bugbot for commit 4b5ceb1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Structure native view resolution failures with
NativeViewResolutionErrorand memoized failure stateNativeViewResolutionErrortagged error class (viaeffect/Schema) carryingnativeModuleNameandcausefields.resolveNativeReviewDiffViewandresolveNativeTerminalSurfaceViewto catch failures, log aNativeViewResolutionErrortoconsole.error, and set a module-level flag so subsequent calls returnnullimmediately without retrying.requireNativeViewfailure, the resolver short-circuits all subsequent calls rather than re-attempting resolution.Macroscope summarized 4b5ceb1.