Skip to content

fix(ui): stabilize streaming chat scroll follow#375

Open
VooDisss wants to merge 5 commits into
NeuralNomadsAI:devfrom
VooDisss:upstream-dev-vfl-only-test
Open

fix(ui): stabilize streaming chat scroll follow#375
VooDisss wants to merge 5 commits into
NeuralNomadsAI:devfrom
VooDisss:upstream-dev-vfl-only-test

Conversation

@VooDisss
Copy link
Copy Markdown
Contributor

Summary

  • Fix sticky chat auto-scroll during streaming by making follow exit/re-entry more reliable in VirtualFollowList.
  • Prevent visible snapbacks by using DOM scroll state as the source of truth for immediate/manual scroll behavior.
  • Keep the upstream surface minimal: this change is scoped to packages/ui/src/components/virtual-follow-list.tsx only.

Testing

  • npm run typecheck --workspace @codenomad/ui
  • npm run build --workspace @codenomad/ui
  • Manually verified:
    • single-notch upward exit from auto-scroll
    • downward re-entry near bottom
    • normal browsing while escaped
    • no visible snapback during rapid streamed output

Review Notes

  • Maintainers may edit or adjust this PR as needed.
  • I will ignore gatekeeper bot reviews for this PR.

Stabilize the chat viewport scroll-follow behavior during streaming with a smaller, one-file fix centered on VirtualFollowList.

The original failure mode was that streaming output could keep follow mode sticky or trigger visible snapbacks when the user tried to inspect earlier parts of a long response. The important fixes are to treat upward wheel input as an immediate exit from follow, to allow native browsing while escaped, to rejoin follow only when the viewport deliberately returns near the bottom, and to rely on the DOM scroller as the source of truth when Virtua's internal scroll offset lags behind real manual movement.

This version keeps the fix narrowly scoped to virtual-follow-list.tsx. Immediate programmatic top/bottom moves use direct DOM scroll position writes, content-growth follow is coalesced through scrollHeight checks, and the state machine is trimmed down to the minimal follow/escape geometry that still preserves smooth exit, smooth re-entry, and no visible snapback during long streamed responses.
@shantur
Copy link
Copy Markdown
Collaborator

shantur commented Apr 27, 2026

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented Apr 27, 2026

Gatekeeper review for latest PR state completed.

Request understood: review PR #375 ruthlessly for regressions and better implementation options. I treated this as review-only and did not make code changes.

Status: request changes

Findings:

  1. Blocking: the long-assistant-reply hold feature is now effectively dead code.

    message-section.tsx still computes and passes autoPinHoldTargetKey, autoPinHoldTopThresholdPx, and resolveAutoPinHoldElement into VirtualFollowList at packages/ui/src/components/message-section.tsx:638 and packages/ui/src/components/message-section.tsx:1089-1094. It also still renders the pause/hold toggle at packages/ui/src/components/message-section.tsx:1127-1144.

    But VirtualFollowList no longer reads those props. The old hold-target state and geometry logic were removed, while the public props remain in VirtualFollowListProps at packages/ui/src/components/virtual-follow-list.tsx:88-103.

    This is a behavior regression: users can toggle “hold long assistant replies”, the UI indicates it is active, but the list never honors the hold target. For long streaming assistant messages that exceed the viewport, the PR will keep forcing bottom follow instead of holding the growing message near the top edge. Either restore a minimal hold implementation or remove the preference/UI/props in the same PR. Leaving a live control backed by no behavior is not acceptable.

  2. Blocking: reset/session changes now reuse stale initial follow state.

    initialFollowEnabled is captured once at component initialization in packages/ui/src/components/virtual-follow-list.tsx:157, then reused on every reset in packages/ui/src/components/virtual-follow-list.tsx:471.

    Before this PR, reset used the current initialAutoScroll() value. That matters because message-section.tsx derives initialAutoScroll from the current session scroll snapshot at packages/ui/src/components/message-section.tsx:605-606, and VirtualFollowList explicitly supports resetKey for reusing the same list instance across different datasets.

    Regression scenario: switch from a session restored at bottom to a session restored away from bottom, or vice versa, while the same component instance survives. The reset path keeps the first session’s follow state instead of the new session snapshot, so subsequent streaming can unexpectedly auto-follow or fail to auto-follow.

    Suggested fix: do not cache initialAutoScroll() into a one-time initialFollowEnabled for reset logic. Compute the current initial follow state during reset, e.g. const followEnabled = Boolean(initialAutoScroll()), then sync both autoScroll and escaped/locked state from that current value.

  3. Major: setAutoScroll no longer maintains the internal escaped/follow-lock state.

    The public API implementation at packages/ui/src/components/virtual-follow-list.tsx:443 only calls setAutoScroll(Boolean(enabled)). The new follow model also depends on escapedFromLock, and several follow paths require !escapedFromLock() before scheduling bottom follow, e.g. packages/ui/src/components/virtual-follow-list.tsx:237 and packages/ui/src/components/virtual-follow-list.tsx:266.

    That means a consumer can call api.setAutoScroll(true) and observe getAutoScroll() === true, while the internal escaped state still prevents automatic bottom following on rendered content. This API is now misleading. It should route through the same state transitions as the rest of the component: enabling should rejoin follow, disabling should escape follow.

Implementation quality notes:

  • The PR is correctly scoped to one component, but the implementation is not actually minimal because it removes an existing feature path while preserving the external API and UI affordance.
  • cancelPendingContentGrowthCheck() at packages/ui/src/components/virtual-follow-list.tsx:200 is unused. Not itself a behavioral blocker, but it is a sign the new frame-cancellation model needs cleanup before merge.
  • scrollToBottom(true) now writes element.scrollTop = element.scrollHeight directly at packages/ui/src/components/virtual-follow-list.tsx:369. That may be intentional to prefer DOM state, but with a virtualizer in the loop this needs confidence/testing around large virtualized histories. The stronger blocker remains the removed hold behavior.

