fix(tui): drop the abandoned ScrollBox remount key - #792
Merged
Conversation
An earlier attempt at the resize-blanking bug keyed the transcript ScrollBox on `cols` to force a fresh Yoga node. It was abandoned once the real cause turned out to be TranscriptScrollbar stretching the row from a stale viewportHeight (fixed in #775), but it had already been swept into #774 and reached main. It is not merely redundant now, it is harmful. Remounting the ScrollBox builds a new listenersRef Set behind useImperativeHandle(..., []), while the three useSyncExternalStore subscribers (useVirtualHistory, useViewportSnapshot, useScrollbarSnapshot) have stable deps and never resubscribe — so every subscription is orphaned on the dead handle, on every resize. It also resets the handle, sending useVirtualHistory down its `vp <= 0` cold-start branch, which on a long transcript mounts only the last 30 items behind a large blank topSpacer. Its comment was also wrong about the mechanism, which is worse than no comment: it told the next reader the vendored Yoga port fails to re-measure and that marking the tree dirty is insufficient. Neither is true. Verified the resize fix still holds without it: PTY at 60 -> 110 columns, blank rows 7/30 before and after, matching #775's numbers. Suite unchanged from main's baseline at 8 failed / 1681 passed across three runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test Results 1 files 1 suites 8m 14s ⏱️ Results for commit 00f88f2. |
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.
Follow-up to #775. Removes a dead-end fix of mine that reached
mainby accident.What happened
My first attempt at the resize-blanking bug keyed the transcript ScrollBox on
cols, to force a fresh Yoga node on every width change. It worked on the repro, but the diagnosis behind it was wrong — the real cause isTranscriptScrollbarstretching the flex row from a staleviewportHeight, which #775 fixes at the source.I abandoned the key, but it had already been swept into #774's
d350b1a9, and that merged. Somaincurrently carries both the real fix and the abandoned one.Why it has to go
It isn't just redundant:
listenersRefSet behinduseImperativeHandle(..., []), while the threeuseSyncExternalStoresubscribers (useVirtualHistory,useViewportSnapshot,useScrollbarSnapshot) have stable deps and never resubscribe. Every subscription ends up on the dead handle — on every resize. fix(tui): stop the scrollbar from blanking the transcript on resize #775 added comments at those three call sites saying exactly this.getViewportHeight()returns 0 anduseVirtualHistorytakes itsvp <= 0branch: last 30 items only, behind a large blanktopSpacer. On a long transcript that is worse than the bug being fixed.Verification
main's baseline (same 8 pre-existing failures). The resize regression test from fix(tui): stop the scrollbar from blanking the transcript on resize #775 stays green.tsc --noEmitclean. The one eslint error inappLayout.tsxis a pre-existing import-order issue, present on pristinemainand untouched here.🤖 Generated with Claude Code