Skip to content
13 changes: 7 additions & 6 deletions apps/meteor/app/lib/server/functions/createDirectRoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { MatchKeysAndValues } from 'mongodb';

import { isTruthy } from '../../../../lib/isTruthy';
import { callbacks } from '../../../../server/lib/callbacks';
import { getNameForDMs } from '../../../../server/services/room/getNameForDMs';
import { settings } from '../../../settings/server';
import { getDefaultSubscriptionPref } from '../../../utils/lib/getDefaultSubscriptionPref';
import { notifyOnRoomChangedById, notifyOnSubscriptionChangedByRoomIdAndUserId } from '../lib/notifyListener';
Expand Down Expand Up @@ -37,9 +38,6 @@ const generateSubscription = (
},
});

const getFname = (members: IUser[]): string => members.map(({ name, username }) => name || username).join(', ');
const getName = (members: IUser[]): string => members.map(({ username }) => username).join(', ');

export async function createDirectRoom(
members: IUser[] | string[],
roomExtraData: Partial<IRoom> = {},
Expand Down Expand Up @@ -71,6 +69,7 @@ export async function createDirectRoom(
await callbacks.run('beforeCreateDirectRoom', membersUsernames, roomExtraData);

const roomMembers = await Users.findUsersByUsernames(membersUsernames).toArray();

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const sortedMembers = roomMembers.sort((u1, u2) => (u1.name! || u1.username!).localeCompare(u2.name! || u2.username!));

Expand Down Expand Up @@ -159,9 +158,9 @@ export async function createDirectRoom(
throw new Meteor.Error('error-creator-not-in-room', 'The creator user must be part of the direct room');
}

for await (const member of membersWithPreferences) {
const otherMembers = sortedMembers.filter(({ _id }) => _id !== member._id);
const roomNames = getNameForDMs(roomMembers);

for await (const member of membersWithPreferences) {
const subscriptionStatus: Partial<ISubscription> =
roomExtraData.federated && options.creator !== member._id && creatorUser
? {
Expand All @@ -177,11 +176,13 @@ export async function createDirectRoom(
}
: {};

const { fname, name } = roomNames[member._id];

const { modifiedCount, upsertedCount } = await Subscriptions.updateOne(
{ rid, 'u._id': member._id },
{
...(options?.creator === member._id && { $set: { open: true } }),
$setOnInsert: generateSubscription(getFname(otherMembers), getName(otherMembers), member, {
$setOnInsert: generateSubscription(fname, name, member, {
...options?.subscriptionExtra,
...(options?.creator !== member._id && { open: members.length > 2 }),
...subscriptionStatus,
Expand Down
21 changes: 15 additions & 6 deletions apps/meteor/app/lib/server/functions/removeUserFromRoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,15 @@ export const performUserRemoval = async function (
return;
}

// make TS happy, this should never happen
if (!user.username) {
throw new Error('User must have a username to be removed from the room');
}

// TODO: move before callbacks to service
await beforeLeaveRoomCallback.run(user, room);

if (subscription) {
const removedUser = user;
if (options?.customSystemMessage) {
await Message.saveSystemMessage(options?.customSystemMessage, room._id, user.username || '', user);
} else if (options?.byUser) {
Expand All @@ -40,16 +44,16 @@ export const performUserRemoval = async function (
};

if (room.teamMain) {
await Message.saveSystemMessage('removed-user-from-team', room._id, user.username || '', user, extraData);
await Message.saveSystemMessage('removed-user-from-team', room._id, user.username, user, extraData);
} else {
await Message.saveSystemMessage('ru', room._id, user.username || '', user, extraData);
await Message.saveSystemMessage('ru', room._id, user.username, user, extraData);
}
} else if (subscription.status === 'INVITED') {
await Message.saveSystemMessage('uir', room._id, removedUser.username || '', removedUser);
await Message.saveSystemMessage('uir', room._id, user.username, user);
} else if (room.teamMain) {
await Message.saveSystemMessage('ult', room._id, removedUser.username || '', removedUser);
await Message.saveSystemMessage('ult', room._id, user.username, user);
} else {
await Message.saveSystemMessage('ul', room._id, removedUser.username || '', removedUser);
await Message.saveSystemMessage('ul', room._id, user.username, user);
}
}

Expand All @@ -70,6 +74,11 @@ export const performUserRemoval = async function (
await Rooms.removeUsersFromE2EEQueueByRoomId(room._id, [user._id]);
}

// remove references to the user in direct message rooms
if (room.t === 'd') {
await Rooms.removeUserReferenceFromDMsById(room._id, user.username, user._id);
}

void notifyOnRoomChangedById(room._id);
};

Expand Down
33 changes: 33 additions & 0 deletions apps/meteor/server/services/room/getNameForDMs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import type { AtLeast, IUser } from '@rocket.chat/core-typings';

const getFname = (members: AtLeast<IUser, 'name' | 'username'>[]): string | undefined => {
if (members.length === 0) {
return;
}
return members.map(({ name, username }) => name || username).join(', ');
};
const getName = (members: AtLeast<IUser, 'name' | 'username'>[]): string | undefined => {
if (members.length === 0) {
return;
}
return members.map(({ username }) => username).join(', ');
};

type NameMap = { [userId: string]: { fname: string; name: string } };

export function getNameForDMs(members: AtLeast<IUser, '_id' | 'name' | 'username'>[]): NameMap {
const nameMap: NameMap = {};

const sortedMembers = members.sort((u1, u2) => (u1.name! || u1.username!).localeCompare(u2.name! || u2.username!));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Mutates input array and uses unsafe non-null assertions.

Two issues on this line:

  1. members.sort() mutates the input array in place. If callers don't expect mutation, this could cause subtle bugs. Consider using [...members].sort() to sort a copy.

  2. The non-null assertions u1.name! and u1.username! are unsafe—if both name and username are undefined (which is possible per IUser type), the expression evaluates to undefined and localeCompare throws.

🔎 Suggested fix:
-	const sortedMembers = members.sort((u1, u2) => (u1.name! || u1.username!).localeCompare(u2.name! || u2.username!));
+	const sortedMembers = [...members].sort((u1, u2) => (u1.name || u1.username || '').localeCompare(u2.name || u2.username || ''));
🤖 Prompt for AI Agents
In apps/meteor/server/services/room/getNameForDMs.ts around line 21, the current
sort mutates the input array and uses unsafe non-null assertions; change to sort
a shallow copy (e.g., [...members]) and replace u1.name! || u1.username! with a
safe fallback such as (u1.name ?? u1.username ?? '') and same for u2 so
localeCompare never receives undefined; ensure you call localeCompare on a
string (or use String(...).localeCompare(...)) to avoid runtime errors.


for (const member of sortedMembers) {
const otherMembers = sortedMembers.filter((m) => m._id !== member._id);

nameMap[member._id] = {
fname: getFname(otherMembers) || member.name || member.username || '',
name: getName(otherMembers) || member.username || '',
};
}

return nameMap;
}
21 changes: 20 additions & 1 deletion apps/meteor/server/services/room/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ import {
} from '@rocket.chat/core-typings';
import { Rooms, Subscriptions, Users } from '@rocket.chat/models';

import { getNameForDMs } from './getNameForDMs';
import { FederationActions } from './hooks/BeforeFederationActions';
import { saveRoomName } from '../../../app/channel-settings/server';
import { saveRoomTopic } from '../../../app/channel-settings/server/functions/saveRoomTopic';
import { performAcceptRoomInvite } from '../../../app/lib/server/functions/acceptRoomInvite';
import { addUserToRoom } from '../../../app/lib/server/functions/addUserToRoom';
import { createRoom } from '../../../app/lib/server/functions/createRoom'; // TODO remove this import
import { removeUserFromRoom, performUserRemoval } from '../../../app/lib/server/functions/removeUserFromRoom';
import { notifyOnSubscriptionChangedById } from '../../../app/lib/server/lib/notifyListener';
import { notifyOnSubscriptionChangedById, notifyOnSubscriptionChangedByRoomIdAndUserId } from '../../../app/lib/server/lib/notifyListener';
import { getDefaultSubscriptionPref } from '../../../app/utils/lib/getDefaultSubscriptionPref';
import { getValidRoomName } from '../../../app/utils/server/lib/getValidRoomName';
import { RoomMemberActions } from '../../../definition/IRoomTypeConfig';
Expand All @@ -35,6 +36,24 @@ import { removeRoomOwner } from '../../methods/removeRoomOwner';
export class RoomService extends ServiceClassInternal implements IRoomService {
protected name = 'room';

async updateDirectMessageRoomName(room: IRoom): Promise<boolean> {
const subs = await Subscriptions.findByRoomId(room._id, { projection: { u: 1 } }).toArray();

const uids = subs.map((sub) => sub.u._id);

const roomMembers = await Users.findUsersByIds(uids, { projection: { name: 1, username: 1 } }).toArray();

const roomNames = getNameForDMs(roomMembers);

for await (const sub of subs) {
await Subscriptions.updateOne({ _id: sub._id }, { $set: roomNames[sub.u._id] });

void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, sub.u._id, 'updated');
}

return true;
}

async create(uid: string, params: ICreateRoomParams): Promise<IRoom> {
const { type, name, members = [], readOnly, extraData, options } = params;

Expand Down
15 changes: 11 additions & 4 deletions apps/meteor/tests/data/rooms.helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,20 @@ export function actionRoom({ action, type, roomId, overrideCredentials = credent
export const deleteRoom = ({ type, roomId }: { type: ActionRoomParams['type']; roomId: IRoom['_id'] }) =>
actionRoom({ action: 'delete', type, roomId, overrideCredentials: credentials });

export const getSubscriptionByRoomId = (roomId: IRoom['_id'], userCredentials = credentials): Promise<ISubscription> =>
new Promise((resolve) => {
void request
export const getSubscriptionByRoomId = (roomId: IRoom['_id'], userCredentials = credentials, req = request): Promise<ISubscription> =>
new Promise((resolve, reject) => {
void req
.get(api('subscriptions.getOne'))
.set(userCredentials)
.query({ roomId })
.end((_err, res) => {
.end((err, res) => {
if (err) {
return reject(err);
}
if (!res.body?.subscription) {
return reject(new Error('Subscription not found'));
}

resolve(res.body.subscription);
});
});
Expand Down
128 changes: 128 additions & 0 deletions apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import { expect } from 'chai';

import { getNameForDMs } from '../../../../../server/services/room/getNameForDMs';

describe('getNameForDMs', () => {
it('should return empty object when members array is empty', () => {
const result = getNameForDMs([]);
expect(result).to.deep.equal({});
});

it('should return own name for single member', () => {
const members = [{ _id: 'user1', name: 'John Doe', username: 'john' }];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'John Doe',
name: 'john',
},
});
});

it('should return name map for two members using name field', () => {
const members = [
{ _id: 'user1', name: 'John Doe', username: 'john' },
{ _id: 'user2', name: 'Jane Smith', username: 'jane' },
];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'Jane Smith',
name: 'jane',
},
user2: {
fname: 'John Doe',
name: 'john',
},
});
});

it('should fallback to username when name is not available', () => {
const members = [
{ _id: 'user1', name: '', username: 'john' },
{ _id: 'user2', name: '', username: 'jane' },
];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'jane',
name: 'jane',
},
user2: {
fname: 'john',
name: 'john',
},
});
});

it('should handle multiple members and sort them alphabetically', () => {
const members = [
{ _id: 'user3', name: 'Charlie Brown', username: 'charlie' },
{ _id: 'user1', name: 'Alice Wonder', username: 'alice' },
{ _id: 'user2', name: 'Bob Builder', username: 'bob' },
];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'Bob Builder, Charlie Brown',
name: 'bob, charlie',
},
user2: {
fname: 'Alice Wonder, Charlie Brown',
name: 'alice, charlie',
},
user3: {
fname: 'Alice Wonder, Bob Builder',
name: 'alice, bob',
},
});
});

it('should handle mix of members with and without names', () => {
const members = [
{ _id: 'user1', name: 'Alice Wonder', username: 'alice' },
{ _id: 'user2', name: '', username: 'bob' },
];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'bob',
name: 'bob',
},
user2: {
fname: 'Alice Wonder',
name: 'alice',
},
});
});

it('should sort by username when names are empty', () => {
const members = [
{ _id: 'user1', name: '', username: 'zebra' },
{ _id: 'user2', name: '', username: 'alpha' },
];

const result = getNameForDMs(members);

expect(result).to.deep.equal({
user1: {
fname: 'alpha',
name: 'alpha',
},
user2: {
fname: 'zebra',
name: 'zebra',
},
});
});
});
7 changes: 6 additions & 1 deletion ee/packages/federation-matrix/src/events/member.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ async function getOrCreateFederatedRoom({
mrid: matrixRoomId,
origin,
},
fname: roomFName,
...(roomType !== 'd' && { fname: roomFName }), // DMs do not have a fname
},
});
} catch (error) {
Expand Down Expand Up @@ -232,6 +232,11 @@ async function handleLeave({

await Room.performUserRemoval(room, leavingUser);

// update room name for DMs
if (room.t === 'd') {
await Room.updateDirectMessageRoomName(room);
}

// TODO check if there are no pending invites to the room, and if so, delete the room
}

Expand Down
Loading
Loading