fix: dispose deferred split scroll listeners (#3390)

This commit is contained in:
Neil
2026-05-29 17:32:52 -07:00
committed by GitHub
parent 38d418779e
commit ff5903b84a
4 changed files with 76 additions and 12 deletions
@@ -128,6 +128,7 @@ export function createPaneDOM(
ligaturesAddon: null,
compositionHandler: null,
pendingSplitScrollState: null,
pendingSplitScrollBufferDisposable: null,
debugLabel: options.debugLabel ?? null
}
@@ -294,6 +295,12 @@ export function disposePane(
pane.terminal.element?.removeEventListener('compositionstart', pane.compositionHandler, true)
pane.compositionHandler = null
}
try {
pane.pendingSplitScrollBufferDisposable?.dispose()
pane.pendingSplitScrollBufferDisposable = null
} catch {
/* ignore */
}
try {
pane.ligaturesAddon?.dispose()
} catch {
@@ -1,4 +1,4 @@
import type { IMarker, Terminal } from '@xterm/xterm'
import type { IDisposable, IMarker, Terminal } from '@xterm/xterm'
import type { ITerminalOptions } from '@xterm/xterm'
import type { FitAddon } from '@xterm/addon-fit'
import type { LigaturesAddon } from '@xterm/addon-ligatures'
@@ -114,6 +114,9 @@ export type ManagedPaneInternal = {
// Why: splitPane reparents DOM; its delayed restore owns scroll until the
// browser settles, so intermediate fits must not compete with it.
pendingSplitScrollState: ScrollState | null
// Stored so repeated split restores and disposePane() can remove the
// deferred alt-screen buffer listener instead of stacking callbacks.
pendingSplitScrollBufferDisposable?: IDisposable | null
debugLabel: string | null
} & ManagedPane
@@ -26,11 +26,11 @@ const TEST_LEAF_ID = '11111111-1111-4111-8111-111111111111' as TerminalLeafId
function createPane(bufferType: 'normal' | 'alternate'): {
pane: ManagedPaneInternal
bufferChangeDisposable: { dispose: ReturnType<typeof vi.fn> }
triggerBufferChange: (bufferType: 'normal' | 'alternate') => void
bufferChangeDisposables: { dispose: ReturnType<typeof vi.fn> }[]
triggerBufferChange: (bufferType: 'normal' | 'alternate', index?: number) => void
} {
let bufferChangeHandler: ((buffer: { type: 'normal' | 'alternate' }) => void) | null = null
const bufferChangeDisposable = { dispose: vi.fn() }
const bufferChangeHandlers: ((buffer: { type: 'normal' | 'alternate' }) => void)[] = []
const bufferChangeDisposables: { dispose: ReturnType<typeof vi.fn> }[] = []
const pane: ManagedPaneInternal = {
id: 1,
leafId: TEST_LEAF_ID,
@@ -44,8 +44,10 @@ function createPane(bufferType: 'normal' | 'alternate'): {
length: 24
},
onBufferChange: vi.fn((handler: (buffer: { type: 'normal' | 'alternate' }) => void) => {
bufferChangeHandler = handler
return bufferChangeDisposable
const disposable = { dispose: vi.fn() }
bufferChangeHandlers.push(handler)
bufferChangeDisposables.push(disposable)
return disposable
})
}
} as never,
@@ -72,12 +74,14 @@ function createPane(bufferType: 'normal' | 'alternate'): {
webLinksAddon: {} as never,
compositionHandler: null,
pendingSplitScrollState: scrollState,
pendingSplitScrollBufferDisposable: null,
debugLabel: null
}
return {
pane,
bufferChangeDisposable,
triggerBufferChange: (bufferType) => bufferChangeHandler?.({ type: bufferType })
bufferChangeDisposables,
triggerBufferChange: (bufferType, index = bufferChangeHandlers.length - 1) =>
bufferChangeHandlers[index]?.({ type: bufferType })
}
}
@@ -121,7 +125,7 @@ describe('scheduleSplitScrollRestore', () => {
})
it('defers WebGL reattach and skips scroll restore for alternate-screen panes', () => {
const { pane, bufferChangeDisposable, triggerBufferChange } = createPane('alternate')
const { pane, bufferChangeDisposables, triggerBufferChange } = createPane('alternate')
const reattachWebgl = vi.fn()
scheduleSplitScrollRestore(
@@ -139,6 +143,7 @@ describe('scheduleSplitScrollRestore', () => {
vi.advanceTimersByTime(200)
expect(pane.pendingSplitScrollState).toBeNull()
expect(pane.pendingSplitScrollBufferDisposable).toBe(bufferChangeDisposables[0])
expect(reattachWebgl).not.toHaveBeenCalled()
expect(restoreScrollState).not.toHaveBeenCalled()
expect(pane.terminal.refresh).not.toHaveBeenCalled()
@@ -146,11 +151,12 @@ describe('scheduleSplitScrollRestore', () => {
triggerBufferChange('alternate')
expect(reattachWebgl).not.toHaveBeenCalled()
expect(bufferChangeDisposable.dispose).not.toHaveBeenCalled()
expect(bufferChangeDisposables[0].dispose).not.toHaveBeenCalled()
triggerBufferChange('normal')
expect(bufferChangeDisposable.dispose).toHaveBeenCalledTimes(1)
expect(bufferChangeDisposables[0].dispose).toHaveBeenCalledTimes(1)
expect(pane.pendingSplitScrollBufferDisposable).toBeNull()
expect(reattachWebgl).toHaveBeenCalledWith(pane)
expect(restoreScrollState).not.toHaveBeenCalled()
expect(pane.terminal.refresh).not.toHaveBeenCalled()
@@ -181,4 +187,40 @@ describe('scheduleSplitScrollRestore', () => {
expect(restoreScrollState).toHaveBeenCalledWith(pane.terminal, scrollState)
expect(pane.terminal.refresh).toHaveBeenCalledWith(0, 23)
})
it('replaces stale deferred buffer listeners when split restore is rescheduled', () => {
const { pane, bufferChangeDisposables, triggerBufferChange } = createPane('alternate')
const reattachWebgl = vi.fn()
scheduleSplitScrollRestore(
() => pane,
pane.id,
scrollState,
() => false,
reattachWebgl
)
vi.advanceTimersByTime(200)
expect(bufferChangeDisposables).toHaveLength(1)
expect(pane.pendingSplitScrollBufferDisposable).toBe(bufferChangeDisposables[0])
scheduleSplitScrollRestore(
() => pane,
pane.id,
scrollState,
() => false,
reattachWebgl
)
vi.advanceTimersByTime(200)
expect(bufferChangeDisposables).toHaveLength(2)
expect(bufferChangeDisposables[0].dispose).toHaveBeenCalledTimes(1)
expect(pane.pendingSplitScrollBufferDisposable).toBe(bufferChangeDisposables[1])
triggerBufferChange('normal')
expect(bufferChangeDisposables[1].dispose).toHaveBeenCalledTimes(1)
expect(pane.pendingSplitScrollBufferDisposable).toBeNull()
expect(reattachWebgl).toHaveBeenCalledTimes(1)
})
})
@@ -10,6 +10,11 @@ function refreshAfterReparent(pane: ManagedPaneInternal): void {
}
}
function clearPendingSplitScrollBufferDisposable(pane: ManagedPaneInternal): void {
pane.pendingSplitScrollBufferDisposable?.dispose()
pane.pendingSplitScrollBufferDisposable = null
}
function runAfterNormalBuffer(
pane: ManagedPaneInternal,
getPaneById: (id: number) => ManagedPaneInternal | undefined,
@@ -17,11 +22,15 @@ function runAfterNormalBuffer(
isDestroyed: () => boolean,
callback: (pane: ManagedPaneInternal) => void
): void {
clearPendingSplitScrollBufferDisposable(pane)
let disposable: IDisposable | null = null
disposable = pane.terminal.buffer.onBufferChange((buffer: IBuffer) => {
if (buffer.type === 'alternate') {
return
}
if (pane.pendingSplitScrollBufferDisposable === disposable) {
pane.pendingSplitScrollBufferDisposable = null
}
disposable?.dispose()
disposable = null
if (isDestroyed()) {
@@ -32,6 +41,7 @@ function runAfterNormalBuffer(
callback(live)
}
})
pane.pendingSplitScrollBufferDisposable = disposable
}
function restoreCapturedScrollState(
@@ -39,6 +49,7 @@ function restoreCapturedScrollState(
scrollState: ScrollState,
reattachWebgl?: (pane: ManagedPaneInternal) => void
): void {
clearPendingSplitScrollBufferDisposable(pane)
pane.pendingSplitScrollState = null
if (reattachWebgl) {
reattachWebgl(pane)
@@ -104,6 +115,7 @@ export function scheduleSplitScrollRestore(
// alternate buffer. Alt-screen has no scrollback, so scroll restore has
// nothing legitimate to do.
if (scrollState.bufferType === 'alternate') {
clearPendingSplitScrollBufferDisposable(live)
live.pendingSplitScrollState = null
if (live.terminal.buffer.active.type === 'alternate' && reattachWebgl) {
runAfterNormalBuffer(live, getPaneById, paneId, isDestroyed, reattachWebgl)