fix(groups): call update13 API on edit form submit#101
Open
deepthi-kolipaka wants to merge 2 commits into
Open
fix(groups): call update13 API on edit form submit#101deepthi-kolipaka wants to merge 2 commits into
deepthi-kolipaka wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughEditGroups.tsx was updated to use a typed group response, populate form fields from typed group data, adjust staff option mapping/display, validate and cast the group id, and submit updates via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Edit as EditGroups Component
participant API as groupsApi Client
participant Server as Backend
User->>Edit: submit form
Edit->>Edit: validate & cast groupId, build PutGroupsGroupIdRequest
Edit->>API: update13(groupId, payload)
API->>Server: HTTP PUT /groups/{id}
Server-->>API: 200 OK (updated group)
API-->>Edit: response data
Edit-->>User: navigate / show success
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. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
94-99:⚠️ Potential issue | 🟡 MinorHarden the submit path for invalid IDs and failed saves.
Line 94 converts
idinline; if it ever becomes non-numeric, you can sendNaNto the API. Also, on failure (Line 98), users only get a console log and no in-UI feedback.🛡️ Proposed diff
const onSubmit = async () => { if (!id) return + const groupId = Number(id) + if (!Number.isFinite(groupId)) { + console.error('Invalid group id:', id) + return + } setSaving(true) try { const payload: any = { name: name.trim(), locale: 'en', dateFormat: 'dd MMMM yyyy', } @@ - await groupsApi.update13(Number(id), payload) + await groupsApi.update13(groupId, payload) navigate(`/groups/${id}/general`) } catch (err) { console.error('Failed to update group', err) + // Consider adding toast/inline error so users know save did not persist. } finally { setSaving(false) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 94 - 99, The submit handler currently calls groupsApi.update13(Number(id), payload) and console.errors on failure; first validate that id parses to a finite number (e.g., parseInt/Number and Number.isFinite) before calling groupsApi.update13 to avoid sending NaN, and bail out with an in-UI error if invalid; second, replace the console.error in the catch with user-visible feedback (set a component error state or dispatch a toast/notification) so the user sees save failures and optionally disable the submit button while saving; locate and update the code around update13, navigate and the submit handler in EditGroups.tsx to implement these checks and UI error handling.
🧹 Nitpick comments (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
8-12: UsePutGroupsGroupIdRequestfor the submit payload instead ofany.The request type is already imported on line 11 but line 82 still uses
any. This weakens compile-time checks for the update request shape. Since you've already added the type import, apply it to the payload:- const payload: any = { + const payload: PutGroupsGroupIdRequest = { name: name.trim(), locale: 'en', dateFormat: 'dd MMMM yyyy', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 8 - 12, Replace the loose any type used for the update payload with the imported PutGroupsGroupIdRequest type: locate the submit/update handler in EditGroups (the function that builds the payload and calls GroupsApi.putGroupsGroupId or similar) and change the payload variable/parameter annotation from any to PutGroupsGroupIdRequest so the request shape is type-checked; keep using the existing imported PutGroupsGroupIdRequest symbol and ensure the object you pass conforms to that interface before calling the API.
🤖 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/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 94-99: The submit handler currently calls
groupsApi.update13(Number(id), payload) and console.errors on failure; first
validate that id parses to a finite number (e.g., parseInt/Number and
Number.isFinite) before calling groupsApi.update13 to avoid sending NaN, and
bail out with an in-UI error if invalid; second, replace the console.error in
the catch with user-visible feedback (set a component error state or dispatch a
toast/notification) so the user sees save failures and optionally disable the
submit button while saving; locate and update the code around update13, navigate
and the submit handler in EditGroups.tsx to implement these checks and UI error
handling.
---
Nitpick comments:
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 8-12: Replace the loose any type used for the update payload with
the imported PutGroupsGroupIdRequest type: locate the submit/update handler in
EditGroups (the function that builds the payload and calls
GroupsApi.putGroupsGroupId or similar) and change the payload variable/parameter
annotation from any to PutGroupsGroupIdRequest so the request shape is
type-checked; keep using the existing imported PutGroupsGroupIdRequest symbol
and ensure the object you pass conforms to that interface before calling the
API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca74f604-a219-4bdd-912b-74652db09ad7
📒 Files selected for processing (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx
gkbishnoi07
requested changes
Mar 20, 2026
Collaborator
gkbishnoi07
left a comment
gkbishnoi07
left a comment
There was a problem hiding this comment.
Please create a Jira ticket for this task, or link the existing one if it already exists.
Also, please resolve the merge conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx (1)
101-105: Consider adding user-facing error feedback.The error is logged to console but users won't be notified if the save fails. They may navigate away thinking their changes were saved. Consider adding a toast notification or error state to inform users of failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 101 - 105, The catch block in the save/update flow (currently catching errors and calling console.error in EditGroups.tsx) should surface failures to the user instead of silently logging; update the catch block inside the save handler (where setSaving(false) is in the finally) to call the app's notification/toast utility or set an error state (e.g., invoke showToast/errorToast or setError) with a clear message like "Failed to save group" plus err.message, and ensure setSaving(false) still runs in finally so the UI is re-enabled; optionally keep console.error for debugging but do not swallow the error without user-facing feedback.
🤖 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/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 71-74: The current staffOptions mapping in EditGroups.tsx uses id:
s.id ?? 0 which injects a 0 ID for missing IDs; instead filter out entries
without a valid numeric id before mapping to avoid submitting staffId: 0 to the
API. Update the logic that builds staffOptions (the group?.staffOptions mapping)
to first filter by a defined/non-null id (e.g., s.id !== undefined && s.id !==
null) and then map to { id: s.id, name: ... } so only valid staff IDs are
included; keep the existing name fallback (s.displayName ?? s.name ??
t('edit.staffFallback', { id: s.id })) but ensure you only call that when s.id
exists.
---
Nitpick comments:
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx`:
- Around line 101-105: The catch block in the save/update flow (currently
catching errors and calling console.error in EditGroups.tsx) should surface
failures to the user instead of silently logging; update the catch block inside
the save handler (where setSaving(false) is in the finally) to call the app's
notification/toast utility or set an error state (e.g., invoke
showToast/errorToast or setError) with a clear message like "Failed to save
group" plus err.message, and ensure setSaving(false) still runs in finally so
the UI is re-enabled; optionally keep console.error for debugging but do not
swallow the error without user-facing feedback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f50a69af-1e42-44d5-93e0-98a25d9cf8fe
📒 Files selected for processing (1)
src/pages/groups/groups-view/group-actions/EditGroups.tsx
Comment on lines
+71
to
+74
| const staffOptions = (group?.staffOptions ?? []).map(s => ({ | ||
| id: s.id ?? 0, | ||
| name: s.displayName ?? s.name ?? t('edit.staffFallback', { id: s.id }), | ||
| })) |
There was a problem hiding this comment.
Fallback id: 0 may cause issues if selected.
If a staff option has no id and the user selects it, submitting staffId: 0 to the API could cause unexpected behavior or validation errors. Consider filtering out staff options without valid IDs instead.
Proposed fix to filter invalid staff options
- const staffOptions = (group?.staffOptions ?? []).map(s => ({
- id: s.id ?? 0,
- name: s.displayName ?? s.name ?? t('edit.staffFallback', { id: s.id }),
- }))
+ const staffOptions = (group?.staffOptions ?? [])
+ .filter(s => s.id != null)
+ .map(s => ({
+ id: s.id!,
+ name: s.displayName ?? s.name ?? t('edit.staffFallback', { id: s.id }),
+ }))📝 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
| const staffOptions = (group?.staffOptions ?? []).map(s => ({ | |
| id: s.id ?? 0, | |
| name: s.displayName ?? s.name ?? t('edit.staffFallback', { id: s.id }), | |
| })) | |
| const staffOptions = (group?.staffOptions ?? []) | |
| .filter(s => s.id != null) | |
| .map(s => ({ | |
| id: s.id!, | |
| name: s.displayName ?? s.name ?? t('edit.staffFallback', { id: s.id }), | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/groups/groups-view/group-actions/EditGroups.tsx` around lines 71 -
74, The current staffOptions mapping in EditGroups.tsx uses id: s.id ?? 0 which
injects a 0 ID for missing IDs; instead filter out entries without a valid
numeric id before mapping to avoid submitting staffId: 0 to the API. Update the
logic that builds staffOptions (the group?.staffOptions mapping) to first filter
by a defined/non-null id (e.g., s.id !== undefined && s.id !== null) and then
map to { id: s.id, name: ... } so only valid staff IDs are included; keep the
existing name fallback (s.displayName ?? s.name ?? t('edit.staffFallback', { id:
s.id })) but ensure you only call that when s.id exists.
Contributor
|
CLA check = failed Please sign our CLA so this can be considered |
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.
Problem
The EditGroups form was collecting all user input and building the
payload correctly, but never actually calling the API to save the changes.
Clicking "Submit" would silently navigate away without persisting anything
to the backend.
Fix
Added the missing
groupsApi.update13()call in theonSubmitfunctionso that changes are now saved to the Fineract backend before navigating
back to the group detail page.
Changes
await groupsApi.update13(Number(id), payload)in EditGroups.tsxPutGroupsGroupIdRequesttype to the importSummary by CodeRabbit
Bug Fixes
Style