* fix: address pr-bug-scan validated finding from #1857 Restored alt-buffer guards at both the double-rAF and 200ms phases of scheduleSplitScrollRestore so TUI panes skip restoreScrollState+refresh, blocking the #1298 cursor-drift regression. * fix: preserve split scroll state around alternate buffers --------- Co-authored-by: orca-bug-scan-bot <orca-bug-scan-bot@stably.ai> Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
This commit is contained in:
parent
b7a00aaf0e
commit
a9b47759fa
|
|
@ -62,6 +62,7 @@ function createPane(): ManagedPaneInternal {
|
|||
compositionHandler: null,
|
||||
debugLabel: null,
|
||||
pendingSplitScrollState: {
|
||||
bufferType: 'normal',
|
||||
wasAtBottom: true,
|
||||
firstVisibleLineContent: '',
|
||||
viewportY: 0,
|
||||
|
|
|
|||
|
|
@ -67,6 +67,7 @@ export type ManagedPane = {
|
|||
// ---------------------------------------------------------------------------
|
||||
|
||||
export type ScrollState = {
|
||||
bufferType: 'normal' | 'alternate'
|
||||
wasAtBottom: boolean
|
||||
firstVisibleLineContent: string
|
||||
viewportY: number
|
||||
|
|
|
|||
|
|
@ -51,11 +51,12 @@ export function findLineByContent(terminal: Terminal, content: string, hintRatio
|
|||
|
||||
export function captureScrollState(terminal: Terminal): ScrollState {
|
||||
const buf = terminal.buffer.active
|
||||
const bufferType = buf.type
|
||||
const viewportY = buf.viewportY
|
||||
const wasAtBottom = viewportY >= buf.baseY
|
||||
const firstVisibleLineContent = buf.getLine(viewportY)?.translateToString(true)?.trimEnd() ?? ''
|
||||
const totalLines = buf.baseY + terminal.rows
|
||||
return { wasAtBottom, firstVisibleLineContent, viewportY, totalLines }
|
||||
return { bufferType, wasAtBottom, firstVisibleLineContent, viewportY, totalLines }
|
||||
}
|
||||
|
||||
export function restoreScrollState(terminal: Terminal, state: ScrollState): void {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,180 @@
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { ManagedPaneInternal, ScrollState } from './pane-manager-types'
|
||||
|
||||
const restoreScrollState = vi.hoisted(() => vi.fn())
|
||||
|
||||
vi.mock('./pane-scroll', () => ({
|
||||
restoreScrollState
|
||||
}))
|
||||
|
||||
import { scheduleSplitScrollRestore } from './pane-split-scroll'
|
||||
|
||||
const scrollState = {
|
||||
bufferType: 'normal',
|
||||
wasAtBottom: true,
|
||||
firstVisibleLineContent: '',
|
||||
viewportY: 0,
|
||||
totalLines: 24
|
||||
} satisfies ScrollState
|
||||
|
||||
const alternateScrollState = {
|
||||
...scrollState,
|
||||
bufferType: 'alternate'
|
||||
} satisfies ScrollState
|
||||
|
||||
function createPane(bufferType: 'normal' | 'alternate'): {
|
||||
pane: ManagedPaneInternal
|
||||
bufferChangeDisposable: { dispose: ReturnType<typeof vi.fn> }
|
||||
triggerBufferChange: (bufferType: 'normal' | 'alternate') => void
|
||||
} {
|
||||
let bufferChangeHandler: ((buffer: { type: 'normal' | 'alternate' }) => void) | null = null
|
||||
const bufferChangeDisposable = { dispose: vi.fn() }
|
||||
const pane: ManagedPaneInternal = {
|
||||
id: 1,
|
||||
terminal: {
|
||||
rows: 24,
|
||||
refresh: vi.fn(),
|
||||
buffer: {
|
||||
active: {
|
||||
type: bufferType,
|
||||
length: 24
|
||||
},
|
||||
onBufferChange: vi.fn((handler: (buffer: { type: 'normal' | 'alternate' }) => void) => {
|
||||
bufferChangeHandler = handler
|
||||
return bufferChangeDisposable
|
||||
})
|
||||
}
|
||||
} as never,
|
||||
container: {
|
||||
querySelectorAll: vi.fn(() => [])
|
||||
} as never,
|
||||
xtermContainer: {} as never,
|
||||
linkTooltip: {} as never,
|
||||
terminalGpuAcceleration: 'auto',
|
||||
gpuRenderingEnabled: true,
|
||||
webglAttachmentDeferred: false,
|
||||
webglDisabledAfterContextLoss: false,
|
||||
hasComplexScriptOutput: false,
|
||||
webglAddon: null,
|
||||
ligaturesAddon: null,
|
||||
fitResizeObserver: null,
|
||||
pendingObservedFitRafId: null,
|
||||
fitAddon: {} as never,
|
||||
searchAddon: {} as never,
|
||||
serializeAddon: {
|
||||
serialize: vi.fn(() => '')
|
||||
} as never,
|
||||
unicode11Addon: {} as never,
|
||||
webLinksAddon: {} as never,
|
||||
compositionHandler: null,
|
||||
pendingSplitScrollState: scrollState,
|
||||
debugLabel: null
|
||||
}
|
||||
return {
|
||||
pane,
|
||||
bufferChangeDisposable,
|
||||
triggerBufferChange: (bufferType) => bufferChangeHandler?.({ type: bufferType })
|
||||
}
|
||||
}
|
||||
|
||||
describe('scheduleSplitScrollRestore', () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => {
|
||||
callback(16)
|
||||
return 1
|
||||
})
|
||||
restoreScrollState.mockClear()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.clearAllTimers()
|
||||
vi.useRealTimers()
|
||||
vi.unstubAllGlobals()
|
||||
})
|
||||
|
||||
it('restores and refreshes normal-screen panes after split reparenting settles', () => {
|
||||
const { pane } = createPane('normal')
|
||||
const reattachWebgl = vi.fn()
|
||||
|
||||
scheduleSplitScrollRestore(
|
||||
() => pane,
|
||||
pane.id,
|
||||
scrollState,
|
||||
() => false,
|
||||
reattachWebgl
|
||||
)
|
||||
|
||||
expect(restoreScrollState).toHaveBeenCalledTimes(1)
|
||||
expect(pane.terminal.refresh).toHaveBeenCalledWith(0, 23)
|
||||
|
||||
vi.advanceTimersByTime(200)
|
||||
|
||||
expect(pane.pendingSplitScrollState).toBeNull()
|
||||
expect(reattachWebgl).toHaveBeenCalledWith(pane)
|
||||
expect(restoreScrollState).toHaveBeenCalledTimes(2)
|
||||
expect(pane.terminal.refresh).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('defers WebGL reattach and skips scroll restore for alternate-screen panes', () => {
|
||||
const { pane, bufferChangeDisposable, triggerBufferChange } = createPane('alternate')
|
||||
const reattachWebgl = vi.fn()
|
||||
|
||||
scheduleSplitScrollRestore(
|
||||
() => pane,
|
||||
pane.id,
|
||||
alternateScrollState,
|
||||
() => false,
|
||||
reattachWebgl
|
||||
)
|
||||
|
||||
expect(restoreScrollState).not.toHaveBeenCalled()
|
||||
expect(pane.terminal.refresh).not.toHaveBeenCalled()
|
||||
expect(pane.pendingSplitScrollState).toBe(scrollState)
|
||||
|
||||
vi.advanceTimersByTime(200)
|
||||
|
||||
expect(pane.pendingSplitScrollState).toBeNull()
|
||||
expect(reattachWebgl).not.toHaveBeenCalled()
|
||||
expect(restoreScrollState).not.toHaveBeenCalled()
|
||||
expect(pane.terminal.refresh).not.toHaveBeenCalled()
|
||||
|
||||
triggerBufferChange('alternate')
|
||||
|
||||
expect(reattachWebgl).not.toHaveBeenCalled()
|
||||
expect(bufferChangeDisposable.dispose).not.toHaveBeenCalled()
|
||||
|
||||
triggerBufferChange('normal')
|
||||
|
||||
expect(bufferChangeDisposable.dispose).toHaveBeenCalledTimes(1)
|
||||
expect(reattachWebgl).toHaveBeenCalledWith(pane)
|
||||
expect(restoreScrollState).not.toHaveBeenCalled()
|
||||
expect(pane.terminal.refresh).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('defers normal-screen scroll restore until an active TUI exits alternate screen', () => {
|
||||
const { pane, triggerBufferChange } = createPane('alternate')
|
||||
const reattachWebgl = vi.fn()
|
||||
|
||||
scheduleSplitScrollRestore(
|
||||
() => pane,
|
||||
pane.id,
|
||||
scrollState,
|
||||
() => false,
|
||||
reattachWebgl
|
||||
)
|
||||
|
||||
vi.advanceTimersByTime(200)
|
||||
|
||||
expect(pane.pendingSplitScrollState).toBe(scrollState)
|
||||
expect(restoreScrollState).not.toHaveBeenCalled()
|
||||
expect(pane.terminal.refresh).not.toHaveBeenCalled()
|
||||
|
||||
triggerBufferChange('normal')
|
||||
|
||||
expect(pane.pendingSplitScrollState).toBeNull()
|
||||
expect(reattachWebgl).toHaveBeenCalledWith(pane)
|
||||
expect(restoreScrollState).toHaveBeenCalledWith(pane.terminal, scrollState)
|
||||
expect(pane.terminal.refresh).toHaveBeenCalledWith(0, 23)
|
||||
})
|
||||
})
|
||||
|
|
@ -1,3 +1,4 @@
|
|||
import type { IBuffer, IDisposable } from '@xterm/xterm'
|
||||
import type { ManagedPaneInternal, ScrollState } from './pane-manager-types'
|
||||
import { restoreScrollState } from './pane-scroll'
|
||||
|
||||
|
|
@ -9,6 +10,43 @@ function refreshAfterReparent(pane: ManagedPaneInternal): void {
|
|||
}
|
||||
}
|
||||
|
||||
function runAfterNormalBuffer(
|
||||
pane: ManagedPaneInternal,
|
||||
getPaneById: (id: number) => ManagedPaneInternal | undefined,
|
||||
paneId: number,
|
||||
isDestroyed: () => boolean,
|
||||
callback: (pane: ManagedPaneInternal) => void
|
||||
): void {
|
||||
let disposable: IDisposable | null = null
|
||||
disposable = pane.terminal.buffer.onBufferChange((buffer: IBuffer) => {
|
||||
if (buffer.type === 'alternate') {
|
||||
return
|
||||
}
|
||||
disposable?.dispose()
|
||||
disposable = null
|
||||
if (isDestroyed()) {
|
||||
return
|
||||
}
|
||||
const live = getPaneById(paneId)
|
||||
if (live) {
|
||||
callback(live)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
function restoreCapturedScrollState(
|
||||
pane: ManagedPaneInternal,
|
||||
scrollState: ScrollState,
|
||||
reattachWebgl?: (pane: ManagedPaneInternal) => void
|
||||
): void {
|
||||
pane.pendingSplitScrollState = null
|
||||
if (reattachWebgl) {
|
||||
reattachWebgl(pane)
|
||||
}
|
||||
restoreScrollState(pane.terminal, scrollState)
|
||||
refreshAfterReparent(pane)
|
||||
}
|
||||
|
||||
function logPaneHealth(pane: ManagedPaneInternal, phase: string): void {
|
||||
const canvases = pane.container.querySelectorAll('canvas')
|
||||
const canvasInfo = Array.from(canvases).map((c) => {
|
||||
|
|
@ -89,10 +127,19 @@ export function scheduleSplitScrollRestore(
|
|||
return
|
||||
}
|
||||
const live = getPaneById(paneId)
|
||||
if (live?.pendingSplitScrollState) {
|
||||
restoreScrollState(live.terminal, scrollState)
|
||||
refreshAfterReparent(live)
|
||||
if (!live?.pendingSplitScrollState) {
|
||||
return
|
||||
}
|
||||
// Why: see the 200ms timer below — the alt-screen buffer belongs to a
|
||||
// TUI and restore-during-draw knocks its cursor one row off (#1298).
|
||||
if (
|
||||
scrollState.bufferType === 'alternate' ||
|
||||
live.terminal.buffer.active.type === 'alternate'
|
||||
) {
|
||||
return
|
||||
}
|
||||
restoreScrollState(live.terminal, scrollState)
|
||||
refreshAfterReparent(live)
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -104,12 +151,32 @@ export function scheduleSplitScrollRestore(
|
|||
if (!live) {
|
||||
return
|
||||
}
|
||||
live.pendingSplitScrollState = null
|
||||
if (reattachWebgl) {
|
||||
reattachWebgl(live)
|
||||
// Why: the alt-screen buffer belongs to a full-screen TUI (Claude Code,
|
||||
// vim, less) that owns its cursor position. Re-running scroll restore
|
||||
// and a full refresh here clobbers an in-progress draw — refresh(0,
|
||||
// rows-1) repaints rows from xterm's buffer, racing the TUI's next
|
||||
// write and leaving its cursor one row off (#1298 regression).
|
||||
// WebGL reattach also refreshes, so defer it until the TUI exits the
|
||||
// alternate buffer. Alt-screen has no scrollback, so scroll restore has
|
||||
// nothing legitimate to do.
|
||||
if (scrollState.bufferType === 'alternate') {
|
||||
live.pendingSplitScrollState = null
|
||||
if (live.terminal.buffer.active.type === 'alternate' && reattachWebgl) {
|
||||
runAfterNormalBuffer(live, getPaneById, paneId, isDestroyed, reattachWebgl)
|
||||
return
|
||||
}
|
||||
if (reattachWebgl) {
|
||||
reattachWebgl(live)
|
||||
}
|
||||
return
|
||||
}
|
||||
restoreScrollState(live.terminal, scrollState)
|
||||
refreshAfterReparent(live)
|
||||
if (live.terminal.buffer.active.type === 'alternate') {
|
||||
runAfterNormalBuffer(live, getPaneById, paneId, isDestroyed, (normalPane) => {
|
||||
restoreCapturedScrollState(normalPane, scrollState, reattachWebgl)
|
||||
})
|
||||
return
|
||||
}
|
||||
restoreCapturedScrollState(live, scrollState, reattachWebgl)
|
||||
}, 200)
|
||||
|
||||
setTimeout(() => {
|
||||
|
|
|
|||
|
|
@ -106,6 +106,7 @@ describe('safeFit', () => {
|
|||
terminalRows: 32
|
||||
})
|
||||
pane.pendingSplitScrollState = {
|
||||
bufferType: 'normal',
|
||||
wasAtBottom: true,
|
||||
firstVisibleLineContent: '',
|
||||
viewportY: 0,
|
||||
|
|
|
|||
Loading…
Reference in New Issue