Skip to content

fix(webxr): keep Connect disabled until XR startup is ready - #1398

Open
yanziz-nvidia wants to merge 3 commits into
NVIDIA:mainfrom
yanziz-nvidia:yanziz/fix-iwer-xr-fallback-main
Open

yanziz-nvidia wants to merge 3 commits into
NVIDIA:mainfrom
yanziz-nvidia:yanziz/fix-iwer-xr-fallback-main

Conversation

@yanziz-nvidia

@yanziz-nvidia yanziz-nvidia commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Connect can turn green while the desktop XR emulator is still loading: form validation and XR-store notifications re-enable the button before startup capability checks finish. Clicking during that window reaches Chrome's native XRSystem and fails with “No XR hardware found”. Reproduced this error in the PR preview by delaying the emulator bundle.

Keep Connect disabled until XR startup and browser capability checks pass, and enforce that readiness across form validation, session notifications, and click handling. Verify immersive support after emulator installation before reporting success. Roll back unsuccessful runtime installations before retrying, and create DevUI only after immersive support is confirmed so failed attempts do not accumulate UI or listeners. Preserve main's bundled IWER/DevUI loading and dependency versions.

Regression coverage delays the real emulator bundle, attempts a pointer click while loading, verifies that no early requestSession call occurs, edits the form, then connects as soon as the button becomes available. Failed emulator loads also keep Connect disabled. Both browser tests share the emulator-bundle route matcher. Remove the previous smoke-test workaround that waited for an installation log before clicking.

The stream-timeout test now expects Stream stopped with error: because that is the console message emitted by CloudXRComponent on main; the user-facing banner still says CloudXR session stopped:.

Validation:

  • 14 targeted loader and UI unit tests pass, including runtime rollback and retry without duplicate DevUI installation.
  • Four visible-Chrome browser tests pass against the full app with MockCloudXR: normal connection, failed emulator load, stream attach timeout, and delayed emulator readiness.
  • Targeted ESLint and Prettier checks pass.
  • Full repository pre-commit checks pass (SKIP=check-copyright-year).

Signed-off-by: Yanzi Zhu <yanziz@nvidia.com>
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 3f44cf2 and bb307cd.

Files selected for processing: 2
  • deps/cloudxr/webxr_client/helpers/LoadIWER.test.ts
  • deps/cloudxr/webxr_client/helpers/LoadIWER.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

After installing the IWER runtime, loadIWER checks immersive VR and AR support. It marks IWER as loaded and sets window.xrDevice only if at least one session type is supported. Tests verify that a call without an installed runtime reports no support and that a later call retries.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • NVIDIA/IsaacCapture#1395: Updates the IWER and DevUI versions loaded by LoadIWER.ts, which provides the runtime checked by this change.
  • NVIDIA/IsaacCapture#1293: Adds Synthetic XR controls that consume IWER readiness, which this change updates.
Merge Risk: Minimal · previous review bb307

The loader checks immersive support before reporting readiness. No merge-blocking risk was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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.
Title check Passed The title clearly describes the main change: prevent Connect from becoming enabled until WebXR startup reports readiness.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@yanziz-nvidia

Copy link
Copy Markdown
Collaborator Author

/preview-docs

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

✅ Preview deployed: https://NVIDIA.github.io/IsaacCapture/preview/pr-1398/client/

Built from commit 4b7b83a. It may take a minute for the updated preview to appear.

Signed-off-by: Yanzi Zhu <yanziz@nvidia.com>
@yanziz-nvidia yanziz-nvidia changed the title fix(webxr): verify desktop XR emulator readiness fix(webxr): keep Connect disabled until XR startup is ready Oct 29, 2026
@yanziz-nvidia

Copy link
Copy Markdown
Collaborator Author

/preview-docs

@gareth-morgan-nv

Copy link
Copy Markdown
Contributor

🤖 Agent-generated (Claude Code · claude-sonnet-5-5)

Summary

Keeps Connect disabled until XR startup and capability checks pass. CloudXR2DUI tracks capabilitiesReady, so form validation and XR-store notifications can no longer re-enable the button early; setStartButtonState forces it disabled while not ready; App.tsx feeds capabilitiesValid into the UI; and LoadIWER verifies immersive support after installing the emulator. Coverage is strong (unit tests, a Playwright test that delays the real emulator bundle and clicks with a real pointer, and a failed-load case), and the old install-log wait workaround is removed. No blockers; a few suggestions below.

Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit

Finding
💡 helpers/LoadIWER.ts — when installation succeeds but immersive support is missing, window.xrDevice is not set, but the runtime or DevUI may already be installed. A retry then builds a second XRDevice. The new test covers the retry but not double installation or DevUI duplication; worth confirming it is harmless.
💡 tests/playwright/ConnectReadiness.spec.js — expect(...).not.toContainText('Failed to start XR session') after the early click passes immediately, since a negative assertion does not wait. It would pass even if the click took effect a moment later. Add a short wait, or assert that requestSession was not called.
💡 ConnectReadiness.spec.js and ClientUIStatesTest.spec.js — the regex /(?:emulator|iwer.*index_js).*\.js/ is duplicated and tied to bundle file names. Extract a shared constant so a rename cannot silently stop delaying or blocking the bundle.
💡 Scope — this targets main and replaces #1397 (release/1.6.x), so a 1.6 backport is probably still needed.
🔍 src/CloudXR2DUI.tsx — setCapabilitiesReady has no doc comment, and the new capabilitiesReady field uses // rather than a doc block, against the repo's documentation standard.
🔍 tests/playwright/ClientUIStatesTest.spec.js — the expected log text changed to Stream stopped with error:. It matches console.error on main, so it looks right, but the PR does not say why it changed.

Actionables (for bots — copy-paste-ready for AI)

Agent-generated suggestions, not human-vetted obligations. Skip anything wrong, already
addressed, or not worth the churn.

  • deps/cloudxr/webxr_client/helpers/LoadIWER.ts — after an installation that reports no immersive support, confirm a retry does not double-install the runtime or DevUI; add a test or a guard if it can.
  • deps/cloudxr/webxr_client/tests/playwright/ConnectReadiness.spec.js — replace the immediate negative not.toContainText('Failed to start XR session') after the early click with a check that waits briefly or asserts requestSession was never called.
  • deps/cloudxr/webxr_client/tests/playwright/ConnectReadiness.spec.js, ClientUIStatesTest.spec.js — move the emulator-bundle route regex into one shared constant.
  • deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx — add a doc comment to setCapabilitiesReady and convert the capabilitiesReady field comment to a doc block.
  • PR description — note the 1.6 backport plan and why the expected log text in ClientUIStatesTest.spec.js changed.

Signed-off-by: Yanzi Zhu <yanziz@nvidia.com>
@yanziz-nvidia

Copy link
Copy Markdown
Collaborator Author

thanks @gareth-morgan-nv

Addressed in 5ccaac8:

  • Failed runtime installs are rolled back before retrying; DevUI is created only after immersive support passes.
  • The early-click test now verifies zero requestSession calls.
  • Both tests share the emulator-bundle matcher.
  • Added the readiness doc comments.
  • Updated the PR description to explain the log-text change and removed the 1.6 references. No backport is planned for now.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants