feat: knowledge tab list #1222

gozineb · 2023-09-20T10:01:48Z

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Checklist before requesting a review

Please delete options that are not relevant.

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented hard-to-understand areas
I have ideally added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged

Screenshots (if appropriate):

vercel · 2023-09-20T10:01:54Z

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 22, 2023 8:03am
quivr-strapi	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 22, 2023 8:03am
quivrapp	❌ Failed (Inspect)			Sep 22, 2023 8:03am

github-actions · 2023-09-20T12:18:14Z

Risk Level 2 - /home/runner/work/quivr/quivr/backend/routes/knowledge_routes.py

The get_all_knowledge function is used without any exception handling. If it fails due to a database error or other unexpected issue, it could cause the entire endpoint to fail. Consider wrapping it in a try/except block to handle potential errors gracefully.
The delete_endpoint function deletes a knowledge item and then attempts to delete its associated file. If the file deletion fails, the knowledge item will already have been deleted. Consider deleting the file first, and only deleting the knowledge item if the file deletion is successful.
The delete_endpoint function assumes that every knowledge item has either a file_name or a url. If a knowledge item has neither, this could cause an error. Consider adding a check to ensure that at least one of these properties is present.

Example:

if not (knowledge.file_name or knowledge.url):
    raise Exception(\"Knowledge item has no associated file or URL\")

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/brains-management/[brainId]/components/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/useKnowledgeItem.ts

The onDeleteKnowledge function sets isDeleting to true at the start, but it only sets it back to false at the end of the function. If an error occurs during the deletion process and an exception is thrown, isDeleting will remain true indefinitely. Consider using a finally block to ensure that isDeleting is always set back to false, even if an error occurs.
The onDeleteKnowledge function does not handle the case where currentBrain?.id is undefined. If this occurs, it will throw an error which is not caught by any catch block. Consider adding error handling for this case.

Example:

if (currentBrain?.id === undefined) {
    console.error(\"Current brain ID is undefined\");
    setIsDeleting(false);
    return;
}

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/lib/api/knowledge/knowledge.ts

The code is generally well-written and follows good practices. However, there is a potential issue with error handling. When the knowledge.file_name and knowledge.url are both null, an error is thrown. It would be better to handle this error more gracefully, perhaps by returning a default value or logging the error for further investigation.

📚💣🔧

Powered by Code Review GPT

...omponents/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/CrawledKnowledgeItem.tsx

backend/models/databases/supabase/knowledge.py

mamadoudicko · 2023-09-22T08:27:22Z

...Id]/components/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/DeleteKnowledge.tsx

+
+  const canDeleteFile = currentBrain?.role === "Owner";
+
+  console.log("isDeleting", isDeleting);


mamadoudicko · 2023-09-22T08:28:31Z

...ents/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/DownloadUploadedKnowledge.tsx

+  return (
+    <a
+      onClick={() => void downloadFile()}
+      style={{ display: "flex", flexDirection: "column", alignItems: "center" }}


mamadoudicko · 2023-09-22T08:32:27Z

...mponents/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/UploadedKnowledgeItem.tsx

+
+export const UploadedKnowledgeItem = ({
+  knowledge,
+}: {


Seems to have many things in common with CrawledKnowledgeItem...

Maybe managing isUploadedKnowledge in another way ?

mamadoudicko · 2023-09-22T08:32:44Z

...ent/[brainId]/components/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/index.tsx

+};
+
+KnowledgeItem.displayName = "KnowledgeItem";
+export default KnowledgeItem;


mamadoudicko · 2023-09-22T08:34:10Z

...Id]/components/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/useKnowledgeItem.ts

+      });
+      console.error(`Error deleting ${knowledge_name ?? ""}`, error);
+    }
+    setIsDeleting(false);


Seems ok but inside finally should be better

mamadoudicko · 2023-09-22T08:35:56Z

...nt/[brainId]/components/BrainManagementTabs/components/KnowledgeTab/hooks/useKnowledgeTab.ts

