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: track prompt and brain changes #1058

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Conversation

mamadoudicko
Copy link
Contributor

No description provided.

@mamadoudicko mamadoudicko temporarily deployed to preview August 29, 2023 15:53 — with GitHub Actions Inactive
@vercel
Copy link

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/lib/context/BrainProvider/hooks/useBrainProvider.ts

The code seems to be well written and follows good practices. However, there are a few points that could be improved:

  1. Avoid using console.error and console.warn: These are used for debugging during development and should not be present in production code. It would be better to handle the errors properly and show a user-friendly message if necessary.
try {
  const brains = await getBrains();
  setAllBrains(brains);
} catch (error) {
  // handle error properly
}
  1. Avoid using empty dependency array in useCallback: An empty dependency array means that the callback will only be created once, similar to componentDidMount. However, it's not a good practice because it can lead to bugs if the callback depends on some props or state. It would be better to specify the dependencies or remove the useCallback if it's not necessary.
const setActiveBrain = useCallback(
  ({ id, name }: { id: UUID; name: string }) => {
    const newActiveBrain = { id, name };
    saveBrainInLocalStorage(newActiveBrain);
    setCurrentBrainId(id);
  },
  [saveBrainInLocalStorage, setCurrentBrainId]
);

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

The code seems to be well written and follows good practices. However, there are a few points that could be improved:

  1. Avoid using void operator: The void operator is used before the analytics.track function. This operator is usually used to ignore a promise that you don't want to wait for its resolution. However, it's not a good practice because it can lead to unhandled promise rejections. It would be better to handle the promise properly with then and catch blocks or async/await.
analytics?.track(\"CHANGE_PROMPT\").catch(error => console.error(error));
  1. Avoid using non-null assertion operator: The non-null assertion operator (!) is used when you are sure that the value is not null or undefined. However, it's not a good practice because it can lead to runtime errors. It would be better to handle the possible null or undefined values properly.
setCurrentPromptId(mention.id as UUID ?? someDefaultValue);

🚫🐞🔍


Powered by Code Review GPT

@gozineb gozineb merged commit 1b63141 into main Aug 30, 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.

2 participants