Fix chat scrollbar not reaching bottom during streaming via scroll anchoring - #1014
Conversation
…choring While the agent is thinking/streaming, the last assistant row grows in place (no new Props.Entries, no ScrollToBottomToken bump, no reliable SizeChanged). The ItemsRepeater extent estimate climbs across realization frames, leaving the offset a few px short of the bottom with nothing to re-pin it -- the "scrollbar doesn't reach the bottom while the agent is thinking" bug. Replace the earlier reactive ViewChanged re-pin (which painted an intermediate short frame -> jitter, and whose programmatic ChangeView fought the user's own drag) with WinUI ScrollViewer scroll anchoring: - Set sv.VerticalAnchorRatio = 1.0 in the one-time ScrollViewer init so the bottom-most realized row stays glued to the viewport bottom BEFORE paint as the extent grows. The ViewChanged handler no longer re-pins; it only refreshes metrics and drives the load-earlier trigger. - QueuePreservePrependOffset toggles anchoring to NaN during the manual load-earlier delta correction and restores it to 1.0 afterward, with a fail-safe restore if a dispatcher enqueue is rejected or the correction throws. Discrete events (append, session switch, initial load, ScrollToBottom token) still use QueueScrollToBottom; anchoring only covers the in-place growth case. Known residual (acceptable temporary fix): slight jitter as the viewport expands during virtualization/streaming. Tracked on issue openclaw#996 (Reactor port). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 2:24 PM ET / 18:24 UTC. Summary Reproducibility: yes. at source level: in-place growth of the final virtualized assistant row bypasses ordinary append/token follow paths, and the PR adds focused UI proofs for that behavior. The supplied video also demonstrates the affected streaming path, although the current-head full UI test result is still pending. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land the bounded scroll-follow improvement only after the current-head test matrix succeeds and maintainers explicitly accept the documented resize residual as tracked follow-up work in #996. Do we have a high-confidence way to reproduce the issue? Yes, at source level: in-place growth of the final virtualized assistant row bypasses ordinary append/token follow paths, and the PR adds focused UI proofs for that behavior. The supplied video also demonstrates the affected streaming path, although the current-head full UI test result is still pending. Is this the best way to solve the issue? Yes, provisionally: native scroll anchoring plus bounded settling is narrower than post-layout scroll fighting and the PR tests user intent and prepend restoration. The remaining resize drift means maintainers must decide whether this partial fix is acceptable before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against bd6c4bf8e575. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (6 earlier review cycles)
|
Using native The current proof/test coverage is not sufficient to merge yet:
Please also document or verify the The code approach looks promising, but the tests currently prove only the happy bottom-pinned path and have not themselves been demonstrated to run. This is a UI behavior change, so current-head native proof is the remaining gate. |
@karkarl, thank you for working on this—the native anchoring approach looks promising. Could you please add a short current-head screen recording? A static screenshot cannot demonstrate the timing/scroll behavior we need to verify. Ideally the recording would show:
Please also include the exact PR SHA and the command/result for the new Tray UI tests. If the UI test suite is difficult to run locally, a link to the current-head CI job/artifact is fine once it demonstrably executes the new test. That proof plus deterministic eventual/polling assertions in place of fixed compositor delays would give maintainers a much stronger basis to land the change. Thanks! |
clawsweeper
Bot
added
proof: 🎥 video
…ic drain Address PR review feedback (shanselman + clawsweeper) requesting deeper test coverage of the VerticalAnchorRatio=1.0 scroll-anchoring fix. Tests (tests/OpenClaw.Tray.UITests/ChatTimelineVirtualizationProofTests.cs): - UserScrolledUpDuringStreaming_KeepsReadingPositionStable: proves streaming growth of the off-screen newest row does NOT yank a scrolled-up user to the bottom (gap stays > FollowThreshold, reading position drift <= 24px across 4 revisions) - the "don't fight the scrollbar" half of the fix. - PrependHistory_PreservesOffset_AndRestoresBottomAnchoring: proves load-earlier prepend preserves the visible offset (shifts by inserted height) AND restores VerticalAnchorRatio to 1.0 (not left at the NaN disable sentinel), then a scroll-to-bottom token follows again end-to-end. Guards the highest-risk path that a fresh live recording cannot exercise. - Gap-value diagnostics logging on the streaming pin test (CHAT_TIMELINE_*). Determinism (UIThreadFixture.cs + test drain): - Add YieldToRenderAsync(): awaits one Low-priority dispatcher callback as a bounded render-queue drain primitive; completes immediately if the dispatcher rejects the enqueue so callers can never block. - Replace DrainRenderQueueAsync's single Task.Delay(50) with 6 bounded passes of UpdateLayout + YieldToRenderAsync + a 5ms floor. Docs (OpenClawChatTimeline.cs): - Expand the QueuePreservePrependOffset comment to document that double.NaN is the documented WinUI sentinel that disables anchoring (default value; 0.0 would anchor to the TOP edge, not disable), per the VerticalAnchorRatio contract. Validation: build.ps1 green; UITests project compiles clean (0 warn/err); Shared 2863 passed; Tray 1717 passed. UITests run on CI (they time out in the local harness); the 2 new tests are exercised by the ci.yml UITests job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
rating: 🦪 silver shellfish
… prepend preservation The ItemsRepeater SizeChanged storm during row realization fired a flood of reactive QueueScrollToBottom calls that each superseded the user's own ChangeView, so the proof tests' scroll-up never "took" and the view stayed pinned. Replace the multi-pass QueuePass stabilization with: - A coalescing guard (scrollPinPendingRef) so host.SizeChanged enqueues at most one reactive pin at a time. Held across the synchronous pin window (which can re-enter SizeChanged) and released only in terminal paths. - An authoritative ViewChanged handler: a genuine user scroll past the abandon gap cancels the in-flight settle timer/pending pin and disables anchoring immediately; symmetric re-arm to 1.0 when back in the bottom band. - A single settle timer (converges on atBottom, hands off to VerticalAnchorRatio = 1.0) instead of parallel timers fighting over ChangeView. - A per-render anchoring gate: VerticalAnchorRatio = following ? 1.0 : NaN, deferring to any pin/prepend that owns the ratio for its async window. Prepend: our FunctionalUI reconciler full-remounts every Entry (openclaw#996), which destroys the element the platform chose as the bottom anchor, so leaving VerticalAnchorRatio = 1.0 re-pinned a scrolled-up reader to the NEW bottom (offset 2528 -> 8531, gap 0). QueuePreservePrependOffset now disables anchoring and manually re-seats the offset by the measured inserted height instead, keeping the reader in the middle band (not reset to top, not dragged to bottom). Also update the ChatTimelineVirtualizationContractTests source-text assertions to the current mechanism and relax the prepend proof's anchoring assertion to the dynamic model (NaN while scrolled up, or 1.0 at bottom). Removed dead DIAG diagnostics and the unused FollowToBottomExtentEpsilon constant. Validation: build.ps1 green; Shared 2863 passed/31 skipped; Tray 1717 passed; UITests ChatTimelineVirtualizationProofTests 5/5; contract tests 3/3. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
rating: 🐚 platinum hermit
When a user scrolls above FollowThreshold (60px) but below the large abandonGap (900px) during an active settle timer, the timer's periodic PinToBottom could override their scroll intent. This happened because: 1. The timer ticks every 16ms and calls PinToBottom each tick 2. PinToBottom's ChangeView could apply before ViewChanged processed the user's scroll, masking the user's intent 3. The existing ViewChanged moderate-cancel clause races with timer ticks Fix: add a two-layer detection in the timer tick handler: Layer 1 (isFollowingRef gate): Check isFollowingRef.Current at the top of each tick AND after UpdateLayout. When the user's ChangeView applies, ViewChanged fires → UpdateScrollMetrics sets isFollowing=false (gap > 60). If this happens before/during the tick, we catch it immediately and abandon without calling PinToBottom. Layer 2 (pinConfirmed + delta): After confirming that PinToBottom successfully converged (offset reached within FollowThreshold of ScrollableHeight), track the confirmed offset. If a subsequent tick sees the pre-layout offset dropped by > 200px from the confirmed position, the user scrolled between ticks. The 200px threshold exceeds the maximum observed WinUI ItemsRepeater re-estimation drift (~140px) to avoid false-positive abandonment during normal extent convergence. Add WinUI UI proof test: ModerateScrollUpDuringSettleTimer_IsNotRepinned verifies that a 400px scroll-up during active settle timer is not re-pinned across 4 streaming revisions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b969653-0736-4a50-8ca8-f7cf06f0d38c
Remove the pinConfirmed/lastConfirmedOffset/offsetPreLayout delta heuristic from the timer tick. The isFollowingRef check at the top of each tick (and after UpdateLayout) is the correct source of truth: when the user scrolls, ViewChanged → UpdateScrollMetrics sets isFollowingRef=false, and the next tick observes it immediately. The offset-based delta duplicated this signal with worse reliability (could misclassify ItemsRepeater re-estimation drift). Keep both mechanisms: 1. Timer-tick isFollowingRef gate (essential, proven by red/green mutation) 2. ViewChanged moderate-cancel clause (supplementary early cancellation) Red/green mutation proof: - Both disabled → test FAILS (gap=0, timer re-pins user scroll) ❌ - Timer-tick isFollowingRef alone → test PASSES ✅ - Both together → test PASSES ✅ Strengthen test: bounded poll confirms settle timer is active (anchoring=NaN) before simulating user scroll, eliminating timing dependency on Task.Delay. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b969653-0736-4a50-8ca8-f7cf06f0d38c
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f0e93ff-d320-454d-b00a-865dac727d0c
@karkarl, thank you for this work. Your original change identified the right root mechanism: WinUI's native During review we extended the branch around cases that native anchoring alone does not cover reliably under the current FunctionalUI/ItemsRepeater full-remount behavior: discrete new-entry/session-switch moves, prepend offset restoration, user scroll intent during a pending bottom settle, stale session/view callbacks, resize, and cleanup. The final implementation uses native anchoring for streaming plus a bounded settle for late virtualized extent correction, with user intent cancelling that settle. We challenged the added complexity rather than assuming it was necessary. In a separate unpushed A/B branch we replaced the bounded settle with a much smaller two-dispatch one-shot design. It was simpler, but in the ordered/full UI suite it reproducibly stopped 208px short of the true bottom after earlier virtualization activity. That experiment is why we retained bounded settling. We also removed an extra independent 200px heuristic after adversarial review, so the final cancellation behavior uses the existing follow-intent signal instead of stacking another distance rule. The final proof now covers real WinUI behavior for bottom-follow during streaming, modest and larger user scroll-up, resume-to-bottom, prepend/history behavior, resize, session/visual lifecycle, accessibility, and full-suite ordering. The selected tree was revalidated after the unrelated localization baseline repair, and all hosted checks passed. The most important takeaway is that your native-anchor direction was correct. The extra machinery exists because the current virtualizer reports its final extent across later frames; once keyed reconciliation work such as #996 removes that remount limitation, some of this settling code may become removable. Thanks again for establishing the platform-correct approach and for the detailed proof work around it. |
|
Follow-up regression work is available in #1044 at Automated WinUI proof now distinguishes explicit user intent from generation-scoped programmatic scrolling. On the original regression baseline, an upward user-equivalent request reversed from The focused PR is intentionally not merge-ready until the user completes a direct normal-profile A/B and confirms a material improvement. |
2 participants
Fix chat scrollbar not reaching bottom during streaming via scroll anchoring
Problem
During streaming (agent thinking + token growth), the chat scrollbar doesn't reach the bottom and gets stuck ~30% from the newest row. Root cause: as WinUI's
ItemsRepeaterre-estimates row heights during virtualized realization, the extent climbs a few px per frame with no reliableSizeChanged/ViewChangedevent to drive a reactive re-pin.Solution
Replace the old reactive post-layout
ChangeViewre-pin with WinUI ScrollViewer scroll anchoring (sv.VerticalAnchorRatio = 1.0), which keeps the bottom-most realized row glued to the viewport bottom pre-paint as the extent grows — zero jitter, no intermediate short frame.The anchoring is dynamic: enabled (
1.0) while the view is in the bottom band (follow mode), disabled (NaN) when the user scrolls up to read earlier history. This prevents extent re-estimation during full-remount from drifting the reader's held position.Three cooperating mechanisms
Settle timer moderate-scroll fix (f791c2f → 7ee0d1b)
When a user scrolls above FollowThreshold (60px) but below the large abandonGap (900px) during an active settle timer, the timer's periodic PinToBottom overrode their scroll intent. Fixed by checking
isFollowingRef.Currentat the top of each timer tick AND afterUpdateLayout:ChangeViewfiresViewChanged→UpdateScrollMetricssetsisFollowingRef=false(gap > 60px)isFollowingRef=false→ stops timer, disables anchoring, never callsPinToBottomThis is the ViewChanged source of truth directly encoding user intent — no offset heuristics, no risk of misclassifying ItemsRepeater re-estimation drift.
Key behaviors preserved
Validation
Head:
7ee0d1b63e4b4ccc0b2cecb133738f6bf3f2d1e4Build: ✅
build.ps1— all 5 projects pass (Shared, Cli, WinNodeCli, SetupEngine, WinUI)Shared tests: ✅ 2863 passed, 31 skipped
Tray tests: ✅ 1716 passed
Tray UI tests (non-accessibility): ✅ 83/83 passed × 2 runs (no flakes, no hangs)
Accessibility tests: ✅ 18/18 passed
Focused anchoring tests (6/6):
Contract tests: ✅ 3/3 passed (ChatTimelineVirtualizationContractTests)
Red/green mutation proof
isFollowingRefchecks + ViewChanged moderate-cancel disabledisFollowingRefis the essential fixResidual risk
ChangeViewis processed AFTER a timer tick fires but BEFORE the tick'sUpdateLayout, the tick could callPinToBottomonce before observing the user's intent on the following tick. In practice, continuous scroll gestures generate multipleViewChangedevents per tick, making single-tick lag non-observable.QueuedSend_InFlightAckWithoutLifecycleis timing-sensitive (passed on re-run, unrelated to scroll anchoring).