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 multiple upload and crawl in parallel #1118

Merged
merged 9 commits into from
Sep 7, 2023
Merged

feat: add multiple upload and crawl in parallel #1118

merged 9 commits into from
Sep 7, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Sep 6, 2023

Screen.Recording.2023-09-06.at.14.13.13.mov

@vercel
Copy link

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ActionsBar/components/KnowledgeToFeed/KnowledgeToFeed.tsx

The code is well written and follows the SOLID principles. However, there is a potential risk of the contents state not being properly managed. If there are multiple instances of KnowledgeToFeed component, they will all share the same state which might lead to unexpected behavior. Consider encapsulating the state inside the component or use a more sophisticated state management solution if necessary.


Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ActionsBar/components/KnowledgeToFeed/components/FileUploader/hooks/useFileUploader.ts

The code seems to be well written and follows good practices. However, there is a potential issue with the useFileUploader function. It seems to be using a redirectToLogin function when the session is null. This could potentially lead to unexpected behavior if the session becomes null during the execution of the function. It would be better to handle this case more gracefully, perhaps by returning an error or throwing an exception.

if (session === null) {
  redirectToLogin();
}

Suggestion:

if (session === null) {
  throw new Error('Session is null');
}

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/components/ActionsBar/hooks/useKnowledgeUploader.ts

The useKnowledgeUploader function seems to be handling errors correctly, but it could be improved by providing more specific error messages. Currently, it is using JSON.stringify(error) to display the error message, which might not be very user-friendly. It would be better to provide a more specific error message based on the type of error.

publish({
  variant: \"danger\",
  text: t(\"crawlFailed\", {
    message: JSON.stringify(error),
    ns: \"upload\",
  }),
});

Suggestion:

publish({
  variant: \"danger\",
  text: t(\"crawlFailed\", {
    message: error instanceof Error ? error.message : 'Unknown error',
    ns: \"upload\",
  }),
});

🔄🔒🚨


Powered by Code Review GPT

@StanGirard
Copy link
Collaborator

Love it 🥰

const response = await uploadFile({ brainId, formData });
const onDrop = (acceptedFiles: File[], fileRejections: FileRejection[]) => {
if (fileRejections.length > 0) {
const firstRejection = fileRejections[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

why first rejection ?

@gozineb gozineb merged commit 711eff0 into main Sep 7, 2023
10 checks passed
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
* feat: explicit accepted files

* feat: un-synchronize upload and chat FileUploader

* feat: add uploading file new ui

* feat: rename +UrlDisplayer to FeedTitleDisplayer

* feat: add icon per file type

* feat: remove file extension on display

* feat: send feed items to backend

* feat: track file upload

* chore: improve dx
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

3 participants