MXWAR-83: Fix ManageGroupMembers non-functional client search, add, and remove#112
Open
adity-a34 wants to merge 1 commit into
Open
MXWAR-83: Fix ManageGroupMembers non-functional client search, add, and remove#112adity-a34 wants to merge 1 commit into
adity-a34 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughManageGroupMembers now performs real client lookups via ClientSearchV2Api.searchByText, stores results in clientsData, and uses groupsApi.activateOrGenerateCollectionSheet to associate/disassociate clients while updating local clientMembers state with duplicate protection. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Component as ManageGroupMembers
participant SearchAPI as ClientSearchV2Api
participant GroupsAPI as groupsApi
participant State as Local\ State
User->>Component: Type search query
Component->>SearchAPI: searchByText(query)
SearchAPI-->>Component: res.data.content
Component->>State: set clientsData
Component-->>User: Show suggestions
User->>Component: Select client to add
Component->>GroupsAPI: activateOrGenerateCollectionSheet("associateClients", { clients: [{ id }] })
GroupsAPI-->>Component: Success
Component->>State: add to clientMembers (prevent duplicates)
Component-->>User: Member added
User->>Component: Remove client
Component->>GroupsAPI: activateOrGenerateCollectionSheet("disassociateClients", { clients: [{ id }] })
GroupsAPI-->>Component: Success
Component->>State: remove from clientMembers
Component-->>User: Member removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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 (2)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx (2)
78-102:⚠️ Potential issue | 🟠 MajorDebounce the search to avoid an API call per keystroke.
The effect re-fires on every change to
search, so a user typing "test" triggers foursearchByTextrequests in rapid succession (visible in the PR's network panel screenshots). Beyond network noise, results may render out of order on slow connections — thecancelflag prevents stale state writes but does not abort the in-flight request, so a fast-typing user can still spam the backend. Add a small debounce (e.g., 250–300 ms) before calling the API.♻️ Suggested debounce
useEffect(() => { let cancel = false + if (!search.trim()) { + setClientsData([]) + return + } + const handle = setTimeout(() => { ;(async () => { - if (!search.trim()) { - setClientsData([]) - return - } try { const res = await clientSearchApi.searchByText({ request: { text: search }, page: 0, size: 8, }) if (!cancel) { const items = ((res.data as any)?.content ?? []) as LiteClient[] setClientsData(items) } } catch (e) { console.error('Client search failed', e) } })() + }, 300) return () => { cancel = true + clearTimeout(handle) } }, [search])🤖 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/ManageGroupMembers.tsx` around lines 78 - 102, The effect that triggers clientSearchApi.searchByText on every change to search should be debounced: add a short timeout (≈250–300ms) before calling the async function in the useEffect that references search, store the timeout id and clear it on cleanup to avoid firing for every keystroke, and replace the boolean cancel flag with an AbortController so you can pass its signal into clientSearchApi.searchByText to abort any in-flight request when the effect cleans up; ensure you only call setClientsData when the request completes successfully and was not aborted.
112-153:⚠️ Potential issue | 🟠 MajorSilent failure surface: surface add/remove errors to the user.
Both
addClientandremoveClientonlyconsole.erroron failure while still re-enabling the buttons viafinally. From the user's perspective, clicking "Add" or "Remove" on a network/permission error appears to do nothing — the same UX problem this PR is trying to fix. Surface a toast/inline error so users know the action did not persist (and consider the same for the search-failure path at Line 96).🤖 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/ManageGroupMembers.tsx` around lines 112 - 153, The addClient and removeClient handlers currently only console.error on failure, leaving the UI silent; update both addClient and removeClient to surface failures to the user by calling the app's notification/toast API or setting an inline error state (e.g., call showToast/errorNotifier or set a local error state) inside the catch blocks in addition to logging, and include a localized message using t (for example t('manageMembers.addFailed', { name: selectedClient?.displayName }) and t('manageMembers.removeFailed', { name: client.displayName })); ensure you still run setBusy(false) in finally but do not clear selectedClient or search on failed add so the user can retry, and apply the same visible error pattern to the search-failure path referenced at the search block.
🧹 Nitpick comments (2)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx (2)
105-110: Redundant (and potentially unsafe) client-side filter.
clientsDataalready comes from the server pre-filtered bysearch, andsize: 8caps the count, so the.filter(...).slice(0, 8)here is a no-op. It also callsc.displayName.toLowerCase()without a guard — if the API ever returns an entry withoutdisplayName, this throws and breaks the dropdown. Either drop the memo and renderclientsDatadirectly, or null-guarddisplayName.♻️ Simplification
- // compute what to show in the dropdown (client list) - const filteredClients = useMemo(() => { - const q = search.toLowerCase() - return clientsData - .filter(c => c.displayName.toLowerCase().includes(q)) - .slice(0, 8) - }, [clientsData, search]) + // server already returns up-to-8 matches for `search` + const filteredClients = clientsData🤖 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/ManageGroupMembers.tsx` around lines 105 - 110, The current useMemo in ManageGroupMembers creates filteredClients by re-filtering and slicing clientsData (which is already server-filtered) and calls c.displayName.toLowerCase() unsafely; remove this useMemo and render clientsData directly wherever filteredClients is used, or if you prefer to keep local filtering keep the variable name but change the implementation to null-guard displayName (e.g., treat missing displayName as empty string) and avoid re-slicing since the API already limits results; update all references to use the chosen source (clientsData or the null-guarded local variable) and remove the redundant slice/filter logic.
92-92: Drop theas anycasts (also clears the ESLint errors).ESLint is flagging all five
anycasts. For Line 92, narrow via the response generic instead of casting the wholeres.data. For Lines 118–119 and 144–145, the third-argparamscast is only needed because of the body cast — typing the body against the generated request model removes both. At minimum, preferas unknown as <ProperType>overas anyso future shape changes are caught.♻️ Example for the search response
- const items = ((res.data as any)?.content ?? []) as LiteClient[] + const items = (res.data?.content ?? []) as LiteClient[]Also applies to: 118-119, 144-145
🤖 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/ManageGroupMembers.tsx` at line 92, The code is using multiple `as any` casts; remove them by typing the HTTP responses and request bodies with the generated models: for the search that assigns `items` in ManageGroupMembers.tsx, replace the untyped `res.data` access by calling the client with a response generic like SearchResponse<LiteClient> (or the correct generated response type) so you can do `const items = res.data.content` without `as any`; for the calls referenced at lines ~118–119 and ~144–145, type the request body/params using the generated request model instead of casting the body, or if necessary use `as unknown as ProperType` as a last resort; update the API call signatures (where `res` is returned) to use the appropriate generic response/request types so the third-arg `params` and body no longer need `as any`.
🤖 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/ManageGroupMembers.tsx`:
- Around line 78-102: The effect that triggers clientSearchApi.searchByText on
every change to search should be debounced: add a short timeout (≈250–300ms)
before calling the async function in the useEffect that references search, store
the timeout id and clear it on cleanup to avoid firing for every keystroke, and
replace the boolean cancel flag with an AbortController so you can pass its
signal into clientSearchApi.searchByText to abort any in-flight request when the
effect cleans up; ensure you only call setClientsData when the request completes
successfully and was not aborted.
- Around line 112-153: The addClient and removeClient handlers currently only
console.error on failure, leaving the UI silent; update both addClient and
removeClient to surface failures to the user by calling the app's
notification/toast API or setting an inline error state (e.g., call
showToast/errorNotifier or set a local error state) inside the catch blocks in
addition to logging, and include a localized message using t (for example
t('manageMembers.addFailed', { name: selectedClient?.displayName }) and
t('manageMembers.removeFailed', { name: client.displayName })); ensure you still
run setBusy(false) in finally but do not clear selectedClient or search on
failed add so the user can retry, and apply the same visible error pattern to
the search-failure path referenced at the search block.
---
Nitpick comments:
In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx`:
- Around line 105-110: The current useMemo in ManageGroupMembers creates
filteredClients by re-filtering and slicing clientsData (which is already
server-filtered) and calls c.displayName.toLowerCase() unsafely; remove this
useMemo and render clientsData directly wherever filteredClients is used, or if
you prefer to keep local filtering keep the variable name but change the
implementation to null-guard displayName (e.g., treat missing displayName as
empty string) and avoid re-slicing since the API already limits results; update
all references to use the chosen source (clientsData or the null-guarded local
variable) and remove the redundant slice/filter logic.
- Line 92: The code is using multiple `as any` casts; remove them by typing the
HTTP responses and request bodies with the generated models: for the search that
assigns `items` in ManageGroupMembers.tsx, replace the untyped `res.data` access
by calling the client with a response generic like SearchResponse<LiteClient>
(or the correct generated response type) so you can do `const items =
res.data.content` without `as any`; for the calls referenced at lines ~118–119
and ~144–145, type the request body/params using the generated request model
instead of casting the body, or if necessary use `as unknown as ProperType` as a
last resort; update the API call signatures (where `res` is returned) to use the
appropriate generic response/request types so the third-arg `params` and body no
longer need `as any`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 522069d9-5dc8-4cec-9d55-9d1a51744ce9
📒 Files selected for processing (1)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx
gkbishnoi07
requested changes
Apr 29, 2026
Collaborator
gkbishnoi07
left a comment
gkbishnoi07
left a comment
There was a problem hiding this comment.
hey @adity-a34 Please avoid using any and use proper OpenAPI types instead
…nd remove Wire ClientSearchV2Api.searchByText for client search, and groupsApi.activateOrGenerateCollectionSheet with associateClients and disassociateClients commands to persist add and remove operations Signed-off-by: Aditya Tiwari <adityatiwari342005@gmail.com>
adity-a34
force-pushed
the
MXWAR-83-fix-manage-group-members-api
branch
from
0618f39 to
3d48d78
Compare
Author
|
Hi, @gkbishnoi07 I have updated the PR as you suggested, & build has been fixed too, |
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/ManageGroupMembers.tsx (1)
103-125:⚠️ Potential issue | 🟠 MajorDon't let existing members reach
associateClients.Current members still show up in the dropdown, and the duplicate guard runs only after the mutation. Selecting an already-associated client will still hit the backend and can turn a no-op into a 4xx.
Suggested guard
const filteredClients = useMemo(() => { const q = search.toLowerCase() + const memberIds = new Set(clientMembers.map(c => c.id)) return clientsData - .filter(c => c.displayName?.toLowerCase().includes(q)) + .filter( + c => + !memberIds.has(c.id) && + c.displayName?.toLowerCase().includes(q) + ) .slice(0, 8) - }, [clientsData, search]) + }, [clientMembers, clientsData, search]) const addClient = async () => { if (!selectedClient || !id) return + if (clientMembers.some(c => c.id === selectedClient.id)) return setBusy(true)🤖 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/ManageGroupMembers.tsx` around lines 103 - 125, filteredClients currently includes clients that are already group members so selecting an existing member still calls groupsApi.activateOrGenerateCollectionSheet with action 'associateClients'; update the code to filter out existing members before showing or attempting to add: change the useMemo that builds filteredClients to exclude any client whose id exists in clientMembers (or in the current members array), and additionally add a defensive check at the start of addClient (using selectedClient.id and clientMembers.find) to return early if the client is already in clientMembers before calling groupsApi.activateOrGenerateCollectionSheet or updating state.
🧹 Nitpick comments (1)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx (1)
76-100: Consider debouncing the live search.Every keypress currently issues a backend request. A short debounce would cut redundant traffic and make the dropdown steadier under latency.
🤖 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/ManageGroupMembers.tsx` around lines 76 - 100, The live search in the useEffect (the effect that watches search and calls clientSearchApi.searchByText) should be debounced to avoid a backend request on every keypress: introduce a debounce (e.g., 250–350ms) using a timer stored in a ref or a debounce utility and only call clientSearchApi.searchByText after the delay; ensure you clear the timer on cleanup and when search changes, preserve the existing cancel flag/race handling (currently in the effect) so you still ignore out-of-date responses, and keep updating setClientsData with the response as before.
🤖 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/ManageGroupMembers.tsx`:
- Around line 103-125: filteredClients currently includes clients that are
already group members so selecting an existing member still calls
groupsApi.activateOrGenerateCollectionSheet with action 'associateClients';
update the code to filter out existing members before showing or attempting to
add: change the useMemo that builds filteredClients to exclude any client whose
id exists in clientMembers (or in the current members array), and additionally
add a defensive check at the start of addClient (using selectedClient.id and
clientMembers.find) to return early if the client is already in clientMembers
before calling groupsApi.activateOrGenerateCollectionSheet or updating state.
---
Nitpick comments:
In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx`:
- Around line 76-100: The live search in the useEffect (the effect that watches
search and calls clientSearchApi.searchByText) should be debounced to avoid a
backend request on every keypress: introduce a debounce (e.g., 250–350ms) using
a timer stored in a ref or a debounce utility and only call
clientSearchApi.searchByText after the delay; ensure you clear the timer on
cleanup and when search changes, preserve the existing cancel flag/race handling
(currently in the effect) so you still ignore out-of-date responses, and keep
updating setClientsData with the response as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4358f0b-a0ec-4c07-8ec5-34ffa7763e2a
📒 Files selected for processing (1)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The Manage Group Members page was a complete UI shell with no backend connectivity. All three core interactions appeared to work visually but made no API calls, meaning every change was silently lost on page refresh.
Client search was hardcoded to always return an empty array, making it impossible to find or select any client. Adding a client only updated local component state due to a TODO placeholder left in place of the actual API call. Removing a client had the same problem, local state only with no persistence.
This fix wires all three interactions to the correct Fineract endpoints:
Search now calls ClientSearchV2Api.searchByText(), the same API used in Clients.tsx, returning up to 8 matching active clients as the user types.
Add client now calls groupsApi.activateOrGenerateCollectionSheet() with command=associateClients, which is the correct endpoint per
fineract.yaml for associating a client to a group.
Remove client calls the same method with command=disassociateClients to persist the removal to the backend before updating local state.
Local state is only updated after a successful API response in all three cases, so the UI stays in sync with the backend.
Related issues and discussion
#MXWAR-83
Screenshots, if any
Before: Typing in the search box fires zero XHR calls. The dropdown stays empty. The + button is permanently disabled.
After: Typing triggers a live client search. Selecting a client enables the + button. Adding and removing members persists to the backend and survives a page refresh.
Checklist
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