-
Notifications
You must be signed in to change notification settings - Fork 8
[LC-1467] - fix: Logout Behavior #880
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
🦋 Changeset detectedLatest commit: 9bdaa1b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for staging-learncardapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for learncarddocs canceled.
|
|
This PR is missing a Jira ticket reference in the title or description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR successfully addresses the main issue by closing modals on logout and avoiding hard refreshes. However, there's a breaking change in the logout function signature that could affect other consumers.
1 issues detected:
🐞 Bug - Removing default parameter value creates a breaking change that could leave users without expected redirect behavior. 🛠️
Details: The logout function signature was changed from having a default redirectUrl parameter to making it optional. This removes the automatic redirect behavior that other parts of the codebase might rely on.
File:packages/learn-card-base/src/hooks/useWeb3AuthSFA.ts (215-215)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
This PR is missing a Jira ticket reference in the title or description. |
|
🥷 Code experts: TaylorBeeston TaylorBeeston has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
LGTM
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
goblincore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerardopar LGTM!
Overview
🎟 Relevant Jira Issues
📚 What is the context and goal of this PR?
logout()useWeb3AuthSFA()->window.location.href = redirectUrl;history.push(redirectUrl)🥴 TL; RL:
💡 Feature Breakdown (screenshots & videos encouraged!)
https://www.loom.com/share/f996736fdbdc49148267785415d0329b
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
Confirm logout works as expected
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📜 Gitbook
📊 Storybook
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Fix logout behavior to prevent redirect issues and ensure proper modal closure by handling navigation within the application instead of delegating to Web3Auth.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how