Verification:

  • git diff --check origin/dev...HEAD: passed.
  • npm run typecheck --workspace @codenomad/ui: failed before giving a useful PR-specific signal due broad existing/missing dependency/type resolution issues in this checkout, including missing solid-js, @opencode-ai/sdk, and many unrelated implicit-any errors outside the touched file.
  • npm run build --workspace @codenomad/ui: failed because vite could not be resolved from the local workspace dependencies.

Gatekeeper verdict: do not merge as-is. The PR likely fixes one scroll snapback symptom, but it regresses the long-stream hold control and introduces inconsistent follow-state semantics. Fix the state reset/API issues and either restore or intentionally remove the hold feature/UI before this should pass review.

--
Yours,
CodeNomadBot

@pascalandr
Copy link
Copy Markdown
Contributor

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented Apr 30, 2026

Gatekeeper review for the latest PR #375 state completed.

Request understood: perform another ruthless gatekeeper pass for regressions and better implementations. I treated this as review-only and did not make code changes.

Status: no new blocking PR-specific findings from static review

What changed since the previous gatekeeper review:

  • The long-assistant-reply hold path is no longer dead code. VirtualFollowList now consumes autoPinHoldTargetKey, preserves activeHoldTargetKey, and calls updateAutoPinHold() from notifyContentRendered().
  • The reset/session bug called out previously is addressed. Reset now reads the current initialAutoScroll() value instead of reusing the first mount’s captured follow value.
  • The public setAutoScroll() API now goes through rejoinFollow() / escapeFromFollow(), so the internal escaped-lock state is kept consistent with the exposed auto-scroll state.

Remaining review notes:

  • The implementation is still delicate. There are now three coupled pieces of state for follow behavior: autoScroll, escapedFromLock, and activeHoldTargetKey. The latest version is coherent enough from static inspection, but this area needs real browser/manual coverage because small ordering mistakes in scroll handlers are easy to miss in code review.
  • scrollToBottom(true) intentionally writes element.scrollTop = element.scrollHeight directly. That matches the PR’s goal of using DOM scroll state as the immediate source of truth, but it should be manually verified with long virtualized histories, not only short streaming chats.
  • packages/ui/src/components/virtual-follow-list.tsx is now about 645 lines. That is above the repo’s source-file warning threshold. I would not block this PR for it because the scope is intentionally isolated, but future changes should consider extracting the follow/hold state machine or scroll-intent handling.

Verification:

  • git diff --check origin/dev...HEAD: passed.
  • npm run typecheck --workspace @codenomad/ui: failed because this checkout cannot resolve workspace dependencies/types such as solid-js, @opencode-ai/sdk, Tauri packages, and Node test types. The failure is broad and not specific to the touched file.
  • npm run build --workspace @codenomad/ui: failed because vite cannot be resolved from local workspace dependencies.

Gatekeeper verdict: the previously reported blockers appear resolved in the latest PR state. I do not see a remaining static-code blocker, but I would still require a clean dependency-installed CI run or maintainer-local run of the advertised UI typecheck/build plus manual scroll scenarios before merge.

--
Yours,
CodeNomadBot

@pascalandr
Copy link
Copy Markdown
Contributor

pascalandr commented May 2, 2026

@VooDisss
I’m seeing reproducible regressions in the latest PR state during manual testing:

  • the overlay up/down scroll buttons can desync the timeline highlight from the visible message
  • auto-scroll can sometimes stop short of the actual bottom, as if hold/follow state was not fully released

I’m still narrowing down ...

@pascalandr
Copy link
Copy Markdown
Contributor

pascalandr commented May 2, 2026

So I found multiple reproducible regressions in the latest PR state:

  1. Overlay up/down scroll buttons can desync the timeline from the visible message.
    message-section.tsx drives the overlay arrows through api.scrollToTop() / api.scrollToBottom(), and both paths set ignoreNextScrollEvent in virtual-follow-list.tsx. The next handleScroll() then returns early after updating scroll buttons only, without recomputing activeKey / onActiveKeyChange. Since the timeline highlight is driven from onActiveKeyChange, the viewport can move while the timeline stays on the previous segment.

I reproduced this in both Tauri and Electron builds with both overlay arrows.

  1. Explicit “go to latest” actions do not reliably stay at the actual bottom.
    scrollToBottom() can immediately be followed by hold logic on the next render via notifyContentRendered() -> updateAutoPinHold(). Since holdLongAssistantReplies is enabled by default, an explicit bottom action (for example the bottom arrow, or End) can go to the bottom and then jump slightly back up instead of staying on the latest content.

This is not just a UX preference issue: explicit “go to latest” should not behave as if hold mode silently overrode it.

  1. Scrolling during auto-follow can leave streaming effectively stuck.
    A single upward wheel event immediately calls escapeFromFollow(), which cancels pending follow work, clears hold, sets escapedFromLock = true, and disables autoScroll. After that, content growth no longer follows until the component explicitly rejoins follow again. In practice this makes streaming feel broken after even a small manual scroll, and the re-entry behavior is inconsistent across wheel, keyboard, and programmatic navigation.

  2. Keyboard downward navigation is not handled consistently with wheel downward navigation.
    The code computes ArrowDown / PageDown / End as downward intent, but unlike the wheel path it does not use that to rejoin follow or perform the same near-bottom handling. That makes the keyboard behavior inconsistent and is likely part of the “End goes to bottom and then behaves oddly” problem.

@VooDisss do you see the same problemes ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants