Fix failing redirect to create new organization page on no organizations #3125
Conversation
WalkthroughThe changes update both the frontend and backend logic to refine authentication, routing, and error handling workflows. On the frontend, adjustments include a new condition in the authentication middleware that allows access to the page when the pathname is 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/routeTree.gen.ts (1)
1-9: Caution: This is an auto-generated file.The file header explicitly states that this file should not be manually modified as it is automatically generated by TanStack Router. Any changes should be made through the router configuration instead of direct edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
frontend/src/pages/middlewares/authenticate.tsx(1 hunks)frontend/src/pages/organization/NoOrgPage/NoOrgPage.tsx(1 hunks)frontend/src/pages/organization/NoOrgPage/route.tsx(1 hunks)frontend/src/pages/organization/layout.tsx(1 hunks)frontend/src/routeTree.gen.ts(13 hunks)frontend/src/routes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/pages/organization/NoOrgPage/NoOrgPage.tsx
- frontend/src/pages/organization/NoOrgPage/route.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
- GitHub Check: Mintlify Deployment
🔇 Additional comments (3)
frontend/src/pages/organization/layout.tsx (1)
1-9: LGTM! Style improvements look good.The changes are purely stylistic, maintaining consistent use of single quotes and improving code formatting.
frontend/src/routes.ts (1)
344-344: LGTM! Route placement is strategically correct.The new route for handling users with no organizations is correctly placed:
- Within the authenticated routes section (requires user authentication)
- Before the organization details injection (prevents redirect loops)
frontend/src/routeTree.gen.ts (1)
381-387: LGTM! Route integration looks correct.The new route
/organization/noneis properly integrated into the routing tree:
- Defined under the authentication middleware
- Correctly configured with path and parent route
- Properly typed in the interfaces
Also applies to: 469-469, 1693-1699
There was a problem hiding this comment.
Code looks good, was able to create a new org 👍 one potential improvement would be handling this situation:
Screen.Recording.2025-02-17.at.10.13.22.AM.mov
And minor nit but I think you might not have your editor configured to use or prettier rules as commits have single quotation and no semicolons
…ere prev logged into it
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/pages/auth/LoginPage/LoginPage.tsx (1)
22-35: 🛠️ Refactor suggestionEnhance error handling and add no-organization case.
The current implementation doesn't fully address the infinite loading issue when users have no organizations. Consider these improvements:
- Handle the no-organization case by redirecting to the create organization page.
- Improve error handling to be more specific and accurate.
Here's a suggested implementation:
const handleRedirects = async () => { try { + // Check if user has any organizations + const orgs = await getUserOrganizations(); + if (!orgs?.length) { + // Redirect to create organization page when user has no organizations + navigate({ to: '/organization/none' }); + return; + } + const callbackPort = queryParams?.get("callback_port"); // case: a callback port is set, meaning it's a cli login request: redirect to select org with callback port if (callbackPort) { navigateToSelectOrganization(callbackPort); } else { // case: no callback port, meaning it's a regular login request: redirect to select org navigateToSelectOrganization(); } - } catch { - console.log("Error - Not logged in yet"); + } catch (error) { + console.error('Failed to handle redirects:', error); + // Handle specific error cases if needed } };Don't forget to:
- Import the
getUserOrganizationsfunction- Import the
navigatefunction from your routing library
🧹 Nitpick comments (3)
frontend/src/pages/auth/SignUpInvitePage/SignUpInvitePage.tsx (1)
252-273: LGTM! Error handling improvement aligns with PR objective.The addition of try-catch block effectively resolves the infinite loading issue by redirecting users to the login page when organization selection fails.
Consider logging the error before redirecting to help with debugging:
} catch (err) { + console.error('Failed to select organization:', err); navigate({ to: "/login" }); }frontend/src/hooks/api/users/queries.tsx (1)
286-302: Consider consolidating session cleanup logic.The clearSession function duplicates storage cleanup logic that exists in the useDeleteMe hook. Consider extracting a common utility function to handle storage cleanup to maintain DRY principles.
-// Utility function to clear session storage and query cache -export const clearSession = (keepQueryClient?: boolean) => { +// Common utility function for storage cleanup +const clearStorageItems = () => { setAuthToken(""); // Clear authentication token localStorage.removeItem("protectedKey"); localStorage.removeItem("protectedKeyIV"); localStorage.removeItem("protectedKeyTag"); localStorage.removeItem("publicKey"); localStorage.removeItem("encryptedPrivateKey"); localStorage.removeItem("iv"); localStorage.removeItem("tag"); localStorage.removeItem("PRIVATE_KEY"); localStorage.removeItem("orgData.id"); sessionStorage.removeItem(SessionStorageKeys.CLI_TERMINAL_TOKEN); +}; + +// Utility function to clear session storage and query cache +export const clearSession = (keepQueryClient?: boolean) => { + clearStorageItems(); if (!keepQueryClient) { queryClient.clear(); // Clear React Query cache } };Then update useDeleteMe to use the common utility:
onSuccess: () => { - localStorage.removeItem("protectedKey"); - localStorage.removeItem("protectedKeyIV"); - localStorage.removeItem("protectedKeyTag"); - localStorage.removeItem("publicKey"); - localStorage.removeItem("encryptedPrivateKey"); - localStorage.removeItem("iv"); - localStorage.removeItem("tag"); - localStorage.removeItem("PRIVATE_KEY"); - localStorage.removeItem("orgData.id"); + clearStorageItems(); queryClient.clear(); }backend/src/services/auth-token/auth-token-service.ts (1)
104-136: Refactor for explicit error handling of expired or invalid tokens.While this method correctly throws an error for invalid tokens, consider adding a try-catch to differentiate between expired tokens (
TokenExpiredError) and other JWT errors. This will provide more precise feedback to clients, e.g.:- const decodedToken = jwt.verify(refreshToken, appCfg.AUTH_SECRET) as AuthModeRefreshJwtTokenPayload; + let decodedToken: AuthModeRefreshJwtTokenPayload; + try { + decodedToken = jwt.verify(refreshToken, appCfg.AUTH_SECRET) as AuthModeRefreshJwtTokenPayload; + } catch (err) { + if (err.name === "TokenExpiredError") { + throw new UnauthorizedError({ + message: "Refresh token has expired", + name: "TokenExpired" + }); + } + throw new UnauthorizedError({ + message: "Invalid or malformed token", + name: "InvalidToken" + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/server/routes/v1/auth-router.ts(3 hunks)backend/src/services/auth-token/auth-token-service.ts(4 hunks)backend/src/services/auth/auth-fns.ts(0 hunks)frontend/src/hooks/api/users/queries.tsx(2 hunks)frontend/src/pages/auth/LoginPage/LoginPage.tsx(1 hunks)frontend/src/pages/auth/SignUpInvitePage/SignUpInvitePage.tsx(1 hunks)frontend/src/pages/middlewares/authenticate.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- backend/src/services/auth/auth-fns.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/middlewares/authenticate.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check API Changes
- GitHub Check: Run integration test
- GitHub Check: Check TS and Lint
🔇 Additional comments (9)
frontend/src/pages/auth/LoginPage/LoginPage.tsx (1)
36-39: LGTM! Good addition of the authentication check.The
isLoggedIn()check ensures that redirect logic only runs for authenticated users, preventing unnecessary redirects.frontend/src/pages/auth/SignUpInvitePage/SignUpInvitePage.tsx (1)
237-280: LGTM! Comprehensive error handling for invite verification.The implementation properly handles all error cases:
- New users are directed to account setup
- Existing users go through organization selection
- Failed verifications redirect to request new invite
frontend/src/hooks/api/users/queries.tsx (3)
27-27: LGTM!The import is correctly added and follows the project's conventions.
281-283: LGTM!The logoutUser function is simple, focused, and handles the API call correctly.
304-309: LGTM!The hook has been simplified by delegating to the new utility functions while maintaining its functionality.
backend/src/server/routes/v1/auth-router.ts (3)
6-6: No issues found with the updated import statement.
23-27: Looks good – centralized token validation.You’ve successfully streamlined the logout flow by removing the one-off validation checks in favor of the shared
validateRefreshTokenmethod. This approach promotes code reuse and consistency. Nice job!
70-71: Consider verifying ongoing user membership.Currently, you’re reissuing the access token based purely on refresh token validity. If necessary, ensure that any subsequent organization membership checks (e.g., in
fnValidateJwtIdentity) remain valid, especially if the user’s org membership has changed.backend/src/services/auth-token/auth-token-service.ts (1)
2-2: New JWT import is fine.No further changes recommended here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/hooks/api/users/queries.tsx (1)
286-302: Consider adding error handling for storage operations.The
clearSessionutility effectively centralizes session cleanup logic, but storage operations could fail.Apply this diff to add error handling:
export const clearSession = (keepQueryClient?: boolean) => { + try { setAuthToken(""); // Clear authentication token localStorage.removeItem("protectedKey"); localStorage.removeItem("protectedKeyIV"); localStorage.removeItem("protectedKeyTag"); localStorage.removeItem("publicKey"); localStorage.removeItem("encryptedPrivateKey"); localStorage.removeItem("iv"); localStorage.removeItem("tag"); localStorage.removeItem("PRIVATE_KEY"); localStorage.removeItem("orgData.id"); sessionStorage.removeItem(SessionStorageKeys.CLI_TERMINAL_TOKEN); if (!keepQueryClient) { qc.clear(); // Clear React Query cache } + } catch (error) { + console.error("Failed to clear session:", error); + // Re-throw to allow mutation error handler to handle it + throw error; + } };backend/src/services/auth-token/auth-token-service.ts (1)
154-186: Well-structured token validation with comprehensive error handling.The
validateRefreshTokenimplementation is robust and secure:
- Proper null check with descriptive error
- JWT verification with correct type assertion
- Token type validation
- Version verification to prevent replay attacks
- Clear error messages for each failure case
However, consider adding a try-catch block around
jwt.verifyto handle malformed tokens gracefully.- const decodedToken = jwt.verify(refreshToken, appCfg.AUTH_SECRET) as AuthModeRefreshJwtTokenPayload; + let decodedToken: AuthModeRefreshJwtTokenPayload; + try { + decodedToken = jwt.verify(refreshToken, appCfg.AUTH_SECRET) as AuthModeRefreshJwtTokenPayload; + } catch (error) { + throw new UnauthorizedError({ + message: "Invalid refresh token", + name: "InvalidToken" + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/server/routes/v1/auth-router.ts(3 hunks)backend/src/services/auth-token/auth-token-service.ts(3 hunks)backend/src/services/kms/kms-root-config-dal.ts(1 hunks)frontend/src/hooks/api/users/queries.tsx(2 hunks)frontend/src/pages/middlewares/authenticate.tsx(2 hunks)frontend/src/pages/organization/NoOrgPage/route.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/services/kms/kms-root-config-dal.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/pages/organization/NoOrgPage/route.tsx
- frontend/src/pages/middlewares/authenticate.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
🔇 Additional comments (5)
frontend/src/hooks/api/users/queries.tsx (2)
281-283: LGTM! Good separation of concerns.The extraction of the logout API call into a separate function improves code organization and reusability.
304-309: LGTM! Clean and concise implementation.The hook now leverages the extracted utilities for better maintainability.
backend/src/server/routes/v1/auth-router.ts (2)
24-28: LGTM! Improved error handling in logout flow.The refactoring simplifies the logout flow by centralizing token validation logic in
validateRefreshToken. This reduces code duplication and ensures consistent error handling.
72-72: LGTM! Consistent token validation approach.The token handler now uses the same centralized validation approach as the logout handler, maintaining consistency across the authentication flows.
backend/src/services/auth-token/auth-token-service.ts (1)
226-226: LGTM! Proper service method export.The new method is correctly exported as part of the service interface.
There was a problem hiding this comment.
Two notes:
- I get a
not part of organizationerror when I'm removed, preventing logout (though refreshing does resolve this) - type error is due newly added catch in
authenticate.tsxsince user is not guaranteed to resolve
Screen.Recording.2025-02-19.at.9.58.23.AM.mov
Description 📣
This PR fixes a bug where users get stuck on an infinitely-loading select organization page when they have deleted all their organizations (or are no longer part of any); the user now gets correctly redirected to the page to create a new organization.
Update: This PR now also fixes another edge-case that is an existing user already logged into a deleted organization getting stuck on an error/spinner screen; in such case, the user token is no longer valid so currently I have them re-login.
Type ✨
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
Chores