WEB-812: Implement RBAC for sidebar menus and client action buttons#75
Open
samruddhikasar05-bit wants to merge 2 commits intoopenMF:devfrom
Open
WEB-812: Implement RBAC for sidebar menus and client action buttons#75samruddhikasar05-bit wants to merge 2 commits intoopenMF:devfrom
samruddhikasar05-bit wants to merge 2 commits intoopenMF:devfrom
Conversation
📝 WalkthroughWalkthroughSidebar and client-view components now read permissions from Redux and conditionally render menu and dropdown items based on per-item permission metadata; username and officeName are rendered from state when available. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Comp as Component (AppSidebar / ClientsView)
participant Store as Redux Store
participant UI as UI Renderer
User->>Comp: Request page / open menu
Comp->>Store: useAppSelector(state => state.login)
Store-->>Comp: returns user object (permissions, username, officeName)
Comp->>Comp: derive permissions array and ALL_FUNCTIONS flag
Comp->>Comp: filter menu/dropdown items by requiredPermission
Comp->>UI: render filtered menu and display username/officeName
UI-->>User: visible menu items and user info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Author
|
WEB-812 Restricted sidebar and client buttons. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/pages/clients/clients-view/ClientsView.tsx (1)
47-94: Consider defining a separate type for permission-aware menu options.The
requiredPermissionfield is not part of theDropdownOptioninterface defined inDropdown.tsx. While the extra field is harmlessly ignored at runtime, this creates a type hygiene issue.♻️ Suggested approach
Define a local interface that extends or is distinct from
DropdownOption:interface PermissionedOption { label: string path?: string disabled?: boolean requiredPermission?: string children?: PermissionedOption[] }Then map to
DropdownOption[]after filtering (strippingrequiredPermission), or extend theDropdownOptioninterface inDropdown.tsxto accept and ignore the extra field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/clients/clients-view/ClientsView.tsx` around lines 47 - 94, The dropdownOptions array currently includes a non-typed requiredPermission field that isn’t in DropdownOption; define a local PermissionedOption interface (with label, path?, disabled?, requiredPermission?, children?: PermissionedOption[]) and type dropdownOptions as PermissionedOption[]. Filter by requiredPermission using permissions/hasAllFunctions, then map the filtered PermissionedOption[] to DropdownOption[] (strip requiredPermission) before passing to the Dropdown component; alternatively, extend DropdownOption in Dropdown.tsx to include optional requiredPermission if you prefer a global change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Around line 44-46: The code reads permissions from the Redux user object
(useAppSelector -> user -> permissions) but doesn't guard against non-array
values, so calls to permissions.includes(...) can throw; update the assignment
of permissions to normalize and defensively handle types (e.g., if
Array.isArray(user?.permissions) use it, else if it's a Set use
Array.from(user.permissions), else default to an empty array) so downstream code
that uses permissions.includes or array methods always receives an array.
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 29-33: The current code assumes user?.permissions is an array and
calls .includes on it; update the logic in ClientsView to defensively handle
unexpected types by checking Array.isArray(user?.permissions) and falling back
to an empty array (e.g., set permissions = Array.isArray(user?.permissions) ?
user.permissions : []), then compute hasAllFunctions from that safe permissions
variable so .includes is never called on a non-array; reference useAppSelector,
user, permissions, and hasAllFunctions when making the change.
- Around line 90-94: The top-level options array is filtered for permissions but
nested children are not, so parent menus (e.g., "Applications", "Actions",
"More") still show child items like "New Loan Account" to unauthorized users;
update the filtering to produce a new filteredOptions by mapping each option
and, for any option with a children array, replace children with
children.filter(child => !child.requiredPermission ||
permissions.includes(child.requiredPermission) || hasAllFunctions) (apply
recursively if deeper nesting is possible), then use this filteredOptions when
rendering the Dropdown (replace the current options prop with filteredOptions)
so only permitted child items appear.
---
Nitpick comments:
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 47-94: The dropdownOptions array currently includes a non-typed
requiredPermission field that isn’t in DropdownOption; define a local
PermissionedOption interface (with label, path?, disabled?, requiredPermission?,
children?: PermissionedOption[]) and type dropdownOptions as
PermissionedOption[]. Filter by requiredPermission using
permissions/hasAllFunctions, then map the filtered PermissionedOption[] to
DropdownOption[] (strip requiredPermission) before passing to the Dropdown
component; alternatively, extend DropdownOption in Dropdown.tsx to include
optional requiredPermission if you prefer a global change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3119e443-7f74-44ca-9f31-a456a20cd0bb
📒 Files selected for processing (3)
src/components/custom/sidebar/AppSidebar.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsx
Contributor
|
@samruddhikasar05-bit this is not the expected repository, let me open a new Jira Ticket for this effort. |
Contributor
|
Use this Jira Ticket https://mifosforge.jira.com/browse/MXWAR-70 |
Author
|
"Hi @IOhacker Done. I have updated the Jira ticket reference o MXWAR-70 as requested. I've also implemented the conditional rendering for the client action buttons in ClientsView.tsx based on the client's status and permissions. Ready for review!" |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/custom/sidebar/AppSidebar.tsx (1)
117-130:⚠️ Potential issue | 🔴 CriticalCritical: Syntax errors prevent compilation.
The array contains invalid JavaScript that will cause parse errors:
- Line 117 has a stray
devidentifier (possibly a merge conflict artifact or debug leftover)- Lines 118-119 duplicate the dashboard and navigation items that already exist on lines 120-129
- Lines 129-130 are malformed — missing proper array structure
This code will not compile. Remove the invalid lines and ensure proper array syntax.
🐛 Proposed fix
<SidebarMenu> {[ - dev - { icon: <Gauge />, label: t('nav.dashboard'), route: 'dashboard', permission: 'ALL_FUNCTIONS' }, - { icon: <Send />, label: t('nav.navigation'), route: 'navigation', permission: 'ALL_FUNCTIONS' }, { icon: <Gauge />, label: t('nav.dashboard'), route: 'dashboard', + permission: 'ALL_FUNCTIONS', }, { icon: <Send />, label: t('nav.navigation'), route: 'navigation', - } - + permission: 'ALL_FUNCTIONS', + }, { icon: <Check />,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` around lines 117 - 130, Remove the stray "dev" token and the duplicated dashboard/navigation entries in the sidebar items array inside AppSidebar so the array is valid JavaScript; keep a single entry each for the Gauge/t('nav.dashboard') route 'dashboard' and the Send/t('nav.navigation') route 'navigation' (with permissions if needed), ensure each object is comma-separated and the array opens/closes correctly so the file compiles without parse errors.src/pages/clients/clients-view/ClientsView.tsx (1)
169-259:⚠️ Potential issue | 🔴 CriticalCritical: Duplicate
optionsprop completely bypasses permission filtering.The
optionsprop is passed twice to the Dropdown component:
- Line 170:
options={dropdownOptions}(permission-filtered)- Lines 172-258:
options={[...]}(hardcoded, no filtering)In JSX, when duplicate props are provided, the last one wins. This means the permission-filtered
dropdownOptionsis overridden by the hardcoded array, and all users will see all menu items regardless of permissions.Remove the duplicate prop block (lines 172-258) and keep only the filtered
dropdownOptions.🐛 Proposed fix to remove duplicate prop
<Dropdown name={ <span className="flex items-center gap-2"> <Menu /> </span> } - options={dropdownOptions} - - options={[ - { label: t('view.menu.edit'), path: `clients/${client?.id}/edit` }, - { - label: t('view.menu.applications'), - children: [ - // ... all children - ], - }, - // ... rest of hardcoded options (lines 172-258) - ]} - />Note: After removing the duplicate, ensure
dropdownOptionsuses i18n translations (see separate comment) and implements recursive children filtering (see previous review).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/clients/clients-view/ClientsView.tsx` around lines 169 - 259, The Dropdown in ClientsView is receiving duplicate options props so the hardcoded array overrides the permission-filtered dropdownOptions; remove the second options=[...] prop block (the large hardcoded array) and leave only options={dropdownOptions} on the Dropdown (ensure dropdownOptions is used for i18n and recursive children filtering afterwards); locate the Dropdown JSX in ClientsView.tsx and remove the hardcoded options array so only the dropdownOptions variable is passed.
♻️ Duplicate comments (1)
src/components/custom/sidebar/AppSidebar.tsx (1)
44-46:⚠️ Potential issue | 🔴 CriticalCritical: Wrong Redux state key —
state.logindoes not exist.The Redux store configuration (see
src/app/store.ts) registers the auth reducer under the keyauth, notlogin:reducer: { auth: authReducer, }Accessing
state.loginreturnsundefined, souserwill always beundefinedand the RBAC filtering will fail silently (all items shown or hidden depending on fallback).🐛 Proposed fix (also addresses past review on defensive array check)
// WEB-812: Fetch user permissions from Redux state - const { user } = useAppSelector((state) => state.login) - const permissions = user?.permissions || [] + const { user } = useAppSelector((state) => state.auth) + const permissions = Array.isArray(user?.permissions) ? user.permissions : []The defensive
Array.isArraycheck was flagged in a previous review and remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` around lines 44 - 46, The selector is using the wrong Redux key (state.login) which doesn't exist; update the useAppSelector call in AppSidebar (where user and permissions are read) to access state.auth (e.g., (state) => state.auth) so it retrieves the correct auth.user, and replace the loose permissions fallback with a defensive Array.isArray check (derive permissions as user?.permissions if Array.isArray(user?.permissions) else [] ) to ensure RBAC filtering works reliably; update references to the variables user and permissions in the RBAC/filter logic accordingly.
🧹 Nitpick comments (2)
src/components/custom/sidebar/AppSidebar.tsx (1)
167-172: Redundant permission check in filter logic.The filter condition checks both
permissions.includes(item.permission)andpermissions.includes("ALL_FUNCTIONS")for every item. If the user hasALL_FUNCTIONS, checking each item's specific permission is unnecessary.Consider extracting
hasAllFunctionsonce before filtering to improve readability and avoid repeated array scans:♻️ Suggested refactor
const permissions = Array.isArray(user?.permissions) ? user.permissions : [] + const hasAllFunctions = permissions.includes('ALL_FUNCTIONS') // ... later in render ... - .filter((item) => - !item.permission || - permissions.includes(item.permission) || - permissions.includes("ALL_FUNCTIONS") - ) + .filter((item) => + !item.permission || + hasAllFunctions || + permissions.includes(item.permission) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` around lines 167 - 172, Extract a precomputed flag (e.g., hasAllFunctions = permissions.includes("ALL_FUNCTIONS")) before the array filter in AppSidebar.tsx and use it inside the filter that currently checks !item.permission || permissions.includes(item.permission) || permissions.includes("ALL_FUNCTIONS"); replace the repeated permissions.includes("ALL_FUNCTIONS") call with the hasAllFunctions variable so the filter becomes !item.permission || hasAllFunctions || permissions.includes(item.permission), reducing repeated array scans and improving readability.src/pages/clients/clients-view/ClientsView.tsx (1)
52-93: Hardcoded labels bypass internationalization.The new
dropdownOptionsuses hardcoded English strings ('Edit','Applications', etc.) instead of the translation function. The duplicate options block below (lines 172-258) correctly usest('view.menu.edit')and similar i18n calls. The filtered options should also use translations for consistency.♻️ Proposed fix to use i18n
const dropdownOptions = [ { - label: 'Edit', + label: t('view.menu.edit'), path: `clients/${client?.id}/edit`, requiredPermission: 'UPDATE_CLIENT' }, { - label: 'Applications', + label: t('view.menu.applications'), children: [ - { label: 'New Loan Account', path: 'signature', disabled: true, requiredPermission: 'CREATE_LOAN' }, - { label: 'New Savings Account', path: 'signature', disabled: true, requiredPermission: 'CREATE_SAVINGSACCOUNT' }, + { label: t('view.menu.newLoanAccount'), path: 'signature', disabled: true, requiredPermission: 'CREATE_LOAN' }, + { label: t('view.menu.newSavingsAccount'), path: 'signature', disabled: true, requiredPermission: 'CREATE_SAVINGSACCOUNT' }, // ... apply similar changes to all other labels ], }, // ... continue for all options ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/clients/clients-view/ClientsView.tsx` around lines 52 - 93, The dropdownOptions array uses hardcoded English labels; replace each label string with the appropriate i18n call (e.g. change 'Edit' to t('view.menu.edit'), 'Applications' to t('view.menu.applications'), 'Actions' to t('view.menu.actions'), 'Unassign Staff' to t('view.menu.unassignStaff') and map all child labels to their corresponding t(...) keys consistent with the existing menu block (use the same translation keys used in the duplicate block below); ensure the file uses the same translation function (t) already imported or from useTranslation() in this component and update every label in dropdownOptions accordingly so the menu is fully internationalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Around line 117-130: Remove the stray "dev" token and the duplicated
dashboard/navigation entries in the sidebar items array inside AppSidebar so the
array is valid JavaScript; keep a single entry each for the
Gauge/t('nav.dashboard') route 'dashboard' and the Send/t('nav.navigation')
route 'navigation' (with permissions if needed), ensure each object is
comma-separated and the array opens/closes correctly so the file compiles
without parse errors.
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 169-259: The Dropdown in ClientsView is receiving duplicate
options props so the hardcoded array overrides the permission-filtered
dropdownOptions; remove the second options=[...] prop block (the large hardcoded
array) and leave only options={dropdownOptions} on the Dropdown (ensure
dropdownOptions is used for i18n and recursive children filtering afterwards);
locate the Dropdown JSX in ClientsView.tsx and remove the hardcoded options
array so only the dropdownOptions variable is passed.
---
Duplicate comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Around line 44-46: The selector is using the wrong Redux key (state.login)
which doesn't exist; update the useAppSelector call in AppSidebar (where user
and permissions are read) to access state.auth (e.g., (state) => state.auth) so
it retrieves the correct auth.user, and replace the loose permissions fallback
with a defensive Array.isArray check (derive permissions as user?.permissions if
Array.isArray(user?.permissions) else [] ) to ensure RBAC filtering works
reliably; update references to the variables user and permissions in the
RBAC/filter logic accordingly.
---
Nitpick comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Around line 167-172: Extract a precomputed flag (e.g., hasAllFunctions =
permissions.includes("ALL_FUNCTIONS")) before the array filter in AppSidebar.tsx
and use it inside the filter that currently checks !item.permission ||
permissions.includes(item.permission) || permissions.includes("ALL_FUNCTIONS");
replace the repeated permissions.includes("ALL_FUNCTIONS") call with the
hasAllFunctions variable so the filter becomes !item.permission ||
hasAllFunctions || permissions.includes(item.permission), reducing repeated
array scans and improving readability.
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 52-93: The dropdownOptions array uses hardcoded English labels;
replace each label string with the appropriate i18n call (e.g. change 'Edit' to
t('view.menu.edit'), 'Applications' to t('view.menu.applications'), 'Actions' to
t('view.menu.actions'), 'Unassign Staff' to t('view.menu.unassignStaff') and map
all child labels to their corresponding t(...) keys consistent with the existing
menu block (use the same translation keys used in the duplicate block below);
ensure the file uses the same translation function (t) already imported or from
useTranslation() in this component and update every label in dropdownOptions
accordingly so the menu is fully internationalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fec3a99c-c7a6-4228-9ab3-5885ca9e0e60
📒 Files selected for processing (3)
src/components/custom/sidebar/AppSidebar.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsx
✅ Files skipped from review due to trivial changes (1)
- src/pages/clients/Clients.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Describe the changes made and why they were made instead of how they were made. List any dependencies that are required for this change.
Related issues and discussion
#{Issue Number}
Screenshots, if any
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes