fix: stop discarding errors in debug - #375
Conversation
Sorry, something went wrong.
|
@coderabbitai review |
Sorry, something went wrong.
|
🧩 Analysis chain
Let me gather the PR details and search for the showErrorDisplay usage. 🏁 Script executed: #!/bin/bash # Get PR diff gh pr view 375 --repo NativeScript/ios --json files,body,title echo "---DIFF---" gh pr diff 375 --repo NativeScript/ios Length of output: 139107 🏁 Script executed: #!/bin/bash # Search for showErrorDisplay references rg -n "showErrorDisplay" --type-add 'ts:*.ts' --type-add 'mm:*.mm' -C5 # Also search for error display related code rg -n "ErrorDisplay\|showError\|TNSExceptionHandler\|handleException\|discardErrors\|debugMode\|isDebug" -C5 Length of output: 5130 @NathanWalker — I've reviewed the diff in detail. Here is the full analysis: Review Summary✅ Core fix is correctThe removal of jsErrorOccurred, the while(true) infinite run loop, all the return true ("lie to iOS") patterns, and the ShowErrorModal-then-return paths in OnUncaughtError / ReThrowToV8 is the right direction. Errors should propagate and crash the app in debug just as they do in release. ⚠️ showErrorDisplay: true is adversely affected for uncaught JS exceptionsThere are two separate pathways for the error display feature, and they are now in an inconsistent state: Path 1 — console.error with stack trace → still works ✅ Path 2 — Uncaught JS exceptions → now broken ❌ The result: if showErrorDisplay: true is set, users will no longer see the branded error overlay when an uncaught JS exception occurs — the app will just crash with only a log output. Suggested approach: rather than removing the ShowErrorModal call entirely from OnUncaughtError, consider calling it before the crash (not instead of it): // In OnUncaughtError, after logging:
if (RuntimeConfig.IsDebug && Runtime::showErrorDisplay()) {
ShowErrorModal(isolate, errorTitle, errorMessage, stackTrace);
// Small synchronous delay to allow modal to render before the crash
}
// Then let the crash proceed normally (don't return)
dispatch_async(dispatch_get_main_queue(), ^(void) {
`@throw` [NSException exceptionWithName:name reason:reasonStr userInfo:...];
});⚠️ Release mode crash path in OnUncaughtError also removedThe old OnUncaughtError contained (under if (!RuntimeConfig.IsDebug)) a dispatch_async that threw an NSException to terminate the app in release. That block is gone in this PR. The new code only logs. If the crash in release mode is now relying entirely on V8's own unhandled exception machinery propagating up the call stack (rather than the explicit NSException throw), that should be verified — especially for async/promise rejection scenarios where the V8 callback may return cleanly to its caller. Minor nits (non-blocking)The large volume of formatting-only changes (whitespace, line wrapping) makes the diff harder to review. Consider splitting those into a separate commit for clarity. ✅ Actions performedReview triggered.
|
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 1c898adb-3f4f-4720-82a8-cd4cb1c00581 📥 CommitsReviewing files that changed from the base of the PR and between dc46a79 and 93f6c82. 📒 Files selected for processing (2)
📝 Walkthrough WalkthroughThis PR removes the global jsErrorOccurred flag and refactors error handling throughout the NativeScript iOS runtime to propagate exceptions directly via V8 instead of silently continuing in debug mode. The changes span module loading, resolution, dynamic imports, worker threads, and app-level fatal error logging. ChangesError Propagation and Exception Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)NativeScript/runtime/ModuleInternal.mm (1)166-179: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
RunModule still treats some .mjs failures as success in debug.
LoadESModule() still has debug-only branches that return an empty handle on compile/link/evaluate failure, and this fast path returns true unconditionally. That means those .mjs errors still continue execution instead of failing the app.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternal.mm` around lines 166 - 179, The ES module fast-path in RunModule treats an empty module handle as success; update the IsESModule branch (where TryCatch tc and moduleNamespace = ModuleInternal::LoadESModule(isolate, path) are used) to check whether moduleNamespace is empty or tc.HasCaught() after the call, and if so log as appropriate and return false (or rethrow the V8 exception via ex.ReThrowToV8 / propagate the TryCatch) instead of unconditionally returning true; ensure LoadESModule failure paths that return an empty Local<Value> do not get treated as success.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@NativeScript/runtime/ModuleInternal.mm`: - Around line 916-920: The current branch uses isolate->ThrowException(reason) then constructs NativeScriptException(isolate, promiseTc, "Module evaluation promise rejected"), but promiseTc is not populated by ThrowException so the exception context is lost; change the throw to use the message-only constructor—throw NativeScriptException(isolate, "Module evaluation promise rejected")—so NativeScriptException and downstream helpers like GetSmartStackTrace/GetFullMessage receive a proper message-only construction instead of an empty TryCatch; update the throw site that follows the promiseTc/HasCaught logic around promise->Result() accordingly. In `@NativeScript/runtime/ModuleInternalCallbacks.mm`: - Around line 2421-2433: The code rejects and returns inside the first maybeModule.IsEmpty() check (both resolveTc.HasCaught() and else branches), making the later fallback that calls __nsHmrRequestModule() unreachable; fix by restructuring so that when maybeModule.IsEmpty() and no exception was thrown you do not immediately return but instead attempt the HMR retry: remove the early return in the "No exception thrown (debug path)" branch (the branch building msg with normalizedSpec and calling resolver->Reject) and instead call __nsHmrRequestModule(context, normalizedSpec, resolver) (or the existing retry path used later) so the dynamic import can be retried via __nsHmrRequestModule() before finally rejecting via resolver->Reject; ensure resolveTc.HasCaught() still rejects immediately with resolveTc.Exception() via resolver->Reject and return as before. In `@NativeScript/runtime/NativeScriptException.mm`: - Around line 157-162: The uncaught-exception branch only logs now and must still render the debug error overlay; before or immediately after detecting !isDiscarded in the handler in NativeScriptException.mm, invoke the existing error-display path (call OnUncaughtError(...) or directly call ShowErrorModal/ShowErrorDisplay with showErrorDisplay: true) using the same fullMessage/stackTrace so the debug modal is shown for uncaught JS exceptions, then allow the exception to continue propagating/being logged as currently done (references: isDiscarded, fullMessage, stackTrace, OnUncaughtError, ShowErrorModal, showErrorDisplay). --- Outside diff comments: In `@NativeScript/runtime/ModuleInternal.mm`: - Around line 166-179: The ES module fast-path in RunModule treats an empty module handle as success; update the IsESModule branch (where TryCatch tc and moduleNamespace = ModuleInternal::LoadESModule(isolate, path) are used) to check whether moduleNamespace is empty or tc.HasCaught() after the call, and if so log as appropriate and return false (or rethrow the V8 exception via ex.ReThrowToV8 / propagate the TryCatch) instead of unconditionally returning true; ensure LoadESModule failure paths that return an empty Local<Value> do not get treated as success.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2fc1168c-e6d8-456c-82bd-18887238b219
📥 CommitsReviewing files that changed from the base of the PR and between 90ac16c and f7133de.
📒 Files selected for processing (6)- NativeScript/NativeScript.mm
- NativeScript/runtime/ModuleInternal.mm
- NativeScript/runtime/ModuleInternalCallbacks.mm
- NativeScript/runtime/NativeScriptException.mm
- NativeScript/runtime/Runtime.mm
- NativeScript/runtime/Worker.mm
Sorry, something went wrong.
| if (maybeModule.IsEmpty()) { | ||
| if (resolveTc.HasCaught()) { | ||
| // Reject the promise with the thrown exception so callers don't hang | ||
| resolver->Reject(context, resolveTc.Exception()).FromMaybe(false); | ||
| return scope.Escape(resolver->GetPromise()); | ||
| } else { | ||
| // No exception thrown (debug path); reject with a helpful error | ||
| std::string msg = "Module resolution failed for dynamic import: "; | ||
| msg += normalizedSpec.empty() ? "<empty>" : normalizedSpec; | ||
| resolver->Reject(context, v8::Exception::Error(tns::ToV8String(isolate, msg.c_str()))) | ||
| .FromMaybe(false); | ||
| return scope.Escape(resolver->GetPromise()); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
The __nsHmrRequestModule() fallback is unreachable.
The first maybeModule.IsEmpty() block already rejects and returns in both branches, so the later maybeModule.IsEmpty() block never gets a chance to call __nsHmrRequestModule(). Dynamic imports that should retry through the HMR bridge will now fail immediately.
Also applies to: 2436-2452
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternalCallbacks.mm` around lines 2421 - 2433, The code rejects and returns inside the first maybeModule.IsEmpty() check (both resolveTc.HasCaught() and else branches), making the later fallback that calls __nsHmrRequestModule() unreachable; fix by restructuring so that when maybeModule.IsEmpty() and no exception was thrown you do not immediately return but instead attempt the HMR retry: remove the early return in the "No exception thrown (debug path)" branch (the branch building msg with normalizedSpec and calling resolver->Reject) and instead call __nsHmrRequestModule(context, normalizedSpec, resolver) (or the existing retry path used later) so the dynamic import can be retried via __nsHmrRequestModule() before finally rejecting via resolver->Reject; ensure resolveTc.HasCaught() still rejects immediately with resolveTc.Exception() via resolver->Reject and return as before.
Sorry, something went wrong.
| if (!isDiscarded) { | ||
| NSString* reasonStr = [NSString stringWithUTF8String:fullMessage.c_str()]; | ||
| if (reasonStr == nil) { | ||
| reasonStr = @"(invalid UTF-8 message from JS)"; | ||
| } | ||
|
|
||
| NSString* name = @"NativeScriptUncaughtJSException"; | ||
|
|
||
| // In debug mode, show error modal instead of crashing | ||
| if (RuntimeConfig.IsDebug) { | ||
| // Mark that a JavaScript error occurred | ||
| jsErrorOccurred = true; | ||
| Log(@"***** JavaScript exception occurred " | ||
| @"in debug mode *****\n"); | ||
| Log(@"%s", fullMessage.c_str()); | ||
| Log(@"***** Fatal JavaScript exception *****\n"); | ||
| Log(@"%s", fullMessage.c_str()); | ||
| if (!stackTrace.empty()) { | ||
| Log(@"%s", stackTrace.c_str()); | ||
| // Log(@"🎨 CALLING ShowErrorModal for OnUncaughtError - should display branded modal"); | ||
|
|
||
| // Show the error modal with same message as terminal | ||
| std::string errorTitle = "Uncaught JavaScript Exception"; | ||
|
|
||
| // Extract just the error type/message (first line) for cleaner display | ||
| std::string errorMessage = "JavaScript error occurred"; | ||
| if (reasonStr) { | ||
| std::string fullMsg = [reasonStr UTF8String]; | ||
| size_t firstNewline = fullMsg.find('\n'); | ||
| if (firstNewline != std::string::npos) { | ||
| errorMessage = fullMsg.substr(0, firstNewline); | ||
| } else { | ||
| errorMessage = fullMsg; | ||
| } | ||
| } | ||
|
|
||
| Log(@"***** End stack trace - Fix error to continue *****\n"); | ||
|
|
||
| ShowErrorModal(isolate, errorTitle, errorMessage, stackTrace); | ||
|
|
||
| // Don't crash in debug mode - just return | ||
| return; | ||
| } | ||
|
|
||
| // In release mode, crash as before - BUT NEVER IN DEBUG MODE | ||
| if (!RuntimeConfig.IsDebug) { | ||
| // we throw the exception on main thread so all meta-data is captured | ||
| dispatch_async(dispatch_get_main_queue(), ^(void) { | ||
| NSException* objcException = | ||
| [NSException exceptionWithName:name | ||
| reason:reasonStr | ||
| userInfo:@{@"sender" : @"onUncaughtError"}]; | ||
|
|
||
| Log(@"***** Fatal JavaScript exception - application has been terminated. *****\n"); | ||
| Log(@"%@", objcException); | ||
| @throw objcException; | ||
| }); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Keep showErrorDisplay active for uncaught exceptions.
This branch now only logs. Since OnUncaughtError no longer invokes ShowErrorModal(), showErrorDisplay: true stops showing the debug overlay for uncaught JS exceptions on the exact path this PR is changing. If debug should still crash, render the modal here first and then let the exception continue propagating.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/NativeScriptException.mm` around lines 157 - 162, The uncaught-exception branch only logs now and must still render the debug error overlay; before or immediately after detecting !isDiscarded in the handler in NativeScriptException.mm, invoke the existing error-display path (call OnUncaughtError(...) or directly call ShowErrorModal/ShowErrorDisplay with showErrorDisplay: true) using the same fullMessage/stackTrace so the debug modal is shown for uncaught JS exceptions, then allow the exception to continue propagating/being logged as currently done (references: isDiscarded, fullMessage, stackTrace, OnUncaughtError, ShowErrorModal, showErrorDisplay).
Sorry, something went wrong.
…xception on promise rejection
|
@copilot resolve the merge conflicts from Worker.mm |
Sorry, something went wrong.
Resolved the Worker.mm merge conflict in commit 93f6c82. |
Sorry, something went wrong.
turns out I missed a few errors in my other PR.
Yes the app should crash in debug. Right now it crashes in release mode but continues fine in debug (?????????????)
Summary by CodeRabbit
Bug Fixes
Refactor