-
Notifications
You must be signed in to change notification settings - Fork 45.8k
fix(frontend): Fix login/logout actions; update credentials cache on state change #10017
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
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
✅ Deploy Preview for auto-gpt-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Here's the code health analysis summary for commits Analysis Summary
|
The PR template is not properly filled out - missing the checklist items and clear description of changes. However, the code changes themselves are minor, focused on frontend auth handling, and the title follows the conventional commit format with proper scope. The changes appear to be strictly related to credential handling during logout which keeps it within scope. |
While the PR shows clear code changes that make sense, the PR template is not properly filled out. The checklist sections are completely missing (not even marked as N/A) and the test plan is very minimal, only showing failed lint checks. Additionally, while the PR title follows the conventional commit format with scope, the description could be more detailed about the need for these changes. However, since you mentioned to not be overly pedantic and we're working on getting the template used at all, these issues aren't severe enough to warrant a complete rejection. |
The PR has issues with incomplete checklist. While the PR title follows conventional commit format and the description clearly explains the intent of the changes (fixing credential caching on login state changes), the author has not properly completed the checklist section. The test plan shows that the linting and type checking tests failed, but there's no indication that the actual functionality was tested. According to the rules, the checklist must be filled out completely, and partially checked off checklists are not accepted. |
The PR mostly follows the required format and rules. The title correctly uses 'fix(frontend)' format. The description references issue #10008 and clearly explains the changes. The checklist is incomplete as the author hasn't confirmed testing according to their test plan - they've created a plan but haven't marked it as completed. This is a requirement according to the rules where checklists must be filled out completely. No backend/data/*.py files are changed and no new frontend routes are added, so those specific rules don't apply. |
Thanks for your PR to fix the credentials cache on login state change! Before we can merge this, please complete the checklist by confirming that you've tested your changes according to the test plan you outlined. The test plan looks comprehensive, but we need confirmation that you've actually run through the steps and verified the expected behavior (credentials for account B showing, credentials for account A not showing). |
Thank you for your PR addressing the credentials cache update on login state change! The changes look good technically - you're updating the useSupabase hook to respond to auth state changes and refreshing credentials when the logged-in user changes. A couple of things to address before this is ready to merge:
The code changes themselves look good - subscribing to auth state changes and properly cleaning up the subscription is the right approach here. The dependency array for the useEffect in credentials-provider.tsx has been appropriately updated to include the user as well. |
ae23514
to
1762f5c
Compare
still flaky |:( |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
Changes 🏗️
useSupabase
hook to propagate auth state changesCredentialsProvider
whenever the user login state changeslogOut
callback touseSupabase
hook that handles (client-side) logoutlogout
action: the Supabase reference implementation does it client-side, and doing both causes a race conditionRefactorings to aid implementation of the above:
@/hooks/useSupabase
->@/lib/supabase/useSupabase
Other improvements:
login
server action based on reference implementationBackendAPI.isAuthenticated()
more efficient and fasterProfileDropdown
componenttests/pages/login.page.ts
Checklist 📋
For code changes:
I have clearly listed my changes in the PR description
I have made a test plan
I have tested my changes according to the test plan:
Note: do not reload the page while going through these steps