fix: duplicate rooms in sidebar when a cached record _id is not a string

.changeset/fix-duplicate-rooms-non-string-id.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Fixes duplicate rooms/subscriptions accumulating in the client-side cache (and sidebar) when a record's `_id` arrives as something other than a plain string (e.g. a BSON ObjectId instance or an EJSON binary wrapper, as can happen with manually inserted or migrated documents). `DocumentMapStore` now normalizes `_id` to a stable string before using it as the underlying Map key, so repeated merges of the same record replace the previous entry instead of piling up a new one each time.

apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts

@@ -0,0 +1,64 @@
+import { createDocumentMapStore } from './DocumentMapStore';
+
+interface ITestRecord {
+	_id: string;
+	name: string;
+}
+
+describe('DocumentMapStore', () => {
+	describe('non-string _id normalization', () => {
+		it('replaces the previous entry when the same BSON ObjectId-like _id is stored twice', () => {
+			const useStore = createDocumentMapStore<ITestRecord>();
+
+			// A Mongo ObjectId instance: not a string, but exposes toHexString().
+			const objectIdLike = { toHexString: () => '507f1f77bcf86cd799439011' };
+
+			useStore.getState().store({ _id: objectIdLike as unknown as string, name: 'first' });
+			// A distinct object instance representing the *same* id, as would arrive on a second DDP merge.
+			const secondInstance = { toHexString: () => '507f1f77bcf86cd799439011' };
+			useStore.getState().store({ _id: secondInstance as unknown as string, name: 'second' });
+
+			expect(useStore.getState().records.size).toBe(1);
+			expect(Array.from(useStore.getState().records.values())[0].name).toBe('second');
+		});
+
+		it('replaces the previous entry when the same EJSON binary _id is stored twice', () => {
+			const useStore = createDocumentMapStore<ITestRecord>();
+
+			const binaryId = { buffer: { $binary: 'an/HuDdrg3tLiqbD' } };
+
+			useStore.getState().store({ _id: binaryId as unknown as string, name: 'first' });
+			useStore.getState().store({ _id: { buffer: { $binary: 'an/HuDdrg3tLiqbD' } } as unknown as string, name: 'second' });
+
+			expect(useStore.getState().records.size).toBe(1);
+		});
+
+		it('deletes a record stored with a non-string _id when given an equivalent but distinct instance', () => {
+			const useStore = createDocumentMapStore<ITestRecord>();
+
+			useStore.getState().store({ _id: { toHexString: () => 'abc123' } as unknown as string, name: 'first' });
+			useStore.getState().delete({ toHexString: () => 'abc123' } as unknown as string);
+
+			expect(useStore.getState().records.size).toBe(0);
+		});
+
+		it('does not collapse different ids into the same entry', () => {
+			const useStore = createDocumentMapStore<ITestRecord>();
+
+			useStore.getState().store({ _id: { toHexString: () => 'aaa' } as unknown as string, name: 'first' });
+			useStore.getState().store({ _id: { toHexString: () => 'bbb' } as unknown as string, name: 'second' });
+
+			expect(useStore.getState().records.size).toBe(2);
+		});
+
+		it('still works for plain string ids (no regression)', () => {
+			const useStore = createDocumentMapStore<ITestRecord>();
+
+			useStore.getState().store({ _id: 'room-1', name: 'first' });
+			useStore.getState().store({ _id: 'room-1', name: 'updated' });
+
+			expect(useStore.getState().records.size).toBe(1);
+			expect(useStore.getState().get('room-1')?.name).toBe('updated');
+		});
+	});
+});

apps/meteor/client/lib/cachedStores/DocumentMapStore.ts

@@ -1,5 +1,38 @@
 import { create } from 'zustand';
 
