GeezerVison is showing github.com — feat(inspector): serve source maps to DevTools via Network.loadNetwor… by edusperoni · Pull Request #1969 · NativeScript/android
Conversation
edusperoni
commented
Jun 11, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
…kResource
Chrome DevTools no longer fetches external source maps itself when debugging remote targets: it issues Network.loadNetworkResource to the target and reads the result back through IO.read/IO.close. None of these embedder-side CDP domains are implemented by V8's inspector, so external source maps failed and apps had to fall back to bloated inline-source-map builds.
- Handle Network.loadNetworkResource natively: resolve the URL back to a file on disk and reply with a stream handle (success:false + net::ERR_FILE_NOT_FOUND when missing).
- Implement IO.read (1MB base64 chunks; eof only on a final empty read, since the frontend discards data accompanying eof) and IO.close.
- Reply with a JSON-RPC error for unsupported schemes (e.g. https) so DevTools keeps its existing fallback of fetching from the host.
- Rewrite sourceMapURL in outgoing Debugger.scriptParsed / Debugger.scriptFailedToParse events from relative/file:// URLs to a custom nsruntime:// scheme. DevTools hard-excludes file:, data: and devtools: URLs from loading through the target, so without the rewrite it would never send Network.loadNetworkResource and instead try (and fail) to read device files from the host machine. data: and http(s) URLs are left untouched, keeping inline source maps working.
- Allow opting out via nativescript.config.ts: android.disableSourceMapURLRewrite (or the same key at the top level).
- Serve these messages on the websocket read thread (new native handleMessageOnSocketThread), since the main-thread queue is unavailable exactly when DevTools needs source maps: the pause loop bypasses dispatchMessage and a busy isolate never drains the queue. The handler is V8-free and returns the response for Java to send on the receiving socket.
- Make Debugger.pause interrupt busy JS via Isolate::RequestInterrupt, skipped while already paused in the nested loop to avoid a spurious re-pause after resume.
- Vendor nlohmann/json v3.12.0 (third_party/json.hpp, header-only) for CDP message handling outside V8.
Ports NativeScript/ios#385 and NativeScript/ios#378 to Android.
Refs: nodejs/node#58077
Summary by CodeRabbit
- New Features
- Faster debugger messaging with immediate responses for some inspector requests.
- Improved DevTools integration: rewritten source-map URLs, on-device resource streaming (read/close), and network/file resource access via the debugger.
- Bug Fixes
- Cleans up streamed resources on disconnect and improves pause behavior to avoid nested pause issues.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…kResource
Chrome DevTools no longer fetches external source maps itself when
debugging remote targets: it issues Network.loadNetworkResource to the
target and reads the result back through IO.read/IO.close. None of these
embedder-side CDP domains are implemented by V8's inspector, so external
source maps failed and apps had to fall back to bloated
inline-source-map builds.
- Handle Network.loadNetworkResource natively: resolve the URL back to a
file on disk and reply with a stream handle (success:false +
net::ERR_FILE_NOT_FOUND when missing).
- Implement IO.read (1MB base64 chunks; eof only on a final empty read,
since the frontend discards data accompanying eof) and IO.close.
- Reply with a JSON-RPC error for unsupported schemes (e.g. https) so
DevTools keeps its existing fallback of fetching from the host.
- Rewrite sourceMapURL in outgoing Debugger.scriptParsed /
Debugger.scriptFailedToParse events from relative/file:// URLs to a
custom nsruntime:// scheme. DevTools hard-excludes file:, data: and
devtools: URLs from loading through the target, so without the rewrite
it would never send Network.loadNetworkResource and instead try (and
fail) to read device files from the host machine. data: and http(s)
URLs are left untouched, keeping inline source maps working.
- Allow opting out via nativescript.config.ts:
android.disableSourceMapURLRewrite (or the same key at the top level).
- Serve these messages on the websocket read thread (new native
handleMessageOnSocketThread), since the main-thread queue is
unavailable exactly when DevTools needs source maps: the pause loop
bypasses dispatchMessage and a busy isolate never drains the queue.
The handler is V8-free and returns the response for Java to send on
the receiving socket.
- Make Debugger.pause interrupt busy JS via Isolate::RequestInterrupt,
skipped while already paused in the nested loop to avoid a spurious
re-pause after resume.
- Vendor nlohmann/json v3.12.0 (third_party/json.hpp, header-only) for
CDP message handling outside V8.
Ports NativeScript/ios#385 and NativeScript/ios#378 to Android.
Refs: nodejs/node#58077
coderabbitai
Bot
commented
Jun 11, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
📝 Walkthrough
Walkthrough
This PR extends the V8 inspector with a websocket-thread fast path that parses DevTools protocol messages on the socket thread (Network.loadNetworkResource, IO.read/close, Debugger.pause fast-path), bridges Java → JNI → C++, streams resources via in-memory ResourceStreams, rewrites source-map URLs to nsruntime://, and clears streams on disconnect.
Changes
Socket-Thread Fast Path and Resource Streaming
| Layer / File(s) |
Summary |
Java socket-thread fast path and JNI bridge
test-app/app/src/main/java/com/tns/AndroidJsV8Inspector.java, test-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpp |
Java declares native handleMessageOnSocketThread(String message) and calls it in onMessage before enqueueing; JNI entry point safely forwards the message to C++ and returns JSON response or null for queued dispatch. |
C++ API declarations and infrastructure headers
test-app/runtime/src/main/cpp/JsV8InspectorClient.h, test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp |
Header adds <atomic>, <map>, <mutex> and declares handleMessageOnSocketThread, ResourceStream struct, and DevTools handler methods (HandleLoadNetworkResource, HandleIORead, HandleIOClose); implementation adds small includes and a JSON alias. |
Source map rewriting and socket-thread message dispatch
test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp |
Implements anonymous-namespace utilities for JSON/base64/source-map rewriting; handleMessageOnSocketThread parses messages and serves Network/IO domains with base64 chunking and protocol responses; manages nested pause state atomically, clears resource streams on disconnect, and rewrites outgoing notifications before sending to the Android bridge. |
Sequence Diagram(s)
sequenceDiagram
participant JsV8InspectorWebSocket
participant handleMessageOnSocketThread
participant MessageParser
participant NetworkHandler as Network.loadNetworkResource
participant IOHandler as IO.read/IO.close
participant DebuggerHandler as Debugger.pause
JsV8InspectorWebSocket->>handleMessageOnSocketThread: message
handleMessageOnSocketThread->>MessageParser: parse protocol message
alt Network domain
MessageParser->>NetworkHandler: load resource URL
NetworkHandler->>handleMessageOnSocketThread: file content + base64 chunks
else IO domain
MessageParser->>IOHandler: read handle or close handle
IOHandler->>handleMessageOnSocketThread: stream chunk or close response
else Debugger.pause
MessageParser->>DebuggerHandler: RequestInterrupt if not in nested loop
DebuggerHandler->>handleMessageOnSocketThread: null (queued for dispatch)
else Other
MessageParser->>handleMessageOnSocketThread: null (queued for dispatch)
end
handleMessageOnSocketThread->>JsV8InspectorWebSocket: response string or null
Loading
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- NativeScript/android#1967: Adds similar websocket “fast path” plumbing (JNI → JsV8InspectorClient) and related socket-thread inspector handling.
Poem
🐰 A nimble rabbit on the thread,
Hops bytes and source maps overhead,
Streams and pauses neatly queued,
Fast paths dance where data's viewed,
JNI bridges send the tune—debugging sped.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name |
Status |
Explanation |
Resolution |
| Docstring Coverage |
⚠️ Warning |
Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. |
Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name |
Status |
Explanation |
| Description Check |
✅ Passed |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
✅ Passed |
The PR title accurately summarizes the main objective: serving source maps to DevTools via Network.loadNetworkResource, which is the core feature added across all modified files. |
| Linked Issues check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
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.
❤️ Share
Comment @coderabbitai help to get the list of available commands and usage tips.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![coderabbitai[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F347564%3Fs%3D60%26v%3D4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 511-529: The code stores full file content in resourceStreams_
(handle created using lastStreamId_) but never removes those entries if the
DevTools session/socket drops, causing memory leaks; fix by tracking resource
stream handles per session (e.g., add a map from sessionId to vector of handles
and push the created handle in the block that sets resourceStreams_) and then
remove/erase those handles from resourceStreams_ during session teardown (the
same place that handles IO.close logic or session cleanup is performed, e.g., in
the method that closes the session or in IO.close), ensuring lastStreamId_ usage
remains unchanged; reference resourceStreams_, lastStreamId_, the handle
creation block, IO.close and FinishResponse to locate where to add the
per-session handle tracking and where to purge them on teardown.
🪄 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: 7221c004-286f-47a0-8160-cea57404d3d4
📥 Commits
Reviewing files that changed from the base of the PR and between bfd7650 and f45830a.
📒 Files selected for processing (5)
test-app/app/src/main/java/com/tns/AndroidJsV8Inspector.javatest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/JsV8InspectorClient.htest-app/runtime/src/main/cpp/com_tns_AndroidJsV8Inspector.cpptest-app/runtime/src/main/cpp/third_party/json.hpp
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (1)
test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp (1)
483-497: 💤 Low value
Consider adding defensive empty check in addVariants.
Accessing p[0] when p is empty is undefined behavior in C++. While the current call sites guarantee non-empty inputs (line 475 validates path, and percent_decode of a non-empty string shouldn't return empty), a defensive check would make the code more robust against future changes.
🛡️ Optional defensive fix
auto addVariants = [&addCandidate](const std::string& p) {
+ if (p.empty()) return;
addCandidate(p);
if (p[0] != '/') {
addCandidate(Constants::APP_ROOT_FOLDER_PATH + p);
}
🤖 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 `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp` around lines 483 -
497, The lambda addVariants reads p[0] without guarding against an empty string;
add a defensive check at the top of the lambda (in JsV8InspectorClient.cpp
inside the addVariants lambda) to return early if p.empty() so addCandidate and
the subsequent path manipulations (Constants::APP_ROOT_FOLDER_PATH concatenation
and kDataData/kDataUser0 rfind/substr logic) never access p[0] or call substr on
an empty input.
🤖 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.
Nitpick comments:
In `@test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp`:
- Around line 483-497: The lambda addVariants reads p[0] without guarding
against an empty string; add a defensive check at the top of the lambda (in
JsV8InspectorClient.cpp inside the addVariants lambda) to return early if
p.empty() so addCandidate and the subsequent path manipulations
(Constants::APP_ROOT_FOLDER_PATH concatenation and kDataData/kDataUser0
rfind/substr logic) never access p[0] or call substr on an empty input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 560fa69e-e164-4711-9bfe-d41ef918f155
📥 Commits
Reviewing files that changed from the base of the PR and between f45830a and 7a6e3d9.
📒 Files selected for processing (1)
test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
rigor789
rigor789 approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|