+
+  const { getAllKnowledge } = useKnowledgeApi();
+  const { data: brainKnowledges, isLoading: isPending } = useQuery({
+    queryKey: [getKnowledgeDataKey(brainId)],


mamadoudicko · 2023-09-22T08:37:48Z

frontend/app/brains-management/layout.tsx

@@ -19,7 +20,7 @@ const Layout = ({ children }: LayoutProps): JSX.Element => {
  return (
    <div className="relative h-full w-full flex justify-stretch items-stretch">
      <BrainsList />
-      {children}
+      <KnowledgeProvider>{children}</KnowledgeProvider>


KnowledgeProvider seems to have be added 2 times

KnowledgeTab/index.tsx too

mamadoudicko · 2023-09-22T08:38:52Z

frontend/lib/api/knowledge/config.ts

@@ -0,0 +1,6 @@
+import { UUID } from "crypto";
+
+const brainDataKey = "quivr-knowledge";


mamadoudicko · 2023-09-22T08:46:32Z

frontend/lib/context/KnowledgeProvider/knowledge-provider.tsx

+}: {
+  children: React.ReactNode;
+}): JSX.Element => {
+  const [allKnowledge, setAllKnowledge] = useState<Knowledge[]>([]);


It appears that allKnowledge is not being used. A new state has been introduced in useknowledgeTab, which is conflicting with this

Right ?

gozineb · 2023-09-22T09:04:18Z

...nt/[brainId]/components/BrainManagementTabs/components/KnowledgeTab/hooks/useKnowledgeTab.ts

+};
+// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
+export const useKnowledgeTab = ({ brainId }: useKnowledgeTabProps) => {
+  const [allKnowledge, setAllKnowledge] = useState<Knowledge[]>([]);


replace get and set from the context

vercel bot deployed to Preview – docs September 20, 2023 10:01 View deployment

vercel bot deployed to Preview – quivrapp September 20, 2023 10:03 View deployment

gozineb force-pushed the feat/knowledge-tab-list branch from 379275e to 4d12e2e Compare September 20, 2023 12:15

gozineb temporarily deployed to preview September 20, 2023 12:15 — with GitHub Actions Inactive

gozineb had a problem deploying to preview September 20, 2023 12:15 — with GitHub Actions Failure

vercel bot deployed to Preview – docs September 20, 2023 12:16 View deployment

vercel bot deployed to Preview – quivrapp September 20, 2023 12:18 View deployment

gozineb force-pushed the feat/knowledge-tab-list branch from 4d12e2e to 3aeef9c Compare September 21, 2023 07:54

gozineb had a problem deploying to preview September 21, 2023 07:54 — with GitHub Actions Failure

gozineb temporarily deployed to preview September 21, 2023 07:54 — with GitHub Actions Inactive

vercel bot had a problem deploying to Preview – quivrapp September 21, 2023 07:57 Failure

vercel bot deployed to Preview – docs September 21, 2023 07:57 View deployment

vercel bot deployed to Preview – quivr-strapi September 21, 2023 07:58 View deployment

gozineb force-pushed the feat/knowledge-tab-list branch from 3aeef9c to 8778b58 Compare September 21, 2023 16:08

gozineb had a problem deploying to preview September 21, 2023 16:08 — with GitHub Actions Failure

gozineb temporarily deployed to preview September 21, 2023 16:08 — with GitHub Actions Inactive

gozineb force-pushed the feat/knowledge-tab-list branch from 8778b58 to 3b1528d Compare September 21, 2023 16:09

gozineb temporarily deployed to preview September 21, 2023 16:09 — with GitHub Actions Inactive

gozineb had a problem deploying to preview September 21, 2023 16:09 — with GitHub Actions Failure

github-advanced-security bot found potential problems Sep 21, 2023

View reviewed changes

...omponents/BrainManagementTabs/components/KnowledgeTab/KnowledgeItem/CrawledKnowledgeItem.tsx Fixed Show fixed Hide fixed

vercel bot had a problem deploying to Preview – quivrapp September 21, 2023 16:13 Failure

mamadoudicko reviewed Sep 21, 2023

View reviewed changes

backend/models/databases/supabase/knowledge.py Outdated Show resolved Hide resolved

mamadoudicko reviewed Sep 21, 2023

View reviewed changes

backend/models/databases/supabase/knowledge.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview – quivr-strapi September 21, 2023 16:19 View deployment

vercel bot deployed to Preview – docs September 21, 2023 16:19 View deployment

gozineb had a problem deploying to preview September 22, 2023 07:54 — with GitHub Actions Failure

vercel bot deployed to Preview – docs September 22, 2023 07:54 View deployment

gozineb added 11 commits September 22, 2023 09:54

✨ add KnowledgeTable component in knowledge tab

edbf997

🔥 remove DocumentData replaced by KnowledgeItem

be11591

🐛 fix weird characters instead of '/'

80140e6

💄 truncate knowledge name

4b00d1a

✨ add DownloadUploadedKnowledge component

e2c7609

⚰️ unused code

f3f9bc4

🏷️ introduce UploadedKnowledge and CrawledKnowledge types

9db26a5

💄 remove thread

b48c61c

💄 bin for delete knowledge

5c13f50

🌐 update wording for knowledge tab

13b230e

🔇 remove logs and comments

bbcf002

gozineb force-pushed the feat/knowledge-tab-list branch from b6c8c59 to bbcf002 Compare September 22, 2023 07:54

gozineb had a problem deploying to preview September 22, 2023 07:54 — with GitHub Actions Failure

gozineb temporarily deployed to preview September 22, 2023 07:54 — with GitHub Actions Inactive

vercel bot had a problem deploying to Preview – quivrapp September 22, 2023 08:00 Failure

vercel bot deployed to Preview – quivr-strapi September 22, 2023 08:03 View deployment

vercel bot deployed to Preview – docs September 22, 2023 08:03 View deployment

mamadoudicko reviewed Sep 22, 2023

View reviewed changes

mamadoudicko approved these changes Sep 22, 2023

View reviewed changes

gozineb commented Sep 22, 2023

View reviewed changes

gozineb merged commit d2b4ef4 into main Sep 22, 2023
9 of 12 checks passed

StanGirard mentioned this pull request Sep 22, 2023

chore(main): release 0.0.81 #1242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: knowledge tab list #1222

feat: knowledge tab list #1222

gozineb commented Sep 20, 2023

vercel bot commented Sep 20, 2023 •

edited

github-actions bot commented Sep 20, 2023 •

edited

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023 •

edited

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

mamadoudicko Sep 22, 2023

gozineb Sep 22, 2023


		const canDeleteFile = currentBrain?.role === "Owner";

		console.log("isDeleting", isDeleting);

		@@ -0,0 +1,6 @@
		import { UUID } from "crypto";

		const brainDataKey = "quivr-knowledge";

feat: knowledge tab list #1222

feat: knowledge tab list #1222

Conversation

gozineb commented Sep 20, 2023

Description

Checklist before requesting a review

Screenshots (if appropriate):

vercel bot commented Sep 20, 2023 • edited

github-actions bot commented Sep 20, 2023 • edited

Powered by Code Review GPT

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamadoudicko Sep 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Sep 20, 2023 •

edited

github-actions bot commented Sep 20, 2023 •

edited

mamadoudicko Sep 22, 2023 •

edited