-
Notifications
You must be signed in to change notification settings - Fork 4
Update logout redirect #1638
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
Update logout redirect #1638
Conversation
WalkthroughThe PR adds Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
571-573: Mobile logout uses helper; consider whetherdomainshould be requiredUsing
buildLogoutUrl(domain)for mobile logout gives you the samenextbehavior and also safely falls back to/logoutifdomainis undefined, which is an improvement over raw string interpolation. Ifdomainis in practice always provided for this component, consider makingdomaina requiredstringin the props to make that contract explicit; otherwise the current optional type plus helper fallback is fine.apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
18-23:buildLogoutUrlcorrectly centralizes logout +next; consider empty-return behavior and testsThe new
buildLogoutUrlcorrectly pullsVITE_AUTH_RETURN_URL, builds the domain‑aware/logout/URL, and always appends a URL‑encodednextparameter, which matches the PR’s goal. The fallback to/logoutwhendomainis omitted is also a nice safety net for callers. The only open question is what you want to happen whenVITE_AUTH_RETURN_URLis missing—right now you’ll get?next=; if that’s not desirable, you could either omit thenextparam in that case or default to a sensible origin. It might also be worth adding a small unit test for this helper (with and withoutdomain, env set vs unset) to guard future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getPublicEnvVariables(19-48)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
buildLogoutUrl(18-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (3)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (2)
38-41: Auth helpers import keeps URL construction DRYImporting both
buildAuthLoginUrlandbuildLogoutUrlhere is a good move; it centralizes auth URL construction and avoids hard‑coded paths scattered through navigation.
408-412: Desktop logout now correctly uses centralized helper withnextSwitching the desktop logout link to
href={buildLogoutUrl(domain)}aligns with the new helper and ensures the Django logout view consistently receives thenextparameter while preserving the existing domain‑based path.apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
3-16: Login helper type change verified—no legacy references foundThe new
LoginPropsinterface is properly scoped to ThunderstoreAuth.tsx with no olderPropstype exported. All six call sites in Navigation.tsx and one in Connections.tsx continue to work with the updated signature. The type refactor is safe and complete.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1638 +/- ##
=========================================
- Coverage 9.89% 9.88% -0.01%
=========================================
Files 310 310
Lines 22462 22468 +6
Branches 406 406
=========================================
- Hits 2223 2222 -1
- Misses 20239 20246 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Implement helper function to build the logout URL. Add a ?next parameter to the logout URL which returns the user to the Nimbus index page. This should override the Django logout function to redirect to Nimbus instead of legacy page index page upon logout. Refs. TS-2779
Replace the static URL with buildLogoutUrl in the navigation links. TS-2779
d04fdb3 to
86a84a9
Compare
Pass a next variable to the django backend's logout view.
Implement helper function to create the logout URL with only domain as parameter.
Refs. TS-2779