chore: improve spotilight search#7406
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughThe monolithic ChangesTwo-phase search refactor with stale-guard
Sequence DiagramsequenceDiagram
participant Component
participant searchLocal
participant searchRemote
participant Spotlight
Component->>Component: increment searchId
Component->>searchLocal: searchLocal({ text, filterUsers, rid })
searchLocal-->>Component: localResults
Component->>Component: isStale ? skip : setState(localResults)
Component->>searchRemote: searchRemote({ text, filterUsers, rid, localData })
searchRemote->>Spotlight: spotlight({ term, usernames... })
Spotlight-->>searchRemote: remote users/rooms
searchRemote-->>Component: mergedResults
Component->>Component: isStale ? skip : setState(mergedResults)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![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 (1)
app/views/RoomsListView/hooks/useSearch.ts (1)
94-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate in-flight requests when stopping search.
stopSearchclears state, but it does not advancesearchId. A previously started request can still passisStale()and dispatch new results after stop.Suggested patch
const stopSearch = useCallback(() => { + searchId.current += 1; dispatch({ type: 'STOP_SEARCH' }); }, []);🤖 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 `@app/views/RoomsListView/hooks/useSearch.ts` around lines 94 - 96, The stopSearch function dispatches STOP_SEARCH but does not advance the searchId, allowing previously started in-flight requests to still pass isStale() checks and update state after the search is stopped. Modify the dispatch action in stopSearch to increment the searchId along with clearing the search state, ensuring that any pending requests from before the stop will be considered stale and their results will be ignored.
🤖 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 `@app/containers/MessageComposer/hooks/useAutocomplete.ts`:
- Around line 116-135: The stale-guard mechanism using searchId and isStale() is
only applied to the @ and # autocomplete types, but the useAutocomplete hook
handles multiple types including ':', '/', '/preview', and '!'. When users
switch quickly between different autocomplete types, older requests can still
pass the isStale() check and overwrite newer autocomplete state. Move the
searchId increment and isStale() check outside the type-specific conditional so
it applies to all autocomplete types, and ensure every async operation branch
(for each type) includes isStale() checks after all awaited calls to prevent
stale results from overwriting current state.
In `@app/views/RoomsListView/hooks/useSearch.ts`:
- Around line 71-87: The search function in useSearch.ts dispatches
SET_SEARCHING but lacks error handling for the async operations searchLocal and
searchRemote. If either of these rejects, the state remains stuck in a searching
state because the final dispatch is never reached. Wrap the entire async
operation block (from searchLocal through searchRemote and dispatch calls) in a
try/catch block, and in the catch block dispatch an appropriate failure or reset
action to ensure the searching state is set back to false when errors occur,
following the coding guidelines for explicit async error handling.
---
Outside diff comments:
In `@app/views/RoomsListView/hooks/useSearch.ts`:
- Around line 94-96: The stopSearch function dispatches STOP_SEARCH but does not
advance the searchId, allowing previously started in-flight requests to still
pass isStale() checks and update state after the search is stopped. Modify the
dispatch action in stopSearch to increment the searchId along with clearing the
search state, ensuring that any pending requests from before the stop will be
considered stale and their results will be ignored.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c670c49-0fdf-41c3-9fbc-14fb4338e608
📒 Files selected for processing (7)
app/containers/MessageComposer/MessageComposer.test.tsxapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.test.tsapp/lib/methods/search.tsapp/views/NewMessageView/index.tsxapp/views/RoomsListView/hooks/useSearch.tsapp/views/SelectedUsersView/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: E2E Run Android (7) / Android Tests
- GitHub Check: E2E Run Android (12) / Android Tests
- GitHub Check: E2E Run Android (8) / Android Tests
- GitHub Check: E2E Run Android (1) / Android Tests
- GitHub Check: E2E Run Android (2) / Android Tests
- GitHub Check: E2E Run Android (10) / Android Tests
- GitHub Check: E2E Run Android (11) / Android Tests
- GitHub Check: E2E Run Android (9) / Android Tests
- GitHub Check: E2E Run Android (3) / Android Tests
- GitHub Check: E2E Run Android (13) / Android Tests
- GitHub Check: E2E Run Android (6) / Android Tests
- GitHub Check: E2E Run Android (5) / Android Tests
- GitHub Check: E2E Run Android (14) / Android Tests
- GitHub Check: E2E Run Android (4) / Android Tests
- GitHub Check: E2E Build iOS / ios-build
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/NewMessageView/index.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/SelectedUsersView/index.tsxapp/lib/methods/search.test.tsapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.tsapp/views/RoomsListView/hooks/useSearch.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/views/NewMessageView/index.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/SelectedUsersView/index.tsxapp/lib/methods/search.test.tsapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.tsapp/views/RoomsListView/hooks/useSearch.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/NewMessageView/index.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/SelectedUsersView/index.tsxapp/lib/methods/search.test.tsapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.tsapp/views/RoomsListView/hooks/useSearch.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/NewMessageView/index.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/SelectedUsersView/index.tsxapp/lib/methods/search.test.tsapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.tsapp/views/RoomsListView/hooks/useSearch.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/NewMessageView/index.tsxapp/views/SelectedUsersView/index.tsxapp/views/RoomsListView/hooks/useSearch.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/MessageComposer/MessageComposer.test.tsxapp/containers/MessageComposer/hooks/useAutocomplete.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/NewMessageView/index.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/SelectedUsersView/index.tsxapp/lib/methods/search.test.tsapp/containers/MessageComposer/hooks/useAutocomplete.tsapp/lib/methods/search.tsapp/views/RoomsListView/hooks/useSearch.ts
🔇 Additional comments (11)
app/lib/methods/search.ts (3)
100-121: LGTM!
123-193: LGTM!
195-205: LGTM!app/lib/methods/search.test.ts (5)
1-48: LGTM!
50-77: LGTM!
79-212: LGTM!
214-260: LGTM!
262-287: LGTM!app/views/NewMessageView/index.tsx (1)
3-3: LGTM!Also applies to: 18-18, 31-33, 71-84
app/views/SelectedUsersView/index.tsx (1)
3-3: LGTM!Also applies to: 25-26, 39-40, 122-135
app/containers/MessageComposer/MessageComposer.test.tsx (1)
18-18: LGTM!Also applies to: 28-30, 462-463, 547-548
…mplete/rooms results
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
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)
app/containers/MessageComposer/hooks/useAutocomplete.ts (1)
234-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard stale failures in the
catchpath before mutating state.Older async requests that fail after cleanup still execute this
catchand clear current autocomplete state. That reintroduces stale overwrite behavior through the error path.Suggested fix
} catch (e) { + if (ignore) return; log(e); setItems([]); updateAutocompleteVisible(false); }🤖 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 `@app/containers/MessageComposer/hooks/useAutocomplete.ts` around lines 234 - 238, The catch block in the autocomplete error handling path unconditionally mutates state with setItems([]) and updateAutocompleteVisible(false), allowing stale async request failures to overwrite current state after cleanup. Add a guard condition in the catch block (before calling setItems and updateAutocompleteVisible) to check if the async request is still valid and not stale, such as checking an abort signal or cleanup flag. This ensures that only failures from the current active request will update the autocomplete state, preventing stale errors from reintroducing old state.
🤖 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.
Outside diff comments:
In `@app/containers/MessageComposer/hooks/useAutocomplete.ts`:
- Around line 234-238: The catch block in the autocomplete error handling path
unconditionally mutates state with setItems([]) and
updateAutocompleteVisible(false), allowing stale async request failures to
overwrite current state after cleanup. Add a guard condition in the catch block
(before calling setItems and updateAutocompleteVisible) to check if the async
request is still valid and not stale, such as checking an abort signal or
cleanup flag. This ensures that only failures from the current active request
will update the autocomplete state, preventing stale errors from reintroducing
old state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c39c43-6d6b-43f3-82b2-5c9fbdff24d7
📒 Files selected for processing (2)
app/containers/MessageComposer/hooks/useAutocomplete.tsapp/views/RoomsListView/hooks/useSearch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/RoomsListView/hooks/useSearch.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/MessageComposer/hooks/useAutocomplete.ts
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
requested a deployment
to
android_build
GitHub Actions
Waiting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/methods/search.ts (1)
146-150: ⚡ Quick winBound the usernames exclusion list before calling spotlight.
At Line 146, remote search now always runs when
searchTextexists. Combined with uncapped local results,usernamescan become very large and hurt latency on busy workspaces. Dedupe + cap the list before sending it upstream.💡 Suggested change
- const usernames = - rid && filterUsers - ? (localData as IUserMessage[]).map(sub => sub.username as string) - : (localData as ISearchLocal[]).map(sub => sub.name); + const usernamesSource = + rid && filterUsers + ? (localData as IUserMessage[]).map(sub => sub.username) + : (localData as ISearchLocal[]).map(sub => sub.name); + const usernames = Array.from(new Set(usernamesSource.filter((value): value is string => Boolean(value)))).slice(0, 200);🤖 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 `@app/lib/methods/search.ts` around lines 146 - 150, The usernames list passed to the spotlight function call can grow unbounded and degrade performance on busy workspaces. Before passing usernames to the spotlight function in the Promise.race block, deduplicate the list to remove any duplicate entries and cap it to a reasonable maximum size (consider using a Set for deduplication and then slicing or limiting the array to a fixed upper bound). Apply these transformations to the usernames variable before it is used in the spotlight function call with searchText and the filter options.
🤖 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.
Nitpick comments:
In `@app/lib/methods/search.ts`:
- Around line 146-150: The usernames list passed to the spotlight function call
can grow unbounded and degrade performance on busy workspaces. Before passing
usernames to the spotlight function in the Promise.race block, deduplicate the
list to remove any duplicate entries and cap it to a reasonable maximum size
(consider using a Set for deduplication and then slicing or limiting the array
to a fixed upper bound). Apply these transformations to the usernames variable
before it is used in the spotlight function call with searchText and the filter
options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9645699-5ffa-432b-96b4-9b53a9317977
📒 Files selected for processing (2)
app/lib/methods/search.test.tsapp/lib/methods/search.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/search.test.tsapp/lib/methods/search.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/methods/search.test.tsapp/lib/methods/search.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/methods/search.test.tsapp/lib/methods/search.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/search.test.tsapp/lib/methods/search.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/search.test.tsapp/lib/methods/search.ts
🔇 Additional comments (2)
app/lib/methods/search.ts (1)
56-69: LGTM!app/lib/methods/search.test.ts (1)
93-100: LGTM!Also applies to: 253-260
1 participant
Proposed changes
Split search() into searchLocal (fast, DB-only) and searchRemote (spotlight merge over the local data).
Search surfaces now paint local results instantly and re-render once the backend responds, instead of blocking the UI on the network round-trip. Updated RoomsListView spotlight, message composer @/# autocomplete, New Message, and Selected Users, each with a request-id guard so a slower older search can't overwrite a newer one. Added unit tests for the local/remote/merge logic. No change to the existing local-vs-web result parity (deferred as follow-up).
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1105
How to test or reproduce
Confirm local matches appear instantly, then remote (spotlight) results fill in.
results merge in without duplicates.
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests