fix(desktop): fall back from terminal WebGL on context loss#4612
fix(desktop): fall back from terminal WebGL on context loss#4612Kitenite wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughWhen the WebGL addon detects a context loss event, it now persistently marks the preferred renderer type as ChangesWebGL context loss DOM fallback
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
This reverts commit c11dedb.
|
Closing this after validation showed the automatic texture-atlas clear on terminal attach can itself trigger the garbled rendering issue. The branch now contains a revert and has no net diff; we should not merge this approach. |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — the change is a one-line addition in two symmetric locations that closes an existing gap without touching the PTY, WebSocket, or any React state. The two files receive identical, minimal additions to a well-understood callback. The rest of the onContextLoss cleanup logic (dispose, null-out, refresh) is unchanged. terminal-runtime.ts is untouched and does not contain the texture-atlas or recovery code flagged in earlier review rounds. No regressions are expected. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-addons.ts | Adds suggestedRendererType = "dom" to the onContextLoss handler; closes gap where runtime context loss did not update the module-level renderer flag. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts | Identical single-line fix in the v1-terminal-cache code path's onContextLoss handler; keeps both terminal creation paths in sync. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[requestAnimationFrame fires] --> B{disposed or\nsuggestedRendererType === 'dom'?}
B -- Yes --> C[Skip WebGL, use DOM renderer]
B -- No --> D[new WebglAddon]
D --> E{loadAddon succeeds?}
E -- No / catch --> F[suggestedRendererType = 'dom']
E -- Yes --> G[WebGL active]
G --> H{onContextLoss fires\nat runtime}
H --> I[suggestedRendererType = 'dom'\n NEW in this PR]
I --> J[dispose webglAddon]
J --> K[terminal.refresh — falls back to DOM]
F --> L[Future terminals skip WebGL]
K --> L
Reviews (2): Last reviewed commit: "fix(desktop): fall back from terminal We..." | Re-trigger Greptile
Kitenite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-addons.ts (1)
46-62: ⚡ Quick winConsider consolidating duplicate WebGL loading logic.
The WebGL loading pattern (rAF deferral, context loss handling, fallback flag) is duplicated in
helpers.tslines 128-144. Both files maintain separate module-scopedsuggestedRendererTypevariables, which means a WebGL failure in one code path won't prevent WebGL attempts in the other.If the intent is truly "for all subsequent runtimes" (line 16), consider extracting this logic to a shared module with a single global state variable. Otherwise, clarify in the comment that the fallback is scoped per terminal creation method.
♻️ Potential consolidation approach
Create a shared module (e.g.,
terminal-webgl.ts):// Shared WebGL state and loading logic let suggestedRendererType: "webgl" | "dom" | undefined; export function loadWebGLAddon( terminal: XTerm, onDisposed: () => boolean ): { dispose: () => void } | null { if (suggestedRendererType === "dom") return null; let webglAddon: WebglAddon | null = null; const rafId = requestAnimationFrame(() => { if (onDisposed() || suggestedRendererType === "dom") return; try { webglAddon = new WebglAddon(); webglAddon.onContextLoss(() => { suggestedRendererType = "dom"; webglAddon?.dispose(); webglAddon = null; terminal.refresh(0, terminal.rows - 1); }); terminal.loadAddon(webglAddon); } catch { suggestedRendererType = "dom"; webglAddon = null; } }); return { dispose: () => { cancelAnimationFrame(rafId); try { webglAddon?.dispose(); } catch {} webglAddon = null; } }; }Then both files can call this shared function.
🤖 Prompt for 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. In `@apps/desktop/src/renderer/lib/terminal/terminal-addons.ts` around lines 46 - 62, The WebGL load/fallback logic is duplicated and uses separate module-scoped suggestedRendererType variables; extract the shared behavior into a single module (e.g., terminal-webgl) that exposes a loadWebGLAddon function which accepts the terminal and a disposal predicate; inside it reuse requestAnimationFrame, create WebglAddon, attach onContextLoss to set a single shared suggestedRendererType = "dom", call terminal.loadAddon(webglAddon), and return a dispose() that cancels the rafId and disposes the addon; update terminal-addons.ts and helpers.ts to call this shared loadWebGLAddon instead of duplicating the requestAnimationFrame/WebglAddon/rafId/suggestedRendererType logic.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-addons.ts`:
- Around line 46-62: The WebGL load/fallback logic is duplicated and uses
separate module-scoped suggestedRendererType variables; extract the shared
behavior into a single module (e.g., terminal-webgl) that exposes a
loadWebGLAddon function which accepts the terminal and a disposal predicate;
inside it reuse requestAnimationFrame, create WebglAddon, attach onContextLoss
to set a single shared suggestedRendererType = "dom", call
terminal.loadAddon(webglAddon), and return a dispose() that cancels the rafId
and disposes the addon; update terminal-addons.ts and helpers.ts to call this
shared loadWebGLAddon instead of duplicating the
requestAnimationFrame/WebglAddon/rafId/suggestedRendererType logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bca9982e-978d-4168-ac3c-7da3eb2bfa1d
📒 Files selected for processing (2)
apps/desktop/src/renderer/lib/terminal/terminal-addons.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Closing this after testing the actual WebGL context-loss path. Forcing The added fallback only changed future terminal creation after context loss, but it does not address the garbled glyph case we were trying to prevent. I removed that code so we do not ship an unproven behavior change. |
1 participant
Summary
Why
Clearing xterm's texture atlas can trigger the garbled glyph rendering issue in our app. A safer prevention path is to stop using WebGL after a context loss rather than rebuilding the atlas in place.
Validation
Summary by CodeRabbit