fix(db): deduplicate loadedSubsets and join key requests#1554
Open
v-anton wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR deduplicates subset loads: subscription snapshot requests now skip when an existing loaded subset covers the predicate, and join lazy-loading tracks loaded join keys to avoid redundant indexed snapshot requests. Tests and a changeset were added. ChangesPrevent loadedSubsets unbounded growth through deduplication
Sequence Diagram(s): sequenceDiagram
participant Subscription
participant _sync
participant loadedSubsets
Subscription->>loadedSubsets: check isPredicateSubset(existing, new)
alt covered
Subscription-->>Subscription: skip _sync.loadSubset
else not covered
Subscription->>_sync: _sync.loadSubset(loadOptions)
_sync-->>Subscription: onLoadSubsetResult
Subscription->>loadedSubsets: push(loadOptions)
end
sequenceDiagram
participant ActiveTap
participant LazyCollection
participant loadedJoinKeys
ActiveTap->>ActiveTap: collect unique non-null joinKeys
ActiveTap->>loadedJoinKeys: exclude already-loaded keys
alt keysToLoad non-empty
ActiveTap->>LazyCollection: requestSnapshot(where: inArray(keysToLoad))
alt loaded
LazyCollection-->>loadedJoinKeys: record loaded keys
else not-loaded
LazyCollection-->>ActiveTap: fallback to full collection request
end
end
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/collection/subscription.ts (1)
380-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDedup bookkeeping happens after issuing
loadSubset, which can break unload symmetry.
loadSubset(loadOptions)is executed before dedup checks, so duplicate/subset requests can still be issued but not recorded inloadedSubsets. On unsubscribe, those untracked loads won’t get matching unload calls.Suggested fix
- const syncResult = this.collection._sync.loadSubset(loadOptions) - - // Pass the raw loadSubset result to the caller for external tracking - opts?.onLoadSubsetResult?.(syncResult) - - // Track this loadSubset call so we can unload it later const isAlreadyCovered = this.loadedSubsets.some((existing) => isPredicateSubset(loadOptions, existing), ) - if (!isAlreadyCovered) { + if (!isAlreadyCovered) { + const syncResult = this.collection._sync.loadSubset(loadOptions) + opts?.onLoadSubsetResult?.(syncResult) this.loadedSubsets.push(loadOptions) + if (trackLoadSubsetPromise) { + this.trackLoadSubsetPromise(syncResult) + } + } else { + // Optional: report no-op load to caller if needed by caller contract + opts?.onLoadSubsetResult?.(true) } - - const trackLoadSubsetPromise = opts?.trackLoadSubsetPromise ?? true - if (trackLoadSubsetPromise) { - this.trackLoadSubsetPromise(syncResult) - }Apply equivalent ordering in
requestLimitedSnapshotas well.Also applies to: 623-634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/collection/subscription.ts` around lines 380 - 391, Reorder the dedup bookkeeping so you check isAlreadyCovered via isPredicateSubset and update loadedSubsets before calling this.collection._sync.loadSubset — only call loadSubset when not already covered — and still invoke opts?.onLoadSubsetResult with the actual load result when you do call it (or a noop/early-return when skipped); apply the same reordering/logic in requestLimitedSnapshot as well so unload symmetry is preserved for both code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/compiler/joins.ts`:
- Around line 321-325: The code is adding joinKeys into loadedJoinKeys before
confirming the async loadSubset succeeded, which can prevent retries when
loadSubset later rejects; in the requestSnapshot / join handling logic
(referencing requestSnapshot, loadSubset, loadedJoinKeys, and joinKeys), move
the loadedJoinKeys.add(key) updates so they only run after the async
loadSubset/requestSnapshot completes successfully (i.e., inside the success path
or then block), and ensure failed loads do not mark keys as loaded so subsequent
batches can retry.
In `@packages/db/tests/query/loaded-subsets-dedup.test.ts`:
- Around line 164-176: The test currently may pass vacuously if no 'in' filters
are produced; update the loop that iterates newCalls and inspects
extractSimpleComparisons(call.where) to assert that at least one filter with
operator === 'in' was seen. For example, introduce a counter or collect matching
filters (referencing newCalls, extractSimpleComparisons, filters and
filter.operator) and after the loop add an assertion that the count/collection
length is > 0 before relying on contains/not.toContain checks.
- Around line 94-112: The test never triggers the second snapshot request; after
calling await liveQuery.preload() capture initialCallCount as you do, then
invoke and await the second snapshot action (e.g., await
liveQuery.requestSnapshot() or the appropriate method that re-requests subsets)
to actually exercise dedup logic, then assert loadSubsetCalls.length remains
equal to initialCallCount; reference liveQuery.preload(),
liveQuery.requestSnapshot(), and loadSubsetCalls in the change.
- Line 125: Replace the unsafe cast by correctly typing parentWrite: change its
type from the ad-hoc (msg: { type: string; value: Parent }) => void to
ChangeMessageOrDeleteKeyMessage<Parent, number> (import
ChangeMessageOrDeleteKeyMessage from ../../src/types.js) and then assign
parentWrite = write (no "as any"); ensure the generic keyof/number matches the
test's Parent and TKey usage so the assignment type-checks.
---
Outside diff comments:
In `@packages/db/src/collection/subscription.ts`:
- Around line 380-391: Reorder the dedup bookkeeping so you check
isAlreadyCovered via isPredicateSubset and update loadedSubsets before calling
this.collection._sync.loadSubset — only call loadSubset when not already covered
— and still invoke opts?.onLoadSubsetResult with the actual load result when you
do call it (or a noop/early-return when skipped); apply the same
reordering/logic in requestLimitedSnapshot as well so unload symmetry is
preserved for both code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5754dc72-3946-45bc-b96e-1abdc624f6a8
📒 Files selected for processing (4)
.changeset/fix-loaded-subsets-dedup.mdpackages/db/src/collection/subscription.tspackages/db/src/query/compiler/joins.tspackages/db/tests/query/loaded-subsets-dedup.test.ts
Comment on lines
+321
to
+325
| if (loaded) { | ||
| for (const key of joinKeys) { | ||
| loadedJoinKeys.add(key) | ||
| } | ||
| } else { |
Contributor
There was a problem hiding this comment.
loadedJoinKeys is updated before async load success, which can suppress needed retries.
requestSnapshot can return true even if the underlying async loadSubset later rejects. Adding keys immediately means failed keys won’t be retried in later batches.
Suggested fix
- const loaded = lazySourceSubscription.requestSnapshot({
+ let loadResult: Promise<void> | true = true
+ const loaded = lazySourceSubscription.requestSnapshot({
where: inArray(lazyJoinRef, joinKeys),
optimizedOnly: true,
+ onLoadSubsetResult: (result) => {
+ loadResult = result
+ },
})
if (loaded) {
- for (const key of joinKeys) {
- loadedJoinKeys.add(key)
- }
+ if (loadResult instanceof Promise) {
+ loadResult
+ .then(() => {
+ for (const key of joinKeys) loadedJoinKeys.add(key)
+ })
+ .catch(() => {
+ // keep keys retriable
+ })
+ } else {
+ for (const key of joinKeys) loadedJoinKeys.add(key)
+ }
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/query/compiler/joins.ts` around lines 321 - 325, The code is
adding joinKeys into loadedJoinKeys before confirming the async loadSubset
succeeded, which can prevent retries when loadSubset later rejects; in the
requestSnapshot / join handling logic (referencing requestSnapshot, loadSubset,
loadedJoinKeys, and joinKeys), move the loadedJoinKeys.add(key) updates so they
only run after the async loadSubset/requestSnapshot completes successfully
(i.e., inside the success path or then block), and ensure failed loads do not
mark keys as loaded so subsequent batches can retry.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/db/tests/query/loaded-subsets-dedup.test.ts (1)
100-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFirst dedup regression test is still not exercising the second request path.
On Line 100 you call
preload()once, then on Line 117 assert call count didn’t grow, but no second snapshot/preload action is performed. This can still pass vacuously and won’t catch regressions in repeated-request dedupe.As per coding guidelines, "Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/loaded-subsets-dedup.test.ts` around lines 100 - 118, The test only calls liveQuery.preload() once then asserts loadSubsetCalls length unchanged, so it never exercises the repeated-request dedupe path; update the test (around liveQuery.preload(), loadSubsetCalls and extractSimpleComparisons usage) to perform the second preload/snapshot call (e.g., call liveQuery.preload() or the snapshot-triggering helper a second time), capture loadSubsetCalls.length before and after the second call, and assert the call count does not increase and the second request (if present) has the same filters as the first by reusing firstCall = loadSubsetCalls[0] and comparing extractSimpleComparisons on any subsequent call (or asserting no new call was pushed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/db/tests/query/loaded-subsets-dedup.test.ts`:
- Around line 100-118: The test only calls liveQuery.preload() once then asserts
loadSubsetCalls length unchanged, so it never exercises the repeated-request
dedupe path; update the test (around liveQuery.preload(), loadSubsetCalls and
extractSimpleComparisons usage) to perform the second preload/snapshot call
(e.g., call liveQuery.preload() or the snapshot-triggering helper a second
time), capture loadSubsetCalls.length before and after the second call, and
assert the call count does not increase and the second request (if present) has
the same filters as the first by reusing firstCall = loadSubsetCalls[0] and
comparing extractSimpleComparisons on any subsequent call (or asserting no new
call was pushed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3471791a-036d-4347-979d-54625d24bc5f
📒 Files selected for processing (3)
packages/db/src/collection/subscription.tspackages/db/tests/collection-subscription.test.tspackages/db/tests/query/loaded-subsets-dedup.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
Supersedes #1205.
Two related fixes for unbounded
loadedSubsetsgrowth:subscription.ts:
requestSnapshotandloadNextItemsnow checkisPredicateSubsetbefore pushing toloadedSubsets, skipping entries already covered by an existing superset predicate.joins.ts: A
loadedJoinKeysSet tracks keys already loaded across pipeline batches. Subsequent batches filter out known keys before callingrequestSnapshot, preventing repeated requests for the same join keys.Without these fixes, join lazy-loading accumulates duplicate
LoadSubsetOptionsentries, causing SQL queries and query keys to grow unboundedly — eventually hitting "too many SQL variables" errors.✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests