diff --git a/CLAUDE.md b/CLAUDE.md index 8504c28..003d62c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -291,7 +291,79 @@ 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. + +**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 86202f3..7fe358d 100644 --- a/src/CodeShellManager/Assets/terminal-init.js +++ b/src/CodeShellManager/Assets/terminal-init.js @@ -29,8 +29,81 @@ 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; + + // 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, token: optionsToken + })); + } 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. + // + // 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(force); + } + + // 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 +168,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 @@ -145,11 +213,12 @@ 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(); - 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 +233,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. // @@ -175,11 +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(function () { - try { fitAddon.fit(); } catch (e) {} - }); - } + // 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') { @@ -323,15 +403,15 @@ }); // ── Fit on resize ────────────────────────────────────────────────────────── - const resizeObserver = new ResizeObserver(() => { - try { fitAddon.fit(); } catch {} - }); + // 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(() => { try { fitAddon.fit(); term.focus(); } catch {} }, 50); - setTimeout(() => { try { fitAddon.fit(); } catch {} }, 250); + setTimeout(() => { doFit(); try { term.focus(); } catch {} }, 50); + setTimeout(function () { doFit(); }, 250); // Re-fit once the font has actually loaded. // @@ -349,9 +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(function () { - try { fitAddon.fit(); } catch (e) {} - }); + // 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 9c4f37f..0aac581 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 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 var pty = new PseudoTerminal(); vm.Pty = pty; diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 50995c4..0da6ce1 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -22,7 +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. - private (int cols, int rows) _lastSize = (80, 24); + // + // 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). @@ -355,6 +372,72 @@ 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; + /// + /// 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 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 whatever + /// size is known and the "resize" handler corrects it whenever it does arrive. + /// + public async Task WaitForInitialSizeAsync(int timeoutMs = 1500) + { + 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) { _pty = pty; @@ -525,11 +608,32 @@ private void OnWebMessageReceived(object? sender, CoreWebView2WebMessageReceived { int cols = root.GetProperty("cols").GetInt32(); int rows = root.GetProperty("rows").GetInt32(); + 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} 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 @@ -614,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); } @@ -650,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); }