fix(desktop): fix terminal text corruption when returning from background#748
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
andreasasprou
force-pushed
the
fix/terminal-visibility-rendering-corruption
branch
from
aac331c to
e256ae1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Around line 79-90: The webglAddon.onContextLoss handler leaves renderer
pointing at the disposed WebglAddon when CanvasAddon instantiation fails; update
the catch block in the webglAddon.onContextLoss callback to explicitly set
renderer = null (and ensure usingWebGL is false) when CanvasAddon creation or
xterm.loadAddon fails so subsequent dispose() or checks don't operate on an
already-disposed addon (references: webglAddon.onContextLoss, renderer,
CanvasAddon, xterm.loadAddon, usingWebGL).
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
560-569: Extract the delay constant and consider clearing the timer on unmount.The 50ms delay is a magic number that should be extracted to a named constant per coding guidelines. Additionally, while the
isUnmountedguard prevents work after unmount, storing and clearing the timer reference would be cleaner.Suggested fix
At the top of the file or in a config module:
const FOCUS_REFRESH_DELAY_MS = 50;In the effect:
+ let focusTimeoutId: ReturnType<typeof setTimeout> | null = null; + const handleWindowFocus = () => { if (isUnmounted) return; - setTimeout(() => { + focusTimeoutId = setTimeout(() => { if (isUnmounted) return; fitAddon.fit(); renderer.clearTextureAtlasAndRefresh(); - }, 50); + }, FOCUS_REFRESH_DELAY_MS); };In cleanup:
return () => { isUnmounted = true; + if (focusTimeoutId) clearTimeout(focusTimeoutId); document.removeEventListener("visibilitychange", handleVisibilityChange);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (4)
60-66: LGTM!Clean interface definition with clear JSDoc documentation describing the purpose of each method.
92-115: LGTM!Good implementation. The dual check
usingWebGL && renderer instanceof WebglAddonis defensive but ensures safety if state somehow becomes inconsistent. Always callingxterm.refresh()regardless of renderer type ensures the terminal is updated.
126-131: LGTM!Return type properly extended to expose the renderer handle, enabling callers to access the refresh functionality.
207-215: LGTM!The renderer is correctly included in the return object and properly disposed in the cleanup function.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
325-339: LGTM!Properly destructures the new
rendererhandle fromcreateTerminalInstancefor use in visibility/focus handlers.
551-558: LGTM!Visibility change handler correctly guards against hidden state and unmount, then refreshes the terminal to fix rendering corruption.
571-577: LGTM!Event listeners are correctly registered and cleaned up on unmount.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
andreasasprou
force-pushed
the
fix/terminal-visibility-rendering-corruption
branch
3 times, most recently
from
6bc8937 to
87122c7
Compare
…ound Add visibility change handler to clear WebGL texture atlas and refresh terminal display when the app regains focus. The corruption (garbled/missing characters) was caused by stale glyphs in the WebGL texture atlas when the app was backgrounded and the WebGL context was invalidated. Changes: - Add RendererHandle interface with clearTextureAtlasAndRefresh method - Track WebGL vs Canvas renderer state for targeted atlas clearing - Add visibilitychange handler to refresh on app return - Sync PTY dimensions if resize occurred while backgrounded - Force refresh after WebGL context loss recovery
andreasasprou
force-pushed
the
fix/terminal-visibility-rendering-corruption
branch
from
87122c7 to
fab17ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
60-113: Guardxterm.refresh(0, xterm.rows - 1)forrows === 0and log renderer recovery failuresThe terminal's
rowsproperty can be 0 when hidden or during context loss recovery, which would callrefresh(0, -1)and cause undefined behavior. Additionally, the catch blocks in renderer initialization and recovery silently swallow failures, violating the guideline to never swallow errors silently.Proposed fix
export interface RendererHandle { dispose: () => void; /** Clear WebGL texture atlas and refresh terminal. Call on visibility change to fix rendering corruption. */ clearTextureAtlasAndRefresh: () => void; } function loadRenderer(xterm: XTerm): RendererHandle { let renderer: WebglAddon | CanvasAddon | null = null; let usingWebGL = false; + const refreshAllRows = () => { + if (xterm.rows <= 0) return; + xterm.refresh(0, xterm.rows - 1); + }; try { const webglAddon = new WebglAddon(); webglAddon.onContextLoss(() => { webglAddon.dispose(); usingWebGL = false; try { renderer = new CanvasAddon(); xterm.loadAddon(renderer); // Force refresh after context loss recovery - xterm.refresh(0, xterm.rows - 1); - } catch { + refreshAllRows(); + } catch (error) { // Canvas fallback failed, use default renderer + console.warn( + "[Terminal/loadRenderer] Canvas fallback failed after WebGL context loss", + error, + ); renderer = null; } }); xterm.loadAddon(webglAddon); renderer = webglAddon; usingWebGL = true; - } catch { + } catch (error) { + console.warn("[Terminal/loadRenderer] WebGL renderer init failed", error); try { renderer = new CanvasAddon(); xterm.loadAddon(renderer); - } catch { + } catch (error2) { // Both renderers failed, use default + console.warn( + "[Terminal/loadRenderer] Canvas renderer init failed; falling back to default", + error2, + ); } } return { dispose: () => renderer?.dispose(), clearTextureAtlasAndRefresh: () => { if (usingWebGL && renderer instanceof WebglAddon) { renderer.clearTextureAtlas(); } // Always refresh to ensure display is up-to-date - xterm.refresh(0, xterm.rows - 1); + refreshAllRows(); }, }; }
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
551-603: Move refresh debounce constant to module scope + harden refresh against fit/visibility edge cases
REFRESH_DEBOUNCE_MSis a new magic number; guidelines prefer module-top constants.- Consider wrapping
fitAddon.fit()in try/catch (some Electron/visibility/layout transitions can temporarily produce invalid measurements). If fit fails, you can still attemptrenderer.clearTextureAtlasAndRefresh()to heal rendering.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
325-340: Good wiring ofrendererhandle into the Terminal instance lifecycle
Nice, this keeps the “clear stale WebGL atlas + full refresh” logic encapsulated behindcreateTerminalInstance(...)/RendererHandle, rather than reaching into xterm internals here.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
121-213:RendererHandlepropagation + cleanup integration looks solid
Returningrenderer: RendererHandlefromcreateTerminalInstance(...)and disposing it incleanup()is a clean seam for Terminal.tsx to call into without exposing addon internals.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Summary
Why / Context
Users reported terminal text appearing corrupted with missing/partial characters after switching to another app and returning to Superset. The corruption pattern showed random character drops throughout the display (e.g., "Updated t e t a e de" instead of proper text).
Root Cause Analysis:
After investigation and validation with external reasoning model, we identified the primary cause as WebGL texture atlas corruption:
How It Works
helpers.tschanges:RendererHandleinterface withclearTextureAtlasAndRefresh()methodloadRenderer()to track WebGL vs Canvas staterenderer = nullwhen Canvas fallback fails to avoid stale referenceTerminal.tsxchanges:refreshTerminalDisplay()function that:fitAddon.fit()to handle any resize that occurred while backgroundedvisibilitychangeANDwindow.focushandlers (see design decision below)Design Decisions
Why both
visibilitychangeANDwindow.focushandlers?We need both because they cover different scenarios:
visibilitychangewindow.focusThe problem: In Electron,
visibilitychangemay not fire in the common case of "switch to another app and back" if the window loses focus but the document doesn't become "hidden". This is platform-dependent.The solution: Handle both events with a simple timestamp-based debounce (100ms) to prevent double-refresh when both fire in quick succession.
Why debounce instead of other approaches?
Alternatives considered:
visibilitychange: Misses focus-only cases in Electronwindow.focus: Misses cases where document is hidden but window isn't focusedThe debounce approach is simple, handles all cases, and prevents redundant work.
Manual QA Checklist
Visibility Change
Window Focus
Resize While Backgrounded
Regression
Testing
bun run typecheck✅Assumptions & Caveats
This fix is based on analysis of the symptom (random missing characters) and validation that this pattern matches WebGL texture atlas issues.
Key assumptions:
terminal.refresh(0, rows-1)forces a full redraw from correct buffer dataNote: This fix addresses render corruption only. It won't fix typing-lag or IPC-batching issues (different layer of the stack).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.