feat: reloadApplication for JS bundle restart without restarting app process#384
Open
NathanWalker wants to merge 1 commit into
Open
feat: reloadApplication for JS bundle restart without restarting app process#384NathanWalker wants to merge 1 commit into
NathanWalker wants to merge 1 commit into
Conversation
…process Helpful for programmatic reset of JS isolate for clean restart of JS application as well as OTA (over-the-air) updates without restarting the entire app process.
📝 WalkthroughWalkthroughThe PR adds a reload path exposed from JavaScript to Objective-C. It stores the active runtime/configuration, registers a JS hook for ChangesRuntime reload flow
Sequence Diagram(s)sequenceDiagram
participant JS as global.NativeScriptRuntime.reloadApplication(baseDir?)
participant InvokeHook as tns::InvokeReloadApplicationHook
participant ObjC as +[NativeScriptRuntime reloadApplication:]
participant MainQueue as dispatch_async(main queue)
JS->>InvokeHook: pass optional baseDir
InvokeHook->>ObjC: forward NSString* or nil
ObjC->>MainQueue: schedule restart with copied Config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/NativeScript.mm`:
- Around line 182-184: The reload path in NativeScript’s restart flow still
carries over the process-global jsErrorOccurred flag, which can make the next
bundle start in the old debug error loop. Update the restart sequence around
currentNativeScript restartWithConfig: and runMainApplication to clear or
reinitialize jsErrorOccurred before launching the new isolate, so a fresh reload
starts from a clean JS error state.
In `@NativeScript/runtime/Runtime.mm`:
- Around line 577-588: Serialize access to the process-global
reloadApplicationHook_ used by SetReloadApplicationHook and
InvokeReloadApplicationHook: protect both reads and writes with synchronization
so initialization and concurrent JavaScript-thread calls cannot race. Update
InvokeReloadApplicationHook to copy the current hook under the lock, then
release the lock before invoking it so user code runs outside the critical
section and avoids deadlocks or lock contention.
🪄 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: 7e5e1d22-c25d-42ab-b77f-3fcaea2ea84b
📒 Files selected for processing (4)
NativeScript/NativeScript.hNativeScript/NativeScript.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mm
Comment on lines
+182
to
+184
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [currentNativeScript restartWithConfig:config]; | ||
| [currentNativeScript runMainApplication]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset stale JS error state before starting the new bundle.
jsErrorOccurred is process-global and survives reloads. After a previous debug error, the new runMainApplication can enter the debug error loop even if the reload was meant to provide a clean isolate restart.
Proposed fix
dispatch_async(dispatch_get_main_queue(), ^{
+ tns::jsErrorOccurred = false;
[currentNativeScript restartWithConfig:config];
[currentNativeScript runMainApplication];
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [currentNativeScript restartWithConfig:config]; | |
| [currentNativeScript runMainApplication]; | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| tns::jsErrorOccurred = false; | |
| [currentNativeScript restartWithConfig:config]; | |
| [currentNativeScript runMainApplication]; |
🤖 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/NativeScript.mm` around lines 182 - 184, The reload path in
NativeScript’s restart flow still carries over the process-global
jsErrorOccurred flag, which can make the next bundle start in the old debug
error loop. Update the restart sequence around currentNativeScript
restartWithConfig: and runMainApplication to clear or reinitialize
jsErrorOccurred before launching the new isolate, so a fresh reload starts from
a clean JS error state.
Comment on lines
+577
to
+588
| static ReloadApplicationHook reloadApplicationHook_; | ||
|
|
||
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | ||
| reloadApplicationHook_ = std::move(hook); | ||
| } | ||
|
|
||
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | ||
| if (!reloadApplicationHook_) { | ||
| return false; | ||
| } | ||
| return reloadApplicationHook_(baseDir); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all reload hook access sites and whether they can be reached from runtime initialization / JS callbacks.
rg -n -C4 '\b(SetReloadApplicationHook|InvokeReloadApplicationHook|DefineNativeScriptRuntime)\b' NativeScriptRepository: NativeScript/ios
Length of output: 5712
🏁 Script executed:
#!/bin/bash
# Check if <mutex> is included in Runtime.mm
head -50 NativeScript/runtime/Runtime.mm | rg "include.*mutex"Repository: NativeScript/ios
Length of output: 171
Serialize access to the global reload hook.
reloadApplicationHook_ is a process-global std::function modified by SetReloadApplicationHook during initialization while InvokeReloadApplicationHook may be called concurrently from JavaScript execution threads. Read and write access must be synchronized, and the hook invocation must occur outside the lock to prevent deadlocks or holding locks during user code execution.
Proposed fix
static ReloadApplicationHook reloadApplicationHook_;
+static std::mutex reloadApplicationHookMutex_;
void SetReloadApplicationHook(ReloadApplicationHook hook) {
+ std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_);
reloadApplicationHook_ = std::move(hook);
}
bool InvokeReloadApplicationHook(const std::string& baseDir) {
- if (!reloadApplicationHook_) {
+ ReloadApplicationHook hook;
+ {
+ std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_);
+ hook = reloadApplicationHook_;
+ }
+ if (!hook) {
return false;
}
- return reloadApplicationHook_(baseDir);
+ return hook(baseDir);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static ReloadApplicationHook reloadApplicationHook_; | |
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | |
| reloadApplicationHook_ = std::move(hook); | |
| } | |
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | |
| if (!reloadApplicationHook_) { | |
| return false; | |
| } | |
| return reloadApplicationHook_(baseDir); | |
| } | |
| static ReloadApplicationHook reloadApplicationHook_; | |
| static std::mutex reloadApplicationHookMutex_; | |
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | |
| std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_); | |
| reloadApplicationHook_ = std::move(hook); | |
| } | |
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | |
| ReloadApplicationHook hook; | |
| { | |
| std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_); | |
| hook = reloadApplicationHook_; | |
| } | |
| if (!hook) { | |
| return false; | |
| } | |
| return hook(baseDir); | |
| } |
🤖 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/Runtime.mm` around lines 577 - 588, Serialize access to
the process-global reloadApplicationHook_ used by SetReloadApplicationHook and
InvokeReloadApplicationHook: protect both reads and writes with synchronization
so initialization and concurrent JavaScript-thread calls cannot race. Update
InvokeReloadApplicationHook to copy the current hook under the lock, then
release the lock before invoking it so user code runs outside the critical
section and avoids deadlocks or lock contention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Helpful for programmatic reset of JS isolate for clean restart of JS application as well as OTA (over-the-air) updates without restarting the entire app process.
NativeScript/NativeScript#11261
Summary by CodeRabbit
New Features
NativeScriptRuntimeAPI for reloading the app at runtime.reloadApplication(baseDir?)method, with optional base directory support.Bug Fixes