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..c8cd771e416fc --- /dev/null +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts @@ -0,0 +1,121 @@ +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 }; +const objectId = { toHexString: () => 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); + }); + + it('should read the hexadecimal id out of an ObjectId', () => { + expect(toRecordId(objectId)).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); + }); + + 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 551fa25915e52..c74223153dd3e 100644 --- a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts @@ -1,5 +1,46 @@ 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; + } + + // 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)); + } + + 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 +210,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 +254,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); @@ -249,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); } @@ -268,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); }