+const isBinaryLike = (value: unknown): value is { $binary: string } =>
+	typeof value === 'object' && value !== null && typeof (value as { $binary?: unknown }).$binary === 'string';
+
+/**
+ * Derives a stable string key for a document's `_id`.
+ *
+ * `_id` is typed as `string`, but records coming from DDP/EJSON, imports or manual database
+ * writes can carry other representations of an id (a BSON ObjectId instance, an EJSON binary
+ * wrapper, etc). Using such a value directly as a `Map` key relies on object identity: every
+ * time an "equal" but distinct instance of the same id arrives, it collides with nothing and a
+ * duplicate entry is stored instead of replacing the previous one. Normalizing to a string here
+ * keeps `store`/`storeMany`/`has`/`get`/`delete` consistent regardless of how the id arrived.
+ */
+const normalizeId = <TId>(id: TId): TId | string => {
+	if (typeof id === 'string' || typeof id !== 'object' || id === null) {
+		return id;
+	}
+
+	if (typeof (id as { toHexString?: unknown }).toHexString === 'function') {
+		return (id as unknown as { toHexString: () => string }).toHexString();
+	}
+
+	if (isBinaryLike(id)) {
+		return id.$binary;
+	}
+
+	if ('buffer' in id && isBinaryLike((id as { buffer: unknown }).buffer)) {
+		return (id as unknown as { buffer: { $binary: string } }).buffer.$binary;
+	}
+
+	return JSON.stringify(id);
+};
+
 export interface IDocumentMapStore<T extends { _id: string }> {
 	readonly records: ReadonlyMap<T['_id'], T>;
 	/**
@@ -169,8 +202,8 @@ export interface IDocumentMapStoreHooks<T extends { _id: string }> {
 export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate, onInvalidateAll }: IDocumentMapStoreHooks<T> = {}) =>
 	create<IDocumentMapStore<T>>()((set, get) => ({
 		records: new Map(),
-		has: (id: T['_id']) => get().records.has(id),
-		get: (id: T['_id']) => get().records.get(id),
+		has: (id: T['_id']) => get().records.has(normalizeId(id)),
+		get: (id: T['_id']) => get().records.get(normalizeId(id)),
 		some: (predicate: (record: T) => boolean) => {
 			for (const record of get().records.values()) {
 				if (predicate(record)) return true;
@@ -213,31 +246,32 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 			return index;
 		},
 		replaceAll: (records: T[]) => {
-			set({ records: new Map(records.map((record) => [record._id, record])) });
+			set({ records: new Map(records.map((record) => [normalizeId(record._id), record])) });
 			onInvalidateAll?.();
 		},
 		store: (doc) => {
-			set((state) => ({ records: new Map(state.records).set(doc._id, doc) }));
+			set((state) => ({ records: new Map(state.records).set(normalizeId(doc._id), doc) }));
 			onInvalidate?.(doc);
 		},
 		storeMany: (docs) => {
 			set((state) => {
 				const records = new Map(state.records);
 
 				for (const doc of docs) {
-					records.set(doc._id, doc);
+					records.set(normalizeId(doc._id), doc);
 				}
 
 				return { records };
 			});
 			onInvalidate?.(...docs);
 		},
 		delete: (_id) => {
+			const key = normalizeId(_id);
 			const affected: T[] = [];
 			set((state) => {
 				const records = new Map(state.records);
-				if (onInvalidate) affected.push(state.records.get(_id)!);
-				records.delete(_id);
+				if (onInvalidate) affected.push(state.records.get(key)!);
+				records.delete(key);
 				return { records };
 			});
 			onInvalidate?.(...affected);
@@ -250,10 +284,10 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 				for (const record of state.records.values()) {
 					if (predicate(record)) {
 						const newRecord = modifier(record);
-						records.set(record._id, newRecord);
+						records.set(normalizeId(newRecord._id), newRecord);
 						if (onInvalidate) affected.push(newRecord);
 					} else {
-						records.set(record._id, record);
+						records.set(normalizeId(record._id), record);
 					}
 				}
 
@@ -269,10 +303,10 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 			for await (const record of get().records.values()) {
 				if (predicate(record)) {
 					const newRecord = await modifier(record);
-					records.set(record._id, newRecord);
+					records.set(normalizeId(newRecord._id), newRecord);
 					if (onInvalidate) affected.push(newRecord);
 				} else {
-					records.set(record._id, record);
+					records.set(normalizeId(record._id), record);
 				}
 			}
 
@@ -289,7 +323,7 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 						if (onInvalidate) affected.push(record);
 						continue;
 					}
-					records.set(record._id, record);
+					records.set(normalizeId(record._id), record);
 				}
 
 				return { records };