Closed
Conversation
…l position When loading older messages, the assistant-ui library's autoScroll feature was interfering with our manual scroll position restoration, causing the viewport to jump to the top instead of maintaining position. Root cause: ThreadPrimitive.Viewport's autoScroll triggers scrollToBottom() on content resize when autoScroll=true and isAtBottom=true. This conflicts with our useLayoutEffect scroll position restoration. Fix: Temporarily disable autoScroll during history load: 1. Set autoScrollEnabled=false when handleLoadMore starts 2. Restore scroll position in useLayoutEffect as before 3. Re-enable autoScroll after position is restored 4. Handle error cases to ensure autoScroll is always re-enabled via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
Add detailed logging to understand why scroll position jumps to top when loading older messages instead of maintaining position. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
The previous implementation tried to restore scroll position immediately when messagesVersion changed, but the new messages hadn't been rendered to DOM yet, causing delta to be 0 and scroll position to remain at top. Fix: Use requestAnimationFrame to wait for DOM update and check if scrollHeight has actually changed before restoring position. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
Remove temporary debug logging added during development. The fix is working correctly - scroll position is now properly maintained when loading older messages. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
| loadLockRef.current = false | ||
| // Re-enable autoScroll after scroll position is restored | ||
| setTimeout(() => setAutoScrollEnabled(true), 50) | ||
| } |
There was a problem hiding this comment.
[MAJOR] UX 自动滚动状态被强制开启
Why this is a problem: 历史加载完成后即使用户不在底部也开启 autoScroll,下一条消息可能跳到底部,打断阅读。
Suggested fix:
const prevAutoScrollRef = useRef(true)
prevAutoScrollRef.current = autoScrollEnabledRef.current
setAutoScrollEnabled(false)
if (prevAutoScrollRef.current) {
setAutoScrollEnabled(true)
}|
|
||
| // If delta is 0, content hasn't been rendered yet, wait a bit | ||
| if (delta === 0 && viewport.scrollHeight === pending.scrollHeight) { | ||
| requestAnimationFrame(checkAndRestore) |
There was a problem hiding this comment.
[MINOR] Performance requestAnimationFrame 无上限轮询
Why this is a problem: delta 长期为 0 时 rAF 递归不退出,持续排队,loadLock 也可能保持等待。
Suggested fix:
let attempts = 0
const MAX_ATTEMPTS = 5
const checkAndRestore = () => {
if (!pendingScrollRef.current) return
const delta = viewport.scrollHeight - pending.scrollHeight
if (delta === 0 && attempts++ < MAX_ATTEMPTS) {
requestAnimationFrame(checkAndRestore)
return
}
viewport.scrollTop = pending.scrollTop + delta
pendingScrollRef.current = null
loadLockRef.current = false
}There was a problem hiding this comment.
Findings
- [Major] 自动滚动被强制开启 — 历史加载完成后仍开启 autoScroll,用户不在底部也可能被拉到底部。证据
web/src/components/AssistantChat/HappyThread.tsx:272
Suggested fix:const prevAutoScrollRef = useRef(true) prevAutoScrollRef.current = autoScrollEnabledRef.current setAutoScrollEnabled(false) if (prevAutoScrollRef.current) { setAutoScrollEnabled(true) }
- [Minor] requestAnimationFrame 无上限轮询 — delta 长期为 0 时 rAF 持续循环。证据
web/src/components/AssistantChat/HappyThread.tsx:263
Suggested fix:let attempts = 0 const MAX_ATTEMPTS = 5 const checkAndRestore = () => { if (!pendingScrollRef.current) return const delta = viewport.scrollHeight - pending.scrollHeight if (delta === 0 && attempts++ < MAX_ATTEMPTS) { requestAnimationFrame(checkAndRestore) return } viewport.scrollTop = pending.scrollTop + delta pendingScrollRef.current = null loadLockRef.current = false }
Summary
- 2 issues: 自动滚动状态覆盖、rAF 无上限轮询;滚动体验与性能风险
Testing
- Not run (automation)
HAPI Bot
Address code review feedback: 1. Preserve autoScroll state across history loads - Save autoScrollEnabled state before disabling it - Restore previous state instead of forcing true - Prevents unwanted scroll-to-bottom when user is viewing middle content 2. Add max attempts limit to requestAnimationFrame loop - Limit to 5 attempts to prevent infinite loop - Gracefully handle cases where DOM doesn't update - Improves performance and prevents browser hang via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
Contributor
Author
|
Closing in favor of #170 (same fix, recreated PR due to stale head SHA sync issue). |
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.
问题描述
在加载历史消息时,自动滚动功能会干扰滚动位置的恢复。
修复内容
via HAPI
Co-Authored-By: HAPI noreply@hapi.run