diff --git a/jest.config.js b/jest.config.js index abc2f65..3bfbf56 100644 --- a/jest.config.js +++ b/jest.config.js @@ -55,9 +55,18 @@ module.exports = { // Coverage reporters coverageReporters: ['text', 'lcov', 'html'], + // Git worktrees live in `/.claude/` by project convention, which puts a + // second full copy of the repo INSIDE the repo. Without this, jest builds one + // module map across both copies, every package.json name collides, and suites fail + // in the MAIN tree as well — reporting failures that have nothing to do with the + // code under test. modulePathIgnorePatterns (not just testPathIgnorePatterns) is + // the one that keeps the copy out of the module map. + modulePathIgnorePatterns: ['/.claude/'], + // Ignore patterns testPathIgnorePatterns: [ '/node_modules/', + '/\\.claude/', // worktrees — see modulePathIgnorePatterns above '/dist/', '/build/', '/tests/e2e/', diff --git a/packages/terminal-core/src/TerminalEngine.ts b/packages/terminal-core/src/TerminalEngine.ts index 8a4630e..7e2c9f6 100644 --- a/packages/terminal-core/src/TerminalEngine.ts +++ b/packages/terminal-core/src/TerminalEngine.ts @@ -163,6 +163,44 @@ const LIVE_WRITE_MAX_MS = 64; // run seconds later is never misattributed. const ED3_EXPECT_WINDOW_MS = 1500; +// How long after a relocation a backend resize is still attributable to that +// relocation, and therefore worth stamping convergenceResizeAt for (design 012 +// §6.2). Comfortably covers "observe() initial callback next frame -> fit() -> +// onResize -> 120ms debounce"; short enough that an unrelated `clear` is very +// unlikely to fall inside it. A TUNED constant, not a derived one (§15.3). +const RELOCATION_CONVERGENCE_ARM_MS = 500; + +/** What `TerminalEngine.relocateTo` did (design 012 §5). */ +export type RelocationResult = 'relocated' | 'aborted'; + +/** + * Everything `relocateTo` must be able to put back if it fails partway. + * + * `abortRelocation`'s contract is "nothing changed", and it is enforced by + * restoring from this snapshot rather than by unwinding step by step. Two fields + * R2 mutates are DELIBERATELY not restored: `resizeEpoch` (a monotonic generation + * counter — rolling it back would resurrect a decision the bump correctly + * invalidated) and any `fitTimer` R3 armed (design 012 §5.1 point 2 — leaving it + * running is provably safe, and the FT rule forbids cancelling it). + */ +interface RelocationSnapshot { + /** The container the element is still in if R6 throws. */ + container: HTMLElement | null; + /** Restored so an aborted relocation can never leave the hidden-pane SIGWINCH + * park disabled — reviews 093 B2 / 094 B4, the highest-priority regression. */ + surfaceDisplayed: boolean; + /** Restored so an aborted canvas -> pane move cannot re-wire the CANVAS host + * with pane chrome — review 094 B5. */ + paneChrome: boolean; + /** Restored so an abort cannot leave a <=500ms arm on an engine that never + * moved — review 096. */ + convergenceArmUntil: number; + /** True once R4/R5 have torn the old wiring down, so an abort at R0's + * precondition check does not re-wire a container that was never unwired + * (which would double-register the four listeners). */ + rewired: boolean; +} + // Backlog 011: window after a suggestion accept during which Enter keydowns are // swallowed. Covers OS key auto-repeat (~30ms interval after a ~500ms delay) // without noticeably delaying a deliberate follow-up Enter. @@ -563,11 +601,26 @@ export class TerminalEngine { // that never call setActive (mirror/grid) keep today's behavior. private paneActive = true; + // Host-reported SURFACE visibility, orthogonal to paneActive above. True while + // this terminal's rendered surface is displayed somewhere other than its pane — + // a Canvas Mode node (design 012 D15). A background tab's terminal shown on + // canvas has paneActive === false but is GENUINELY VISIBLE, so every geometry + // path must run. Written ONLY by setSurfaceDisplayed, whose only caller in this + // design is relocateTo (R3, §5.4) — putting the transition inside the operation + // is what makes an aborted relocation incapable of leaving eligibility raised + // (reviews 093 B2 / 094 B4). + private surfaceDisplayed = false; + // Debounce for backend PTY resizes (see BACKEND_RESIZE_DEBOUNCE_MS). xterm's // own resize is immediate; only the bridge.resize() call is coalesced. Cleared // on unmount so it can't fire against a torn-down mount. private resizeTimer: ReturnType | null = null; private pendingResize: { cols: number; rows: number } | null = null; + // Was `pendingResize` measured while the engine was INELIGIBLE? Decides whether + // unmount()'s force bypass may send it (see flushBackendResize). Set per scheduled + // value: the engine is ineligible at teardown in both the shipped force case and + // the parked one, so only the value's own provenance separates them. + private pendingResizeMeasuredWhileIneligible = false; // True while a bridge.resize() round-trip is outstanding (set by flushBackendResize // and the hydrate pre-resize, cleared when it settles). The heal skips while set — // flushBackendResize nulls pendingResize BEFORE awaiting, so pendingResize alone @@ -585,6 +638,14 @@ export class TerminalEngine { // Bumped on every scheduleBackendResize so a heal that began before a resize can // detect the change after its await and abort. private resizeEpoch = 0; + // Epoch ms until which a backend resize is attributable to a relocation. Set by + // relocateTo's R2 to Date.now() + RELOCATION_CONVERGENCE_ARM_MS, consumed at most + // ONCE by stampConvergenceIfArmed, and restored from the R0 snapshot on every + // abort path — otherwise an aborted relocation would leave a <=500ms arm on an + // engine that never moved, and an unrelated resize would open a spurious 1500ms + // ED3 repair window: exactly the misattribution :2386-2388 exists to avoid + // (design 012 §5.1, review 096). + private convergenceArmUntil = 0; // Global suppression after a backend pipeline-healed jiggle (which resizes EVERY // terminal's PTY). Static so one event quiets all engines. static suppressHealUntil = 0; @@ -673,6 +734,21 @@ export class TerminalEngine { // only by dispose()/cleanupTerminalCache). private disposables: Array<() => void> = []; + // CONTAINER-LOCAL disposables — the four DOM listeners bound to the container + // argument (click-to-focus, zoom keydown, Ctrl/Cmd+F, modifier+wheel). Split + // out of `disposables` above so relocateTo() can tear down the OLD container's + // bindings while every xterm/addon subscription — bound to the surviving + // `Terminal` — stays live (design 012 D6). Mirrored onto the cache entry by + // reference, exactly like `disposables`. + private containerDisposables: Array<() => void> = []; + // The chrome mode of the container this engine is currently wired to. + // `true` = the container sits inside the TerminalDisplay subtree that renders + // the pane's chrome (search bar, context menu, suggest popup). mount() always + // wires with `true`, so this initializer matches today's behaviour for the + // window before mount() runs (design 012 §5.8). Read by the suggest gate + // (§5.11) and restored from the R0 snapshot on an aborted relocation (§5.1). + private paneChromeActive = true; + constructor(bridge: TerminalBridge, opts: TerminalEngineOptions = {}) { this.bridge = bridge; this.opts = opts; @@ -684,6 +760,230 @@ export class TerminalEngine { this.paneActive = opts.active ?? true; } + /** + * Wire everything that binds to the CURRENT container: the four DOM listeners + * and the ResizeObserver. Called by `mount()` and by `relocateTo()` (design + * 012 §5.8), so the two paths cannot drift. + * + * The four listeners bind to the `container` PARAMETER, so they are inherently + * tied to whichever container was passed. The observer's normal-branch fit body + * reads `this.container` (not the parameter) — which relocateTo's R6 has + * already updated by the time it calls this — and only `observe(container)` + * uses the parameter. Both are correct after R6, and both are wrong if this + * wiring is duplicated between the two call sites instead of shared. That is + * why this helper is mandatory rather than stylistic (review 089). + * + * NOT a straight cut of :1785-1927: `autoFocus` and the rAF settle-fit sit + * BETWEEN the wheel listener and the observer and stay in mount(), so a single + * call placed at mount()'s old :1785 moves the observer's creation ahead of + * those two. Immaterial — `observe()` delivers its initial callback + * asynchronously, so nothing observes the reorder (design 012 §5.8). + * + * `o.paneChrome` gates the two affordances that belong to a PANE and not to a + * chromeless canvas host (design 012 D16): click-to-focus and Ctrl/Cmd+F. Zoom + * (keys and wheel) is wired in BOTH modes — 010:376 keeps it as "existing + * behaviour, unchanged". + */ + private wireContainerLocals( + container: HTMLElement, + boundTerm: Terminal, + fit: FitAddon | undefined, + o: { paneChrome: boolean }, + ): void { + // --- Click-to-focus (source 535-542) --- + // Pane hosts only. design 012 D16 / 010:377-378 reserve a single click on a + // canvas node for node SELECTION; focusing is 010's double-click, after the + // fly-to-z=1 (010:223-225), and Canvas Mode calls the engine's public focus() + // itself. NOTE this omission is a SECONDARY measure only: xterm binds its own + // "always on" mousedown to term.element (CoreBrowserTerminal bindMouse), which + // TRAVELS WITH THE ELEMENT, so the actual guarantee comes from the host giving + // term.element no pointer events while unfocused (design 012 D19 / §4.4 row 8). + if (o.paneChrome) { + const clickHandler = () => { + boundTerm.focus(); + }; + container.addEventListener('click', clickHandler); + this.containerDisposables.push(() => { + container.removeEventListener('click', clickHandler); + }); + } + + // --- Capture-phase zoom listener (source 544-582) --- + // Dual path: this fires in the CAPTURE phase and stopsPropagation, so the + // custom key handler does NOT also fire -> exactly +1 per keypress. + // OS-aware modifier: Cmd on macOS, Ctrl elsewhere. + // Both locals must live HERE: the keydown handler AND the wheel handler close + // over zoomModifier, and the Ctrl/Cmd+F handler closes over isMac. + const isMac = + this.opts.isMac ?? + (typeof navigator !== 'undefined' && !!navigator.platform?.includes('Mac')); + const zoomModifier = (event: KeyboardEvent | WheelEvent): boolean => + isMac ? event.metaKey : event.ctrlKey; + + const zoomHandler = (event: KeyboardEvent) => { + if (!zoomModifier(event)) return; + const key = event.key; + const code = event.code; + + if (key === '=' || key === '+' || code === 'Equal' || code === 'NumpadAdd') { + event.preventDefault(); + event.stopPropagation(); + this.handleZoom('in'); + return; + } + if (key === '-' || key === '_' || code === 'Minus' || code === 'NumpadSubtract') { + event.preventDefault(); + event.stopPropagation(); + this.handleZoom('out'); + return; + } + if (key === '0' || code === 'Digit0' || code === 'Numpad0') { + event.preventDefault(); + event.stopPropagation(); + this.handleZoom('reset'); + return; + } + }; + container.addEventListener('keydown', zoomHandler, true); + this.containerDisposables.push(() => { + container.removeEventListener('keydown', zoomHandler, true); + }); + + // Ctrl+F (Win/Linux) / Cmd+F (macOS) opens the host search overlay. Intercept in + // the CAPTURE phase so we preventDefault the browser's native find-in-page dialog + // before it opens, and only while THIS pane is focused (the listener is on the + // pane container). Shift/Alt excluded so Ctrl+Shift+F etc. pass through. + // + // Pane hosts only (design 012 §8): on a chromeless host this would call + // onOpenSearch -> setSearchOpen(true) -> the bar renders in the OFF-SCREEN pane + // and autofocuses its input (TerminalSearchBar.tsx:39-41), pulling focus out of + // the canvas. Unwired, xterm forwards ^F to the PTY as a normal terminal key. + if (o.paneChrome) { + const searchKeyHandler = (event: KeyboardEvent) => { + const modifier = isMac ? event.metaKey : event.ctrlKey; + if (!modifier || event.shiftKey || event.altKey) return; + if (event.key === 'f' || event.key === 'F' || event.code === 'KeyF') { + event.preventDefault(); + event.stopPropagation(); + this.opts.onOpenSearch?.(); + } + }; + container.addEventListener('keydown', searchKeyHandler, true); + this.containerDisposables.push(() => { + container.removeEventListener('keydown', searchKeyHandler, true); + }); + } + + // --- Modifier + mouse-wheel zoom (capture, non-passive so preventDefault + // actually blocks the WebView's native page zoom + xterm scrollback). Routes + // through the same handleZoom path as the keys. --- + const wheelZoomHandler = (event: WheelEvent) => { + if (!zoomModifier(event)) return; + event.preventDefault(); + event.stopPropagation(); + if (event.deltaY < 0) this.handleZoom('in'); + else if (event.deltaY > 0) this.handleZoom('out'); + }; + container.addEventListener('wheel', wheelZoomHandler, { passive: false, capture: true }); + this.containerDisposables.push(() => { + container.removeEventListener('wheel', wheelZoomHandler, true); + }); + + // --- ResizeObserver: rAF-debounced fit (source 598-621) --- + // NOT a containerDisposable (design 012 D7): `this.resizeObserver` is its + // single owner, disconnected explicitly by mount(), unmount() and relocateTo. + if (typeof ResizeObserver === 'function') { + if (this.opts.mirror) { + // Mirror: the GRID stays pinned to the backend; on pane resize we only + // re-fit the FONT (zoom-to-fit) so the whole terminal stays visible. Works + // per-pane, so grid view fits every cell independently. + const ro = new ResizeObserver(() => { + if (typeof requestAnimationFrame !== 'function') { + this.applyMirrorFit(); + return; + } + requestAnimationFrame(() => this.applyMirrorFit()); + }); + ro.observe(container); + this.resizeObserver = ro; + } else { + const resizeObserver = new ResizeObserver(() => { + if (typeof requestAnimationFrame !== 'function') return; + requestAnimationFrame(() => { + // Hidden pane (background tab): don't follow layout changes — no + // xterm resize, no backend SIGWINCH (codex ED3 wipe). The + // setActive(true) fit flushes the final geometry on activation. + if (!this.geometryEligible()) return; + const el = this.container; + if (el && el.offsetWidth > 50 && el.offsetHeight > 50) { + try { + const dims = fit?.proposeDimensions(); + // Diagnostics (source TerminalDisplay.tsx:606-609): observer-driven fit. + this.opts.onDiag?.( + () => `[TERM-DIAG] ResizeObserver | xterm=${this.term?.cols}x${this.term?.rows}`, + ); + if (dims && dims.cols > 10 && dims.rows > 5) { + fit?.fit(); + } + } catch (error) { + console.warn('terminal-core/engine: Failed to fit terminal:', error); + } + } + }); + }); + resizeObserver.observe(container); + this.resizeObserver = resizeObserver; + } + } + + // Last: record the chrome mode this engine is now wired for. An aborted + // relocation restores it from the R0 snapshot (design 012 §5.1 / review 094 B5). + this.paneChromeActive = o.paneChrome; + } + + // --------------------------------------------------------------------------- + /** + * Remove any OTHER cached terminal's render element from `container` before this + * mount attaches its own (design 012 §14 criterion 7 — external review 103 + * finding 2). + * + * `mount()` is append-only and `unmount()` deliberately leaves `term.element` in + * the DOM, because the cache still owns the live Terminal and a later mount + * reattaches it (see the closing comment of unmount()). Both are correct on their + * own. Together they leak whenever a pane node is REUSED for a different terminal + * id: `TerminalPane` renders an unkeyed `TerminalDisplay`, so changing + * `terminalId` in place keeps the same DOM node, and engine A's surface is still + * sitting in it when engine B appends its own. The pane then hosts both — both + * full-height, with A still painting through its cache-lifetime bridge + * subscription while its input wiring is gone. + * + * It also pins A's cache entry: `enforceCacheCap` skips any entry whose element + * is still `isConnected` (`cache.ts:142`), so a connected orphan is never evicted + * and holds its Terminal, scrollback and two bridge subscriptions forever. + * + * Keyed on ELEMENT IDENTITY via the cache, not on a `.xterm` class sweep: the + * only nodes we may remove are ones we can positively identify as some other + * engine's surface. Anything else in the container — overlays, the WebGL scratch + * canvas, future chrome — is none of this method's business. + * + * Removing is safe and non-destructive. The element stays owned by its cache + * entry, and A's own `mount()` reattaches it with `container.appendChild`, which + * works just as well from a detached node. + * + * NOTE this is not a P0-B defect — the bare mount/unmount/mount sequence + * reproduces it with no canvas involved, which is why the repair belongs here + * rather than in the relocation cleanup that made it reachable. + */ + private detachForeignSurfaces(container: HTMLElement, ours: HTMLElement | null): void { + for (const [key, entry] of terminalCache) { + if (key === this.cacheKey) continue; + const element = entry.terminal.element; + if (element && element !== ours && element.parentElement === container) { + element.remove(); + } + } + } + // --------------------------------------------------------------------------- // mount — create-or-reattach the xterm instance into `container`. // Ports TerminalDisplay.tsx:242-630 (minus the hydration effect / Task 4). @@ -697,8 +997,13 @@ export class TerminalEngine { this.container = container; this.disposables = []; + this.containerDisposables = []; let cached = terminalCache.get(this.cacheKey); + // Evict ANOTHER terminal's surface from this container before we put ours in + // (design 012 §14 criterion 7, external review 103 finding 2). + this.detachForeignSurfaces(container, cached?.terminal.element ?? null); + let term: Terminal | undefined; let fit: FitAddon | undefined; let search: SearchAddon | undefined; @@ -740,6 +1045,11 @@ export class TerminalEngine { // Dispose the previous mount's local event handlers before re-wiring. cached.disposables.forEach((dispose) => dispose()); + // …and the previous CONTAINER's listeners (design 012 §5.5 site 4). + // LOAD-BEARING: without this a remount leaves the old container's four + // listeners attached to the abandoned node, still focusing this terminal + // and still opening its search bar from a pane that is no longer on screen. + cached.containerDisposables.forEach((dispose) => dispose()); try { const existingElement = term.element; @@ -1003,6 +1313,7 @@ export class TerminalEngine { pendingOutput: [], pendingOutputBytes: 0, disposables: [], + containerDisposables: [], hydrationGeneration: 0, protocolDisposables, edRepairGeneration: 0, @@ -1751,6 +2062,13 @@ export class TerminalEngine { // switch — exactly the repaint storm 035 warns about. lastSentSize: existingCache?.lastSentSize, disposables: this.disposables, + // Explicit AFTER the `...existingCache` spread at :1758 on purpose: the + // spread would otherwise carry the PREVIOUS mount's already-run array + // forward, and relocateTo's R4 — which disposes off the ENTRY's reference, + // not the engine's — would then dispose the wrong one (design 012 §5.5 + // site 6). What is stored is the ARRAY REFERENCE; the container wiring + // below mutates it. Do not "fix" this by copying the array. + containerDisposables: this.containerDisposables, dataDisposable: existingCache?.dataDisposable, exitDisposable: existingCache?.exitDisposable, hydrationGeneration: existingCache?.hydrationGeneration ?? 0, @@ -1782,86 +2100,10 @@ export class TerminalEngine { }); enforceCacheCap(); - // --- Click-to-focus (source 535-542) --- - const clickHandler = () => { - boundTerm.focus(); - }; - container.addEventListener('click', clickHandler); - this.disposables.push(() => { - container.removeEventListener('click', clickHandler); - }); - - // --- Capture-phase zoom listener (source 544-582) --- - // Dual path: this fires in the CAPTURE phase and stopsPropagation, so the - // custom key handler above does NOT also fire -> exactly +1 per keypress. - // OS-aware modifier: Cmd on macOS, Ctrl elsewhere. - const isMac = - this.opts.isMac ?? - (typeof navigator !== 'undefined' && !!navigator.platform?.includes('Mac')); - const zoomModifier = (event: KeyboardEvent | WheelEvent): boolean => - isMac ? event.metaKey : event.ctrlKey; - - const zoomHandler = (event: KeyboardEvent) => { - if (!zoomModifier(event)) return; - const key = event.key; - const code = event.code; - - if (key === '=' || key === '+' || code === 'Equal' || code === 'NumpadAdd') { - event.preventDefault(); - event.stopPropagation(); - this.handleZoom('in'); - return; - } - if (key === '-' || key === '_' || code === 'Minus' || code === 'NumpadSubtract') { - event.preventDefault(); - event.stopPropagation(); - this.handleZoom('out'); - return; - } - if (key === '0' || code === 'Digit0' || code === 'Numpad0') { - event.preventDefault(); - event.stopPropagation(); - this.handleZoom('reset'); - return; - } - }; - container.addEventListener('keydown', zoomHandler, true); - this.disposables.push(() => { - container.removeEventListener('keydown', zoomHandler, true); - }); - - // Ctrl+F (Win/Linux) / Cmd+F (macOS) opens the host search overlay. Intercept in - // the CAPTURE phase so we preventDefault the browser's native find-in-page dialog - // before it opens, and only while THIS pane is focused (the listener is on the - // pane container). Shift/Alt excluded so Ctrl+Shift+F etc. pass through. - const searchKeyHandler = (event: KeyboardEvent) => { - const modifier = isMac ? event.metaKey : event.ctrlKey; - if (!modifier || event.shiftKey || event.altKey) return; - if (event.key === 'f' || event.key === 'F' || event.code === 'KeyF') { - event.preventDefault(); - event.stopPropagation(); - this.opts.onOpenSearch?.(); - } - }; - container.addEventListener('keydown', searchKeyHandler, true); - this.disposables.push(() => { - container.removeEventListener('keydown', searchKeyHandler, true); - }); - - // --- Modifier + mouse-wheel zoom (capture, non-passive so preventDefault - // actually blocks the WebView's native page zoom + xterm scrollback). Routes - // through the same handleZoom path as the keys. --- - const wheelZoomHandler = (event: WheelEvent) => { - if (!zoomModifier(event)) return; - event.preventDefault(); - event.stopPropagation(); - if (event.deltaY < 0) this.handleZoom('in'); - else if (event.deltaY > 0) this.handleZoom('out'); - }; - container.addEventListener('wheel', wheelZoomHandler, { passive: false, capture: true }); - this.disposables.push(() => { - container.removeEventListener('wheel', wheelZoomHandler, true); - }); + // design 012 §5.8: one definition of "everything bound to the container", + // shared with relocateTo(). mount() always wires pane chrome — that preserves + // today's behaviour byte-for-byte. + this.wireContainerLocals(container, boundTerm, fit, { paneChrome: true }); // Focus the terminal (source 589). Gated by autoFocus (default true) so grid // panes that aren't selected don't steal focus from each other on mount. @@ -1882,51 +2124,6 @@ export class TerminalEngine { this.disposables.push(() => cancelAnimationFrame(rafId)); } - // --- ResizeObserver: rAF-debounced fit (source 598-621) --- - if (typeof ResizeObserver === 'function') { - if (this.opts.mirror) { - // Mirror: the GRID stays pinned to the backend; on pane resize we only - // re-fit the FONT (zoom-to-fit) so the whole terminal stays visible. Works - // per-pane, so grid view fits every cell independently. - const ro = new ResizeObserver(() => { - if (typeof requestAnimationFrame !== 'function') { - this.applyMirrorFit(); - return; - } - requestAnimationFrame(() => this.applyMirrorFit()); - }); - ro.observe(container); - this.resizeObserver = ro; - } else { - const resizeObserver = new ResizeObserver(() => { - if (typeof requestAnimationFrame !== 'function') return; - requestAnimationFrame(() => { - // Hidden pane (background tab): don't follow layout changes — no - // xterm resize, no backend SIGWINCH (codex ED3 wipe). The - // setActive(true) fit flushes the final geometry on activation. - if (!this.paneActive) return; - const el = this.container; - if (el && el.offsetWidth > 50 && el.offsetHeight > 50) { - try { - const dims = fit?.proposeDimensions(); - // Diagnostics (source TerminalDisplay.tsx:606-609): observer-driven fit. - this.opts.onDiag?.( - () => `[TERM-DIAG] ResizeObserver | xterm=${this.term?.cols}x${this.term?.rows}`, - ); - if (dims && dims.cols > 10 && dims.rows > 5) { - fit?.fit(); - } - } catch (error) { - console.warn('terminal-core/engine: Failed to fit terminal:', error); - } - } - }); - }); - resizeObserver.observe(container); - this.resizeObserver = resizeObserver; - } - } - // Reconcile the BACKEND size with the size xterm already adopted on REATTACH. // // The reattach fit (:655) runs BEFORE the onResize listener above is wired @@ -2181,6 +2378,10 @@ export class TerminalEngine { await this.bridge.resize(processId, cols, rows); const e = terminalCache.get(this.cacheKey); if (e) e.lastSentSize = { cols, rows }; + // design 012 §6.2 / H2: the ONE direct sender that bypasses + // scheduleBackendResize. A relocation landing while this hydration was + // still awaiting must not SIGWINCH a ratatui PTY with no repair armed. + this.stampConvergenceIfArmed(); } catch (e) { console.warn( `terminal-core/engine: pre-hydration resize failed for ${this.cacheKey}:`, @@ -2342,31 +2543,109 @@ export class TerminalEngine { this.paneActive = active; // Deactivation: cancel any armed fit so it can't resize a now-hidden pane // (e.g. an activation fit scheduled 50ms before a quick tab switch away). + // + // THE ONE CANCEL the FT rule permits (design 012 §5.3 / D10), and only while + // the surface is NOT displayed elsewhere: hiding the TAB must never kill the + // settle fit a canvas display armed. A tab hide is not a move — nothing is + // racing to replace this timer — which is exactly why setSurfaceDisplayed + // (always part of a move) must NOT mirror it. if (!active) { - if (this.fitTimer) { + if (!this.surfaceDisplayed && this.fitTimer) { clearTimeout(this.fitTimer); this.fitTimer = null; } return; } - // Re-fit on activation with a 50ms settle (source 228-240 / R7). Also the - // FLUSH point for geometry changes deferred while hidden (paneActive above): - // fit() re-measures the container, and an actual size change flows through - // xterm onResize -> scheduleBackendResize (deduped when nothing changed). - if (active && this.fitAddon) { - if (this.fitTimer) clearTimeout(this.fitTimer); - this.fitTimer = setTimeout(() => { - this.fitTimer = null; - try { - this.fitAddon?.fit(); - } catch (error) { - console.warn('terminal-core/engine: Failed to fit terminal on activation:', error); - } - // Deliver geometry deferred while hidden (parked pending, or a stale - // backend size the no-op fit above didn't correct). MUST run after fit(). - this.flushDeferredResizeOnActivation(); - }, 50); + // Re-fit on activation with a 50ms settle (source 228-240 / R7). Unconditional + // even when surfaceDisplayed already made us eligible: that asymmetry with + // setSurfaceDisplayed(true) is DELIBERATE (design 012 §7.2). This is shipped + // tab-switch behaviour exercised on every activation; narrowing it to save one + // no-op FitAddon.fit() would change a hot, well-tested path for no benefit. + // Do not "fix" it. + this.armActivationFit(); + } + + /** + * Arm the 50ms settle fit and the deferred-resize flush that follows it — + * the body of the old setActive(true) branch, MINUS its `active` test, so it + * can also be driven by setSurfaceDisplayed (design 012 §7.2). + */ + private armActivationFit(): void { + if (!this.fitAddon) return; + if (this.fitTimer) clearTimeout(this.fitTimer); + this.fitTimer = setTimeout(() => { + this.fitTimer = null; + try { + this.fitAddon?.fit(); + } catch (error) { + console.warn('terminal-core/engine: Failed to fit terminal on activation:', error); + } + // Deliver geometry deferred while hidden (parked pending, or a stale + // backend size the no-op fit above didn't correct). MUST run after fit(). + this.flushDeferredResizeOnActivation(); + }, 50); + } + + /** + * Host tells the engine its surface is (or is no longer) displayed somewhere + * other than its pane — a Canvas Mode node (design 012 D15). + * + * Public because design/010 and design/013 need the concept, but in THIS design + * `relocateTo` is its only caller: the transition lives inside the operation so + * that every abort path can restore it from the R0 snapshot (§5.4). + * + * It NEVER touches focus, and it never leaves the engine with NO pending fit — + * in either direction (the FT rule, design 012 §5.3 / D10 / §7.2 rows 4/4a). On + * the return trip R3 lowers eligibility BEFORE R6 moves the element and BEFORE + * R7 re-arms the observer, whose initial callback is gated on geometryEligible() + * — so on a BACKGROUND pane nothing in `relocateTo` would otherwise measure the + * pane, and this method owns the only fit that will. + */ + setSurfaceDisplayed(displayed: boolean): void { + const wasEligible = this.geometryEligible(); + this.surfaceDisplayed = displayed; + // A false->true ELIGIBILITY transition arms the settle fit — identical + // semantics to setActive(true), minus focus. Already eligible => record and + // return; a second settle fit is churn. + if (displayed && !wasEligible) { + this.armActivationFit(); + return; } + // §7.2 row 4a — the return leg onto a BACKGROUND pane (external review 103 + // finding 1). Eligibility is now false, so R7's observer callback is skipped + // and no other geometry path in relocateTo will run: this arm is the whole + // repair. Rev 6 recorded and returned here, on the strength of §7.3's claim + // that "the surviving fitTimer" fills the gap. That claim only holds if the + // canvas visit was SHORTER THAN 50ms — the outbound timer nulls itself when it + // fires, so after any real visit there is nothing left to survive, and xterm + // stayed at the canvas node's grid until the tab was next activated. + // + // armActivationFit() clears-and-replaces rather than dropping, so the FT rule + // still holds as stated: the fit is preserved, rescheduled to fire 50ms from + // HERE — i.e. after R6's synchronous move, so it measures the pane, which is + // strictly better than a timer armed before the outbound leg. + // + // This does NOT breach the hidden-pane SIGWINCH park (§6.2): the fit resizes + // xterm only, and the backend resize it provokes hits flushBackendResize's + // ineligibility check and parks. The PTY still learns the size on the next + // activation, via the flush this same timer's callback performs. Narrow to an + // inactive pane on purpose — with paneActive true, R7's observer already fits. + if (!displayed && !this.paneActive) this.armActivationFit(); + } + + /** + * "Is this terminal's geometry live?" — the single predicate the six sites in + * design 012 §7.1 gate on, replacing a bare `this.paneActive` read at each: + * the ResizeObserver rAF callback, flushDeferredResizeOnActivation, setFontSize, + * flushBackendResize's park, and healOnce (entry + post-await recheck). + * + * Leaving any of them on the old flag alone silently breaks a canvas-displayed + * background tab: the observer ignores its resize, the parked flush never fires, + * a font change never refits, the PTY never learns its size, and the dimension + * heal aborts AFTER paying for getSize(). + */ + private geometryEligible(): boolean { + return this.paneActive || this.surfaceDisplayed; } // Called by the setActive(true) settle fit. The fit() above re-measures the @@ -2375,7 +2654,7 @@ export class TerminalEngine { // resize was PARKED by flushBackendResize while hidden, or a hidden hydration // deferred its size — so reconcile the PTY to xterm's real size exactly once. private flushDeferredResizeOnActivation(): void { - if (this.opts.mirror || !this.paneActive) return; + if (this.opts.mirror || !this.geometryEligible()) return; const term = this.term; if (!term || term.cols <= 0 || term.rows <= 0 || this.resizeInFlight) return; // ED3 resize-wipe repair: this function only ever runs as part of the @@ -2386,17 +2665,13 @@ export class TerminalEngine { // Narrower than stamping unconditionally on every activation: a reactivation // that resizes nothing never stamps, so it can't misattribute an unrelated // wipe days later. - const stampConvergenceResize = () => { - const e = terminalCache.get(this.cacheKey); - if (e) e.convergenceResizeAt = Date.now(); - }; if (this.pendingResize) { // A size is pending (freshly armed by the fit's onResize just above — it // synchronously calls scheduleBackendResize on a real dimension change — // or parked at deactivation with its timer cleared). Either way this IS // the convergence resize; stamp regardless of whether the debounce timer // still needs (re)arming. - stampConvergenceResize(); + this.stampConvergenceResize(); if (!this.resizeTimer) { this.resizeTimer = setTimeout(() => this.flushBackendResize(), BACKEND_RESIZE_DEBOUNCE_MS); } @@ -2404,11 +2679,47 @@ export class TerminalEngine { } const sent = terminalCache.get(this.cacheKey)?.lastSentSize; if (!sent || sent.cols !== term.cols || sent.rows !== term.rows) { - stampConvergenceResize(); + this.stampConvergenceResize(); this.scheduleBackendResize(term.cols, term.rows); } } + /** + * Mark "this terminal's PTY size is converging RIGHT NOW", so a CSI-3J arriving + * within ED3_EXPECT_WINDOW_MS can be attributed to OUR resize rather than to an + * unrelated clear. Extracted from flushDeferredResizeOnActivation's local + * closure so relocation can reuse it (design 012 §6.2); the two call sites there + * (:2399, :2407) are unchanged in behaviour. + */ + private stampConvergenceResize(): void { + const e = terminalCache.get(this.cacheKey); + if (e) e.convergenceResizeAt = Date.now(); + } + + /** + * Stamp only if a relocation armed the window, and consume the arm. + * + * Called from the two places a size can reach the PTY (design 012 §6.2): + * - scheduleBackendResize — the debounced choke point, and the route the + * relocation path itself takes (onResize at :1119); + * - hydrate()'s direct pre-hydration resize at :2181, the ONE sender that + * bypasses the debounce. A hydrate() started by an earlier attach() can + * still be awaiting when a relocation lands, and its SIGWINCH would then hit + * a ratatui PTY inside the arm window with no repair armed (review 093 B5). + * + * Armed rather than unconditional so an unchanged-geometry relocation — which + * produces no resize at all — never stamps, preserving the property the comment + * at :2386-2388 protects. If some OTHER caller fires inside the window (a heal + * at :2657, a hydrate reconcile at :2274), that is still a genuine PTY resize + * converging immediately after a relocation, so attributing a following ED3 to + * it is correct rather than a misattribution. + */ + private stampConvergenceIfArmed(): void { + if (Date.now() >= this.convergenceArmUntil) return; + this.convergenceArmUntil = 0; + this.stampConvergenceResize(); + } + // ED3 resize-wipe repair: debounce on output idle (so codex's post-ED3 re-emit // burst finishes) before repainting from the backend's authoritative full // scrollback. Full reset+rewrite, not a splice — no alignment/duplication risk, @@ -2506,19 +2817,29 @@ export class TerminalEngine { this.term.options.fontSize = px; // Hidden pane: apply the render option but defer the geometry refit to the // setActive(true) fit — a refit here would SIGWINCH a background PTY. - if (!this.paneActive) return; + if (!this.geometryEligible()) return; // Re-fit after font size change with a 50ms settle (source 217-225 / R7). - if (this.fitAddon) { - if (this.fitTimer) clearTimeout(this.fitTimer); - this.fitTimer = setTimeout(() => { - this.fitTimer = null; - try { - this.fitAddon?.fit(); - } catch (e) { - console.warn('terminal-core/engine: fit after font change failed:', e); - } - }, 50); - } + // + // This MUST be `armActivationFit`, not a bespoke fit-only timer (external + // review 103, finding 3). The two timers share one `fitTimer` slot, so + // whichever is armed second replaces the first — and before this change the + // font timer replaced it with a callback that fits but never calls + // `flushDeferredResizeOnActivation()`. That is a strand: + // + // hidden pane parks a resize (pendingResize set, resizeTimer null) + // -> relocateTo(canvas) raises eligibility and arms the ONLY callback that + // will ever flush that parked value + // -> a font change inside those 50ms replaces it with the fit-only timer + // -> if the new fit proposes the same grid, xterm emits no onResize, so + // nothing reschedules — and `healOnce` refuses to run while + // `pendingResize` is set, so the PTY stays stale indefinitely. + // + // Note this path is reachable at all only because §7.1 row 3 widened this + // gate from `paneActive` to `geometryEligible()`: on develop the early + // return above fired for a hidden pane and the timers could never collide. + // Arming the richer callback costs nothing — `flushDeferredResizeOnActivation` + // early-returns when there is nothing parked. + this.armActivationFit(); } focus(): void { @@ -2563,8 +2884,18 @@ export class TerminalEngine { // Coalesce rapid xterm resizes into a single backend PTY resize at the final // size (see BACKEND_RESIZE_DEBOUNCE_MS). private scheduleBackendResize(cols: number, rows: number): void { + // design 012 §6.2 / H2: if a relocation armed the window, this resize IS the + // convergence resize — stamp it so the ED3 detector at :1261-1274 opens its + // 1500ms repair window and a codex/ratatui scrollback wipe is repaired rather + // than lost. No-op when nothing armed it. + this.stampConvergenceIfArmed(); this.resizeEpoch++; this.pendingResize = { cols, rows }; + // Provenance, for unmount()'s force bypass (external review 105). Recorded per + // scheduled value, because it is the VALUE's history that decides whether + // teardown may send it — not the engine's state at teardown time, which is + // ineligible in both cases. + this.pendingResizeMeasuredWhileIneligible = !this.geometryEligible(); if (this.resizeTimer) clearTimeout(this.resizeTimer); this.resizeTimer = setTimeout(() => this.flushBackendResize(), BACKEND_RESIZE_DEBOUNCE_MS); } @@ -2580,13 +2911,32 @@ export class TerminalEngine { // pane is inactive we PARK the pending geometry here instead of sending it — the // setActive(true) activation reconcile flushes it. `force` (unmount teardown) // bypasses the park so the PTY still ends at the correct final size. + // + // …but ONLY for geometry that was MEASURED while eligible (external review 105). + // The shipped force case is an interrupted in-flight resize: the pane was visible, + // a real resize was measured and debounced, and the pane was hidden and torn down + // before the 120 ms elapsed — teardown must still deliver it, which is what the + // pane-collapse fix relies on. That value was always allowed to be sent; only the + // timing was interrupted. + // + // Geometry measured while INELIGIBLE is the opposite case. The park is not a delay + // there, it is a refusal: §6.2 says this engine may not SIGWINCH its PTY at all + // right now. Forcing such a value out at teardown is strictly worse than dropping + // it, because the ED3 detector that repairs a wipe is a PER-MOUNT disposable + // (disposed a few lines below this call) while the data subscription that receives + // the wipe is CACHE-LIFETIME — so the PTY's ESC[2J ESC[3J answer lands + // asynchronously, after the only thing that could repair it is gone. + // + // Dropping it costs nothing: the geometry is not lost, because the next mount's + // reattach fit re-measures the same container and re-sends it — with the detector + // armed. Same SIGWINCH, same final size, delivered where it can be repaired. private flushBackendResize(force = false): void { if (this.resizeTimer) { clearTimeout(this.resizeTimer); this.resizeTimer = null; } // Park: keep pendingResize (do NOT null it) so activation can deliver it. - if (!this.paneActive && !force) return; + if (!this.geometryEligible() && !(force && !this.pendingResizeMeasuredWhileIneligible)) return; const pending = this.pendingResize; this.pendingResize = null; if (!pending || !this.attachedProcessId) return; @@ -2631,7 +2981,7 @@ export class TerminalEngine { // via `visibility:hidden` (offsetParent stays non-null), so the offsetParent // check below never catches them — the host's setActive signal does. A heal // resize against a hidden codex pane wipes its scrollback (ED3 re-emit). - if (!this.paneActive) return; + if (!this.geometryEligible()) return; const c = this.container; if (!c || c.offsetParent === null || c.offsetWidth <= 50) return; // pane visible if (Date.now() < TerminalEngine.suppressHealUntil) return; // post-jiggle @@ -2644,7 +2994,7 @@ export class TerminalEngine { const size = await this.getBackendSize(pid); if (!size || size.cols <= 0 || size.rows <= 0) return; // Re-validate after the await. - if (!this.paneActive) return; // pane hidden while getSize was in flight + if (!this.geometryEligible()) return; // pane hidden while getSize was in flight if (this.attachedProcessId !== pid) return; const e2 = terminalCache.get(this.cacheKey); if (!e2 || e2.terminal !== term || e2.hydrating) return; @@ -2927,6 +3277,13 @@ export class TerminalEngine { } private emitInputLine(text: string): void { + // design 012 §5.11 / §8.1: on a chromeless host (a Canvas Mode node) the + // engine emits NOTHING, so useCommandSuggest never re-opens the popup, so + // suggestState never leaves 'closed', so the interception at :1346 never + // claims Up/Down/Tab/Enter. True by construction rather than by a one-shot + // close (review 093 B3). The capture heuristic itself keeps running — only + // the emission to the host stops (§5.12). + if (!this.paneChromeActive) return; if (text === this.lastEmittedInput) return; this.lastEmittedInput = text; this.opts.onInputLineChanged?.(text); @@ -2934,7 +3291,9 @@ export class TerminalEngine { /** Host tells the engine the popup's current state (drives key interception). */ setSuggestPopupState(state: SuggestPopupState): void { - this.suggestState = state; + // Belt-and-braces for any future caller (design 012 §5.11): the popup cannot + // exist on a chromeless host, so it must not be able to claim keys there. + this.suggestState = this.paneChromeActive ? state : 'closed'; } /** Insert a history command at the prompt: move the cursor to the end of the @@ -3249,8 +3608,13 @@ export class TerminalEngine { } // Flush (don't drop) any pending backend resize so the PTY isn't left at a // stale size when a drag is interrupted by this unmount (tab switch / pane move). - // force=true bypasses the hidden-pane park: teardown must deliver the final size - // even for a background tab (the next mount reattaches to that PTY). + // + // `force` bypasses the hidden-pane park ONLY for geometry that was measured + // while the engine was eligible — the interrupted-mid-debounce case this line + // exists for. Geometry the park REFUSED (measured while ineligible) stays + // parked and is dropped here; the next mount re-measures and re-sends it with + // the ED3 detector armed. See flushBackendResize for why that distinction is + // load-bearing rather than cautious (external review 105). this.flushBackendResize(true); if (this.resizeObserver) { this.resizeObserver.disconnect(); @@ -3259,6 +3623,9 @@ export class TerminalEngine { this.stopHealWatchdog(); this.disposables.forEach((dispose) => dispose()); this.disposables = []; + // design 012 §5.5 site 7: the container listeners are a separate array now. + this.containerDisposables.forEach((dispose) => dispose()); + this.containerDisposables = []; this.container = null; // Clear the active query AND its decorations. The SearchAddon lives on the // cached terminal, so highlights drawn before this unmount would otherwise stay @@ -3275,6 +3642,231 @@ export class TerminalEngine { // owns the live instance, and a later mount() reattaches it. } + // --------------------------------------------------------------------------- + // relocateTo — design 012 §5. Move this terminal's RENDERED SURFACE to a new + // container without ending the session. + // + // It is NOT mount() (design 012 D2). mount() does eight things relocation must + // not do (§5.0): construct a second EndedRegionTracker (which would leak the + // live one's onRender subscription, debounce timer and rail layer), construct a + // second HeuristicCapture (which would restore a stale mark), re-apply + // opts.fontSize (reverting every zoom since engine creation), focus + // unconditionally (stealing focus for a background pane), rebuild the cache + // entry (breaking design/013's identity keying), call enforceCacheCap(), call + // startHealWatchdog(), and — worst of all — fall through to the CREATE branch on + // any thrown error (:757-767), producing a brand-new blank Terminal with the + // entire scrollback gone, silently. That is hazard H1. + // + // The mechanism is one synchronous `container.appendChild(this.term.element)` + // (D1). Per the DOM spec that is remove-then-insert inside ONE synchronous + // algorithm; spike 004 Q2 measured that no observer of any kind ever sees + // `isConnected === false`. No portal, anywhere. + // --------------------------------------------------------------------------- + relocateTo(container: HTMLElement, opts?: { paneChrome?: boolean }): RelocationResult { + // --- R0: normalise, preconditions, restore snapshot (§5.1) --- + // Normalised ONCE, here. Every later step reads THIS local, never + // `opts.paneChrome` — which is undefined on the documented call + // `relocateTo(container)` (reviews 098 C1 + 096). + const paneChrome = opts?.paneChrome ?? false; + + // Placed BEFORE the identity no-op deliberately: a mirror engine must never + // reach any part of this operation, including the free path. Grid-view mirrors + // are not canvas nodes (design 012 §12), and applyMirrorFit's container reads + // (:3066, :3071) are dismissed by this line rather than merely documented. + if (this.opts.mirror) { + console.error('terminal-core/engine: relocateTo is not supported for mirror engines'); + return 'aborted'; + } + + // The identity no-op. LOAD-BEARING: both §4.2.2 cleanups call + // relocateTo(pane, { paneChrome: true }) unconditionally, and this is what + // makes the redundant one free. It must perform NO work at all — no dispose, + // no re-wire, no observer churn, no epoch bump, no convergenceArmUntil write, + // no eligibility change (§13 T2b). + if (container === this.container) return 'relocated'; + + const term = this.term; + const entry = terminalCache.get(this.cacheKey); + const snap: RelocationSnapshot = { + container: this.container, + surfaceDisplayed: this.surfaceDisplayed, + paneChrome: this.paneChromeActive, + convergenceArmUntil: this.convergenceArmUntil, + rewired: false, + }; + + // This precondition exists specifically to avoid mount()'s reattach catch-all + // (:757-763), which terminalCache.delete()s and falls into the create branch + // (:767). H1. + if (!term || !entry || entry.terminal !== term || !term.element) { + return this.abortRelocation( + `relocateTo: preconditions failed for ${this.cacheKey} ` + + `(term=${!!term} entry=${!!entry} owned=${entry?.terminal === term} ` + + `element=${!!term?.element})`, + snap, + ); + } + const element = term.element; + + // --- R1: capture focus ownership (§5.2) --- + // Sampled HERE, before anything moves, while everything is still attached. + // Spike 004 Q3 measured that the move blurs synchronously — `blur` and + // `focusout` fire as part of it and document.activeElement is already false on + // the next synchronous line — so this must be read first. + const hadFocus = + typeof document !== 'undefined' && !!element.contains(document.activeElement); + + // --- R2: invalidate in-flight geometry work, arm the convergence stamp, + // cancel NOTHING (§5.3) --- + // resizeEpoch is already this engine's "geometry intent changed" generation + // (sole writer scheduleBackendResize at :2566, sole readers healOnce at :2641 + // and :2652). Reuse before reinvent: do NOT add a second counter. The risk it + // covers is a DUPLICATE backend resize — healOnce decided to resize by + // comparing the backend size against dims measured before the move, and the + // relocation's own fit is about to supersede that decision (H6). + this.resizeEpoch++; + // Arm the ED3 repair window (§6.2 / H2): the resize this move is about to + // cause reaches the PTY through :1119 -> scheduleBackendResize, which stamps + // convergenceResizeAt only while this arm is live. Without the stamp a + // ratatui/codex PTY answers the SIGWINCH with ESC[2J ESC[3J and the scrollback + // is wiped with no repair armed — silent data loss. + this.convergenceArmUntil = Date.now() + RELOCATION_CONVERGENCE_ARM_MS; + + // --- R3: raise (or lower) surface eligibility (§5.4) --- + // INSIDE the operation, not in the renderer: that is what makes an aborted + // relocation incapable of leaving eligibility raised (reviews 093 B2 / 094 B4). + // Rev 4 had the renderer do this one line before and one line after, and an + // abort then left surfaceDisplayed === true for the life of the engine — + // permanently un-gating flushBackendResize's park (:2589), the observer fit and + // both healOnce gates, so a hidden ratatui/codex pane gets SIGWINCH'd and its + // scrollback wiped. + // + // ORDERING IS LOAD-BEARING: eligibility must already be true when R7 arms the + // new ResizeObserver, because that observer's initial callback is gated on + // geometryEligible(). On the return trip a BACKGROUND pane does get a gap — + // R7's callback is skipped — and the fitTimer the FT rule preserves (§5.3) is + // what fills it. Raising eligibility on the way home instead would defeat the + // hidden-pane SIGWINCH park §6.2 exists to arm. + // + // Under the FT rule this call can never leave the engine with NO pending fit, + // in either direction: outbound it arms one (§7.2 row 2), on the return leg onto + // a background pane it arms one (row 4a), and on the return leg onto a visible + // pane eligibility never drops so R7's observer fits on its own. + this.setSurfaceDisplayed(!paneChrome); + + // --- R4: dispose the PREVIOUS container's disposables (§5.5) --- + // Off the CACHE ENTRY's reference, not this.containerDisposables — exactly + // what mount()'s reattach branch already does at :742. Imitate, do not invent. + // Double-dispose is a pre-existing tolerated pattern here (unmount() runs the + // disposers but never clears the entry's reference, so cleanupTerminalCache + // runs them again), so every disposer already survives a second call. + entry.containerDisposables.forEach((d) => d()); + this.containerDisposables = []; + entry.containerDisposables = this.containerDisposables; // share the reference + + // --- R5: disconnect the ResizeObserver (§5.6) --- + // The same two lines mount() runs at :695-696 and unmount() at :3255-3258. + // Per D7 this is its ONLY owner, so this is a real step, not a re-run of R4. + this.resizeObserver?.disconnect(); + this.resizeObserver = null; + // Set HERE, at the end of the teardown pair, so "the old wiring is fully down" + // is the literal precondition for the abort re-wire, and so the re-wire cannot + // double-register (this.resizeObserver is already null). + snap.rewired = true; + + // --- R6: move the xterm element (§5.7) --- + // The catch is not theatre: appendChild throws HierarchyRequestError if + // `container` is inside term.element — reachable if a canvas node ever + // registers a host that is a descendant of the terminal. appendChild leaves + // the tree unchanged when it throws, so the element is still in snap.container. + try { + container.appendChild(element); + } catch (e) { + return this.abortRelocation(`relocateTo: appendChild failed: ${e}`, snap); + } + this.container = container; + + // --- R7: re-wire the container locals, record the chrome mode (§5.8) --- + // The four listeners bind to the `container` parameter; the observer's fit body + // reads this.container, which R6 has just updated. Both correct only because + // the wiring is SHARED with mount() rather than duplicated. + this.wireContainerLocals(container, term, this.fitAddon ?? undefined, { paneChrome }); + + // --- R8: re-target the ended-region rail (§5.9) --- + // MUST run after R6: wrapper resolution walks UP from term.element. Reuses + // this.endedRegions untouched — same instance, same regions, same open span, + // same colours, same single onRender subscription. Moving the memoised layer + // carries every child region.railEl with it in one DOM operation. + this.endedRegions?.retargetRail(); + + // --- R9: restore focus (§5.10) --- + // Conditional on R1's sample, so a background pane relocated onto canvas + // cannot steal focus. Same synchronous task as the move: spike 004 Q3 measured + // exactly one focus/focusin pair and nothing arriving late. + if (hadFocus) term.focus(); + + // --- R10: chrome-mode side effects — the suggest gate (§5.11) --- + // R7 has already set this.paneChromeActive from the same normalised local. + // Stops the key interception at :1346 dead: with the popup unable to open, no + // key is ever claimed (§8.1). + if (!this.paneChromeActive) this.suggestState = 'closed'; + // UNCONDITIONAL, in both directions: the dedup at :2930 must not swallow the + // first input line emitted after the return trip. + this.lastEmittedInput = ''; + + // --- R11: the in-progress capture mark — do nothing (§5.12) --- + // HeuristicCapture holds `private mark` and a `readonly term` + // (commandCapture.ts:58-65): zero DOM references, no listeners, no timers. + // Nothing binds it to a container, so it is reused outright. The cache's + // captureMark — written only by unmount() at :3226-3229 — goes stale during a + // relocated session, which is harmless: nothing reads it until the next real + // mount() (:1063), which is always preceded by the unmount() that refreshes it. + // NOTE what §6 makes of this: a geometry-CHANGING relocation runs the live + // onResize handler, which cancels the mark and sets suppressUntilSubmit + // (:1112-1116). That is the existing, correct reflow behaviour — an absolute + // row/col mark is untranslatable after reflow — and relocation must not defeat + // it. Under §6.5's rate contract it happens exactly twice per terminal per + // canvas session, which is why RC1 matters: without it, this fires per pan tick. + + return 'relocated'; + } + + /** + * Undo a partial relocation and report it. Aborting CHANGES NOTHING: the + * terminal has not moved and is still fully wired in its current, live + * container, with its eligibility, chrome mode and convergence arm exactly as + * they were (design 012 §5.1). + * + * `surfaceDisplayed` is restored by DIRECT FIELD WRITE, not through + * setSurfaceDisplayed(): restoring a snapshot must be a state assignment, not a + * transition. Going through the setter would run the transition table, and on an + * aborted return trip (snapshot true, current false) that means arming a FRESH + * 50ms fit on an engine that never moved. Harmless but wrong — arming a timer is + * a change. + */ + private abortRelocation(reason: string, snap: RelocationSnapshot): RelocationResult { + this.surfaceDisplayed = snap.surfaceDisplayed; + this.paneChromeActive = snap.paneChrome; + this.convergenceArmUntil = snap.convergenceArmUntil; + // Only re-wire if the wiring was actually removed, and re-wire with the + // RECORDED chrome mode, not a literal `true`: on an aborted canvas -> pane + // move snap.container IS the canvas host, and pane chrome there would install + // Ctrl/Cmd+F on it (review 094 B5). `this.container` is untouched on every + // abort path — R6 assigns it only after a successful appendChild — so there is + // nothing to restore there. + if (snap.rewired && snap.container && this.term) { + this.wireContainerLocals( + snap.container, + this.term, + this.fitAddon ?? undefined, + { paneChrome: snap.paneChrome }, + ); + } + console.error(`terminal-core/engine: ${reason}`); + this.opts.onDiag?.(() => `[TERM-DIAG] ${reason}`); + return 'aborted'; + } + // --------------------------------------------------------------------------- // dispose — full teardown incl. cache removal (disposes EVERYTHING, including // Task 4's cache-lifetime subscriptions, via cleanupTerminalCache). diff --git a/packages/terminal-core/src/__mocks__/xterm.ts b/packages/terminal-core/src/__mocks__/xterm.ts index 584631f..0ae15bf 100644 --- a/packages/terminal-core/src/__mocks__/xterm.ts +++ b/packages/terminal-core/src/__mocks__/xterm.ts @@ -167,6 +167,9 @@ export class Terminal { // Real xterm appends its render element to the container; emulate enough that // the reattach path (`cached.terminal.element`) sees a live node. const el = (typeof document !== 'undefined' ? document.createElement('div') : ({} as HTMLElement)); + // Real xterm classes its render element `terminal xterm`. Carried here so a + // test can tell a terminal surface apart from the pane's other children. + if (typeof (el as HTMLElement).className === 'string') el.className = 'terminal xterm'; this.element = el; if (container && typeof (container as HTMLElement).appendChild === 'function') { container.appendChild(el); diff --git a/packages/terminal-core/src/__tests__/cache.test.ts b/packages/terminal-core/src/__tests__/cache.test.ts index 5779341..be5b14d 100644 --- a/packages/terminal-core/src/__tests__/cache.test.ts +++ b/packages/terminal-core/src/__tests__/cache.test.ts @@ -19,6 +19,7 @@ function fakeEntry() { hydrating: false, pendingOutput: [], disposables: [() => disposed.push('d1')], + containerDisposables: [], dataDisposable: { dispose: () => disposed.push('data') }, exitDisposable: { dispose: () => disposed.push('exit') }, } as unknown as TerminalCacheEntry; @@ -70,6 +71,7 @@ test('cleanupTerminalCache works when R1 subscriptions are absent', () => { hydrating: false, pendingOutput: [], disposables: [() => disposed.push('d1')], + containerDisposables: [], } as unknown as TerminalCacheEntry; terminalCache.set('t2', entry); @@ -93,6 +95,7 @@ test('cleanupTerminalCache: a throwing webglAddon.dispose() still tears down the hydrating: false, pendingOutput: [], disposables: [() => disposed.push('d1')], + containerDisposables: [], } as unknown as TerminalCacheEntry; terminalCache.set('t3', entry); @@ -113,6 +116,7 @@ test('cleanupTerminalCache disposes protocolDisposables (backlog 003)', () => { hydrating: false, pendingOutput: [], disposables: [() => disposed.push('d1')], + containerDisposables: [], protocolDisposables: [() => disposed.push('proto1'), () => disposed.push('proto2')], } as unknown as TerminalCacheEntry; terminalCache.set('t-proto', entry); @@ -133,6 +137,7 @@ test('cleanupTerminalCache: a throwing protocolDisposable still lets the rest te hydrating: false, pendingOutput: [], disposables: [], + containerDisposables: [], protocolDisposables: [ () => { throw new Error('boom'); @@ -165,6 +170,7 @@ test('cleanupTerminalCache: a throwing local disposable still lets data/exit sub }, () => disposed.push('d2'), ], + containerDisposables: [], dataDisposable: { dispose: () => disposed.push('data') }, exitDisposable: { dispose: () => disposed.push('exit') }, } as unknown as TerminalCacheEntry; diff --git a/packages/terminal-core/src/__tests__/engine.container-disposables.test.ts b/packages/terminal-core/src/__tests__/engine.container-disposables.test.ts new file mode 100644 index 0000000..d135aca --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.container-disposables.test.ts @@ -0,0 +1,179 @@ +/** + * engine.container-disposables.test.ts + * + * design/012 D6 + §5.5 + §5.8 — §13 T3, T4, T13 (the mount()-side halves; the + * relocateTo halves land in Task 7). + * + * The split: `disposables` holds everything bound to something that SURVIVES a + * relocation (the same `boundTerm`, the reused SearchAddon, one-shot mount-time + * timers). `containerDisposables` holds exactly the four listeners bound to the + * `container` ARGUMENT: click-to-focus (TerminalEngine.ts:1785-1792), + * capture-phase zoom keydown (:1804-1831), Ctrl/Cmd+F (:1837-1849) and + * modifier+wheel (:1854-1864). The ResizeObserver is deliberately NOT one of + * them (D7) — it has exactly one owner, `this.resizeObserver`. + * + * Why the split matters: because the xterm subscriptions are never torn down by + * relocation, `boundTerm.onResize` (:1101-1120) stays live across the whole + * operation. That is what retires the orphaned-resize class documented by + * engine.remount-resize.test.ts:12-20 — a bug that exists precisely because + * mount() disposes onResize at :742 BEFORE fitting at :749 and re-wires it ~350 + * lines later at :1101. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +function makeContainer(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +describe('design/012 D6 — the container/local disposables split', () => { + // §13 T13, first clause. The array must actually be POPULATED — an empty + // `containerDisposables` is the exact defect review 094 B6 found in rev 4. + it('puts the four container listeners in containerDisposables and nowhere else', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cd-membership' }); + const localsBefore = (engine as any).disposables.length; + engine.mount(makeContainer()); + + expect((engine as any).containerDisposables.length).toBe(4); + // The entry mirrors the engine's LIVE array by reference, not by copy — + // exactly how `disposables: this.disposables` already works (:1753). + expect(terminalCache.get('cd-membership')!.containerDisposables) + .toBe((engine as any).containerDisposables); + // And they are not ALSO in `disposables`, or relocation would tear down the + // xterm subscriptions with them. + expect((engine as any).disposables.length).toBeGreaterThan(localsBefore); + expect((engine as any).disposables) + .not.toEqual(expect.arrayContaining((engine as any).containerDisposables)); + }); + + // §13 T3, mount() half: with paneChrome:true (what mount() always passes) + // every listener behaves byte-for-byte as it does today. + it('wires all four listeners on the container mount() was given', () => { + let openSearchCalls = 0; + let zoomCalls = 0; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'cd-wired', + isMac: false, + onOpenSearch: () => { openSearchCalls += 1; }, + onZoom: () => { zoomCalls += 1; }, + }); + const container = makeContainer(); + engine.mount(container); + const term = terminalCache.get('cd-wired')!.terminal as any; + + const focusBefore = term.focusCount; + container.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusBefore + 1); + + container.dispatchEvent( + new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true }), + ); + expect(openSearchCalls).toBe(1); + + container.dispatchEvent( + new KeyboardEvent('keydown', { key: '=', ctrlKey: true, bubbles: true }), + ); + expect(zoomCalls).toBe(1); + + container.dispatchEvent( + new WheelEvent('wheel', { deltaY: -1, ctrlKey: true, bubbles: true }), + ); + expect(zoomCalls).toBe(2); + }); + + // §13 T13, last clause / §5.5 site 4. Without the new dispose line at :742 a + // remount leaves the PREVIOUS container's four listeners attached to the + // abandoned node — they would keep focusing a terminal from a dead pane. + it('disposes the previous mount\'s container listeners on a mount() without unmount()', () => { + let openSearchCalls = 0; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'cd-remount', + isMac: false, + onOpenSearch: () => { openSearchCalls += 1; }, + }); + const a = makeContainer(); + engine.mount(a); + const term = terminalCache.get('cd-remount')!.terminal as any; + + const b = makeContainer(); + engine.mount(b); + + const focusBefore = term.focusCount; + a.dispatchEvent(new MouseEvent('click', { bubbles: true })); + a.dispatchEvent(new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true })); + expect(term.focusCount).toBe(focusBefore); + expect(openSearchCalls).toBe(0); + + // …and the NEW container is fully wired. + b.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusBefore + 1); + expect((engine as any).containerDisposables.length).toBe(4); + }); + + // §13 T13, second clause: unmount() must run BOTH arrays. Before the split it + // ran one; if it kept running only `disposables` the four listeners would + // outlive the engine. + it('runs both arrays on unmount()', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cd-unmount' }); + const container = makeContainer(); + engine.mount(container); + const term = terminalCache.get('cd-unmount')!.terminal as any; + + engine.unmount(); + expect((engine as any).containerDisposables).toEqual([]); + + const focusBefore = term.focusCount; + container.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusBefore); + }); + + // §13 T4, mount() half / D7: the observer is NOT a containerDisposable. It has + // exactly one owner and one explicit disconnect, which lifecycle-timers.test.ts + // :109-148 already pins for the mount()/unmount() paths. + it('keeps the ResizeObserver out of containerDisposables', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cd-ro' }); + engine.mount(makeContainer()); + expect((engine as any).containerDisposables.length).toBe(4); + expect((engine as any).resizeObserver).not.toBeNull(); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.convergence-stamp.test.ts b/packages/terminal-core/src/__tests__/engine.convergence-stamp.test.ts new file mode 100644 index 0000000..03dbae4 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.convergence-stamp.test.ts @@ -0,0 +1,182 @@ +/** + * engine.convergence-stamp.test.ts + * + * design/012 §6.2 / hazard H2 — the groundwork half of §13 T8d and T8e. + * (The relocation-driven halves land in Task 10, once relocateTo exists to arm + * the window; this task pins the ARM/CONSUME machinery on its own.) + * + * The stamp is ARMED, not unconditional, on purpose. The code's own comment at + * TerminalEngine.ts:2386-2388 explains why stamping unconditionally is avoided: + * a reactivation that resizes nothing would otherwise misattribute an unrelated + * `clear` days later. The armed form keeps that property exactly — an + * unchanged-geometry relocation produces no resize at all, so it never stamps. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge( + onResize?: (processId: string, cols: number, rows: number) => void, +): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: onResize ?? (() => {}), + }; +} + +function makeContainer(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + jest.useRealTimers(); + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +describe('design/012 §6.2 — the armed convergence stamp', () => { + it('does not stamp when nothing armed it', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-unarmed' }); + engine.mount(makeContainer()); + const entry = terminalCache.get('cs-unarmed')!; + entry.convergenceResizeAt = undefined; + + (engine as any).scheduleBackendResize(100, 30); + + expect(entry.convergenceResizeAt).toBeUndefined(); + }); + + it('stamps a debounced resize that lands inside the arm window', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-armed' }); + engine.mount(makeContainer()); + const entry = terminalCache.get('cs-armed')!; + entry.convergenceResizeAt = undefined; + + (engine as any).convergenceArmUntil = Date.now() + 500; + (engine as any).scheduleBackendResize(100, 30); + + expect(typeof entry.convergenceResizeAt).toBe('number'); + }); + + // §13 T8e, first clause: the arm is consumed ONCE. A hydrate() landing early in + // the window consumes it, so the relocation's own scheduleBackendResize does not + // re-stamp — harmless (the relocation resize lands at most ~620ms later: 500ms + // arm + BACKEND_RESIZE_DEBOUNCE_MS 120, comfortably inside ED3_EXPECT_WINDOW_MS + // 1500), but "consumes it once" must be asserted rather than assumed. + it('consumes the arm exactly once', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-once' }); + engine.mount(makeContainer()); + const entry = terminalCache.get('cs-once')!; + + (engine as any).convergenceArmUntil = Date.now() + 500; + (engine as any).scheduleBackendResize(100, 30); + const first = entry.convergenceResizeAt as number; + expect(typeof first).toBe('number'); + expect((engine as any).convergenceArmUntil).toBe(0); + + entry.convergenceResizeAt = undefined; + (engine as any).scheduleBackendResize(101, 30); + expect(entry.convergenceResizeAt).toBeUndefined(); + }); + + it('does not stamp once the arm window has expired', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-expired' }); + engine.mount(makeContainer()); + const entry = terminalCache.get('cs-expired')!; + entry.convergenceResizeAt = undefined; + + (engine as any).convergenceArmUntil = Date.now() - 1; + (engine as any).scheduleBackendResize(100, 30); + + expect(entry.convergenceResizeAt).toBeUndefined(); + }); + + // §13 T8e, second clause / review 093 B5: hydrate()'s pre-hydration resize + // (:2181) is the ONE direct sender that bypasses scheduleBackendResize. A + // relocation landing while an earlier attach()'s hydration is still awaiting + // would otherwise SIGWINCH the PTY inside the arm window with no stamp. + it('stamps from hydrate()\'s direct pre-hydration resize too', async () => { + jest.useFakeTimers(); + const engine = new TerminalEngine( + makeFakeBridge(), + { cacheKey: 'cs-hydrate' }, + ); + engine.mount(makeContainer()); + const entry = terminalCache.get('cs-hydrate')!; + entry.convergenceResizeAt = undefined; + + // Arm, then let attach() drive hydrate() -> :2181 bridge.resize -> :2183. + (engine as any).convergenceArmUntil = Date.now() + 500; + engine.attach('pid-hydrate'); + await jest.runAllTimersAsync(); + + expect(typeof entry.convergenceResizeAt).toBe('number'); + expect((engine as any).convergenceArmUntil).toBe(0); + }); + + // The refactor is behaviour-preserving: flushDeferredResizeOnActivation still + // stamps at BOTH of its existing sites (:2399 and :2407), unconditionally, + // independent of the arm. Rev 4 wrote "its existing site", singular; review 093 + // is right that there are two (design 012 correction 0.3.1). + it('keeps flushDeferredResizeOnActivation stamping at both of its sites', async () => { + // attach() kicks off hydrate()'s pre-hydration resize (:2295) WITHOUT the + // test awaiting it, which leaves resizeInFlight true and would otherwise + // mask the assertions below behind flushDeferredResizeOnActivation's own + // `|| this.resizeInFlight) return;` guard (:2553) — a guard this test does + // not intend to exercise. Fake timers + runAllTimersAsync let that + // in-flight hydration settle first, exactly as the hydrate test above does. + jest.useFakeTimers(); + + // Site A: a pending resize exists (parked at deactivation, timer cleared). + const a = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-site-a' }); + a.mount(makeContainer()); + a.attach('pid-a'); + await jest.runAllTimersAsync(); + const entryA = terminalCache.get('cs-site-a')!; + entryA.convergenceResizeAt = undefined; + (a as any).convergenceArmUntil = 0; // the arm plays no part here + (a as any).pendingResize = { cols: 120, rows: 40 }; + (a as any).flushDeferredResizeOnActivation(); + expect(typeof entryA.convergenceResizeAt).toBe('number'); + + // Site B: nothing pending, but lastSentSize disagrees with xterm. + const b = new TerminalEngine(makeFakeBridge(), { cacheKey: 'cs-site-b' }); + b.mount(makeContainer()); + b.attach('pid-b'); + await jest.runAllTimersAsync(); + const entryB = terminalCache.get('cs-site-b')!; + entryB.convergenceResizeAt = undefined; + (b as any).convergenceArmUntil = 0; + (b as any).pendingResize = null; + entryB.lastSentSize = { cols: 1, rows: 1 }; + (b as any).flushDeferredResizeOnActivation(); + expect(typeof entryB.convergenceResizeAt).toBe('number'); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.ended-region-lifetime.test.ts b/packages/terminal-core/src/__tests__/engine.ended-region-lifetime.test.ts new file mode 100644 index 0000000..fdf3d75 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.ended-region-lifetime.test.ts @@ -0,0 +1,143 @@ +/** + * engine.ended-region-lifetime.test.ts + * + * design/012 §2 "STEP 0", as an executable baseline. + * + * Two facts the relocation design is scoped from, neither of which was pinned + * anywhere before: + * + * 1. Ended-region marks do NOT survive a real remount. `mount()` constructs a + * brand-new EndedRegionTracker unconditionally (TerminalEngine.ts:1054-1055) + * and there is no persistence path — TerminalCacheEntry (cache.ts:17-105) has + * no regions/tracker field. So "relocation preserves the marks" is a NEW + * guarantee, not a regression fix (§2.3 item 1). + * + * 2. A second mount() WITHOUT an intervening unmount() LEAKS the live tracker: + * registerEndedRegionTracker uses Map.set (endedRegions.ts:57), which + * overwrites without disposing, so the previous tracker keeps its onRender + * subscription (endedRegions.ts:207-215) — which keeps it strongly reachable + * — and strands its `.ended-rail-layer` div (endedRegions.ts:197, removed + * only by dispose() at :423). Avoiding THAT leak is why P0-B owns the + * tracker; keeping the marks is the free consequence (§2.3 item 2). + * + * These are CHARACTERIZATION tests of shipped behaviour. They must stay green + * across P0-B: relocation must not change either of them, and §13 T5 is their + * contrast — the same assertions with relocateTo() instead of mount() must show + * ONE tracker, ONE subscription and the SAME regions. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +/** jsdom gives a real element; force a usable size so the >50px guards pass. */ +function makeContainer(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +/** Live (non-disposed) bottom-layer wash rows — the visible ended-region marks. */ +function liveWashRows(term: any): number[] { + return term.decorations + .filter((d: any) => !d.disposed && d.options.layer === 'bottom') + .map((d: any) => (d.options.marker as { line: number }).line) + .sort((a: number, b: number) => a - b); +} + +/** + * Drive the engine's real tracker path to produce ONE closed ended region: + * prompt (opens a span) -> a program ran -> prompt (closes it). + * Prompts arrive through the OSC 7 handler the engine registers at + * TerminalEngine.ts:1236, which calls endedRegions.onPrompt() at :1208. + */ +function plantRegion(engine: TerminalEngine, term: any, height = 5): void { + engine.setEndedRegionColors('#2a2a2a', '#7aa2f7'); + term.__setCursorLine(0); + term.oscHandlers[7](''); + engine.markProgramActive(); + term.__setCursorLine(height); + term.oscHandlers[7](''); +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +describe('design/012 §2 STEP 0 — the tracker lifetime P0-B inherits', () => { + it('loses every ended-region mark across a real unmount()/mount() cycle', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'step0-remount' }); + engine.mount(makeContainer()); + const term = terminalCache.get('step0-remount')!.terminal as any; + + plantRegion(engine, term); + expect(liveWashRows(term).length).toBeGreaterThan(0); + + // A real remount: pane collapse, cross-window detach, app reload. + engine.unmount(); + engine.mount(makeContainer()); + + // unmount() disposed the tracker (TerminalEngine.ts:3271) and mount() + // constructed a fresh one with `regions = []` (endedRegions.ts:185). + expect(liveWashRows(term)).toEqual([]); + }); + + it('leaks the live tracker when mount() is called again without unmount()', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'step0-leak' }); + engine.mount(makeContainer()); + const term = terminalCache.get('step0-leak')!.terminal as any; + + plantRegion(engine, term); + const subsAfterFirstMount = term.renderCallbacks.length; + const railLayersAfterFirstMount = + document.querySelectorAll('.ended-rail-layer').length; + expect(subsAfterFirstMount).toBeGreaterThan(0); + expect(railLayersAfterFirstMount).toBe(1); + + // The pattern TerminalEngine.ts:692-694 itself names as legitimate. + engine.mount(makeContainer()); + + // The SECOND tracker's onRender subscription is added; the FIRST one's is + // never disposed, because registerEndedRegionTracker overwrites the map + // entry without disposing (endedRegions.ts:57). That subscription is what + // keeps the dead tracker reachable. + expect(term.renderCallbacks.length).toBe(subsAfterFirstMount + 1); + + // And the first tracker's rail layer is stranded in the OLD wrapper: only + // dispose() removes it (endedRegions.ts:423), and nothing disposed it. + expect(document.querySelectorAll('.ended-rail-layer').length) + .toBeGreaterThanOrEqual(1); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.mount-foreign-surface.test.ts b/packages/terminal-core/src/__tests__/engine.mount-foreign-surface.test.ts new file mode 100644 index 0000000..054d664 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.mount-foreign-surface.test.ts @@ -0,0 +1,147 @@ +/** + * engine.mount-foreign-surface.test.ts + * + * design/012 §5.7 / §14 criterion 7 — external review 103 finding 2. + * + * `mount()` is append-only and `unmount()` deliberately leaves `term.element` in + * the DOM (the cache still owns the live Terminal, and a later mount reattaches + * it). Those two facts are individually correct and jointly leave a hole: when a + * pane node is REUSED for a different terminal id, the outgoing engine's surface + * is never removed, so the pane ends up hosting two `.xterm` elements — both + * full-height, the old one still painting backend output through its + * cache-lifetime subscription, and its cache entry pinned against eviction + * because `cache.ts:142` skips any entry whose element is still `isConnected`. + * + * P0-B makes this reachable on a supported path: the relocation cleanup returns + * engine A to the captured pane before engine B is installed in it. It is not + * caused by P0-B though — the bare mount/unmount/mount sequence below reproduces + * it on `develop` with no canvas involved, which is why the fix belongs in + * `mount()` rather than in the relocation cleanup. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +function makeHost(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + TerminalEngine.suppressHealUntil = 0; + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + jest.useRealTimers(); + TerminalEngine.suppressHealUntil = 0; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +function newEngine(cacheKey: string) { + return new TerminalEngine(makeFakeBridge(), { cacheKey, isMac: false }); +} + +const xtermsIn = (host: HTMLElement) => Array.from(host.querySelectorAll('.xterm')); + +describe('design/012 §14 criterion 7 — one engine\'s surface never lands in another\'s pane', () => { + it('drops a foreign surface when a pane node is reused for a different terminal id', + async () => { + jest.useFakeTimers(); + const pane = makeHost(); + + // Engine A takes the pane. + const a = newEngine('mount-foreign-a'); + a.mount(pane); + a.attach('pid-a'); + await jest.runAllTimersAsync(); + const aElement = terminalCache.get('mount-foreign-a')!.terminal.element!; + expect(xtermsIn(pane)).toEqual([aElement]); + + // A goes away, but unmount() deliberately leaves its element in the DOM. + a.unmount(); + expect(aElement.isConnected).toBe(true); + + // The SAME pane node is now reused for a different terminal id. + const b = newEngine('mount-foreign-b'); + b.mount(pane); + b.attach('pid-b'); + await jest.runAllTimersAsync(); + const bElement = terminalCache.get('mount-foreign-b')!.terminal.element!; + + // THE ASSERTION: exactly one surface in the pane, and it is B's. + expect(xtermsIn(pane)).toEqual([bElement]); + // …and A's is detached, which is what makes its cache entry eligible for + // the cap eviction at cache.ts:142 again. + expect(aElement.isConnected).toBe(false); + }); + + it('does not disturb its own surface when the same engine remounts into the same pane', + async () => { + jest.useFakeTimers(); + const pane = makeHost(); + + const a = newEngine('mount-foreign-same'); + a.mount(pane); + a.attach('pid-a'); + await jest.runAllTimersAsync(); + const aElement = terminalCache.get('mount-foreign-same')!.terminal.element!; + + a.unmount(); + // A tab switch back: same cache key, same pane node, reattach path. + const a2 = newEngine('mount-foreign-same'); + a2.mount(pane); + await jest.runAllTimersAsync(); + + // The cached Terminal is reused, so the very same element must still be + // there — the guard must key on identity, not on "is an .xterm". + expect(terminalCache.get('mount-foreign-same')!.terminal.element).toBe(aElement); + expect(xtermsIn(pane)).toEqual([aElement]); + expect(aElement.isConnected).toBe(true); + }); + + it('leaves non-terminal siblings in the pane alone', async () => { + jest.useFakeTimers(); + const pane = makeHost(); + const overlay = document.createElement('div'); + overlay.className = 'terminal-overlay'; + pane.appendChild(overlay); + + const a = newEngine('mount-foreign-siblings'); + a.mount(pane); + await jest.runAllTimersAsync(); + + expect(overlay.isConnected).toBe(true); + expect(pane.contains(overlay)).toBe(true); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.relocate-eligibility.test.ts b/packages/terminal-core/src/__tests__/engine.relocate-eligibility.test.ts new file mode 100644 index 0000000..c58d739 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.relocate-eligibility.test.ts @@ -0,0 +1,353 @@ +/** + * engine.relocate-eligibility.test.ts + * + * design/012 §5.4 (R3), §5.3 (the FT rule + the PARK invariant), §7.3 — + * §13 T2d, T10(b), T10c, T10d. + * + * T2d is the highest-priority regression in the whole list: an aborted outbound + * relocation must leave geometryEligible() EXACTLY as it was, because the + * alternative is a permanently disabled hidden-pane SIGWINCH park and a wiped + * codex scrollback. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +interface FakeBridgeOptions { + resize?: (processId: string, cols: number, rows: number) => void | Promise; +} + +function makeFakeBridge(opts: FakeBridgeOptions = {}): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: opts.resize ?? (() => {}), + }; +} + +function makeHost(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + TerminalEngine.suppressHealUntil = 0; + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + jest.useRealTimers(); + TerminalEngine.suppressHealUntil = 0; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +async function mountAttached(cacheKey: string) { + const resizeCalls: Array<[number, number]> = []; + const engine = new TerminalEngine( + makeFakeBridge({ resize: (_pid, c, r) => { resizeCalls.push([c, r]); } }), + { cacheKey, isMac: false }, + ); + const pane = makeHost(); + engine.mount(pane); + engine.attach('pid-1'); + await jest.runAllTimersAsync(); + const entry = terminalCache.get(cacheKey)!; + resizeCalls.length = 0; + return { + engine, pane, entry, + term: entry.terminal as any, + fit: entry.fitAddon as any, + resizeCalls, + }; +} + +describe('design/012 §5.4 R3 — eligibility lives INSIDE relocateTo', () => { + it('raises eligibility on the way out and lowers it on the way home', async () => { + jest.useFakeTimers(); + const { engine, pane } = await mountAttached('rel-elig-basic'); + engine.setActive(false); + expect((engine as any).surfaceDisplayed).toBe(false); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + expect((engine as any).surfaceDisplayed).toBe(true); + expect((engine as any).geometryEligible()).toBe(true); + + engine.relocateTo(pane, { paneChrome: true }); + expect((engine as any).surfaceDisplayed).toBe(false); + expect((engine as any).geometryEligible()).toBe(false); + }); + + // §13 T2d — reviews 093 B2 / 094 B4. An aborted outbound relocation must leave + // the ED3 guard intact, and it does so because the GATE is restored, not because + // any fit is cancelled. + it('leaves geometryEligible() exactly as it was after an abort, and a hidden pane ' + + 'still parks', async () => { + jest.useFakeTimers(); + const { engine, term, fit, resizeCalls } = await mountAttached('rel-elig-abort'); + engine.setActive(false); + expect((engine as any).geometryEligible()).toBe(false); + + // Force R6 to throw: a host that is a descendant of the moved element. + const illegal = document.createElement('div'); + term.element!.appendChild(illegal); + expect(engine.relocateTo(illegal, { paneChrome: false })).toBe('aborted'); + + expect((engine as any).surfaceDisplayed).toBe(false); + expect((engine as any).geometryEligible()).toBe(false); + + // …and the park is still armed: zero bridge.resize calls on a hidden pane. + fit.setNextFit(160, 24); + fit.fit(); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([]); + }); + + // §13 T10(b) / H3's regression test. A resize parked while hidden is delivered + // when the terminal is displayed on canvas, including when the relocation lands + // inside the 50ms fitTimer window. + it('delivers a resize parked while hidden once the surface is displayed on canvas', + async () => { + jest.useFakeTimers(); + const { engine, term, fit, resizeCalls } = await mountAttached('rel-elig-parked'); + + engine.setActive(false); + fit.setNextFit(160, 24); + fit.fit(); // xterm resizes; the backend resize parks + await jest.runAllTimersAsync(); + expect(term.cols).toBe(160); + expect(resizeCalls).toEqual([]); // parked: pendingResize set, no timer + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + await jest.runAllTimersAsync(); // R3's 50ms armActivationFit + 120ms debounce + + expect(resizeCalls).toEqual([[160, 24]]); + }); +}); + +describe('design/012 §5.3 — the FT rule and the PARK invariant under relocation', () => { + // §13 T10c — review 099 T1-F1's counterexample, verbatim, as a regression test. + // Fails against rev 5, which cancelled fitTimer in setSurfaceDisplayed(false). + it('rapid canvas-enter/canvas-exit on an inactive pane keeps the armed fit', async () => { + jest.useFakeTimers(); + const { engine, pane, fit, resizeCalls } = await mountAttached('rel-ft-t10c'); + + // The parked state flushBackendResize leaves at :2584-2589: pendingResize set, + // resizeTimer null. + engine.setActive(false); + fit.setNextFit(160, 24); + fit.fit(); + await jest.runAllTimersAsync(); + expect((engine as any).pendingResize).not.toBeNull(); + expect((engine as any).resizeTimer).toBeNull(); + expect(resizeCalls).toEqual([]); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); // R3 arms the 50ms fit + expect((engine as any).fitTimer).not.toBeNull(); + + // …and exit again INSIDE those 50ms. + engine.relocateTo(pane, { paneChrome: true }); + + // THE ASSERTION: a fit is still armed. setSurfaceDisplayed(false) never leaves + // the engine with none (the FT rule). Under rev 7 the return leg re-arms rather + // than merely preserving — either way a fit remains, which is what the rule is + // actually protecting; rev 5's cancel-and-leave-nothing is what it forbids. + expect((engine as any).fitTimer).not.toBeNull(); + + const fitsBefore = fit.fitCount; + await jest.runAllTimersAsync(); + // It fired and measured the PANE… + expect(fit.fitCount).toBeGreaterThan(fitsBefore); + // …flushDeferredResizeOnActivation early-returned on geometryEligible()… + expect(resizeCalls).toEqual([]); + // …so the parked value is still there (the PARK invariant: a designed state, + // not a strand). + expect((engine as any).pendingResize).not.toBeNull(); + + // And the next eligibility transition delivers exactly one resize. + engine.setActive(true); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([[160, 24]]); + }); + + // §13 T10d — the observable consequence of T10c, and the reason not-cancelling is + // BETTER rather than merely safe. Cancelling leaves xterm at the canvas node's + // grid while parked in the pane, until the tab is next activated. + // + // NOTE on the sequencing: this covers the SHORT visit, where the mechanism rev 6's + // §7.3 named — "the SURVIVING fitTimer", the one R3 armed on the way OUT — has not + // yet fired. The canvas node's grid is therefore applied SYNCHRONOUSLY here, + // modelling the canvas host's own ResizeObserver fit landing, so that timer is + // still pending when the terminal comes home. + // + // An earlier revision of this comment called the drained-timer variant + // "unreachable by design rather than by a bug". That was wrong, and external + // review 103 finding 1 caught it: draining is what a canvas visit longer than 50ms + // does, i.e. all of them. T10e directly below is that case, and rev 7's §7.2 row 4a + // is what makes it pass. Keep BOTH — they exercise different arms of the same rule. + it('a background pane returning from a differently sized node ends at the PANE grid', + async () => { + jest.useFakeTimers(); + const { engine, pane, term, fit } = await mountAttached('rel-ft-t10d'); + engine.setActive(false); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); // R3 arms the 50ms fit + expect((engine as any).fitTimer).not.toBeNull(); + + // The canvas node's own observer fit lands first, taking xterm to the NODE's + // grid, while the fit R3 armed is still pending. + fit.setNextFit(200, 50); // the canvas node's grid + fit.fit(); + expect(term.cols).toBe(200); + + // Home again, with NO tab activation anywhere in this test. + fit.setNextFit(80, 24); // what the pane measures + engine.relocateTo(pane, { paneChrome: true }); + expect((engine as any).fitTimer).not.toBeNull(); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(80); + }); + + // §13 T10e — external review 103 finding 1. T10d above covers the SHORT visit, + // where the outbound fitTimer is still pending when the surface comes home. That + // is the rare case. A real canvas session lasts longer than 50ms, so by the time + // the user leaves the canvas the outbound timer has long since fired and nulled + // itself, and there is no "surviving fitTimer" for §7.3 to lean on. + // + // Rev 6's §7.2 row 4 recorded and returned here, which left xterm at the CANVAS + // node's grid — parked in a pane it had never measured — until the tab was next + // activated. Rev 7 arms the settle fit on that return leg instead (row 4a). + it('a background pane returning after the outbound fit has already fired still ' + + 'ends at the PANE grid', async () => { + jest.useFakeTimers(); + const { engine, pane, term, fit, resizeCalls } = await mountAttached('rel-ft-t10e'); + engine.setActive(false); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fit.setNextFit(200, 50); // the canvas node's grid + await jest.runAllTimersAsync(); // the outbound fit fires AND NULLS + expect(term.cols).toBe(200); + expect((engine as any).fitTimer).toBeNull(); // nothing survives to fill the gap + // While displayed on canvas the terminal IS eligible, so the node's grid + // reaches the PTY — §6.1's normal case, not a park. + expect(resizeCalls).toEqual([[200, 50]]); + resizeCalls.length = 0; + + // Home again, with NO tab activation anywhere in this test. + fit.setNextFit(80, 24); // what the pane measures + engine.relocateTo(pane, { paneChrome: true }); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(80); + // …and the PARK invariant still holds: the pane is hidden, so the PTY is NOT + // SIGWINCH'd. The pane-sized resize is parked for the next activation. + expect(resizeCalls).toEqual([]); + expect((engine as any).pendingResize).not.toBeNull(); + + engine.setActive(true); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([[80, 24]]); + }); + + // §13 T10f — external review 105 (the CRITICAL). T10e's return-leg fit creates a + // parked pendingResize where rev 6 created none, and unmount()'s force bypass sent + // it to the still-hidden PTY. That is the one SIGWINCH §6.2 exists to prevent, and + // it is worse than an ordinary one: the ED3 detector that repairs a ESC[2J ESC[3J + // wipe is a per-mount disposable, disposed immediately after the flush, while the + // subscription that receives the wipe is cache-lifetime. So the answer lands with + // nothing armed to repair it. + // + // The fix keys on the VALUE's provenance, not the engine's state at teardown — + // the engine is ineligible at teardown in the shipped force case too. + it('unmount does NOT force a resize that was measured while the pane was ineligible', + async () => { + jest.useFakeTimers(); + const { engine, pane, term, fit, resizeCalls } = await mountAttached('rel-ft-t10f'); + engine.setActive(false); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fit.setNextFit(200, 50); + await jest.runAllTimersAsync(); + resizeCalls.length = 0; + + // Home again, still hidden. The return-leg fit parks the pane's grid. + fit.setNextFit(80, 24); + engine.relocateTo(pane, { paneChrome: true }); + await jest.runAllTimersAsync(); + expect(term.cols).toBe(80); + expect((engine as any).pendingResize).not.toBeNull(); + expect(resizeCalls).toEqual([]); + + // Teardown BEFORE any activation — a pane collapse, an in-place terminalId + // swap, a tab close. + engine.unmount(); + await jest.runAllTimersAsync(); + + // THE ASSERTION: nothing reached the PTY. + expect(resizeCalls).toEqual([]); + + // …and the geometry is DROPPED, not LOST. That distinction is the whole + // justification for suppressing the force flush, so assert it rather than + // asserting the drop alone: a new mount on the same cache key reattaches, + // re-measures the same pane, and delivers the size — this time with the + // per-mount ED3 detector armed to repair a ratatui/codex wipe. + const engine2 = new TerminalEngine( + makeFakeBridge({ resize: (_pid, c, r) => { resizeCalls.push([c, r]); } }), + { cacheKey: 'rel-ft-t10f', isMac: false }, + ); + fit.setNextFit(80, 24); + engine2.mount(pane); + engine2.attach('pid-1'); + engine2.setActive(true); + await jest.runAllTimersAsync(); + + expect(resizeCalls).toEqual([[80, 24]]); + }); + + // The other half of that rule, so the fix cannot be "solved" by disabling the force + // bypass altogether. This is the SHIPPED case the pane-collapse fix depends on: + // the value was measured while the pane was VISIBLE and merely interrupted + // mid-debounce by the hide + teardown. Teardown must still deliver it. + it('unmount DOES still force a resize that was measured while the pane was visible', + async () => { + jest.useFakeTimers(); + const { engine, fit, resizeCalls } = await mountAttached('rel-ft-t10f-visible'); + + fit.setNextFit(170, 40); + fit.fit(); // measured while ACTIVE and eligible + jest.advanceTimersByTime(20); // not yet past the 120ms debounce + engine.setActive(false); + engine.unmount(); + await jest.runAllTimersAsync(); + + expect(resizeCalls).toEqual([[170, 40]]); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.relocate-geometry.test.ts b/packages/terminal-core/src/__tests__/engine.relocate-geometry.test.ts new file mode 100644 index 0000000..6081e63 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.relocate-geometry.test.ts @@ -0,0 +1,275 @@ +/** + * engine.relocate-geometry.test.ts + * + * design/012 §6.1-§6.4 — §13 T7(b), T8(a)-(e), T9, T-multi. + * + * The changing case is the DEFAULT: a canvas node is not the same pixel box as a + * pane. Without the convergenceResizeAt stamp a ratatui/codex PTY answers the + * SIGWINCH with ESC[2J ESC[3J and the scrollback is wiped with no repair armed + * (D9 / H2 / §6.2) — silent data loss, and rev 3's §14 criterion 4 asserted the + * opposite. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +interface FakeBridgeOptions { + resize?: (processId: string, cols: number, rows: number) => void | Promise; +} + +function makeFakeBridge(opts: FakeBridgeOptions = {}): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: opts.resize ?? (() => {}), + }; +} + +function makeHost(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +/** ResizeObserver stub whose callback tests fire, standing in for the initial + * observe() callback the real one delivers after R7 re-arms it. */ +class CapturingResizeObserver { + static instances: CapturingResizeObserver[] = []; + cb: ResizeObserverCallback; + constructor(cb: ResizeObserverCallback) { + this.cb = cb; + CapturingResizeObserver.instances.push(this); + } + observe(): void {} + disconnect(): void {} + unobserve(): void {} +} + +function fireLatestObserver(): void { + const inst = CapturingResizeObserver.instances[CapturingResizeObserver.instances.length - 1]; + inst.cb([] as unknown as ResizeObserverEntry[], inst as unknown as ResizeObserver); +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + TerminalEngine.suppressHealUntil = 0; + CapturingResizeObserver.instances = []; + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = CapturingResizeObserver; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + jest.useRealTimers(); + TerminalEngine.suppressHealUntil = 0; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +async function mountAttached(cacheKey: string, resize?: FakeBridgeOptions['resize']) { + const resizeCalls: Array<[number, number]> = []; + const engine = new TerminalEngine( + makeFakeBridge({ + resize: resize ?? ((_pid, c, r) => { resizeCalls.push([c, r]); }), + }), + { cacheKey, isMac: false }, + ); + const pane = makeHost(); + engine.mount(pane); + engine.attach('pid-1'); + await jest.runAllTimersAsync(); + const entry = terminalCache.get(cacheKey)!; + resizeCalls.length = 0; + entry.convergenceResizeAt = undefined; + return { + engine, pane, entry, + term: entry.terminal as any, + fit: entry.fitAddon as any, + resizeCalls, + }; +} + +describe('design/012 §6 — geometry across a relocation', () => { + // §13 T8(a) + §6.3. The unchanged case: proposeDimensions() matches, so fit() + // does not call term.resize(); no onResize, no scheduleBackendResize, no + // SIGWINCH, no stamp. + it('unchanged geometry sends nothing to the PTY and never stamps', async () => { + jest.useFakeTimers(); + const { engine, entry, resizeCalls } = await mountAttached('geo-t8a'); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fireLatestObserver(); // the observer's initial callback + await jest.runAllTimersAsync(); + + expect(resizeCalls).toEqual([]); + expect(entry.convergenceResizeAt).toBeUndefined(); + }); + + // §13 T8(b) + T8(d). The changing case: exactly ONE resize, at the NEW dims, and + // it is STAMPED so the ED3 detector at :1261-1274 opens its 1500ms repair window. + it('changed geometry sends exactly one stamped resize at the new dimensions', async () => { + jest.useFakeTimers(); + const { engine, term, fit, entry, resizeCalls } = await mountAttached('geo-t8b'); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fit.setNextFit(140, 36); // the canvas node's grid + fireLatestObserver(); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(140); + expect(resizeCalls).toEqual([[140, 36]]); + expect(typeof entry.convergenceResizeAt).toBe('number'); + }); + + // §13 T7(b). A geometry-CHANGING relocation runs the live onResize handler, + // which cancels the capture mark and sets suppressUntilSubmit (:1112-1116). That + // is the existing, correct reflow behaviour — an absolute row/col mark is + // untranslatable after reflow — and relocation must not defeat it. + it('cancels the capture mark on a geometry-changing relocation', async () => { + jest.useFakeTimers(); + const { engine, term, fit } = await mountAttached('geo-t7b'); + term.__setCursor(4, 0); + (engine as any).capture.noteUserKey(); + expect((engine as any).capture.hasMark()).toBe(true); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fit.setNextFit(140, 36); + fireLatestObserver(); + await jest.runAllTimersAsync(); + + expect((engine as any).capture.hasMark()).toBe(false); + expect((engine as any).suppressUntilSubmit).toBe(true); + }); + + // §13 T8(c) / H7. mount() would fail this: unmount()'s forced + // flushBackendResize(true) sets resizeInFlight synchronously (:2593) and clears + // it only in a .finally() (:2603), which always defers at least one microtask, so + // the flag is GUARANTEED still true through an immediately following synchronous + // mount() — whose reconcile is gated on `&& !this.resizeInFlight && + // !this.pendingResize` (:1963-1964). relocateTo never steps in it because it + // never calls unmount() and never uses that reconcile (§6.4). + it('still delivers the new size with a resize already in flight', async () => { + jest.useFakeTimers(); + let release: () => void = () => {}; + const resizeCalls: Array<[number, number]> = []; + const { engine, term, fit } = await mountAttached('geo-t8c', (_pid, c, r) => { + resizeCalls.push([c, r]); + return new Promise((res) => { release = res; }); + }); + + // Put a real bridge.resize in flight: fit -> onResize -> debounce -> flush. + fit.setNextFit(100, 30); + fit.fit(); + jest.advanceTimersByTime(200); + expect((engine as any).resizeInFlight).toBe(true); + resizeCalls.length = 0; + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + fit.setNextFit(140, 36); + fireLatestObserver(); + release(); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(140); + expect(resizeCalls).toEqual([[140, 36]]); + }); + + // §13 T8(e) / review 096. stampConvergenceIfArmed consumes the arm on its FIRST + // call, so a hydrate() at :2181 landing early in the 500ms window consumes it and + // the relocation's own scheduleBackendResize does NOT re-stamp. Harmless — the + // relocation resize lands at most ~620ms later (500ms arm + 120ms debounce), + // comfortably inside ED3_EXPECT_WINDOW_MS = 1500 — but "consumes it once" alone + // does not prove it, so assert the WINDOW, not just the consumption. + it('a hydrate inside the arm window stamps once, and the relocation SIGWINCH still ' + + 'falls inside that window', async () => { + jest.useFakeTimers(); + const { engine, entry, fit, resizeCalls } = await mountAttached('geo-t8e'); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); // arms 500ms + engine.attach('pid-hydrate-2'); // hydrate -> :2181 -> stamp + await jest.runAllTimersAsync(); + const stampedAt = entry.convergenceResizeAt as number; + expect(typeof stampedAt).toBe('number'); + expect((engine as any).convergenceArmUntil).toBe(0); + + // The relocation's own resize now lands WITHOUT re-stamping… + resizeCalls.length = 0; + fit.setNextFit(140, 36); + fireLatestObserver(); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([[140, 36]]); + expect(entry.convergenceResizeAt).toBe(stampedAt); + // …and it is still inside the window the earlier stamp opened. + expect(Date.now() - stampedAt).toBeLessThan(1500); + }); + + // §13 T9 — the judgement call (§15.1 / D12). A pendingResize armed BEFORE the + // move is neither cancelled nor duplicated: all five scheduleBackendResize + // callers pass xterm's OWN live dims, which are host-independent, so delivering a + // surviving pending value after the move is correct rather than stale. + it('neither cancels nor duplicates a resize pending across the move', async () => { + jest.useFakeTimers(); + const { engine, fit, resizeCalls } = await mountAttached('geo-t9'); + + fit.setNextFit(150, 40); + fit.fit(); // arms pendingResize + the 120ms timer + expect((engine as any).pendingResize).toEqual({ cols: 150, rows: 40 }); + + const host = makeHost(); + engine.relocateTo(host, { paneChrome: false }); + await jest.runAllTimersAsync(); + + expect(resizeCalls).toEqual([[150, 40]]); // exactly one, at xterm's live dims + }); + + // §13 T-multi / H9. Two engines relocating in the same synchronous block do not + // interfere: the operation is per-engine with no shared mutable state, and the one + // shared structure it touches (terminalCache) is read and mutated per key. + // Entering Canvas Mode does this for EVERY terminal in the workspace at once — + // O(all terminals), not O(visible), because RC4 forbids culling from relocating. + // The N-large case stays a manual gate (§13). + it('two engines relocate in the same synchronous block without interfering', async () => { + jest.useFakeTimers(); + const a = await mountAttached('geo-multi-a'); + const b = await mountAttached('geo-multi-b'); + const hostA = makeHost(); + const hostB = makeHost(); + + expect(a.engine.relocateTo(hostA, { paneChrome: false })).toBe('relocated'); + expect(b.engine.relocateTo(hostB, { paneChrome: false })).toBe('relocated'); + + expect(hostA.contains(a.term.element!)).toBe(true); + expect(hostB.contains(b.term.element!)).toBe(true); + expect(a.term.element!.isConnected).toBe(true); + expect(b.term.element!.isConnected).toBe(true); + expect(terminalCache.get('geo-multi-a')!.terminal).toBe(a.term); + expect(terminalCache.get('geo-multi-b')!.terminal).toBe(b.term); + + // b's observer is the most recently created one. + b.fit.setNextFit(90, 20); + fireLatestObserver(); + await jest.runAllTimersAsync(); + + expect(b.resizeCalls).toEqual([[90, 20]]); + expect(a.resizeCalls).toEqual([]); // a is untouched by b's fit + expect(a.term.cols).toBe(80); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.relocate-side-effects.test.ts b/packages/terminal-core/src/__tests__/engine.relocate-side-effects.test.ts new file mode 100644 index 0000000..e73d9a0 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.relocate-side-effects.test.ts @@ -0,0 +1,250 @@ +/** + * engine.relocate-side-effects.test.ts + * + * design/012 §5.2 (R1), §5.9 (R8), §5.10 (R9), §5.11 (R10), §5.12 (R11) — + * §13 T5, T6 (call-site half), T7(a), T12, T17e (relocation half), T22a. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +function makeHost(width = 800, height = 600): { wrapper: HTMLElement; display: HTMLElement } { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const display = document.createElement('div'); + display.className = 'terminal-display'; + Object.defineProperty(display, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(display, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(display, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(display); + document.body.appendChild(wrapper); + return { wrapper, display }; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +function mounted(cacheKey: string, opts: Record = {}) { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey, isMac: false, ...opts }); + const pane = makeHost(); + engine.mount(pane.display); + const entry = terminalCache.get(cacheKey)!; + return { engine, pane, entry, term: entry.terminal as any }; +} + +/** Drive the engine's real tracker path to produce ONE closed ended region: + * prompt (opens a span) -> a program ran -> prompt (closes it). Prompts arrive + * through the OSC 7 handler the engine registers at TerminalEngine.ts:1236, + * which calls endedRegions.onPrompt() at :1208. */ +function plantRegion(engine: TerminalEngine, term: any, height = 5): void { + engine.setEndedRegionColors('#2a2a2a', '#7aa2f7'); + term.__setCursorLine(0); + term.oscHandlers[7](''); + engine.markProgramActive(); + term.__setCursorLine(height); + term.oscHandlers[7](''); +} + +describe('design/012 §5.9 R8 — the ended-region rail follows the terminal', () => { + // §13 T5. The CONTRAST with Task 1's characterization test: mount() constructs a + // second tracker and strands the first; relocateTo constructs none. + it('reuses the SAME tracker — no second instance, no extra onRender subscription', () => { + const { engine, term } = mounted('rse-t5'); + plantRegion(engine, term); + const trackerBefore = (engine as any).endedRegions; + const subsBefore = term.renderCallbacks.length; + const regionsBefore = trackerBefore.regionCount(); + + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect((engine as any).endedRegions).toBe(trackerBefore); + expect(term.renderCallbacks.length).toBe(subsBefore); + expect(trackerBefore.regionCount()).toBe(regionsBefore); + }); + + // §13 T6, call-site half: the SAME .ended-rail-layer node ends up in the NEW + // wrapper. Ordering is load-bearing — R8 must run AFTER R6, because wrapper + // resolution walks UP from term.element. + it('moves the same rail-layer node into the new wrapper', () => { + const { engine, pane, term } = mounted('rse-t6'); + plantRegion(engine, term); + + const layer = pane.wrapper.querySelector('.ended-rail-layer'); + expect(layer).not.toBeNull(); + + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect(host.wrapper.querySelector('.ended-rail-layer')).toBe(layer); + expect(pane.wrapper.querySelector('.ended-rail-layer')).toBeNull(); + }); +}); + +describe('design/012 §5.2 / §5.10 R1+R9 — focus is restored only if it was owned', () => { + // §13 T12, first clause. Spike 004 Q3 measured that the blur is SYNCHRONOUS and + // part of the move, and that a same-task .focus() restores activeElement before + // any paint with exactly one focus/focusin pair. + it('re-focuses the terminal when it owned focus, within the same call', () => { + const { engine, term } = mounted('rse-t12-owned'); + // Model "focus lives inside term.element" the way real xterm does — its helper + // textarea is a descendant of the element being moved. + const textarea = document.createElement('textarea'); + term.element!.appendChild(textarea); + textarea.focus(); + expect(term.element!.contains(document.activeElement)).toBe(true); + + const focusBefore = term.focusCount; + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect(term.focusCount).toBe(focusBefore + 1); + }); + + // §13 T12, second clause. A background pane relocated onto canvas must not steal + // focus — which is exactly what mount()'s autoFocus path (:1869-1871) would do, + // and why §5.0 forbids THAT. + it('does not focus a terminal that did not own focus', () => { + const { engine, term } = mounted('rse-t12-unowned'); + const elsewhere = document.createElement('input'); + document.body.appendChild(elsewhere); + elsewhere.focus(); + + const focusBefore = term.focusCount; + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect(term.focusCount).toBe(focusBefore); + expect(document.activeElement).toBe(elsewhere); + }); +}); + +describe('design/012 §5.11 R10 — the suggest gate flips with the chrome mode', () => { + // §13 T17e, relocation half. + it('closes the popup state and stops emitting on the way out', () => { + const emitted: string[] = []; + const { engine } = mounted('rse-r10-out', { + onInputLineChanged: (t: string) => emitted.push(t), + }); + engine.setSuggestPopupState('passive'); + expect((engine as any).suggestState).toBe('passive'); + + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect((engine as any).paneChromeActive).toBe(false); + expect((engine as any).suggestState).toBe('closed'); + (engine as any).emitInputLine('git st'); + expect(emitted).toEqual([]); + }); + + // The dedup at :2930 must not swallow the first line after the return trip, so + // R10 resets lastEmittedInput UNCONDITIONALLY — in both directions. + it('resets the emit dedup so the first line after the return trip is not swallowed', () => { + const emitted: string[] = []; + const { engine, pane } = mounted('rse-r10-back', { + onInputLineChanged: (t: string) => emitted.push(t), + }); + (engine as any).emitInputLine('git status'); + expect(emitted).toEqual(['git status']); + + const host = makeHost(); + engine.relocateTo(host.display, { paneChrome: false }); + engine.relocateTo(pane.display, { paneChrome: true }); + + expect((engine as any).paneChromeActive).toBe(true); + expect((engine as any).lastEmittedInput).toBe(''); + (engine as any).emitInputLine('git status'); + expect(emitted).toEqual(['git status', 'git status']); + }); +}); + +describe('design/012 §5.12 R11 — the capture instance is reused outright', () => { + // §13 T7(a). HeuristicCapture holds `private mark` and a `readonly term` + // (commandCapture.ts:58-65) — ZERO DOM references, no listeners, no timers. + // Nothing binds it to a container, so an unchanged-geometry relocation preserves + // both the instance and its mark. (The geometry-CHANGING case, where the live + // onResize handler cancels the mark, is Task 10's T7(b).) + it('preserves the capture instance and its mark across a relocation', () => { + const { engine, term } = mounted('rse-r11'); + term.__setCursor(4, 0); + (engine as any).capture.noteUserKey(); + const captureBefore = (engine as any).capture; + const markBefore = captureBefore.getMark(); + expect(markBefore).not.toBeNull(); + + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + expect((engine as any).capture).toBe(captureBefore); + expect((engine as any).capture.getMark()).toEqual(markBefore); + }); +}); + +describe('design/012 D19 / H14 — §13 T22a: why the pointer gate is necessary', () => { + /** + * The half of T22 that IS implementable. jsdom has no layout engine and no hit + * testing, so `pointer-events: none` cannot be asserted here at all — that half + * stays the manual gate §13 already lists (plan ground-truth correction G3). + * + * What this test proves instead is the NON-CIRCULAR claim D19 rests on: xterm + * binds its own "always on" mousedown to term.element itself + * (CoreBrowserTerminal.ts:602-604 bindMouse / :779-781 `ev.preventDefault(); + * this.focus();`), that listener TRAVELS WITH THE ELEMENT, and relocateTo cannot + * touch it. So declining to wire the ENGINE's container click listener removes + * only the 4px-padding supplement — it does NOT deliver design 012 §8's + * "click-to-focus: Absent" row. Only the host's pointer gate can, and that gate + * lives in design/010 (§12). + * + * terminal-core's xterm mock does not reproduce bindMouse, so the listener is + * installed explicitly and labelled as a model of it. + */ + it('cannot remove a listener bound to term.element, even with paneChrome false', () => { + const { engine, term } = mounted('rse-t22a'); + let elementMouseDowns = 0; + // MODELS @xterm/xterm CoreBrowserTerminal.ts:779-781, which the mock omits. + term.element!.addEventListener('mousedown', () => { + elementMouseDowns += 1; + term.focus(); + }); + + const host = makeHost(); + expect(engine.relocateTo(host.display, { paneChrome: false })).toBe('relocated'); + + const focusBefore = term.focusCount; + term.element!.dispatchEvent(new MouseEvent('mousedown', { bubbles: true })); + + expect(elementMouseDowns).toBe(1); + expect(term.focusCount).toBe(focusBefore + 1); + // …while the engine's OWN container listener is correctly absent. + host.display.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusBefore + 1); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.relocate.test.ts b/packages/terminal-core/src/__tests__/engine.relocate.test.ts new file mode 100644 index 0000000..ae2a15e --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.relocate.test.ts @@ -0,0 +1,347 @@ +/** + * engine.relocate.test.ts + * + * design/012 §5 R0-R7 — §13 T1, T2, T2b, T2c, T3 (canvas half), T4, T8f, T13 + * (relocation half). + * + * The central invariant: relocateTo moves ONLY xterm's own `term.element`, in one + * synchronous `appendChild`, and is NOT mount(). Per the DOM spec, appendChild of + * an already-parented node is remove-then-insert inside ONE synchronous algorithm; + * spike 004 Q2 measured that no observer of any kind (MutationObserver, + * ResizeObserver, a microtask queued immediately before the call, a synchronous + * read) ever sees isConnected === false. + * + * H1 is the worst failure mode in the file and it is what §5.0 exists to avoid: + * mount()'s reattach catch-all (TerminalEngine.ts:757-763) deletes the cache entry + * on ANY thrown error and falls through to CREATE at :767 — a brand-new blank + * Terminal, the entire scrollback gone, silently. relocateTo never deletes and + * never creates: it returns 'aborted' and re-wires the previous container WITH ITS + * RECORDED CHROME MODE. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +/** A wrapper > display pair, matching TerminalDisplay's real structure and the + * host contract design 012 D17 requires of a canvas node. */ +function makeHost(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +class CountingResizeObserver { + static instances: CountingResizeObserver[] = []; + static disconnects = 0; + static observed: Element[] = []; + constructor(public cb: ResizeObserverCallback) { + CountingResizeObserver.instances.push(this); + } + observe(el: Element): void { CountingResizeObserver.observed.push(el); } + disconnect(): void { CountingResizeObserver.disconnects += 1; } + unobserve(): void {} +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + CountingResizeObserver.instances = []; + CountingResizeObserver.disconnects = 0; + CountingResizeObserver.observed = []; + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = CountingResizeObserver; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +function mounted(cacheKey: string, opts: Record = {}) { + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey, + isMac: false, + ...opts, + }); + const pane = makeHost(); + engine.mount(pane); + const entry = terminalCache.get(cacheKey)!; + return { engine, pane, entry, term: entry.terminal as any }; +} + +describe('design/012 §5 — relocateTo, the move', () => { + // §13 T1 + §5.0's exclusion table, as executable assertions. + it('keeps the same Terminal, the same cache entry, and touches none of mount()\'s eight things', + () => { + const { engine, pane, entry, term } = mounted('rel-t1', { fontSize: 14 }); + const host = makeHost(); + + const terminalBefore = entry.terminal; + const entryBefore = terminalCache.get('rel-t1'); + const trackerBefore = (engine as any).endedRegions; + const captureBefore = (engine as any).capture; + const fontBefore = term.options.fontSize; + const focusBefore = term.focusCount; + const cacheSizeBefore = terminalCache.size; + + expect(term.element!.isConnected).toBe(true); + expect(engine.relocateTo(host, { paneChrome: false })).toBe('relocated'); + expect(term.element!.isConnected).toBe(true); + + expect(host.contains(term.element!)).toBe(true); + expect(pane.contains(term.element!)).toBe(false); + + // Same session, same objects — no mount(), no unmount(), no dispose(). + expect(terminalCache.get('rel-t1')).toBe(entryBefore); + expect(terminalCache.get('rel-t1')!.terminal).toBe(terminalBefore); + expect(terminalCache.size).toBe(cacheSizeBefore); + // No second EndedRegionTracker, no second HeuristicCapture (§5.0 rows 1-2). + expect((engine as any).endedRegions).toBe(trackerBefore); + expect((engine as any).capture).toBe(captureBefore); + // fontSize untouched (§5.0 row 3 / H4): re-applying it would revert every + // zoom since engine creation, because `opts` is frozen at :678. + expect(term.options.fontSize).toBe(fontBefore); + // No unconditional focus (§5.0 row 4): a background pane relocated onto + // canvas must not steal focus. + expect(term.focusCount).toBe(focusBefore); + }); + + // §13 T2 / H1. `entry.terminal !== this.term` is the precondition that keeps + // relocation away from mount()'s catch-all, which deletes the entry and creates + // a blank Terminal. + it('aborts and mutates nothing when the preconditions fail', () => { + const { engine, pane, term } = mounted('rel-t2'); + const host = makeHost(); + const entry = terminalCache.get('rel-t2')!; + + // A foreign Terminal in the entry: the cache no longer describes this engine. + const foreign = { element: document.createElement('div') } as any; + entry.terminal = foreign; + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('aborted'); + + // Nothing moved, nothing was deleted, and no new Terminal was constructed. + expect(pane.contains(term.element!)).toBe(true); + expect(terminalCache.get('rel-t2')).toBe(entry); + expect(terminalCache.get('rel-t2')!.terminal).toBe(foreign); + expect(CountingResizeObserver.disconnects).toBe(0); + }); + + it('aborts when the cache entry is missing', () => { + const { engine, pane, term } = mounted('rel-t2-missing'); + const host = makeHost(); + terminalCache.delete('rel-t2-missing'); + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('aborted'); + expect(pane.contains(term.element!)).toBe(true); + }); + + it('aborts for a mirror engine before doing anything at all', () => { + const { engine, pane, term } = mounted('rel-mirror', { mirror: true }); + const host = makeHost(); + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('aborted'); + expect(pane.contains(term.element!)).toBe(true); + }); + + // §13 T2b — the identity no-op. Both §4.2.2 cleanups call this redundantly BY + // DESIGN, so it must be free. + it('is a free no-op when the container is already the current one', () => { + const { engine, pane, entry } = mounted('rel-t2b'); + const disposablesBefore = (engine as any).containerDisposables; + const epochBefore = (engine as any).resizeEpoch; + const armBefore = (engine as any).convergenceArmUntil; + const eligibleBefore = (engine as any).surfaceDisplayed; + const observerBefore = (engine as any).resizeObserver; + const disconnectsBefore = CountingResizeObserver.disconnects; + + expect(engine.relocateTo(pane, { paneChrome: true })).toBe('relocated'); + + expect((engine as any).containerDisposables).toBe(disposablesBefore); + expect((engine as any).resizeEpoch).toBe(epochBefore); + expect((engine as any).convergenceArmUntil).toBe(armBefore); + expect((engine as any).surfaceDisplayed).toBe(eligibleBefore); + expect((engine as any).resizeObserver).toBe(observerBefore); + expect(CountingResizeObserver.disconnects).toBe(disconnectsBefore); + expect(entry.containerDisposables).toBe(disposablesBefore); + }); + + // The documented call shape. Rev 5 read `opts.paneChrome` bare in R3, which + // threw TypeError here (reviews 098 C1 + 096, independently). + it('accepts the no-options call and defaults paneChrome to false', () => { + const { engine, term } = mounted('rel-optional'); + const host = makeHost(); + + expect(engine.relocateTo(host)).toBe('relocated'); + expect(host.contains(term.element!)).toBe(true); + expect((engine as any).paneChromeActive).toBe(false); + }); + + // §13 T2c / review 094 B5. appendChild throws HierarchyRequestError if the + // target is INSIDE term.element — reachable if a canvas node ever registers a + // host that is a descendant of the terminal. appendChild leaves the tree + // unchanged when it throws, so the element is still in snap.container, and R4/R5 + // have already run: the abort must re-wire, WITH THE RECORDED CHROME MODE. + it('restores the previous container AND its chrome mode when appendChild throws', () => { + let openSearchCalls = 0; + const { engine, pane, term } = mounted('rel-t2c', { + onOpenSearch: () => { openSearchCalls += 1; }, + }); + + // A host that is a descendant of the element we are trying to move. + const illegal = document.createElement('div'); + term.element!.appendChild(illegal); + + expect(engine.relocateTo(illegal, { paneChrome: false })).toBe('aborted'); + + // The element never left. + expect(pane.contains(term.element!)).toBe(true); + // And the PANE's four listeners work again, at the chrome mode it had. + const focusBefore = term.focusCount; + pane.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusBefore + 1); + pane.dispatchEvent(new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true })); + expect(openSearchCalls).toBe(1); + expect((engine as any).containerDisposables.length).toBe(4); + expect((engine as any).paneChromeActive).toBe(true); + }); + + // The direction review 094 B5 caught rev 4 getting WRONG: on an aborted + // canvas -> pane move, snap.container IS the canvas host, and re-wiring it with a + // hardcoded `paneChrome: true` would install Ctrl/Cmd+F on it — so ^F would call + // onOpenSearch, render the bar in the off-screen pane and autofocus its input + // (TerminalSearchBar.tsx:39-41), pulling focus out of the canvas. + it('restores a CANVAS container without pane chrome after a failed return trip', () => { + let openSearchCalls = 0; + const { engine, term } = mounted('rel-t2c-canvas', { + onOpenSearch: () => { openSearchCalls += 1; }, + }); + const host = makeHost(); + expect(engine.relocateTo(host, { paneChrome: false })).toBe('relocated'); + expect((engine as any).paneChromeActive).toBe(false); + + const illegal = document.createElement('div'); + term.element!.appendChild(illegal); + expect(engine.relocateTo(illegal, { paneChrome: true })).toBe('aborted'); + + expect((engine as any).paneChromeActive).toBe(false); + const focusBefore = term.focusCount; + host.dispatchEvent(new MouseEvent('click', { bubbles: true })); + host.dispatchEvent(new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true })); + expect(term.focusCount).toBe(focusBefore); // no click-to-focus on a canvas host + expect(openSearchCalls).toBe(0); // and no Ctrl/Cmd+F + }); + + // §13 T8f / review 096. Without the restore, an abort leaves a <=500ms arm on an + // engine that never moved, so an unrelated scheduleBackendResize stamps + // convergenceResizeAt and opens a spurious 1500ms ED3 repair window — exactly the + // misattribution TerminalEngine.ts:2386-2388 avoids. + it('restores convergenceArmUntil on abort', () => { + const { engine, term, entry } = mounted('rel-t8f'); + expect((engine as any).convergenceArmUntil).toBe(0); + + const illegal = document.createElement('div'); + term.element!.appendChild(illegal); + expect(engine.relocateTo(illegal, { paneChrome: false })).toBe('aborted'); + + expect((engine as any).convergenceArmUntil).toBe(0); + entry.convergenceResizeAt = undefined; + (engine as any).scheduleBackendResize(100, 30); + expect(entry.convergenceResizeAt).toBeUndefined(); + }); + + // §13 T3, canvas half + T13, relocation half. The four listeners follow the + // move; on a chromeless host only the two zoom bindings are wired, because + // 010:376 keeps zoom as "existing behaviour, unchanged" while D16 removes + // click-to-focus and Ctrl/Cmd+F. + it('re-wires the container listeners on the new host, gated by paneChrome', () => { + let openSearchCalls = 0; + let zoomCalls = 0; + const { engine, pane, term } = mounted('rel-t3', { + onOpenSearch: () => { openSearchCalls += 1; }, + onZoom: () => { zoomCalls += 1; }, + }); + const host = makeHost(); + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('relocated'); + expect((engine as any).containerDisposables.length).toBe(2); + + // The OLD container is inert. + const focusAfterMove = term.focusCount; + pane.dispatchEvent(new MouseEvent('click', { bubbles: true })); + pane.dispatchEvent(new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true })); + pane.dispatchEvent(new KeyboardEvent('keydown', { key: '=', ctrlKey: true, bubbles: true })); + expect(term.focusCount).toBe(focusAfterMove); + expect(openSearchCalls).toBe(0); + expect(zoomCalls).toBe(0); + + // The NEW host: zoom in BOTH modes, chrome in neither. + host.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusAfterMove); // D16: no click-to-focus + host.dispatchEvent(new KeyboardEvent('keydown', { key: 'f', ctrlKey: true, bubbles: true })); + expect(openSearchCalls).toBe(0); // D16: no Ctrl/Cmd+F + host.dispatchEvent(new KeyboardEvent('keydown', { key: '=', ctrlKey: true, bubbles: true })); + expect(zoomCalls).toBe(1); // zoom keys: present + host.dispatchEvent(new WheelEvent('wheel', { deltaY: -1, ctrlKey: true, bubbles: true })); + expect(zoomCalls).toBe(2); // modifier+wheel: present + + // …and the return trip restores full pane chrome. + expect(engine.relocateTo(pane, { paneChrome: true })).toBe('relocated'); + expect((engine as any).containerDisposables.length).toBe(4); + pane.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(term.focusCount).toBe(focusAfterMove + 1); + }); + + // §13 T4 / D7. Exactly one disconnect, and the new one observes the new host. + it('disconnects the previous ResizeObserver exactly once and observes the new host', () => { + const { engine } = mounted('rel-t4'); + const host = makeHost(); + const disconnectsBefore = CountingResizeObserver.disconnects; + CountingResizeObserver.observed = []; + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('relocated'); + + expect(CountingResizeObserver.disconnects).toBe(disconnectsBefore + 1); + expect(CountingResizeObserver.observed).toEqual([host]); + expect((engine as any).resizeObserver).not.toBeNull(); + }); + + // The xterm/addon subscriptions are bound to the SURVIVING Terminal and must be + // untouched — that is the whole point of the D6 split, and it is what keeps + // boundTerm.onResize (:1101-1120) alive across the operation. + it('leaves every xterm subscription intact across the move', () => { + const { engine, term } = mounted('rel-subs'); + const host = makeHost(); + + const dataSubs = term.dataCallbacks.length; + const resizeSubs = term.resizeCallbacks.length; + const renderSubs = term.renderCallbacks.length; + + expect(engine.relocateTo(host, { paneChrome: false })).toBe('relocated'); + + expect(term.dataCallbacks.length).toBe(dataSubs); + expect(term.resizeCallbacks.length).toBe(resizeSubs); + expect(term.renderCallbacks.length).toBe(renderSubs); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.suggest-gate.test.ts b/packages/terminal-core/src/__tests__/engine.suggest-gate.test.ts new file mode 100644 index 0000000..75ae0f6 --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.suggest-gate.test.ts @@ -0,0 +1,155 @@ +/** + * engine.suggest-gate.test.ts + * + * design/012 §5.11 + §8.1 — §13 T17e. + * + * Why a GATE and not a one-shot close (review 093 B3). Without it: + * emitInputLine -> opts.onInputLineChanged -> useCommandSuggest's + * onInputLineChanged (useCommandSuggest.ts:40-58) re-opens on the next matching + * input line, calling setSuggestPopupState('passive') (:57) and + * setState({ open: true … anchor }) (:58). From that moment the engine intercepts + * the popup key set instead of forwarding it, because the interception is gated + * ONLY on `this.suggestState !== 'closed'` (TerminalEngine.ts:1346). So after the + * user submits one command on a canvas node, arrow keys, Tab and Enter stop + * reaching the shell — while the popup is drawn inside the OFF-SCREEN pane, + * anchored by getCursorPixelPosition() (:2963-2976) which reads `this.container` + * (:2965), now the canvas host. Doubly wrong, and a broken terminal. + * + * The gate makes design 012 §8's "Suggest popup: cannot open" true BY + * CONSTRUCTION: while paneChromeActive is false the engine emits no input lines, + * so the hook never re-opens, so suggestState never leaves 'closed', so no key is + * ever claimed. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +function makeFakeBridge(): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: () => {}, + }; +} + +function makeContainer(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = class { + observe(): void {} + disconnect(): void {} + unobserve(): void {} + }; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +describe('design/012 §8.1 — the suggest gate', () => { + it('emits input lines normally while pane chrome is active', () => { + const emitted: string[] = []; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'sg-on', + onInputLineChanged: (t) => emitted.push(t), + }); + engine.mount(makeContainer()); + + (engine as any).emitInputLine('git st'); + expect(emitted).toEqual(['git st']); + }); + + it('emits nothing at all while pane chrome is inactive', () => { + const emitted: string[] = []; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'sg-off', + onInputLineChanged: (t) => emitted.push(t), + }); + engine.mount(makeContainer()); + (engine as any).paneChromeActive = false; + + (engine as any).emitInputLine('git st'); + (engine as any).emitInputLine('git status'); + expect(emitted).toEqual([]); + }); + + // Belt-and-braces for any future caller: even a direct setSuggestPopupState + // cannot raise the state on a chromeless host. + it('refuses to leave the closed state while pane chrome is inactive', () => { + const engine = new TerminalEngine(makeFakeBridge(), { cacheKey: 'sg-state' }); + engine.mount(makeContainer()); + (engine as any).paneChromeActive = false; + + engine.setSuggestPopupState('passive'); + expect((engine as any).suggestState).toBe('closed'); + + (engine as any).paneChromeActive = true; + engine.setSuggestPopupState('passive'); + expect((engine as any).suggestState).toBe('passive'); + }); + + // The consequence that makes the terminal usable: with the popup unable to open, + // the key interception at :1346 never claims Up/Down/Tab/Enter. + it('never claims a popup key while pane chrome is inactive', () => { + const actions: unknown[] = []; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'sg-keys', + onSuggestAction: (a) => actions.push(a), + }); + engine.mount(makeContainer()); + const term = terminalCache.get('sg-keys')!.terminal as any; + + (engine as any).paneChromeActive = false; + engine.setSuggestPopupState('passive'); // guarded to 'closed' + + for (const key of ['ArrowUp', 'ArrowDown', 'Tab', 'Enter']) { + const handled = term.keyHandler( + new KeyboardEvent('keydown', { key, bubbles: true }), + ); + // `true` means "xterm, you handle it" — i.e. the engine did NOT claim it. + expect(handled).not.toBe(false); + } + expect(actions).toEqual([]); + }); + + // The dedup at :2930 must not swallow the first line after the return trip. + // R10 resets lastEmittedInput unconditionally for exactly this reason (Task 8). + it('emits again once pane chrome is restored', () => { + const emitted: string[] = []; + const engine = new TerminalEngine(makeFakeBridge(), { + cacheKey: 'sg-return', + onInputLineChanged: (t) => emitted.push(t), + }); + engine.mount(makeContainer()); + + (engine as any).emitInputLine('git st'); + (engine as any).paneChromeActive = false; + (engine as any).emitInputLine('git status'); // swallowed by the gate + (engine as any).paneChromeActive = true; + (engine as any).lastEmittedInput = ''; // what R10 does on the way home + (engine as any).emitInputLine('git status'); + + expect(emitted).toEqual(['git st', 'git status']); + }); +}); diff --git a/packages/terminal-core/src/__tests__/engine.surface-displayed.test.ts b/packages/terminal-core/src/__tests__/engine.surface-displayed.test.ts new file mode 100644 index 0000000..889a03e --- /dev/null +++ b/packages/terminal-core/src/__tests__/engine.surface-displayed.test.ts @@ -0,0 +1,322 @@ +/** + * engine.surface-displayed.test.ts + * + * design/012 §7 (gates AND transitions) + §5.3's FT rule — §13 T10(a), T11. + * + * `paneActive` means "the owning TAB is visible" (TerminalDisplay.tsx:207, + * :374-376) — it is NOT focus. A canvas-displayed terminal from a BACKGROUND tab + * has paneActive === false but is genuinely visible, so all six geometry gates + * must run. `geometryEligible()` is `paneActive || surfaceDisplayed`. + * + * Gates alone are insufficient: setActive is not just a setter. setActive(false) + * cancels fitTimer and returns without flushing (:2345-2350), and only + * setActive(true) arms the 50ms settle fit that then calls + * flushDeferredResizeOnActivation() (:2356-2369). Without an equivalent, a resize + * parked while the tab was hidden stays parked forever when the terminal becomes + * visible only through canvas. + */ + +import { TerminalEngine } from '../TerminalEngine'; +import { terminalCache } from '../cache'; +import type { TerminalBridge, Disposable } from '../types'; + +interface FakeBridgeOptions { + resize?: (processId: string, cols: number, rows: number) => void | Promise; + getSize?: (processId: string) => Promise<{ cols: number; rows: number }>; +} + +function makeFakeBridge(opts: FakeBridgeOptions = {}): TerminalBridge { + const noopDisposable: Disposable = { dispose() {} }; + return { + onData: () => noopDisposable, + onExit: () => noopDisposable, + write: () => {}, + resize: opts.resize ?? (() => {}), + getSize: opts.getSize, + }; +} + +function makeContainer(width = 800, height = 600): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const el = document.createElement('div'); + el.className = 'terminal-display'; + Object.defineProperty(el, 'offsetWidth', { value: width, configurable: true }); + Object.defineProperty(el, 'offsetHeight', { value: height, configurable: true }); + Object.defineProperty(el, 'offsetParent', { value: document.body, configurable: true }); + wrapper.appendChild(el); + document.body.appendChild(wrapper); + return el; +} + +/** ResizeObserver stub that captures each instance's callback so tests can fire it. */ +class CapturingResizeObserver { + static instances: CapturingResizeObserver[] = []; + cb: ResizeObserverCallback; + constructor(cb: ResizeObserverCallback) { + this.cb = cb; + CapturingResizeObserver.instances.push(this); + } + observe(): void {} + disconnect(): void {} + unobserve(): void {} +} + +function fireResizeObserver(): void { + const inst = CapturingResizeObserver.instances[CapturingResizeObserver.instances.length - 1]; + inst.cb([] as unknown as ResizeObserverEntry[], inst as unknown as ResizeObserver); +} + +let prevRO: unknown; + +beforeEach(() => { + terminalCache.clear(); + TerminalEngine.suppressHealUntil = 0; + CapturingResizeObserver.instances = []; + prevRO = (globalThis as any).ResizeObserver; + (globalThis as any).ResizeObserver = CapturingResizeObserver; +}); + +afterEach(() => { + terminalCache.clear(); + document.body.innerHTML = ''; + jest.useRealTimers(); + TerminalEngine.suppressHealUntil = 0; + if (prevRO === undefined) delete (globalThis as any).ResizeObserver; + else (globalThis as any).ResizeObserver = prevRO; +}); + +async function mountAttached(cacheKey: string) { + const resizeCalls: Array<[number, number]> = []; + const bridge = makeFakeBridge({ + resize: (_pid, c, r) => { + resizeCalls.push([c, r]); + }, + }); + const engine = new TerminalEngine(bridge, { cacheKey }); + engine.mount(makeContainer()); + engine.attach('pid-1'); + await jest.runAllTimersAsync(); + + const entry = terminalCache.get(cacheKey)!; + const term = entry.terminal as any; + const fit = entry.fitAddon as any; + resizeCalls.length = 0; // drop mount/hydrate baseline sizing + return { engine, term, fit, resizeCalls }; +} + +describe('design/012 §7 — surfaceDisplayed gates every geometry path paneActive gated', () => { + // §13 T10(a), gates. A hidden tab whose terminal is displayed on canvas must do + // ALL its geometry work: observer fit, xterm resize, and the PTY SIGWINCH. + it('runs the observer fit and the backend resize on a hidden pane that is surface-displayed', + async () => { + jest.useFakeTimers(); + const { engine, term, fit, resizeCalls } = await mountAttached('sd-gates'); + + engine.setActive(false); // the owning TAB goes to the background + engine.setSurfaceDisplayed(true); // …but the surface is live on a canvas node + fit.setNextFit(160, 24); + fireResizeObserver(); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(160); + expect(resizeCalls).toEqual([[160, 24]]); + }); + + // The mirror image, unchanged from today: hidden AND not surface-displayed + // still parks. This is the ED3 scrollback-wipe guard; it must not regress. + it('still parks a hidden pane that is NOT surface-displayed', async () => { + jest.useFakeTimers(); + const { engine, term, fit, resizeCalls } = await mountAttached('sd-park'); + + engine.setActive(false); + fit.setNextFit(160, 24); + fireResizeObserver(); + await jest.runAllTimersAsync(); + + expect(term.cols).toBe(80); + expect(resizeCalls).toEqual([]); + }); + + // §12 §7.2 row 2: a false->true eligibility transition arms the SAME 50ms + // settle fit setActive(true) arms, so a resize parked while hidden is delivered + // when the terminal becomes visible ONLY through canvas. + it('flushes a parked resize when the surface becomes displayed', async () => { + jest.useFakeTimers(); + const { engine, term, fit, resizeCalls } = await mountAttached('sd-flush'); + + engine.setActive(false); + fit.setNextFit(160, 24); + fireResizeObserver(); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([]); // parked + + engine.setSurfaceDisplayed(true); + await jest.runAllTimersAsync(); // 50ms settle fit + 120ms debounce + + expect(term.cols).toBe(160); + expect(resizeCalls).toEqual([[160, 24]]); + }); + + // §7.2 row 1: already eligible => record and return. A second settle fit is churn. + it('does not arm a second settle fit when the pane was already eligible', async () => { + jest.useFakeTimers(); + const { engine, fit } = await mountAttached('sd-idempotent'); + + engine.setActive(true); + await jest.runAllTimersAsync(); + const before = fit.fitCount; + + engine.setSurfaceDisplayed(true); + await jest.runAllTimersAsync(); + + expect(fit.fitCount).toBe(before); + }); + + // §7.2 row 5 / the FT rule: hiding the TAB must not kill the canvas's settle fit. + it('setActive(false) does NOT cancel fitTimer while surface-displayed', async () => { + jest.useFakeTimers(); + const { engine, fit } = await mountAttached('sd-ft-keep'); + + engine.setActive(false); + engine.setSurfaceDisplayed(true); // arms the 50ms fit (false->true transition) + expect((engine as any).fitTimer).not.toBeNull(); + + engine.setActive(false); // tab hidden again — must NOT cancel + expect((engine as any).fitTimer).not.toBeNull(); + + const before = fit.fitCount; + await jest.runAllTimersAsync(); + expect(fit.fitCount).toBeGreaterThan(before); + }); + + // §7.2 row 6 / the FT rule: THE ONE CANCEL. Unchanged shipped behaviour, with + // its own stated reason at TerminalEngine.ts:2343-2344 — an activation fit + // scheduled 50ms before a quick tab switch away must not resize a hidden pane. + it('setActive(false) DOES cancel fitTimer when not surface-displayed', async () => { + jest.useFakeTimers(); + const { engine } = await mountAttached('sd-ft-cancel'); + + engine.setActive(true); + expect((engine as any).fitTimer).not.toBeNull(); + + engine.setActive(false); + expect((engine as any).fitTimer).toBeNull(); + }); + + // §7.2 rows 3 AND 4a. Rev 5 CANCELLED in row 4 "to mirror setActive(false)", which + // contradicted D10 outright (review 099 T1-F1 + 098 B1, found independently). + // The FT rule as rev 7 states it: a property of the END STATE, not a prohibition + // on clearTimeout. setSurfaceDisplayed(false) must never leave the engine with no + // pending fit — by preserving one (visible pane) or by arming a fresh one + // (background pane, §7.2 row 4a). Rev 6 phrased this as "never cancels", which a + // bare record-and-return satisfies while producing exactly the state the rule + // forbids; that phrasing is what let review 103 finding 1 through. + it('setSurfaceDisplayed(false) always leaves a fit pending, visible tab or not', async () => { + jest.useFakeTimers(); + + const visible = await mountAttached('sd-ft-nocancel-visible'); + visible.engine.setActive(true); + expect((visible.engine as any).fitTimer).not.toBeNull(); + visible.engine.setSurfaceDisplayed(false); + expect((visible.engine as any).fitTimer).not.toBeNull(); + + const hidden = await mountAttached('sd-ft-nocancel-hidden'); + hidden.engine.setActive(false); + hidden.engine.setSurfaceDisplayed(true); // arms the fit + expect((hidden.engine as any).fitTimer).not.toBeNull(); + hidden.engine.setSurfaceDisplayed(false); // paneActive false — row 4a re-arms + expect((hidden.engine as any).fitTimer).not.toBeNull(); + + // …and it is still armed after the outbound one would have expired, which is + // the half rev 6 could not deliver. + await jest.advanceTimersByTimeAsync(200); + hidden.engine.setSurfaceDisplayed(true); + await jest.advanceTimersByTimeAsync(200); + expect((hidden.engine as any).fitTimer).toBeNull(); // fired and cleared itself + hidden.engine.setSurfaceDisplayed(false); + expect((hidden.engine as any).fitTimer).not.toBeNull(); + }); +}); + +describe('design/012 §13 T11 — the paneActive tripwire', () => { + // A TRIPWIRE, not a correctness test: it asserts a SOURCE-TEXT property so a + // future edit cannot quietly reintroduce a bare `paneActive` gate at one of the + // six sites §7.1 enumerates. Reworded per review 096: count gate READS, not + // total occurrences — the predicate's own declaration and setActive's + // precondition read are legitimate extra occurrences. + it('has no bare paneActive gate left at any of the six geometry sites', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const fs = require('fs') as typeof import('fs'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const path = require('path') as typeof import('path'); + const src = fs.readFileSync( + path.join(__dirname, '..', 'TerminalEngine.ts'), + 'utf8', + ); + + // The six gates now read through the predicate. + const eligibleReads = (src.match(/this\.geometryEligible\(\)/g) ?? []).length; + expect(eligibleReads).toBeGreaterThanOrEqual(6); + + // Count gate reads in CODE only. §13 T11 is explicit — "count gate READS, or + // drop the count" — because a raw substring count over the whole file also + // counts prose. That version of this test made a doc comment able to fail the + // suite, and the first thing it did was make someone reword a comment to + // satisfy a test, which is the tail wagging the dog. Comments are stripped + // first so the assertion tracks the thing it is actually guarding. + const code = src + .replace(/\/\*[\s\S]*?\*\//g, '') // block comments, including JSDoc + .replace(/(^|[^:])\/\/.*$/gm, '$1'); // line comments, but not the // in a URL + // `this.paneActive` survives in code ONLY as: the constructor seed, its write + // in `setActive`, and `geometryEligible()`'s own read. Everything else must + // have moved to the predicate. + const paneActiveReads = (code.match(/this\.paneActive/g) ?? []).length; + expect(paneActiveReads).toBeLessThanOrEqual(4); + + // And specifically: no `!this.paneActive` early-return survives anywhere. + expect(src).not.toContain('if (!this.paneActive) return'); + }); +}); + +/** + * External review 103, finding 3 — the two 50ms timers share one slot. + * + * `armActivationFit` and `setFontSize` both write `this.fitTimer`, so whichever + * arms second replaces the first. Before the fix `setFontSize` installed a + * FIT-ONLY callback, which silently dropped the deferred-resize flush that the + * activation callback carries — stranding a parked `pendingResize` with no + * remaining path to deliver it, because `healOnce` also refuses to run while + * `pendingResize` is set. + * + * Reachable only on this branch: §7.1 row 3 widened setFontSize's gate from + * `paneActive` to `geometryEligible()`, so a canvas-displayed terminal on a + * BACKGROUND tab now gets past the early return for the first time. + */ +describe('design/012 §7 — setFontSize must not replace the flush-bearing timer', () => { + it('flushes a resize parked while hidden, even when a font change lands inside the 50ms window', + async () => { + jest.useFakeTimers(); + const { engine, resizeCalls, fit } = await mountAttached('ft-fontsize-strand'); + + // A background tab: not active, so a resize parks rather than sending. + engine.setActive(false); + resizeCalls.length = 0; + fit.setNextFit(160, 24); + fit.fit(); + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([]); // parked, as designed + + // Displayed on canvas: eligibility goes false->true and arms the ONLY + // callback that will flush that parked value. + engine.setSurfaceDisplayed(true); + expect((engine as any).fitTimer).not.toBeNull(); + + // A font change inside the 50ms window. It re-arms the shared slot. + engine.setFontSize(15); + + // The parked resize must still be delivered. + await jest.runAllTimersAsync(); + expect(resizeCalls).toEqual([[160, 24]]); + }); +}); diff --git a/packages/terminal-core/src/cache.ts b/packages/terminal-core/src/cache.ts index 37ad7c4..6cbaac4 100644 --- a/packages/terminal-core/src/cache.ts +++ b/packages/terminal-core/src/cache.ts @@ -38,6 +38,24 @@ export interface TerminalCacheEntry { // Declared now; consumed by Task 4 (R1 output delivery). lastHydratedProcessId?: string; disposables: Array<() => void>; + /** + * Teardowns for the four DOM listeners bound to the CURRENT container — the + * only things in the engine tied to the container rather than to the terminal + * (design 012 D6 / §5.5): click-to-focus, capture-phase zoom keydown, + * Ctrl/Cmd+F, and modifier+wheel zoom. + * + * Split out of `disposables` so `relocateTo()` can tear down the OLD + * container's bindings without touching the xterm/addon subscriptions, which + * are bound to the surviving `Terminal` and must stay live across the move. + * Keeping `boundTerm.onResize` alive is what makes relocation immune to the + * orphaned-resize class documented by engine.remount-resize.test.ts:12-20. + * + * REQUIRED, not optional, mirroring `disposables` above: the compiler then + * enforces both `terminalCache.set` literals instead of a `?? []` at the read + * site. The ResizeObserver is deliberately NOT here — it has exactly one + * owner, `TerminalEngine.resizeObserver` (design 012 D7). + */ + containerDisposables: Array<() => void>; // Spec §17 R1: cache-lifetime bridge subscriptions. Created in mount (first time // for a cacheKey), disposed ONLY in cleanupTerminalCache/dispose — never in unmount(). // Declared now; consumed by Task 4 (R1 output delivery). @@ -175,6 +193,17 @@ export const cleanupTerminalCache = (terminalId: string) => { } }); + // design 012 §5.5 site 7: the container listeners live in their own array + // now, and a cache teardown must run them too or the four DOM listeners + // outlive the terminal they were bound to. + cached.containerDisposables.forEach(dispose => { + try { + dispose(); + } catch (e) { + console.warn(`terminal-core/cache: Error disposing container disposable for ${terminalId}:`, e); + } + }); + // Spec §17 R1: dispose the cache-lifetime bridge subscriptions if present. if (cached.dataDisposable) { try { diff --git a/packages/terminal-core/src/endedRegions.test.ts b/packages/terminal-core/src/endedRegions.test.ts index cbcc828..af43ee0 100644 --- a/packages/terminal-core/src/endedRegions.test.ts +++ b/packages/terminal-core/src/endedRegions.test.ts @@ -616,3 +616,103 @@ describe('reflow', () => { expect(t.regionCount()).toBe(2); }); }); + +describe('design/012 §5.9 — retargetRail after the xterm element moves', () => { + /** A wrapper > display pair, matching TerminalDisplay's real structure. */ + function makeHost(): { wrapper: HTMLElement; display: HTMLElement } { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const display = document.createElement('div'); + display.className = 'terminal-display'; + wrapper.appendChild(display); + document.body.appendChild(wrapper); + return { wrapper, display }; + } + + afterEach(() => { + document.body.innerHTML = ''; + }); + + // §13 T6, first clause. Moving the memoised layer carries every child railEl + // with it in ONE DOM operation. Rebuilding it would leak the old layer and — via + // each Region's memoised railEl (:126) plus ensureRail's early return (:569-570) + // — stop existing rails ever reappearing. + it('moves the SAME rail-layer node into the new wrapper', () => { + const a = makeHost(); + const b = makeHost(); + const term = newTerm(80); + term.open(a.display); + + const t = new EndedRegionTracker(term); + t.setColors('#2a2a2a', '#7aa2f7'); + t.onPrompt(); + t.markProgramActive(); + term.__setCursorLine(5); + t.onPrompt(); + + const layer = a.wrapper.querySelector('.ended-rail-layer'); + expect(layer).not.toBeNull(); + const railsBefore = layer!.children.length; + expect(railsBefore).toBeGreaterThan(0); + + // The move relocateTo's R6 performs, then R8. + b.display.appendChild(term.element!); + t.retargetRail(); + + expect(b.wrapper.querySelector('.ended-rail-layer')).toBe(layer); + expect(a.wrapper.querySelector('.ended-rail-layer')).toBeNull(); + expect(layer!.children.length).toBe(railsBefore); + expect(t.regionCount()).toBe(1); + }); + + // Lazy creation: ensureRailLayer() has not run yet, so there is nothing to move + // and retargetRail must be a silent no-op — the layer resolves the new wrapper + // on first paint instead. + it('does nothing when no rail layer has been created yet', () => { + const a = makeHost(); + const b = makeHost(); + const term = newTerm(80); + term.open(a.display); + + const t = new EndedRegionTracker(term); // no colours => no rail => no layer + b.display.appendChild(term.element!); + expect(() => t.retargetRail()).not.toThrow(); + expect(document.querySelectorAll('.ended-rail-layer').length).toBe(0); + }); + + // §13 T6, second clause / design 012 correction 0.3.10: rev 4 specified a bare + // requestAnimationFrame with no cancellation. The handle is stored and cancelled + // by dispose(), so a torn-down tracker cannot repaint on the next frame. + it('cancels its follow-up rAF on dispose()', () => { + const a = makeHost(); + const b = makeHost(); + const term = newTerm(80); + term.open(a.display); + + const cancelled: number[] = []; + const realRaf = globalThis.requestAnimationFrame; + const realCancel = globalThis.cancelAnimationFrame; + (globalThis as any).requestAnimationFrame = (_cb: FrameRequestCallback) => 4242; + (globalThis as any).cancelAnimationFrame = (h: number) => { cancelled.push(h); }; + + try { + const t = new EndedRegionTracker(term); + t.setColors('#2a2a2a', '#7aa2f7'); + t.onPrompt(); + t.markProgramActive(); + term.__setCursorLine(5); + t.onPrompt(); + + b.display.appendChild(term.element!); + t.retargetRail(); + expect((t as any).retargetRaf).toBe(4242); + + t.dispose(); + expect(cancelled).toContain(4242); + expect((t as any).retargetRaf).toBeUndefined(); + } finally { + (globalThis as any).requestAnimationFrame = realRaf; + (globalThis as any).cancelAnimationFrame = realCancel; + } + }); +}); diff --git a/packages/terminal-core/src/endedRegions.ts b/packages/terminal-core/src/endedRegions.ts index a8a8e3f..0763b7b 100644 --- a/packages/terminal-core/src/endedRegions.ts +++ b/packages/terminal-core/src/endedRegions.ts @@ -195,6 +195,9 @@ export class EndedRegionTracker { private resizeTimer: ReturnType | undefined; private pendingWiden = false; private railLayer: HTMLElement | undefined; + /** Handle for retargetRail's follow-up reposition frame, so dispose() can cancel + * it (design 012 §5.9 / correction 0.3.10). Undefined when none is armed. */ + private retargetRaf: number | undefined; private readonly renderSub: IDisposable | undefined; constructor(private readonly term: Terminal, opts: EndedRegionOptions = {}) { @@ -417,6 +420,10 @@ export class EndedRegionTracker { clearTimeout(this.resizeTimer); this.resizeTimer = undefined; } + if (this.retargetRaf !== undefined) { + cancelAnimationFrame(this.retargetRaf); + this.retargetRaf = undefined; + } this.renderSub?.dispose(); for (const r of this.regions) this.disposeRegion(r); this.regions = []; @@ -553,6 +560,42 @@ export class EndedRegionTracker { el.style.display = 'block'; } + /** + * Move the memoised rail layer into the wrapper that now contains term.element. + * + * MUST run AFTER the xterm element has been appended to its new container + * (design 012 §5.9, ordering R8-after-R6): wrapper resolution walks UP from + * term.element, so before the move it would resolve the OLD wrapper. + * + * Moving the layer carries every child region.railEl with it in one DOM + * operation. Do NOT drop and rebuild it: each Region memoises `railEl` (:126) + * and ensureRail returns early when it is set (:569-570), so a rebuild leaks the + * old layer AND stops existing rails ever reappearing. + * + * The extra frame answers review 089: onRender (:207-215) fires on GRID changes + * only, so an idle terminal relocated into a host whose layout settles a frame + * later would otherwise get no repositioning trigger at all — there is no + * ResizeObserver on the wrapper and no output to render. + */ + retargetRail(): void { + const layer = this.railLayer; + if (!layer) return; // created lazily by ensureRailLayer() on first paint, + // which will resolve the NEW wrapper by itself + const el = this.term.element; + if (!el) return; + const wrapper = el.closest(WRAPPER_SELECTOR) ?? el.parentElement; + if (wrapper && wrapper !== layer.parentElement) { + wrapper.appendChild(layer); + } + this.positionRails(); + if (typeof requestAnimationFrame !== 'function') return; + if (this.retargetRaf !== undefined) cancelAnimationFrame(this.retargetRaf); + this.retargetRaf = requestAnimationFrame(() => { + this.retargetRaf = undefined; + this.positionRails(); + }); + } + /** Create the rail layer in the terminal's outer wrapper, once the DOM exists. */ private ensureRailLayer(): HTMLElement | undefined { if (this.railLayer) return this.railLayer; diff --git a/packages/terminal-core/src/index.ts b/packages/terminal-core/src/index.ts index a8fb70e..b769ec5 100644 --- a/packages/terminal-core/src/index.ts +++ b/packages/terminal-core/src/index.ts @@ -1,4 +1,5 @@ export { TerminalEngine, DEFAULT_THEME } from './TerminalEngine'; +export type { RelocationResult } from './TerminalEngine'; export { terminalCache, pasteToTerminal, diff --git a/src/renderer/components/Terminal/TerminalDisplay.tsx b/src/renderer/components/Terminal/TerminalDisplay.tsx index 62449dc..dd6cba7 100644 --- a/src/renderer/components/Terminal/TerminalDisplay.tsx +++ b/src/renderer/components/Terminal/TerminalDisplay.tsx @@ -9,6 +9,7 @@ import { TerminalSearchBar } from './TerminalSearchBar'; import { CommandSuggestPopup } from './CommandSuggestPopup'; import { ScrollToBottomButton } from './ScrollToBottomButton'; import { useCommandSuggest } from './useCommandSuggest'; +import { useSurfaceRelocation } from './useSurfaceRelocation'; import { commandHistoryService } from '../../services/commandHistoryService'; import { getCwdSnapshot } from '../../services/cwdSnapshot'; import { inputHandler } from '../../services/InputHandler'; @@ -173,10 +174,53 @@ export const TerminalDisplay: React.FC = ({ const suggestRef = useRef(suggest); suggestRef.current = suggest; + // Canvas Mode surface relocation (design 012 §4.2). Placed here because its + // callbacks close over dispatch (:85), setContextMenu (:123), setPathPicker + // (:126), setSchemaPicker (:133) and suggestRef (:173), all declared above. + // `engineMounted` is a stable useCallback the engine effect below calls right + // after mount() — that bump is what makes relocation-at-mount reachable at all + // (hazard H12, measured by spike 004 Q1). + const { engineMounted } = useSurfaceRelocation({ + terminalId, + engineRef, + paneRef: terminalRef, + onRelocated: (toCanvas) => { + // The suggest popup's REACT state — the engine's own gate (design 012 §8.1) + // is what stops it coming back while relocated. Only on the way out: the + // return trip should be able to re-open it normally. + if (toCanvas) suggestRef.current.close(); + // ContextMenu portals to with position: fixed at literal x/y + // (ContextMenu.tsx:63, :67), so a menu opened before the move floats at a + // viewport point unrelated to the terminal. Same for both pickers. + setContextMenu(null); + setPathPicker(null); + setSchemaPicker(null); + // The SEARCH BAR is deliberately left open with its state intact (§8): it + // holds user-typed query/caseSensitive/wholeWord/regex + // (TerminalSearchBar.tsx:27-30) and the SearchAddon's highlights live on the + // buffer and travel with term.element. Closing it would call clearSearch() + // and discard their query. + }, + onAborted: () => { + // design 012 §5.1's recovery contract. The engine is fully restored and the + // terminal is still usable in its previous container; the surface-host + // registration is left alone, so the canvas node shows an empty box. + dispatch(addToast({ message: 'Could not move this terminal', type: 'error' })); + }, + }); + // Create the engine + mount it once per terminalId. Reattach existing process // when available. Cleanup → unmount() (NOT dispose — preserve the cache). useEffect(() => { - if (!terminalRef.current) return; + // CAPTURED, not re-read at cleanup time: on a whole-component deletion React + // detaches host refs (terminalRef.current = null) during the deletion traversal, + // which runs BEFORE passive deletion cleanup. Rev 5 of design 012 guarded the + // cleanup on `terminalRef.current` and that guard was FALSE on exactly the + // interleaving it was written for, so the fallback cover relocated nothing + // (review 099 T1-F3). The captured element is still a real — now detached — div, + // which is all appendChild needs. + const pane = terminalRef.current; + if (!pane) return; // Ensure the host-level pipeline-healed suppressor is registered once. ensurePipelineHealSuppression(); @@ -300,7 +344,8 @@ export const TerminalDisplay: React.FC = ({ }); engineRef.current = engine; - engine.mount(terminalRef.current); + engine.mount(pane); // the identical element captured at the top of this effect + engineMounted(); // ADDED — the relocation dep (design 012 §4.2.1) setAtBottom(engine.isScrolledToBottom()); const scrollPositionDisposable = engine.onScrollPosition(setAtBottom); // Scope this pane's slack/scrollbar background to its own effective scheme @@ -320,6 +365,23 @@ export const TerminalDisplay: React.FC = ({ return () => { scrollPositionDisposable.dispose(); + // The ORDERED FALLBACK (design 012 §4.2.2). The relocation effect's LAYOUT + // cleanup is the PRIMARY cover and the only one that returns the element to a + // CONNECTED node; this one runs later, in the passive phase, and lands it in a + // detached pane div — the same place today's every remount already leaves it. + // Whichever runs second is a free R0 identity no-op. + // + // It must run BEFORE unmount(): unmount() disposes every subscription, removes + // the rail layer and nulls this.container, but it NEVER removes term.element + // from the DOM (TerminalEngine.ts:3218-3276) — so without this, a pane teardown + // while displayed on canvas strands a live-painting, input-dead surface in the + // canvas host with nothing to reclaim it (hazard H11). + // + // Both bindings are captured: `engine` is this effect's own local, and `pane` + // was captured at the top of this effect body — NOT re-read here, because + // React has already nulled `terminalRef.current` by the time a deletion's + // passive cleanup runs (review 099 T1-F3). + engine.relocateTo(pane, { paneChrome: true }); engine.unmount(); engineRef.current = null; }; diff --git a/src/renderer/components/Terminal/__tests__/canvasHostContract.test.ts b/src/renderer/components/Terminal/__tests__/canvasHostContract.test.ts new file mode 100644 index 0000000..f11ff8e --- /dev/null +++ b/src/renderer/components/Terminal/__tests__/canvasHostContract.test.ts @@ -0,0 +1,199 @@ +/** + * @jest-environment jsdom + * + * design/012 §4.4 (D17) + row 8 (D19) — §13 T16, T22b. + * + * Because only term.element moves, everything scoped to its FORMER ANCESTORS has + * to be reproduced by the canvas host. Four independent things break without D17's + * shape (`.terminal-display-wrapper > .terminal-display[data-terminal-id]` with a + * real layout box): + * 1. 15 CSS rules scoped under `.terminal-display` (TerminalDisplay.css) — + * without the class the WebGL scratch-canvas sliver bug returns (:80-92) and + * the grid loses its 8px rail gutter (:30-35). + * 2. The global Ctrl+C guard, `activeElement.closest('.terminal-display')` + * (InputHandler.ts:268-269). Its sibling branch + * `activeElement.classList.contains('xterm')` does NOT save this — the focused + * node is xterm's helper TEXTAREA, not `.xterm`. + * 3. The rail layer's `.closest('.terminal-display-wrapper')` + * (endedRegions.ts:560, selector const at :100). + * 4. FitAddon.proposeDimensions(), which reads term.element.parentElement — i.e. + * the HOST itself and never the wrapper (spike 004 Q4, measured). + */ +import { isEditableNonTerminalTarget } from '../../../services/inputTargets'; +import { setPaneBackgroundVar } from '../../../store/terminalTheme'; + +/** The pane's structure, as TerminalDisplay.tsx:542-549 renders it. */ +function makePane(id: string): { wrapper: HTMLElement; display: HTMLElement } { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper'; + const display = document.createElement('div'); + display.className = 'terminal-display'; + display.setAttribute('data-terminal-id', id); + wrapper.appendChild(display); + document.body.appendChild(wrapper); + return { wrapper, display }; +} + +/** A canvas node host built to design 012 D17's contract. */ +function makeCanvasHost(id: string): { wrapper: HTMLElement; display: HTMLElement } { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper canvas-surface'; + const display = document.createElement('div'); + display.className = 'terminal-display'; + display.setAttribute('data-terminal-id', id); + wrapper.appendChild(display); + document.body.appendChild(wrapper); + return { wrapper, display }; +} + +/** What xterm puts inside its host: `.xterm` with a helper textarea. */ +function makeXtermElement(): { element: HTMLElement; textarea: HTMLTextAreaElement } { + const element = document.createElement('div'); + element.className = 'xterm'; + const textarea = document.createElement('textarea'); + textarea.className = 'xterm-helper-textarea'; + element.appendChild(textarea); + return { element, textarea }; +} + +// PLAN CORRECTION (015 Task 14). jsdom does not implement the `CSS` global at all, +// so `setPaneBackgroundVar`'s `CSS.escape(terminalId)` (terminalTheme.ts:39) throws +// `ReferenceError: CSS is not defined` under this runner — the plan predicted a +// wrong VALUE, not a throw. Only the test environment is short of the API; every +// real webview has it, so the production call stays as it is and the gap is filled +// here with the spec's own escaping rules (enough for the identifiers under test). +beforeAll(() => { + const g = globalThis as unknown as { CSS?: { escape(value: string): string } }; + if (typeof g.CSS === 'undefined') { + g.CSS = { + escape: (value: string) => String(value).replace(/[^a-zA-Z0-9_-]/g, (c) => `\\${c}`), + }; + } +}); + +afterEach(() => { + document.body.innerHTML = ''; +}); + +describe('design/012 D17 — the canvas host contract (§13 T16)', () => { + it('resolves the Ctrl+C guard selector from inside the relocated element', () => { + const host = makeCanvasHost('tb-ctrlc'); + const { element, textarea } = makeXtermElement(); + host.display.appendChild(element); + + // InputHandler.ts:268-269's exact expression. + expect(textarea.classList.contains('xterm')).toBe(false); // the sibling branch fails… + expect(textarea.closest('.terminal-display')).toBe(host.display); // …this one saves it + }); + + it('keeps isEditableNonTerminalTarget false for the relocated helper textarea', () => { + const host = makeCanvasHost('tb-editable'); + const { element, textarea } = makeXtermElement(); + host.display.appendChild(element); + + // inputTargets.ts:16 checks `.xterm` FIRST, which resolves from inside + // term.element regardless of the host — so this row of §4.4 needs NOTHING from + // the host. Asserted so a future "simplification" of that helper is caught. + expect(isEditableNonTerminalTarget(textarea)).toBe(false); + }); + + it('resolves the rail wrapper from inside the relocated element', () => { + const host = makeCanvasHost('tb-rail'); + const { element } = makeXtermElement(); + host.display.appendChild(element); + + // endedRegions.ts:560, with WRAPPER_SELECTOR from :100. + expect(element.closest('.terminal-display-wrapper')).toBe(host.wrapper); + }); + + it('makes the host — not the wrapper — term.element\'s parentElement', () => { + const host = makeCanvasHost('tb-fit'); + const { element } = makeXtermElement(); + host.display.appendChild(element); + + // Spike 004 Q4 measured that FitAddon.proposeDimensions() reads + // term.element.parentElement (FitAddon.ts:56,72) — pinning .terminal-display to + // an independent 400x200 inside an untouched 800x400 wrapper moved its output + // to {cols:54, rows:13}, matching the DISPLAY and not the wrapper. So RC2's + // "constant CSS-pixel box" is a constraint on the HOST specifically. + expect(element.parentElement).toBe(host.display); + expect(element.parentElement).not.toBe(host.wrapper); + }); + + // §4.4 row 6: the ONE production change. Both the pane node and the canvas host + // must receive the per-pane background var, including on later scheme changes via + // applyEffectiveThemes (terminalTheme.ts:53-73). + it('writes --terminal-display-background onto BOTH the pane and the canvas host', () => { + const pane = makePane('tb-bg'); + const host = makeCanvasHost('tb-bg'); + + setPaneBackgroundVar('tb-bg', '#101010'); + + expect(pane.display.style.getPropertyValue('--terminal-display-background')).toBe('#101010'); + expect(host.display.style.getPropertyValue('--terminal-display-background')).toBe('#101010'); + }); + + it('does not write onto a different terminal\'s nodes', () => { + const mine = makePane('tb-mine'); + const theirs = makePane('tb-theirs'); + + setPaneBackgroundVar('tb-mine', '#202020'); + + expect(mine.display.style.getPropertyValue('--terminal-display-background')).toBe('#202020'); + expect(theirs.display.style.getPropertyValue('--terminal-display-background')).toBe(''); + }); + + it('is a no-op without a background, and safe when nothing matches', () => { + const pane = makePane('tb-noop'); + setPaneBackgroundVar('tb-noop', undefined); + expect(pane.display.style.getPropertyValue('--terminal-display-background')).toBe(''); + expect(() => setPaneBackgroundVar('tb-absent', '#303030')).not.toThrow(); + }); +}); + +describe('design/012 D19 / §4.4 row 8 — §13 T22b: the pointer gate can inherit', () => { + /** + * The CSS-contract half of T22. The hit test itself is not assertable in jsdom — + * there is no layout engine and no hit testing, so `pointer-events: none` has no + * observable effect on dispatchEvent (plan ground-truth correction G3); that half + * is the manual gate §13 already lists. + * + * What IS assertable, and what D19 actually depends on, is that NOTHING under + * `.terminal-display` re-enables pointer events — otherwise a host-level + * `pointer-events: none` would not reach term.element and the gate would silently + * do nothing. design 012 §4.4 row 8: "the only two pointer-events declarations + * are :46 and :55, on the rail layer and the rail, not on the grid". + */ + it('no rule in TerminalDisplay.css sets pointer-events on the grid', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const fs = require('fs') as typeof import('fs'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const path = require('path') as typeof import('path'); + const css = fs.readFileSync( + path.join(__dirname, '..', 'TerminalDisplay.css'), + 'utf8', + ); + + // PLAN CORRECTION (015 Task 14): strip comments FIRST. The plan split the raw + // stylesheet on `}`, which leaves the comment block preceding a rule glued to + // its selector — so `rule.split('{')[0]` for `.ended-rail-layer` came back as + // the whole "Ended-region RAIL (see …)" comment, which both fails the selector + // regex and (because that comment says "terminal-display and xterm padding") + // trips the two `not.toContain` assertions. Removing comments makes the split + // yield the bare selectors the assertions below were written for. + const withoutComments = css.replace(/\/\*[\s\S]*?\*\//g, ''); + + // Split into rules and keep only those that declare pointer-events. + const rules = withoutComments.split('}').map((r) => r.trim()).filter(Boolean); + const pointerRules = rules.filter((r) => /pointer-events\s*:/.test(r)); + + expect(pointerRules.length).toBe(2); + for (const rule of pointerRules) { + const selector = rule.split('{')[0].trim(); + // Both belong to the ended-region rail, which is a SIBLING of the grid. + expect(selector).toMatch(/^\.ended-rail(-layer)?$/); + expect(selector).not.toContain('.terminal-display'); + expect(selector).not.toContain('.xterm'); + } + }); +}); diff --git a/src/renderer/components/Terminal/__tests__/terminalDisplayRelocationWiring.test.ts b/src/renderer/components/Terminal/__tests__/terminalDisplayRelocationWiring.test.ts new file mode 100644 index 0000000..33e48f6 --- /dev/null +++ b/src/renderer/components/Terminal/__tests__/terminalDisplayRelocationWiring.test.ts @@ -0,0 +1,108 @@ +/** + * design/012 §4.2.1 + §4.2.2, §8 — §13 T14's "byte-identical to today" clause and + * the wiring §15.8 records rev 5 getting DEAD WRONG. + * + * A TRIPWIRE over TerminalDisplay.tsx's source, not a behavioural test: the + * component cannot be mounted under the root Jest config (two CSS imports with no + * transform, @tauri-apps/api/event, the Redux store, and a real xterm + * Terminal.open() that needs a canvas 2D context jsdom lacks). The BEHAVIOUR these + * lines produce is covered by useSurfaceRelocation.test.tsx (T19/T20/T21/T23); + * what this file guards is that the real component still has the shape those tests + * assume — above all that the engine effect's cleanup uses the CAPTURED pane and + * not `terminalRef.current`, which React has already nulled by then (099 T1-F3). + */ +import * as fs from 'fs'; +import * as path from 'path'; + +const SOURCE = fs.readFileSync( + path.join(__dirname, '..', 'TerminalDisplay.tsx'), + 'utf8', +); + +describe('TerminalDisplay relocation wiring', () => { + // §13 T14's last clause / §4.2: the render output is LITERALLY unchanged, and + // there is no portal anywhere. D1 killed the portal in rev 4 and reviews 089/090 + // showed the render shape is unbuildable. + it('renders the same tree as before and uses no portal', () => { + expect(SOURCE).toContain('
'); + expect(SOURCE).toContain('className="terminal-display"'); + expect(SOURCE).toContain('data-terminal-id={terminalId}'); + expect(SOURCE).toContain('onContextMenu={handleContextMenu}'); + expect(SOURCE).not.toContain('createPortal'); + }); + + // §4.2.1: the engine effect stays PASSIVE with deps [terminalId] (D3). Making it + // a layout effect is spike 004's V2 — rejected here because TerminalContainer + // renders EVERY tab, so it would move N xterm constructions onto the pre-paint + // critical path at app start (§15.2). + it('keeps the engine effect passive and keyed on terminalId alone', () => { + expect(SOURCE).toContain('engine.mount(pane);'); + expect(SOURCE).not.toContain('useLayoutEffect(() => {\n if (!terminalRef.current)'); + // The engine effect's dep array, unchanged. + expect(SOURCE).toContain('}, [terminalId]);'); + }); + + // §4.2.1: the pane is CAPTURED in the effect body and the cleanup uses the + // capture. Rev 5 wrote `if (terminalRef.current) { engine.relocateTo(...) }`, + // whose guard is FALSE on whole-component deletion — React detaches host refs + // during the deletion traversal, before passive cleanup — so that cover relocated + // NOTHING on the exact interleaving it was written for (099 T1-F3). + it('captures the pane element and relocates home with it before unmount()', () => { + expect(SOURCE).toContain('const pane = terminalRef.current;'); + expect(SOURCE).toMatch(/engine\.relocateTo\(pane,\s*\{\s*paneChrome:\s*true\s*\}\)/); + // Ordering: relocate home, THEN unmount. unmount() never removes term.element + // from the DOM (TerminalEngine.ts:3218-3276), so the reverse order strands a + // live-painting, input-dead surface in the canvas host (hazard H11). + const relocateAt = SOURCE.indexOf('engine.relocateTo(pane,'); + const unmountAt = SOURCE.indexOf('engine.unmount();'); + expect(relocateAt).toBeGreaterThan(-1); + expect(unmountAt).toBeGreaterThan(-1); + expect(relocateAt).toBeLessThan(unmountAt); + // And the cleanup must NOT re-read the ref. + expect(SOURCE).not.toContain('engine.relocateTo(terminalRef.current'); + }); + + // §4.2.1: the generation bump is what makes relocation-at-mount reachable (H12). + it('bumps the engine generation right after mount()', () => { + expect(SOURCE).toContain('useSurfaceRelocation'); + expect(SOURCE).toContain('engineMounted();'); + const mountAt = SOURCE.indexOf('engine.mount(pane);'); + const bumpAt = SOURCE.indexOf('engineMounted();'); + expect(mountAt).toBeGreaterThan(-1); + expect(bumpAt).toBeGreaterThan(mountAt); + }); + + // §8 / §13 T17, renderer half: the overlays anchored to coordinates that stop + // meaning anything are closed on EVERY relocation; the suggest popup's React + // state is cleared only on the way OUT (the engine gate stops it coming back); + // and the SEARCH BAR is deliberately left open with its state intact, because it + // holds user-typed state and its highlights travel with the buffer. + it('closes the coordinate-anchored overlays on relocation and leaves search alone', () => { + const start = SOURCE.indexOf('onRelocated:'); + expect(start).toBeGreaterThan(-1); + // PLAN CORRECTION (015 Task 13): the plan sliced 400 chars, which cannot reach + // the calls its own Step 4 snippet places — the prescribed comment blocks put + // `setContextMenu(null)` at +536 and `setSchemaPicker(null)` at +591. 900 still + // ends strictly INSIDE the onRelocated callback (`onAborted:` begins at +968), + // so the locality this tripwire exists to assert is preserved. + const body = SOURCE.slice(start, start + 900); + expect(body).toContain('setContextMenu(null)'); + expect(body).toContain('setPathPicker(null)'); + expect(body).toContain('setSchemaPicker(null)'); + expect(body).toContain('suggestRef.current.close()'); + expect(body).not.toContain('setSearchOpen(false)'); + }); + + // §5.1's recovery contract: an 'aborted' return must TELL THE USER. That, not a + // bare console.error, is what satisfies §14 criterion 2. + it('raises a toast when a relocation aborts', () => { + const start = SOURCE.indexOf('onAborted:'); + expect(start).toBeGreaterThan(-1); + // PLAN CORRECTION (015 Task 13), same cause as above: the prescribed comment + // block puts `type: 'error'` at +329, past the plan's 300-char window. The + // onAborted callback body itself ends at +351. + const body = SOURCE.slice(start, start + 360); + expect(body).toContain('addToast'); + expect(body).toContain("type: 'error'"); + }); +}); diff --git a/src/renderer/components/Terminal/__tests__/useSurfaceRelocation.test.tsx b/src/renderer/components/Terminal/__tests__/useSurfaceRelocation.test.tsx new file mode 100644 index 0000000..ac32e41 --- /dev/null +++ b/src/renderer/components/Terminal/__tests__/useSurfaceRelocation.test.tsx @@ -0,0 +1,408 @@ +/** + * @jest-environment jsdom + * + * design/012 §4.2.1 + §4.2.2 — §13 T15, T18, T19, T20, T21, T23. + * + * These drive the REAL hook with a fake engine. They cannot mount the real + * TerminalDisplay: it imports two stylesheets (root Jest has no CSS transform), + * @tauri-apps/api/event, the Redux store and getWindowsBuildNumber, and its engine + * effect calls mount() -> real Terminal.open(), which needs a canvas 2D context + * jsdom does not provide. That is why §4.2.2's effect is extracted into this hook + * (plan ground-truth correction G4) — the hook IS the code under test. + * + * The harness reproduces TerminalDisplay's ENGINE effect (the passive one that + * creates the engine, bumps the generation and relocates home in its cleanup) + * because that effect stays in the component. Task 13 adds a source tripwire over + * TerminalDisplay.tsx so the real one cannot drift from this shape. + */ +import React, { act, useEffect, useRef } from 'react'; +import { createRoot, Root } from 'react-dom/client'; +import { useSurfaceRelocation, type RelocatableEngine } from '../useSurfaceRelocation'; +import { + setSurfaceHost, + clearSurfaceHost, + __resetSurfaceHostsForTest, +} from '../../../services/surfaceHosts'; + +type Call = + | { kind: 'relocate'; engine: string; target: HTMLElement; paneChrome: boolean; connected: boolean } + | { kind: 'unmount'; engine: string }; + +let calls: Call[] = []; + +/** A fake engine that performs the same DOM move the real one does, so residence + * assertions mean something. `abort` makes relocateTo fail without moving. */ +class FakeEngine implements RelocatableEngine { + element = document.createElement('div'); + container: HTMLElement | null = null; + abort = false; + constructor(public name: string) { + this.element.className = 'xterm'; + } + relocateTo(container: HTMLElement, opts?: { paneChrome?: boolean }): 'relocated' | 'aborted' { + calls.push({ + kind: 'relocate', + engine: this.name, + target: container, + paneChrome: opts?.paneChrome ?? false, + connected: container.isConnected, + }); + if (this.abort) return 'aborted'; + if (container === this.container) return 'relocated'; // the R0 identity no-op + container.appendChild(this.element); + this.container = container; + return 'relocated'; + } + unmount(): void { + calls.push({ kind: 'unmount', engine: this.name }); + } +} + +const engines = new Map(); +function engineFor(terminalId: string): FakeEngine { + const existing = engines.get(terminalId); + if (existing) return existing; + const created = new FakeEngine(terminalId); + engines.set(terminalId, created); + return created; +} + +interface HarnessProps { + terminalId: string; + /** Set to drop the pane ref before teardown — models React nulling an object ref + * during the deletion traversal (099 T1-F3). */ + nullPaneRefBeforeTeardown?: boolean; + onAborted?: () => void; + onRelocated?: (toCanvas: boolean) => void; +} + +/** Reproduces TerminalDisplay's structure: the pane div, the engine effect, and + * the relocation hook. */ +function Harness({ + terminalId, + nullPaneRefBeforeTeardown, + onAborted, + onRelocated, +}: HarnessProps) { + const paneRef = useRef(null); + const engineRef = useRef(null); + + const { engineMounted } = useSurfaceRelocation({ + terminalId, + engineRef, + paneRef, + onRelocated: onRelocated ?? (() => {}), + onAborted: onAborted ?? (() => {}), + }); + + // TerminalDisplay.tsx:178-328's engine effect, in the shape Task 13 gives it. + useEffect(() => { + const pane = paneRef.current; // CAPTURED — 099 T1-F3 + if (!pane) return; + const engine = engineFor(terminalId); + engineRef.current = engine; + // Stands in for engine.mount(pane). Mirrors mount()'s two DOM steps in order: + // evict any OTHER engine's surface from this container (detachForeignSurfaces, + // review 103 F2), then attach ours. + for (const other of engines.values()) { + if (other !== engine && other.element.parentElement === pane) other.element.remove(); + } + pane.appendChild(engine.element); + engine.container = pane; + engineMounted(); // ADDED — the relocation dep (§4.2.1) + return () => { + if (nullPaneRefBeforeTeardown) { + (paneRef as { current: HTMLDivElement | null }).current = null; + } + engine.relocateTo(pane, { paneChrome: true }); // the ORDERED FALLBACK + engine.unmount(); + engineRef.current = null; + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [terminalId]); + + return ( +
+
+
+ ); +} + +let container: HTMLDivElement; +let root: Root; + +beforeAll(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; +}); + +beforeEach(() => { + calls = []; + engines.clear(); + __resetSurfaceHostsForTest(); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + container.remove(); + document.body.innerHTML = ''; + __resetSurfaceHostsForTest(); +}); + +/** A canvas host div, mounted outside the harness so it can be torn down + * independently. */ +function makeCanvasHost(): HTMLElement { + const wrapper = document.createElement('div'); + wrapper.className = 'terminal-display-wrapper canvas-surface'; + const host = document.createElement('div'); + host.className = 'terminal-display'; + wrapper.appendChild(host); + document.body.appendChild(wrapper); + return host; +} + +describe('design/012 §4.2 — the relocation effect', () => { + // §13 T15. No registered host => the effect relocates to the PANE, which the + // engine's own R0 identity no-op makes free. + it('targets the pane when no host is registered', () => { + act(() => { root.render(); }); + + const relocations = calls.filter((c) => c.kind === 'relocate'); + expect(relocations.length).toBeGreaterThan(0); + for (const c of relocations) { + if (c.kind !== 'relocate') continue; + expect(c.paneChrome).toBe(true); + expect(c.target).toBe(container.querySelector('.terminal-display')); + } + act(() => { root.unmount(); }); + }); + + // §13 T19 / H12 — spike 004 Q1's V1 failure, as a regression test. The host is + // registered BEFORE the harness mounts, so nothing about `host` ever changes. + // Fails against a `[host]`-only dep list: the layout effect fires before the + // passive engine effect, sees a null ref, and never re-runs. + it('honours a host that was already registered when the component mounted', () => { + const host = makeCanvasHost(); + setSurfaceHost('tb-19', host); + + act(() => { root.render(); }); + + const engine = engines.get('tb-19')!; + expect(host.contains(engine.element)).toBe(true); + expect(engine.container).toBe(host); + act(() => { root.unmount(); }); + }); + + it('relocates when a host is registered AFTER mount, and back when it is cleared', () => { + act(() => { root.render(); }); + const engine = engines.get('tb-late')!; + const pane = container.querySelector('.terminal-display') as HTMLElement; + expect(pane.contains(engine.element)).toBe(true); + + const host = makeCanvasHost(); + act(() => { setSurfaceHost('tb-late', host); }); + expect(host.contains(engine.element)).toBe(true); + + act(() => { clearSurfaceHost('tb-late', host); }); + expect(pane.contains(engine.element)).toBe(true); + act(() => { root.unmount(); }); + }); + + // §13 T18 / §5.1's recovery contract. An 'aborted' return raises the host's + // error affordance and leaves the surface-host REGISTRATION untouched — the + // canvas node shows an empty box while the terminal stays usable in its pane. + it('reports an abort and leaves the host registration alone', () => { + const host = makeCanvasHost(); + const aborts: number[] = []; + const relocated: boolean[] = []; + + act(() => { + root.render( + aborts.push(1)} + onRelocated={(toCanvas) => relocated.push(toCanvas)} + />, + ); + }); + const engine = engines.get('tb-18')!; + relocated.length = 0; + + engine.abort = true; + act(() => { setSurfaceHost('tb-18', host); }); + + expect(aborts.length).toBe(1); + expect(relocated).toEqual([]); // onRelocated must NOT fire on an abort + expect(host.contains(engine.element)).toBe(false); + act(() => { root.unmount(); }); + }); + + it('tells the host which direction a successful relocation went', () => { + const host = makeCanvasHost(); + const relocated: boolean[] = []; + act(() => { + root.render( + relocated.push(toCanvas)} />, + ); + }); + relocated.length = 0; + + act(() => { setSurfaceHost('tb-dir', host); }); + expect(relocated).toEqual([true]); + + act(() => { clearSurfaceHost('tb-dir', host); }); + expect(relocated).toEqual([true, false]); + act(() => { root.unmount(); }); + }); +}); + +describe('design/012 §4.2.2 — the teardown orderings (H11)', () => { + // §13 T20, interleaving 1: the canvas host unmounts while the pane stays. The + // LAYOUT cleanup runs in the same commit and appendChilds the element back into + // the live pane node. appendChild moves a node out of an already-detached parent + // just as well as an attached one, so correctness does not depend on whether + // React has already removed the host div — only on the cleanup running in the + // same synchronous flush, which a layout cleanup does. + it('canvas host unmounts, pane stays: the element comes home CONNECTED', () => { + const host = makeCanvasHost(); + act(() => { root.render(); }); + const engine = engines.get('tb-20a')!; + const pane = container.querySelector('.terminal-display') as HTMLElement; + + act(() => { setSurfaceHost('tb-20a', host); }); + expect(host.contains(engine.element)).toBe(true); + + act(() => { + clearSurfaceHost('tb-20a', host); + host.parentElement!.remove(); // the node really leaves the document + }); + + expect(pane.contains(engine.element)).toBe(true); + expect(engine.element.isConnected).toBe(true); + act(() => { root.unmount(); }); + }); + + // §13 T21(a) — the PRIMARY cover. TerminalDisplay unmounting while displayed on + // canvas must return the surface to a CONNECTED pane node BEFORE engine.unmount() + // runs. Without it, unmount() leaves the element in the canvas host — it disposes + // every subscription, removes the rail layer and nulls this.container, but it + // NEVER removes term.element from the DOM (TerminalEngine.ts:3218-3276) — still + // painting live output and dead to input, with nothing to reclaim it. + it('unmounting while on canvas relocates home BEFORE unmount(), to a connected node', () => { + const host = makeCanvasHost(); + act(() => { root.render(); }); + act(() => { setSurfaceHost('tb-21a', host); }); + const engine = engines.get('tb-21a')!; + const pane = container.querySelector('.terminal-display') as HTMLElement; + calls = []; + + act(() => { root.unmount(); }); + + const firstRelocate = calls.find((c) => c.kind === 'relocate'); + const unmountAt = calls.findIndex((c) => c.kind === 'unmount'); + const firstRelocateAt = calls.findIndex((c) => c.kind === 'relocate'); + expect(firstRelocate).toBeDefined(); + expect(firstRelocateAt).toBeLessThan(unmountAt); + if (firstRelocate && firstRelocate.kind === 'relocate') { + expect(firstRelocate.target).toBe(pane); + expect(firstRelocate.connected).toBe(true); // the PRIMARY cover keeps it connected + expect(firstRelocate.paneChrome).toBe(true); + } + expect(pane.contains(engine.element)).toBe(true); + }); + + // §13 T21(b) / 099 T1-F3 — the ordered FALLBACK, and the exact defect rev 5 + // shipped. On a whole-component deletion React detaches host refs + // (ref.current = null) during the deletion traversal, BEFORE passive deletion + // cleanup. Rev 5's cleanup read `terminalRef.current` and its + // `if (terminalRef.current)` guard was therefore FALSE — the second cover + // relocated nothing. Capturing `pane` in the effect body is what makes it real. + it('the engine-effect cleanup uses the CAPTURED pane, not the ref React has nulled', () => { + act(() => { root.render(); }); + const pane = container.querySelector('.terminal-display') as HTMLElement; + calls = []; + + act(() => { root.unmount(); }); + + // The engine effect's cleanup still relocated, and to the CAPTURED element. + const relocations = calls.filter((c) => c.kind === 'relocate'); + expect(relocations.length).toBeGreaterThan(0); + expect(relocations.every((c) => c.kind === 'relocate' && c.target === pane)).toBe(true); + const unmountAt = calls.findIndex((c) => c.kind === 'unmount'); + expect(calls.findIndex((c) => c.kind === 'relocate')).toBeLessThan(unmountAt); + }); + + // §13 T20, interleaving 3: both unmount in one commit. The element ends detached + // in the old pane div — EXACTLY where today's every remount already leaves it, + // and where mount()'s reattach branch (:748) picks it up. No new state. + it('both unmount in one commit: the element ends where a remount already leaves it', () => { + const host = makeCanvasHost(); + act(() => { root.render(); }); + act(() => { setSurfaceHost('tb-20c', host); }); + const engine = engines.get('tb-20c')!; + const pane = container.querySelector('.terminal-display') as HTMLElement; + + act(() => { + root.unmount(); + clearSurfaceHost('tb-20c', host); + host.parentElement!.remove(); + }); + + expect(pane.contains(engine.element)).toBe(true); + expect(host.contains(engine.element)).toBe(false); + }); +}); + +describe('design/012 §4.2.2 — cleanup identity (H13)', () => { + // §13 T23 / review 098 A1. TerminalDisplay is rendered WITHOUT a key + // (TerminalPane.tsx:713-…) and TerminalPane's reuse path lets terminalId change + // on the SAME component instance without an unmount (TerminalPane.tsx:174-201). + // So a cleanup registered by generation G can run in a commit where + // engineRef.current is a DIFFERENT engine. Capturing `engine` fixes the + // wrong-target half; the `engineRef.current === engine` guard fixes the other — + // without it the captured OLD engine (already unmounted, but still holding a live + // term) would appendChild its element into the pane div the SUCCESSOR has already + // mounted into, putting two xterm elements in one host. + it('a cleanup registered against engine A does nothing once B is the live engine', () => { + act(() => { root.render(); }); + const a = engines.get('tb-A')!; + calls = []; + + // The reuse path: terminalId changes IN PLACE, no remount. + act(() => { root.render(); }); + const b = engines.get('tb-B')!; + const pane = container.querySelector('.terminal-display') as HTMLElement; + + // B owns the pane, B's element is the last thing appended to it, and it is the + // ONLY surface there. + // + // HISTORY (015 Task 12). An earlier revision of this comment said the plan's + // "exactly one `.xterm`, A's element gone" assertion was unreachable "in this + // harness or in the real component", because `unmount()` never removes + // `term.element` and `mount()` was append-only on both paths. That was an + // accurate reading of the code and the wrong conclusion to draw from it: it + // recorded a real defect as a fixed property of the world. External review 103 + // finding 2 pushed back, and `mount()` now evicts a foreign surface before + // attaching its own (`detachForeignSurfaces`), so the plan's assertion is + // reachable after all — restored below. + // + // The engines here are fakes, so what this pins is the CLEANUP GUARD; the DOM + // hygiene itself is pinned against the real `mount()` in terminal-core's + // engine.mount-foreign-surface.test.ts. + expect(b.container).toBe(pane); + expect(pane.contains(b.element)).toBe(true); + expect(pane.lastElementChild).toBe(b.element); + expect(pane.querySelectorAll('.xterm')).toHaveLength(1); + expect(pane.contains(a.element)).toBe(false); + + // A's relocations all happened while A was still the live engine (its own + // engine-effect cleanup, which runs BEFORE B is created). Nothing relocated A + // after B took over. + const aRelocationsAfterB = calls + .slice(calls.findIndex((c) => c.kind === 'unmount' && c.engine === 'tb-A') + 1) + .filter((c) => c.kind === 'relocate' && c.engine === 'tb-A'); + expect(aRelocationsAfterB).toEqual([]); + act(() => { root.unmount(); }); + }); +}); diff --git a/src/renderer/components/Terminal/useSurfaceRelocation.ts b/src/renderer/components/Terminal/useSurfaceRelocation.ts new file mode 100644 index 0000000..b9c5b89 --- /dev/null +++ b/src/renderer/components/Terminal/useSurfaceRelocation.ts @@ -0,0 +1,114 @@ +/** + * Move a terminal's rendered surface between its pane and a registered canvas + * host, in the pre-paint flush (design 012 §4.2.1 + §4.2.2). + * + * Extracted from TerminalDisplay so the ordering guarantees below are unit + * testable: TerminalDisplay cannot be mounted under the root Jest config (two CSS + * imports with no transform, @tauri-apps/api/event, the Redux store, and a real + * xterm Terminal.open() that needs a canvas 2D context jsdom lacks). The semantics + * are exactly the spec's — same useLayoutEffect, same [host, engineGeneration] + * deps, same captured engine and pane, same identity-guarded cleanup. + * + * WHY `engineGeneration` (hazard H12, MEASURED by spike 004 Q1). A relocation + * useLayoutEffect keyed `[host]` alone runs BEFORE the passive useEffect that + * creates the engine — all layout effects for a commit fire before any passive + * effect, unconditionally — so on the mount commit it sees a null ref, and if + * nothing about `host` changes afterwards it NEVER gets a second chance. + * Relocation-at-mount is then unreachable, which is a shipping defect on the paths + * the design names: a pane-collapse remount while displayed on canvas, a + * cross-window detach, a webview reload with canvas mode active. Bumping a + * useState token from the engine effect forces a second commit — React flushes it + * synchronously before paint — and this effect re-fires with the ref populated. + * + * `terminalId` is deliberately NOT in the deps (review 094 B1's second suggestion, + * declined with a reason): a terminalId change re-runs this effect on the new + * commit, which still runs before the new engine's passive effect, so it hits the + * same null ref. `engineGeneration` covers terminalId TRANSITIVELY, because the + * engine effect that bumps it is itself keyed [terminalId]. One dep, not two, and + * it is the one that actually corresponds to the precondition being waited on. + */ +import { useCallback, useLayoutEffect, useRef, useState } from 'react'; +import type { RefObject } from 'react'; +import { useSurfaceHost } from '../../services/surfaceHosts'; + +/** The slice of TerminalEngine this hook needs. Structural, so the hook has no + * dependency on terminal-core and the tests can drive a fake. */ +export interface RelocatableEngine { + relocateTo( + container: HTMLElement, + opts?: { paneChrome?: boolean }, + ): 'relocated' | 'aborted'; +} + +export interface SurfaceRelocationParams { + /** The renderer LEAF id — the surface-host registry key (design 012 §9 C3). */ + terminalId: string; + engineRef: RefObject; + /** The pane's `.terminal-display` div: the implicit fallback host. */ + paneRef: RefObject; + /** Fired after a successful relocation. `toCanvas` is true when the surface just + * left the pane, so the caller can close pane chrome that no longer applies. */ + onRelocated: (toCanvas: boolean) => void; + /** Fired when relocateTo returned 'aborted'. The engine is fully restored + * (design 012 §5.1); the caller's job is to tell the user. */ + onAborted: () => void; +} + +export function useSurfaceRelocation( + params: SurfaceRelocationParams, +): { engineMounted: () => void } { + const { terminalId, engineRef, paneRef } = params; + const host = useSurfaceHost(terminalId); + const [engineGeneration, setEngineGeneration] = useState(0); + + // The callbacks change identity every render; keep them out of the deps so a + // parent re-render cannot re-run the move. Same pattern as + // TerminalDisplay's onTitleChangeRef (TerminalDisplay.tsx:153-154). + const paramsRef = useRef(params); + paramsRef.current = params; + + /** Call from the engine effect, right after mount(), so this hook can re-run + * against a ref that is finally populated (H12). Stable identity. */ + const engineMounted = useCallback(() => { + setEngineGeneration((g) => g + 1); + }, []); + + useLayoutEffect(() => { + // CAPTURED, both of them, in the effect BODY. + // - `engine`: engineRef.current at CLEANUP time can be a DIFFERENT engine + // (review 098 A1) — TerminalDisplay is rendered without a key + // (TerminalPane.tsx:713-…) and TerminalPane's reuse path lets terminalId + // change on the same component instance (TerminalPane.tsx:174-201). + // - `pane`: on a whole-component deletion React detaches host refs + // (ref.current = null) during the deletion traversal, BEFORE passive + // cleanup (review 099 T1-F3), so re-reading the ref at cleanup time yields + // null and the cleanup silently does nothing. + const engine = engineRef.current; + const pane = paneRef.current; + if (!engine || !pane) return; // covered by engineGeneration + const target = host ?? pane; + const result = engine.relocateTo(target, { paneChrome: !host }); + if (result === 'aborted') { + paramsRef.current.onAborted(); + return; // engine state fully restored by R0/§5.1 + } + paramsRef.current.onRelocated(!!host); + return () => { + // Return the surface to the pane BEFORE the canvas host can leave the + // document. Free (an R0 identity no-op) whenever the element is already home. + // + // The identity guard, not merely the capture: without it the captured OLD + // engine — already unmount()ed by the engine effect's own cleanup, but still + // holding a live term — would appendChild its element into the pane div the + // SUCCESSOR has already mounted into, putting two xterm elements in one host + // (hazard H13). Skipping is safe because the engine effect's cleanup has + // already relocated that engine home, unconditionally, before unmount(). + if (engineRef.current !== engine) return; + engine.relocateTo(pane, { paneChrome: true }); + }; + // `engineRef`/`paneRef` are stable refs and the callbacks live in paramsRef. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [host, engineGeneration]); + + return { engineMounted }; +} diff --git a/src/renderer/services/__tests__/surfaceHosts.test.tsx b/src/renderer/services/__tests__/surfaceHosts.test.tsx new file mode 100644 index 0000000..9ca2cd9 --- /dev/null +++ b/src/renderer/services/__tests__/surfaceHosts.test.tsx @@ -0,0 +1,195 @@ +/** + * @jest-environment jsdom + * + * design/012 §4.1 + D5 — §13 T14. + * + * Why the clear is IDENTITY-CHECKED and why the signature has no `| null` + * (review 094 B3, accepted in full). Rev 4 declared + * `setSurfaceHost(terminalId, el: HTMLElement | null)` and leaned the whole of + * §4.1 on "setSurfaceHost(id, null) clears ONLY if the registered element is the + * one being cleared". That is not implementable: React invokes a bare callback ref + * with `null` on detach, and `null` carries no identity to compare against. React + * 19 (package.json:104, "react": "^19.1.0") supports a ref callback RETURNING a + * cleanup function; when it does, React calls the cleanup instead of re-invoking + * the ref with null — so at runtime `el` is always a real element and the identity + * check has something to check. + * + * What the identity check does and does not buy (spike 004 Q5, measured over four + * teardown exercises): it DOES stop a stale cleanup wiping a slot something else + * has since overwritten with a DIFFERENT element; it does NOT detect "am I the last + * owner", and it says NOTHING about where the element physically lives. Residence + * is a separate problem, handled by the relocation effect (Task 12). + * + * The repo deliberately avoids React Testing Library (its installed v13 predates + * React 19), so this drives react-dom/client + React.act, mirroring + * ToastContainer.test.tsx. + */ +import React, { act, useCallback } from 'react'; +import { createRoot, Root } from 'react-dom/client'; +import { + setSurfaceHost, + clearSurfaceHost, + useSurfaceHost, + subscribeSurfaceHosts, + __resetSurfaceHostsForTest, +} from '../surfaceHosts'; + +let container: HTMLDivElement; +let root: Root; + +beforeAll(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; +}); + +beforeEach(() => { + __resetSurfaceHostsForTest(); + container = document.createElement('div'); + document.body.appendChild(container); +}); + +afterEach(() => { + act(() => root?.unmount()); + container.remove(); + __resetSurfaceHostsForTest(); +}); + +/** A reader that renders whatever host is registered for `id`. */ +function Reader({ id }: { id: string }) { + const host = useSurfaceHost(id); + return {host ? host.id || 'anon' : 'none'}; +} + +function mount(node: React.ReactElement) { + root = createRoot(container); + act(() => { root.render(node); }); +} + +describe('design/012 §4.1 — the surface-host registry', () => { + it('reports null when nothing is registered, and the element once one is', () => { + mount(); + expect(container.textContent).toBe('none'); + + const host = document.createElement('div'); + host.id = 'host-a'; + act(() => { setSurfaceHost('tb-1', host); }); + expect(container.textContent).toBe('host-a'); + + act(() => { clearSurfaceHost('tb-1', host); }); + expect(container.textContent).toBe('none'); + }); + + // The identity check. A stale cleanup must not wipe a slot something else has + // since overwritten with a DIFFERENT element (spike 004 Q5). + it('a stale cleanup with a mismatched element does not clear', () => { + const first = document.createElement('div'); + first.id = 'first'; + const second = document.createElement('div'); + second.id = 'second'; + + mount(); + act(() => { setSurfaceHost('tb-2', first); }); + act(() => { setSurfaceHost('tb-2', second); }); + expect(container.textContent).toBe('second'); + + // `first`'s cleanup runs late — it must be a no-op. + act(() => { clearSurfaceHost('tb-2', first); }); + expect(container.textContent).toBe('second'); + + act(() => { clearSurfaceHost('tb-2', second); }); + expect(container.textContent).toBe('none'); + }); + + // The 086 Q2 failure, as a regression test: a host element REPLACED under a + // stable component must re-register the new node. + it('re-registers when the host element is replaced under a stable component', () => { + const first = document.createElement('div'); + first.id = 'first'; + const second = document.createElement('div'); + second.id = 'second'; + + mount(); + act(() => { setSurfaceHost('tb-3', first); }); + expect(container.textContent).toBe('first'); + act(() => { setSurfaceHost('tb-3', second); }); + expect(container.textContent).toBe('second'); + }); + + // Both writers are NO-OPS when they would not change the map, and only a real + // change notifies subscribers — otherwise every render of a canvas node would + // schedule a useSyncExternalStore re-render for nothing. + it('notifies only on a real change', () => { + let notifications = 0; + const unsubscribe = subscribeSurfaceHosts(() => { notifications += 1; }); + try { + const host = document.createElement('div'); + setSurfaceHost('tb-4', host); + expect(notifications).toBe(1); + setSurfaceHost('tb-4', host); // same element — no-op + expect(notifications).toBe(1); + + const other = document.createElement('div'); + clearSurfaceHost('tb-4', other); // identity mismatch — no-op + expect(notifications).toBe(1); + + clearSurfaceHost('tb-4', host); + expect(notifications).toBe(2); + clearSurfaceHost('tb-4', host); // already gone — no-op + expect(notifications).toBe(2); + } finally { + unsubscribe(); + } + }); + + // Keys are independent: one terminal's host must never surface for another. + it('keys hosts independently per terminal id', () => { + const a = document.createElement('div'); + a.id = 'a'; + mount(); + act(() => { setSurfaceHost('tb-OTHER', a); }); + expect(container.textContent).toBe('none'); + }); + + // §13 T14, last clause: the useCallback([id]) STABILITY requirement. A fresh + // arrow every render makes React detach and re-attach the ref on every commit — + // clear + re-register — which is pure notification churn at best. This is the + // shape design 012 §4.1 prescribes for the canvas node, exercised end to end. + it('the prescribed callback-ref shape registers once per host, not once per render', () => { + let notifications = 0; + const unsubscribe = subscribeSurfaceHosts(() => { notifications += 1; }); + + function CanvasNode({ id, tick }: { id: string; tick: number }) { + // EXACTLY the shape design 012 §4.1 prescribes. + const hostRef = useCallback>( + (el) => { + if (el === null) return; + setSurfaceHost(id, el); + return () => clearSurfaceHost(id, el); + }, + [id], + ); + return ( +
+
+
+ ); + } + + try { + root = createRoot(container); + act(() => { root.render(); }); + expect(notifications).toBe(1); + + // A re-render with no id change must produce ZERO further notifications. + act(() => { root.render(); }); + act(() => { root.render(); }); + expect(notifications).toBe(1); + + // Unmounting runs the returned cleanup — React 19 calls it instead of + // re-invoking the ref with null. + act(() => { root.render(
); }); + expect(notifications).toBe(2); + } finally { + unsubscribe(); + } + }); +}); diff --git a/src/renderer/services/surfaceHosts.ts b/src/renderer/services/surfaceHosts.ts new file mode 100644 index 0000000..0c38b74 --- /dev/null +++ b/src/renderer/services/surfaceHosts.ts @@ -0,0 +1,83 @@ +/** + * Where a terminal's rendered SURFACE currently belongs (design 012 §4.1). + * + * A subscribable `rendererTerminalId -> HTMLElement` map. `null` means "belongs in + * the pane": the pane never registers, it is the implicit fallback in + * `host ?? terminalRef.current` (see useSurfaceRelocation). Canvas Mode registers + * a node's host div by callback ref and lets React 19's ref-cleanup closure + * unregister it. + * + * KEY CONTRACT (design 012 §9 C3): the key is the renderer LEAF id — `tb-*` for a + * root/solo pane, `tm-*` for a split pane — the same key `terminalCache` and the + * ended-region tracker registry use. The OWNING TAB id must never key a surface, + * or two split panes alias and one terminal shows in two places. + * + * SINGLE OWNER PER KEY, and that is load-bearing. Spike 004 Q5 measured that with + * two owners of one id-keyed slot, whichever unmounts FIRST clears the registry + * while the other is still displaying that exact element — an identity-checked + * pointer is insufficient for that shape and it would need a refcount. If Canvas + * Mode ever adds a second registrant for the same terminalId (a minimap preview, a + * detached inspector), this module must change first (design 012 §10.2 row 8). + * + * Both writers are NO-OPS when they would not change the map, and only a real + * change notifies — otherwise every render of a canvas node schedules a + * useSyncExternalStore re-render for nothing. + */ +import { useCallback, useSyncExternalStore } from 'react'; + +const hosts = new Map(); +const listeners = new Set<() => void>(); + +function emit(): void { + listeners.forEach((listener) => listener()); +} + +/** + * Register `el` as the surface host for `terminalId`. + * + * NOTE the signature takes a NON-NULLABLE HTMLElement. `| null` cannot clear here, + * because `null` carries no identity to check against (review 094 B3) — use + * `clearSurfaceHost` with the element you registered. + */ +export function setSurfaceHost(terminalId: string, el: HTMLElement): void { + if (hosts.get(terminalId) === el) return; + hosts.set(terminalId, el); + emit(); +} + +/** + * Unregister `expected` from `terminalId` — IDENTITY-CHECKED, so a stale cleanup + * cannot wipe a slot something else has since overwritten with a different element + * (spike 004 Q5 exercises 1-3). + */ +export function clearSurfaceHost(terminalId: string, expected: HTMLElement): void { + if (hosts.get(terminalId) !== expected) return; + hosts.delete(terminalId); + emit(); +} + +/** Subscribe to any registry change. Returns the unsubscribe. */ +export function subscribeSurfaceHosts(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +/** + * The host this terminal's surface belongs in, or `null` for "its own pane". + * + * `useSyncExternalStore` rather than a context or a Redux slice: the value is a + * live DOM node, it changes outside React's data flow, and every consumer is + * keyed independently. + */ +export function useSurfaceHost(terminalId: string): HTMLElement | null { + const getSnapshot = useCallback(() => hosts.get(terminalId) ?? null, [terminalId]); + return useSyncExternalStore(subscribeSurfaceHosts, getSnapshot, getSnapshot); +} + +/** Test-only: drop all registrations and subscribers between cases. */ +export function __resetSurfaceHostsForTest(): void { + hosts.clear(); + listeners.clear(); +} diff --git a/src/renderer/store/terminalTheme.ts b/src/renderer/store/terminalTheme.ts index 31289b5..ab40830 100644 --- a/src/renderer/store/terminalTheme.ts +++ b/src/renderer/store/terminalTheme.ts @@ -38,10 +38,22 @@ export function resolveSchemaId( * scrollbar pseudo-elements. No-op outside the browser / before the pane mounts. */ export function setPaneBackgroundVar(terminalId: string, background: string | undefined): void { if (typeof document === 'undefined' || !background) return; - const el = document.querySelector( + // querySelectorAll, not querySelector (design 012 §4.4 row 6): while a terminal + // is displayed on a Canvas Mode node there are TWO nodes carrying this + // data-terminal-id — the pane's `.terminal-display` and the node's host — and + // both need the var, because `term.element` has left the React-owned node's + // subtree while the pane keeps painting its own slack/scrollbar chrome. + // + // Safe: `data-terminal-id` appears in exactly two places in the repo + // (TerminalDisplay.tsx:548 and the selector below), so no consumer assumes + // uniqueness. PlaybackViewer.tsx also renders a `.terminal-display`, but without + // the attribute, so this selector never matches it. + const nodes = document.querySelectorAll( `.terminal-display[data-terminal-id="${CSS.escape(terminalId)}"]`, - ) as HTMLElement | null; - el?.style.setProperty('--terminal-display-background', background); + ); + nodes.forEach((el) => { + el.style.setProperty('--terminal-display-background', background); + }); } /**