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 custom prompt fields on brain setting pages #837

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

mamadoudicko
Copy link
Contributor

Description

@vercel
Copy link

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

LOGAF Level 2 - /home/runner/work/quivr/quivr/frontend/lib/components/AddBrainModal/hooks/useAddBrainModal.ts

  1. The getCreatingBrainPromptId function could be improved. Currently, it checks if the prompt.title and prompt.content are not empty strings. However, it does not handle the case where prompt is undefined. Consider adding a check for prompt before accessing its properties.

Example:

if (prompt && prompt.title.trim() !== "" && prompt.content.trim() !== "") {
  1. The error handling in the handleSubmit function could be improved. Currently, it checks if the error is an Axios error and if the response status is 429. However, it does not handle other types of errors. Consider adding a general error handler to catch all other types of errors.

LOGAF Level 2 - /home/runner/work/quivr/quivr/frontend/lib/context/BrainConfigProvider/brain-config-provider.tsx

  1. The updateConfig function could be improved. Currently, it merges the new config with the existing config and then saves it to local storage. However, this could lead to outdated or incorrect data being saved if the existing config is not up-to-date. Consider fetching the latest config from the state before merging it with the new config.

Example:

const updatedConfig: BrainConfig = {
  ...brainConfig,
  ...removeUndefined(newConfig),
};
  1. The useEffect hook could be improved. Currently, it sets the brain config to the value from local storage or the default config when the component mounts. However, this could lead to unnecessary re-renders if the value from local storage is the same as the current state. Consider adding a check to only update the state if the value from local storage is different.

LOGAF Level 3 - /home/runner/work/quivr/quivr/frontend/lib/components/AddBrainModal/AddBrainModal.tsx

  1. The OpenAI API Key is being exposed in the form field. This is a security risk and should be handled in a more secure way. Consider storing the API key in a secure environment variable or secure storage.

  2. The form submission handling could be improved. Currently, the form submission is prevented and then the handleSubmit function is called. Instead, you could pass the handleSubmit function directly to the form's onSubmit prop. This would automatically prevent the default form submission and call your handleSubmit function with the form data.

Example:

<form onSubmit={handleSubmit} className="my-10 flex flex-col items-center gap-2">
  1. The void keyword is used before handleSubmit(). This is unnecessary and can be removed for cleaner code.

🔒🔄🔧


Powered by Code Review GPT

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.

super clean PR @mamadoudicko ! GReat work

@mamadoudicko mamadoudicko merged commit 99a3fa9 into main Aug 3, 2023
7 checks passed
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
* feat(sdk): add prompt apis to sdk

* feat: implement prompt creation-n

* feat: add brain custom prompt fields

* fix: change tables creation order
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.

Please change the order of the table creation because of the table prompts (reference error)
2 participants