From b4f1f0ea6aa59f1330c71e41018b92f8f4bc91f8 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 30 Apr 2026 22:24:03 -0700 Subject: [PATCH] fix(terminal): prevent dead terminal after split via WebGL lifecycle and worktree dedup (#1298) Co-authored-by: Orca --- .../src/lib/pane-manager/pane-manager.ts | 118 ++++++------------ .../src/lib/pane-manager/pane-split-scroll.ts | 87 +++++++++++-- .../src/lib/pane-manager/pane-tree-ops.ts | 51 +++----- src/renderer/src/store/selectors.ts | 8 +- src/renderer/src/store/slices/worktrees.ts | 23 ++-- 5 files changed, 152 insertions(+), 135 deletions(-) diff --git a/src/renderer/src/lib/pane-manager/pane-manager.ts b/src/renderer/src/lib/pane-manager/pane-manager.ts index bf70c770f..182e4170f 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager.ts @@ -41,6 +41,12 @@ import { toPublicPane } from './pane-public-view' export type { PaneManagerOptions, PaneStyleOptions, ManagedPane, DropZone } +function reattachWebglIfNeeded(pane: ManagedPaneInternal): void { + if (pane.gpuRenderingEnabled && !pane.webglAddon && !pane.webglDisabledAfterContextLoss) { + attachWebgl(pane) + } +} + export class PaneManager { private root: HTMLElement private panes: Map = new Map() @@ -60,25 +66,16 @@ export class PaneManager { this.renderingSuspended = options.initialRenderingSuspended === true } - // ----------------------------------------------------------------------- - // Public API - // ----------------------------------------------------------------------- - createInitialPane(opts?: { focus?: boolean }): ManagedPane { const pane = this.createPaneInternal() - - // When the pane is the sole child of root (no splits), it must - // fill the root container so FitAddon calculates correct dimensions. - pane.container.style.width = '100%' - pane.container.style.height = '100%' - pane.container.style.position = 'relative' - pane.container.style.overflow = 'hidden' - - // Place directly into root + Object.assign(pane.container.style, { + width: '100%', + height: '100%', + position: 'relative', + overflow: 'hidden' + }) this.root.appendChild(pane.container) - openTerminal(pane) - this.activePaneId = pane.id applyPaneOpacity(this.panes.values(), this.activePaneId, this.styleOptions) @@ -108,43 +105,40 @@ export class PaneManager { const isVertical = direction === 'vertical' const divider = this.createDividerWrapped(isVertical) - // Why: wrapInSplit reparents the existing container, which causes the - // browser to asynchronously reset scrollTop to 0 during layout. Capture - // the scroll state before reparenting so we can restore it after all - // layout and reflow have settled. + // Why: wrapInSplit reparents the existing container, resetting scrollTop. const scrollState = captureScrollState(existing.terminal) - - // Why: multiple async operations fire after the split (rAFs from - // queueResizeAll, WebGL context loss, ResizeObserver 150ms debounce). - // Each would independently try to restore scroll, potentially to wrong - // positions due to intermediate buffer states. The lock makes safeFit - // and fitAllPanesInternal skip their own scroll restoration, leaving - // the authoritative restore to the timeout below. + // Why: lock prevents safeFit/fitAllPanes from restoring scroll during + // the async settle window — scheduleSplitScrollRestore owns the restore. existing.pendingSplitScrollState = scrollState + // Why: DOM reparenting can silently invalidate a WebGL context without + // firing contextlost — Chromium reclaims the oldest context near its + // ~8–16 limit. Dispose before the move, reattach in the 200ms timer. + const hadWebgl = !!existing.webglAddon + disposeWebgl(existing) + wrapInSplit(existing.container, newPane.container, isVertical, divider, opts) openTerminal(newPane) this.activePaneId = newPane.id applyPaneOpacity(this.panes.values(), this.activePaneId, this.styleOptions) - this.applyDividerStylesWrapped() + applyDividerStyles(this.root, this.styleOptions) newPane.terminal?.focus() updateMultiPaneState(this.getDragCallbacks()) - // Why: forward the caller's spawn hint so onPaneCreated → connectPanePty - // can boot the new PTY in the source pane's live cwd instead of the - // worktree root. The hint is synchronous-only: splitPane returns after - // onPaneCreated runs, so there is no reason for it to outlive this call. + // Why: forward cwd hint so the new PTY spawns in the source pane's cwd. void this.options.onPaneCreated?.( toPublicPane(newPane), opts?.cwd ? { cwd: opts.cwd } : undefined ) this.options.onLayoutChanged?.() + const reattach = hadWebgl ? reattachWebglIfNeeded : undefined scheduleSplitScrollRestore( (id) => this.panes.get(id), existing.id, scrollState, - () => this.destroyed + () => this.destroyed, + reattach ) return toPublicPane(newPane) @@ -171,13 +165,9 @@ export class PaneManager { paneContainer.remove() } if (this.activePaneId === paneId) { - const remaining = Array.from(this.panes.values()) - if (remaining.length > 0) { - this.activePaneId = remaining[0].id - remaining[0].terminal.focus() - } else { - this.activePaneId = null - } + const next = this.panes.values().next().value as ManagedPaneInternal | undefined + this.activePaneId = next?.id ?? null + next?.terminal.focus() } applyPaneOpacity(this.panes.values(), this.activePaneId, this.styleOptions) for (const p of this.panes.values()) { @@ -225,14 +215,10 @@ export class PaneManager { setPaneStyleOptions(opts: PaneStyleOptions): void { this.styleOptions = { ...opts } applyPaneOpacity(this.panes.values(), this.activePaneId, this.styleOptions) - this.applyDividerStylesWrapped() + applyDividerStyles(this.root, this.styleOptions) applyRootBackground(this.root, this.styleOptions) } - /** Enable or disable programming-ligatures rendering on a single pane. - * Called by applyTerminalAppearance whenever the resolved ligatures state - * changes, so toggling the setting or switching fonts takes effect on - * live panes without restarting. */ setPaneLigaturesEnabled(paneId: number, enabled: boolean): void { const pane = this.panes.get(paneId) if (!pane) { @@ -272,25 +258,18 @@ export class PaneManager { this.renderingSuspended = false for (const pane of this.panes.values()) { pane.webglAttachmentDeferred = false - if (pane.gpuRenderingEnabled && !pane.webglDisabledAfterContextLoss && !pane.webglAddon) { - attachWebgl(pane) - // Why: the fitPanes() optimization skips panes whose dimensions are - // unchanged (common when a worktree goes hidden→visible at the same - // window size). But the fresh WebGL canvas created by attachWebgl() - // has no painted content — without an explicit refresh the terminal - // appears frozen until something forces a dimension change (e.g. a - // split). This mirrors the onContextLoss handler in attachWebgl which - // calls the same refresh after falling back to the DOM renderer. + reattachWebglIfNeeded(pane) + // Why: fresh WebGL canvas has no content — refresh prevents frozen terminal. + if (pane.webglAddon) { try { pane.terminal.refresh(0, pane.terminal.rows - 1) } catch { - /* ignore — pane may not be fully initialised yet */ + /* ignore */ } } } } - /** Move a pane from its current position to a new position relative to a target pane. */ movePane(sourcePaneId: number, targetPaneId: number, zone: DropZone): void { handlePaneDrop(sourcePaneId, targetPaneId, zone, this.dragState, this.getDragCallbacks()) } @@ -305,10 +284,6 @@ export class PaneManager { this.activePaneId = null } - // ----------------------------------------------------------------------- - // Internal helpers - // ----------------------------------------------------------------------- - private createPaneInternal(): ManagedPaneInternal { const id = this.nextPaneId++ const pane = createPaneDOM( @@ -316,14 +291,10 @@ export class PaneManager { this.options, this.dragState, this.getDragCallbacks(), + // Why: always re-focus even if already active — after splits the + // browser's real textarea focus can lag the manager's activePaneId. (paneId) => { if (!this.destroyed) { - // Why: split-pane layout/focus callbacks can leave the manager's - // activePaneId temporarily in sync while the browser's real focused - // xterm textarea is still on a different pane. Clicking a pane must - // always re-focus its terminal, even if the manager already thinks - // that pane is active; otherwise input can keep going to the wrong - // split after vertical/horizontal splits. this.setActivePane(paneId, { focus: true }) } }, @@ -336,16 +307,6 @@ export class PaneManager { return pane } - /** - * Focus-follows-mouse entry point. Collects gate inputs from the manager - * and delegates to the pure gate helper. - * - * Invariant for future contributors: modal overlays (context menus, close - * dialogs, command palette) must be rendered as portals/siblings OUTSIDE - * the pane container. If a future overlay is ever rendered inside a .pane - * element, mouseenter will still fire on the pane underneath and this - * handler will incorrectly switch focus. Keep overlays out of the pane. - */ private handlePaneMouseEnter(paneId: number, event: MouseEvent): void { if ( shouldFollowMouseFocus({ @@ -368,11 +329,6 @@ export class PaneManager { }) } - private applyDividerStylesWrapped(): void { - applyDividerStyles(this.root, this.styleOptions) - } - - /** Build the callbacks object for drag-reorder functions. */ private getDragCallbacks() { return { getPanes: () => this.panes, @@ -382,7 +338,7 @@ export class PaneManager { safeFit: (pane: ManagedPaneInternal) => safeFit(pane), applyPaneOpacity: () => applyPaneOpacity(this.panes.values(), this.activePaneId, this.styleOptions), - applyDividerStyles: () => this.applyDividerStylesWrapped(), + applyDividerStyles: () => applyDividerStyles(this.root, this.styleOptions), refitPanesUnder: (el: HTMLElement) => refitPanesUnder(el, this.panes), onLayoutChanged: this.options.onLayoutChanged } diff --git a/src/renderer/src/lib/pane-manager/pane-split-scroll.ts b/src/renderer/src/lib/pane-manager/pane-split-scroll.ts index f33637845..ac102135d 100644 --- a/src/renderer/src/lib/pane-manager/pane-split-scroll.ts +++ b/src/renderer/src/lib/pane-manager/pane-split-scroll.ts @@ -1,13 +1,6 @@ import type { ManagedPaneInternal, ScrollState } from './pane-manager-types' import { restoreScrollState } from './pane-scroll' -// Why: wrapInSplit reparents the existing pane's container, which briefly -// detaches the WebGL canvas from the DOM. The WebGL renderer's internal -// render state can become stale after the re-attachment, leaving the canvas -// blank even though the terminal buffer has data. This mirrors the explicit -// refresh in resumeRendering() (pane-manager.ts) and the onContextLoss -// handler (pane-lifecycle.ts) which address the same "frozen terminal" -// symptom for analogous WebGL state transitions. function refreshAfterReparent(pane: ManagedPaneInternal): void { try { pane.terminal.refresh(0, pane.terminal.rows - 1) @@ -16,15 +9,79 @@ function refreshAfterReparent(pane: ManagedPaneInternal): void { } } +function logPaneHealth(pane: ManagedPaneInternal, phase: string): void { + const canvases = pane.container.querySelectorAll('canvas') + const canvasInfo = Array.from(canvases).map((c) => { + const gl = c.getContext('webgl2') ?? c.getContext('webgl') + return { + w: c.width, + h: c.height, + inDOM: c.isConnected, + ctxLost: gl ? gl.isContextLost() : 'no-ctx' + } + }) + const content = pane.serializeAddon?.serialize?.() ?? '' + // oxlint-disable-next-line no-control-regex + const stripped = content.replace(/[\s\x00-\x1f]/g, '') + const info = { + phase, + paneId: pane.id, + webgl: !!pane.webglAddon, + webglDeferred: pane.webglAttachmentDeferred, + webglDisabled: pane.webglDisabledAfterContextLoss, + canvases: canvasInfo, + contentLen: stripped.length, + bufferLines: pane.terminal.buffer.active.length + } + const hasBufferData = pane.terminal.buffer.active.length > pane.terminal.rows + if (stripped.length === 0 && hasBufferData) { + console.error( + '[split-diag] DEAD TERMINAL — pane', + pane.id, + pane.debugLabel ?? '', + 'has buffer data but no rendered content at', + phase, + info + ) + } else if (stripped.length === 0) { + console.log( + '[split-diag] pane', + pane.id, + pane.debugLabel ?? '', + 'no content yet at', + phase, + '(PTY likely still spawning)' + ) + } else { + console.log( + '[split-diag] pane', + pane.id, + pane.debugLabel ?? '', + 'healthy at', + phase, + '— content:', + stripped.length + ) + } +} + // Why: reparenting a terminal container during split resets the viewport // scroll position (browser clears scrollTop on DOM move). This schedules a // two-phase restore: an early double-rAF (~32ms) to minimise the visible // flash, plus a 200ms authoritative restore that also clears the scroll lock. +// +// The optional reattachWebgl callback re-creates the WebGL addon after the +// DOM has settled. splitPane() disposes WebGL before wrapInSplit() to free +// the GPU context slot (Chromium silently kills the oldest context when +// approaching its limit without firing contextlost). Reattaching at 200ms +// — after all layout and reflow have completed — creates a fresh context on +// a stable DOM tree. export function scheduleSplitScrollRestore( getPaneById: (id: number) => ManagedPaneInternal | undefined, paneId: number, scrollState: ScrollState, - isDestroyed: () => boolean + isDestroyed: () => boolean, + reattachWebgl?: (pane: ManagedPaneInternal) => void ): void { requestAnimationFrame(() => { requestAnimationFrame(() => { @@ -48,7 +105,21 @@ export function scheduleSplitScrollRestore( return } live.pendingSplitScrollState = null + if (reattachWebgl) { + reattachWebgl(live) + } restoreScrollState(live.terminal, scrollState) refreshAfterReparent(live) }, 200) + + setTimeout(() => { + if (isDestroyed()) { + return + } + const live = getPaneById(paneId) + // Skip suspended panes — they have no WebGL/content by design. + if (live && !live.webglAttachmentDeferred) { + logPaneHealth(live, '1s-health-check') + } + }, 1000) } diff --git a/src/renderer/src/lib/pane-manager/pane-tree-ops.ts b/src/renderer/src/lib/pane-manager/pane-tree-ops.ts index 31b37b8c3..71f152854 100644 --- a/src/renderer/src/lib/pane-manager/pane-tree-ops.ts +++ b/src/renderer/src/lib/pane-manager/pane-tree-ops.ts @@ -1,5 +1,6 @@ import type { DropZone, ManagedPaneInternal, PaneStyleOptions } from './pane-manager-types' import { createDivider } from './pane-divider' +import { disposeWebgl, attachWebgl } from './pane-lifecycle' export { findLineByContent, captureScrollState, restoreScrollState } from './pane-scroll' @@ -40,29 +41,6 @@ export function safeFit(pane: ManagedPaneInternal): void { // Why: divider drags fire refits every frame, but most frames do not // cross a cell boundary. Skipping those avoids FitAddon.clear()+refresh() // churn, which was causing visible terminal blinking while resizing. - // - // Why: wrapInSplit() reparents the pane's container, which can leave - // the WebGL canvas stale even when proposed dimensions match current - // (the browser detaches and reattaches the canvas during the DOM move). - // When pendingSplitScrollState is set we must force a fit + refresh so - // the WebGL renderer repaints. Without this, the pane appears blank - // until something forces a dimension change. - if (pane.pendingSplitScrollState) { - console.warn( - '[terminal] safeFit forcing fit+refresh during pending split for pane', - pane.id, - `— dims ${dims.cols}×${dims.rows} match current, webgl:`, - !!pane.webglAddon, - pane.debugLabel ? `(${pane.debugLabel})` : '' - ) - pane.fitAddon.fit() - try { - pane.terminal.refresh(0, pane.terminal.rows - 1) - } catch { - /* ignore — terminal may not be fully initialised */ - } - return - } return } pane.fitAddon.fit() @@ -183,6 +161,13 @@ export function insertPaneNextTo( applyPaneFlexStyle(source.container) applyPaneFlexStyle(targetContainer) + // Why: same pattern as splitPane — dispose WebGL before the DOM reparent + // to free GPU context slots, then reattach after layout settles. + const sourceHadWebgl = !!source.webglAddon + const targetHadWebgl = !!target.webglAddon + disposeWebgl(source) + disposeWebgl(target) + // Replace target with the split in the DOM parent.replaceChild(split, targetContainer) @@ -197,23 +182,15 @@ export function insertPaneNextTo( split.appendChild(source.container) } - // Refit both and refresh rendering surfaces — both panes were reparented - // into the new split wrapper, which can leave the WebGL canvas in a stale - // state (same mechanism as wrapInSplit; see refreshAfterReparent in - // pane-split-scroll.ts). requestAnimationFrame(() => { + if (sourceHadWebgl && source.gpuRenderingEnabled && !source.webglDisabledAfterContextLoss) { + attachWebgl(source) + } + if (targetHadWebgl && target.gpuRenderingEnabled && !target.webglDisabledAfterContextLoss) { + attachWebgl(target) + } callbacks.safeFit(source) callbacks.safeFit(target) - try { - source.terminal.refresh(0, source.terminal.rows - 1) - } catch { - /* ignore */ - } - try { - target.terminal.refresh(0, target.terminal.rows - 1) - } catch { - /* ignore */ - } }) } diff --git a/src/renderer/src/store/selectors.ts b/src/renderer/src/store/selectors.ts index da410c0dd..d188fe901 100644 --- a/src/renderer/src/store/selectors.ts +++ b/src/renderer/src/store/selectors.ts @@ -23,11 +23,15 @@ function getWorktreeSnapshot(worktreesByRepo: AppState['worktreesByRepo']): Work return cachedSnapshot } - const allWorktrees = Object.values(worktreesByRepo).flat() + // Why: a race between createWorktree (which appends) and fetchWorktrees + // (which replaces) can produce duplicate entries for the same worktree ID + // within a single repo's array. Deduplicating here prevents React from + // seeing duplicate keys, which can corrupt terminal DOM containers. const worktreeMap = new Map() - for (const worktree of allWorktrees) { + for (const worktree of Object.values(worktreesByRepo).flat()) { worktreeMap.set(worktree.id, worktree) } + const allWorktrees = Array.from(worktreeMap.values()) const snapshot = { allWorktrees, worktreeMap } worktreeSnapshotCache.set(worktreesByRepo, snapshot) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 55d9cae08..8a47ddaa3 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -105,13 +105,22 @@ export const createWorktreeSlice: StateCreator baseBranch, setupDecision }) - set((s) => ({ - worktreesByRepo: { - ...s.worktreesByRepo, - [repoId]: [...(s.worktreesByRepo[repoId] ?? []), result.worktree] - }, - sortEpoch: s.sortEpoch + 1 - })) + // Why: a file watcher (worktrees.onChanged) can fire between the + // backend creating the worktree and this callback running, causing + // fetchWorktrees to add the worktree first. Appending unconditionally + // then produces a duplicate entry in worktreesByRepo, which gives + // React duplicate keys and can corrupt terminal DOM containers. + set((s) => { + const current = s.worktreesByRepo[repoId] ?? [] + const alreadyPresent = current.some((w) => w.id === result.worktree.id) + return { + worktreesByRepo: { + ...s.worktreesByRepo, + [repoId]: alreadyPresent ? current : [...current, result.worktree] + }, + sortEpoch: s.sortEpoch + 1 + } + }) return result } catch (error) { const message = error instanceof Error ? error.message : String(error)