{{ message }}
feat: reloadApplication for JS bundle restart without restarting app process#1963
Open
NathanWalker wants to merge 5 commits into
Open
feat: reloadApplication for JS bundle restart without restarting app process#1963NathanWalker wants to merge 5 commits into
NathanWalker wants to merge 5 commits 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.
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
|
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: 4
🤖 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 `@test-app/app/src/main/java/com/tns/NativeScriptRuntime.java`:
- Around line 11-12: The NativeScriptRuntime.reloadApplication(String baseDir)
overload currently drops its baseDir argument, so callers cannot reload from a
chosen bundle location. Update this method to either pass baseDir through to a
new RuntimeHelper.reloadApplication(String baseDir) implementation that rebuilds
the runtime config from that directory, or remove the overload if it is not
meant to be supported; keep the API behavior aligned with the method signature
and related RuntimeHelper reload logic.
In `@test-app/app/src/main/java/com/tns/RuntimeHelper.java`:
- Around line 340-349: `registerTimezoneChangedListener` is unregistering
`timezoneChangedReceiver` with whatever `context` is passed in, but that can
differ between the initial call and `reloadApplication`, causing the old
receiver to remain registered. Update the listener bookkeeping so the same
Context instance used for registration is also used for `unregisterReceiver`
(for example by storing the registering Context alongside
`timezoneChangedReceiver` or consistently using `applicationContext` in
`registerTimezoneChangedListener` and its callers). Keep the existing
`IllegalArgumentException` handling only for truly stale registrations, and
ensure the old receiver is fully removed before assigning and registering the
new one.
In `@test-app/runtime/src/main/cpp/com_tns_Runtime.cpp`:
- Around line 358-362: The null-runtime checks in the affected callbacks do not
stop execution, so `runtime->GetIsolate()` can still be called on a null pointer
and crash. In each callback that resolves a `runtime` and then uses it
(`com_tns_Runtime` helpers around the reported branches), add an immediate
return inside the `if (runtime == nullptr)` path before any dereference, and
apply the same early-exit pattern consistently to all listed null-runtime
branches.
In `@test-app/runtime/src/main/java/com/tns/Runtime.java`:
- Around line 478-500: The termination flow in Runtime.destroyMainRuntime() and
terminateWorkers() is racing because the TerminateThread message is posted
asynchronously and workerIdToHandler is cleared immediately afterward. Add a
blocking handshake so terminateWorkers() waits until each worker has actually
processed the termination signal before destroyMainRuntime() continues with
runtimeCache.remove(), pendingWorkerMessages.clear(), currentRuntime.remove(),
and TerminateRuntimeCallback. Use the existing Runtime and workerHandler
messaging path to coordinate completion, and only clear workerIdToHandler after
all workers confirm shutdown.
🪄 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: e52793de-9a24-4365-9c36-5c412d5895b3
📒 Files selected for processing (4)
test-app/app/src/main/java/com/tns/NativeScriptRuntime.javatest-app/app/src/main/java/com/tns/RuntimeHelper.javatest-app/runtime/src/main/cpp/com_tns_Runtime.cpptest-app/runtime/src/main/java/com/tns/Runtime.java
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
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/ios#384
Summary by CodeRabbit