fix(terminal): report the measured size, so a new pane isn't stuck at 80x24 - #141
Merged
Merged
Conversation
… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX
…shake 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX
Contributor
Author
|
Review addressed in 4924c96 — all four findings were real; I verified each against the code before changing anything.
Finding 1 was the important one — thank you, the comment was describing something the code didn't do. Two further things that fell out of the fixes and weren't in the review:
Verified with a harness modelling both sides plus the two library early-returns and Build clean, 597/597 tests pass. The visual result still needs the manual check noted in the PR body. |
3 of 8 tasks
AThraen
pushed a commit
that referenced
this pull request
Sep 30, 2026
…asured (#143) Follow-up to #141. Review of the size handshake found that the page handed the mechanism back the bug it exists to prevent, plus a way for it to stall. 1. The token was promoted on arrival (terminal-init.js). The message handler stamped optionsToken = msg.token at the very top, before the new options had taken measurable effect. The doFit() that runs straight after the option assignments still measures the OLD metrics (the code's own comment says so), yet it posted that size under the NEW token. Any 'fit' or 'focus' message landing before the next frame did the same. The host's NoteOptionsToken then saw an echo at least as new as it was waiting for, and WaitForInitialSizeAsync released on a pre-font measurement, so a session with a profile font override could still get its ConPTY at the default font's column count. The token now lands in a per-message newToken and is promoted inside settle(), immediately before the forced refit, so a report can only carry the new token once the new metrics are the ones measured. Promotion is max() rather than assignment, so two setOptions in flight (ApplyFontSettings then ApplyProfileOverrides) can never walk it backwards. 2. The ack depended on a frame being rendered. Both the forced refit and the optionsApplied ack lived only inside requestAnimationFrame, which WebView2 suspends while the control isn't rendering: window minimized, or the wrapper detached by RefreshTerminalLayout's TerminalGrid.Children.Clear() while a launch sits in its await. Every other release path is gated off in that state (doFit declines an unmeasurable pane, the ResizeObserver needs a size change, and the 50ms/250ms one-shots fired long ago at page load), so nothing acked and each affected launch burned the full 1.5s, roughly +37s across a 25-session restore. settle() now runs from requestAnimationFrame or a 250ms timer, whichever comes first, guarded so it runs once. rAF still wins whenever frames are running, so the measured-metrics path is unchanged in the normal case. Not exercised by the unit tests (it is page-side). Check by launching a session with a profile font override and looking for RESIZE ... token=N released=True under DebugTerminalTrace. Claude-Session: https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
AThraen
added a commit
that referenced
this pull request
Sep 30, 2026
`term.onResize(postSize)` passed xterm's `{cols, rows}` event object straight
into postSize's `force` parameter, where it is truthy. That is the same latent
bug #141 wrapped the ResizeObserver, requestAnimationFrame and
document.fonts.ready sites for, and this one call site was missed — both #143's
review and a review of the merged #141 found it independently.
Effect is small: a resize originating inside the terminal (CSI 8 t) bypassed the
duplicate-size check and posted even when the size already matched what the host
had been told. `force` exists to acknowledge an options token when the column
count happens not to change; it is not meant to be on for ordinary resizes.
Every doFit/postSize call site is now either explicit or wrapped; the only
`force: true` left is settle()'s options ack, which is deliberate.
Verified with `node --check`; build clean at 0 warnings, 597/597 tests (none
cover page-side JS — this is reasoned, not exercised).
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A new session drew its program into roughly a sixth of the pane and stayed that way until the window was resized by hand. Reported as "the window is like 1/4 or 1/5 of the window, and needs a resize before it actually uses the full screen, causes texts etc to look weird".
cc @AThraen
Cause
term.onResizewas registered some seventy lines below the initialfitAddon.fit()inAssets/terminal-init.js. That fit is the one call that genuinely changes the size — xterm is constructed at its 80x24 default andfit()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 verbatim out of the shipped bundles:
xterm-addon-fit.jsfit()skipsterm.resize()when the proposed dimensions already matchxterm.jsresize(c,r)early-returns whenc === cols && r === rowsSo once the first fit has landed, every later fit is a silent no-op: the 50ms/250ms timeouts,
document.fonts.ready, theResizeObserver, andTerminalBridge.FitTerminal's ownfitmessage included._lastSizetherefore stayed at its(80, 24)placeholder,pty.Startcreated the ConPTY 80 columns wide, andAttachPty's_pty.Resize(_lastSize)re-applied the same wrong value. xterm drew ~220x55 while the program believed it had 80x24 — hence a boxed TUI painting into ~1/6 of the pane. Only a real change in element size (resizing the window, switching layout) ever fired the event again, which is exactly the reported workaround.This affected every session type; it was just least visible in a plain shell, which only wraps at 80 columns instead of drawing a frame.
Changes
Assets/terminal-init.js— every fit goes throughdoFit()(fit →postSize()), andonResizeis registered before the first fit.postSize()reportsterm.cols/term.rowsdirectly, deduped against the last pair, so a no-op fit re-syncs the host exactly once and a genuine one isn't reported twice. All seven remaining directfitAddon.fit()call sites routed through it.Terminal/TerminalBridge.cs— newWaitForInitialSizeAsync(1500), completed by the firstresizemessage.NavigationCompletedis 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, soInitializeAsynccan return — and the PTY be created — first. Bounded so a wedged renderer cannot block a launch.MainWindow.xaml.cs—LaunchSessionAsyncawaits it afterApplyFontSettings/ApplyProfileOverrides(both can change the font, and cols derives from the measured advance width) and beforepty.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.CLAUDE.md— new section "A fit that changes nothing must still report the size". CallingfitAddon.fit()directly is precisely the shape someone would "simplify" back to.The
resizehandler now tracesRESIZE cols= rows= first=under the existingDebugTerminalTraceflag. The absence of that line is the signature of this bug, and nothing logged it before.Verification
Build clean (0 warnings), 597/597 unit tests pass.
Both shapes run against a harness stubbing the two library early-returns above:
Not verified: the visual result. There is no headless way to measure a WebView2's glyph metrics, so this needs one manual check — open a new Claude session and confirm the pane fills. If it doesn't, enable
DebugTerminalTraceand look for theRESIZEline incrash.log; its absence would mean the page still isn't reporting.🤖 Generated with Claude Code
https://claude.ai/code/session_01NsYm9iLc2aRnRDfV2uZRQX