fix: duplicate rooms in sidebar when a cached record _id is not a string - #42112
fix: duplicate rooms in sidebar when a cached record _id is not a string#42112sudoKrishna wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 0b3b46f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
Walkthrough
ChangesDocumentMapStore identifier normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested labels: Suggested reviewers: Merge Risk: ⚪ Minimal · up to Identifier normalization is applied across the store’s key operations and covered by focused tests for equivalent, distinct, deletion, and string identifiers. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Errors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/lib/cachedStores/DocumentMapStore.ts">
<violation number="1" location="apps/meteor/client/lib/cachedStores/DocumentMapStore.ts:22">
P2: When a valid string `_id` shares its text with an ObjectId (or a binary ID shares bytes with another subtype), `normalizeId` maps both records to the same key and the store drops one. Preserve the BSON representation/type (and binary subtype) in the normalized key instead of returning an untagged payload string.</violation>
</file>
<file name="apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts">
<violation number="1" location="apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts:36">
P3: The PR description states normalizeId() was applied to store/storeMany/replaceAll/has/get/delete/update/updateAsync/remove, but the spec only exercises store, delete, and get. has, storeMany, replaceAll, update, updateAsync, and remove all take a non-string _id through normalizeId and are the easiest places to regress the exact bug being fixed; add cases that store an ObjectId/EJSON _id and assert these methods hit the same normalized key (e.g. has() after an ObjectId store, and delete() of a binary _id stored via storeMany).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| if (typeof (id as { toHexString?: unknown }).toHexString === 'function') { | ||
| return (id as unknown as { toHexString: () => string }).toHexString(); |
There was a problem hiding this comment.
P2: When a valid string _id shares its text with an ObjectId (or a binary ID shares bytes with another subtype), normalizeId maps both records to the same key and the store drops one. Preserve the BSON representation/type (and binary subtype) in the normalized key instead of returning an untagged payload string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/cachedStores/DocumentMapStore.ts, line 22:
<comment>When a valid string `_id` shares its text with an ObjectId (or a binary ID shares bytes with another subtype), `normalizeId` maps both records to the same key and the store drops one. Preserve the BSON representation/type (and binary subtype) in the normalized key instead of returning an untagged payload string.</comment>
<file context>
@@ -1,5 +1,38 @@
+ }
+
+ if (typeof (id as { toHexString?: unknown }).toHexString === 'function') {
+ return (id as unknown as { toHexString: () => string }).toHexString();
+ }
+
</file context>
| expect(useStore.getState().records.size).toBe(1); | ||
| }); | ||
|
|
||
| it('deletes a record stored with a non-string _id when given an equivalent but distinct instance', () => { |
There was a problem hiding this comment.
P3: The PR description states normalizeId() was applied to store/storeMany/replaceAll/has/get/delete/update/updateAsync/remove, but the spec only exercises store, delete, and get. has, storeMany, replaceAll, update, updateAsync, and remove all take a non-string _id through normalizeId and are the easiest places to regress the exact bug being fixed; add cases that store an ObjectId/EJSON _id and assert these methods hit the same normalized key (e.g. has() after an ObjectId store, and delete() of a binary _id stored via storeMany).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts, line 36:
<comment>The PR description states normalizeId() was applied to store/storeMany/replaceAll/has/get/delete/update/updateAsync/remove, but the spec only exercises store, delete, and get. has, storeMany, replaceAll, update, updateAsync, and remove all take a non-string _id through normalizeId and are the easiest places to regress the exact bug being fixed; add cases that store an ObjectId/EJSON _id and assert these methods hit the same normalized key (e.g. has() after an ObjectId store, and delete() of a binary _id stored via storeMany).</comment>
<file context>
@@ -0,0 +1,64 @@
+ 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>();
+
</file context>
1 participant
Fixes #42109
Problem
DocumentMapStorekeys its cached records (rooms, subscriptions, etc.) byrecord._idin aMap._idis typed asstring, but DDP/EJSON payloadsor manual DB writes can send a BSON ObjectId or EJSON binary wrapper
instead. Two "equal" but distinct instances of the same id are different
Map keys, so every re-merge of that record adds a new entry instead of
replacing it — the sidebar then shows duplicate rows for the same room.
Fix
Added
normalizeId()inDocumentMapStore.tsto reduce any_idshape(string, ObjectId via
toHexString(), EJSON$binary) to a stable string,applied everywhere
_idis used as a Map key (store,storeMany,replaceAll,has,get,delete,update,updateAsync,remove).Testing
Added
DocumentMapStore.spec.tscovering ObjectId/EJSON dupes, delete byan equivalent instance, no over-collapsing of different ids, and a plain
string regression check. Before/after screenshots attached (failing on
develop, passing on this branch).eslintandtsc --noEmitclean.before :
after :
Summary by CodeRabbit