Open
Conversation
📝 WalkthroughWalkthroughReplaces the wide, content-heavy AppSidebar with a compact, icon-only, permission-driven sidebar; refactors LoansView to simplify permission checks, introduce an actionsBuild helper, consolidate loan fetching into local state, and reduce header/tabs to a leaner UI (all internal changes; component APIs unchanged). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppSidebar
participant Store as "Redux Store\n(state.auth)"
participant Router
participant LogoutHandler
rect rgba(200,200,255,0.5)
User->>AppSidebar: hover icon
AppSidebar->>AppSidebar: show Tooltip(label)
end
rect rgba(200,255,200,0.5)
User->>AppSidebar: click icon
AppSidebar->>Store: read user.permissions
alt permission satisfied or no requiredPermission
AppSidebar->>Router: navigate(route)
else no permission
AppSidebar->>User: (no action / disabled)
end
end
rect rgba(255,200,200,0.5)
User->>AppSidebar: click Logout
AppSidebar->>LogoutHandler: dispatch(logout)
LogoutHandler->>Router: navigate("/login")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
IOhacker
requested changes
Mar 16, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Hello, please review my post on the #gsoc channel the WEB preffix is for the Angular version, the MXWAR is for react. The WEB-812 is for the Angular version
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/components/custom/sidebar/AppSidebar.tsx (2)
40-40: Unused translation hook - labels should be internationalized.The
tfunction fromuseTranslationis imported but never used. Menu labels (lines 50-55) and tooltip content (lines 91, 104, 118) use hard-coded English strings.♻️ Example fix for one menu item
- { icon: <Home size={24} />, label: 'Home', route: 'dashboard', requiredPermission: null}, + { icon: <Home size={24} />, label: t('sidebar.home'), route: 'dashboard', requiredPermission: null},And for tooltip content:
- <TooltipContent side="right"><p>{item.label}</p></TooltipContent> + <TooltipContent side="right"><p>{t(item.labelKey)}</p></TooltipContent>Also applies to: 49-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` at line 40, The file AppSidebar imports useTranslation and destructures t but never uses it; replace hard-coded menu labels and tooltip strings with internationalized keys by calling t(...) where those strings are defined (e.g., in the JSX for menu items around the block that renders the menu labels at lines referenced as menu labels and tooltip content locations). Update the menu items rendered in AppSidebar (search for the JSX that renders the menu labels and tooltip props) to use t('sidebar.<key>') or appropriate keys, and ensure corresponding keys are added to the common translation namespace used by useTranslation('common').
95-121: Consider semantic structure of bottom action area.The
<div className="mt-auto...">is nested directly insideSidebarMenu, which may expect onlySidebarMenuItemchildren. This could cause styling or accessibility issues depending on the Sidebar component implementation.♻️ Alternative structure
</SidebarMenu> + <SidebarMenu className="mt-auto flex flex-col items-center gap-4 w-full"> - <div className="mt-auto pt-10 flex flex-col items-center gap-4 w-full"> {permissions.includes('ALL_FUNCTIONS') && ( ... )} <SidebarMenuItem ...> ... </SidebarMenuItem> - </div> + </SidebarMenu> - </SidebarMenu>🤖 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 95 - 121, The bottom actions are wrapped in a plain <div> inside SidebarMenu which may expect only SidebarMenuItem children; replace that <div className="mt-auto pt-10 flex..."> with valid menu items or a menu-grouping component so SidebarMenu only contains SidebarMenuItem children (e.g., move the mt-auto/pt-10 flex classes onto a wrapping SidebarMenuItem or create a SidebarMenuGroup component and nest the existing SidebarMenuItem entries inside it) to preserve styling and accessibility for the Settings and Logout elements referenced by SidebarMenu, SidebarMenuItem, SidebarMenuButton, handleLogout and navigate.src/pages/loans/loans-view/LoansView.tsx (1)
24-24: Consider replacinganywith proper type.
Loan = anyloses type safety. Consider using the generated type from the Fineract OpenAPI client if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/loans/loans-view/LoansView.tsx` at line 24, Replace the broad alias "type Loan = any" with the specific generated Fineract OpenAPI loan type: import the appropriate loan interface/type from the OpenAPI client (e.g., the generated Loan or LoanResponse type) and update the LoansView component's type alias/usages to use that imported type (search for the "Loan" alias and the "LoansView" component to locate places to change); ensure any array or paginated shapes match the exact generated types and update related props/state types accordingly.
🤖 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`:
- Line 55: In AppSidebar.tsx the Accounting menu item's property is misspelled
as requiredPersmission so the permission filter (which checks
item.requiredPermission) always treats it as missing; rename the property to
requiredPermission on the Accounting menu item object (the entry with label
'Accounting' and icon Network) so the existing permission filter logic will
correctly hide/show it based on user permissions.
- Line 76: The permission check is inconsistent: AppSidebar.tsx uses the literal
'ALL_FUNCTIONS' while LoansView.tsx uses 'ALL_PERMISSIONS'; create a single
exported constant (e.g., SUPER_ADMIN_PERMISSION) in a shared module (e.g.,
permissions constant file) and replace the literal uses in both
components—specifically update the checks in AppSidebar (the expression using
permissions.includes(...)) and in LoansView to reference SUPER_ADMIN_PERMISSION
so both components use the same backend-backed value.
In `@src/pages/loans/loans-view/LoansView.tsx`:
- Line 43: The template string for base (`const base =
\`/groups/${groupId}/loans-accounts/${loanId}\``) can produce
`/groups/undefined/...` if groupId is missing; update LoansView to guard against
undefined groupId (and optionally loanId) by early-returning or redirecting when
useParams() yields no groupId, or substitute a safe fallback value and surface
an error (e.g., show a not-found UI or call navigate to a safe route). Locate
the `base` declaration in LoansView.tsx and implement the chosen guard (early
return null/UI, navigate(), or throw) before any code that uses `base` so the
app never constructs or uses a URL with `undefined` in it.
- Around line 25-28: The Action type declares path: string but the fallback
DropdownOption lacks path, causing a type mismatch; update the Action type to
make path optional (change path: string to path?: string) or modify the fallback
option to include a valid placeholder path string so Action and DropdownOption
shapes align—adjust the type definition named Action and the fallback option
used where DropdownOption is provided to keep typings consistent.
- Line 37: The permissions check uses the string literal 'ALL_PERMISSIONS' (in
the expression that sets hasAll from permissions) which is inconsistent with
AppSidebar's 'ALL_FUNCTIONS'; update the constant used by replacing
'ALL_PERMISSIONS' with the canonical 'ALL_FUNCTIONS' (or reference a shared
constant if available) so that the hasAll computation
(permissions.includes(...)) matches AppSidebar's permission key.
- Line 50: The actions array is pushing an incorrect path string with a typo;
update the actions.push call that currently uses path:
`${base}/actions/Appprove` to use the correctly spelled path:
`${base}/actions/Approve` (adjust the "Appprove" token to "Approve") so
navigation from LoansView (where actions.push is invoked) points to the intended
route.
- Around line 39-42: The useEffect calling loansApi.retrieveLoan(loanId) lacks
error handling; update the effect around useEffect / loansApi.retrieveLoan /
setloan to catch failures (use async/await with try/catch or .catch) and handle
loading/error state: set a loading flag, on success call setloan(res.data), on
error set an error state (or clear loan via setloan(null)) and surface user
feedback (e.g., show a toast or render an error message), and guard against
stale responses by using an abort flag or abort controller so out-of-order
promises don’t overwrite state.
---
Nitpick comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Line 40: The file AppSidebar imports useTranslation and destructures t but
never uses it; replace hard-coded menu labels and tooltip strings with
internationalized keys by calling t(...) where those strings are defined (e.g.,
in the JSX for menu items around the block that renders the menu labels at lines
referenced as menu labels and tooltip content locations). Update the menu items
rendered in AppSidebar (search for the JSX that renders the menu labels and
tooltip props) to use t('sidebar.<key>') or appropriate keys, and ensure
corresponding keys are added to the common translation namespace used by
useTranslation('common').
- Around line 95-121: The bottom actions are wrapped in a plain <div> inside
SidebarMenu which may expect only SidebarMenuItem children; replace that <div
className="mt-auto pt-10 flex..."> with valid menu items or a menu-grouping
component so SidebarMenu only contains SidebarMenuItem children (e.g., move the
mt-auto/pt-10 flex classes onto a wrapping SidebarMenuItem or create a
SidebarMenuGroup component and nest the existing SidebarMenuItem entries inside
it) to preserve styling and accessibility for the Settings and Logout elements
referenced by SidebarMenu, SidebarMenuItem, SidebarMenuButton, handleLogout and
navigate.
In `@src/pages/loans/loans-view/LoansView.tsx`:
- Line 24: Replace the broad alias "type Loan = any" with the specific generated
Fineract OpenAPI loan type: import the appropriate loan interface/type from the
OpenAPI client (e.g., the generated Loan or LoanResponse type) and update the
LoansView component's type alias/usages to use that imported type (search for
the "Loan" alias and the "LoansView" component to locate places to change);
ensure any array or paginated shapes match the exact generated types and update
related props/state types accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 910768db-cc23-4124-b882-bc7d048ce281
📒 Files selected for processing (2)
src/components/custom/sidebar/AppSidebar.tsxsrc/pages/loans/loans-view/LoansView.tsx
ELVIS-KATO
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components/custom/sidebar/AppSidebar.tsx (3)
76-76: Consider extracting theALL_FUNCTIONSconstant.A past review flagged an inconsistency between
'ALL_FUNCTIONS'(here) and'ALL_PERMISSIONS'(in LoansView). That inconsistency has been fixed—both files now use'ALL_FUNCTIONS'.However, the recommended fix of creating a shared constant was not implemented. Extracting this to a central permissions module would prevent future drift and make it easier to update if the backend value changes.
Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` at line 76, Extract the magic string 'ALL_FUNCTIONS' into a shared exported constant (e.g., export const ALL_FUNCTIONS = 'ALL_FUNCTIONS') in a central permissions module and replace all literal uses with that constant; update the check in AppSidebar (the expression using item.requiredPermission and permissions.includes('ALL_FUNCTIONS')) to use the imported ALL_FUNCTIONS constant, and do the same in LoansView and any other file that previously used the string literal so all comparisons use the single source-of-truth constant.
100-102: Missing aria-labels on icon-only buttons.The Settings and Logout buttons lack
aria-labelattributes, unlike the regular menu items which correctly havearia-label={item.label}(line 86). Screen reader users cannot identify these buttons.♿ Proposed fix for accessibility
- <SidebarMenuButton className="w-12 h-12 flex items-center justify-center rounded-xl" onClick={() => navigate('/settings')}> + <SidebarMenuButton className="w-12 h-12 flex items-center justify-center rounded-xl" onClick={() => navigate('/settings')} aria-label="Settings"> <Cog size={24} /> </SidebarMenuButton><SidebarMenuButton className="w-12 h-12 flex items-center justify-center text-gray-400 hover:text-red-500 transition-colors" onClick={handleLogout} + aria-label="Logout" >Also applies to: 111-116
🤖 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 100 - 102, The icon-only SidebarMenuButton instances (the settings and logout buttons) are missing aria-labels—add an aria-label prop to each SidebarMenuButton (e.g., aria-label="Settings" for the button wrapping <Cog> and aria-label="Logout" for the logout button or use the corresponding label variable if available) so screen readers can identify them; update the SidebarMenuButton elements that call navigate('/settings') and the logout handler to include aria-label={...} consistent with the other menu items.
40-40: Unused translation hook and hardcoded strings.The
tfunction fromuseTranslationis imported but never used. All user-facing labels ('Home', 'Clients', 'Settings', 'Logout', etc.) are hardcoded English strings, which will break i18n support.Consider wrapping all visible text with
t()calls, or remove the unused import if i18n is not yet implemented.Also applies to: 91-91, 104-104, 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/sidebar/AppSidebar.tsx` at line 40, The component AppSidebar imports useTranslation and declares const { t } = useTranslation('common') but never uses t; replace all hardcoded visible strings in AppSidebar (e.g., menu labels "Home", "Clients", "Settings", "Logout" at the noted locations) with calls to t('key') using appropriate keys (for example t('sidebar.home'), t('sidebar.clients'), t('sidebar.settings'), t('sidebar.logout')) or, if i18n is not ready, remove the useTranslation import and the const { t } declaration; ensure keys map to your translation files and update any JSX elements in AppSidebar that render those labels to use t(...) instead of raw strings.src/pages/loans/loans-view/LoansView.tsx (1)
67-67: Hardcoded strings without i18n support.User-visible text (breadcrumb labels, header text, action labels) is hardcoded in English. Consider using
useTranslationfromreact-i18nextfor consistency with other components and future localization support.Also applies to: 72-73, 49-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/loans/loans-view/LoansView.tsx` at line 67, The breadcrumb and other visible strings in LoansView (e.g., AppBreadCrumbs items, header text and action labels rendered in the LoansView component) are hardcoded; import and call useTranslation from react-i18next at the top of the LoansView component, replace literals like 'Home', 'Loans', header titles and button labels with t('loans.home'), t('loans.title'), t('loans.actionName') (or similar hierarchical keys) and ensure you add corresponding entries to your i18n resource files; keep the keys descriptive and consistent with existing translation namespaces so all user-facing text in AppBreadCrumbs and the LoansView JSX uses t(...) instead of raw strings.
🤖 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/loans/loans-view/LoansView.tsx`:
- Line 67: The Home breadcrumb in the AppBreadCrumbs call inside LoansView (the
AppBreadCrumbs invocation in LoansView.tsx) points to '/home' but the app uses
'/dashboard'; update the Home item's href from '/home' to '/dashboard' so the
breadcrumb matches the sidebar routing and avoids 404s.
- Around line 59-63: The tabs array in LoansView.tsx currently defines only
General, Transactions, and Notes but omits routes that still exist in the router
(accountdetail, repayment-schedule, loan-collateral, term-variations,
loan-documents), leaving them unreachable from the UI; update the tabs constant
in LoansView.tsx to include entries for those missing routes (e.g., labels and
hrefs like `${base}/accountdetail`, `${base}/repayment-schedule`,
`${base}/loan-collateral`, `${base}/term-variations`, `${base}/loan-documents`)
so the UI exposes all child routes, or alternatively remove those orphaned child
routes from the router configuration if they are intentionally deprecated.
Ensure tabs uses the exact route names defined in the router so navigation
matches.
---
Nitpick comments:
In `@src/components/custom/sidebar/AppSidebar.tsx`:
- Line 76: Extract the magic string 'ALL_FUNCTIONS' into a shared exported
constant (e.g., export const ALL_FUNCTIONS = 'ALL_FUNCTIONS') in a central
permissions module and replace all literal uses with that constant; update the
check in AppSidebar (the expression using item.requiredPermission and
permissions.includes('ALL_FUNCTIONS')) to use the imported ALL_FUNCTIONS
constant, and do the same in LoansView and any other file that previously used
the string literal so all comparisons use the single source-of-truth constant.
- Around line 100-102: The icon-only SidebarMenuButton instances (the settings
and logout buttons) are missing aria-labels—add an aria-label prop to each
SidebarMenuButton (e.g., aria-label="Settings" for the button wrapping <Cog> and
aria-label="Logout" for the logout button or use the corresponding label
variable if available) so screen readers can identify them; update the
SidebarMenuButton elements that call navigate('/settings') and the logout
handler to include aria-label={...} consistent with the other menu items.
- Line 40: The component AppSidebar imports useTranslation and declares const {
t } = useTranslation('common') but never uses t; replace all hardcoded visible
strings in AppSidebar (e.g., menu labels "Home", "Clients", "Settings", "Logout"
at the noted locations) with calls to t('key') using appropriate keys (for
example t('sidebar.home'), t('sidebar.clients'), t('sidebar.settings'),
t('sidebar.logout')) or, if i18n is not ready, remove the useTranslation import
and the const { t } declaration; ensure keys map to your translation files and
update any JSX elements in AppSidebar that render those labels to use t(...)
instead of raw strings.
In `@src/pages/loans/loans-view/LoansView.tsx`:
- Line 67: The breadcrumb and other visible strings in LoansView (e.g.,
AppBreadCrumbs items, header text and action labels rendered in the LoansView
component) are hardcoded; import and call useTranslation from react-i18next at
the top of the LoansView component, replace literals like 'Home', 'Loans',
header titles and button labels with t('loans.home'), t('loans.title'),
t('loans.actionName') (or similar hierarchical keys) and ensure you add
corresponding entries to your i18n resource files; keep the keys descriptive and
consistent with existing translation namespaces so all user-facing text in
AppBreadCrumbs and the LoansView JSX uses t(...) instead of raw strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 194e142a-37c1-49c7-9f1e-880603132af6
📒 Files selected for processing (2)
src/components/custom/sidebar/AppSidebar.tsxsrc/pages/loans/loans-view/LoansView.tsx
Comment on lines
59
to
63
| const tabs = [ | ||
| { label: 'General', href: `${base}/general` }, | ||
| { label: 'Account Details', href: `${base}/accountdetail` }, | ||
| { label: 'Repayment Schedule', href: `${base}/repayment-schedule` }, | ||
| { label: 'Transactions', href: `${base}/transactions` }, | ||
| { label: 'Loan Collateral Details', href: `${base}/loan-collateral` }, | ||
| { label: 'Term Variations', href: `${base}/term-variations` }, | ||
| { label: 'Loan Documents', href: `${base}/loan-documents` }, | ||
| { label: 'Notes', href: `${base}/notes` }, | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify child routes defined under loans-accounts that are no longer in tabs
echo "=== Routes defined in AppRoutes.tsx under loans-accounts ==="
rg -n "path=\"(general|accountdetail|repayment-schedule|transactions|loan-collateral|term-variations|loan-documents|notes)\"" --type tsx --type ts
echo ""
echo "=== Tabs defined in LoansView.tsx ==="
rg -n "label:.*href:.*base" src/pages/loans/loans-view/LoansView.tsx -A1Repository: openMF/mifos-x-web-app-react
Length of output: 391
🏁 Script executed:
#!/bin/bash
# Find AppRoutes.tsx
echo "=== Locating AppRoutes.tsx ==="
find . -name "AppRoutes.tsx" -o -name "AppRoutes.ts"
# Check if file exists and get line count
if [ -f "src/router/AppRoutes.tsx" ]; then
echo ""
echo "=== Checking AppRoutes.tsx line count ==="
wc -l src/router/AppRoutes.tsx
fiRepository: openMF/mifos-x-web-app-react
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review comment
echo "=== AppRoutes.tsx lines 1087-1108 (claimed orphaned routes) ==="
sed -n '1087,1108p' src/router/AppRoutes.tsx
echo ""
echo "=== Broader context: lines 1080-1120 ==="
sed -n '1080,1120p' src/router/AppRoutes.tsxRepository: openMF/mifos-x-web-app-react
Length of output: 2674
🏁 Script executed:
#!/bin/bash
# Search for loans routes in AppRoutes.tsx
echo "=== Searching for loans routes ==="
grep -n "loans" src/router/AppRoutes.tsx | head -30
echo ""
echo "=== Searching for LoansView import/usage ==="
grep -n "LoansView" src/router/AppRoutes.tsxRepository: openMF/mifos-x-web-app-react
Length of output: 3440
🏁 Script executed:
#!/bin/bash
# Read the actual loans route configuration
echo "=== AppRoutes.tsx lines 1130-1160 (loans route) ==="
sed -n '1130,1160p' src/router/AppRoutes.tsxRepository: openMF/mifos-x-web-app-react
Length of output: 1116
Tabs array omits 5 routes that are still defined in the router.
The tabs in LoansView.tsx display only 3 routes (General, Transactions, Notes), but the router configuration defines 8 child routes:
accountdetailrepayment-scheduleloan-collateralterm-variationsloan-documents
These routes remain accessible via direct URL but are unreachable from the UI. Either remove the orphaned routes from the router or restore them to the tabs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/loans/loans-view/LoansView.tsx` around lines 59 - 63, The tabs
array in LoansView.tsx currently defines only General, Transactions, and Notes
but omits routes that still exist in the router (accountdetail,
repayment-schedule, loan-collateral, term-variations, loan-documents), leaving
them unreachable from the UI; update the tabs constant in LoansView.tsx to
include entries for those missing routes (e.g., labels and hrefs like
`${base}/accountdetail`, `${base}/repayment-schedule`,
`${base}/loan-collateral`, `${base}/term-variations`, `${base}/loan-documents`)
so the UI exposes all child routes, or alternatively remove those orphaned child
routes from the router configuration if they are intentionally deprecated.
Ensure tabs uses the exact route names defined in the router so navigation
matches.
| /> | ||
| return ( | ||
| <div className="p-6"> | ||
| <AppBreadCrumbs items={[{ label: 'Home', href: '/home' }, { label: 'Loans', href: '#' }]} /> |
There was a problem hiding this comment.
Breadcrumb 'Home' links to /home but the dashboard route is /dashboard.
The sidebar (AppSidebar.tsx) navigates to /dashboard for the Home item, but this breadcrumb links to /home. If /home is not a valid route, users will see a 404 or blank page when clicking this breadcrumb.
🐛 Proposed fix
- <AppBreadCrumbs items={[{ label: 'Home', href: '/home' }, { label: 'Loans', href: '#' }]} />
+ <AppBreadCrumbs items={[{ label: 'Home', href: '/dashboard' }, { label: 'Loans', href: '#' }]} />📝 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
| <AppBreadCrumbs items={[{ label: 'Home', href: '/home' }, { label: 'Loans', href: '#' }]} /> | |
| <AppBreadCrumbs items={[{ label: 'Home', href: '/dashboard' }, { label: 'Loans', href: '#' }]} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/loans/loans-view/LoansView.tsx` at line 67, The Home breadcrumb in
the AppBreadCrumbs call inside LoansView (the AppBreadCrumbs invocation in
LoansView.tsx) points to '/home' but the app uses '/dashboard'; update the Home
item's href from '/home' to '/dashboard' so the breadcrumb matches the sidebar
routing and avoids 404s.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/pages/loans/loans-view/LoansView.tsx (2)
49-67:⚠️ Potential issue | 🟡 MinorGuard route params before constructing
baselinks.Line 49 can still build
/groups/undefined/loans-accounts/undefinedand propagate broken hrefs into tabs/actions when params are missing. This was raised earlier and is still present.🔧 Suggested fix
+ if (!groupId || !loanId) { + return <div className="p-6">Invalid loan route.</div> + } const base = `/groups/${groupId}/loans-accounts/${loanId}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/loans/loans-view/LoansView.tsx` around lines 49 - 67, The code builds the `base` path and uses it in `actionsBuild` and `tabs` without validating `groupId` and `loanId`, which can produce `/groups/undefined/...`; modify the logic in the `base`/link construction to guard the route params (groupId and loanId) before using them: if either param is missing return safe fallbacks (e.g., empty actions from `actionsBuild` or tabs with disabled/placeholder hrefs) or early-return UI state until params exist; update `actionsBuild`, `tabs`, and any usage of `base` to reference these guarded values so no link is constructed when `groupId` or `loanId` is falsy.
71-71:⚠️ Potential issue | 🟡 MinorHome breadcrumb still targets
/homeinstead of/dashboard.Line 71 is still inconsistent with the app’s dashboard route and can send users to a non-canonical destination. This was flagged previously.
🔧 Suggested fix
- <AppBreadCrumbs items={[{ label: 'Home', href: '/home' }, { label: 'Loans', href: '#' }]} /> + <AppBreadCrumbs items={[{ label: 'Home', href: '/dashboard' }, { label: 'Loans', href: '#' }]} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/loans/loans-view/LoansView.tsx` at line 71, Update the breadcrumb item in the LoansView component so the Home breadcrumb points to the canonical dashboard route: locate the AppBreadCrumbs usage in LoansView (AppBreadCrumbs items prop) and change the first item's href from '/home' to '/dashboard' so the items array becomes [{ label: 'Home', href: '/dashboard' }, { label: 'Loans', href: '#' }].
🤖 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/loans/loans-view/LoansView.tsx`:
- Around line 39-42: The code reads user?.permissions and falls back to [] but
TypeScript still sees it as unknown and calling .includes may throw at runtime;
update the permissions normalization in LoansView by checking that
user?.permissions is an array (e.g., via Array.isArray) and coercing/typing it
to string[] before computing hasAll so that useAppSelector, user, permissions
and hasAll operate on a safe array of strings; if not an array, set permissions
to an empty string[].
---
Duplicate comments:
In `@src/pages/loans/loans-view/LoansView.tsx`:
- Around line 49-67: The code builds the `base` path and uses it in
`actionsBuild` and `tabs` without validating `groupId` and `loanId`, which can
produce `/groups/undefined/...`; modify the logic in the `base`/link
construction to guard the route params (groupId and loanId) before using them:
if either param is missing return safe fallbacks (e.g., empty actions from
`actionsBuild` or tabs with disabled/placeholder hrefs) or early-return UI state
until params exist; update `actionsBuild`, `tabs`, and any usage of `base` to
reference these guarded values so no link is constructed when `groupId` or
`loanId` is falsy.
- Line 71: Update the breadcrumb item in the LoansView component so the Home
breadcrumb points to the canonical dashboard route: locate the AppBreadCrumbs
usage in LoansView (AppBreadCrumbs items prop) and change the first item's href
from '/home' to '/dashboard' so the items array becomes [{ label: 'Home', href:
'/dashboard' }, { label: 'Loans', href: '#' }].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b3d9c3a-f0ee-410a-825e-041363c44d3a
📒 Files selected for processing (1)
src/pages/loans/loans-view/LoansView.tsx
Comment on lines
+39
to
+42
| const { user } = useAppSelector((state) => state.auth) | ||
| const permissions = user?.permissions || [] | ||
| const hasAll = permissions.includes('ALL_FUNCTIONS') | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "LoansView.tsx" --exec head -50 {}Repository: openMF/mifos-x-web-app-react
Length of output: 1753
🏁 Script executed:
fd -t f "auth" --exec echo {} \; | grep -E "(slice|reducer|state|type)" | head -20Repository: openMF/mifos-x-web-app-react
Length of output: 54
🏁 Script executed:
rg -l "useAppSelector.*auth" -t tsx -t ts --max-count 10Repository: openMF/mifos-x-web-app-react
Length of output: 99
🏁 Script executed:
fd -t f "auth" | grep -E "\.(ts|tsx)$" | head -20Repository: openMF/mifos-x-web-app-react
Length of output: 54
🏁 Script executed:
rg "state\.auth" -t ts -A 3 | head -40Repository: openMF/mifos-x-web-app-react
Length of output: 1229
🏁 Script executed:
fd -t f -path "*redux*auth*" -o -path "*store*auth*"Repository: openMF/mifos-x-web-app-react
Length of output: 240
🏁 Script executed:
find . -type f \( -name "*auth*" -o -name "*slice*" \) | grep -E "\.(ts|tsx)$" | head -20Repository: openMF/mifos-x-web-app-react
Length of output: 54
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "authSlice\|auth.*Slice\|createSlice.*auth" 2>/dev/null | head -10Repository: openMF/mifos-x-web-app-react
Length of output: 122
🏁 Script executed:
rg "permissions.*:" -B 5 -A 5 -t ts | head -60Repository: openMF/mifos-x-web-app-react
Length of output: 54
🏁 Script executed:
rg "interface.*User|type.*User" -t ts -A 10 | head -80Repository: openMF/mifos-x-web-app-react
Length of output: 3051
🏁 Script executed:
cat -n src/pages/login/loginSlice.tsRepository: openMF/mifos-x-web-app-react
Length of output: 2491
🏁 Script executed:
cat -n src/app/store.tsRepository: openMF/mifos-x-web-app-react
Length of output: 742
Normalize permissions before using .includes().
The auth state defines user as Record<string, unknown> | null, making user?.permissions an unknown type. The fallback || [] doesn't narrow this to an array for TypeScript, so calling .includes() on an untyped value is unsafe. At runtime, if permissions is not an array, this will throw an error.
🔧 Suggested fix
-const { user } = useAppSelector((state) => state.auth)
-const permissions = user?.permissions || []
-const hasAll = permissions.includes('ALL_FUNCTIONS')
+const { user } = useAppSelector((state) => state.auth)
+const rawPermissions = (user as { permissions?: unknown } | null)?.permissions
+const permissions: string[] = Array.isArray(rawPermissions)
+ ? rawPermissions.filter((p): p is string => typeof p === 'string')
+ : []
+const hasAll = permissions.includes('ALL_FUNCTIONS')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/loans/loans-view/LoansView.tsx` around lines 39 - 42, The code
reads user?.permissions and falls back to [] but TypeScript still sees it as
unknown and calling .includes may throw at runtime; update the permissions
normalization in LoansView by checking that user?.permissions is an array (e.g.,
via Array.isArray) and coercing/typing it to string[] before computing hasAll so
that useAppSelector, user, permissions and hasAll operate on a safe array of
strings; if not an array, set permissions to an empty string[].
gkbishnoi07
requested changes
Mar 20, 2026
Collaborator
gkbishnoi07
left a comment
gkbishnoi07
left a comment
There was a problem hiding this comment.
your one workflow is failing please fix it
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
3 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
Made changes whereby the Side Bar Menu Icons are visible to the user basing on their roles and Privileges or Permissions
Related issues and discussion
Then if a user has no permissions? The .filter() method in the Sidebar will find no matches and render an empty list.
#{Issue Number}
MXWAR-70
Screenshots, if any
These are some of the fixes made :
This before:
The After the changes
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