Address comments

Author: fabioh8010

lib/OnyxCache.ts

@@ -344,22 +344,15 @@ class OnyxCache {
             iterResult = iterator.next();
         }
 
-        const affectedCollections = new Set<OnyxKey>();
-
         for (const key of keysToRemove) {
             delete this.storageMap[key];
 
-            // Track affected collections for snapshot rebuild
             const collectionKey = OnyxKeys.getCollectionKey(key);
             if (collectionKey) {
-                affectedCollections.add(collectionKey);
+                this.dirtyCollections.add(collectionKey);
             }
             this.recentKeys.delete(key);
         }
-
-        for (const collectionKey of affectedCollections) {
-            this.dirtyCollections.add(collectionKey);
-        }
     }
 
     /** Set the recent keys list size */
@@ -478,10 +471,10 @@ class OnyxCache {
      * @param collectionKey - The collection key to rebuild
      */
     private rebuildCollectionSnapshot(collectionKey: OnyxKey): void {
-        const oldSnapshot = this.collectionSnapshots.get(collectionKey);
+        const previousSnapshot = this.collectionSnapshots.get(collectionKey);
 
         const members: NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>> = {};
-        let hasChanges = false;
+        let hasMemberChanges = false;
         let newMemberCount = 0;
 
         // Use the indexed forward lookup for O(collectionMembers) iteration.
@@ -495,29 +488,31 @@ class OnyxCache {
                 continue;
             }
             const val = this.storageMap[key];
+            // Skip null/undefined values — they represent deleted or unset keys
+            // and should not be included in the frozen collection snapshot.
             if (val !== undefined && val !== null) {
                 members[key] = val;
                 newMemberCount++;
 
                 // Check if this member's reference changed from the old snapshot
-                if (!hasChanges && (!oldSnapshot || oldSnapshot[key] !== val)) {
-                    hasChanges = true;
+                if (!hasMemberChanges && (!previousSnapshot || previousSnapshot[key] !== val)) {
+                    hasMemberChanges = true;
                 }
             }
         }
 
         // Check if any members were removed (old snapshot had more keys)
-        if (!hasChanges && oldSnapshot) {
-            const oldMemberCount = Object.keys(oldSnapshot).length;
+        if (!hasMemberChanges && previousSnapshot) {
+            const oldMemberCount = Object.keys(previousSnapshot).length;
             if (oldMemberCount !== newMemberCount) {
-                hasChanges = true;
+                hasMemberChanges = true;
             }
         }
 
         // If nothing actually changed, reuse the old snapshot reference.
         // This is critical: useSyncExternalStore uses === to detect changes,
         // so returning the same reference prevents unnecessary re-renders.
-        if (!hasChanges && oldSnapshot) {
+        if (!hasMemberChanges && previousSnapshot) {
             return;
         }
 
@@ -538,7 +533,7 @@ class OnyxCache {
         }
 
         const snapshot = this.collectionSnapshots.get(collectionKey);
-        if (!snapshot || Object.keys(snapshot).length === 0) {
+        if (utils.isEmptyObject(snapshot)) {
             // If we know we have storage keys loaded, return a stable empty reference
             // to avoid new {} allocations that break useSyncExternalStore === equality.
             if (this.storageKeys.size > 0) {

lib/utils.ts

@@ -172,7 +172,19 @@ function mergeObject<TObject extends Record<string, unknown>>(
 
 /** Checks whether the given object is an object and not null/undefined. */
 function isEmptyObject<T>(obj: T | EmptyValue): obj is EmptyValue {
-    return typeof obj === 'object' && Object.keys(obj || {}).length === 0;
+    if (typeof obj !== 'object') {
+        return false;
+    }
+
+    // Use for-in loop to avoid an unnecessary array allocation from Object.keys()
+    // eslint-disable-next-line no-restricted-syntax
+    for (const key in obj) {
+        if (Object.hasOwn(obj, key)) {
+            return false;
+        }
+    }
+
+    return true;
 }
 
 /**

tests/unit/utilsTest.ts

@@ -389,4 +389,36 @@ describe('utils', () => {
             });
         });
     });
+
+    describe('isEmptyObject', () => {
+        it('should return true for an empty object', () => {
+            expect(utils.isEmptyObject({})).toBe(true);
+        });
+
+        it('should return true for null', () => {
+            expect(utils.isEmptyObject(null)).toBe(true);
+        });
+
+        it('should return false for undefined', () => {
+            expect(utils.isEmptyObject(undefined)).toBe(false);
+        });
+
+        it('should return false for an object with properties', () => {
+            expect(utils.isEmptyObject({a: 1})).toBe(false);
+        });
+
+        it('should return false for non-object types', () => {
+            expect(utils.isEmptyObject('hello')).toBe(false);
+            expect(utils.isEmptyObject(42)).toBe(false);
+            expect(utils.isEmptyObject(true)).toBe(false);
+        });
+
+        it('should return true for an empty array', () => {
+            expect(utils.isEmptyObject([])).toBe(true);
+        });
+
+        it('should return false for a non-empty array', () => {
+            expect(utils.isEmptyObject([1, 2])).toBe(false);
+        });
+    });
 });