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 tanstack query and optimistic fetch on brains settings page #1087

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Sep 1, 2023

Before:

Screen.Recording.2023-09-01.at.12.38.30.mov

After:

Screen.Recording.2023-09-01.at.12.39.13.mov

To improve UX on first load, we can add a loader... WDYT @Chloeroumengas @danielcgilibert @lamiabnn ?

@vercel
Copy link

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

@mamadoudicko mamadoudicko temporarily deployed to preview September 1, 2023 10:38 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/App.tsx

The code seems to be well written and follows good practices. However, there is a potential issue with the useEffect hook. The dependencies array includes session?.user which means the effect will run every time the user property of session changes. If session is null or undefined, this could lead to unnecessary re-renders. Consider checking if session is defined before including session.user in the dependencies array.

useEffect(() => {
  void fetchAllBrains();
  void fetchAndSetActiveBrain();
  void fetchPublicPrompts();
}, [session && session.user]);

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/app/brains-management/[brainId]/components/BrainManagementTabs/components/SettingsTab/hooks/useSettingsTab.ts

  1. The useEffect hook that updates the form values when brain changes might cause unnecessary re-renders. Consider using the useMemo hook to memoize the form values based on brain to avoid unnecessary re-renders.
const formValues = useMemo(() => {
  // calculate form values based on brain
}, [brain]);
  1. The handleSubmit function is quite large and complex. Consider breaking it down into smaller, more manageable functions. This will make the code easier to read and maintain.

  2. The setAsDefaultBrainHandler function sets isSettingAsDefault to true but does not set it back to false in case of an error. This could lead to a state where the UI indicates that the operation is still in progress even though it has failed. Consider adding a finally block to ensure isSettingAsDefault is always set back to false.

try {
  setIsSettingAsDefault(true);
  // operation
} catch (error) {
  // handle error
} finally {
  setIsSettingAsDefault(false);
}

🔄🧠🔧


Powered by Code Review GPT

@mamadoudicko
Copy link
Contributor Author

mamadoudicko commented Sep 1, 2023

Nice PR. Love it!

🫶🏽

Copy link
Contributor

@gozineb gozineb left a comment

Choose a reason for hiding this comment

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

Magnifique ! Thanks mamadz !!

@mamadoudicko mamadoudicko merged commit bea275f into main Sep 1, 2023
15 checks passed
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
…#1087)

* refactor: remove <TrackingWrapper />

* feat: add tanstack query

* feat: add optimistic fetch to brain settings page
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