-
Notifications
You must be signed in to change notification settings - Fork 33
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
[platform] : (Fix) Login Issue for most Users #2551
base: staging
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant UL as UserLogin Component
participant PS as Preferences Service
participant LS as LocalStorage
UL->>PS: Fetch user preferences
PS-->>UL: Return preferences
alt Valid preference with group_id exists
UL->>UL: Assign activeGroup from preference
else
UL->>UL: Search for fallback group "airqo"
end
alt activeGroup found
UL->>LS: Save activeGroup to local storage
else
UL->>UL: Throw error (contact support)
end
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/platform/src/pages/account/login/index.jsx (2)
76-86
: Improved preference validation logicThe change correctly adds validation to check if preferences exist and have a valid group_id before trying to access them. This is a robust approach that prevents potential runtime errors.
However, there's a minor issue: the code doesn't handle the case where
user.groups
might be undefined, which could happen in edge cases.Consider adding an additional check to ensure
user.groups
is defined before callingfind
:// Try to get the group from the first preference if exists and valid if (preferences.length > 0 && preferences[0].group_id) { - activeGroup = user.groups.find( + activeGroup = user.groups?.find( (group) => group._id === preferences[0].group_id, ); }
87-92
: Good fallback mechanism implementationThe fallback to find a group with the title 'airqo' is sensible and ensures users can still log in even if their preferences don't contain a valid group.
Consider making the group title comparison case-insensitive to make it more robust:
if (!activeGroup) { activeGroup = user.groups.find( - (group) => group.grp_title.toLowerCase() === 'airqo', + (group) => group.grp_title?.toLowerCase() === 'airqo', ); }This includes a null check on
grp_title
and ensures the comparison works regardless of the case stored in the database.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/platform/src/pages/account/login/index.jsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (2)
src/platform/src/pages/account/login/index.jsx (2)
93-98
: Appropriate error handling addedGood addition of explicit error handling when no valid group is found. This provides a clear message for users and guides them to contact support.
100-100
: Correctly relocated localStorage assignmentMoving the localStorage assignment for
activeGroup
to occur after all validations is correct. This ensures we only store a valid group in localStorage.
New next-platform changes available for preview here |
1 similar comment
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
Refactor
Bug Fixes