Mxwar 70 client action buttons#83
Open
samruddhikasar05-bit wants to merge 3 commits intoopenMF:devfrom
Open
Conversation
📝 WalkthroughWalkthroughIntegrates Redux user state to apply permission-based filtering to sidebar items and client-view dropdowns; UI elements now include required-permission metadata and render only when the user has matching permissions (including ALL_FUNCTIONS). Sidebar header shows username and officeName. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientsView
participant Redux as Store
participant AppSidebar as Sidebar
User->>ClientsView: open client page
ClientsView->>Store: read current user & permissions
ClientsView-->>ClientsView: build dropdownOptions (status + requiredPermission)
ClientsView->>ClientsView: filter options by permissions
ClientsView-->>User: render allowed actions
User->>Sidebar: open sidebar
Sidebar->>Store: read current user & permissions
Sidebar-->>Sidebar: filter sidebar items by permission
Sidebar-->>User: render permitted menu items
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 43-46: The selector is using the wrong Redux slice key; update the
useAppSelector call in AppSidebar to read the auth slice instead of login so it
correctly retrieves the user and permissions (change useAppSelector((state) =>
state.login) to useAppSelector((state) => state.auth)); ensure the local const
user and const permissions = user?.permissions || [] now receive real data so
permission checks (permissions usage) and username/office rendering use the
actual user object.
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 62-66: The current menu options filtering only checks top-level
items (the array assigned to options) and ignores nested children under
option.children, so child items with requiredPermission (e.g., CREATE_LOAN,
CLOSE_CLIENT) still appear; implement a recursive filter (e.g., create a helper
like filterMenuOptions or filterChildren) that walks each option, removes
children that fail the permission check (using option.requiredPermission,
permissions, and hasAllFunctions), and drops parent items that become empty
unless they themselves pass; apply this recursive filter where the existing
].filter(...) is used so both top-level and nested menu children are filtered by
permissions.
- Around line 31-33: The menu items in ClientsView.tsx for "New Share Account",
"New Recurring Deposit Account", and "New Fixed Deposit Account" use a
placeholder path 'signature' which can cause incorrect navigation if the
disabled flag is ever bypassed; update those entries (the objects with label
'New Share Account', 'New Recurring Deposit Account', 'New Fixed Deposit
Account') to use either their real route names or set path to null/undefined and
ensure the rendering/navigation logic (where the menu consumes item.path) guards
against falsy paths so clicks do nothing when disabled or path is absent.
- Around line 1-6: The file currently only defines dropdownOptions and is
missing the ClientsView React component, necessary imports, hooks and variable
definitions; restore or reimplement the full component by adding React and
router imports, declare the ClientsView function/component, initialize state and
hooks (useState, useEffect, useParams) to populate client and permissions,
implement or import hasAllFunctions helper and ensure client, permissions are
defined before building dropdownOptions, and return the JSX wrapper rendering
the dropdownOptions; ensure symbols referenced (dropdownOptions, client,
permissions, hasAllFunctions, ClientsView) are defined and wired into the
component lifecycle so the file compiles and behaves like the previous version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ab95abb-aade-427f-bccc-5fd4f801dfa8
📒 Files selected for processing (3)
src/components/custom/sidebar/AppSidebar.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsx
Comment on lines
+43
to
+46
|
|
||
| // WEB-812: Fetch user permissions from Redux state | ||
| const { user } = useAppSelector((state) => state.login) | ||
| const permissions = user?.permissions || [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Redux store configuration to confirm the correct state path
echo "=== Checking store configuration ==="
cat src/app/store.ts
echo -e "\n=== Checking if any other files use state.login ==="
rg -n "state\.login" --type=ts --type=tsxRepository: openMF/mifos-x-web-app-react
Length of output: 751
🏁 Script executed:
cat -n src/components/custom/sidebar/AppSidebar.tsx | head -80Repository: openMF/mifos-x-web-app-react
Length of output: 2987
🏁 Script executed:
rg -n "permissions" src/components/custom/sidebar/AppSidebar.tsxRepository: openMF/mifos-x-web-app-react
Length of output: 297
🏁 Script executed:
rg -n "state\." src/components/custom/sidebar/AppSidebar.tsxRepository: openMF/mifos-x-web-app-react
Length of output: 132
Incorrect Redux state path: state.login should be state.auth.
The store configuration in src/app/store.ts mounts the auth reducer under the auth key, not login. This selector will always return undefined, causing:
userto beundefinedpermissionsto default to[]- Permission checks in lines 158–159 to fail silently
- Username/office display in line 79 to always show fallback values
Fix
- const { user } = useAppSelector((state) => state.login)
+ const { user } = useAppSelector((state) => state.auth)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // WEB-812: Fetch user permissions from Redux state | |
| const { user } = useAppSelector((state) => state.login) | |
| const permissions = user?.permissions || [] | |
| // WEB-812: Fetch user permissions from Redux state | |
| const { user } = useAppSelector((state) => state.auth) | |
| const permissions = user?.permissions || [] |
🤖 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 43 - 46, The
selector is using the wrong Redux slice key; update the useAppSelector call in
AppSidebar to read the auth slice instead of login so it correctly retrieves the
user and permissions (change useAppSelector((state) => state.login) to
useAppSelector((state) => state.auth)); ensure the local const user and const
permissions = user?.permissions || [] now receive real data so permission checks
(permissions usage) and username/office rendering use the actual user object.
Comment on lines
+31
to
+33
| { label: 'New Share Account', path: 'signature', disabled: true }, | ||
| { label: 'New Recurring Deposit Account', path: 'signature', disabled: true }, | ||
| { label: 'New Fixed Deposit Account', path: 'signature', disabled: true }, |
There was a problem hiding this comment.
Placeholder paths may cause confusion or navigation bugs.
These disabled items use path: 'signature' which appears to be a placeholder. If these items are accidentally enabled (or the disabled check fails), users would navigate to an incorrect route.
💡 Suggested improvement
- { label: 'New Share Account', path: 'signature', disabled: true },
- { label: 'New Recurring Deposit Account', path: 'signature', disabled: true },
- { label: 'New Fixed Deposit Account', path: 'signature', disabled: true },
+ { label: 'New Share Account', path: `clients/${client?.id}/new-share`, disabled: true }, // TODO: Implement share account creation
+ { label: 'New Recurring Deposit Account', path: `clients/${client?.id}/new-recurring-deposit`, disabled: true }, // TODO: Implement recurring deposit
+ { label: 'New Fixed Deposit Account', path: `clients/${client?.id}/new-fixed-deposit`, disabled: true }, // TODO: Implement fixed deposit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { label: 'New Share Account', path: 'signature', disabled: true }, | |
| { label: 'New Recurring Deposit Account', path: 'signature', disabled: true }, | |
| { label: 'New Fixed Deposit Account', path: 'signature', disabled: true }, | |
| { label: 'New Share Account', path: `clients/${client?.id}/new-share`, disabled: true }, // TODO: Implement share account creation | |
| { label: 'New Recurring Deposit Account', path: `clients/${client?.id}/new-recurring-deposit`, disabled: true }, // TODO: Implement recurring deposit | |
| { label: 'New Fixed Deposit Account', path: `clients/${client?.id}/new-fixed-deposit`, disabled: true }, // TODO: Implement fixed deposit |
🤖 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 31 - 33, The
menu items in ClientsView.tsx for "New Share Account", "New Recurring Deposit
Account", and "New Fixed Deposit Account" use a placeholder path 'signature'
which can cause incorrect navigation if the disabled flag is ever bypassed;
update those entries (the objects with label 'New Share Account', 'New Recurring
Deposit Account', 'New Fixed Deposit Account') to use either their real route
names or set path to null/undefined and ensure the rendering/navigation logic
(where the menu consumes item.path) guards against falsy paths so clicks do
nothing when disabled or path is absent.
Comment on lines
+62
to
+66
| ].filter(option => | ||
| !option.requiredPermission || | ||
| permissions.includes(option.requiredPermission) || | ||
| hasAllFunctions | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Nested children items are not filtered by permission.
The filter only checks option.requiredPermission on top-level items. Nested children within Applications, Actions, and More groups have requiredPermission fields that are never evaluated. Users without permissions like CREATE_LOAN or CLOSE_CLIENT will still see those menu items.
🐛 Proposed fix: Filter nested children recursively
- ].filter(option =>
- !option.requiredPermission ||
- permissions.includes(option.requiredPermission) ||
- hasAllFunctions
- )
+ ].map(option => {
+ if (option.children) {
+ return {
+ ...option,
+ children: option.children.filter(child =>
+ !child.requiredPermission ||
+ permissions.includes(child.requiredPermission) ||
+ hasAllFunctions
+ ),
+ }
+ }
+ return option
+ }).filter(option => {
+ // Filter top-level items by permission
+ if (option.requiredPermission) {
+ return permissions.includes(option.requiredPermission) || hasAllFunctions
+ }
+ // Filter out parent items with no remaining children
+ if (option.children) {
+ return option.children.length > 0
+ }
+ return true
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ].filter(option => | |
| !option.requiredPermission || | |
| permissions.includes(option.requiredPermission) || | |
| hasAllFunctions | |
| ) | |
| ].map(option => { | |
| if (option.children) { | |
| return { | |
| ...option, | |
| children: option.children.filter(child => | |
| !child.requiredPermission || | |
| permissions.includes(child.requiredPermission) || | |
| hasAllFunctions | |
| ), | |
| } | |
| } | |
| return option | |
| }).filter(option => { | |
| // Filter top-level items by permission | |
| if (option.requiredPermission) { | |
| return permissions.includes(option.requiredPermission) || hasAllFunctions | |
| } | |
| // Filter out parent items with no remaining children | |
| if (option.children) { | |
| return option.children.length > 0 | |
| } | |
| return true | |
| }) |
🤖 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 62 - 66, The
current menu options filtering only checks top-level items (the array assigned
to options) and ignores nested children under option.children, so child items
with requiredPermission (e.g., CREATE_LOAN, CLOSE_CLIENT) still appear;
implement a recursive filter (e.g., create a helper like filterMenuOptions or
filterChildren) that walks each option, removes children that fail the
permission check (using option.requiredPermission, permissions, and
hasAllFunctions), and drops parent items that become empty unless they
themselves pass; apply this recursive filter where the existing ].filter(...) is
used so both top-level and nested menu children are filtered by permissions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/clients/clients-view/ClientsView.tsx (1)
107-111:⚠️ Potential issue | 🟠 MajorNested children items bypass permission filtering.
The
.filter()at lines 107-111 only evaluatesrequiredPermissionon top-level items. Children withinApplications,Actions, andMorehaverequiredPermissionfields (e.g.,CREATE_LOAN,CLOSE_CLIENT) that are never checked. Users without these permissions will still see the child menu items.🐛 Proposed fix: Filter nested children recursively
- ].filter(option => - !option.requiredPermission || - permissions.includes(option.requiredPermission) || - hasAllFunctions - ) + ].map(option => { + if (option.children) { + return { + ...option, + children: option.children.filter(child => + child.disabled || + !child.requiredPermission || + permissions.includes(child.requiredPermission) || + hasAllFunctions + ), + } + } + return option + }).filter(option => { + // Filter top-level items by permission + if (option.requiredPermission) { + return permissions.includes(option.requiredPermission) || hasAllFunctions + } + // Filter out parent items with no remaining children + if (option.children) { + return option.children.length > 0 + } + return true + })🤖 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 107 - 111, The top-level filtering of the menu `options` in ClientsView.tsx only checks `requiredPermission` for parent items and does not remove child entries that have their own `requiredPermission`; modify the logic so each menu item is processed recursively: keep the current check for `option.requiredPermission` using `permissions` or `hasAllFunctions`, and if `option.children` exists, replace it with a filtered version that applies the same permission check to each child (and recurses for nested children), and only include parent items whose filtered `children` array is non-empty or that themselves pass the permission check; implement this as a small helper (e.g., `filterMenuOptions` or `filterOption`) and use it instead of the current inline `.filter(...)` on `options`.
🤖 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/pages/clients/clients-view/ClientsView.tsx`:
- Around line 47-52: The dropdownOptions array is building route strings using
client?.id which yields "clients/undefined/edit" while client data is loading;
update ClientsView to avoid constructing or rendering the dropdown until client
is defined—either return early from the component or conditionally render the
dropdown/menu (or provide disabled items) so dropdownOptions is only created
when client is non-null; locate the dropdownOptions declaration and the
component render that consumes it and wrap that logic with a guard like if
(!client) return null or render a loading/disabled state.
---
Duplicate comments:
In `@src/pages/clients/clients-view/ClientsView.tsx`:
- Around line 107-111: The top-level filtering of the menu `options` in
ClientsView.tsx only checks `requiredPermission` for parent items and does not
remove child entries that have their own `requiredPermission`; modify the logic
so each menu item is processed recursively: keep the current check for
`option.requiredPermission` using `permissions` or `hasAllFunctions`, and if
`option.children` exists, replace it with a filtered version that applies the
same permission check to each child (and recurses for nested children), and only
include parent items whose filtered `children` array is non-empty or that
themselves pass the permission check; implement this as a small helper (e.g.,
`filterMenuOptions` or `filterOption`) and use it instead of the current inline
`.filter(...)` on `options`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb74d347-f691-4f7d-a9ad-cdac7fc805fe
📒 Files selected for processing (1)
src/pages/clients/clients-view/ClientsView.tsx
Comment on lines
+47
to
+52
| const dropdownOptions = [ | ||
| { | ||
| label: 'Edit', | ||
| path: `clients/${client?.id}/edit`, | ||
| requiredPermission: 'UPDATE_CLIENT' | ||
| }, |
There was a problem hiding this comment.
Paths contain undefined before client data loads.
When client is undefined (before fetch completes), paths like `clients/${client?.id}/edit` resolve to clients/undefined/edit. If a user clicks the dropdown before data loads, navigation would go to an invalid route.
Consider guarding the dropdown or returning early when client is not yet available.
💡 Suggested fix: Conditionally render dropdown
- <Dropdown
- name={
- <span className="flex items-center gap-2">
- <Menu />
- </span>
- }
- options={dropdownOptions}
- />
+ {client?.id && (
+ <Dropdown
+ name={
+ <span className="flex items-center gap-2">
+ <Menu />
+ </span>
+ }
+ options={dropdownOptions}
+ />
+ )}🤖 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 - 52, The
dropdownOptions array is building route strings using client?.id which yields
"clients/undefined/edit" while client data is loading; update ClientsView to
avoid constructing or rendering the dropdown until client is defined—either
return early from the component or conditionally render the dropdown/menu (or
provide disabled items) so dropdownOptions is only created when client is
non-null; locate the dropdownOptions declaration and the component render that
consumes it and wrap that logic with a guard like if (!client) return null or
render a loading/disabled state.
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
1 participant
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