fix: room member search to use server data instead of local data #6938
Conversation
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughAdds a 500ms debounced search to RoomMembersView that resets pagination on new queries, issues server-side filtered fetches, tracks request IDs to ignore stale responses, deduplicates incoming members into state, and removes client-side sanitizeLikeString filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant SB as SearchBox
participant RMV as RoomMembersView
participant API as ServerAPI
User->>SB: type query
SB->>RMV: onChangeText (debounced 500ms)
rect rgba(200,200,255,0.5)
RMV->>RMV: latestSearchRequest++\nreset page, members, end
end
RMV->>API: fetchMembers(searchFilter, requestId)
API-->>RMV: response (members, page, requestId)
alt response.requestId == latestSearchRequest
rect rgba(200,255,200,0.5)
RMV->>RMV: deduplicate & merge members\nupdate page & end\nclear loading
end
else stale response
rect rgba(255,200,200,0.5)
RMV-->>RMV: ignore response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/views/RoomMembersView/index.tsx (2)
44-44: Refetch on cleared search + usesanitizeLikeString(fixes lint).
Line 44 currently fails lint, and when the filter is cleared the list is reset but never reloaded. Normalize the filter and always fetch so the list repopulates and the unused import is resolved.✅ Proposed fix
- const handleFilterChange = (text: string) => { - const trimmedFilter = text.trim(); + const handleFilterChange = (text: string) => { + const normalizedFilter = sanitizeLikeString(text.trim()); updateState({ - filter: trimmedFilter, + filter: normalizedFilter, page: 0, members: [], end: false }); - if (trimmedFilter.length > 0) { - fetchMembersWithNewFilter(trimmedFilter); - } + fetchMembersWithNewFilter(normalizedFilter); };Also applies to: 425-437
440-468: Usestop()instead ofcancel()for debounce cleanup, and remove the redundantfilteredMembersvariable.The debounce helper exposes a
stop()method (notcancel()), andfilteredMembersis redundant—it always equalsstate.membersbecause the conditionstate.members.length > 0 ? state.members : nullfollowed byfilteredMembers || state.membersalways evaluates tostate.members.The memoization pattern is correct to prevent recreating the debounced function per render and avoid orphaned callbacks after unmount.
♻️ Corrected refactor
-import React, { useEffect, useReducer } from 'react'; +import React, { useEffect, useReducer, useCallback, useMemo } from 'react'; -const handleFilterChange = (text: string) => { +const handleFilterChange = useCallback((text: string) => { // existing body -}; +}, [fetchMembersWithNewFilter]); -const debounceFilterChange = debounce(handleFilterChange, 500); -const filteredMembers = state.members.length > 0 ? state.members : null; +const debounceFilterChange = useMemo(() => debounce(handleFilterChange, 500), [handleFilterChange]); +useEffect(() => () => debounceFilterChange.stop?.(), [debounceFilterChange]);And update the FlatList data prop from
filteredMembers || state.membersto simplystate.members.
🤖 Fix all issues with AI agents
In `@app/views/RoomMembersView/index.tsx`:
- Around line 371-381: When filter is active the code assigns newMembers =
membersResult which replaces prior pages and breaks scrolling; update the logic
in the block that references filter, membersResult, newMembers and members so
that when filter is truthy you either (A) append membersResult to existing
members with de-duplication by _id (e.g. merge members + membersResult and
remove duplicates by _id) to preserve pagination, or (B) explicitly disable
pagination while filtered (set end = true or prevent page increments) so
filtered requests don't replace earlier pages—apply the chosen approach
consistently where PAGE_SIZE, membersResult, newMembers and members are used.
- Around line 395-418: The fetchMembersWithNewFilter function can have
out-of-order debounced responses overwrite newer results; add a request-tracking
guard (e.g., increment a requestId counter or store the latestFilter in
component state) before calling getRoomMembers and capture the current
id/filter, then when the response returns only call updateState(members...) if
the captured id/filter still matches the latest one in state; ensure
requestId/latestFilter is updated at the start of fetchMembersWithNewFilter and
used to ignore stale responses so members/end/page/isLoading updates come only
from the most recent request.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/views/RoomMembersView/index.tsx`:
- Line 447: debounceFilterChange is recreated every render which breaks
debouncing; memoize the debounced function instead of creating it in the
component body by wrapping the debounce(handleFilterChange, 500) call in useMemo
(or store it in a useRef) so the same debounced instance persists across
renders; if handleFilterChange needs stable identity or fresh state, make
handleFilterChange a useCallback with appropriate deps or keep a ref to the
latest handler and call that from the debounced function.
- Around line 432-445: handleFilterChange currently clears members and resets
state but only calls fetchMembersWithNewFilter when trimmedFilter.length > 0,
leaving the list empty when the user clears the search; modify
handleFilterChange so that when trimmedFilter is empty it also triggers the
unfiltered fetch (e.g., call fetchMembersWithNewFilter('') or a dedicated
fetchUnfilteredMembers function) after updateState, keeping the page reset and
end flag behavior intact; reference the handleFilterChange function,
trimmedFilter variable, updateState call, and fetchMembersWithNewFilter to
locate and change the logic.
♻️ Duplicate comments (1)
app/views/RoomMembersView/index.tsx (1)
374-380: Filtered pagination still drops earlier pages.When a filter is active and the user scrolls to load more,
fetchMembersis triggered viaonEndReached. The current logic replacesmemberswith just the new page instead of appending. Consider either appending with de-duplication for filtered paging, or disablingonEndReachedwhile a filter is active.🐛 Proposed fix: Append with de-dupe for all cases
- let newMembers: TUserModel[]; - if (filter) { - newMembers = membersResult || []; - } else { - const membersResultFiltered = membersResult?.filter((member: TUserModel) => !members.some(m => m._id === member._id)); - newMembers = [...members, ...membersResultFiltered]; - } + const membersResultFiltered = membersResult?.filter((member: TUserModel) => !members.some(m => m._id === member._id)); + const newMembers = [...members, ...(membersResultFiltered || [])];
🧹 Nitpick comments (1)
app/views/RoomMembersView/index.tsx (1)
448-448: RedundantfilteredMembersvariable.The expression
filteredMembers || state.membersalways evaluates tostate.members:
- If
state.members.length > 0:filteredMembers=state.members- If
state.members.length === 0:filteredMembers=null, fallback =state.members(empty array)♻️ Simplify by using state.members directly
-const filteredMembers = state.members.length > 0 ? state.members : null; ... -data={filteredMembers || state.members} +data={state.members}Also applies to: 453-453
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
requested a deployment
to
official_android_build
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/RoomMembersView/index.tsx (1)
352-386: Prevent stale pagination responses from mixing with a new search.
If a page fetch started before a filter/status change returns late, it can append unfiltered members into the filtered list becausefetchMembershas no staleness guard. Add a shared request token (or query signature ref) so only the latest request updates state.🐛 Proposed fix (shared request guard)
@@ -const latestSearchRequest = useRef(0); +const latestSearchRequest = useRef(0); +const latestMembersRequest = useRef(0); +const latestQueryRef = useRef({ filter: '', allUsers: false }); + +useEffect(() => { + latestQueryRef.current = { filter: state.filter, allUsers: state.allUsers }; +}, [state.filter, state.allUsers]); @@ const fetchMembers = async (status: boolean) => { + const requestId = ++latestMembersRequest.current; + const filterAtCall = state.filter; + const allUsersAtCall = status; const { members, isLoading, end, room, filter, page } = state; @@ try { const membersResult = await getRoomMembers({ @@ const end = membersResult?.length < PAGE_SIZE; const membersResultFiltered = membersResult?.filter((member: TUserModel) => !members.some(m => m._id === member._id)); const newMembers = [...members, ...(membersResultFiltered || [])]; + if ( + requestId !== latestMembersRequest.current || + filterAtCall !== latestQueryRef.current.filter || + allUsersAtCall !== latestQueryRef.current.allUsers + ) { + return; + } + updateState({ members: newMembers, isLoading: false, end, page: page + 1 }); } catch (e) { log(e); - updateState({ isLoading: false }); + if (requestId === latestMembersRequest.current) { + updateState({ isLoading: false }); + } } };
♻️ Duplicate comments (1)
app/views/RoomMembersView/index.tsx (1)
442-470: Debounced handler is recreated every render (debounce breaks + stale timers).
Line 442 creates a new debounce instance each render, so timeouts from prior renders can still fire and the debounce no longer coalesces rapid input. Memoize the debounced function and clean it up on unmount.🐛 Proposed fix (stable debounce + cleanup)
-import React, { useEffect, useReducer, useRef } from 'react'; +import React, { useEffect, useMemo, useReducer, useRef } from 'react'; @@ -const debounceFilterChange = debounce(handleFilterChange, 500); +const handleFilterChangeRef = useRef(handleFilterChange); +useEffect(() => { + handleFilterChangeRef.current = handleFilterChange; +}, [handleFilterChange]); + +const debounceFilterChange = useMemo( + () => debounce((text: string) => handleFilterChangeRef.current(text), 500), + [] +); +useEffect(() => () => debounceFilterChange.stop(), [debounceFilterChange]);
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/views/RoomMembersView/index.tsx`:
- Around line 171-184: The filter debounce handler (debounceFilterChange) and
fetchMembersWithNewFilter call flow can be clobbered by in-flight fetchMembers
responses because there’s no shared request-id guard; add a single shared
request identifier (e.g., currentRequestId) used by both fetchMembers and
fetchMembersWithNewFilter so every fetch captures the id before dispatch and
each response checks it and ignores stale ids, and when the filter is cleared
(trimmedFilter.length === 0) increment/invalidate the currentRequestId so any
pending filtered responses are ignored; updateState calls remain the same but
ensure all code paths that call fetchMembers or fetchMembersWithNewFilter use
that guard to prevent stale overwrites.
🧹 Nitpick comments (1)
app/views/RoomMembersView/index.tsx (1)
442-447: Simplify redundantfilteredMemberslogic.
filteredMembersis eitherstate.membersornull, anddata={filteredMembers || state.members}always resolves tostate.members. You can remove the intermediate variable.♻️ Suggested simplification
-const filteredMembers = state.members.length > 0 ? state.members : null; ... - data={filteredMembers || state.members} + data={state.members}
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
@coderabbitai review |
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/RoomMembersView/index.tsx (1)
454-459: Search becomes a no‑op on servers < 3.16 because the fallback API ignores thefilterparameter.The issue is confirmed:
getRoomMembersonly applies thefilterparameter on servers >= 3.16.0; the legacygetUsersOfRoompath doesn't accept it at all. With no client-side filtering fallback, the SearchBox won't filter results on older servers.Current filtering flow
Both
fetchMembers()andfetchMembersWithNewFilter()pass thefilterparameter togetRoomMembers(), which:
- For servers ≥ 3.16.0: Uses
{roomType}.membersAPI with the filter applied- For servers < 3.16.0: Falls back to
getUsersOfRoom()method call, passing onlyrid,allUsers, and pagination—filter is silently droppedThe FlatList then displays
state.membersdirectly with no client-side filtering.Consider implementing client-side filtering (or disabling search) when the server doesn't support the filter parameter, similar to the proposed solution. If the app's minimum supported server version is 3.16 or higher, document and enforce that requirement instead.
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.maestro/tests/room/search-member.yaml:
- Around line 35-39: The test uses a hardcoded username ("rohit.bansal") in
inputText and waits for id 'room-members-view-item-rohit.bansal', creating an
external dependency; instead, create a second test user at runtime via
output.utils.createUser() (as in unread-badge.yaml), assign the returned
username to the inputText and update the extendedWaitUntil id to use that
dynamic username (replace 'room-members-view-item-rohit.bansal' with the
generated user's identifier) so the test is self-contained and no longer depends
on an existing external user.
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
I've tested this against Prod and Web behaviors and found a failing case.
Scenario: If you search for a user and then switch the filter to 'Online', the app loads the entire list of online users, ignoring the current search term.
Expected behavior: It should apply the status filter to the current search results (matching both the name and the status).
app/views/RoomMembersView/index.tsx
Outdated
| state.members && state.members.length > 0 && state.filter | ||
| ? state.members.filter(m => m.username.toLowerCase().match(filter) || m.name?.toLowerCase().match(filter)) | ||
| : null; | ||
| const fetchMembersWithNewFilter = async (searchFilter: string) => { |
There was a problem hiding this comment.
We have two functions that do almost the same thing... can't we unify it?
Example:
const loadMembers = async ({
filterParam,
allUsersParam,
reset = false
}: {
filterParam?: string;
allUsersParam?: boolean;
reset?: boolean;
} = {}) => {
const { members, isLoading, end, room, filter, page, allUsers } = state;
// Determine values to use (param override -> state -> default)
const currentFilter = filterParam !== undefined ? filterParam : filter;
const currentAllUsers = allUsersParam !== undefined ? allUsersParam : allUsers;
// Prevent fetching if loading or reached end (unless it's a reset/new search)
if (!reset && (isLoading || end)) {
return;
}
const requestId = ++latestSearchRequest.current;
// Optimistic State Updates
if (reset) {
updateState({
isLoading: true,
members: [],
end: false,
filter: currentFilter,
allUsers: currentAllUsers,
page: 0
});
} else {
updateState({ isLoading: true });
}
try {
const membersResult = await getRoomMembers({
rid: room.rid,
roomType: room.t,
type: !currentAllUsers ? 'all' : 'online',
filter: currentFilter,
skip: reset ? 0 : PAGE_SIZE * page,
limit: PAGE_SIZE,
allUsers: !currentAllUsers
});
// Race Condition Check
if (requestId !== latestSearchRequest.current) {
return;
}
const isEnd = (membersResult?.length ?? 0) < PAGE_SIZE;
let newMembers = membersResult || [];
// If appending (not resetting), filter out duplicates
if (!reset) {
const filteredNewMembers = newMembers.filter(member => !members.some(m => m._id === member._id));
newMembers = [...members, ...filteredNewMembers];
}
updateState({
members: newMembers,
isLoading: false,
end: isEnd,
page: reset ? 1 : page + 1
});
} catch (e) {
log(e);
if (requestId === latestSearchRequest.current) {
updateState({ isLoading: false });
}
}
};
| - extendedWaitUntil: | ||
| visible: | ||
| id: 'room-members-view-item-rohit.bansal' | ||
| timeout: 60000 |
There was a problem hiding this comment.
Missing cases like search and filter.
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/views/RoomMembersView/index.tsx`:
- Around line 171-189: The debounced callback debounceFilterChange captures a
stale fetchMembersWithNewFilter because useDebounce/useDebouncedCallback has no
dependency array; fix by making fetchMembersWithNewFilter stable with
useCallback and include all values it reads (room, allUsers, any state setters
like updateState) in its dependency list so the debounced callback always calls
the current logic; ensure latestSearchRequest.current usage remains as-is to
discard responses and that debounceFilterChange continues to call the memoized
fetchMembersWithNewFilter.
🧹 Nitpick comments (1)
app/views/RoomMembersView/index.tsx (1)
416-452: Consider unifying fetch functions.Both
fetchMembersandfetchMembersWithNewFiltershare significant logic (request ID tracking, error handling, API call structure). As noted in a previous review comment, these could be consolidated into a singleloadMembersfunction with aresetparameter to reduce duplication and simplify maintenance.This is an optional refactor that can be deferred.
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
official_android_build
GitHub Actions
Waiting
Proposed changes
Search functionality in the room/group members list fails to return results for users who haven’t been loaded into the local state. Only users loaded initially or after scrolling to the bottom are searchable.
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-973
How to test or reproduce
Screenshots
iOS E2E
Android E2E
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.