fix(db): boundary expansion for multi-column orderBy pagination#1556
Open
v-anton wants to merge 3 commits into
Open
fix(db): boundary expansion for multi-column orderBy pagination#1556v-anton wants to merge 3 commits into
v-anton wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR expands pagination boundaries for multi-column ChangesBoundary Expansion for Multi-Column OrderBy
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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/tests/boundary-expansion-multi-orderby.test.ts`:
- Around line 12-154: Add new test cases in the same test suite to cover
pagination/async corner cases: create additional it() cases using
createCollection + mockSyncCollectionOptions and createLiveQueryCollection that
(1) exercise limit edge values (limit 0, limit 1, limit equal to total rows) and
assert results.length and ids, (2) use an offset beyond available rows (e.g.,
call .offset(...) on the query) and assert an empty result set, and (3) simulate
async/race ordering by inserting or updating items after liveQuery.preload() and
resolving promises using flushPromises() to ensure orderBy + limit still returns
the correct top-K; use the same helper symbols (createCollection,
mockSyncCollectionOptions, createLiveQueryCollection, preload, flushPromises,
orderBy, limit, offset, select) so the tests run alongside existing cases and
lock the pagination boundary-expansion logic against regressions.
🪄 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: 75e1b86e-f39d-4813-82ce-42491c0fbe28
📒 Files selected for processing (3)
.changeset/fix-boundary-expansion-multi-orderby.mdpackages/db/src/collection/subscription.tspackages/db/tests/boundary-expansion-multi-orderby.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/db/tests/boundary-expansion-multi-orderby.test.ts (1)
154-292:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the remaining pagination edge-case tests (
limit(0)and out-of-rangeoffset).Great additions for
limit(1),limit == total, and dynamic insert. One gap remains: this suite still doesn’t coverlimit(0)andoffsetbeyond result length, which are important regressions for paginated top-K behavior.Proposed test additions
+ it(`should return no rows when limit is 0`, async () => { + const initialData: Array<Item> = [ + { id: `a1`, category: `A`, name: `Zeta` }, + { id: `a2`, category: `A`, name: `Alpha` }, + { id: `b1`, category: `B`, name: `Gamma` }, + ] + + const sourceCollection = createCollection( + mockSyncCollectionOptions({ + id: `boundary-limit-0`, + getKey: (item: Item) => item.id, + initialData, + autoIndex: `eager`, + }), + ) + + await sourceCollection.preload() + + const liveQuery = createLiveQueryCollection((q) => + q + .from({ items: sourceCollection }) + .orderBy(({ items }) => items.category, `asc`) + .orderBy(({ items }) => items.name, `asc`) + .limit(0) + .select(({ items }) => ({ + id: items.id, + category: items.category, + name: items.name, + })), + ) + + await liveQuery.preload() + await flushPromises() + expect(Array.from(liveQuery.values())).toEqual([]) + }) + + it(`should return no rows when offset exceeds available rows`, async () => { + const initialData: Array<Item> = [ + { id: `a1`, category: `A`, name: `Zeta` }, + { id: `a2`, category: `A`, name: `Alpha` }, + { id: `b1`, category: `B`, name: `Gamma` }, + ] + + const sourceCollection = createCollection( + mockSyncCollectionOptions({ + id: `boundary-offset-out-of-range`, + getKey: (item: Item) => item.id, + initialData, + autoIndex: `eager`, + }), + ) + + await sourceCollection.preload() + + const liveQuery = createLiveQueryCollection((q) => + q + .from({ items: sourceCollection }) + .orderBy(({ items }) => items.category, `asc`) + .orderBy(({ items }) => items.name, `asc`) + .offset(10) + .limit(2) + .select(({ items }) => ({ + id: items.id, + category: items.category, + name: items.name, + })), + ) + + await liveQuery.preload() + await flushPromises() + expect(Array.from(liveQuery.values())).toEqual([]) + })As per coding guidelines,
**/*.test.{ts,tsx,js}: “Test corner cases including: ... and limit/offset edge cases”.🤖 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/boundary-expansion-multi-orderby.test.ts` around lines 154 - 292, Add two tests to cover pagination edge cases: one "limit(0)" test and one "offset beyond results" test. For each, mirror the existing pattern using createCollection(mockSyncCollectionOptions(...)) and createLiveQueryCollection where you .from({ items: sourceCollection }).orderBy(({ items }) => items.category, 'asc').orderBy(({ items }) => items.name, 'asc') and .select(({ items }) => ({ id: items.id, category: items.category, name: items.name })); in the limit(0) test call .limit(0) and assert Array.from(liveQuery.values()) has length 0; in the offset-out-of-range test call .limit(n) with a reasonable n and .offset(valueGreaterThanTotal) then assert values() is empty, making sure to await sourceCollection.preload(), liveQuery.preload() and flushPromises() as in the other tests.
🤖 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/boundary-expansion-multi-orderby.test.ts`:
- Around line 154-292: Add two tests to cover pagination edge cases: one
"limit(0)" test and one "offset beyond results" test. For each, mirror the
existing pattern using createCollection(mockSyncCollectionOptions(...)) and
createLiveQueryCollection where you .from({ items: sourceCollection
}).orderBy(({ items }) => items.category, 'asc').orderBy(({ items }) =>
items.name, 'asc') and .select(({ items }) => ({ id: items.id, category:
items.category, name: items.name })); in the limit(0) test call .limit(0) and
assert Array.from(liveQuery.values()) has length 0; in the offset-out-of-range
test call .limit(n) with a reasonable n and .offset(valueGreaterThanTotal) then
assert values() is empty, making sure to await sourceCollection.preload(),
liveQuery.preload() and flushPromises() as in the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf545383-af0a-400c-953a-b10ab693cb50
📒 Files selected for processing (1)
packages/db/tests/boundary-expansion-multi-orderby.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/boundary-expansion-multi-orderby.test.ts (1)
301-311: ⚡ Quick winExtract duplicated source setup into a small helper.
These new edge-case tests repeat the same collection bootstrap/preload block. A shared helper would keep each test focused on the pagination assertion and reduce drift.
♻️ Suggested refactor
+const createSeededSourceCollection = async ( + id: string, + initialData: Array<Item>, +) => { + const sourceCollection = createCollection( + mockSyncCollectionOptions({ + id, + getKey: (item: Item) => item.id, + initialData, + autoIndex: `eager`, + }), + ) + await sourceCollection.preload() + return sourceCollection +}Then replace each repeated block with:
-const sourceCollection = createCollection( - mockSyncCollectionOptions({ - id: `...`, - getKey: (item: Item) => item.id, - initialData, - autoIndex: `eager`, - }), -) -await sourceCollection.preload() +const sourceCollection = await createSeededSourceCollection(`...`, initialData)As per coding guidelines,
**/*.{ts,tsx,js}: “Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places” and “Prefer small, focused utility functions over large inline implementations”.Also applies to: 340-350
🤖 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/boundary-expansion-multi-orderby.test.ts` around lines 301 - 311, Multiple tests duplicate the collection bootstrap/preload block; extract that repeated setup into a small helper (e.g., createAndPreloadSourceCollection) that accepts parameters used in the diff (mockSyncCollectionOptions inputs like id, getKey, initialData, autoIndex) and returns the preloaded collection; replace the repeated blocks that call createCollection(... mockSyncCollectionOptions(...)) followed by await sourceCollection.preload() with calls to this helper. Ensure the helper calls createCollection and awaits sourceCollection.preload() internally and keep the same return type so existing assertions using the collection continue to work.
🤖 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.
Nitpick comments:
In `@packages/db/tests/boundary-expansion-multi-orderby.test.ts`:
- Around line 301-311: Multiple tests duplicate the collection bootstrap/preload
block; extract that repeated setup into a small helper (e.g.,
createAndPreloadSourceCollection) that accepts parameters used in the diff
(mockSyncCollectionOptions inputs like id, getKey, initialData, autoIndex) and
returns the preloaded collection; replace the repeated blocks that call
createCollection(... mockSyncCollectionOptions(...)) followed by await
sourceCollection.preload() with calls to this helper. Ensure the helper calls
createCollection and awaits sourceCollection.preload() internally and keep the
same return type so existing assertions using the collection continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b058005-b217-4c60-b664-c2d005793c95
📒 Files selected for processing (1)
packages/db/tests/boundary-expansion-multi-orderby.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
loadNextItemsinCollectionSubscriptionuses a BTree index that orders by(first_col, _id). For multi-columnorderBy, items sharing the same first-column value as the boundary but not selected by BTree'stake()may rank higher by the full comparator. These missing candidates cause D2 to pick incorrect items for the top-K window.The fix adds a boundary expansion step after
index.take()that fetches all items matching the boundary's first-column value and sends them as additional candidates to D2.✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit