EDM-2730: Redirect to valid page after switching organization#409
Conversation
WalkthroughReplaced full-page reload on organization switch with a client-side redirect in PageNavigation. Added a list of primary routes and a helper that maps detail/edit paths to corresponding list/overview paths, using location from app context to compute the redirect and preserve user context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-04-10T12:22:38.239ZApplied to files:
🧬 Code graph analysis (1)libs/ui-components/src/components/common/PageNavigation.tsx (3)
⏰ 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). (4)
🔇 Additional comments (4)
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/ui-components/src/components/common/PageNavigation.tsx (2)
67-84: Validate redirect heuristics and reduce duplication of route stringsThe helper is clear and solves the “detail → parent list” case, but a few points are worth double‑checking:
- It hard‑codes path segments (
/edge,/devicemanagement,/admin,/overview,authproviders) instead of usingROUTE/appRoutes, so future route/base‑path changes can drift from this logic.- Non‑detail URLs like
/edge/devicesor/admin/authproviderswill fall through to'/overview'; switching org from those list pages will jump the user away instead of keeping them on the same list. Please confirm this is intentional for the UX you want.- The regexes won’t match if routing ever introduces trailing slashes or slightly different segment names (e.g.
device-managementvsdevicemanagement), so it’s worth verifying them against the currentappRoutesconfig.I’d recommend:
- Basing the returned paths on
ROUTE/appRoutes(e.g. mapping the captured entity segment to a route constant) to keep a single source of truth.- Adding small unit tests for
getRedirectPathAfterOrgSwitchwith representative paths (device details incl. subroutes, admin details, list pages, and unknown paths) so regressions are caught early.
24-24: Reconsider full-page navigation here; prefer router navigation and route constants if possibleFunctionally this works, but a couple of aspects could be improved:
- Using
window.location.href = targetPathtriggers a full reload of the SPA. If you don’t rely on a hard reload to pick up new org context, it would be cleaner to navigate via the router (eitheruseNavigateorrouter.useNavigate) and feed it a route constant instead of a literal path. That would also centralize path construction inappRoutesrather than scattering string paths.getRedirectPathAfterOrgSwitchalready derives the “parent” context; instead of returning/edge/...strings, it could return aROUTEkey (or be split into “determine parent entity type” + “map entity type to route constant”), then this callback could just callnavigate(parentRoute). That avoids coupling this component to hard‑coded URLs and the top‑level/overviewpath.If you do need to keep the full reload semantics for now, consider at least basing
targetPathonROUTE/appRoutesso future route renames or base‑path changes don’t silently break this redirect.Also applies to: 89-90, 148-149
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-components/src/components/common/PageNavigation.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 363
File: libs/ui-components/src/hooks/useWebSocket.ts:54-58
Timestamp: 2025-10-08T11:21:15.680Z
Learning: In flightctl-ui, when organizations are enabled, all API requests require the org_id parameter and fail with 403 if it's missing. This means users can only reach the Device details page (and its Terminal tab) if they have a valid organization selected, so organizationId will always be defined in that context.
🧬 Code graph analysis (1)
libs/ui-components/src/components/common/PageNavigation.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(123-123)
⏰ 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). (4)
- GitHub Check: integration-tests
- GitHub Check: Lint
- GitHub Check: Build ocp plugin
- GitHub Check: Build
ee3ebfc to
8a117f8
Compare
When users switch organization while logged in and while viewing a resource details page, after the switch they don't have access to the same resource as it belongs to the previous organization.
Now we change the page to either the parent page (eg. view repository --> repository list), or Overview as a fallback.
switch-org-details.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.