Skip to content

refactor: remove meteor calls from spotlight and browseChannels (server) #35836

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.

Sign up for GitHub

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

Merged
ggazzo merged 3 commits into from
Apr 29, 2025
Merged

refactor: remove meteor calls from spotlight and browseChannels (server) #35836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
121bf3e
refactor: remove meteor calls from spotlight and browseChannels (server)
Apr 17, 2025
93b9196
Merge branch 'develop' into refactor/remove-meteor-calls-misc
Apr 22, 2025
63a33cf
fix: userId not required
Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions apps/meteor/app/api/server/v1/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

import { i18n } from '../../../../server/lib/i18n';
import { SystemLogger } from '../../../../server/lib/logger/system';
import { browseChannelsMethod } from '../../../../server/methods/browseChannels';
import { spotlightMethod } from '../../../../server/publications/spotlight';
import { resetAuditedSettingByUser, updateAuditedByUser } from '../../../../server/settings/lib/auditedSettingUpdates';
import { getLogs } from '../../../../server/stream/stdout';
import { passwordPolicy } from '../../../lib/server';
Expand Down Expand Up @@ -330,7 +332,7 @@
async get() {
const { query } = this.queryParams;

const result = await Meteor.callAsync('spotlight', query);
const result = await spotlightMethod({ text: query, userId: this.userId });

return API.v1.success(result);
},
Expand Down Expand Up @@ -362,13 +364,16 @@
const sortBy = sort ? Object.keys(sort)[0] : undefined;
const sortDirection = sort && Object.values(sort)[0] === 1 ? 'asc' : 'desc';

const result = await Meteor.callAsync('browseChannels', {
...filter,
sortBy,
sortDirection,
offset: Math.max(0, offset),
limit: Math.max(0, count),
});
const result = await browseChannelsMethod(
{
...filter,
sortBy,
sortDirection,
offset: Math.max(0, offset),
limit: Math.max(0, count),
},
this.user,
);

if (!result) {
return API.v1.failure('Please verify the parameters');
Expand Down Expand Up @@ -669,7 +674,7 @@

const auditSettingOperation = updateAuditedByUser({
_id: this.userId,
username: this.user.username!,

Check warning on line 677 in apps/meteor/app/api/server/v1/misc.ts

View workflow job for this annotation

GitHub Actions / 🔎 Code Check / Code Lint 

Forbidden non-null assertion
ip: this.requestIp,
useragent: this.request.headers.get('user-agent') || '',
});
Expand All @@ -689,7 +694,7 @@

return resetAuditedSettingByUser({
_id: this.userId,
username: this.user.username!,

Check warning on line 697 in apps/meteor/app/api/server/v1/misc.ts

View workflow job for this annotation

GitHub Actions / 🔎 Code Check / Code Lint 

Forbidden non-null assertion
ip: this.requestIp,
useragent: this.request.headers.get('user-agent') || '',
})(Settings.resetValueById, settingId);
Expand Down
101 changes: 54 additions & 47 deletions apps/meteor/server/methods/browseChannels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,24 +308,26 @@ const getUsers = async (
};
};

type BrowseChannelsParams = {
text?: string;
workspace?: string;
type?: 'channels' | 'users' | 'teams' | string;
sortBy?: 'name' | 'createdAt' | 'usersCount' | 'lastMessage' | 'usernames' | string;
sortDirection?: 'asc' | 'desc';
page?: number;
offset?: number;
limit?: number;
};

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
interface ServerMethods {
browseChannels: (params: {
text?: string;
workspace?: string;
type?: 'channels' | 'users' | 'teams';
sortBy?: 'name' | 'createdAt' | 'usersCount' | 'lastMessage' | 'usernames';
sortDirection?: 'asc' | 'desc';
page?: number;
offset?: number;
limit?: number;
}) => Promise<unknown>;
browseChannels: (params: BrowseChannelsParams) => Promise<unknown>;
}
}

Meteor.methods<ServerMethods>({
async browseChannels({
export const browseChannelsMethod = async (
{
text = '',
workspace = '',
type = 'channels',
Expand All @@ -334,50 +336,55 @@ Meteor.methods<ServerMethods>({
page = 0,
offset = 0,
limit = 10,
}) {
const searchTerm = trim(escapeRegExp(text));

if (
!['channels', 'users', 'teams'].includes(type) ||
!['asc', 'desc'].includes(sortDirection) ||
(!page && page !== 0 && !offset && offset !== 0)
) {
return;
}
}: BrowseChannelsParams,
user: IUser | undefined | null,
) => {
const searchTerm = trim(escapeRegExp(text));

const roomParams = ['channels', 'teams'].includes(type) ? ['usernames', 'lastMessage'] : [];
const userParams = type === 'users' ? ['username', 'email', 'bio'] : [];
if (
!['channels', 'users', 'teams'].includes(type) ||
!['asc', 'desc'].includes(sortDirection) ||
(!page && page !== 0 && !offset && offset !== 0)
) {
return;
}

if (!['name', 'createdAt', 'usersCount', ...roomParams, ...userParams].includes(sortBy)) {
return;
}
const roomParams = ['channels', 'teams'].includes(type) ? ['usernames', 'lastMessage'] : [];
const userParams = type === 'users' ? ['username', 'email', 'bio'] : [];

const skip = Math.max(0, offset || (page > -1 ? limit * page : 0));
if (!['name', 'createdAt', 'usersCount', ...roomParams, ...userParams].includes(sortBy)) {
return;
}

limit = limit > 0 ? limit : 10;
const skip = Math.max(0, offset || (page > -1 ? limit * page : 0));

const pagination = {
skip,
limit,
};
limit = limit > 0 ? limit : 10;

const canViewAnonymous = !!settings.get('Accounts_AllowAnonymousRead');
const pagination = {
skip,
limit,
};

const user = (await Meteor.userAsync()) as IUser | null;
const canViewAnonymous = !!settings.get('Accounts_AllowAnonymousRead');

if (!user) {
return;
}
if (!user) {
return;
}
Comment on lines +370 to +372
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Error Handling high

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 returning undefined to 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.

​

​


switch (type) {
case 'channels':
return getChannelsAndGroups(user, canViewAnonymous, searchTerm, sortChannels(sortBy, sortDirection), pagination);
case 'teams':
return getTeams(user, searchTerm, sortChannels(sortBy, sortDirection), pagination);
case 'users':
return getUsers(user, text, workspace, sortUsers(sortBy, sortDirection), pagination);
default:
}
switch (type) {
case 'channels':
return getChannelsAndGroups(user, canViewAnonymous, searchTerm, sortChannels(sortBy, sortDirection), pagination);
case 'teams':
return getTeams(user, searchTerm, sortChannels(sortBy, sortDirection), pagination);
case 'users':
return getUsers(user, text, workspace, sortUsers(sortBy, sortDirection), pagination);
default:
}
};

Meteor.methods<ServerMethods>({
async browseChannels(params: BrowseChannelsParams) {
return browseChannelsMethod(params, (await Meteor.userAsync()) as IUser | null);
},
});

Expand Down
62 changes: 39 additions & 23 deletions apps/meteor/server/publications/spotlight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,20 @@ import { Meteor } from 'meteor/meteor';

import { Spotlight } from '../lib/spotlight';

type SpotlightType = {
users?: boolean;
rooms?: boolean;
mentions?: boolean;
includeFederatedRooms?: boolean;
};

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
interface ServerMethods {
spotlight(
text: string,
usernames?: string[],
type?: {
users?: boolean;
rooms?: boolean;
mentions?: boolean;
includeFederatedRooms?: boolean;
},
type?: SpotlightType,
rid?: string,
): {
rooms: { _id: string; name: string; t: string; uids?: string[] }[];
Expand All @@ -32,27 +34,41 @@ declare module '@rocket.chat/ddp-client' {
}
}

Meteor.methods<ServerMethods>({
async spotlight(text, usernames = [], type = { users: true, rooms: true, mentions: false, includeFederatedRooms: false }, rid) {
const spotlight = new Spotlight();
const { mentions, includeFederatedRooms } = type;
export const spotlightMethod = async ({
text,
usernames = [],
type = { users: true, rooms: true, mentions: false, includeFederatedRooms: false },
rid,
userId,
}: {
text: string;
usernames?: string[];
type?: SpotlightType;
rid?: string;
userId?: string | null;
}) => {
const spotlight = new Spotlight();
const { mentions, includeFederatedRooms } = type;

if (text.startsWith('#')) {
type.users = false;
text = text.slice(1);
}
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);
}
Comment on lines +53 to +61
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Maintainability high

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 the type object 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 { userId } = this;
return {
users: type.users ? await spotlight.searchUsers({ userId, rid, text, usernames, mentions }) : [],
rooms: type.rooms ? await spotlight.searchRooms({ userId, text, includeFederatedRooms }) : [],
};
};

return {
users: type.users ? await spotlight.searchUsers({ userId, rid, text, usernames, mentions }) : [],
rooms: type.rooms ? await spotlight.searchRooms({ userId, text, includeFederatedRooms }) : [],
};
Meteor.methods<ServerMethods>({
async spotlight(text, usernames = [], type = { users: true, rooms: true, mentions: false, includeFederatedRooms: false }, rid) {
return spotlightMethod({ text, usernames, type, rid, userId: this.userId });
},
});

Expand Down
Loading