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: add notifications components #1148

Merged
merged 19 commits into from
Sep 12, 2023
Merged

feat: add notifications components #1148

merged 19 commits into from
Sep 12, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Sep 11, 2023

  • Add notifications component

#1131

Screen.Recording.2023-09-12.at.17.01.45.mp4

@vercel
Copy link

vercel bot commented Sep 11, 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 Sep 12, 2023 1:40pm
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2023 1:40pm

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2023

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

The code changes are generally safe, but there is a potential risk. The developer has added a new state variable messages from the useChatContext hook. However, it's not clear how this state is managed. If not handled properly, it could lead to unexpected behavior in the application. It's recommended to ensure that the state management for messages is robust and handles all edge cases.


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

The code changes are generally safe, but there is a potential risk. The developer has added a try-catch block to handle errors when parsing the data. However, the error is only logged to the console and not handled in any other way. This could lead to silent failures in the application. It's recommended to handle the error in a way that either recovers from the error or informs the user about the error.


Risk Level 3 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ChatDialogueArea/components/ChatDialogue/hooks/useChatDialogue.ts

The useChatDialogue hook uses a hardcoded value for chatInputHeightEstimation. This could lead to issues if the actual height of the chat input changes. Consider calculating this value dynamically or passing it as a prop.

Also, the scrollToBottom function is debounced using lodash's _debounce function. This might not be necessary if the function is not called frequently. Removing the debounce could improve performance.

Lastly, the computeCardHeight function is called on every window resize event. This could be optimized by debouncing the event listener callback.

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

// Pass chatInputHeight as a prop
export const useChatDialogue = (chatInputHeight: number) => {
  // ...
  const computeCardHeight = useCallback(() => {
    // ...
    const cardHeight = windowHeight - cardTop - chatInputHeight;
    // ...
  }, [chatInputHeight]);
  // ...
  useEffect(() => {
    const debouncedComputeCardHeight = _debounce(computeCardHeight, 100);
    window.addEventListener(\"resize\", debouncedComputeCardHeight);
    return () => {
      window.removeEventListener(\"resize\", debouncedComputeCardHeight);
    };
  }, [computeCardHeight]);
  // ...
}

🔍📝⚠️


Powered by Code Review GPT

@gozineb gozineb merged commit 10af0c9 into main Sep 12, 2023
8 of 9 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.

2 participants