Skip to content

fix(webxr): keep config changes from re-arming warm-up watchdogs during idle - #1262

Merged
gareth-morgan-nv merged 1 commit into
mainfrom
gmorgan/webxr-idle-config-rearm-fix
Oct 14, 2026
Merged

gareth-morgan-nv merged 1 commit into
mainfrom
gmorgan/webxr-idle-config-rearm-fix

Conversation

@gareth-morgan-nv

@gareth-morgan-nv gareth-morgan-nv commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #1260 addressing its remaining review findings (the review landed while #1260 was being merged).

  • Config change during idle: re-registering the effect on a config change re-armed the warm-up watchdog even while idle frames were arriving, so a config edit during idle (or its quiet window) disconnected before quiet period plus the phase budget, and the later quiet expiry could arm a second timer and orphan the first. The re-arm is now skipped while idle is active or after real video was seen (the idle quiet timer resumes the watchdog itself), and each arm function clears any existing timer before replacing it.
  • One source of truth: removed idleFramesActiveRef; idleStatusRef.current.active is used instead.
  • Test coverage: StreamAttachTimeoutTest gains idle-then-silence scenarios for warm-up that never began and warm-up that stalled (the watchdog must fire no earlier than quiet period plus phase budget after the last idle frame), and a scenario with config changes during idle and inside the quiet window.

Still inert today: the SDK does not currently emit the Received idle frame log and the session logLevel is Info.

Test plan

  • StreamAttachTimeoutTest.spec.js (5 tests) passes 3/3; reverting the config guard fails the config-change scenario, and making the quiet callback never re-arm fails all three idle-silence scenarios
  • jest (119 tests), eslint, prettier, SKIP=check-copyright-year pre-commit run --all-files clean; tsc --noEmit shows no errors in the touched files
  • Playwright specs are not run in CI, so that run is local only

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved stream startup timeout handling during idle periods, preventing warm-up timers from starting or restarting at the wrong time.
    • Warm-up monitoring now resumes after idle frames stop and the quiet period has elapsed, including when configuration changes during that period.
    • Replacing a timer now clears the existing timer of the same type, helping avoid conflicting timeout behavior.

…ng idle

Review findings on the idle-frame handling:

- Re-registering the effect on a config change re-armed the warm-up watchdog even
  while idle frames were arriving, so a config edit during idle (or its quiet
  window) disconnected before quiet plus the phase budget, and the later quiet
  expiry could arm a second timer and orphan the first. Skip the config re-arm
  while idle is active or after real video was seen, and have each arm function
  clear any existing timer before replacing it.
- Drop idleFramesActiveRef; idleStatusRef.active is the single source of truth.

StreamAttachTimeoutTest gains idle-then-silence scenarios for warm-up that never
began and warm-up that stalled, and a config change during idle and its quiet
window. Each fails if its fix is reverted.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/IsaacCapture/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 1c81ffd6-3e56-45cb-9e8f-35f5dadca45b
📥 Commits

Reviewing files that changed from the base of the PR and between 655e647 and 6b5e62b.

📒 Files selected for processing: 3
  • deps/cloudxr/webxr_client/helpers/react/CloudXRComponent.tsx
  • deps/cloudxr/webxr_client/tests/mock/StreamAttachTimeoutTest.tsx
  • deps/cloudxr/webxr_client/tests/playwright/StreamAttachTimeoutTest.spec.js

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

The component now uses idle status as the source for idle-frame state. Timer-arm helpers clear an existing timer before scheduling a replacement. Config-change watchdog re-arming skips periods when idle frames are active or real video has been seen, and arms only the timer for the incomplete warm-up phase. Mock and Playwright tests cover three idle-silence scenarios, including config changes during idle and the quiet period.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 6b5e6

No actionable issue remains identified; the PR is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. 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.
Title check Passed The title clearly and concisely describes the main change: preventing configuration changes from re-arming WebXR warm-up watchdogs during idle frames.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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.

@gareth-morgan-nv
gareth-morgan-nv merged commit 3201002 into main Oct 14, 2026
40 checks passed
@gareth-morgan-nv
gareth-morgan-nv deleted the gmorgan/webxr-idle-config-rearm-fix branch October 14, 2026 16:09
github-actions Bot added a commit that referenced this pull request Oct 14, 2026

This branch was successfully deployed

1 active deployment
dev — 6b5e62b0 Deployed Oct 13, 2026 by gareth-morgan-nv via publish-wheel #5595
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