From 5ef85f26a0b7ef5a89ee7967f3f1791c336ba9ed Mon Sep 17 00:00:00 2001 From: Naab2k3 <161471076+Naab2k3@users.noreply.github.com> Date: Sat, 12 Sep 2026 01:38:30 +0700 Subject: [PATCH 1/2] fix: normalize the id used to key cached store records A record can reach the cached stores carrying another representation of its id - a BSON ObjectId, or the raw bytes of an EJSON binary. Those values are not stable map keys, so every merge stored a new entry instead of replacing the previous one, piling duplicates up in the store and in the persisted cache. Users see it as the same room repeated in the sidebar, and it only goes away when they clear their site data. Records are normalized as they enter the store (store, storeMany, replaceAll) and lookups normalize the id they receive, which also covers the keys rebuilt by update, updateAsync and remove. The cache version is left alone: loading a cache re-keys its records in memory. --- .changeset/olive-pandas-listen.md | 5 ++ .../lib/cachedStores/DocumentMapStore.spec.ts | 71 +++++++++++++++++++ .../lib/cachedStores/DocumentMapStore.ts | 62 +++++++++++++--- 3 files changed, 129 insertions(+), 9 deletions(-) create mode 100644 .changeset/olive-pandas-listen.md create mode 100644 apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts diff --git a/.changeset/olive-pandas-listen.md b/.changeset/olive-pandas-listen.md new file mode 100644 index 0000000000000..f2b5fcf411b5d --- /dev/null +++ b/.changeset/olive-pandas-listen.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes rooms showing up more than once in the sidebar, caused by the cached stores keeping a separate entry for every merge of a record whose `_id` does not arrive as a string. diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts new file mode 100644 index 0000000000000..b813427fba035 --- /dev/null +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts @@ -0,0 +1,71 @@ +import { createDocumentMapStore, toRecordId } from './DocumentMapStore'; + +const hexId = '6a7fc7b8376b837b4b8aa6c3'; +const binaryId = Uint8Array.from(hexId.match(/../g) ?? [], (byte) => parseInt(byte, 16)); + +interface ITestRecord { + _id: string; + rid: string; +} + +const fromBinary = { _id: binaryId as unknown as string, rid: hexId }; +const fromString = { _id: hexId, rid: hexId }; + +describe('cachedStores/DocumentMapStore', () => { + describe('toRecordId', () => { + it('should keep a string id as it is', () => { + expect(toRecordId(hexId)).toBe(hexId); + }); + + it('should read the hexadecimal id out of a binary id', () => { + expect(toRecordId(binaryId)).toBe(hexId); + }); + }); + + describe('createDocumentMapStore', () => { + it('should store a document carrying a binary id under its hexadecimal id', () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + + expect([...store.getState().records.keys()]).toEqual([hexId]); + expect(store.getState().has(hexId)).toBe(true); + expect(store.getState().has(fromBinary._id)).toBe(true); + expect(store.getState().get(hexId)?.rid).toBe(hexId); + }); + + it('should not store the same document twice when its id arrives in different representations', () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + store.getState().store(fromString); + + expect(store.getState().records.size).toBe(1); + }); + + it('should not duplicate a document when many records are stored at once', () => { + const store = createDocumentMapStore(); + + store.getState().storeMany([fromBinary, fromString]); + + expect(store.getState().records.size).toBe(1); + }); + + it('should collapse the records of a cache written with mixed representations', () => { + const store = createDocumentMapStore(); + + store.getState().replaceAll([fromString, fromBinary, fromBinary, fromString]); + + expect(store.getState().records.size).toBe(1); + }); + + it('should delete a document by any representation of its id', () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + store.getState().delete(fromBinary._id); + + expect(store.getState().records.size).toBe(0); + }); + }); +}); diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts index 551fa25915e52..3eec85c86c370 100644 --- a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts @@ -1,5 +1,41 @@ import { create } from 'zustand'; +const bytesToHex = (bytes: Uint8Array): string => Array.from(bytes, (byte) => byte.toString(16).padStart(2, '0')).join(''); + +/** + * Documents are identified by a string `_id`, but a document can reach the store + * carrying another representation of the same id, e.g. a BSON ObjectId or the raw + * bytes of an EJSON binary. Those are not stable map keys: every merge of such a + * document stores a new entry instead of replacing the previous one, which makes + * repeated records pile up in the store (and in the persisted cache). + * + * @param id - The id of a document, in any of the representations it may arrive in. + * @returns The id as a string. + */ +export const toRecordId = (id: unknown): string => { + if (typeof id === 'string') { + return id; + } + + if (ArrayBuffer.isView(id)) { + return bytesToHex(new Uint8Array(id.buffer, id.byteOffset, id.byteLength)); + } + + return JSON.stringify(id) ?? String(id); +}; + +const withStringId = (record: T): T => { + const _id = toRecordId(record._id); + + return _id === record._id ? record : { ...record, _id }; +}; + +const toEntry = (record: T): [string, T] => { + const stored = withStringId(record); + + return [stored._id, stored]; +}; + export interface IDocumentMapStore { readonly records: ReadonlyMap; /** @@ -169,8 +205,8 @@ export interface IDocumentMapStoreHooks { export const createDocumentMapStore = ({ onInvalidate, onInvalidateAll }: IDocumentMapStoreHooks = {}) => create>()((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(toRecordId(id)), + get: (id: T['_id']) => get().records.get(toRecordId(id)), some: (predicate: (record: T) => boolean) => { for (const record of get().records.values()) { if (predicate(record)) return true; @@ -213,31 +249,39 @@ export const createDocumentMapStore = ({ onInvalidate return index; }, replaceAll: (records: T[]) => { - set({ records: new Map(records.map((record) => [record._id, record])) }); + set({ records: new Map(records.map(toEntry)) }); onInvalidateAll?.(); }, store: (doc) => { - set((state) => ({ records: new Map(state.records).set(doc._id, doc) })); - onInvalidate?.(doc); + const record = withStringId(doc); + + set((state) => ({ records: new Map(state.records).set(record._id, record) })); + onInvalidate?.(record); }, storeMany: (docs) => { + const stored: T[] = []; + set((state) => { const records = new Map(state.records); for (const doc of docs) { - records.set(doc._id, doc); + const entry = toEntry(doc); + + records.set(...entry); + stored.push(entry[1]); } return { records }; }); - onInvalidate?.(...docs); + onInvalidate?.(...stored); }, delete: (_id) => { const affected: T[] = []; + const key = toRecordId(_id); 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); From 7e1ecfe4230bd0052e510f5b7bba2a0e4fc0a265 Mon Sep 17 00:00:00 2001 From: Naab2k3 <161471076+Naab2k3@users.noreply.github.com> Date: Sat, 12 Sep 2026 07:39:56 +0700 Subject: [PATCH 2/2] fix: read the id out of ObjectIds and normalize update modifiers From review: - toRecordId now reads the hexadecimal id out of an ObjectId instead of falling back to JSON.stringify, which produced a quoted key and kept such a record from merging with the same record carrying a string id - update and updateAsync normalize the record the modifier returns, so an entry cannot end up keyed by a string id while holding a binary one - the spec covers update, updateAsync and remove --- .../lib/cachedStores/DocumentMapStore.spec.ts | 50 +++++++++++++++++++ .../lib/cachedStores/DocumentMapStore.ts | 19 ++++--- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts index b813427fba035..c8cd771e416fc 100644 --- a/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts @@ -10,6 +10,7 @@ interface ITestRecord { const fromBinary = { _id: binaryId as unknown as string, rid: hexId }; const fromString = { _id: hexId, rid: hexId }; +const objectId = { toHexString: () => hexId }; describe('cachedStores/DocumentMapStore', () => { describe('toRecordId', () => { @@ -20,6 +21,10 @@ describe('cachedStores/DocumentMapStore', () => { it('should read the hexadecimal id out of a binary id', () => { expect(toRecordId(binaryId)).toBe(hexId); }); + + it('should read the hexadecimal id out of an ObjectId', () => { + expect(toRecordId(objectId)).toBe(hexId); + }); }); describe('createDocumentMapStore', () => { @@ -67,5 +72,50 @@ describe('cachedStores/DocumentMapStore', () => { expect(store.getState().records.size).toBe(0); }); + + it('should merge a document carrying an ObjectId with the same document carrying a string id', () => { + const store = createDocumentMapStore(); + + store.getState().store({ _id: objectId as unknown as string, rid: hexId }); + store.getState().store(fromString); + + expect(store.getState().records.size).toBe(1); + expect([...store.getState().records.keys()]).toEqual([hexId]); + }); + + it('should keep an updated document keyed by its hexadecimal id', () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + store.getState().update( + (record) => record.rid === hexId, + (record) => ({ ...record }), + ); + + expect(store.getState().records.size).toBe(1); + expect([...store.getState().records.keys()]).toEqual([hexId]); + }); + + it('should keep an asynchronously updated document keyed by its hexadecimal id', async () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + await store.getState().updateAsync( + (record) => record.rid === hexId, + async (record) => ({ ...record }), + ); + + expect(store.getState().records.size).toBe(1); + expect([...store.getState().records.keys()]).toEqual([hexId]); + }); + + it('should remove a document that was stored under a binary id', () => { + const store = createDocumentMapStore(); + + store.getState().store(fromBinary); + store.getState().remove((record) => record.rid === hexId); + + expect(store.getState().records.size).toBe(0); + }); }); }); diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts index 3eec85c86c370..c74223153dd3e 100644 --- a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts @@ -17,6 +17,11 @@ export const toRecordId = (id: unknown): string => { return id; } + // an ObjectId carries the same 24-character id in its hexadecimal form + if (typeof (id as { toHexString?: unknown })?.toHexString === 'function') { + return (id as { toHexString: () => string }).toHexString(); + } + if (ArrayBuffer.isView(id)) { return bytesToHex(new Uint8Array(id.buffer, id.byteOffset, id.byteLength)); } @@ -293,9 +298,10 @@ export const createDocumentMapStore = ({ onInvalidate const records = new Map(); for (const record of state.records.values()) { if (predicate(record)) { - const newRecord = modifier(record); - records.set(record._id, newRecord); - if (onInvalidate) affected.push(newRecord); + const entry = toEntry(modifier(record)); + + records.set(...entry); + if (onInvalidate) affected.push(entry[1]); } else { records.set(record._id, record); } @@ -312,9 +318,10 @@ export const createDocumentMapStore = ({ onInvalidate for await (const record of get().records.values()) { if (predicate(record)) { - const newRecord = await modifier(record); - records.set(record._id, newRecord); - if (onInvalidate) affected.push(newRecord); + const entry = toEntry(await modifier(record)); + + records.set(...entry); + if (onInvalidate) affected.push(entry[1]); } else { records.set(record._id, record); }