Conversation
|
There was a problem hiding this comment.
Pull request overview
Refactors permission handling in several React UI routes by introducing a shared hook that checks access and redirects when a required permission is missing.
Changes:
- Added
useCheckAccessAndRedirect(permission)hook to centralize permission-check + redirect logic. - Applied the new hook to OpenOps Tables, OpenOps Analytics, and Flows routes to prevent rendering when access is missing.
- Updated side-menu link metadata to lock Tables/Analytics links based on write permissions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-ui/src/app/common/hooks/authorization-hooks.ts | Adds useCheckAccessAndRedirect hook for permission-based redirects. |
| packages/react-ui/src/app/routes/openops-tables/index.tsx | Uses the new access+redirect hook for Tables route. |
| packages/react-ui/src/app/routes/openops-analytics/index.tsx | Uses the new access+redirect hook for Analytics route. |
| packages/react-ui/src/app/routes/flows/index.tsx | Adds access+redirect gate to Flows list route. |
| packages/react-ui/src/app/routes/flows/id/index.tsx | Adds access+redirect gate to Flow builder route. |
| packages/react-ui/src/app/features/templates/components/select-flow-template-dialog-content.tsx | Replaces local permission check with redirecting hook in a dialog context. |
| packages/react-ui/src/app/features/navigation/lib/menu-links-hook.ts | Locks Tables/Analytics menu links based on permissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| export const useCheckAccessAndRedirect = (permission: Permission) => { | ||
| const { checkAccess } = useAuthorization(); | ||
| const navigate = useNavigate(); | ||
| const hasAccess = checkAccess(permission); | ||
|
|
There was a problem hiding this comment.
useCheckAccessAndRedirect relies on checkAccess(permission), but checkAccess currently always returns true, so the redirect and hasAccess gating will never trigger. If this PR is meant to enforce permissions, checkAccess needs to implement real permission evaluation (e.g., based on the user/project role/permissions) or the new hook won’t have any effect.
| const hasWriteFlowPermission = useCheckAccessAndRedirect( | ||
| Permission.WRITE_FLOW, | ||
| ); |
There was a problem hiding this comment.
This dialog content previously used checkAccess only to disable the “use template” action; switching to useCheckAccessAndRedirect will navigate the user away from the current page when they lack WRITE_FLOW, even though the UI already supports a read-only experience (it just omits useTemplate). Consider using useAuthorization().checkAccess here (no redirect), or make the redirect hook optional/configurable so non-route components don’t cause unexpected navigation.
| @@ -51,6 +53,10 @@ const FlowBuilderPage = () => { | |||
| refetchOnWindowFocus: 'always', | |||
There was a problem hiding this comment.
useQuery will still fetch the flow even when hasAccess is false (the redirect happens in an effect after render). To avoid unnecessary API calls (and caching potentially sensitive data client-side), gate the query with an enabled condition that includes hasAccess (and flowId).
| refetchOnWindowFocus: 'always', | |
| refetchOnWindowFocus: 'always', | |
| enabled: Boolean(hasAccess && flowId), |
| export const useCheckAccessAndRedirect = (permission: Permission) => { | ||
| const { checkAccess } = useAuthorization(); | ||
| const navigate = useNavigate(); | ||
| const hasAccess = checkAccess(permission); | ||
|
|
||
| useEffect(() => { | ||
| if (!hasAccess) { | ||
| navigate('/', { replace: true }); | ||
| } | ||
| }, [hasAccess, navigate]); | ||
|
|
||
| return hasAccess; | ||
| }; |
There was a problem hiding this comment.
The PR description still contains placeholders (e.g., Fixes #<issue_number>) and the template checklist isn’t filled out. Please update the PR metadata so reviewers can verify intent, scope, and testing expectations.
Fixes #<issue_number>.
Additional Notes
Testing Checklist
Check all that apply:
I tested the feature thoroughly, including edge cases
I verified all affected areas still work as expected
Automated tests were added/updated if necessary
Changes are backwards compatible with any existing data, otherwise a migration script is provided
Visual Changes (if applicable)
If there are UI/UX changes, include at least one of the following: