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: validate api key before saving #1057

Merged
merged 1 commit into from
Aug 30, 2023
Merged

feat: validate api key before saving #1057

merged 1 commit into from
Aug 30, 2023

Conversation

mamadoudicko
Copy link
Contributor

  • Validate api key before saving
Screen.Recording.2023-08-29.at.17.42.40.mov

@mamadoudicko mamadoudicko temporarily deployed to preview August 29, 2023 15:43 — 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:13am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2023 7:13am

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

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

The code seems to be well written and follows good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. Avoid magic numbers: The setTimeout function has a delay of 50 milliseconds. It would be better to define this value as a constant at the top of the file, giving it a descriptive name. This makes the code easier to understand and modify.
const MODEL_SET_DELAY = 50;
// ...
setTimeout(() => {
  if (brain.model !== undefined) {
    setValue(\"model\", brain.model);
  }
}, MODEL_SET_DELAY);
  1. Error handling: There are several places where async functions are called without try-catch blocks. This could lead to unhandled promise rejections if these functions throw errors. It's recommended to always handle errors from async functions.
try {
  const brain = await getBrain(brainId);
  // ...
} catch (error) {
  // Handle error
}
  1. Use optional chaining: There are several places where properties are accessed on objects that could potentially be undefined. This could lead to runtime errors. It's recommended to use optional chaining (?.) to safely access these properties.
const isDefaultBrain = defaultBrainId === brainId;
// becomes
const isDefaultBrain = defaultBrainId?. === brainId;

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/user/components/ApiKeyConfig/hooks/useApiKeyConfig.ts

The code is generally well written, but there are a few areas that could be improved:

  1. Avoid console logging: There are several instances where errors are logged to the console. This is not a good practice for production code as it can expose sensitive information. It's recommended to use a logging service that can capture and manage logs securely.
try {
  const createdApiKey = await createApiKey();
  setApiKey(createdApiKey);
} catch (error) {
  // Replace console.error with a secure logging service
}
  1. Error handling: Similar to the previous file, there are several places where async functions are called without try-catch blocks. This could lead to unhandled promise rejections if these functions throw errors. It's recommended to always handle errors from async functions.
try {
  await updateUserIdentity({
    openai_api_key: null,
  });
  // ...
} catch (error) {
  // Handle error
}

🔢🔍🔐


Powered by Code Review GPT

@gozineb gozineb merged commit 04e9f57 into main Aug 30, 2023
7 checks passed
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
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