feat: support more user-friendly scrolling #5821
Conversation
|
@Sherlocksuper is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/components/chat.tsx (3)
971-975: Simplify the calculation oftopDistanceTo improve code readability, consider simplifying the calculation of
topDistanceby using optional chaining and moving calculations into a single return statement.Apply this diff:
-return topDistance < 100; +return ( + lastMessage.getBoundingClientRect().top - + scrollRef.current.getBoundingClientRect().top < + 100 +);
963-976: OptimizeisAttachWithTopfunction for performanceThe
isAttachWithTopfunction recalculates values on every render due to dependencies onsession.messagesandscrollRef. Sincesession.messagescan change frequently, it's important to ensure this function remains performant.Consider memoizing computations or minimizing dependencies to enhance performance.
🧰 Tools
🪛 eslint
[error] 963-963: React Hook "useCallback" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
963-976: Provide fallback or default behavior for scrolling when messages are absentIn the
isAttachWithTopfunction, ifsession.messagesis empty,lastMessageIdcould be undefined, potentially causing issues. It's important to handle this edge case.Consider adding a default return value or handling when there are no messages.
🧰 Tools
🪛 eslint
[error] 963-963: React Hook "useCallback" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/components/chat.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
app/components/chat.tsx
[error] 963-963: React Hook "useCallback" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
[error] 982-982: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
🔇 Additional comments (3)
app/components/chat.tsx (3)
980-984: Adjust the condition for auto-scrolling behavior
The condition passed to useScrollToBottom may not correctly reflect the desired scrolling behavior, especially considering the isTyping state and isAttachWithTop() result.
Please verify that the condition (isScrolledToBottom || isAttachWithTop()) && !isTyping accurately represents when auto-scrolling should occur. If not, consider revising the logic to match the intended behavior described in the PR objectives.
🧰 Tools
🪛 eslint
[error] 982-982: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
978-979: Ensure isTyping state accurately reflects user input
The check const isTyping = userInput !== ""; determines if the user is currently typing. Verify that userInput accurately captures the user's input state at all relevant times, especially when images are being attached or uploaded.
Ensure that the isTyping state is correctly managed throughout the component lifecycle to prevent unexpected auto-scrolling behavior.
982-984: Verify the dependencies passed to useScrollToBottom
The dependencies for useScrollToBottom function include scrollRef and a complex condition. Ensure that all necessary dependencies are included and that the function behaves as expected when these dependencies change.
Review the dependencies array and update it if necessary to prevent potential bugs related to stale closures.
🧰 Tools
🪛 eslint
[error] 982-982: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
Sherlocksuper
force-pushed
the
scroll
branch
from
6dfee8e to
e6b9c07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/components/chat.tsx (2)
963-977: Consider extracting magic number and adding commentsThe implementation looks good, but could benefit from some minor improvements:
- Extract the magic number
100into a named constant for better maintainability- Add a comment explaining the distance threshold's purpose
+// Threshold in pixels to determine if the last message is close enough to the viewport top +const TOP_DISTANCE_THRESHOLD = 100; + const isAttachWithTop = useCallback(() => { if (!scrollRef.current) return false; const lastMessageId = session.messages[session.messages.length - 1]?.id; if (!lastMessageId) return false; const lastMessage = document.getElementById(lastMessageId); if (!lastMessage) return false; const topDistance = lastMessage.getBoundingClientRect().top - scrollRef.current.getBoundingClientRect().top; - return topDistance < 100; + return topDistance < TOP_DISTANCE_THRESHOLD; }, [session.messages, scrollRef]);🧰 Tools
🪛 eslint
[error] 963-963: React Hook "useCallback" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
1740-1740: Add a comment explaining the ID's purposeConsider adding a comment to explain that this ID is used for scroll behavior management.
+ {/* Message ID used by isAttachWithTop for scroll behavior */} id={message.id}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/components/chat.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
app/components/chat.tsx (1)
Learnt from: Sherlocksuper
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5821
File: app/components/chat.tsx:963-963
Timestamp: 2024-11-14T02:52:38.538Z
Learning: 在此项目中，拉取请求（PR）应只包含一个功能或修改，其他改动应在单独的 PR 中提交。
🪛 eslint
app/components/chat.tsx
[error] 963-963: React Hook "useCallback" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
[error] 983-983: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
🔇 Additional comments (1)
app/components/chat.tsx (1)
982-985: LGTM! Smart scroll behavior implementation
The scroll behavior implementation effectively addresses the PR objectives by:
- Auto-scrolling when the user is typing
- Disabling auto-scroll for long AI responses when the message is not near the viewport top
🧰 Tools
🪛 eslint
[error] 983-983: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
Sherlocksuper
force-pushed
the
scroll
branch
from
e6b9c07 to
9cacaa2
Compare
Sherlocksuper
force-pushed
the
scroll
branch
from
9cacaa2 to
19facc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/components/chat.tsx (2)
963-972: Consider performance optimizations for DOM measurementsThe
isAttachWithTopfunction performs DOM measurements within auseMemohook. While the implementation is functionally correct, there are a few considerations:
- DOM measurements can trigger layout reflows
- The calculations are performed on every render where the dependencies change
Consider using ResizeObserver or debouncing the measurements to reduce performance impact. Here's a potential optimization:
-const isAttachWithTop = useMemo(() => { +const isAttachWithTop = useCallback(() => { const lastMessage = scrollRef.current?.lastElementChild as HTMLElement; if (!scrollRef?.current || !lastMessage) return false; const topDistance = lastMessage!.getBoundingClientRect().top - scrollRef.current.getBoundingClientRect().top; return topDistance < 100; -}, [scrollRef?.current?.scrollHeight]); +}, []); + +const debouncedIsAttachWithTop = useMemo( + () => debounce(isAttachWithTop, 100), + [isAttachWithTop] +);🧰 Tools
🪛 eslint
[error] 963-963: React Hook "useMemo" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
1735-1735: Consider using a more semantic id formatWhile adding an id attribute is good for DOM operations, using a more semantic format could improve maintainability.
Consider using a more descriptive format:
-id={message.id} +id={`chat-message-${message.id}`}This makes it clearer that the element is a chat message when inspecting the DOM or using selectors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/components/chat.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
app/components/chat.tsx
[error] 963-963: React Hook "useMemo" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
[error] 978-978: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
🔇 Additional comments (1)
app/components/chat.tsx (1)
976-980: LGTM! Improved scroll behavior implementation
The modifications to useScrollToBottom effectively implement the desired scrolling behavior:
- Auto-scrolls when user is typing
- Maintains scroll position when viewing previous messages
- Considers message position relative to viewport
🧰 Tools
🪛 eslint
[error] 978-978: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/components/chat.tsx (1)
963-972: Consider improving function name and extracting magic numberThe memoization is well-implemented, but a few improvements could make the code more maintainable:
- Rename
isAttachWithTopto something more descriptive likeisLastMessageNearToporshouldAutoScrollDisabled- Extract the magic number 100 into a named constant like
SCROLL_THRESHOLD_PX+const SCROLL_THRESHOLD_PX = 100; + const isAttachWithTop = useMemo(() => { const lastMessage = scrollRef.current?.lastElementChild as HTMLElement; // if scrolllRef is not ready or no message, return false if (!scrollRef?.current || !lastMessage) return false; const topDistance = lastMessage!.getBoundingClientRect().top - scrollRef.current.getBoundingClientRect().top; // leave some space for user question - return topDistance < 100; + return topDistance < SCROLL_THRESHOLD_PX; }, [scrollRef?.current?.scrollHeight]);🧰 Tools
🪛 eslint
[error] 963-963: React Hook "useMemo" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/components/chat.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
app/components/chat.tsx
[error] 963-963: React Hook "useMemo" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
[error] 978-978: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
🔇 Additional comments (1)
app/components/chat.tsx (1)
974-980: LGTM! Scroll behavior enhancement implemented correctly
The updated scroll behavior successfully implements the PR objective by:
- Auto-scrolling when user is typing
- Auto-scrolling when already at bottom
- Disabling auto-scroll when the last message is near the top
This improves the user experience by preventing unwanted scrolling when reading previous messages.
🧰 Tools
🪛 eslint
[error] 978-978: React Hook "useScrollToBottom" is called in function "_Chat" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".
(react-hooks/rules-of-hooks)
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
支持更人性化滚动设置
附件为演示视频
📝 补充信息 | Additional Information
相关issue:
#5798
可能优化的点：或许我们可以使用ref来标记最后一条消息，而不是findElementById，这样性能上会有优化（可能会很小），不过我担心这样在代码结构上影响较大并不美观 ?
1731500423219.zip
Summary by CodeRabbit
New Features
Improvements