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(notificatins): higher refresh rate #1184

Merged
merged 3 commits into from
Sep 16, 2023
Merged

feat(notificatins): higher refresh rate #1184

merged 3 commits into from
Sep 16, 2023

Conversation

StanGirard
Copy link
Collaborator

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

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

@StanGirard StanGirard temporarily deployed to preview September 16, 2023 11:50 — with GitHub Actions Inactive
@StanGirard StanGirard merged commit f362269 into main Sep 16, 2023
6 of 7 checks passed
@github-actions
Copy link
Contributor

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/[chatId]/hooks/useSelectedChatPage.ts

  1. The useSelectedChatPage function is using the useParams hook to get the chatId but it does not handle the case where chatId is undefined before calling getChatNotificationsQueryKey. This could potentially lead to bugs. Consider adding a check for chatId before calling this function.
const chatId = params?.chatId as string | undefined;
if (!chatId) {
  // handle error
  return;
}
const chatNotificationsQueryKey = getChatNotificationsQueryKey(chatId);
  1. The refetchInterval function in the useQuery hook has been changed to return 2000 (2 seconds) instead of 30000 (30 seconds). This could potentially lead to performance issues due to the increased frequency of network requests. Consider reverting this change or ensuring that the backend can handle this increased load.

Risk Level 3 - /home/runner/work/quivr/quivr/backend/routes/chat_routes.py

  1. The delete_chat_from_db function is catching all exceptions and just printing them. This is not a good practice as it can make debugging difficult. It would be better to log the exceptions with more context or re-raise them after logging.
try:
    supabase_db.delete_chat_history(chat_id)
except Exception as e:
    logger.error(f'Error deleting chat history for chat_id {chat_id}: {e}')
    raise
  1. The check_user_requests_limit function is using a pass statement in the else clause. This is unnecessary and can be removed.

  2. The create_question_handler and create_stream_question_handler functions have a lot of duplicated code. Consider refactoring this into a separate function to adhere to the DRY (Don't Repeat Yourself) principle.


🐛💻🚀


Powered by Code Review GPT

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.

1 participant