fallback to canvas renderer if webgl is not available, debug toggle for testing#3081
fallback to canvas renderer if webgl is not available, debug toggle for testing#3081
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes add WebGL vs Canvas renderer selection and UI controls for the terminal. On the frontend, TermWrap exposes renderer state and lifecycle (setTermRenderer, getTermRenderer, isWebGlEnabled) and manages WebGL and Canvas addons and context-loss disposal; TermViewModel gains UI integration (getWebGlIconButton, getTermRenderer, isWebGlEnabled, toggleWebGl) to render a WebGL status/toggle button when the new debug:webglstatus setting is enabled. Type surfaces and schema are extended with the "debug:webglstatus" flag, a Go config constant and SettingsType field are added, and package.json adds the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying waveterm with
|
| Latest commit: |
f23ca8e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f72c83d5.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-webgl.waveterm.pages.dev |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/view/term/termwrap.ts (1)
49-60:⚠️ Potential issue | 🔴 CriticalProbe WebGL2, not WebGL1.
detectWebGLSupport()probesgetContext("webgl"), but@xterm/addon-webglv5.5.0 explicitly requires WebGL2 (getContext("webgl2")). On browsers that expose WebGL1 but not WebGL2, the detection passes and line 188 selects the WebGL renderer, causing the addon to throw "WebGL2 not supported" with no fallback to canvas.Fix
function detectWebGLSupport(): boolean { try { const canvas = document.createElement("canvas"); - const ctx = canvas.getContext("webgl"); + const ctx = canvas.getContext("webgl2"); return !!ctx; } catch (e) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 49 - 60, The WebGL detection currently checks for WebGL1 which causes `@xterm/addon-webgl` v5.5.0 to fail; update detectWebGLSupport() to request a "webgl2" context (e.g., canvas.getContext("webgl2")) and return true only if that succeeds, leaving the try/catch behavior intact; ensure the exported WebGLSupported constant remains tied to this updated detectWebGLSupport() so the code that chooses the renderer (which reads WebGLSupported) will not select the WebGL addon on browsers that only support WebGL1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 49-60: The WebGL detection currently checks for WebGL1 which
causes `@xterm/addon-webgl` v5.5.0 to fail; update detectWebGLSupport() to request
a "webgl2" context (e.g., canvas.getContext("webgl2")) and return true only if
that succeeds, leaving the try/catch behavior intact; ensure the exported
WebGLSupported constant remains tied to this updated detectWebGLSupport() so the
code that chooses the renderer (which reads WebGLSupported) will not select the
WebGL addon on browsers that only support WebGL1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d315c97-251b-4e21-9764-675cc9a06f4a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
frontend/app/view/term/term-model.tsfrontend/app/view/term/termwrap.tsfrontend/types/gotypes.d.tspackage.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.goschema/settings.json
There was a problem hiding this comment.
Code Review Summary
Status: No Issues Found | Recommendation: Merge
Overview
This PR implements a WebGL to Canvas fallback mechanism for the terminal renderer, along with a debug toggle for testing purposes. The implementation is well-structured and handles edge cases properly.
Changes Reviewed
-
termwrap.ts: WebGL detection now correctly checks for WebGL2 context (as required by @xterm/addon-webgl), with proper fallback to Canvas renderer. Added
setTermRenderer,getTermRenderer, andisWebGlEnabledmethods for runtime switching. -
term-model.ts: Added WebGL toggle button functionality accessible via the
debug:webglstatusconfig setting. The button properly reflects WebGL state and allows runtime switching. -
Configuration: Added
debug:webglstatussetting to enable the WebGL status button in the terminal header for testing. -
Dependencies: Added @xterm/addon-canvas as a new dependency.
Key Implementation Details Verified
- WebGL context loss is handled by automatically falling back to Canvas
- Toggle logic correctly switches between WebGL and Canvas renderers
- Debug toggle is properly gated behind the
debug:webglstatussetting - Atom state management for WebGL enabled status is properly initialized
Files Reviewed (7 files)
frontend/app/view/term/term-model.tsfrontend/app/view/term/termwrap.tsfrontend/types/gotypes.d.tspackage.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.goschema/settings.json
No description provided.