-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: room member search to use server data instead of local data #6938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughAdds a 500ms debounced search to RoomMembersView that triggers server-side filtered fetches, tracks request IDs to ignore stale responses, resets pagination on new filters, deduplicates paginated results into component state, and removes client-side sanitizeLikeString usage. 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 pagination/state
end
RMV->>API: fetchMembersWithNewFilter(searchFilter, requestId)
API-->>RMV: response (members, page, requestId)
alt response.requestId == latestSearchRequest
RMV->>RMV: replace members / set end & page
else stale response
RMV-->>RMV: ignore response
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing cases like search and filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| const debounceFilterChange = useDebounce((text: string) => { | ||
| const trimmedFilter = text.trim(); | ||
|
|
||
| if (!trimmedFilter) { | ||
| latestSearchRequest.current += 1; | ||
| } | ||
|
|
||
| updateState({ | ||
| filter: trimmedFilter, | ||
| page: 0, | ||
| members: [], | ||
| end: false, | ||
| isLoading: false | ||
| }); | ||
|
|
||
| if (trimmedFilter.length > 0) { | ||
| fetchMembersWithNewFilter(trimmedFilter); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the debounce implementation
find . -name "debounce.ts" -o -name "debounce.js" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Search for useDebounce hook definition
rg -n "export.*useDebounce|function useDebounce" --type ts --type tsx -B 2 -A 15Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Find the exact file path from the repo structure
git ls-files | grep -i debounceRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Examine the debounce.ts implementation
wc -l app/lib/methods/helpers/debounce.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Read the debounce.ts file
cat -n app/lib/methods/helpers/debounce.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1165
🏁 Script executed:
#!/bin/bash
# Examine RoomMembersView to see how fetchMembersWithNewFilter is defined and used
wc -l app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Read RoomMembersView around the debounceFilterChange definition (lines 171-189)
# Also need to see the context before and after
sed -n '150,210p' app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Find where fetchMembersWithNewFilter is defined
rg -n "fetchMembersWithNewFilter" app/views/RoomMembersView/index.tsx -B 5 -A 10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Read more context to see the full useDebounce call and state setup
sed -n '1,50p' app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2337
🏁 Script executed:
#!/bin/bash
# Search for state definition and useReducer/useState usage
rg -n "const state|const \[state|useReducer|useState" app/views/RoomMembersView/index.tsx | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Get more context around the debounceFilterChange definition to see if there's a deps array
sed -n '165,195p' app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Check the exact structure around the debounceFilterChange - is there a deps array?
sed -n '171,195p' app/views/RoomMembersView/index.tsx | cat -nRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 796
🌐 Web query:
use-debounce useDebouncedCallback dependency array options
💡 Result:
use-debounce’s useDebouncedCallback does not take a “dependency array” argument (older versions did; it was removed). If your debounced function needs to “depend on” props/state, wrap the function you pass in with useCallback(..., deps) so the callback updates when its dependencies change. [2] (You can also include the debounced function itself in effect deps, e.g. for flush() on unmount.) [1]
Current signature (conceptually):
const debounced = useDebouncedCallback(fn, delayMs, options?)[1]
Options (options argument): maxWait, leading, trailing (and equalityFn is for useDebounce value debouncing only, not useDebouncedCallback). [1]
Example with “deps”:
const debouncedSave = useDebouncedCallback(
useCallback(() => saveDraft(draftId, text), [draftId, text]),
500,
{ maxWait: 2000, leading: false, trailing: true }
);[1][2]
🏁 Script executed:
#!/bin/bash
# Verify if fetchMembersWithNewFilter is wrapped in useCallback or defined without it
sed -n '410,440p' app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 761
🏁 Script executed:
#!/bin/bash
# Check if there's a useCallback import and usage pattern elsewhere in the file
rg -n "useCallback|useMemo" app/views/RoomMembersView/index.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
Stale closure in debounced callback may cause requests to use outdated state.
The useDebounce hook wraps useDebouncedCallback, which does not support a dependency array. The fetchMembersWithNewFilter function is redefined on every render and reads state (specifically room and allUsers) through closure. Since debounceFilterChange has no dependency array, it captures the first version of fetchMembersWithNewFilter and never updates this reference on subsequent renders. When state changes and a new fetchMembersWithNewFilter is created, the debounced callback still invokes the stale closure, potentially using outdated room or allUsers values.
While the request tracking via latestSearchRequest.current helps discard stale responses, the stale closure can still cause unnecessary API calls or brief race conditions during rapid state changes.
Wrap fetchMembersWithNewFilter in useCallback with proper dependencies, or move it outside the component if it doesn't depend on component state.
🤖 Prompt for AI Agents
In `@app/views/RoomMembersView/index.tsx` around lines 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.
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.