Adds out of band control via adb to simplify wearing headset on neck scenario#312
Draft
yanziz-nvidia wants to merge 1 commit intomainfrom
Draft
Adds out of band control via adb to simplify wearing headset on neck scenario#312yanziz-nvidia wants to merge 1 commit intomainfrom
yanziz-nvidia wants to merge 1 commit intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a bidirectional teleop control system: a WebSocket hub server, operator dashboard UI/assets, a headset-side control channel client, WSS proxy/HTTP integration, docs, tests, and App/UI wiring to route session commands, config updates, and periodic metrics between dashboard and headset. Changes
Sequence Diagram(s)sequenceDiagram
participant Dashboard as Operator Dashboard
participant Hub as TeleopControlHub
participant Headset as WebXR Headset
participant App as WebXR App
Dashboard->>Hub: WS register { role: "dashboard", token? }
Headset->>Hub: WS register { role: "headset", token?, deviceLabel? }
Hub->>Headset: hello { clientId, configVersion, config }
Hub->>Dashboard: hello { clientId }
Dashboard->>Hub: setConfig { config, targetClientId? }
Hub->>Hub: Merge config, increment configVersion
Hub->>Headset: config { config, configVersion }
Dashboard->>Hub: command { action: "connect"/"disconnect", targetClientId? }
Hub->>Headset: sessionCommand { action, requestId? }
Headset->>App: onSessionCommand -> doConnect()/doDisconnect()
Headset->>Hub: clientMetrics { cadence, metrics }
Hub->>Hub: Store metrics by cadence
Dashboard->>Hub: GET /api/teleop/v1/state (poll)
Hub->>Dashboard: snapshot JSON { configVersion, headsets[{metrics}], ... }
sequenceDiagram
participant WSS as WSS Proxy
participant Browser as Operator Browser
participant Hub as TeleopControlHub
Browser->>WSS: GET /teleop -> index.html
Browser->>WSS: GET /teleop/operator.js
Browser->>WSS: WS /teleop/v1/ws
WSS->>Hub: upgrade/forward WS connection
Browser->>WSS: GET /api/teleop/v1/state
WSS->>Hub: hub.get_snapshot()
Hub->>WSS: snapshot JSON
WSS->>Browser: 200 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
from
38c28cd to
6ec535a
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
from
6ec535a to
22ca5c8
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
3 times, most recently
from
4be6331 to
5cf801d
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
from
5cf801d to
d81f3f3
Compare
yanziz-nvidia
requested review from
gareth-morgan-nv,
jiwenc-nv and
lotusl-code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx`:
- Around line 729-736: The setStreamConfig method signature declares
useSecureConnection but never applies it; either remove it from the parameter
list or apply it to the stored configuration. To fix: locate setStreamConfig and
either delete useSecureConnection from its parameter type and any callers, or
when config.useSecureConnection is defined assign it into the component's
configuration/state (e.g. this.configuration.useSecureConnection =
config.useSecureConnection) and ensure
updateConfiguration()/getDefaultConfiguration() and any WebSocket/connect logic
read that field so the runtime uses the new value; update any tests/call sites
accordingly.
In `@src/core/cloudxr/python/__main__.py`:
- Around line 95-98: Replace the try/except/pass around loop.add_signal_handler
with contextlib.suppress to make the intent explicit: import contextlib and wrap
the call to loop.add_signal_handler(sig, on_signal) inside
contextlib.suppress(NotImplementedError) so that NotImplementedError is quietly
ignored (e.g., contextlib.suppress(NotImplementedError):
loop.add_signal_handler(sig, on_signal)); keep the on_signal handler and loop
variables unchanged.
In `@src/core/cloudxr/python/static/operator/index.html`:
- Around line 360-372: Add explicit for attributes on each label to improve
accessibility: set the label for="headsetSelect" for the "Headset" label,
for="serverIP" for the "Server IP" label, for="port" for the "Port" label, and
for="proxyUrl" for the "Proxy URL" label so each label is explicitly linked to
its corresponding input/select elements (IDs: headsetSelect, serverIP, port,
proxyUrl).
In `@src/core/cloudxr/python/static/operator/operator.js`:
- Around line 318-336: The metric label/value are injected into HTML via tiles
and metricsPanelEl.innerHTML without escaping (see formatMetric and
humanizeMetricKey usage), which can lead to XSS; fix by escaping or avoiding
innerHTML: build DOM nodes with document.createElement and set textContent (or
run display.label/display.value through a sanitizer/escape function) when
creating metric tiles instead of interpolating raw strings into tiles and
metricsPanelEl.innerHTML so that humanizeMetricKey output cannot execute script.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 121-147: get_snapshot currently reads shared state
(self._headsets, self._dashboards) without synchronization which can produce
inconsistent snapshots under concurrent access; make get_snapshot an async def
and acquire the existing self._lock (await self._lock.acquire() / use async with
if it's an AsyncLock) around the construction of headsets/dashboards and the
returned dict, then release the lock, and update all callers to await
hub.get_snapshot() so callers hold the same async contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a34c4032-67b2-4aec-a8f7-a3e92b5aaab9
📒 Files selected for processing (16)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdocs/source/index.rstdocs/source/references/teleop_control_protocol.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/static/operator/index.htmlsrc/core/cloudxr/python/static/operator/operator.jssrc/core/cloudxr/python/teleop_ws_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_teleop_ws_hub.pysrc/core/python/MANIFEST.insrc/core/python/pyproject.toml.in
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/core/cloudxr/python/static/operator/index.html (1)
360-372:⚠️ Potential issue | 🟡 MinorAdd
forattributes to labels for accessibility compliance.The
<label>elements lack explicitforattributes linking them to their corresponding inputs. While implicit association may work in some browsers, explicit association ensures reliable screen reader support.♿ Proposed fix
- <label>Headset</label> + <label for="headsetSelect">Headset</label> <select id="headsetSelect" ...> - <label>Server IP</label> + <label for="serverIP">Server IP</label> <input id="serverIP" type="text" ... /> - <label>Port</label> + <label for="port">Port</label> <input id="port" type="number" ... /> - <label>Proxy URL <span ...>(optional)</span></label> + <label for="proxyUrl">Proxy URL <span ...>(optional)</span></label> <input id="proxyUrl" type="text" ... />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/static/operator/index.html` around lines 360 - 372, Add explicit for attributes on each label to link them to their inputs for accessibility: update the <label> elements for the fields headsetSelect, serverIP, port, and proxyUrl so they include for="headsetSelect", for="serverIP", for="port", and for="proxyUrl" respectively, ensuring each label matches the id of its corresponding <select> or <input> element (Headset → headsetSelect, Server IP → serverIP, Port → port, Proxy URL → proxyUrl).deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx (1)
729-736:⚠️ Potential issue | 🟡 Minor
useSecureConnectionparameter is declared but not handled.The method signature includes
useSecureConnection?: booleanbut the method body doesn't use it. If the hub pushes this configuration, it will be silently ignored. Either remove the unused parameter or implement handling logic.💡 Option: Remove unused parameter
public setStreamConfig(config: { serverIP?: string; port?: number; - useSecureConnection?: boolean; proxyUrl?: string | null; mediaAddress?: string; mediaPort?: number; }): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx` around lines 729 - 736, The setStreamConfig signature includes useSecureConnection but the method body ignores it; update setStreamConfig to either remove the unused useSecureConnection parameter or wire it into the component state/connection config (e.g., assign to this.useSecureConnection or update whatever connectionOptions object is used by CloudXR2DUI) so the hub-provided secure/secure flag is honored when building URLs or opening sockets (use it to choose ws:// vs wss:// or http:// vs https:// in connection setup); ensure any code that constructs connection endpoints (connectionOptions, createConnection, or similar methods) reads this flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/cloudxr/webxr_client/helpers/controlChannel.ts`:
- Around line 100-104: The _openWebSocket method can throw synchronously when
calling new WebSocket(this.opts.url) for malformed controlWsUrl values; wrap the
constructor in a try/catch inside _openWebSocket (guarding early return when
this.disposed) to catch constructor errors, log or forward the error via the
same error/close handling path used by the existing onerror/onclose handlers,
ensure this.ws is unset on failure, and trigger any reconnect/backoff logic so
the control-channel setup doesn't abort silently; update references inside
_openWebSocket (new WebSocket, this.ws, this.opts.url, and any existing
onerror/onclose/reconnect calls) to use the caught error path.
In `@deps/cloudxr/webxr_client/src/App.tsx`:
- Around line 640-643: The onSessionCommand handler currently calls
doConnectRef.current?.() and drops the returned promise, causing unhandled
rejections if doConnect (which calls enterAR/enterVR) fails; update the
'connect' branch to handle promise rejections by attaching a catch that calls
setErrorMessage(...) with a user-friendly error string (or await within an async
wrapper) so failures show in the UI; reference the onSessionCommand handler,
doConnectRef.current / doConnect(), enterAR()/enterVR(), and setErrorMessage
when making this change.
In `@docs/source/references/teleop_control_protocol.rst`:
- Around line 18-20: The docs claim teleop routes return 404 without
--enable-hub, but the WSS handler currently falls through to the certificate
page with HTTP 200; either update the doc to state the actual behavior or change
the handler to return 404 when the hub is disabled. Locate the request dispatch
logic in the WSS handler (e.g., functions/methods in the WSS class or request
handler in src/core/cloudxr/python/wss.py such as the route dispatch or
certificate page fallback) and either: 1) modify the fallback so that when the
enable-hub flag is false the handler responds with 404 for teleop paths, or 2)
update teleop_control_protocol.rst to reflect that teleop paths currently return
the certificate page with HTTP 200 when the hub is off.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 269-287: The code updates _stream_config and _config_version
before checking targetClientId, which can commit a config change even if the
specified headset doesn't exist; instead, validate target_id (from
payload.get("targetClientId")) against self._headsets/all_headsets and return
the NOT_FOUND via self._send_error if missing before mutating _stream_config or
bumping _config_version; only after target existence is confirmed should you
merge into self._stream_config, increment _config_version, take
config_snapshot/version, and proceed to notify targets.
- Around line 178-208: Holding self._lock while awaiting network I/O (await
self._send) can block other operations; modify register logic in the method that
contains the async with self._lock so that you only mutate shared state
(_headsets, _dashboards) and log inside the lock, but prepare the hello payload
(for headset: {"clientId": client_id, "configVersion": self._config_version,
"config": dict(self._stream_config)}; for dashboard: {"clientId": client_id})
and the target ws outside the lock, then release the lock and call await
self._send(ws, "hello", payload) afterwards; keep use of
_HeadsetState/_DashboardState, client_id, ws, and self._send as the locating
symbols.
In `@src/core/python/MANIFEST.in`:
- Line 30: The MANIFEST.in line currently uses "recursive-include
isaacteleop/cloudxr/static *" which diverges from pyproject.toml.in that only
includes "static/operator/*"; update either pyproject.toml.in to include the
broader static tree (e.g., static/**/* if supported) or make MANIFEST.in
explicit to match pyproject by replacing the recursive-include with
"recursive-include isaacteleop/cloudxr/static/operator *" so sdist and wheel
package-data remain consistent; adjust the file where "recursive-include
isaacteleop/cloudxr/static *" appears accordingly.
---
Duplicate comments:
In `@deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx`:
- Around line 729-736: The setStreamConfig signature includes
useSecureConnection but the method body ignores it; update setStreamConfig to
either remove the unused useSecureConnection parameter or wire it into the
component state/connection config (e.g., assign to this.useSecureConnection or
update whatever connectionOptions object is used by CloudXR2DUI) so the
hub-provided secure/secure flag is honored when building URLs or opening sockets
(use it to choose ws:// vs wss:// or http:// vs https:// in connection setup);
ensure any code that constructs connection endpoints (connectionOptions,
createConnection, or similar methods) reads this flag.
In `@src/core/cloudxr/python/static/operator/index.html`:
- Around line 360-372: Add explicit for attributes on each label to link them to
their inputs for accessibility: update the <label> elements for the fields
headsetSelect, serverIP, port, and proxyUrl so they include for="headsetSelect",
for="serverIP", for="port", and for="proxyUrl" respectively, ensuring each label
matches the id of its corresponding <select> or <input> element (Headset →
headsetSelect, Server IP → serverIP, Port → port, Proxy URL → proxyUrl).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f16be14-37a6-46d3-8ae6-62d544c49718
📒 Files selected for processing (16)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdocs/source/index.rstdocs/source/references/teleop_control_protocol.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/static/operator/index.htmlsrc/core/cloudxr/python/static/operator/operator.jssrc/core/cloudxr/python/teleop_ws_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_teleop_ws_hub.pysrc/core/python/MANIFEST.insrc/core/python/pyproject.toml.in
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
2 times, most recently
from
afd5a42 to
6699624
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/core/cloudxr/python/static/operator/index.html (1)
360-372:⚠️ Potential issue | 🟡 MinorAdd
forattributes to labels for accessibility.The labels lack explicit
forattributes to associate them with their inputs. This was flagged by static analysis and a previous review.♿ Proposed fix
- <label>Headset</label> + <label for="headsetSelect">Headset</label> <select id="headsetSelect" ...> - <label>Server IP</label> + <label for="serverIP">Server IP</label> <input id="serverIP" type="text" .../> - <label>Port</label> + <label for="port">Port</label> <input id="port" type="number" .../> - <label>Proxy URL ...</label> + <label for="proxyUrl">Proxy URL ...</label> <input id="proxyUrl" type="text" .../>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/static/operator/index.html` around lines 360 - 372, Add explicit for attributes to each label to improve accessibility by matching them to their corresponding input/select element ids: set the Headset label's for="headsetSelect", the Server IP label's for="serverIP", the Port label's for="port", and the Proxy URL label's for="proxyUrl". Ensure each label's for value exactly matches the id attribute (headsetSelect, serverIP, port, proxyUrl) so clicking the label focuses the associated control.src/core/cloudxr/python/teleop_ws_hub.py (2)
270-292:⚠️ Potential issue | 🟠 MajorConfig is committed before validating
targetClientId.Lines 276-277 update
_stream_configand_config_versionbefore lines 283-289 check if the target headset exists. IftargetClientIdis invalid, the config change is already committed and subsequent identical updates become no-ops.Consider validating the target before mutating state:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/teleop_ws_hub.py` around lines 270 - 292, The code currently updates self._stream_config and increments self._config_version inside the async with self._lock block before validating targetClientId (target_id), which can commit an unwanted no-op change if the target headset (from self._headsets) doesn't exist; fix by validating the target first (resolve targets from self._headsets using target_id and call self._send_error if missing) while holding the lock, and only after confirming the target(s) exist assign self._stream_config = merged, increment self._config_version, snapshot version/config, and then proceed with the rest of the logic so state is not mutated on invalid targetClientId.
179-211:⚠️ Potential issue | 🟠 MajorNetwork I/O while holding the lock can cause contention.
Lines 193-201 and 209 await
self._send()whileself._lockis held. A slow or backpressured client during registration can stall other register/config/command operations.Consider preparing the hello payload inside the lock, then sending outside:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/teleop_ws_hub.py` around lines 179 - 211, The registration code is awaiting network I/O (await self._send) while holding self._lock, which can block other operations; fix by creating and storing the _HeadsetState or _DashboardState and preparing the hello payload and clientId/config data while inside the lock (use self._headsets, self._dashboards, self._stream_config, self._config_version to build the payload), then release the lock and call await self._send(...) outside the locked block (keep use of self._lock and the state assignments intact but move the awaits so only non-blocking work is done while locked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/cloudxr_tests/python/pyproject.toml`:
- Around line 10-18: The dev and test optional dependency groups are duplicated;
update the pyproject.toml so you don't repeat the same list: either remove one
group and keep a single optional-dependencies group (e.g., keep "dev") or make
the "test" group reference "dev" (or vice versa) so the same dependency list is
reused; edit the [project.optional-dependencies] section to eliminate the
duplicate arrays for dev and test and ensure tooling that reads these keys still
recognizes the intended group name.
In `@src/core/python/pyproject.toml.in`:
- Around line 66-72: The package data pattern for "isaacteleop.cloudxr"
currently uses "static/operator/*" which only includes files directly under
operator/; update this pattern to a recursive glob (e.g.,
"static/operator/**/*") to include any future subdirectories (requires
setuptools ≥62.3.0), or explicitly document/ensure assets remain flat; edit the
entry for "isaacteleop.cloudxr" in pyproject.toml.in to replace the
non-recursive pattern with the recursive one (or add additional patterns for
nested dirs) so nested assets like static/operator/assets/... are packaged.
---
Duplicate comments:
In `@src/core/cloudxr/python/static/operator/index.html`:
- Around line 360-372: Add explicit for attributes to each label to improve
accessibility by matching them to their corresponding input/select element ids:
set the Headset label's for="headsetSelect", the Server IP label's
for="serverIP", the Port label's for="port", and the Proxy URL label's
for="proxyUrl". Ensure each label's for value exactly matches the id attribute
(headsetSelect, serverIP, port, proxyUrl) so clicking the label focuses the
associated control.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 270-292: The code currently updates self._stream_config and
increments self._config_version inside the async with self._lock block before
validating targetClientId (target_id), which can commit an unwanted no-op change
if the target headset (from self._headsets) doesn't exist; fix by validating the
target first (resolve targets from self._headsets using target_id and call
self._send_error if missing) while holding the lock, and only after confirming
the target(s) exist assign self._stream_config = merged, increment
self._config_version, snapshot version/config, and then proceed with the rest of
the logic so state is not mutated on invalid targetClientId.
- Around line 179-211: The registration code is awaiting network I/O (await
self._send) while holding self._lock, which can block other operations; fix by
creating and storing the _HeadsetState or _DashboardState and preparing the
hello payload and clientId/config data while inside the lock (use
self._headsets, self._dashboards, self._stream_config, self._config_version to
build the payload), then release the lock and call await self._send(...) outside
the locked block (keep use of self._lock and the state assignments intact but
move the awaits so only non-blocking work is done while locked).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: adb6bf8d-1f95-4e1b-80c3-605d6b765d13
📒 Files selected for processing (16)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdocs/source/index.rstdocs/source/references/teleop_control_protocol.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/static/operator/index.htmlsrc/core/cloudxr/python/static/operator/operator.jssrc/core/cloudxr/python/teleop_ws_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_teleop_ws_hub.pysrc/core/python/MANIFEST.insrc/core/python/pyproject.toml.in
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/core/cloudxr/python/static/operator/index.html (1)
360-372:⚠️ Potential issue | 🟠 MajorAssociate these labels with their controls.
These
<label>elements are not wrapping the<select>/<input>elements, so screen readers will not announce the field names for the dashboard’s primary controls. Please add explicitforattributes forheadsetSelect,serverIP,port, andproxyUrl.♿ Suggested fix
- <label>Headset</label> + <label for="headsetSelect">Headset</label> <select id="headsetSelect" style="width:100%;background:var(--surface2);border:1px solid var(--border);border-radius:6px;color:var(--text);font-size:0.875rem;padding:7px 10px;outline:none;"> <option value="">— no headset selected —</option> </select> - <label>Server IP</label> + <label for="serverIP">Server IP</label> <input id="serverIP" type="text" placeholder="192.168.1.100" /> - <label>Port</label> + <label for="port">Port</label> <input id="port" type="number" placeholder="48322" min="1" max="65535" /> - <label>Proxy URL <span style="color:var(--text-muted);font-weight:400">(optional)</span></label> + <label for="proxyUrl">Proxy URL <span style="color:var(--text-muted);font-weight:400">(optional)</span></label> <input id="proxyUrl" type="text" placeholder="https://proxy.example.com/" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/static/operator/index.html` around lines 360 - 372, Add explicit for attributes to each label so they associate with their corresponding controls: set the label for="headsetSelect" for the select with id "headsetSelect", label for="serverIP" for the input id "serverIP", label for="port" for the input id "port", and label for="proxyUrl" for the input id "proxyUrl" in the operator UI (ids: headsetSelect, serverIP, port, proxyUrl) to ensure screen readers announce the field names.src/core/cloudxr/python/teleop_ws_hub.py (1)
274-277:⚠️ Potential issue | 🟠 MajorCheck
targetClientIdbefore the no-op fast path.The early
merged == self._stream_configreturn still runs before target validation. That means a stale/disconnectedtargetClientIdcombined with{}or an identical config is treated as a silent success instead ofNOT_FOUND, so targeted updates become indistinguishable from a real no-op.♻️ Proposed fix
async with self._lock: - merged = {**self._stream_config, **new_config} - if merged == self._stream_config: - return # no-op: empty {}, redundant values, or no change - all_headsets = list(self._headsets.values()) if target_id is not None: targets = [s for s in all_headsets if s.client_id == target_id] if not targets: missing_target = target_id - else: - self._stream_config = merged - self._config_version += 1 - version = self._config_version - config_snapshot = dict(self._stream_config) else: targets = all_headsets + + if missing_target is None: + merged = {**self._stream_config, **new_config} + if merged == self._stream_config: + return self._stream_config = merged self._config_version += 1 version = self._config_version config_snapshot = dict(self._stream_config)Also applies to: 279-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/teleop_ws_hub.py` around lines 274 - 277, The early no-op check returns before validating a requested target, causing stale/disconnected targetClientId updates to be treated as success; inside the same async with self._lock block (around the merged = {**self._stream_config, **new_config} logic) validate any requested targetClientId from new_config or merged before taking the fast-path return: if a targetClientId is present, confirm it refers to a currently connected/known client (use your hub’s client lookup, e.g. self._clients or self._client_by_id / equivalent check), and if not found return the NOT_FOUND error instead of returning early; apply the same targetClientId validation to the other similar fast-paths noted (the block covering lines 279–301).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/cloudxr/webxr_client/src/App.tsx`:
- Around line 301-336: doConnect currently only validates per-eye resolution and
then calls store.enterAR()/enterVR(), letting control-channel triggered connects
bypass the button path guards; refactor so doConnect enforces the same preflight
checks the UI connect button uses: verify reprojection-grid validation,
capability/immersive-support readiness, and that there is no active XR session
before calling store.enterAR()/store.enterVR(); implement this by invoking the
existing button preflight validation helper(s) (or extract the button’s guard
logic into a shared function like validatePreconnect or ensureConnectAllowed) at
the top of doConnect (using ui.getConfiguration() and whatever guard functions
the button path uses), aborting early and updating UI state if any guard fails,
then proceed to setFrameRate as currently done.
In `@src/core/cloudxr/python/CMakeLists.txt`:
- Around line 112-115: Add a removal step to purge the staged static tree before
the copy so stale/renamed files don’t survive incremental builds: before the
existing COMMAND that runs ${CMAKE_COMMAND} -E copy_directory
"${CMAKE_CURRENT_SOURCE_DIR}/static" "${CLOUDXR_PYTHON_DIR}/static", insert a
COMMAND invoking ${CMAKE_COMMAND} -E rm -rf on "${CLOUDXR_PYTHON_DIR}/static"
(similar to the existing cleanup for "${CLOUDXR_PYTHON_DIR}/__pycache__") so the
static directory is cleared prior to copying.
In `@src/core/cloudxr/python/static/operator/index.html`:
- Around line 382-385: The comment says the displayed shortcut hint claiming C/D
only work when focus is not in a field is false because the keydown listener on
document in src/core/cloudxr/python/static/operator/operator.js does not ignore
focused inputs; update the keydown handler (the function attached via
document.addEventListener('keydown', ...) or the handler named handleKeyDown) to
early-return when document.activeElement is an input, textarea, select, or any
element with contenteditable=true (use element.tagName and
element.isContentEditable), so typing in the stream-target form won't trigger
connect/disconnect; adjust the handler logic and unit/behavior tests if present.
In `@src/core/cloudxr/python/static/operator/operator.js`:
- Around line 21-24: WS_URL is conditionally overridden by params.get('wsUrl')
but STATE_URL still uses window.location.host; update STATE_URL to derive its
host from the same wsUrl override so both socket and polling endpoints point to
the hub target. Parse the host/authority from params.get('wsUrl') (if present)
and build STATE_URL using that origin (fallback to window.location.host) so
STATE_URL and WS_URL remain consistent; touch the constants WS_URL and STATE_URL
in operator.js and ensure parsing handles wss/wss/https scheme differences.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 259-264: The handler _handle_set_config currently accepts any JSON
object from payload.get("config") without validation; update _handle_set_config
to validate the config schema before persisting or forwarding: verify required
fields exist and have correct types (e.g., "port" is an integer within 1–65535,
"serverIP" is a non-empty string matching an IP/hostname pattern, booleans are
bools, numeric ranges are enforced), and on any invalid field call
_send_error(ws, "BAD_REQUEST", "<field> invalid") and return; centralize the
rules (a small helper validate_config(config) or inline checks) so only
validated config is stored and forwarded.
---
Duplicate comments:
In `@src/core/cloudxr/python/static/operator/index.html`:
- Around line 360-372: Add explicit for attributes to each label so they
associate with their corresponding controls: set the label for="headsetSelect"
for the select with id "headsetSelect", label for="serverIP" for the input id
"serverIP", label for="port" for the input id "port", and label for="proxyUrl"
for the input id "proxyUrl" in the operator UI (ids: headsetSelect, serverIP,
port, proxyUrl) to ensure screen readers announce the field names.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 274-277: The early no-op check returns before validating a
requested target, causing stale/disconnected targetClientId updates to be
treated as success; inside the same async with self._lock block (around the
merged = {**self._stream_config, **new_config} logic) validate any requested
targetClientId from new_config or merged before taking the fast-path return: if
a targetClientId is present, confirm it refers to a currently connected/known
client (use your hub’s client lookup, e.g. self._clients or self._client_by_id /
equivalent check), and if not found return the NOT_FOUND error instead of
returning early; apply the same targetClientId validation to the other similar
fast-paths noted (the block covering lines 279–301).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c4c2226-a49e-484c-b2b0-39ae357922b8
📒 Files selected for processing (16)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdocs/source/index.rstdocs/source/references/teleop_control_protocol.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/static/operator/index.htmlsrc/core/cloudxr/python/static/operator/operator.jssrc/core/cloudxr/python/teleop_ws_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_teleop_ws_hub.pysrc/core/python/MANIFEST.insrc/core/python/pyproject.toml.in
Comment on lines
+301
to
+336
| const doConnect = async () => { | ||
| const config = ui.getConfiguration(); | ||
| const resolutionError = getResolutionValidationError( | ||
| config.perEyeWidth, | ||
| config.perEyeHeight | ||
| ); | ||
| if (resolutionError) { | ||
| ui.updateConnectButtonState(); | ||
| return; | ||
| } | ||
| // Start XR session | ||
| if (config.immersiveMode === 'ar') { | ||
| await store.enterAR(); | ||
| } else if (config.immersiveMode === 'vr') { | ||
| await store.enterVR(); | ||
| } else { | ||
| setErrorMessage('Unrecognized immersive mode'); | ||
| } | ||
| store.setFrameRate((supportedFrameRates: ArrayLike<number>): number | false => { | ||
| let frameRate = ui.getConfiguration().deviceFrameRate; | ||
| let found = false; | ||
| for (let i = 0; i < supportedFrameRates.length; ++i) { | ||
| if (supportedFrameRates[i] === frameRate) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| // Start XR session | ||
| if (config.immersiveMode === 'ar') { | ||
| await store.enterAR(); | ||
| } else if (config.immersiveMode === 'vr') { | ||
| await store.enterVR(); | ||
| if (found) { | ||
| console.info('Changed frame rate to', frameRate); | ||
| return frameRate; | ||
| } else { | ||
| setErrorMessage('Unrecognized immersive mode'); | ||
| console.error('Failed to change frame rate to', frameRate); | ||
| return false; | ||
| } | ||
| store.setFrameRate((supportedFrameRates: ArrayLike<number>): number | false => { | ||
| let frameRate = ui.getConfiguration().deviceFrameRate; | ||
| let found = false; | ||
| for (let i = 0; i < supportedFrameRates.length; ++i) { | ||
| if (supportedFrameRates[i] === frameRate) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if (found) { | ||
| console.info('Changed frame rate to', frameRate); | ||
| return frameRate; | ||
| } else { | ||
| console.error('Failed to change frame rate to', frameRate); | ||
| return false; | ||
| } | ||
| }); | ||
| }, | ||
| (error: Error) => { | ||
| setErrorMessage(`Failed to start XR session: ${error}`); | ||
| } | ||
| ); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Make doConnect() enforce the same preflight checks as the button path.
After the control-channel path started calling doConnectRef.current directly, this function became the shared connect entry point. It still only checks per-eye resolution, so hub-triggered connects bypass the button’s other guards: reprojection-grid validation, capability/immersive-support readiness, and the “XR session already active” gate. That means the operator can trigger enterAR()/enterVR() while the UI correctly shows CONNECT as disabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/cloudxr/webxr_client/src/App.tsx` around lines 301 - 336, doConnect
currently only validates per-eye resolution and then calls
store.enterAR()/enterVR(), letting control-channel triggered connects bypass the
button path guards; refactor so doConnect enforces the same preflight checks the
UI connect button uses: verify reprojection-grid validation,
capability/immersive-support readiness, and that there is no active XR session
before calling store.enterAR()/store.enterVR(); implement this by invoking the
existing button preflight validation helper(s) (or extract the button’s guard
logic into a shared function like validatePreconnect or ensureConnectAllowed) at
the top of doConnect (using ui.getConfiguration() and whatever guard functions
the button path uses), aborting early and updating UI state if any guard fails,
then proceed to setFrameRate as currently done.
Comment on lines
112
to
115
| COMMAND ${CMAKE_COMMAND} -E copy_directory | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/static" | ||
| "${CLOUDXR_PYTHON_DIR}/static" | ||
| COMMAND ${CMAKE_COMMAND} -E rm -rf "${CLOUDXR_PYTHON_DIR}/__pycache__" |
There was a problem hiding this comment.
Purge the staged static/ tree before copying it.
copy_directory only overlays files. If an operator asset gets renamed or deleted, the old file survives under ${CLOUDXR_PYTHON_DIR}/static and can leak into incremental packages/builds. You already clean __pycache__; do the same for static/ before the copy.
🧹 Suggested fix
add_custom_target(cloudxr_python ALL
COMMAND ${CMAKE_COMMAND} -E make_directory "${CLOUDXR_PYTHON_DIR}"
+ COMMAND ${CMAKE_COMMAND} -E rm -rf "${CLOUDXR_PYTHON_DIR}/static"
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_CURRENT_SOURCE_DIR}/__init__.py"
"${CMAKE_CURRENT_SOURCE_DIR}/__main__.py"
"${CMAKE_CURRENT_SOURCE_DIR}/env_config.py"
"${CMAKE_CURRENT_SOURCE_DIR}/runtime.py"
"${CMAKE_CURRENT_SOURCE_DIR}/wss.py"
"${CMAKE_CURRENT_SOURCE_DIR}/teleop_ws_hub.py"
"${CLOUDXR_PYTHON_DIR}/"
COMMAND ${CMAKE_COMMAND} -E copy_directory
"${CMAKE_CURRENT_SOURCE_DIR}/static"
"${CLOUDXR_PYTHON_DIR}/static"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/cloudxr/python/CMakeLists.txt` around lines 112 - 115, Add a removal
step to purge the staged static tree before the copy so stale/renamed files
don’t survive incremental builds: before the existing COMMAND that runs
${CMAKE_COMMAND} -E copy_directory "${CMAKE_CURRENT_SOURCE_DIR}/static"
"${CLOUDXR_PYTHON_DIR}/static", insert a COMMAND invoking ${CMAKE_COMMAND} -E rm
-rf on "${CLOUDXR_PYTHON_DIR}/static" (similar to the existing cleanup for
"${CLOUDXR_PYTHON_DIR}/__pycache__") so the static directory is cleared prior to
copying.
jiwenc-nv
reviewed
Mar 25, 2026
jiwenc-nv
reviewed
Mar 25, 2026
| Teleop Control Protocol | ||
| ======================= | ||
|
|
||
| Two-way control channel between a **PC operator dashboard** and an **XR headset** running the |
Collaborator
There was a problem hiding this comment.
the term "operator" maybe a bit overloaded.
|
|
||
| An unknown ``type`` causes the server to send an ``error`` message and ignore the payload. | ||
|
|
||
| Messages: Client → Server |
Collaborator
There was a problem hiding this comment.
is this the "Isaac Teleop web client"? or the new UI meant to be running on a separate workstation?
| .. SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| .. SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| Teleop Control Protocol |
Collaborator
There was a problem hiding this comment.
the architecture is getting complex now, we have 3 computer in this case:
- XR headset the human is wearing
- A PC the human is using for command & control
- The compute runs CXR Runtime & Teleop Session (Cloud VM, on robot, OR a separate workstation)
the rest of the doc mentions client <-> server which is not painting the whole picture and can get a reader confused about which computer is talking to which.
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
from
6699624 to
1cfd56d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/core/cloudxr/python/teleop_ws_hub.py (2)
270-287:⚠️ Potential issue | 🟠 MajorResolve
targetClientIdbefore the no-op fast path.
merged == self._stream_configreturns before any target lookup, so an empty/identical update aimed at a staletargetClientIdis silently accepted instead of returningNOT_FOUND. That makes stale dashboard selections look successful even though no headset exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/teleop_ws_hub.py` around lines 270 - 287, Resolve the requested targetClientId before taking the no-op fast path: inside the async with self._lock block, compute target_id = payload.get("targetClientId") and derive all_headsets = list(self._headsets.values()) and targets = [s for s in all_headsets if s.client_id == target_id] (set missing_target when not found) before computing merged = {**self._stream_config, **new_config} and checking merged == self._stream_config; if missing_target is set, return the NOT_FOUND response immediately so stale targetClientId is rejected even for no-op/identical updates.
262-267:⚠️ Potential issue | 🟠 MajorValidate the stream-config schema before merging it.
Checking only
isinstance(new_config, dict)lets values like{"port": "48322"}or{"serverIP": []}become authoritative hub state and get pushed to headsets. The dashboard form validates locally, but this endpoint is still a public protocol surface and needs server-side field/range checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/teleop_ws_hub.py` around lines 262 - 267, In _handle_set_config, do full server-side schema validation instead of only isinstance(new_config, dict): validate expected keys (e.g., "port", "serverIP", any known config fields), enforce types (port must be an int), ranges (port within 1-65535), and formats (serverIP is a valid IP/hostname string), reject unknown keys, and only merge into hub state if validation passes; if validation fails, call self._send_error(ws, "BAD_REQUEST", "<specific message>") with a clear reason. Ensure you update the merge path that currently applies new_config so it only executes after successful validation performed inside _handle_set_config (refer to function name _handle_set_config and the existing _send_error helper).src/core/cloudxr/python/wss.py (1)
470-479:⚠️ Potential issue | 🔴 CriticalDo not start the control hub in an unauthenticated mode by default.
When
enable_hubis true andCONTROL_TOKENis unset, this exposes/teleop/,/api/teleop/v1/state, and/teleop/v1/wswith no credential check. Any browser that can reach the proxy port can then register a dashboard/headset and issuesetConfig/command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/wss.py` around lines 470 - 479, Currently the code will start TeleopControlHub when enable_hub is true even if CONTROL_TOKEN is unset, enabling unauthenticated access; change the logic in wss.py so TeleopControlHub is only instantiated when a valid control token is provided (i.e., check os.environ.get("CONTROL_TOKEN") and refuse/skip starting the hub if it is None or empty), and log a clear warning/error instead of enabling the hub in unauthenticated mode; reference the enable_hub variable, the CONTROL_TOKEN env read, and the TeleopControlHub constructor to locate and update the code path.deps/cloudxr/webxr_client/src/App.tsx (1)
301-336:⚠️ Potential issue | 🟠 MajorReuse the full connect-button preflight in
doConnect().
doConnect()is now the shared entry point for both the local button and remotesessionCommand.connect, but it still only revalidates per-eye resolution. The button path inCloudXR2DUI.setupConnectButtonHandler()rejects other disabled states too, so hub-triggered connects can still callenterAR()/enterVR()while the local UI would keep CONNECT disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cloudxr/webxr_client/src/App.tsx` around lines 301 - 336, doConnect currently only revalidates per-eye resolution but must run the same full "connect" preflight used by the local button handler to avoid hub-triggered connects bypassing UI disable rules; modify doConnect to invoke the same validation routine used by CloudXR2DUI.setupConnectButtonHandler (e.g., call the shared preflight function or the same checks that updateConnectButtonState uses), and bail out if that preflight indicates connect should be disabled before calling store.enterAR()/enterVR(); keep the existing resolution check but replace the simple updateConnectButtonState() call with the full connect-enabled check so both button and remote triggers share identical validation.src/core/cloudxr/python/static/operator/operator.js (1)
21-24:⚠️ Potential issue | 🟠 MajorDerive
STATE_URLfrom the same hub target asWS_URL.Overriding
?wsUrl=only changes the WebSocket endpoint. Snapshot polling still goes towindow.location.host, so a dashboard hosted on one origin cannot render headsets, config, or metrics from a hub on another origin.♻️ Suggested fix
const WS_URL = params.get('wsUrl') || `wss://${window.location.host}/teleop/v1/ws`; - const STATE_URL = `https://${window.location.host}/api/teleop/v1/state`; + const HUB_ORIGIN = (() => { + const u = new URL(WS_URL, window.location.href); + return `${u.protocol === 'wss:' ? 'https:' : 'http:'}//${u.host}`; + })(); + const STATE_URL = `${HUB_ORIGIN}/api/teleop/v1/state`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cloudxr/python/static/operator/operator.js` around lines 21 - 24, STATE_URL is still built from window.location.host while WS_URL can be overridden by params.get('wsUrl'), causing polling to hit the wrong host; change the code to derive STATE_URL from the same hub target used for WS_URL: read params.get('wsUrl') into the same variable used to set WS_URL, parse it with the URL constructor (or equivalent) to extract host and protocol, convert ws/wss -> http/https accordingly, and build STATE_URL as `${protocol}//${host}/api/teleop/v1/state` (falling back to window.location.host if no wsUrl provided) so both WS_URL and STATE_URL point at the same hub.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/references/teleop_control_protocol.rst`:
- Around line 203-227: The subsection underlines for the headings "setConfig
(control console / ``dashboard`` role only)" and "command (control console /
``dashboard`` role only)" are too short; update the RST underline rows (the
lines of ~ characters) so each underline is exactly the same length as its title
text (use the same ~ character) to restore proper heading formatting for the two
subsections in teleop_control_protocol.rst.
- Around line 223-225: Update the docs to state that configVersion is only
incremented when the merged StreamConfig actually changes: explain that
payload.config is shallow-merged over the current StreamConfig but the hub's
setConfig path (see src/core/cloudxr/python/teleop_ws_hub.py) returns early for
empty or identical merges, leaving configVersion unchanged; rephrase the current
sentence to reflect this no-op behavior and mention that tests assert
empty/identical updates do not bump the version.
In `@src/core/cloudxr/python/static/operator/index.html`:
- Line 7: Remove the restriction that disables pinch-zoom by editing the
viewport meta tag in index.html: replace the current meta element that contains
"user-scalable=no" with a viewport string that allows zooming (for example keep
"width=device-width, initial-scale=1.0" and remove "user-scalable=no" or add
"maximum-scale=5, minimum-scale=1") so the control console supports browser zoom
for accessibility.
In `@src/core/cloudxr/python/static/operator/operator.js`:
- Around line 150-167: When the websocket closes (ws.onclose) you must
immediately disable headset-dependent UI and prevent false success toasts: in
the ws.onclose handler (where setStatusConnected(false), stopPolling(),
reconnectTimer and ws nulling happen) also clear the last snapshot and current
selection and call updateHeadsetDependentButtons() so buttons reflect
disconnection; then modify send(type,payload) to return a boolean or throw when
not connected (instead of only showing a toast) and update all action handlers
that call send(...) to check its return/exception and only show success toasts
on actual successful sends. Ensure references to wsConnected, send,
updateHeadsetDependentButtons, and the ws.onclose handler are updated
accordingly.
In `@src/core/cloudxr/python/wss.py`:
- Around line 67-79: control_console_urls() advertises HTTPS URLs using the LAN
IP, but ensure_certificate() only creates a cert for CN=localhost causing
hostname mismatch; update ensure_certificate() to include Subject Alternative
Names (SANs) that cover 127.0.0.1 and the guessed LAN IPv4 (from
_guess_lan_ipv4()) and/or the machine hostname so the generated self-signed cert
matches the URLs returned by control_console_urls() and the
wss://<LAN-IP>/teleop/v1/ws clients; ensure wss_proxy_port() usage is unchanged
and fallback behavior remains if no LAN IP is found.
---
Duplicate comments:
In `@deps/cloudxr/webxr_client/src/App.tsx`:
- Around line 301-336: doConnect currently only revalidates per-eye resolution
but must run the same full "connect" preflight used by the local button handler
to avoid hub-triggered connects bypassing UI disable rules; modify doConnect to
invoke the same validation routine used by CloudXR2DUI.setupConnectButtonHandler
(e.g., call the shared preflight function or the same checks that
updateConnectButtonState uses), and bail out if that preflight indicates connect
should be disabled before calling store.enterAR()/enterVR(); keep the existing
resolution check but replace the simple updateConnectButtonState() call with the
full connect-enabled check so both button and remote triggers share identical
validation.
In `@src/core/cloudxr/python/static/operator/operator.js`:
- Around line 21-24: STATE_URL is still built from window.location.host while
WS_URL can be overridden by params.get('wsUrl'), causing polling to hit the
wrong host; change the code to derive STATE_URL from the same hub target used
for WS_URL: read params.get('wsUrl') into the same variable used to set WS_URL,
parse it with the URL constructor (or equivalent) to extract host and protocol,
convert ws/wss -> http/https accordingly, and build STATE_URL as
`${protocol}//${host}/api/teleop/v1/state` (falling back to window.location.host
if no wsUrl provided) so both WS_URL and STATE_URL point at the same hub.
In `@src/core/cloudxr/python/teleop_ws_hub.py`:
- Around line 270-287: Resolve the requested targetClientId before taking the
no-op fast path: inside the async with self._lock block, compute target_id =
payload.get("targetClientId") and derive all_headsets =
list(self._headsets.values()) and targets = [s for s in all_headsets if
s.client_id == target_id] (set missing_target when not found) before computing
merged = {**self._stream_config, **new_config} and checking merged ==
self._stream_config; if missing_target is set, return the NOT_FOUND response
immediately so stale targetClientId is rejected even for no-op/identical
updates.
- Around line 262-267: In _handle_set_config, do full server-side schema
validation instead of only isinstance(new_config, dict): validate expected keys
(e.g., "port", "serverIP", any known config fields), enforce types (port must be
an int), ranges (port within 1-65535), and formats (serverIP is a valid
IP/hostname string), reject unknown keys, and only merge into hub state if
validation passes; if validation fails, call self._send_error(ws, "BAD_REQUEST",
"<specific message>") with a clear reason. Ensure you update the merge path that
currently applies new_config so it only executes after successful validation
performed inside _handle_set_config (refer to function name _handle_set_config
and the existing _send_error helper).
In `@src/core/cloudxr/python/wss.py`:
- Around line 470-479: Currently the code will start TeleopControlHub when
enable_hub is true even if CONTROL_TOKEN is unset, enabling unauthenticated
access; change the logic in wss.py so TeleopControlHub is only instantiated when
a valid control token is provided (i.e., check os.environ.get("CONTROL_TOKEN")
and refuse/skip starting the hub if it is None or empty), and log a clear
warning/error instead of enabling the hub in unauthenticated mode; reference the
enable_hub variable, the CONTROL_TOKEN env read, and the TeleopControlHub
constructor to locate and update the code path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 014f905b-ad78-4d08-bc94-69ec43acf6fe
📒 Files selected for processing (16)
deps/cloudxr/webxr_client/helpers/controlChannel.tsdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdocs/source/index.rstdocs/source/references/teleop_control_protocol.rstsrc/core/cloudxr/python/CMakeLists.txtsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/static/operator/index.htmlsrc/core/cloudxr/python/static/operator/operator.jssrc/core/cloudxr/python/teleop_ws_hub.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/conftest.pysrc/core/cloudxr_tests/python/pyproject.tomlsrc/core/cloudxr_tests/python/test_teleop_ws_hub.pysrc/core/python/MANIFEST.insrc/core/python/pyproject.toml.in
Comment on lines
+223
to
+225
| ``payload.config`` may be **partial**; the hub shallow-merges it over the current | ||
| ``StreamConfig``. ``configVersion`` is always monotonically incremented by the hub. | ||
|
|
There was a problem hiding this comment.
Document the no-op setConfig behavior accurately.
The hub does not always increment configVersion: src/core/cloudxr/python/teleop_ws_hub.py returns early when the merged config is unchanged, and the new tests assert that empty/identical updates keep the version unchanged. This sentence currently contradicts the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/source/references/teleop_control_protocol.rst` around lines 223 - 225,
Update the docs to state that configVersion is only incremented when the merged
StreamConfig actually changes: explain that payload.config is shallow-merged
over the current StreamConfig but the hub's setConfig path (see
src/core/cloudxr/python/teleop_ws_hub.py) returns early for empty or identical
merges, leaving configVersion unchanged; rephrase the current sentence to
reflect this no-op behavior and mention that tests assert empty/identical
updates do not bump the version.
Contributor
Author
|
per discussion we want to have a more automated minimal no UI solution, will think about it |
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
2 times, most recently
from
e345f6a to
4eced4e
Compare
yanziz-nvidia
changed the title
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
6 times, most recently
from
9c68695 to
78bf0df
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
4 times, most recently
from
ea1d556 to
1d11c6b
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
3 times, most recently
from
7d8fc23 to
0008919
Compare
yanziz-nvidia
force-pushed
the
yanziz/operator-dashboard
branch
from
0008919 to
dff8f49
Compare
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
2 participants
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.
Adds an operator hub to enable controls on desktop. Added path /teleop/ for the control UI.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores