cloudxr: wire client reconnect/warm-up-timeout config through OOB launch - #1145
Open
gareth-morgan-nv wants to merge 1 commit into
Open
gareth-morgan-nv wants to merge 1 commit into
gareth-morgan-nv wants to merge 1 commit into
Conversation
client_ui_fields_from_env() previously only forwarded codec and panelHiddenAtStart from TELEOP_CLIENT_* env vars into the headset bookmark URL, so reconnectEnabled, reconnectMaxAttempts, reconnectDelayMs, streamAttachTimeoutMs, and the two new warmupBeginTimeoutMs/warmupEndTimeoutMs props (all now supported client-side, see gmorgan/passthrough-only-detection) had no way to reach a real headset launch - they'd sit at their client-side defaults regardless of what an operator configured. Adds the same env-var -> URL-param forwarding for all five, following the existing codec/panelHiddenAtStart pattern exactly: - TELEOP_CLIENT_RECONNECT_ENABLED (bool) - TELEOP_CLIENT_RECONNECT_MAX_ATTEMPTS (int) - TELEOP_CLIENT_RECONNECT_DELAY_MS (int) - TELEOP_CLIENT_STREAM_ATTACH_TIMEOUT_MS (int) - TELEOP_CLIENT_WARMUP_BEGIN_TIMEOUT_MS (int) - TELEOP_CLIENT_WARMUP_END_TIMEOUT_MS (int) build_teleop_url()/build_headset_bookmark_url() needed no other changes: client_ui_fields_from_env()'s output is merged into stream_config generically, so the new keys flow through the existing pipeline automatically. Tests: unit coverage on both boundary functions (client_ui_fields_from_env, build_headset_bookmark_url) in test_oob_teleop_env.py, plus one end-to-end test through build_teleop_url() in test_oob_teleop_adb.py so the full env-var -> bookmark-URL chain is exercised, not just its two ends in isolation. Signed-off-by: Gareth Morgan <gmorgan@nvidia.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: NVIDIA/IsaacCapture/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Open
4 tasks done
gareth-morgan-nv
requested review from
AndreusNvidius,
jiwenc-nv,
nv-jakob and
yanziz-nvidia
September 29, 2026 14:46
jiwenc-nv
requested changes
Sep 29, 2026
| out["panelHiddenAtStart"] = True | ||
| elif ph in ("0", "false", "no", "off"): | ||
| out["panelHiddenAtStart"] = False | ||
| re_enabled = os.environ.get("TELEOP_CLIENT_RECONNECT_ENABLED", "").strip().lower() |
Collaborator
There was a problem hiding this comment.
actually, I would really prefer us to move away from adding environment vars to gate different behaviors. this really causes lots of unnecessary fragmentation in the user flow.
any reason we don't just make reconnect the default and only behavior?
Contributor
Author
There was a problem hiding this comment.
As discussed on slack. I think the retry is better done by the OOB system as it can do so more reliably. Though we could also do via the component as well.
gareth-morgan-nv
added a commit
that referenced
this pull request
Sep 29, 2026
… builds, add crash/relaunch coverage Switch every real-browser test from the real @nvidia/cloudxr SDK to MockCloudXR (npm run build:app-mock): the real SDK genuinely tries to stream against whatever's on backend_port, which is unpredictable and gives a test nothing to assert against. MockCloudXR mounts the identical App.tsx/CloudXRComponent.tsx UI with a fully deterministic, externally controllable session, and never opens a socket of its own, so minimal_mock_runtime() is gone entirely. Switch from a dev-server to a static production build (static_webxr_build()). Two real bugs made this necessary: webpack HMR re-executes a module's top-level code on live-reload, creating a second, disconnected instance of cloudxr-mock-alias.ts's activeSession that window.__mockCloudXRFail() silently bound to instead of the real running session; and <React.StrictMode> (present regardless of HMR) double-invokes effects in a dev-mode build, calling CloudXR.createSession() twice for the same reason. webpack.app-mock.js now builds in production mode. Along the way, a stale webpack persistent filesystem cache (keyed only on the config file, not source files) was found silently serving an old compiled bundle; static_webxr_build() now clears it before every build. window.__mockCloudXRFail() gains an optional `code` param: a code-less failure is "recoverable" per isRecoverable(), and CloudXRComponent.tsx auto-reconnects on those without ever showing the error banner, so forcing a real terminal error needs a code in the non-retryable range. Add two new real-browser tests: one proving the crash-trigger mechanism itself (a real, deterministic, DOM-visible error banner on demand), and one anticipatory test written against gmorgan/oob-error-relaunch (#1146) before that branch has merged here - it asserts on black-box observable recovery (a second real `am start`, a second real tab) rather than importing any private function, so it fails cleanly on that assertion today and should turn green once #1146 lands. Also add test_build_teleop_url_forwards_reliability_config_from_env, the same "write it now, let it fail until merged" idea applied to gmorgan/oob-reliability-config-wiring (#1145)'s plain env-var/string forwarding - no browser needed for that one. Signed-off-by: Gareth Morgan <gmorgan@nvidia.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
3 tasks done
This branch has not been deployed
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
3 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.
Summary
client_ui_fields_from_env()only forwardedcodec/panelHiddenAtStartfromTELEOP_CLIENT_*env vars into the headset bookmark URL.reconnectEnabled,reconnectMaxAttempts,reconnectDelayMs,streamAttachTimeoutMs, and the two newwarmupBeginTimeoutMs/warmupEndTimeoutMsprops (client-side support added in webxr: detect passthrough-only (stream that never attaches) #1142) had no way to reach a real headset launch - they'd sit at their client-side defaults regardless of what an operator configured.codec/panelHiddenAtStartpattern exactly (newTELEOP_CLIENT_RECONNECT_ENABLED,_RECONNECT_MAX_ATTEMPTS,_RECONNECT_DELAY_MS,_STREAM_ATTACH_TIMEOUT_MS,_WARMUP_BEGIN_TIMEOUT_MS,_WARMUP_END_TIMEOUT_MSenv vars).build_teleop_url()/build_headset_bookmark_url()needed no other changes -client_ui_fields_from_env()'s output merges intostream_configgenerically, so new keys flow through the existing pipeline automatically.Test plan
client_ui_fields_from_env,build_headset_bookmark_url) intest_oob_teleop_env.pybuild_teleop_url()intest_oob_teleop_adb.py, exercising the full env-var -> bookmark-URL chain, not just its two ends in isolationruff check/ruff format --checkclean on all three touched filespytest tests/python/core/cloudxr/- 100/100 passing🤖 Generated with Claude Code