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.

Sign up for GitHub

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

Jump to bottom

fix: fix minor bugs #1070

Merged
merged 3 commits into from
Aug 31, 2023
Merged

fix: fix minor bugs #1070

merged 3 commits into from
Aug 31, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Aug 31, 2023 •
edited by gozineb
Loading

Fixed:

  • Fix share brain with multiple people
  • Fix flaky behaviour of input question

Improvement:

  • Render empty popver on @brain in order to still se the "create brain" option

@mamadoudicko mamadoudicko temporarily deployed to preview August 31, 2023 07:21 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview August 31, 2023 07:21 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Aug 31, 2023 •
edited
Loading

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 31, 2023 8:11am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 31, 2023 8:11am

@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023 •
edited
Loading

Risk Level 2 - /home/runner/work/quivr/quivr/backend/repository/brain_subscription/subscription_invitation_service.py

The code seems to be well written and follows the SOLID principles. However, there is a minor issue with the order of operations in the create_or_update_subscription_invitation method. The get_brain_for_user function is called only if user_id is not None, but it should be called regardless of the user_id value. Here is the suggested change:

if user_id is not None:
    brain_user = get_brain_for_user(user_id, brain_subscription.brain_id)

Change to:

brain_user = get_brain_for_user(user_id, brain_subscription.brain_id) if user_id else None

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ActionsBar/components/ChatInput/components/ChatBar/components/MentionInput/hooks/helpers/getEditorText.ts

The new function getEditorText seems to be implemented correctly. However, the use of replace in a loop could potentially be a performance issue if there are a large number of mentions. Consider using a more efficient method to remove all mentions from the content.

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ActionsBar/components/ChatInput/components/ChatBar/components/MentionInput/hooks/useMentionInput.tsx

There are several changes in this file, including the addition of new state variables and changes to the onAddMention, onSearchChange, keyBindingFn, and handleEditorChange functions. These changes could potentially introduce bugs or performance issues. For example, the onAddMention function now has a dependency on analytics, setCurrentBrainId, and setCurrentPromptId, which could cause unexpected behavior if these dependencies change. Consider adding tests to ensure these functions work as expected.

📝🐛🚀

Powered by Code Review GPT

@mamadoudicko mamadoudicko temporarily deployed to preview August 31, 2023 08:09 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview August 31, 2023 08:09 — with GitHub Actions Inactive
gozineb
gozineb reviewed Aug 31, 2023
View reviewed changes
...mponents/ChatInput/components/ChatBar/components/MentionInput/hooks/helpers/getEditorText.ts
let content = editorState.getCurrentContent().getPlainText();

for (const mention of mentions) {
content = content.replace(`@#${mention}`, "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny risk of collision here :/

@gozineb gozineb merged commit bd2358f into main Aug 31, 2023
@StanGirard StanGirard mentioned this pull request Aug 30, 2023
Merged
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
@mamadoudicko
fix: fix minor bugs (#1070)
ea01614 
* fix: update get_user_id_by_user_email type

* fix: update getEditorText logic

* feat(mentionSuggestions): render empty popover
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gozineb gozineb gozineb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mamadoudicko @gozineb