Test/network by gurpreet319 · Pull Request #149 · rdkcentral/rdkNativeScript

gurpreet319 · 2026-09-23T10:11:18Z

No description provided.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical threading, connection-lifetime, capture, and API-compatibility issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 4

Open (8)

Off-thread connection lookup can race and use freed connections · New
Download callback accesses connections from the wrong thread · New
Ethan logging incorrectly enables network metric collection and dumping · New
Remote inspector capture is blocked unless environment variables are set · New
Environment-controlled metric dumping is skipped without inspector capture · New
JSRuntimeDownloadMetrics breaks its existing query contract · New
Capture remains enabled after the inspector disconnects · New
HAR timing heuristic misinterprets explicitly millisecond fields · New

What changed in this PR

Adds live network monitoring and HAR export to the NativeScript inspector, with native request-metric streaming.

Changes:

Adds Network panel rendering and HAR export.
Streams metrics through the inspector WebSocket.
Adds runtime capture state, serialization, and delivery APIs.

File	Changes and final findings
`utils/nativejsinspector.html`	Adds Network UI and HAR export. Moderate: `harPickTiming` misinterprets explicit `*Ms` values (3 votes); rendering 2,000 rows per request causes O(n²) DOM work (1 vote); disconnect does not disable capture (2 votes); Transfer size uses outgoing headers (1 vote); HAR timestamps use export time instead of request time (1 vote).
`src/jsc/JavaScriptUtils.cpp`	Adds metric capture and toggle behavior. Critical: inspector capture remains gated by environment variables (3 votes); `ETHAN_LOGGING_PIPE` changes collection and logging behavior, exposing request metadata (1 vote). Moderate: `JSRuntimeDownloadMetrics` breaks its existing query contract (3 votes).
`src/jsc/JavaScriptContext.cpp`	Adds metric serialization and forwarding. Critical: callbacks access connections and send WebSocket data off the owning thread, risking races and lifetime errors (1 vote). Moderate: legacy metric dumping is skipped without inspector capture (3 votes); streaming unconditionally appends metrics to the log, causing I/O, growth, and metadata exposure (1 vote).
`src/InspectorHTTPServer.cpp`	Adds network event delivery. Critical: `onMetricsData` accesses connections off the inspector thread and can race with unref/send operations (2 votes).
`include/jsc/JavaScriptContext.h`	Adds capture-state declarations. Moderate: declares `std::mutex` without directly including `<mutex>` (1 vote).
`include/InspectorHTTPServer.h`	Declares the network event delivery API.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    SoupWebsocketConnection* targetConnection = nullptr;
+    for (const auto& pair : m_connections) {
+        if (pair.second == globalContext) {
+            targetConnection = pair.first;
+            break;


+    #ifdef REMOTE_INSPECTOR_ENABLE
+    InspectorHTTPServer::singleton().sendNetworkMetric(mContext, params.str().c_str());
+    #endif


+    std::lock_guard<std::mutex> lock(mNetworkMetricsMutex);
+    if (!mNetworkCaptureEnabled) {
+        // Recording is off (inspector Network tab inactive) — drop without storing.
+        delete net;
+        return;


+  bool enabled = (numArgs >= 1) ? args[0].toBool() : false;
+  jscContext->setNetworkCaptureEnabled(enabled);
+  if (result) *result = true;


+                // Keep network capture enabled for the whole inspector session,
+                // similar to console logging.
+                setNetworkCapture(true);


+        function harPickTiming(flat, keys) {
+            for (const key of keys) {
+                if (flat[key] !== undefined) return harToMillis(flat[key]);
+            }
+            return -1;
+        }


Copilot

Copilot review overview

🟡 Changes recommended

Critical and moderate unresolved issues affect compilation, data privacy, API compatibility, and thread safety.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6 · 4

Open (10)

Missing <mutex> include in public header · New
Inspector capture bypasses opt-in for sensitive file logging · New
Remote inspector capture is blocked unless environment variables are set
Ethan logging incorrectly enables network metric collection and dumping
Download callback accesses connections from the wrong thread
Off-thread connection lookup can race and use freed connections
HAR timing heuristic misinterprets explicitly millisecond fields
Capture remains enabled after the inspector disconnects
JSRuntimeDownloadMetrics breaks its existing query contract
Environment-controlled metric dumping is skipped without inspector capture

Previously missed (2)

In code that hasn't changed since last review

Environment-enabled network diagnostics are incorrectly discarded

src/jsc/JavaScriptContext.cpp:671

When NATIVEJS_DUMP_NETWORKMETRIC or ETHAN_LOGGING_PIPE is set, the request producer still creates metrics, but mNetworkCaptureEnabled starts false unless the inspector sends the new setter. This branch deletes those metrics, so the existing environment-driven diagnostics no longer collect anything in headless or non-inspector runs; preserve the environment opt-in separately from the inspector flag.

HAR content length reports request size as response size

utils/nativejsinspector.html:459

entry.headers is serialized from rtHttpRequest::headers() (the outgoing request headers), so a Content-Length found here is the upload/request-body length, not the response transfer size shown by this column. POST/PUT rows will report the wrong direction; use a response size metric or response headers, or label this as request size.

+    InspectorHTTPServer::singleton().sendNetworkMetric(mContext, params.c_str());
+    #endif

    dumpNetworkMetricData(net, this->getUrl());


Copilot

Copilot review overview

🟡 Changes recommended

The review identified a compilation blocker, security and privacy risks, concurrency issues, and functional regressions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 8 · 4

Open (12)

Undefined sendText call breaks compilation · New
Unauthenticated inspector exposes sensitive request headers · New
Inspector capture bypasses opt-in for sensitive file logging
Missing <mutex> include in public header
Remote inspector capture is blocked unless environment variables are set
Ethan logging incorrectly enables network metric collection and dumping
Download callback accesses connections from the wrong thread
Off-thread connection lookup can race and use freed connections
HAR timing heuristic misinterprets explicitly millisecond fields
Capture remains enabled after the inspector disconnects
JSRuntimeDownloadMetrics breaks its existing query contract
Environment-controlled metric dumping is skipped without inspector capture

Previously missed (1)

In code that hasn't changed since last review

HAR entries lose original request timestamps

utils/nativejsinspector.html:652

Every exported entry is assigned the time of the HAR export, not when that request occurred. A HAR containing multiple completed requests therefore loses request chronology and makes all requests appear to start simultaneously; record the capture timestamp with each streamed metric and use it here.

-        if (soup_websocket_connection_get_state(conn) == SOUP_WEBSOCKET_STATE_OPEN) {
-            soup_websocket_connection_send_text(conn, evtStr.c_str());
-        }
+        sendText(pair.first, evtStr);


+        if (j) {
+            params << ",";
+        }
+        params << "\"" << jsonEscape(net.headers[j].cString()) << "\"";


Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical connection-lifetime and public-binding compatibility issues, along with capture, privacy, and HAR correctness issues, block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 10 · 4

Open (14)

The lookup is protected only until this function returns the raw pointer. onWebSocketClosed can… · New
This changes the existing JSRuntimeDownloadMetrics() binding from a zero-argument getter… · New
Unauthenticated inspector exposes sensitive request headers
Undefined sendText call breaks compilation
Inspector capture bypasses opt-in for sensitive file logging
Missing <mutex> include in public header
Remote inspector capture is blocked unless environment variables are set
Ethan logging incorrectly enables network metric collection and dumping
Download callback accesses connections from the wrong thread
Off-thread connection lookup can race and use freed connections
HAR timing heuristic misinterprets explicitly millisecond fields
Capture remains enabled after the inspector disconnects
JSRuntimeDownloadMetrics breaks its existing query contract
Environment-controlled metric dumping is skipped without inspector capture

Previously missed (1)

In code that hasn't changed since last review

NetworkMetrics.headers is populated from rtHttpRequestEx::headers() (the outgoing request…

utils/nativejsinspector.html:459

NetworkMetrics.headers is populated from rtHttpRequestEx::headers() (the outgoing request headers), not response headers. Consequently a request body's Content-Length can be shown as the response transfer size, even though it is unrelated to downloaded bytes. Only use a response byte metric/header here, or omit this shortcut rather than reporting a misleading size.

+SoupWebsocketConnection* InspectorHTTPServer::findConnectionForContext(JSGlobalContextRef context)
+{
+    std::lock_guard<std::mutex> connectionsLock(m_connectionsMutex);
+    for (const auto& pair : m_connections) {
+        if (pair.second == context) {


gurpreet319 added 3 commits September 23, 2026 07:18

test network debugger

02168f4

test

44e61d9

test

04cb959

Copilot AI lite review requested due to automatic review settings September 23, 2026 10:11

gurpreet319 requested a review from a team as a code owner September 23, 2026 10:11

Copilot started reviewing on behalf of gurpreet319 September 23, 2026 10:12 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

test

45be903

Copilot AI review requested due to automatic review settings September 23, 2026 10:37

Copilot started reviewing on behalf of gurpreet319 September 23, 2026 10:38 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread include/jsc/JavaScriptContext.h

Comment thread src/jsc/JavaScriptContext.cpp

InspectorHTTPServer::singleton().sendNetworkMetric(mContext, params.c_str());

#endif

dumpNetworkMetricData(net, this->getUrl());

test

d78c704

Copilot AI review requested due to automatic review settings September 23, 2026 11:10

Copilot started reviewing on behalf of gurpreet319 September 23, 2026 11:10 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

test

7c9d9a9

Copilot AI review requested due to automatic review settings September 23, 2026 11:35

Copilot started reviewing on behalf of gurpreet319 September 23, 2026 11:36 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Navigation Menu

Test/network - #149

Test/network#149
gurpreet319 wants to merge 6 commits into
developfrom
test/network

gurpreet319 commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gurpreet319 commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants