feat(web): group debug settings, add reset-to-defaults and URL param help#705
Conversation
…help The web client's Debug Settings were a flat list of ~16 controls with no categorization, and accepted URL query params were undocumented (visible only in source). This reworks the panel for first-time setup: - Keep Device Profile as the always-visible basic step; fold the rest under an "Advanced settings" divider into 6 collapsed <details> groups (Video & Streaming, Tracking & Pose, In-XR Controls & Display, Network, Device Workarounds, Developer & Session). All element IDs unchanged. - Add a "Reset to defaults" button: clears global + per-project localStorage and the countdown, strips form-backed URL params (keeps direct transport/OOB params + teleop hash), then replaceState + reload. localStorage keys are centralized in localStorageBindings() so setup and reset can't drift. - Add an opt-in `description` to UrlParam and render a collapsed "URL parameters" help list from URL_PARAMS, so it can never drift from the registry; secrets/transport internals stay out by omitting a description. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codec is commonly changed, so surface it next to Device Profile above the "Advanced settings" divider instead of folding it into the Video & Streaming group. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ ± offsets Reorganize the settings panel around how a teleoperator works rather than by technical domain, and address two operator pain points. - Regroup: visible "Setup" block (Device Profile, Codec, Reference Space + offsets) above an "Advanced settings" divider; fold the rest into three task-based groups: "Image quality", "In-XR comfort", "Advanced / troubleshooting". Rename the panel heading from "Debug Settings" to "Setup & Advanced" since the basics are no longer debug-only. All element IDs unchanged. - Reference Space + offsets promoted to the visible Setup block (operators use them to line up their view). - Negative offsets on headset keyboards: some on-screen keyboards lack a minus key, so each XR offset field gets a ± button that flips the value's sign. - Device Profile now defaults from the headset user-agent on a fresh load, reusing the capability check's UA detection (extracted as detectHeadset()). Per Meta's Browser Specs the UA exposes the model token but cannot tell Quest 3 from Quest 3S; the only meaningful split is hardware AV1, so Quest 3/3S -> quest3 (AV1), Quest 2 / Quest 1 / Quest Pro -> quest2 (H.265), Pico -> pico4ultra, desktop/emulator -> custom. An explicit choice (stored deviceProfile) always suppresses the auto-default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/preview-docs |
Records why the UA-based device-profile default resolves to 'custom' under the IWER emulator (it emulates the WebXR device API but does not change navigator.userAgent). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move Maximum Streaming Bitrate, Proxy, and Media Server into a dedicated "Networking" group under Advanced settings (bitrate is a bandwidth setting, so it sits with the network config rather than under Image quality). Rename the "Advanced / troubleshooting" group to just "Troubleshooting". Element IDs unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Act on operator feedback that the daily-driver set is small. Keep the always- visible controls minimal and move the rest into the folded advanced groups. - Rename the left panel "Settings" -> "Connectivity Settings" (server backend, IP + cert, port); move Immersive Mode out of it. - Right panel now shows just Immersive Mode then Video Codec, then the "Advanced settings" divider. - Demote Device Profile into Advanced (it auto-detects from the UA, so manual selection is rare). - Move Reference Space + offsets into a folded "Teleoperator adjustment" group. - Persist each advanced group's expanded/collapsed state in localStorage (cxr.group.<id>); cleared by Reset to defaults. All element IDs unchanged, so URL params and form binding are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/preview-docs |
|
❌ No successful |
stats-gl (via @react-three/drei) nests its own three@0.170 beside the top-level three@0.172. Two three instances in the bundle make React-Three-Fiber throw "Cannot read properties of undefined (reading 'V')" at ACESFilmicToneMapping during Canvas init (surfaces intermittently as build module order shifts). Alias `three` to the top-level copy so the bundle has a single instance. Pre-existing issue, independent of the settings UI changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/preview-docs |
The three alias was based on a stale local node_modules (three 0.172). With the actual dependency set (package.json pins three ^0.184.0; @iwer/* nest 0.165 and stats-gl 0.170), forcing all `three` imports to the top-level 0.184 copy could break those packages. It also does not address the real preview crash, which is a corrupted minified bundle from CI's non-deterministic fresh install (the gitignored lockfile lets the build toolchain float). Reverting to avoid introducing a new failure; the build-determinism fix is tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
webpack was unpinned (pulled transitively via webpack-cli/webpack-dev-server), so CI's fresh install (package-lock.json is gitignored) resolved webpack 5.108.0, which mis-emits re-export property accesses as split multi-segment chains (e.g. `s.A.C.E.S...`, `s.F.V`, `s.X.r.R`). At runtime React-Three-Fiber reads an undefined intermediate during Canvas init and throws "Cannot read properties of undefined (reading 'C'/'V')", so the client fails to load. Reproduced with a clean install (three 0.184, terser 5.48, webpack 5.108 -> corrupt) and confirmed webpack 5.105.4 builds clean; terser version is not the factor. Pin webpack to the known-good 5.105.4 as a direct devDependency so the floating CI install can't pull the regressed minor. Revisit when a fixed webpack ships. Longer term, commit a lockfile + use `npm ci` for reproducible builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx`:
- Around line 221-228: After applying the UA default in CloudXR2DUI
initialization, make sure profile-linked URL seed values are reconciled so the
selected device profile stays in sync with the visible form. Update the startup
flow around applyDefaultDeviceProfileFromUserAgent() and applyUrlSeeds() to
re-run the profile reconciliation logic used by setProfileToCustomIfNeeded(), so
any seeded overrides switch the profile to custom instead of leaving
deviceProfileId pointing at the auto-detected profile.
🪄 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: CHILL
Plan: Enterprise
Run ID: e0063144-19a7-49e0-9251-c38de0cc982c
📒 Files selected for processing (5)
deps/cloudxr/webxr_client/helpers/BrowserCapabilities.tsdeps/cloudxr/webxr_client/helpers/DeviceProfiles.tsdeps/cloudxr/webxr_client/package.jsondeps/cloudxr/webxr_client/src/CloudXR2DUI.tsxdeps/cloudxr/webxr_client/src/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- deps/cloudxr/webxr_client/src/index.html
yanziz-nvidia
left a comment
There was a problem hiding this comment.
Reviewed by yanziz-reviewer-bot
Summary
Refactors the web client debug settings panel into collapsible accordion groups, adds a "Reset to defaults" button backed by a centralized localStorageBindings() map, introduces UA-based device profile auto-detection, and generates the URL parameter help list from the URL_PARAMS registry.
Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit
| Finding | |
|---|---|
| 🚫 | deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx:349 — applyDefaultDeviceProfileFromUserAgent() silently overwrites user-edited per-field values. Guard fires on deviceProfile == null, but a user who edits individual fields (codec, perEyeWidth, etc.) without touching the Device Profile dropdown never stores that key — so on next load, UA detection fires and overwrites their stored values. Fix: after calling applyDeviceProfileToForm(detected), persist deviceProfile to localStorage so subsequent loads take the early-return path. |
| 💡 | deps/cloudxr/webxr_client/src/App.tsx:139 — COUNTDOWN_STORAGE_KEY is module-private; resetToDefaults() duplicates the string literal with a "keep in sync" comment. Lift to module scope, export it, and import in CloudXR2DUI.tsx. |
| 💡 | deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx:570 — Per-project reset keys are hardcoded; a future savePerProject caller would silently survive a reset. Mirror the localStorageBindings() pattern: define a perProjectStorageBindings() constant shared by savePerProject callers and resetToDefaults(). |
| 🔍 | deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx:801 — Sign-flip handler writes '0' to a blanked field and persists it. Guard: if (!Number.isFinite(value)) return;. |
Actionables (for bots — copy-paste-ready for AI)
Fix if it makes sense in context — these are agent-generated suggestions, not human-vetted obligations. Skip anything that's wrong, already addressed, or not worth the churn.
CloudXR2DUI.tsx:349— AfterapplyDeviceProfileToForm(detected), persistdeviceProfileto localStorage so the== nullguard fires only on a true first visit.App.tsx:139— ExportCOUNTDOWN_STORAGE_KEYat module scope; import it inCloudXR2DUI.tsx:573instead of the duplicate string literal.CloudXR2DUI.tsx:570— Replace hardcoded['panelHiddenAtStart', 'headless']with a sharedperProjectStorageBindings()constant, mirroringlocalStorageBindings().CloudXR2DUI.tsx:801— In sign-flip handler:if (!Number.isFinite(value)) return;before negating.
Review cleanups: - Share COUNTDOWN_STORAGE_KEY: define it once in CloudXR2DUI (with the other storage keys) and import it in App.tsx, replacing the duplicate literal that reset relied on. App already imports CloudXR2DUI, so no import cycle. - Centralize the per-project setting keys in a PER_PROJECT_SETTING_KEYS constant used by resetToDefaults, instead of a hardcoded ['panelHiddenAtStart','headless']. - ± offset button: return early on a non-finite value so it is a pure sign flip rather than inserting 0 into an empty field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
fixed comments from bot. Persisting deviceProfile alone would actually introduce a bug: on the next load, setupLocalStorage would restore the dropdown to "Quest 3", but the profile-driven fields (resolution, codec, bitrate…) aren't persisted by the auto-detect path, so they'd revert to HTML defaults — dropdown and fields out of sync. |
The web client's Debug Settings were a flat list of ~16 controls with no categorization, and accepted URL query params were undocumented (visible only in source). This reworks the panel for first-time setup:
Details
groups (Video & Streaming, Tracking & Pose, In-XR Controls & Display, Network, Device Workarounds, Developer & Session). All element IDs unchanged.descriptionto UrlParam and render a collapsed "URL parameters" help list from URL_PARAMS, so it can never drift from the registry; secrets/transport internals stay out by omitting a description.Description
Fixes #(issue)
Type of change
Testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
Improvements
Bug Fixes