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.

Sign up for GitHub

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

Jump to bottom

fix(analytics): june debug for real #1033

Merged
merged 6 commits into from
Aug 25, 2023
Merged

fix(analytics): june debug for real #1033

merged 6 commits into from
Aug 25, 2023

Conversation

gozineb
Copy link
Contributor

@gozineb gozineb commented Aug 24, 2023

Description

Bug origin was missing "track" of june hook from dependencies

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):

@gozineb gozineb temporarily deployed to preview August 24, 2023 17:38 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Aug 24, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Aug 24, 2023 5:39pm
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 24, 2023 5:39pm

@github-actions
Copy link
Contributor

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

The code is generally well written and follows good practices. However, there are a few areas that could be improved for better error handling and readability.

  1. The upload function is logging to the console. This is fine for development, but these logs should be removed or replaced with a proper logging solution for production.

  2. The upload function is throwing an error if the upload fails. It might be better to display an error message to the user and allow them to retry the upload.

  3. The uploadAllFiles function is optimistically updating the UI before the upload operation has been confirmed to be successful. This could lead to a confusing user experience if the upload operation fails. Consider moving the UI update to after the successful response from the upload operation.

Risk Level 3 - /home/runner/work/quivr/quivr/frontend/services/analytics/useEventTracking.ts

The changes in this file include the addition of console logs and the use of a new analytics service. While console logs can be useful for debugging, they should be removed in production code to avoid exposing potentially sensitive information. The use of a new analytics service could potentially introduce new bugs, so thorough testing is recommended. Also, the user's email is being sent as a property to the identify method. Make sure this is in compliance with privacy regulations.

await analytics.identify(session?.user.id, { email: session?.user.email });

Risk Level 4 - /home/runner/work/quivr/quivr/frontend/services/analytics/useJune.ts

The changes in this file include the addition of console logs and the use of an API key from the environment variables. As mentioned before, console logs should be removed in production code. The use of the API key from the environment variables is a good practice as it keeps sensitive information out of the code. However, it's important to ensure that the environment variable is properly set in all environments where the code will run. If the API key is not set, the analytics service will not work.

const juneApiKey = process.env.NEXT_PUBLIC_JUNE_API_KEY;

📝🔍🔐

Powered by Code Review GPT

mamadoudicko
mamadoudicko reviewed Aug 25, 2023
View reviewed changes
frontend/app/upload/components/Crawler/hooks/useCrawler.ts
@@ -31,6 +32,7 @@ export const useCrawler = () => {
const url = urlInputRef.current ? urlInputRef.current.value : null;

if (url === null || !isValidUrl(url)) {
console.log("Invalid URL");
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

mamadoudicko
mamadoudicko reviewed Aug 25, 2023
View reviewed changes
frontend/app/upload/components/Crawler/hooks/useCrawler.ts
@@ -52,6 +54,7 @@ export const useCrawler = () => {

setCrawling(true);

console.log("Tracking URL_CRAWLED");
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@gozineb gozineb merged commit a9411c9 into main Aug 25, 2023
@StanGirard StanGirard mentioned this pull request Aug 24, 2023
Merged
StanGirard pushed a commit that referenced this pull request Sep 12, 2023
@gozineb
fix(analytics): june debug for real (#1033)
8251810 
* 🔥 remove Suspense because no ssr in front anymore

* ✨ add debugging

* 🔥 remove debugger and better logs

* 🐛 fixed tracker by adding it to dependencies

* 🚨 add dependencies to hooks

* 🚨 remove linter warning from unawaited promise
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mamadoudicko mamadoudicko mamadoudicko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gozineb @mamadoudicko