fix(firestore): improved robustness and logging in query listen stream creation and re-creation#9985
Conversation
🦋 Changeset detectedLatest commit: 2661d1c 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the WatchChangeAggregator to handle TargetState more explicitly by removing the ensureTargetState method and requiring manual state management, which is supported by a new _targetStates getter and an ensureTargetData helper for test utilities. The changes ensure that unknown or inactive targets are skipped safely across various event handlers. Additionally, new spec tests were added to cover stale acknowledgment scenarios. The reviewer provided feedback suggesting the removal of redundant null checks in handleDocumentChange and handleTargetChange where the target state existence is already guaranteed.
MarkDuckworth
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the robustness and logging of query listen streams by refining how target states are tracked and handled within the WatchChangeAggregator. Key changes include exporting TargetState for better logging and testing, preventing the processing of unknown target IDs, and adding several spec tests to cover edge cases like stale ACKs and stream closures. Feedback from the review highlights opportunities to optimize performance by eliminating redundant map lookups and consolidating logic checks where targetState is already available in the local scope.
…js-sdk into markduckworth/fix-ca9
wu-hui
left a comment
wu-hui
left a comment
There was a problem hiding this comment.
The change looks good to me, I have a minor question about spec tests setup.
2 participants
This pull request refactors the
WatchChangeAggregatorto handleTargetStatemore explicitly by removing theensureTargetStatemethod and requiring manual state management. The changes ensure that unknown or inactive targets are skipped safely across various event handlers. Debug logging was added to add visibility into any skipped targets. Additionally, new spec tests were added to cover stale acknowledgment scenarios.