MXWAR-88: Fix users create/edit flow and breadcrumb#116
Merged
IOhacker merged 1 commit intoMay 13, 2026
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThree user management pages (Create, Edit, View) are updated with typed Axios error handling, controlled form components, and submission handlers for create, edit, and delete operations. Each page adds a consistent error helper to derive user-facing messages from Fineract error responses and integrates form validation and API calls with proper navigation and error alerting. ChangesUser Management Form Submission & Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/users/ViewUsers.tsx (2)
132-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix copy-paste error in delete confirmation dialog.
The dialog description says "Are you sure you want to delete gl account" but should say "user" instead.
📝 Correct the dialog text
<AlertDialogDescription> - Are you sure you want to delete gl account + Are you sure you want to delete this user? </AlertDialogDescription>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/ViewUsers.tsx` at line 132, In ViewUsers.tsx update the delete confirmation dialog copy: replace the incorrect string "Are you sure you want to delete gl account" with "Are you sure you want to delete this user" (or "user" per UX) inside the JSX where the confirmation dialog is rendered (look for the exact string or the confirmation dialog component in the ViewUsers component, e.g., the delete confirmation text used with the dialog/Modal or functions like handleDelete/confirmDelete). Ensure only the displayed label/text is changed; do not alter dialog logic or handlers.
150-173:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement or remove the "Change Password" dialog.
The "Change Password" button opens a dialog (lines 150-173) with a hardcoded title "Delete" and description "Are you sure you want to delete gl account," and the confirm action does nothing. This appears to be placeholder code copied from the delete dialog.
Either implement the password change functionality or remove this button until it's ready.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/ViewUsers.tsx` around lines 150 - 173, The Change Password dialog is a placeholder copied from the delete dialog: the Button with the FontAwesomeIcon (faGear) labeled "Change Password" opens an AlertDialog whose AlertDialogTitle and AlertDialogDescription are wrong and the AlertDialogAction has no handler; either implement the password-change flow or remove the dialog/button. To fix, either (A) implement a proper change-password modal: replace AlertDialogTitle/AlertDialogDescription with appropriate text, swap to a form component that collects current/new password fields, wire AlertDialogAction to call your changePassword handler (or userService.updatePassword) and handle validation/errors, or (B) remove the AlertDialog/AlertDialogTrigger/Button block (the Button with faGear and label "Change Password") until the feature is implemented. Ensure you update references to AlertDialogAction, AlertDialogTitle, and AlertDialogDescription accordingly.
🧹 Nitpick comments (6)
src/pages/users/EditUsers.tsx (3)
237-242: Use consistent boolean conversion pattern.This file uses
Boolean(value)at line 240, whileCreateUsers.tsxusesvalue === truefor the same checkbox logic. Pick one pattern and apply it consistently across the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/EditUsers.tsx` around lines 237 - 242, The checkbox onCheckedChange handler in EditUsers.tsx uses Boolean(value) to set passwordNeverExpiers; make this consistent with CreateUsers.tsx by using the same boolean conversion pattern (value === true). Update the onCheckedChange callback that calls setFormData({... , passwordNeverExpiers: Boolean(value) }) to use passwordNeverExpiers: value === true so the checkbox coercion matches the pattern used elsewhere.
176-182: 💤 Low valueRemove unnecessary onChange handler from disabled username input.
The username input is marked
disabledat line 176, so theonChangehandler at lines 177-182 will never fire. This code is redundant and can be removed for clarity.♻️ Remove unused onChange
<Input placeholder="Enter username" className="w-full" value={formData.username} disabled - onChange={e => - setFormData(prev => ({ - ...prev, - username: e.target.value, - })) - } />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/EditUsers.tsx` around lines 176 - 182, The username input in EditUsers.tsx is marked disabled but still includes an onChange that calls setFormData to update username; remove the entire onChange handler for that input (the function that references setFormData and e.target.value for username) so the disabled field has no redundant event handler and the code is clearer.
33-54: ⚡ Quick winConsider adding a type guard for safer error handling.
The code casts
errortoAxiosError<UserApiErrorResponse>without verifying it is actually an Axios error. If a non-Axios error is thrown, the type assertion could lead to runtime issues.♻️ Add type guard
+import axios, { type AxiosError } from 'axios' -import type { AxiosError } from 'axios' const getUserErrorMessage = (error: unknown) => { + if (!axios.isAxiosError(error)) { + return error instanceof Error ? error.message : 'Failed to update user' + } + const axiosError = error as AxiosError<UserApiErrorResponse> const responseData = axiosError.response?.data return ( responseData?.errors?.[0]?.defaultUserMessage || responseData?.errors?.[0]?.developerMessage || responseData?.defaultUserMessage || responseData?.developerMessage || axiosError.message || 'Failed to update user' ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/EditUsers.tsx` around lines 33 - 54, The getUserErrorMessage function currently asserts error as AxiosError<UserApiErrorResponse> without checking its shape; add a type guard (e.g., isAxiosError) that verifies the error has the AxiosError characteristics (presence of isAxiosError flag or response object) and only then treat it as AxiosError<UserApiErrorResponse>; if the guard fails, fall back to using String(error) or a generic message. Update getUserErrorMessage to call the guard before accessing axiosError.response?.data and preserve the existing priority of messages (errors[0].defaultUserMessage → developerMessage → defaultUserMessage → developerMessage → axiosError.message → fallback).src/pages/users/CreateUsers.tsx (2)
32-53: ⚡ Quick winConsider adding a type guard for safer error handling.
The code casts
errortoAxiosError<UserApiErrorResponse>without verifying it is actually an Axios error. If a non-Axios error is thrown (e.g., a network failure or a programming error), the type assertion could lead to runtime issues.♻️ Add type guard
+import axios, { type AxiosError } from 'axios' -import type { AxiosError } from 'axios' const getUserErrorMessage = (error: unknown) => { + if (!axios.isAxiosError(error)) { + return error instanceof Error ? error.message : 'Failed to create user' + } + - const axiosError = error as AxiosError<UserApiErrorResponse> + const axiosError = error as AxiosError<UserApiErrorResponse> const responseData = axiosError.response?.data return ( responseData?.errors?.[0]?.defaultUserMessage || responseData?.errors?.[0]?.developerMessage || responseData?.defaultUserMessage || responseData?.developerMessage || axiosError.message || 'Failed to create user' ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/CreateUsers.tsx` around lines 32 - 53, The getUserErrorMessage function currently asserts error as AxiosError<UserApiErrorResponse> without checking its shape; add a type guard (e.g., isAxiosError or similar) that verifies error has the AxiosError properties (like isAxiosError boolean or response object) and use that guard inside getUserErrorMessage to safely access axiosError.response?.data; fall back to generic error handling (use error instanceof Error ? error.message : 'Failed to create user') when the guard fails. Ensure the guard and its usage reference getUserErrorMessage and the UserApiErrorResponse shape so non-Axios errors don't cause runtime property access issues.
156-163: Replace alert() with a toast notification for better UX.Using
alert()blocks the UI and provides a poor user experience. Consider using a toast notification library that matches the design system.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/CreateUsers.tsx` around lines 156 - 163, The current try/catch in CreateUsers.tsx uses blocking alert() calls; replace them with non-blocking toast notifications from your app's notification system (e.g., react-toastify or the design-system toast). In the success branch after await userApi.create15(payload) call toast.success('User created successfully!') and then call navigate('/appusers'); in the catch branch call toast.error(getUserErrorMessage(err)) (or the design-system equivalent) and keep the console.error('Failed to create user', err) for debugging. Also add the necessary import for the toast utility and ensure any toast provider/wrapper is mounted in the app.src/pages/users/ViewUsers.tsx (1)
43-64: ⚡ Quick winConsider adding a type guard for safer error handling.
The code casts
errortoAxiosError<UserApiErrorResponse>without verifying it is actually an Axios error. If a non-Axios error is thrown, the type assertion could lead to runtime issues.♻️ Add type guard
+import axios, { type AxiosError } from 'axios' -import type { AxiosError } from 'axios' const getUserErrorMessage = (error: unknown) => { + if (!axios.isAxiosError(error)) { + return error instanceof Error ? error.message : 'Failed to delete user' + } + const axiosError = error as AxiosError<UserApiErrorResponse> const responseData = axiosError.response?.data return ( responseData?.errors?.[0]?.defaultUserMessage || responseData?.errors?.[0]?.developerMessage || responseData?.defaultUserMessage || responseData?.developerMessage || axiosError.message || 'Failed to delete user' ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/users/ViewUsers.tsx` around lines 43 - 64, The function getUserErrorMessage blindly casts error to AxiosError<UserApiErrorResponse>; add a type guard isAxiosError(error): error is AxiosError to detect Axios errors first, then only extract responseData from axiosError.response?.data when isAxiosError(error) returns true; otherwise fallback to using String(error) or a generic 'Failed to delete user' message. Update getUserErrorMessage to call isAxiosError and reference the UserApiErrorResponse and AxiosError types when extracting defaultUserMessage/developerMessage to avoid unsafe assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/users/CreateUsers.tsx`:
- Around line 147-149: The current mapping that sets officeId, roles and staffId
uses Number(...) directly which can yield NaN for empty or malformed strings;
before converting in CreateUsers (where officeId, roles and staffId are
assigned) validate the source strings (formData.office, formData.roles,
formData.staff) using an explicit parseInt(value, 10) and check isNaN — if
invalid, handle by returning/throwing a validation error or set undefined as
appropriate so the API doesn't receive NaN; ensure roles remains an array of
numbers only when parseInt succeeds and preserve existing truthiness checks
around form submission (the block that constructs the payload in
CreateUsers.tsx).
In `@src/pages/users/EditUsers.tsx`:
- Line 143: The payload currently includes sendPasswordToEmail:
formData.sendPasswordToEmail in EditUsers.tsx but there is no UI control to
change it; either remove sendPasswordToEmail from the update payload where you
build the payload from formData (so you stop sending the implicit default), or
add a checkbox/input to the EditUsers form bound to formData.sendPasswordToEmail
(initialize it from the existing state at line 66), wire it into form state
handlers, validate it, and only include sendPasswordToEmail in the payload when
the user can actually set it.
- Around line 140-142: The code currently converts formData.office,
formData.roles, and formData.staff to numbers directly (officeId:
Number(formData.office), roles: [Number(formData.roles)], staffId:
formData.staff ? Number(formData.staff) : undefined) which can yield NaN; add
validation before conversion by verifying each ID string is a valid integer
(e.g., /^\d+$/ or parseInt + !isNaN) or use a small helper like isValidId(id)
and only call Number()/parseInt when valid, otherwise set the field to undefined
or omit it; update the construction site (officeId, roles array, staffId) to
perform this check so malformed/empty strings don't produce NaN sent to the API.
---
Outside diff comments:
In `@src/pages/users/ViewUsers.tsx`:
- Line 132: In ViewUsers.tsx update the delete confirmation dialog copy: replace
the incorrect string "Are you sure you want to delete gl account" with "Are you
sure you want to delete this user" (or "user" per UX) inside the JSX where the
confirmation dialog is rendered (look for the exact string or the confirmation
dialog component in the ViewUsers component, e.g., the delete confirmation text
used with the dialog/Modal or functions like handleDelete/confirmDelete). Ensure
only the displayed label/text is changed; do not alter dialog logic or handlers.
- Around line 150-173: The Change Password dialog is a placeholder copied from
the delete dialog: the Button with the FontAwesomeIcon (faGear) labeled "Change
Password" opens an AlertDialog whose AlertDialogTitle and AlertDialogDescription
are wrong and the AlertDialogAction has no handler; either implement the
password-change flow or remove the dialog/button. To fix, either (A) implement a
proper change-password modal: replace AlertDialogTitle/AlertDialogDescription
with appropriate text, swap to a form component that collects current/new
password fields, wire AlertDialogAction to call your changePassword handler (or
userService.updatePassword) and handle validation/errors, or (B) remove the
AlertDialog/AlertDialogTrigger/Button block (the Button with faGear and label
"Change Password") until the feature is implemented. Ensure you update
references to AlertDialogAction, AlertDialogTitle, and AlertDialogDescription
accordingly.
---
Nitpick comments:
In `@src/pages/users/CreateUsers.tsx`:
- Around line 32-53: The getUserErrorMessage function currently asserts error as
AxiosError<UserApiErrorResponse> without checking its shape; add a type guard
(e.g., isAxiosError or similar) that verifies error has the AxiosError
properties (like isAxiosError boolean or response object) and use that guard
inside getUserErrorMessage to safely access axiosError.response?.data; fall back
to generic error handling (use error instanceof Error ? error.message : 'Failed
to create user') when the guard fails. Ensure the guard and its usage reference
getUserErrorMessage and the UserApiErrorResponse shape so non-Axios errors don't
cause runtime property access issues.
- Around line 156-163: The current try/catch in CreateUsers.tsx uses blocking
alert() calls; replace them with non-blocking toast notifications from your
app's notification system (e.g., react-toastify or the design-system toast). In
the success branch after await userApi.create15(payload) call
toast.success('User created successfully!') and then call navigate('/appusers');
in the catch branch call toast.error(getUserErrorMessage(err)) (or the
design-system equivalent) and keep the console.error('Failed to create user',
err) for debugging. Also add the necessary import for the toast utility and
ensure any toast provider/wrapper is mounted in the app.
In `@src/pages/users/EditUsers.tsx`:
- Around line 237-242: The checkbox onCheckedChange handler in EditUsers.tsx
uses Boolean(value) to set passwordNeverExpiers; make this consistent with
CreateUsers.tsx by using the same boolean conversion pattern (value === true).
Update the onCheckedChange callback that calls setFormData({... ,
passwordNeverExpiers: Boolean(value) }) to use passwordNeverExpiers: value ===
true so the checkbox coercion matches the pattern used elsewhere.
- Around line 176-182: The username input in EditUsers.tsx is marked disabled
but still includes an onChange that calls setFormData to update username; remove
the entire onChange handler for that input (the function that references
setFormData and e.target.value for username) so the disabled field has no
redundant event handler and the code is clearer.
- Around line 33-54: The getUserErrorMessage function currently asserts error as
AxiosError<UserApiErrorResponse> without checking its shape; add a type guard
(e.g., isAxiosError) that verifies the error has the AxiosError characteristics
(presence of isAxiosError flag or response object) and only then treat it as
AxiosError<UserApiErrorResponse>; if the guard fails, fall back to using
String(error) or a generic message. Update getUserErrorMessage to call the guard
before accessing axiosError.response?.data and preserve the existing priority of
messages (errors[0].defaultUserMessage → developerMessage → defaultUserMessage →
developerMessage → axiosError.message → fallback).
In `@src/pages/users/ViewUsers.tsx`:
- Around line 43-64: The function getUserErrorMessage blindly casts error to
AxiosError<UserApiErrorResponse>; add a type guard isAxiosError(error): error is
AxiosError to detect Axios errors first, then only extract responseData from
axiosError.response?.data when isAxiosError(error) returns true; otherwise
fallback to using String(error) or a generic 'Failed to delete user' message.
Update getUserErrorMessage to call isAxiosError and reference the
UserApiErrorResponse and AxiosError types when extracting
defaultUserMessage/developerMessage to avoid unsafe assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be281060-4cdd-4bcd-8738-acf68818d703
📒 Files selected for processing (3)
src/pages/users/CreateUsers.tsxsrc/pages/users/EditUsers.tsxsrc/pages/users/ViewUsers.tsx
There was a problem hiding this comment.
Pull request overview
This PR fixes the Users module’s create/edit/delete flows by wiring the correct API calls, improving backend error surfacing, and correcting navigation (including the Users breadcrumb) so users are returned to the /appusers list after successful actions.
Changes:
- Implemented proper submit handlers for Create User and Edit User, including payload shaping and backend validation error messaging.
- Wired delete action in View User and updated breadcrumb link to
/appusers. - Disabled username editing in Edit User to match backend limitations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/pages/users/ViewUsers.tsx | Adds delete integration + improved delete error messaging; fixes Users breadcrumb link. |
| src/pages/users/EditUsers.tsx | Fixes edit validation/submission via update API call; improves error messaging; disables username editing. |
| src/pages/users/CreateUsers.tsx | Fixes create validation/submission via create API call; improves error messaging; redirects on success. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Flashl3opard
force-pushed
the
MXWAR-88-users-create-edit-fixes
branch
from
c71d498 to
ff91ac8
Compare
Contributor
Author
|
Addressed all CoderRabbitai + Copilot feedback |
Contributor
|
LGTM |
IOhacker
approved these changes
May 13, 2026
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
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.
Related Jira issue: MXWAR-88
Description
Fix Users module create/edit flows so they submit correctly and return to the users list after success. Also wire delete action and improve error messaging so backend validation errors are visible. Updated the Users breadcrumb link to avoid 404s. No new dependencies added.
Changes Included:
/appusers./appusersto prevent 404 errors.Screenshots, if any
Before
After
Attached Video
web-app-react-userflow.mp4
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
Improvements