From dc2c74c89b4b5f6cfffae4a78ff4957b1baee8ee Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 6 Jul 2026 22:44:10 -0700 Subject: [PATCH] Deregister Arabic shaping joiner when disposing pane Since xterm.js does not automatically remove registered character joiners upon terminal disposal, they must be explicitly deregistered to avoid memory and registration leaks. - Update `registerArabicShapingJoiner` to return a cleanup function. - Store and call the cleanup handler when disposing a managed pane. - Update tests to assert proper registration and deregistration. --- .../src/lib/pane-manager/pane-dom-creation.ts | 1 + .../src/lib/pane-manager/pane-lifecycle.ts | 9 ++++++++- .../src/lib/pane-manager/pane-manager-types.ts | 3 +++ .../terminal-arabic-shaping-joiner.test.ts | 13 +++++++++++-- .../terminal-arabic-shaping-joiner.ts | 15 ++++++++++----- 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts index 43b628ffbd5..365bfd80178 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts @@ -148,6 +148,7 @@ export function createPaneDOM( compositionHandler: null, focusClassSyncCleanup: null, terminalScrollIntentDisposable: null, + arabicShapingJoinerCleanup: null, pendingSplitScrollState: null, pendingSplitScrollRafIds: [], pendingSplitScrollTimerId: null, diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts index 04d38fe4afa..59583e648c5 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts @@ -76,7 +76,7 @@ export function openTerminal(pane: ManagedPaneInternal): void { // Why: without run-joining, Arabic/Hebrew output renders as disconnected // letters in reversed order (#5262). Registered up front so restored // scrollback and reattach replays shape correctly, not just live output. - registerArabicShapingJoiner(terminal) + pane.arabicShapingJoinerCleanup = registerArabicShapingJoiner(terminal) // Why: the OS reads the focused textarea's screen rect at compositionstart to // decide where to display the IME candidate window. xterm positions that @@ -230,6 +230,13 @@ export function disposePane( pane.focusClassSyncCleanup = null pane.terminalScrollIntentDisposable?.dispose() pane.terminalScrollIntentDisposable = null + // Deregister the RTL shaping joiner: terminal.dispose() below does not. + try { + pane.arabicShapingJoinerCleanup?.() + } catch { + /* ignore */ + } + pane.arabicShapingJoinerCleanup = null if (pane.compositionHandler) { pane.terminal.element?.removeEventListener('compositionstart', pane.compositionHandler) pane.terminal.element?.removeEventListener('compositionupdate', pane.compositionHandler) diff --git a/src/renderer/src/lib/pane-manager/pane-manager-types.ts b/src/renderer/src/lib/pane-manager/pane-manager-types.ts index 1ab542bf787..8218de5e856 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-types.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-types.ts @@ -161,6 +161,9 @@ export type ManagedPaneInternal = { focusClassSyncCleanup?: (() => void) | null // Stored so disposePane() can remove user-scroll intent listeners. terminalScrollIntentDisposable?: IDisposable | null + // Stored so disposePane() can deregister the joiner; terminal.dispose() + // does not remove registered character joiners. + arabicShapingJoinerCleanup?: (() => void) | null // Why: splitPane reparents DOM; its delayed restore owns scroll until the // browser settles, so intermediate fits must not compete with it. pendingSplitScrollState: ScrollState | null diff --git a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts index 871ea94975e..f7921a3890f 100644 --- a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts +++ b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts @@ -122,15 +122,24 @@ describe('findRtlJoinRanges', () => { }) describe('registerArabicShapingJoiner', () => { - it('registers the range finder with the terminal and returns the joiner id', () => { + it('registers the range finder and returns a cleanup that deregisters it', () => { let registered: ((text: string) => [number, number][]) | null = null + let deregistered: number | null = null const terminal = { registerCharacterJoiner(handler: (text: string) => [number, number][]): number { registered = handler return 7 + }, + deregisterCharacterJoiner(joinerId: number): void { + deregistered = joinerId } } - expect(registerArabicShapingJoiner(terminal)).toBe(7) + const cleanup = registerArabicShapingJoiner(terminal) expect(registered).toBe(findRtlJoinRanges) + expect(deregistered).toBeNull() + + // terminal.dispose() does not deregister joiners, so cleanup must. + cleanup() + expect(deregistered).toBe(7) }) }) diff --git a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts index a7f672982fc..26b43ed5080 100644 --- a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts +++ b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts @@ -116,10 +116,15 @@ export function findRtlJoinRanges(text: string): [number, number][] { return ranges } -/** Register the RTL shaping joiner on a terminal. Registration is per - * terminal instance and is torn down with the terminal itself. */ +/** Register the RTL shaping joiner on a terminal. Returns a cleanup that + * deregisters the joiner — `Terminal.dispose()` does not remove registered + * character joiners, so disposePane() must call this to avoid leaking the + * registration (xtermjs/xterm.js#3289). */ export function registerArabicShapingJoiner( - terminal: Pick -): number { - return terminal.registerCharacterJoiner(findRtlJoinRanges) + terminal: Pick +): () => void { + const joinerId = terminal.registerCharacterJoiner(findRtlJoinRanges) + return () => { + terminal.deregisterCharacterJoiner(joinerId) + } }