From cf43dd1a8883a60b417ee035b200d2c41ab2bf7a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:35:22 -0700 Subject: [PATCH] Cancel pending cursor updates when a terminal closes (#24566) Reuse the existing pane frame tracker to cancel owned deferred focus-class updates during existing cleanup, with disposed guards against late or reentrant delivery. --- .../pane-container-listener-lifecycle.test.ts | 28 +++ ...ane-dom-focus-class-sync-lifecycle.test.ts | 210 ++++++++++++++++++ .../pane-manager/pane-dom-focus-class-sync.ts | 14 +- 3 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync-lifecycle.test.ts diff --git a/src/renderer/src/lib/pane-manager/pane-container-listener-lifecycle.test.ts b/src/renderer/src/lib/pane-manager/pane-container-listener-lifecycle.test.ts index 99147c065f6..ba80462ac87 100644 --- a/src/renderer/src/lib/pane-manager/pane-container-listener-lifecycle.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-container-listener-lifecycle.test.ts @@ -1,6 +1,8 @@ +// @vitest-environment happy-dom import { describe, expect, it, vi } from 'vitest' import type { ManagedPaneInternal } from './pane-manager-types' import { disposePane } from './pane-lifecycle' +import { attachDomRendererFocusClassSync } from './pane-dom-focus-class-sync' function makePane(): ManagedPaneInternal { const leafId = '11111111-1111-4111-8111-111111111111' as never @@ -36,6 +38,32 @@ function makePane(): ManagedPaneInternal { } describe('disposePane container listener cleanup', () => { + it('cancels focus sync frames before disposing the terminal and removing its pane', () => { + const pending = new Map() + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + pending.set(1, callback) + return 1 + }) + vi.stubGlobal('cancelAnimationFrame', (id: number) => pending.delete(id)) + try { + const pane = makePane() + pane.focusClassSyncCleanup = attachDomRendererFocusClassSync(document.createElement('div')) + const panes = new Map([[pane.id, pane]]) + let pendingAtDispose: number | undefined + vi.mocked(pane.terminal.dispose).mockImplementation(() => { + pendingAtDispose = pending.size + }) + expect(pending.size).toBe(1) + disposePane(pane, panes) + expect(pendingAtDispose).toBe(0) + expect(pane.terminal.dispose).toHaveBeenCalledOnce() + expect(pane.focusClassSyncCleanup).toBeNull() + expect(panes.has(pane.id)).toBe(false) + } finally { + vi.unstubAllGlobals() + } + }) + it('removes pane container focus listeners', () => { const pane = makePane() const pointerDownHandler = pane.panePointerDownHandler diff --git a/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync-lifecycle.test.ts b/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync-lifecycle.test.ts new file mode 100644 index 00000000000..c52ec59fa3a --- /dev/null +++ b/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync-lifecycle.test.ts @@ -0,0 +1,210 @@ +// @vitest-environment happy-dom +import { afterEach, describe, expect, it, vi } from 'vitest' +import { attachDomRendererFocusClassSync } from './pane-dom-focus-class-sync' + +function frameQueue() { + let nextId = 0 + const pending = new Map() + const cancel = vi.fn((id: number) => pending.delete(id)) + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + const id = ++nextId + pending.set(id, callback) + return id + }) + vi.stubGlobal('cancelAnimationFrame', cancel) + return { + pending, + cancel, + flush: () => { + const scheduled = Array.from(pending) + for (const [id, callback] of scheduled) { + pending.delete(id) + callback(16) + } + } + } +} + +function captureObserver(): () => void { + let notify = (): void => {} + vi.stubGlobal( + 'MutationObserver', + class { + constructor(callback: MutationCallback) { + notify = () => callback([], this) + } + observe(): void {} + disconnect(): void {} + takeRecords(): MutationRecord[] { + return [] + } + } + ) + return () => notify() +} + +function terminalElement(): HTMLDivElement { + const element = document.createElement('div') + element.innerHTML = '
' + return element +} + +function disposedTargets(count: number): WeakRef[] { + const targets: WeakRef[] = [] + for (let index = 0; index < count; index += 1) { + const element = terminalElement() + const release = attachDomRendererFocusClassSync(element) + targets.push(new WeakRef(element)) + release() + } + return targets +} + +afterEach(() => { + vi.restoreAllMocks() + vi.unstubAllGlobals() +}) + +describe('terminal DOM focus sync lifetime', () => { + it('releases every disposed terminal DOM target while animation frames remain suspended', async () => { + const frames = frameQueue() + const targets = disposedTargets(64) + if (typeof globalThis.gc !== 'function') { + throw new Error('The test runner must enable --expose-gc') + } + await new Promise((resolve) => setImmediate(resolve)) + globalThis.gc() + globalThis.gc() + expect({ + callbacks: frames.pending.size, + targets: targets.filter((target) => target.deref() !== undefined).length + }).toEqual({ callbacks: 0, targets: 0 }) + }) + + it('keeps every immediate and deferred live sync in the same order without coalescing', () => { + const frames = frameQueue() + const notify = captureObserver() + const element = terminalElement() + const rows = element.firstElementChild + if (!rows) { + throw new Error('missing terminal rows') + } + const toggle = vi.spyOn(rows.classList, 'toggle') + const release = attachDomRendererFocusClassSync(element) + try { + element.classList.add('focus') + element.dispatchEvent(new Event('focusin')) + element.classList.remove('focus') + notify() + element.classList.add('focus') + element.dispatchEvent(new Event('focusout')) + expect(frames.pending.size).toBe(4) + frames.flush() + expect(toggle.mock.calls).toEqual( + [false, true, false, true, true, true, true, true].map((focused) => [ + 'xterm-focus', + focused + ]) + ) + expect(frames.pending.size).toBe(0) + release() + expect(frames.cancel).not.toHaveBeenCalled() + } finally { + release() + } + }) + + it('ignores callbacks and already queued listener delivery after disposal', () => { + const frames = frameQueue() + const notify = captureObserver() + const element = terminalElement() + const add = vi.spyOn(element, 'addEventListener') + const query = vi.spyOn(element, 'querySelector') + const release = attachDomRendererFocusClassSync(element) + const queued = [...frames.pending.values()] + release() + query.mockClear() + for (const callback of queued) { + callback(16) + } + for (const [, listener] of add.mock.calls) { + if (typeof listener === 'function') { + listener.call(element, new Event('focusin')) + } + } + notify() + element.dispatchEvent(new Event('focusin')) + expect(query).not.toHaveBeenCalled() + expect(frames.pending.size).toBe(0) + expect(frames.cancel).toHaveBeenCalledOnce() + }) + + it('does not queue another frame when the immediate sync disposes its own owner', () => { + const frames = frameQueue() + captureObserver() + const element = terminalElement() + const release = attachDomRendererFocusClassSync(element) + const query = element.querySelector.bind(element) + vi.spyOn(element, 'querySelector').mockImplementation((selector) => { + release() + return query(selector) + }) + element.dispatchEvent(new Event('focusin')) + expect(frames.pending.size).toBe(0) + expect(frames.cancel).toHaveBeenCalledOnce() + }) + + it('cancels only the old attachment when the same terminal element gets a successor', () => { + const frames = frameQueue() + captureObserver() + const element = terminalElement() + const oldRelease = attachDomRendererFocusClassSync(element) + const oldFrames = [...frames.pending.values()] + const release = attachDomRendererFocusClassSync(element) + try { + oldRelease() + expect(frames.pending.size).toBe(1) + for (const callback of oldFrames) { + callback(16) + } + element.classList.add('focus') + element.dispatchEvent(new Event('focusin')) + expect(frames.pending.size).toBe(2) + frames.flush() + expect(element.firstElementChild?.classList.contains('xterm-focus')).toBe(true) + release() + expect(frames.cancel).toHaveBeenCalledOnce() + } finally { + oldRelease() + release() + } + }) + + it('keeps synchronously completed frame shims out of the pending set', () => { + const cancel = vi.fn() + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + callback(16) + return 1 + }) + vi.stubGlobal('cancelAnimationFrame', cancel) + const element = terminalElement() + const query = vi.spyOn(element, 'querySelector') + const release = attachDomRendererFocusClassSync(element) + expect(query).toHaveBeenCalledTimes(2) + release() + expect(cancel).not.toHaveBeenCalled() + }) + + it('keeps missing elements and rows safe and releases repeated cleanup', () => { + const frames = frameQueue() + const missingRelease = attachDomRendererFocusClassSync(undefined) + missingRelease() + expect(frames.pending.size).toBe(0) + const release = attachDomRendererFocusClassSync(document.createElement('div')) + frames.flush() + release() + release() + expect(frames.pending.size).toBe(0) + expect(frames.cancel).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync.ts b/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync.ts index 137b9346ae8..c6540a9b8b6 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-focus-class-sync.ts @@ -1,3 +1,5 @@ +import { PaneReparentFrameTracker } from './pane-reparent-frame-tracker' + export function attachDomRendererFocusClassSync( terminalElement: HTMLElement | undefined ): () => void { @@ -5,6 +7,9 @@ export function attachDomRendererFocusClassSync( return () => undefined } + let disposed = false + const frames = new PaneReparentFrameTracker(() => disposed) + const sync = (): void => { const rows = terminalElement.querySelector('.xterm-rows') if (!rows) { @@ -16,8 +21,13 @@ export function attachDomRendererFocusClassSync( } const scheduleSync = (): void => { + if (disposed) { + return + } sync() - requestAnimationFrame(sync) + if (!disposed) { + frames.request(sync) + } } const observer = new MutationObserver(scheduleSync) @@ -27,6 +37,8 @@ export function attachDomRendererFocusClassSync( scheduleSync() return () => { + disposed = true + frames.cancelPending() observer.disconnect() terminalElement.removeEventListener('focusin', scheduleSync) terminalElement.removeEventListener('focusout', scheduleSync)