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

fix(Analytics): no tags tracking for upload & crawl #1024

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Conversation

gozineb
Copy link
Contributor

@gozineb gozineb commented Aug 22, 2023

Description

Fix tracking for file_uploaded,url_crawled and invalid_url
Add api for backend requests for upload and crawl
Fix some linter issues

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
Copy link

vercel bot commented Aug 22, 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 23, 2023 8:02am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2023 8:02am

@@ -64,7 +64,7 @@ export const useLogin = () => {
redirect(previousPage);
}
}
}, [session?.user]);
}, [session?.user, track]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ? Since track is a static function


useEffect(() => {
void fetchAllBrains();
void fetchAndSetActiveBrain();
}, [session?.user]);
}, [fetchAllBrains, fetchAndSetActiveBrain]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? The fetch seems to be required primarily only when the auth status changes, plus these functions are static.

@@ -32,7 +32,7 @@ services:
context: backend
dockerfile: Dockerfile
container_name: backend-core
command: uvicorn main:app --host 0.0.0.0 --port 5050
command: uvicorn main:app --reload --host 0.0.0.0 --port 5050
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks :D

@@ -17,7 +17,7 @@ export const usePublicPrompts = ({ onSelect }: UsePublicPromptsProps) => {
setPublicPrompts(await getPublicPrompts());
};
void fetchPublicPrompts();
}, []);
}, [getPublicPrompts]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Static function too

@@ -1,41 +1,41 @@
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

});
} finally {
setCrawling(false);
}
},
[session.access_token, publish]
[publish, crawlWebsiteUrl, t, track]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

function Crawler() {
return (
return (
<Suspense fallback={"Loading..."}>
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not lo longer using server component so we can remove these

@github-actions
Copy link
Contributor

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/upload/components/Crawler/hooks/useCrawler.ts

The useCrawler hook is using the session.access_token as a dependency for the useCallback hook. This might cause unnecessary re-renders if the access_token changes frequently. Consider removing it from the dependency array if it's not necessary for the function to re-run when it changes.

const crawlWebsite = useCallback(
  async (brainId: UUID | undefined) => {
    // function body
  },
  [] // remove session.access_token from dependencies
);

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/upload/components/FileUploader/hooks/useFileUploader.ts

The upload function is using the session.access_token as a dependency for the useCallback hook. This might cause unnecessary re-renders if the access_token changes frequently. Consider removing it from the dependency array if it's not necessary for the function to re-run when it changes.

const upload = useCallback(
  async (file: File, brainId: UUID) => {
    // function body
  },
  [] // remove session.access_token from dependencies
);

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/app/upload/components/Crawler/index.tsx

The component has been stripped of its contents and only empty divs are left. This will result in an empty component being rendered. Ensure that the necessary JSX elements are included.

return (
  <div className=\"w-full\">
    <div className=\"flex justify-center gap-5 px-6\">
      <div className=\"max-w-xl w-full\">
        <div className=\"flex-col justify-center gap-5\">
          // Add necessary JSX elements here
        </div>
      </div>
    </div>
  </div>
);

🔄🔒🚫


Powered by Code Review GPT

@gozineb gozineb merged commit 2b74ebc into main Aug 23, 2023
7 checks passed
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
* 🚚 create useCrawlApi to use in useCrawler hook

* 🚑 fix tracking in Crawl

* 🧑‍💻 add hot reloading within docker containers

* 🚑 fix tracking for upload

* 🚚 create useUploadApi for fileUpload request

* 📈 add june tag for Language change

* 🩹 revert dependencies
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

2 participants