refactor: remove meteor calls from spotlight and browseChannels (server) #35836
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 |
|
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| if (!user) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
if (!user) {
throw new Error('User is not authenticated');
}The browseChannelsMethod function returns undefined for invalid inputs or unauthenticated users, which is not explicit and can hinder debugging.
This issue appears in multiple locations:
- apps/meteor/server/methods/browseChannels.ts: Lines 370-372
- apps/meteor/server/methods/browseChannels.ts: Lines 344-349
Please throw descriptive errors instead of returningundefinedto improve error handling and debugging.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (text.startsWith('#')) { | ||
| type.users = false; | ||
| text = text.slice(1); | ||
| } | ||
|
|
||
| if (text.startsWith('@')) { | ||
| type.rooms = false; | ||
| text = text.slice(1); | ||
| } | ||
| if (text.startsWith('@')) { | ||
| type.rooms = false; | ||
| text = text.slice(1); | ||
| } |
There was a problem hiding this comment.
const modifiedType = { ...type };
if (text.startsWith('#')) {
modifiedType.users = false;
text = text.slice(1);
}
if (text.startsWith('@')) {
modifiedType.rooms = false;
text = text.slice(1);
}
return {
users: modifiedType.users ? await spotlight.searchUsers({ userId, rid, text, usernames, mentions }) : [],
rooms: modifiedType.rooms ? await spotlight.searchRooms({ userId, text, includeFederatedRooms }) : []
};The spotlightMethod function directly modifies the type object passed as an argument, which can lead to unintended side effects.
This issue appears in multiple locations:
- apps/meteor/server/publications/spotlight.ts: Lines 47-55
- apps/meteor/server/publications/spotlight.ts: Lines 57-60
Please create a copy of thetypeobject before modifying it to prevent unintended side effects.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const result = await spotlightMethod(this.userId, query); | ||
|
|
||
| return API.v1.success(result); |
There was a problem hiding this comment.
try {
const result = await spotlightMethod(this.userId, query);
return API.v1.success(result);
} catch (error) {
SystemLogger.error({ msg: 'Error executing spotlightMethod', error });
return API.v1.failure('An error occurred while processing your request.');
}Consider adding error handling for the 'spotlightMethod' call to manage potential exceptions that might occur during its execution. This will ensure that any unexpected errors are caught and handled gracefully, providing a better user experience and more robust API behavior.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #35836 +/- ##
===========================================
- Coverage 61.17% 61.17% -0.01%
===========================================
Files 3005 3005
Lines 71387 71382 -5
Branches 16342 16341 -1
===========================================
- Hits 43669 43665 -4
+ Misses 24750 24749 -1
Partials 2968 2968
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
https://rocketchat.atlassian.net/browse/ARCH-1581
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This pull request refactors the Rocket.Chat server code by removing direct Meteor method calls from the 'spotlight' and 'browseChannels' functionalities. Instead, it introduces server-side method imports to enhance code modularity and maintainability. Specifically, the
browseChannelsmethod is refactored into a separate function namedbrowseChannelsMethod, with the addition of a new typeBrowseChannelsParamsfor improved type safety. Similarly, the spotlight method is extracted into a separate function, with a new type definitionSpotlightTypeintroduced. These changes aim to streamline the codebase by reducing reliance on Meteor's method call system.