fix(search): ensure DefaultProvider callback is always executed (#32966)#38650
fix(search): ensure DefaultProvider callback is always executed (#32966)#38650Yashika-code wants to merge 3 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can customize the tone of the review comments and chat replies.Configure the 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 |
Yashika-code
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/search/server/provider/DefaultProvider.ts`:
- Around line 36-73: The search method (async search(...) in DefaultProvider) is
mis-indented: its parameter list and body are at the same level as the method
declaration instead of nested inside the method block; adjust the indentation so
the parameters, opening brace, and all statements inside the async search method
are indented one level (matching the constructor's indentation style) to restore
proper class method scope and readability.
🧹 Nitpick comments (2)
apps/meteor/app/search/server/provider/DefaultProvider.ts (2)
60-61: Remove the code comment.As per coding guidelines, "Avoid code comments in the implementation" for
**/*.{ts,tsx,js}files.Proposed fix
- // If SearchProvider expects promise-based handling - return; + return;
56-58:return callback(...)in anasyncmethod returns whatever the callback returns, notvoid.Since
searchis declaredasyncreturningPromise<void>, andcallbackis typed to returnvoid, this works in practice. However, if a caller ever changes the callback signature to return something, thereturn callback(...)pattern could cause subtle issues. This is a minor style point — using separate statements (callback(...); return;) would be more explicit, though not strictly required.Optional: separate callback invocation from return
if (callback) { - return callback(null, safeResult); + callback(null, safeResult); + return; }Apply the same pattern to the error callback on lines 63–69.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/search/server/provider/DefaultProvider.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/search/server/provider/DefaultProvider.ts
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/app/search/server/provider/DefaultProvider.ts (1)
50-54: Verify thatIRawSearchResultincludesusersandroomsfields.The safe default includes
usersandrooms, butmessageSearchmay only returnmessages. IfIRawSearchResultdoesn't defineusers/rooms, this creates a type mismatch; if it does, confirm thatmessageSearchever populates them.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async search( | ||
| userId: string, | ||
| text: string, | ||
| context: { uid?: IUser['_id']; rid: IRoom['_id'] }, | ||
| payload: { searchAll?: boolean; limit?: number } = {}, | ||
| callback?: (error: Error | null, result: IRawSearchResult) => void, | ||
| ): Promise<void> { | ||
| userId: string, | ||
| text: string, | ||
| context: { uid?: IUser['_id']; rid: IRoom['_id'] }, | ||
| payload: { searchAll?: boolean; limit?: number } = {}, | ||
| callback?: (error: Error | null, result: IRawSearchResult) => void, | ||
| ): Promise<void> { | ||
| try { | ||
| const _rid = payload.searchAll ? undefined : context.rid; | ||
|
|
||
| const _limit = payload.limit || this._settings.get<number>('PageSize'); | ||
|
|
||
| const result = await messageSearch(userId, text, _rid, _limit); | ||
| if (callback && result !== false) { | ||
| return callback(null, result); | ||
|
|
||
| const safeResult: IRawSearchResult = result || { | ||
| messages: [], | ||
| users: [], | ||
| rooms: [], | ||
| }; | ||
|
|
||
| if (callback) { | ||
| return callback(null, safeResult); | ||
| } | ||
|
|
||
| // If SearchProvider expects promise-based handling | ||
| return; | ||
| } catch (error) { | ||
| if (callback) { | ||
| return callback(error as Error, { | ||
| messages: [], | ||
| users: [], | ||
| rooms: [], | ||
| }); | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Indentation: method body appears to be dedented one level from the class method scope.
The method parameters (lines 37–42) and the method body (lines 43–72) appear to be at the same indentation level as the method declaration, rather than being indented inside the method body block. Compare with the constructor (lines 14–23) where the body is indented one additional level.
#!/bin/bash
# Verify indentation of the search method body vs constructor body
fd "DefaultProvider.ts" --exec head -73 {} | cat -A | tail -40🧰 Tools
🪛 Biome (2.3.14)
[error] 73-73: expected } but instead the file ends
the file ends here
(parse)
🤖 Prompt for AI Agents
In `@apps/meteor/app/search/server/provider/DefaultProvider.ts` around lines 36 -
73, The search method (async search(...) in DefaultProvider) is mis-indented:
its parameter list and body are at the same level as the method declaration
instead of nested inside the method block; adjust the indentation so the
parameters, opening brace, and all statements inside the async search method are
indented one level (matching the constructor's indentation style) to restore
proper class method scope and readability.
2 participants
Proposed changes
Fixes an issue where DefaultProvider.search did not execute the callback when messageSearch returned a falsy value.
The update ensures:
Callback is always executed when provided.
Empty results are returned safely.
Errors are properly handled.
Search flow remains consistent and predictable.
Issue(s)
Closes #32966
Steps to test
Summary by CodeRabbit