Fix metadata sync issue for cache listeners in multi-tab (#8343)

firebase · Sep 12, 2024 · 629919e · 629919e
1 parent 16d62d4
commit 629919e
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 16 deletions.
diff --git a/.changeset/warm-oranges-itch.md b/.changeset/warm-oranges-itch.md
@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': patch
+---
+
+Fix an issue with metadata `fromCache` defaulting to `true` when listening to cache in multi-tabs.
diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts
@@ -353,9 +353,11 @@ async function allocateTargetAndMaybeListen(
   // not registering it in shared client state, and directly calculate initial snapshots and
   // subsequent updates from cache. Otherwise, register the target ID with local Firestore client
   // as active watch target.
-  const status: QueryTargetState = shouldListenToRemote
-    ? syncEngineImpl.sharedClientState.addLocalQueryTarget(targetId)
-    : 'not-current';
+  const status: QueryTargetState =
+    syncEngineImpl.sharedClientState.addLocalQueryTarget(
+      targetId,
+      /* addToActiveTargetIds= */ shouldListenToRemote
+    );
 
   let viewSnapshot;
   if (shouldInitializeView) {

diff --git a/packages/firestore/src/local/shared_client_state.ts b/packages/firestore/src/local/shared_client_state.ts
@@ -104,7 +104,10 @@ export interface SharedClientState {
    * If the target id is already associated with local client, the method simply
    * returns its `QueryTargetState`.
    */
-  addLocalQueryTarget(targetId: TargetId): QueryTargetState;
+  addLocalQueryTarget(
+    targetId: TargetId,
+    addToActiveTargetIds?: boolean
+  ): QueryTargetState;
 
   /** Removes the Query Target ID association from the local client. */
   removeLocalQueryTarget(targetId: TargetId): void;
@@ -655,7 +658,10 @@ export class WebStorageSharedClientState implements SharedClientState {
     this.removeMutationState(batchId);
   }
 
-  addLocalQueryTarget(targetId: TargetId): QueryTargetState {
+  addLocalQueryTarget(
+    targetId: TargetId,
+    addToActiveTargetIds: boolean = true
+  ): QueryTargetState {
     let queryState: QueryTargetState = 'not-current';
 
     // Lookup an existing query state if the target ID was already registered
@@ -676,9 +682,13 @@ export class WebStorageSharedClientState implements SharedClientState {
       }
     }
 
-    this.localClientState.addQueryTarget(targetId);
-    this.persistClientState();
+    // If the query is listening to cache only, the target ID should not be registered with the
+    // local Firestore client as an active watch target.
+    if (addToActiveTargetIds) {
+      this.localClientState.addQueryTarget(targetId);
+    }
 
+    this.persistClientState();
     return queryState;
   }
 
@@ -1110,8 +1120,13 @@ export class MemorySharedClientState implements SharedClientState {
     // No op.
   }
 
-  addLocalQueryTarget(targetId: TargetId): QueryTargetState {
-    this.localState.addQueryTarget(targetId);
+  addLocalQueryTarget(
+    targetId: TargetId,
+    addToActiveTargetIds: boolean = true
+  ): QueryTargetState {
+    if (addToActiveTargetIds) {
+      this.localState.addQueryTarget(targetId);
+    }
     return this.queryState[targetId] || 'not-current';
   }
 

diff --git a/packages/firestore/test/unit/specs/listen_source_spec.test.ts b/packages/firestore/test/unit/specs/listen_source_spec.test.ts
@@ -491,10 +491,10 @@ describeSpec('Listens source options:', [], () => {
           // Listen to cache in secondary clients
           .client(1)
           .userListensToCache(query1)
-          .expectEvents(query1, { added: [docA], fromCache: true })
+          .expectEvents(query1, { added: [docA] })
           .client(2)
           .userListensToCache(query1)
-          .expectEvents(query1, { added: [docA], fromCache: true })
+          .expectEvents(query1, { added: [docA] })
           // Updates in the primary client notifies listeners sourcing from cache
           // in secondary clients.
           .client(0)
@@ -748,7 +748,7 @@ describeSpec('Listens source options:', [], () => {
           //Listen to cache in secondary client
           .client(1)
           .userListensToCache(limitToLast)
-          .expectEvents(limitToLast, { added: [docB, docA], fromCache: true })
+          .expectEvents(limitToLast, { added: [docB, docA] })
           // Watch sends document changes
           .client(0)
           .watchSends({ affects: [limit] }, docC)
@@ -794,7 +794,7 @@ describeSpec('Listens source options:', [], () => {
           // Listen to cache in primary client
           .client(0)
           .userListensToCache(limitToLast)
-          .expectEvents(limitToLast, { added: [docB, docA], fromCache: true })
+          .expectEvents(limitToLast, { added: [docB, docA] })
           // Watch sends document changes
           .watchSends({ affects: [limit] }, docC)
           .watchSnapshots(2000)
@@ -825,16 +825,15 @@ describeSpec('Listens source options:', [], () => {
           // Listen to cache in secondary client
           .client(1)
           .userListensToCache(query1)
-          .expectEvents(query1, { added: [docA], fromCache: true })
+          .expectEvents(query1, { added: [docA] })
           .client(0)
           .userUnlistens(query1)
           .userSets('collection/b', { key: 'b' })
           // The other listener should work as expected
           .client(1)
           .expectEvents(query1, {
             hasPendingWrites: true,
-            added: [docB],
-            fromCache: true
+            added: [docB]
           })
           .userUnlistensToCache(query1)
       );