-
Notifications
You must be signed in to change notification settings - Fork 19
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
Citizen logout redirect #1627
base: develop
Are you sure you want to change the base?
Citizen logout redirect #1627
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (1)
Pattern
**/*.js
: check
🔇 Additional comments (2)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (2)
Line range hint
108-110
: LGTM! Clean implementation of loading state.The conditional rendering of the Loader component is implemented correctly using the early return pattern.
99-99
: Verify no hardcoded employee login redirects exist.Let's ensure there are no other components that might override this redirect behavior.
✅ Verification successful
Let me try a different search strategy to verify this concern.
This should help us find:
- All login-related redirects
- Logout handling logic
- Specific citizen login/logout flows
Based on the search results, I can now provide a definitive response:
No hardcoded employee login redirects found in citizen logout flows
The codebase search reveals that:
- All citizen-related logout redirects consistently point to
/citizen/login
- The only employee login redirect exists in a generic utility (
Request.js
) that properly handles both citizen and employee cases using a conditional check- The logout implementation in
StaticCitizenSideBar.js
follows the correct pattern for citizen redirectsThe changes in the PR are consistent with the existing logout behavior across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential hardcoded employee login redirects in citizen flows rg -l "employee/login" | xargs rg "citizen.*logout" -C 3 # Search for other logout handlers that might need similar updates ast-grep --pattern 'UserService.logout()'Length of output: 100
Script:
#!/bin/bash # Search for login/logout related redirects in the codebase rg -g '*.js' -g '*.jsx' "window\.location\.href.*login" -C 2 # Search for logout handlers rg -g '*.js' -g '*.jsx' "logout" -C 2 # Search specifically for citizen related paths rg -g '*.js' -g '*.jsx' "citizen.*login|login.*citizen" -C 2Length of output: 24686
@@ -96,7 +96,7 @@ const StaticCitizenSideBar = ({ linkData, islinkDataLoading }) => { | |||
const handleOnSubmit = () => { | |||
Digit.UserService.logout(); | |||
setShowDialog(false); | |||
}; | |||
window.location.href = `/${window?.contextPath}/citizen/login`; }; |
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.
🛠️ Refactor suggestion
Add error handling for logout operation.
While the redirect logic is correct, consider handling potential logout failures before redirecting.
Consider this implementation:
- window.location.href = `/${window?.contextPath}/citizen/login`;
+ try {
+ await Digit.UserService.logout();
+ setShowDialog(false);
+ window.location.href = `/${window?.contextPath}/citizen/login`;
+ } catch (error) {
+ console.error('Logout failed:', error);
+ // Show error notification to user
+ Digit.Hooks.useToast.error(t('LOGOUT_ERROR'));
+ }
Committable suggestion was skipped due to low confidence.
due to usertype employee citizen logout was redirecting to employee login page.
Summary by CodeRabbit
New Features
Bug Fixes