From ffcfcb8102be9d986d63e47bd9834e554619a00a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Morten=20Aslo-=C3=98stergaard?= Date: Wed, 30 Sep 2026 10:19:28 +0200 Subject: [PATCH 1/2] fix(terminal): report the measured size, so a new pane isn't stuck at 80x24 A new session drew its program into roughly a sixth of the pane and stayed that way until the window was resized by hand. `term.onResize` was registered some seventy lines BELOW the initial `fitAddon.fit()`. That fit is the one call that genuinely changes the size -- xterm is constructed at its 80x24 default and fit() measures the real pane -- so it fired the event with no listener attached, and the host never learned the size at all. Moving the registration up is necessary but not sufficient, and this is the half that is invisible at the call site. Read out of the shipped bundles: xterm-addon-fit.js fit() skips term.resize() when the proposed dimensions already match xterm.js resize(c,r) early-returns when c===cols && r===rows So once the first fit has landed, every later fit is a silent no-op: the 50ms/250ms timeouts, document.fonts.ready, the ResizeObserver, and TerminalBridge.FitTerminal's own "fit" message included. `_lastSize` stayed at its (80, 24) placeholder, `pty.Start` created the ConPTY 80 columns wide, and AttachPty's `_pty.Resize(_lastSize)` re-applied the same wrong value. xterm drew ~220x55 while the program believed it had 80x24. Only a real change in ELEMENT size -- resizing the window, switching layout -- ever fired the event again, which is exactly the reported workaround. Every fit now goes through `doFit()`, which fits and then calls `postSize()` to report term.cols/term.rows directly, deduped against the last pair: a no-op fit re-syncs the host exactly once, a genuine one is not reported twice. Verified against the library contract above -- the pre-fix shape reports [] after launch, the new one reports the real size once. LaunchSessionAsync also awaits the new TerminalBridge.WaitForInitialSizeAsync after ApplyFontSettings / ApplyProfileOverrides (both can change the font, and cols derives from the measured advance width) and before pty.Start, so the ConPTY is created at the right size rather than corrected a frame later -- which a TUI that has already painted its first frame renders at the wrong width until a full redraw. NavigationCompleted is not that signal: the page posts its size during load, but that message reaches the host as a separate dispatcher item, so InitializeAsync can return first. Bounded at 1.5s so a wedged renderer cannot block a launch. The resize handler now traces `RESIZE cols= rows= first=` under DebugTerminalTrace. The absence of that line is the signature of this bug, and nothing logged it before. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX --- CLAUDE.md | 45 ++++++++++- src/CodeShellManager/Assets/terminal-init.js | 79 ++++++++++++++----- src/CodeShellManager/MainWindow.xaml.cs | 8 ++ .../Terminal/TerminalBridge.cs | 33 ++++++++ 4 files changed, 143 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8504c28..09dba48 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -291,7 +291,50 @@ Both (2) and (3) **must come from the page**, and this is the part that is easy - **WebView2 is an `HwndHost`.** Mouse input landing on hosted native content raises **no** WPF routed events, tunnelling `Preview*` ones included. A `PreviewMouseLeftButtonDown` on the host Border only ever fires for the thin ring around the terminal (#108). The same fact bites on the way out too — WPF content cannot be *drawn* over a pane either, whatever `Panel.ZIndex` says. See "Session Spinners". - **xterm's `onData` is not "the user typed".** It also carries replies the terminal generates itself — device attributes (`ESC[?1;2c`), cursor-position reports, OSC colour replies, focus in/out (`ESC[I`/`ESC[O`) — plus mouse reports when the app enables tracking. Filtering those by inspecting the bytes cannot work; a device-attribute reply is not distinguishable from typing by shape. xterm knows internally (`triggerDataEvent`'s `wasUserInput`) but does not expose it on `onData`. `onKey` is the only honest source (#106). -The page-side `mousedown` handler also calls `fitAddon.fit()`, and the initial fit is re-run on `document.fonts.ready`. xterm derives its column count from the *measured advance width* of the font, so a fit that runs before the font loads computes the wrong `cols` and tells the PTY a width that doesn't match what is drawn — text then overlaps mid-line. The `ResizeObserver` cannot catch that, because the element size never changed, only the glyph metrics (#113). +The page-side `mousedown` handler also calls `doFit()`, and the initial fit is re-run on `document.fonts.ready`. xterm derives its column count from the *measured advance width* of the font, so a fit that runs before the font loads computes the wrong `cols` and tells the PTY a width that doesn't match what is drawn — text then overlaps mid-line. The `ResizeObserver` cannot catch that, because the element size never changed, only the glyph metrics (#113). + +## A fit that changes nothing must still report the size + +Every fit in `terminal-init.js` goes through `doFit()`, which calls `fitAddon.fit()` and +then `postSize()`. **Do not call `fitAddon.fit()` directly** — that is the shape that +loses the size report, and it cost a new session three quarters of its pane. + +Two library facts combine into the trap, and neither is visible at the call site: + +| | | +|---|---| +| `FitAddon.fit()` | skips `term.resize()` entirely when the proposed dimensions already match | +| `Terminal.resize(c, r)` | early-returns when `c === this.cols && r === this.rows` | + +So `onResize` fires **only on a change**. `new Terminal()` starts at xterm's default 80x24, +which means the *initial* fit is the one call that genuinely changes the size — and +`term.onResize` used to be registered some seventy lines below it. The host therefore never +learned the size at all, and every later fit was a silent no-op: the 50ms/250ms timeouts, +`document.fonts.ready`, the `ResizeObserver`, and `TerminalBridge.FitTerminal`'s own `fit` +message included. `_lastSize` stayed at its `(80, 24)` placeholder, `pty.Start` created the +ConPTY 80 columns wide, and `AttachPty`'s `_pty.Resize(_lastSize)` re-applied the same wrong +value. + +xterm drew ~220x55 while the program believed it had 80x24, so Claude Code painted its frame +into roughly a sixth of the pane. The only thing that ever fixed it was a real change in +**element** size — resizing the window or switching layout — which is why the bug presented +as "a new session needs a resize before it uses the full screen". + +Moving the registration above the first fit is necessary but **not sufficient**: it fixes +only the first fit, and leaves every subsequent one unable to correct a size the host got +wrong. `postSize()` reports `term.cols`/`term.rows` directly and dedupes against the last +pair, so a no-op fit re-syncs the host exactly once and a genuine one is not reported twice. + +**`bridge.TerminalSize` is a placeholder until the page reports.** `LaunchSessionAsync` +awaits `TerminalBridge.WaitForInitialSizeAsync()` after `ApplyFontSettings` / +`ApplyProfileOverrides` (both can change the font, and cols is derived from the measured +advance width) and before `pty.Start`, so the ConPTY is *created* at the right size. +`NavigationCompleted` alone is not that signal — the page posts its size during load, but +that message reaches the host as a separate dispatcher item, so `InitializeAsync` can return +first. The wait is bounded (1.5s) so a wedged renderer cannot block a launch; the `resize` +handler still fixes the size whenever it arrives. That handler also traces +`RESIZE cols= rows= first=` under `DebugTerminalTrace` — the *absence* of that line is the +signature of this bug. ## Session Lifecycle diff --git a/src/CodeShellManager/Assets/terminal-init.js b/src/CodeShellManager/Assets/terminal-init.js index 86202f3..8bb65f0 100644 --- a/src/CodeShellManager/Assets/terminal-init.js +++ b/src/CodeShellManager/Assets/terminal-init.js @@ -29,8 +29,56 @@ const fitAddon = new FitAddon.FitAddon(); term.loadAddon(fitAddon); + + // ── Size reporting: registered BEFORE the first fit, and never only via onResize ── + // + // Both halves below are load-bearing, and the second is the non-obvious one. + // + // * onResize used to be registered ~70 lines further down, AFTER the initial + // fit(). That fit is the one call that genuinely changes the size — xterm is + // constructed at its 80x24 default and fit() measures the real pane — so it + // fired the event with no listener attached, and the host never learned the + // size at all. + // + // * Moving the registration up is still not sufficient. FitAddon.fit() skips + // term.resize() outright when the proposed dimensions already match, and + // Terminal.resize() early-returns on an unchanged size. So once the first fit + // has landed, EVERY later fit is a silent no-op: the 50ms/250ms timeouts, + // document.fonts.ready, the ResizeObserver, and the host's own "fit" message + // included. Only a real change in ELEMENT size — resizing the window, + // switching layout — ever produced another event. + // + // Left at the host's (80, 24) initializer, the ConPTY was created 80 columns wide + // while xterm drew ~220, so a full-screen TUI like Claude Code painted its frame + // into roughly a sixth of the pane and stayed that way until the user resized + // something by hand. postSize() reports the measured size directly and dedupes + // against the last pair, so a no-op fit still re-syncs the host exactly once. + var lastPostedCols = -1, lastPostedRows = -1; + + function postSize() { + if (term.cols === lastPostedCols && term.rows === lastPostedRows) return; + lastPostedCols = term.cols; + lastPostedRows = term.rows; + try { + window.chrome.webview.postMessage(JSON.stringify({ + type: 'resize', cols: term.cols, rows: term.rows + })); + } catch (e) {} + } + + // Every fit in this file goes through doFit(). Resist calling fitAddon.fit() + // directly — that is the shape that loses the size report. + function doFit() { + try { fitAddon.fit(); } catch (e) {} + postSize(); + } + + // Still worth keeping alongside doFit(): a resize can also originate inside the + // terminal (CSI 8 t) rather than from a fit of ours. + term.onResize(postSize); + term.open(document.getElementById('terminal')); - fitAddon.fit(); + doFit(); // ── Shell integration: OSC 9001;key=value;key=value;ST ───────────────────── // A program inside the terminal can push session state up to CSM by emitting: @@ -95,15 +143,10 @@ var now = Date.now(); if (now - lastActivate < 300) return; lastActivate = now; - try { fitAddon.fit(); } catch (e) {} + doFit(); window.chrome.webview.postMessage(JSON.stringify({ type: 'activate' })); }, { capture: true }); - // ── Resize notification ──────────────────────────────────────────────────── - term.onResize(({ cols, rows }) => { - window.chrome.webview.postMessage(JSON.stringify({ type: 'resize', cols, rows })); - }); - // ── Page-side diagnostics (issue #70) ────────────────────────────────────── // The host's timing ends at PostWebMessageAsString. If the renderer process is the // starved component — plausible at 25 panes, where 60+ WebView2 processes were measured @@ -148,8 +191,8 @@ if (msg.type === 'output') diagWrite(msg.data); else if (msg.type === 'setDiag') diagOn = !!msg.on; else if (msg.type === 'clear') term.clear(); - else if (msg.type === 'focus') { term.focus(); fitAddon.fit(); } - else if (msg.type === 'fit') { fitAddon.fit(); term.focus(); } + else if (msg.type === 'focus') { term.focus(); doFit(); } + else if (msg.type === 'fit') { doFit(); term.focus(); } else if (msg.type === 'paste') term.paste(msg.data); else if (msg.type === 'setOptions') { const opts = msg.options; @@ -164,7 +207,7 @@ if (opts.cursorBlink !== undefined) term.options.cursorBlink = opts.cursorBlink; if (opts.padding !== undefined) document.getElementById('terminal').style.padding = opts.padding; if (opts.retro !== undefined) document.body.classList.toggle('retro', !!opts.retro); - fitAddon.fit(); + doFit(); // A profile override can switch fontFamily/fontSize, so the fit above measures the // old metrics. Re-fit on the next frame, once the new ones are in effect. // @@ -176,9 +219,7 @@ // having matched nothing. requestAnimationFrame is the honest signal: it fires // after the style change has been applied and measured. if (opts.fontFamily !== undefined || opts.fontSize !== undefined) { - requestAnimationFrame(function () { - try { fitAddon.fit(); } catch (e) {} - }); + requestAnimationFrame(doFit); } } else if (msg.type === 'dropOverlayClear') overlay.classList.remove('active'); @@ -323,15 +364,13 @@ }); // ── Fit on resize ────────────────────────────────────────────────────────── - const resizeObserver = new ResizeObserver(() => { - try { fitAddon.fit(); } catch {} - }); + const resizeObserver = new ResizeObserver(doFit); resizeObserver.observe(document.getElementById('terminal')); // Initial fit may have run while the WebView2 container was Collapsed (0×0). // Re-fit after a short delay so xterm picks up the real dimensions once visible. - setTimeout(() => { try { fitAddon.fit(); term.focus(); } catch {} }, 50); - setTimeout(() => { try { fitAddon.fit(); } catch {} }, 250); + setTimeout(() => { doFit(); try { term.focus(); } catch {} }, 50); + setTimeout(doFit, 250); // Re-fit once the font has actually loaded. // @@ -349,9 +388,7 @@ // easily too early during a heavy restore with many WebView2s initialising. They // stay as a fallback for the 0x0 case; this is the real signal. if (document.fonts && document.fonts.ready) { - document.fonts.ready.then(function () { - try { fitAddon.fit(); } catch (e) {} - }); + document.fonts.ready.then(doFit); } term.focus(); diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 9c4f37f..e2e33c6 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -1414,6 +1414,14 @@ private async Task LaunchSessionAsync(ShellSession session, bool restoring = fal bridge.ApplyFontSettings(_vm.Settings); bridge.ApplyProfileOverrides(session); + // Both calls above can change the font, and xterm derives its column count from + // the measured advance width — so wait here, after them, for the size the page + // actually measured. bridge.TerminalSize is a placeholder until that arrives, and + // creating the ConPTY at it means a full-screen TUI paints its first frame 80 + // columns wide inside a pane that draws ~220. Bounded, so a page that never + // reports still launches. + await bridge.WaitForInitialSizeAsync(); + // Start PTY now that bridge is ready var pty = new PseudoTerminal(); vm.Pty = pty; diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 50995c4..24e5790 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -22,8 +22,14 @@ public sealed class TerminalBridge : IDisposable private bool _ready; // Last terminal size reported by xterm.js — applied immediately on PTY attach // so the PTY starts at the right dimensions even if resize fired before AttachPty. + // The initializer is a placeholder only; treat it as "the page hasn't measured + // itself yet" rather than as a real size, and see WaitForInitialSizeAsync. private (int cols, int rows) _lastSize = (80, 24); + // Completed by the first "resize" message. See WaitForInitialSizeAsync. + private readonly TaskCompletionSource _initialSizeReported = + new(TaskCreationOptions.RunContinuationsAsynchronously); + // Boot overlay — set by MainWindow before InitializeAsync; posted as setBootState after // navigation completes, and hidden via bootDone on the first PTY byte (see OnPtyData). // Fallback hides the overlay after BootDoneFallbackMs so silent sessions (e.g. a child @@ -355,6 +361,28 @@ void NavCompleted(object? s, CoreWebView2NavigationCompletedEventArgs e) /// Last terminal size reported by xterm.js. Use this to start the PTY at the right size. public (int cols, int rows) TerminalSize => _lastSize; + /// + /// Waits for the page to report the size it actually measured, so a caller can create + /// the ConPTY at the right dimensions instead of at 's + /// placeholder initializer. + /// + /// NavigationCompleted is not a sufficient signal on its own. The page posts its size + /// during load, but that message is delivered to the host as a separate dispatcher + /// item — so can return, and the PTY be created, before + /// it is processed. The PTY then starts at 80x24 and is corrected a frame later, which + /// a TUI that has already painted its first frame (Claude Code) renders at the wrong + /// width until something forces a full redraw. + /// + /// Bounded on purpose: a page that never reports — a navigation failure, a wedged + /// renderer — must not block the launch, so the timeout falls through to the + /// placeholder and the "resize" handler fixes the size whenever it does arrive. + /// + public async Task WaitForInitialSizeAsync(int timeoutMs = 1500) + { + if (_initialSizeReported.Task.IsCompleted) return; + await Task.WhenAny(_initialSizeReported.Task, Task.Delay(timeoutMs)); + } + public void AttachPty(PseudoTerminal pty) { _pty = pty; @@ -525,7 +553,12 @@ private void OnWebMessageReceived(object? sender, CoreWebView2WebMessageReceived { int cols = root.GetProperty("cols").GetInt32(); int rows = root.GetProperty("rows").GetInt32(); + bool first = _initialSizeReported.TrySetResult(true); _lastSize = (cols, rows); + // Traced because the absence of this message is exactly how the pane + // stayed at 80x24: the page reported its size once, before anything + // was listening, and every later fit was a no-op that reported nothing. + Trace($"RESIZE cols={cols} rows={rows} first={first} pty={(_pty != null)}"); _pty?.Resize(cols, rows); break; } From 4924c96f79e0c4260dd324523bbf655359040c4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Morten=20Aslo-=C3=98stergaard?= Date: Wed, 30 Sep 2026 10:45:26 +0200 Subject: [PATCH 2/2] fix(terminal): close the four holes the review found in the size handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four were real; verified each against the code before changing anything. 1. The wait did not wait for what its comment claimed (MainWindow.xaml.cs). ApplyFontSettings and ApplyProfileOverrides post their setOptions through Dispatcher.BeginInvoke, and cols derives from the measured advance width — so completing on the FIRST size report resolved against the default font's column count. Worse, it usually resolved synchronously (the report had already arrived), never yielding the UI thread, so the queued BeginInvoke could not even have run. A session with a profile font override therefore got its ConPTY created at the wrong width: precisely the failure this all exists to prevent. Each setOptions now carries an incrementing token, stamped synchronously before the BeginInvoke (inside the closure would reintroduce the race). The page echoes it on every size report, and the wait resolves only on an echo at least as new as the last token stamped — a happens-after relationship rather than a timing guess. 2. An unmeasurable pane could pin the ConPTY to a bogus size (terminal-init.js). proposeDimensions() returns undefined when cell metrics are 0 and otherwise clamps to Math.max(2,…)/Math.max(1,…), so a 0x0 container yielded either xterm's untouched 80x24 or a 2x1 clamp. Harmless pre-fix, since those reports were dropped; not harmless once the host CREATES the PTY from the first size it is told. doFit() now declines to report in that state. 3. postSize() recorded the dedupe pair before the postMessage inside the try, so a throw left the size marked as delivered and the host stuck until the next genuine size change — the same bug class with a narrower trigger. The pair is now recorded only after a successful post. 4. _lastSize's placeholder was (80, 24) while PseudoTerminal.Start's own defaults are (220, 50). The one path the bounded wait cannot rescue — navigation failure, wedged renderer — therefore created the ConPTY at the narrowest plausible width, straight back into the symptom. Aligned with Start. Also fixed two things the review did not flag: * Giving doFit() a `force` parameter made every bare callback reference a latent bug — ResizeObserver passes the entries array, requestAnimationFrame a timestamp, .then() the FontFaceSet, all truthy. Those four sites are now wrapped. * With the wait gated on a token, a pane that cannot be measured would never ack and the launch would burn the full 1.5s timeout. The page now posts a separate optionsApplied ack; the host adopts a size only from `resize` and releases the wait from either, so it keeps both properties. Verified with a harness modelling both sides plus the library early-returns and proposeDimensions' undefined/clamp behaviour: plain session released | pty 220x55 font override released | pty 154x55 (default font would give 220) options, same cols released | pty 220x55 (forced ack, no stall) collapsed pane nothing posted, host keeps 220x50 placeholder collapsed + wait released by the ack, not the timeout Build clean, 597/597 tests pass. The visual result still needs a manual check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX --- CLAUDE.md | 49 +++++-- src/CodeShellManager/Assets/terminal-init.js | 70 ++++++++-- src/CodeShellManager/MainWindow.xaml.cs | 12 +- .../Terminal/TerminalBridge.cs | 129 ++++++++++++++---- 4 files changed, 207 insertions(+), 53 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 09dba48..003d62c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -325,16 +325,45 @@ only the first fit, and leaves every subsequent one unable to correct a size the wrong. `postSize()` reports `term.cols`/`term.rows` directly and dedupes against the last pair, so a no-op fit re-syncs the host exactly once and a genuine one is not reported twice. -**`bridge.TerminalSize` is a placeholder until the page reports.** `LaunchSessionAsync` -awaits `TerminalBridge.WaitForInitialSizeAsync()` after `ApplyFontSettings` / -`ApplyProfileOverrides` (both can change the font, and cols is derived from the measured -advance width) and before `pty.Start`, so the ConPTY is *created* at the right size. -`NavigationCompleted` alone is not that signal — the page posts its size during load, but -that message reaches the host as a separate dispatcher item, so `InitializeAsync` can return -first. The wait is bounded (1.5s) so a wedged renderer cannot block a launch; the `resize` -handler still fixes the size whenever it arrives. That handler also traces -`RESIZE cols= rows= first=` under `DebugTerminalTrace` — the *absence* of that line is the -signature of this bug. +**Only report a size that was actually measured.** `FitAddon.proposeDimensions()` returns +`undefined` when the cell metrics are still 0, and otherwise clamps to `Math.max(2, …)` / +`Math.max(1, …)`. So a pane whose container is 0×0 — the case the 50ms/250ms fallbacks exist +for — yields either xterm's untouched 80×24 default or a **2×1** clamp. `doFit()` therefore +declines to report at all in that state. This matters much more now that the host *creates* +the ConPTY from the first size it is told: pre-fix those bogus reports were simply dropped. + +**`bridge.TerminalSize` is a placeholder until the page reports**, and its initializer +deliberately matches `PseudoTerminal.Start`'s own `cols = 220, rows = 50` defaults. It was +`(80, 24)`, which meant the one path the wait cannot rescue — navigation failure, wedged +renderer — created the ConPTY at the narrowest plausible width, straight back into the +symptom. + +**The wait is gated on an options token, not on arrival order.** `LaunchSessionAsync` awaits +`TerminalBridge.WaitForInitialSizeAsync()` after `ApplyFontSettings` / `ApplyProfileOverrides` +and before `pty.Start`. Waiting for merely the *first* size report is not enough, and this is +the subtle part: both of those post their `setOptions` through `Dispatcher.BeginInvoke`, and +cols derives from the measured advance width, so a first-report wait resolves on the size +measured with the **default** font — and usually resolves *synchronously*, never yielding the +UI thread, so the queued `BeginInvoke` cannot even have run. A session with a profile font +override would get its ConPTY created at the wrong column count: the very failure this +exists to prevent. + +So each `setOptions` carries an incrementing token (stamped **synchronously**, before the +`BeginInvoke` — inside the closure would reintroduce the race), the page echoes it on every +size report, and the wait resolves only once an echo is at least as new as the last token +stamped. That is a happens-after relationship rather than a timing guess. + +The page also posts a separate `optionsApplied` ack, and that split is load-bearing in both +directions: `doFit()` may legitimately decline to report an unmeasurable pane, and without an +ack of its own a launch waiting on that token would burn the full 1.5s timeout per session. +The host adopts a size only from `resize`, and releases the wait from either. +`NavigationCompleted` is not a substitute for any of this — the page posts its size during +load, but that message reaches the host as a separate dispatcher item, so `InitializeAsync` +can return first. + +The wait is bounded (1.5s) so a wedged renderer cannot block a launch; the `resize` handler +still corrects the size whenever it arrives. It traces `RESIZE cols= rows= token= released=` +under `DebugTerminalTrace` — the *absence* of that line is the signature of this bug. ## Session Lifecycle diff --git a/src/CodeShellManager/Assets/terminal-init.js b/src/CodeShellManager/Assets/terminal-init.js index 8bb65f0..7fe358d 100644 --- a/src/CodeShellManager/Assets/terminal-init.js +++ b/src/CodeShellManager/Assets/terminal-init.js @@ -55,22 +55,47 @@ // against the last pair, so a no-op fit still re-syncs the host exactly once. var lastPostedCols = -1, lastPostedRows = -1; - function postSize() { - if (term.cols === lastPostedCols && term.rows === lastPostedRows) return; - lastPostedCols = term.cols; - lastPostedRows = term.rows; + // Mirrors the token the host stamps on each setOptions message, and is echoed back on + // every size report. It is how the host can tell "the size measured with the font you + // just asked for" from "the size measured before it" — see WaitForInitialSizeAsync. + var optionsToken = 0; + + // force: report even when the size is unchanged. Needed to ACK an options token, since + // a font change that happens not to alter the column count would otherwise be silent + // and leave the host waiting for a report that never comes. + function postSize(force) { + if (!force && term.cols === lastPostedCols && term.rows === lastPostedRows) return; try { window.chrome.webview.postMessage(JSON.stringify({ - type: 'resize', cols: term.cols, rows: term.rows + type: 'resize', cols: term.cols, rows: term.rows, token: optionsToken })); - } catch (e) {} + } catch (e) { + // Leave the dedupe pair unset so the next fit retries. Recording a size we failed + // to deliver is the same "host never learns the size" bug with a narrower trigger. + return; + } + lastPostedCols = term.cols; + lastPostedRows = term.rows; } // Every fit in this file goes through doFit(). Resist calling fitAddon.fit() // directly — that is the shape that loses the size report. - function doFit() { + // + // The measurability guard is not optional. FitAddon.proposeDimensions() bails out + // when the cell metrics are still 0 (nothing rendered yet) and otherwise clamps its + // answer to Math.max(2, …) / Math.max(1, …). So on a pane whose container is 0x0 — + // exactly the case the 50ms/250ms fallbacks below exist for — a fit does nothing and + // a report would hand the host either xterm's untouched 80x24 default or a 2x1 clamp. + // Since the host now CREATES the ConPTY from the first size it is told, reporting + // either would be worse than reporting nothing: pre-fix those were merely dropped. + function doFit(force) { + var dims = null; + try { dims = fitAddon.proposeDimensions(); } catch (e) {} + if (!dims || isNaN(dims.cols) || isNaN(dims.rows)) return; + var parent = term.element && term.element.parentElement; + if (parent && (parent.clientWidth < 1 || parent.clientHeight < 1)) return; try { fitAddon.fit(); } catch (e) {} - postSize(); + postSize(force); } // Still worth keeping alongside doFit(): a resize can also originate inside the @@ -188,6 +213,7 @@ window.chrome.webview.addEventListener('message', e => { try { const msg = JSON.parse(e.data); + if (typeof msg.token === 'number') optionsToken = msg.token; if (msg.type === 'output') diagWrite(msg.data); else if (msg.type === 'setDiag') diagOn = !!msg.on; else if (msg.type === 'clear') term.clear(); @@ -218,9 +244,22 @@ // OS-installed and there are no such rules, so it resolves on the next microtask // having matched nothing. requestAnimationFrame is the honest signal: it fires // after the style change has been applied and measured. - if (opts.fontFamily !== undefined || opts.fontSize !== undefined) { - requestAnimationFrame(doFit); - } + // Unconditional now, and forced, so a font change that does not happen to alter + // the column count is still reported rather than silently deduped away. + // + // The ack is posted separately and never skipped. doFit() declines to report an + // unmeasurable pane (0x0 container, cell metrics not computed yet), and a host + // waiting on this token would then have nothing to wait for but its own timeout — + // 1.5s of dead launch per session. Splitting them keeps both properties: the host + // only ever adopts a size it actually measured, and the wait always ends promptly. + requestAnimationFrame(function () { + doFit(true); + try { + window.chrome.webview.postMessage(JSON.stringify({ + type: 'optionsApplied', token: optionsToken + })); + } catch (e) {} + }); } else if (msg.type === 'dropOverlayClear') overlay.classList.remove('active'); else if (msg.type === 'setBootState') { @@ -364,13 +403,15 @@ }); // ── Fit on resize ────────────────────────────────────────────────────────── - const resizeObserver = new ResizeObserver(doFit); + // Wrapped, not passed by reference: ResizeObserver hands its callback the entries + // array, which would arrive as doFit's truthy `force`. + const resizeObserver = new ResizeObserver(function () { doFit(); }); resizeObserver.observe(document.getElementById('terminal')); // Initial fit may have run while the WebView2 container was Collapsed (0×0). // Re-fit after a short delay so xterm picks up the real dimensions once visible. setTimeout(() => { doFit(); try { term.focus(); } catch {} }, 50); - setTimeout(doFit, 250); + setTimeout(function () { doFit(); }, 250); // Re-fit once the font has actually loaded. // @@ -388,7 +429,8 @@ // easily too early during a heavy restore with many WebView2s initialising. They // stay as a fallback for the 0x0 case; this is the real signal. if (document.fonts && document.fonts.ready) { - document.fonts.ready.then(doFit); + // Wrapped: .then() would pass the FontFaceSet as doFit's `force`. + document.fonts.ready.then(function () { doFit(); }); } term.focus(); diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index e2e33c6..0aac581 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -1414,12 +1414,12 @@ private async Task LaunchSessionAsync(ShellSession session, bool restoring = fal bridge.ApplyFontSettings(_vm.Settings); bridge.ApplyProfileOverrides(session); - // Both calls above can change the font, and xterm derives its column count from - // the measured advance width — so wait here, after them, for the size the page - // actually measured. bridge.TerminalSize is a placeholder until that arrives, and - // creating the ConPTY at it means a full-screen TUI paints its first frame 80 - // columns wide inside a pane that draws ~220. Bounded, so a page that never - // reports still launches. + // Both calls above can change the font, and xterm derives its column count from the + // measured advance width — so the ConPTY must be created from a size measured with + // them already applied, not merely from the first size the page happened to report. + // WaitForInitialSizeAsync gates on the token those two calls stamp, so this is a + // happens-after relationship and not a sleep; it returns immediately when the page + // has already acknowledged. Bounded, so a page that never reports still launches. await bridge.WaitForInitialSizeAsync(); // Start PTY now that bridge is ready diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 24e5790..0da6ce1 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -22,13 +22,24 @@ public sealed class TerminalBridge : IDisposable private bool _ready; // Last terminal size reported by xterm.js — applied immediately on PTY attach // so the PTY starts at the right dimensions even if resize fired before AttachPty. - // The initializer is a placeholder only; treat it as "the page hasn't measured - // itself yet" rather than as a real size, and see WaitForInitialSizeAsync. - private (int cols, int rows) _lastSize = (80, 24); - - // Completed by the first "resize" message. See WaitForInitialSizeAsync. - private readonly TaskCompletionSource _initialSizeReported = - new(TaskCreationOptions.RunContinuationsAsynchronously); + // + // The initializer is a placeholder for "the page hasn't measured itself yet", and it + // deliberately matches PseudoTerminal.Start's own cols/rows defaults. It used to be + // (80, 24), which meant the one path WaitForInitialSizeAsync cannot rescue — a + // navigation failure, a wedged renderer — created the ConPTY at the narrowest + // plausible width, i.e. straight back into the symptom. A full-pane guess degrades + // far better than an 80-column one. + private (int cols, int rows) _lastSize = (220, 50); + + // ── Size handshake ──────────────────────────────────────────────────────────────── + // Each setOptions message carries an incrementing token, which the page echoes on + // every size report. That is what lets WaitForInitialSizeAsync distinguish a size + // measured WITH the font we asked for from one measured before it. + private readonly object _sizeLock = new(); + private int _optionsToken; // last token stamped on a setOptions message + private int _reportedToken = -1; // highest token seen on a size report + private int _sizeWaiterToken; // token the pending waiter needs to see + private TaskCompletionSource? _sizeWaiter; // Boot overlay — set by MainWindow before InitializeAsync; posted as setBootState after // navigation completes, and hidden via bootDone on the first PTY byte (see OnPtyData). @@ -362,25 +373,69 @@ void NavCompleted(object? s, CoreWebView2NavigationCompletedEventArgs e) public (int cols, int rows) TerminalSize => _lastSize; /// - /// Waits for the page to report the size it actually measured, so a caller can create - /// the ConPTY at the right dimensions instead of at 's - /// placeholder initializer. + /// Records the highest options token the page has acknowledged and releases a pending + /// once it is new enough. Returns whether this + /// call is what released it, for the trace. + /// + private bool NoteOptionsToken(int token) + { + TaskCompletionSource? waiter = null; + lock (_sizeLock) + { + if (token > _reportedToken) _reportedToken = token; + if (_sizeWaiter != null && _reportedToken >= _sizeWaiterToken) + { + waiter = _sizeWaiter; + _sizeWaiter = null; + } + } + waiter?.TrySetResult(true); + return waiter != null; + } + + /// + /// Waits for the page to report a size it measured with every option posted so far + /// already applied, so a caller can create the ConPTY at the right dimensions instead + /// of at 's placeholder. /// /// NavigationCompleted is not a sufficient signal on its own. The page posts its size - /// during load, but that message is delivered to the host as a separate dispatcher - /// item — so can return, and the PTY be created, before - /// it is processed. The PTY then starts at 80x24 and is corrected a frame later, which - /// a TUI that has already painted its first frame (Claude Code) renders at the wrong - /// width until something forces a full redraw. + /// during load, but that message reaches the host as a separate dispatcher item — so + /// can return, and the PTY be created, before it is + /// processed. The PTY then starts at the placeholder and is corrected a frame later, + /// which a TUI that has already painted its first frame (Claude Code) renders at the + /// wrong width until something forces a full redraw. + /// + /// **Waiting for merely the FIRST report is also not enough**, and that is the subtle + /// half. and post + /// their setOptions through Dispatcher.BeginInvoke, and cols is derived from the + /// measured advance width — so a first-report wait would return on the size measured + /// with the DEFAULT font. Worse, it would usually return synchronously (the report has + /// already arrived), never yielding the UI thread, so the queued BeginInvoke could not + /// even have run. A session with a profile font override would then get its ConPTY + /// created at the wrong column count: the exact failure this all exists to prevent. + /// + /// So the wait is gated on the options token instead of on arrival order. It resolves + /// only once the page has echoed a token at least as new as the last setOptions we + /// stamped, which is a happens-after relationship rather than a timing guess. The page + /// force-reports after applying options even when the column count is unchanged, so + /// the token is always acknowledged. /// /// Bounded on purpose: a page that never reports — a navigation failure, a wedged - /// renderer — must not block the launch, so the timeout falls through to the - /// placeholder and the "resize" handler fixes the size whenever it does arrive. + /// renderer — must not block the launch, so the timeout falls through to whatever + /// size is known and the "resize" handler corrects it whenever it does arrive. /// public async Task WaitForInitialSizeAsync(int timeoutMs = 1500) { - if (_initialSizeReported.Task.IsCompleted) return; - await Task.WhenAny(_initialSizeReported.Task, Task.Delay(timeoutMs)); + TaskCompletionSource tcs; + lock (_sizeLock) + { + int needed = _optionsToken; + if (_reportedToken >= needed) return; + _sizeWaiterToken = needed; + _sizeWaiter = tcs = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + } + await Task.WhenAny(tcs.Task, Task.Delay(timeoutMs)); } public void AttachPty(PseudoTerminal pty) @@ -553,16 +608,32 @@ private void OnWebMessageReceived(object? sender, CoreWebView2WebMessageReceived { int cols = root.GetProperty("cols").GetInt32(); int rows = root.GetProperty("rows").GetInt32(); - bool first = _initialSizeReported.TrySetResult(true); + int token = root.TryGetProperty("token", out var tk) ? tk.GetInt32() : 0; _lastSize = (cols, rows); + + bool released = NoteOptionsToken(token); + // Traced because the absence of this message is exactly how the pane // stayed at 80x24: the page reported its size once, before anything // was listening, and every later fit was a no-op that reported nothing. - Trace($"RESIZE cols={cols} rows={rows} first={first} pty={(_pty != null)}"); + Trace($"RESIZE cols={cols} rows={rows} token={token} " + + $"released={released} pty={(_pty != null)}"); _pty?.Resize(cols, rows); break; } + // The page finished applying a setOptions and re-fitted. Posted separately + // from the size because the page declines to report an UNMEASURABLE pane + // (0x0 container, cell metrics not computed yet) — without its own ack, a + // launch waiting on that token would just burn the full timeout. + case "optionsApplied": + { + int token = root.TryGetProperty("token", out var otk) ? otk.GetInt32() : 0; + bool released = NoteOptionsToken(token); + Trace($"OPTIONS-APPLIED token={token} released={released}"); + break; + } + case "getClipboard": // xterm.js wants to paste — round-trip the text through term.paste() so // bracketed paste mode (CSI ?2004h) is honored. Apps like Claude Code @@ -647,7 +718,13 @@ public void ApplyFontSettings(AppSettings settings) letterSpacing = settings.TerminalLetterSpacing, lineHeight = settings.TerminalLineHeight, }; - string json = JsonSerializer.Serialize(new { type = "setOptions", options = opts }); + // Stamped synchronously, BEFORE the BeginInvoke below: WaitForInitialSizeAsync + // reads _optionsToken on the caller's turn, so incrementing it inside the queued + // closure would let the wait resolve against a pre-options size. + int sizeToken; + lock (_sizeLock) { sizeToken = ++_optionsToken; } + string json = JsonSerializer.Serialize( + new { type = "setOptions", options = opts, token = sizeToken }); WpfApplication.Current?.Dispatcher.BeginInvoke(() => { try { _webView.CoreWebView2?.PostWebMessageAsString(json); } @@ -683,7 +760,13 @@ public void ApplyProfileOverrides(ShellSession session) } } - string json = JsonSerializer.Serialize(new { type = "setOptions", options = opts }); + // Stamped synchronously, BEFORE the BeginInvoke below: WaitForInitialSizeAsync + // reads _optionsToken on the caller's turn, so incrementing it inside the queued + // closure would let the wait resolve against a pre-options size. + int sizeToken; + lock (_sizeLock) { sizeToken = ++_optionsToken; } + string json = JsonSerializer.Serialize( + new { type = "setOptions", options = opts, token = sizeToken }); WpfApplication.Current?.Dispatcher.BeginInvoke(() => { try { _webView.CoreWebView2?.PostWebMessageAsString(json); }