Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(messagesList): auto scroll on new message #1040

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

mamadoudicko
Copy link
Contributor

Description

Screen.Recording.2023-08-25.at.11.58.03.mov

@vercel
Copy link

vercel bot commented Aug 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 25, 2023 10:01am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 25, 2023 10:01am

@mamadoudicko mamadoudicko temporarily deployed to preview August 25, 2023 09:58 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ChatMessages/hooks/useChatMessages.ts

The code seems to be well written, but there are a few points that could be improved for better performance and readability:

  1. The scrollToBottom function is being debounced, but it's not clear why this is necessary. If the function is being called too often, it might be better to throttle it instead of debouncing it. Debouncing might cause the scroll to lag behind, especially if there are many messages being added in quick succession.

  2. The useEffect hook that calls scrollToBottom is dependent on history and scrollToBottom. This means that it will be called every time history or scrollToBottom changes. If history changes often, this could lead to performance issues. Consider optimizing this.

  3. The computeCardHeight function is directly manipulating the DOM, which is generally not recommended in React. Consider using state to store the card height and updating it using setState.

Here's an example of how you might implement these changes:

const [cardHeight, setCardHeight] = useState(0);

const computeCardHeight = () => {
  if (chatListRef.current) {
    const cardTop = chatListRef.current.getBoundingClientRect().top;
    const windowHeight = window.innerHeight;
    const newCardHeight = windowHeight - cardTop - chatInputHeightEstimation;
    setCardHeight(newCardHeight);
  }
};

useEffect(() => {
  computeCardHeight();
  window.addEventListener(\"resize\", computeCardHeight);

  return () => {
    window.removeEventListener(\"resize\", computeCardHeight);
  };
}, []);

chatListRef.current.style.height = `${cardHeight}px`;

🔄🚀📏


Powered by Code Review GPT

@mamadoudicko mamadoudicko merged commit 66bafcf into main Aug 25, 2023
7 checks passed
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.

None yet

2 participants