fix(core): fix _hasManualScroll lifecycle for correct sticky scroll re-engagement#816
Draft
Matteo-DelliRocioli wants to merge 2 commits intoanomalyco:mainfrom
Draft
Conversation
…rProps The else branch in recalculateBarProps() was force-scrolling to bottom/top via _stickyScrollBottom/_stickyScrollTop without checking _hasManualScroll, causing the viewport to yank back to bottom during streaming even when the user had scrolled up. Add the same !_hasManualScroll guard that the stickyStart branch already uses, making both paths consistent.
onMouseEvent and handleKeyPress both unconditionally set _hasManualScroll = true after scroll events, overriding the correct state set by the scrollTop/scrollLeft setters and updateStickyState(). This caused _hasManualScroll to be permanently stuck on true after any scroll event — even when the user scrolled back to the bottom — because the unconditional set ran after updateStickyState() had already cleared the flag. The scrollTop/scrollLeft setters already handle _hasManualScroll correctly with isAtStickyPosition() checks, and the scrollbar onChange callbacks do the same. These redundant sets are not needed and actively break sticky scroll re-engagement.
@opentui/core
@opentui/react
@opentui/solid
@opentui/core-darwin-arm64
@opentui/core-darwin-x64
@opentui/core-linux-arm64
@opentui/core-linux-x64
@opentui/core-win32-arm64
@opentui/core-win32-x64
commit: |
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.
Summary
Two fixes to make
_hasManualScrollbehave correctly inScrollBoxRenderable:Guard the
elsebranch inrecalculateBarProps()with!this._hasManualScroll, making it consistent with thestickyStartbranch which already has this guard.Remove redundant
_hasManualScroll = truesets inonMouseEventandhandleKeyPressthat unconditionally override the correct state already managed by thescrollTop/scrollLeftsetters andupdateStickyState().Context
This is a follow-up to the work done in #531 and #722, which addressed two earlier aspects of the same underlying problem:
_hasManualScrollwas never reset, permanently breaking sticky scroll after user interaction. Fixed by clearing the flag inupdateStickyState()when the user scrolls back to the sticky edge._hasManualScrollthroughupdateStickyState()during recalculation. Fixed by adding the_isApplyingStickyScrollre-entrancy guard.PR #722 solved the flag corruption (the flag is no longer accidentally cleared), but two problems remained:
Problem 1: Missing guard in
recalculateBarProps()The
elsebranch force-scrolls via_stickyScrollBottom/_stickyScrollTopwithout checking_hasManualScroll:Fix: Change
} else {to} else if (!this._hasManualScroll) {.Problem 2: Redundant
_hasManualScroll = truein input handlersBoth
onMouseEvent(line 571) andhandleKeyPress(lines 585, 591) unconditionally set_hasManualScroll = trueafter scroll events. This runs afterupdateStickyState()has already correctly cleared the flag (when the user is at the sticky edge), permanently re-setting it totrue:This means scrolling to the bottom never re-engages auto-scroll, because
_hasManualScrollis stuck ontrue.Fix: Remove the redundant sets. The
scrollTop/scrollLeftsetters already handle_hasManualScrollcorrectly withisAtStickyPosition()checks, and the scrollbaronChangecallbacks do the same. The removed code only ran in two cases:Why this matters for consumers with
stickyStartsetAny consumer using
stickyStart="bottom"(e.g., OpenCode's session view) is affected:Normal auto-scroll (user hasn't scrolled up):
_stickyStartis truthy,_hasManualScrollisfalse→true && true→ branch 1 runs →applyStickyStart("bottom")→ correctly auto-scrolls. Branch 2 is never reached.User scrolls up during streaming:
_stickyStartis still truthy, but_hasManualScrollis nowtrue→true && false→ branch 1 fails → falls to branch 2. Without the guard, branch 2 force-scrolls back to bottom.So for any consumer with
stickyStartset, the only way to reach branch 2 is when_hasManualScrollistrue— exactly the case where force-scrolling should be blocked.How it works end-to-end
_hasManualScroll = true→ guard blocks force-scrolling → viewport stays putscrollTopsetter seesisAtStickyPosition() === true→ doesn't set_hasManualScroll;updateStickyState()clears it →_hasManualScroll = false→ auto-scroll resumes on next content resizescrollToresets position →_hasManualScrollclears → sticky scroll re-engagesTest plan
stickyStart="bottom"and withoutstickyStartset