fix(terminal): don't let onResize force a size report - #145
Merged
Merged
Conversation
`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).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
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
1 participant
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.
Why
term.onResize(postSize)passed xterm's{cols, rows}event object straight intopostSize'sforceparameter, where it is truthy.That is exactly the latent bug #141 wrapped the
ResizeObserver,requestAnimationFrameanddocument.fonts.readysites for — this call site was simply missed. Both the review behind #143 and a separate review of the merged #141 flagged it independently, and #143 explicitly left it as a known one-liner.Effect
Small, which is why it survived: a resize originating inside the terminal (
CSI 8 t) bypassed the duplicate-size check and posted aresizeeven when the size already matched what the host had been told. The host just calls_pty.Resizewith the same values, so nothing breaks — butforceexists for one purpose, acknowledging an options token when the column count happens not to change, and having it permanently on for ordinary resizes defeats the dedupe the same mechanism relies on.After this
Every
doFit/postSizecall site is either an explicit call or wrapped, and the onlyforce: trueleft in the file issettle()'s options ack, which is deliberate.Testing
node --checkclean; build clean at 0 warnings, 597/597 unit tests pass. None of those cover page-side JS, so this is reasoned against the shippedxterm.jsbehaviour rather than exercised in a browser.🤖 Generated with Claude Code
https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be