resolve comments

Author: milaGGL

packages/firestore/src/core/firestore_client.ts

@@ -729,7 +729,7 @@ async function executeQueryFromCache(
     const viewDocChanges = view.computeDocChanges(queryResult.documents);
     const viewChange = view.applyChanges(
       viewDocChanges,
-      /* updateLimboDocuments= */ false
+      /* limboResolutionEnabled= */ false
     );
     result.resolve(viewChange.snapshot!);
   } catch (e) {

packages/firestore/src/core/sync_engine_impl.ts

@@ -368,7 +368,7 @@ async function initializeViewAndComputeSnapshot(
     );
   const viewChange = view.applyChanges(
     viewDocChanges,
-    /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient,
+    /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient,
     synthesizedTargetChange
   );
   updateTrackedLimbos(syncEngineImpl, targetId, viewChange.limboChanges);
@@ -1081,13 +1081,13 @@ async function applyDocChanges(
 
   const targetChange =
     remoteEvent && remoteEvent.targetChanges.get(queryView.targetId);
-  const waitForRequeryResult =
+  const targetIsPendingReset =
     remoteEvent && remoteEvent.targetMismatches.get(queryView.targetId) != null;
   const viewChange = queryView.view.applyChanges(
     viewDocChanges,
-    /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient,
+    /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient,
     targetChange,
-    waitForRequeryResult
+    targetIsPendingReset
   );
   updateTrackedLimbos(
     syncEngineImpl,

packages/firestore/src/core/view.ts

@@ -271,20 +271,21 @@ export class View {
    * Updates the view with the given ViewDocumentChanges and optionally updates
    * limbo docs and sync state from the provided target change.
    * @param docChanges - The set of changes to make to the view's docs.
-   * @param updateLimboDocuments - Whether to update limbo documents based on
+   * @param limboResolutionEnabled - Whether to update limbo documents based on
    *        this change.
    * @param targetChange - A target change to apply for computing limbo docs and
    *        sync state.
-   * @param waitForRequeryResult - Whether the target is pending to run a full
-   *        re-query due to existence filter mismatch.
+   * @param targetIsPendingReset - Whether the target is pending to reset due to
+   *        existence filter mismatch. If not explicitly specified, it is treated
+   *        equivalently to false.
    * @returns A new ViewChange with the given docs, changes, and sync state.
    */
   // PORTING NOTE: The iOS/Android clients always compute limbo document changes.
   applyChanges(
     docChanges: ViewDocumentChanges,
-    updateLimboDocuments: boolean,
+    limboResolutionEnabled: boolean,
     targetChange?: TargetChange,
-    waitForRequeryResult?: boolean
+    targetIsPendingReset?: boolean
   ): ViewChange {
     debugAssert(
       !docChanges.needsRefill,
@@ -303,15 +304,17 @@ export class View {
     });
 
     this.applyTargetChange(targetChange);
-    const limboChanges = updateLimboDocuments
-      ? this.updateLimboDocuments(waitForRequeryResult ?? false)
-      : [];
+
+    targetIsPendingReset = targetIsPendingReset ?? false;
+    const limboChanges =
+      limboResolutionEnabled && !targetIsPendingReset
+        ? this.updateLimboDocuments()
+        : [];
 
     // We are at synced state if there is no limbo docs are waiting to be resolved, view is current
-    // with the backend, and the query is not pending for full re-query result due to existence
-    // filter mismatch.
+    // with the backend, and the query is not pending to reset due to existence filter mismatch.
     const synced =
-      this.limboDocuments.size === 0 && this.current && !waitForRequeryResult;
+      this.limboDocuments.size === 0 && this.current && !targetIsPendingReset;
 
     const newSyncState = synced ? SyncState.Synced : SyncState.Local;
     const syncStateChanged = newSyncState !== this.syncState;
@@ -359,7 +362,7 @@ export class View {
           mutatedKeys: this.mutatedKeys,
           needsRefill: false
         },
-        /* updateLimboDocuments= */ false
+        /* limboResolutionEnabled= */ false
       );
     } else {
       // No effect, just return a no-op ViewChange.
@@ -412,11 +415,9 @@ export class View {
     }
   }
 
-  private updateLimboDocuments(
-    waitForRequeryResult: boolean
-  ): LimboDocumentChange[] {
+  private updateLimboDocuments(): LimboDocumentChange[] {
     // We can only determine limbo documents when we're in-sync with the server.
-    if (waitForRequeryResult || !this.current) {
+    if (!this.current) {
       return [];
     }
 
@@ -469,7 +470,7 @@ export class View {
     this._syncedDocuments = queryResult.remoteKeys;
     this.limboDocuments = documentKeySet();
     const docChanges = this.computeDocChanges(queryResult.documents);
-    return this.applyChanges(docChanges, /*updateLimboDocuments=*/ true);
+    return this.applyChanges(docChanges, /* limboResolutionEnabled= */ true);
   }
 
   /**

packages/firestore/test/unit/core/view.test.ts

@@ -304,19 +304,19 @@ describe('View', () => {
     let changes = view.computeDocChanges(documentUpdates(doc1));
     let viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true
+      /* limboResolutionEnabled= */ true
     );
     expect(viewChange.snapshot!.fromCache).to.be.true;
 
     // Add doc2 to generate a snapshot. Doc1 is still missing.
     changes = view.computeDocChanges(documentUpdates(doc2));
-    viewChange = view.applyChanges(changes, /* updateLimboDocuments= */ true);
+    viewChange = view.applyChanges(changes, /* limboResolutionEnabled= */ true);
     expect(viewChange.snapshot!.fromCache).to.be.true;
 
     // Add doc2 to the backend's result set.
     viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true,
+      /* limboResolutionEnabled= */ true,
       updateMapping(version(0), [doc2], [], [], /* current= */ true)
     );
     // We are CURRENT but doc1 is in limbo.
@@ -325,7 +325,7 @@ describe('View', () => {
     // Add doc1 to the backend's result set.
     viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true,
+      /* limboResolutionEnabled= */ true,
       updateMapping(version(0), [doc1], [], [], /* current= */ true)
     );
     expect(viewChange.snapshot!.fromCache).to.be.false;

packages/firestore/test/unit/local/query_engine.test.ts

@@ -253,7 +253,7 @@ function genericQueryEngineTest(
               const viewDocChanges = view.computeDocChanges(docs);
               return view.applyChanges(
                 viewDocChanges,
-                /*updateLimboDocuments=*/ true
+                /* limboResolutionEnabled= */ true
               ).snapshot!.docs;
             });
         });

packages/firestore/test/unit/specs/limbo_spec.test.ts

@@ -928,7 +928,7 @@ describeSpec('Limbo Documents:', [], () => {
     }
   );
 
-  // Reproduce the bug in b/238823695
+  // Regression test for the bug in https://github.com/firebase/firebase-android-sdk/issues/5357
   specTest(
     'Limbo resolution should wait for full re-query result if there is an existence filter mismatch ',
     [],
@@ -949,8 +949,6 @@ describeSpec('Limbo Documents:', [], () => {
           .watchAcks(query1)
           // DocB is deleted in the next sync.
           .watchFilters([query1], [docA.key])
-          // Bugged behavior: Missing watchCurrent here will move
-          // all the docs to limbo unnecessarily.
           .watchCurrents(query1, 'resume-token-2000')
           .watchSnapshots(2000)
           .expectActiveTargets({
@@ -962,7 +960,7 @@ describeSpec('Limbo Documents:', [], () => {
           .watchAcksFull(query1, 3000, docA)
           // Only the deleted doc is moved to limbo after re-query result.
           .expectLimboDocs(docB.key)
-          .ackLimbo(3000, deletedDoc('collection/b', 3000))
+          .ackLimbo(3000, deletedDoc(docB.key.toString(), 3000))
           .expectLimboDocs()
           .expectEvents(query1, {
             removed: [docB]