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: update header and remove prompt / brain on backspace #1052

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

mamadoudicko
Copy link
Contributor

  • Update header turn setting icon into brain icon and remove brain dropdown
  • Remove selected prompt / brain on backspace

@mamadoudicko mamadoudicko temporarily deployed to preview August 28, 2023 16:58 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Aug 28, 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 29, 2023 0:34am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2023 0:34am

@github-actions
Copy link
Contributor

github-actions bot commented Aug 28, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/invitation/[brainId]/hooks/useInvitation.ts

The function useInvitation is quite large and complex. Consider breaking it down into smaller, more manageable functions. This will improve readability and maintainability.


Risk Level 2 - /home/runner/work/quivr/quivr/frontend/lib/components/BrainUsers/components/BrainUser/hooks/useBrainUser.ts

The error handling could be improved. Currently, the error message is directly displayed to the user. It would be better to have a more user-friendly message and log the actual error for debugging. For example:

if (axiosError !== undefined && axiosError.status === 403) {
  publish({ 
    variant: \"danger\",
    text: t(\"userRoleUpdateFailed\", { email: email, role: newRole, ns: \"brain\" }),
  });
  console.error(axiosError.message);
} else {
  publish({
    variant: \"danger\",
    text: t(\"userRoleUpdateFailed\", { email: email, role: newRole, ns: \"brain\" }),
  });
  console.error(e);
}

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/lib/hooks/useShareBrain.ts

  1. The inviteUsers function is quite large and complex. It would be beneficial to break it down into smaller, more manageable functions. This would improve readability and maintainability.

  2. The inviteUsers function has a side effect of setting state within a try/catch/finally block. This could potentially lead to unexpected behavior if the component unmounts before the async operation completes. Consider using an isMounted pattern to prevent this.

const isMounted = useRef(true);

useEffect(() => {
  return () => {
    isMounted.current = false;
  };
}, []);

// Then, in your async function:
if (isMounted.current) {
  setSendingInvitation(false);
}
  1. The inviteUsers function does not handle network errors. If the addBrainSubscriptions request fails due to a network error, the error will not be caught and the app could crash. Consider adding a catch block for network errors.
} catch (error) {
  if (axios.isAxiosError(error)) {
    // Handle Axios errors
  } else {
    // Handle network errors
  }
}

📚🔧⚠️


Powered by Code Review GPT

@gozineb
Copy link
Contributor

gozineb commented Aug 29, 2023

I miss your loom videos mamadou 😭

@mamadoudicko mamadoudicko merged commit c5a7b8f into main Aug 29, 2023
Colin-coder pushed a commit to Colin-coder/quivr that referenced this pull request Aug 30, 2023
)

* feat: update header

* feat: remove selected prompt / brain on backspace

* feat(chat): update suggestions component

* refactor: add getAxiosErrorParams
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
* feat: update header

* feat: remove selected prompt / brain on backspace

* feat(chat): update suggestions component

* refactor: add getAxiosErrorParams
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