fix: stop discarding errors in debug#375
Conversation
|
@coderabbitai review |
🧩 Analysis chainLet me gather the PR details and search for the 🏁 Script executed: Length of output: 139107 🏁 Script executed: Length of output: 5130
Review Summary✅ Core fix is correctThe removal of
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes the global 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
RunModulestill treats some.mjsfailures as success in debug.
LoadESModule()still has debug-only branches that return an empty handle on compile/link/evaluate failure, and this fast path returnstrueunconditionally. That means those.mjserrors still continue execution instead of failing the app.🤖 Prompt for AI Agents
Verify 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.
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix (Beta)
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
📒 Files selected for processing (6)
NativeScript/NativeScript.mmNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/ModuleInternalCallbacks.mmNativeScript/runtime/NativeScriptException.mmNativeScript/runtime/Runtime.mmNativeScript/runtime/Worker.mm
| 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.
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 Agents
Verify 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.
| 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.
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 Agents
Verify 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).
…xception on promise rejection
2 participants
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