fix(db): emit change on order-only reorder in ordered live queries#1601
fix(db): emit change on order-only reorder in ordered live queries#1601v-anton wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughExtends ChangesorderBy reorder emission for live queries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
🚥 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 |
Relationship to other open
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/db/tests/live-query-orderby-reorder.test.ts (2)
128-153: ⚡ Quick winAdd explicit
limit/offsetboundary tests for reorder behavior.You already cover a standard
limitpath; please addlimit(0)and offset-beyond-length cases to lock down empty-window semantics during reorder updates.As per coding guidelines, "Handle limit and offset edge cases: consider what happens when limit is 0, undefined, or when offset exceeds data length" and test corner cases in
*.test.ts.🤖 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/live-query-orderby-reorder.test.ts` around lines 128 - 153, The current test covers a standard limit case with limit(3). Add two additional test cases following the same pattern: one testing the reorder behavior when limit(0) is used to create an empty result window, and another testing when offset exceeds the data length to verify empty-window semantics during reorder updates. Use the same manualCollection, createLiveQueryCollection, write, and subscribeChanges pattern to verify that reorder updates are handled correctly when the query window is empty or beyond available data boundaries.Source: Coding guidelines
9-9: ⚡ Quick winRemove
anyfrom test helper/projection paths and use explicit test-local types.Typed helper contracts and projected row types keep regressions catchable at compile-time and make assertions safer.
As per coding guidelines, "Avoid using
anytypes; useunknowninstead when the type is truly unknown, and provide proper type annotations for return values".Also applies to: 54-55, 87-87, 95-95, 124-124, 140-140, 152-152
🤖 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/live-query-orderby-reorder.test.ts` at line 9, Replace all instances of the `any` type throughout the file (found at the variable declaration `let fns: any` and at the other locations noted in lines 54-55, 87, 95, 124, 140, 152) with explicit type annotations. For test helper functions and projection row types, define appropriate test-local types that clearly express what these variables and functions should return, rather than using `any` or `unknown`. This ensures better type safety and makes assertions more robust by allowing the compiler to catch type-related regressions.Source: Coding guidelines
packages/db/src/query/live/collection-config-builder.ts (1)
1024-1030: 🏗️ Heavy liftAvoid reaching into
_changesviaany; delegate through a typed internal method.This hard-couples
CollectionConfigBuildertoCollectioninternals and weakens compile-time safety. Prefer an internal delegate onCollection/sync methods for forced emits, then call that API here.♻️ Proposed direction
- const changesManager = (config.collection as any)._changes as { - emitEvents: ( - changes: Array<ChangeMessage<TResult>>, - forceEmit?: boolean, - ) => void - } - changesManager.emitEvents(moves, true) + config.collection.emitInternalChanges(moves, { forceEmit: true })As per coding guidelines, "Encapsulate implementation details within responsible classes; use delegation to maintain clean boundaries between components" and "Avoid exposing internal properties directly; instead add public methods that delegate to internal implementations".
🤖 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/live/collection-config-builder.ts` around lines 1024 - 1030, The code directly accesses the private _changes property on the collection object through any type casting to call emitEvents, which breaks encapsulation and type safety. Instead of reaching into _changes via any, create an internal delegate method on the Collection class (such as emitChangesForced or similar) that encapsulates the _changes.emitEvents call, then replace the direct changesManager assignment and emitEvents invocation in the current code with a call to this new internal method on the collection object. This maintains clean boundaries between components while preserving the forced emit behavior.Source: Coding guidelines
packages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx (2)
37-37: ⚡ Quick winReplace
anyin result mapping with a concrete row type.This keeps hook assertions type-safe and avoids masking contract drift in
useLiveQuerydata shape.As per coding guidelines, "Avoid using
anytypes; useunknowninstead when the type is truly unknown, and provide proper type annotations for return values".Also applies to: 44-44
🤖 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/react-db/tests/useLiveQuery-orderby-reorder.test.tsx` at line 37, The map function in the result.current.data transformation uses `any` type annotation for the row parameter, which reduces type safety. Identify the concrete row type that useLiveQuery returns (likely defined in your test setup or data models), and replace the `any` type annotation with that concrete type in both the mapping operation at line 37 and the additional occurrence at line 44. This ensures type-safe assertions and prevents masking any contract drift in the useLiveQuery data shape.Source: Coding guidelines
21-47: ⚡ Quick winAdd a rapid consecutive updates test to cover hook-level async race ordering.
A case with back-to-back writes that reorder in opposite directions would strengthen confidence that stale order does not render between emissions.
As per coding guidelines, test corner cases including "async race conditions where operations may resolve in unexpected order".
🤖 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/react-db/tests/useLiveQuery-orderby-reorder.test.tsx` around lines 21 - 47, Add a new test case within the existing describe block for useLiveQuery orderBy + select(id-only) that tests rapid consecutive updates with reordering in opposite directions. This test should perform multiple back-to-back writes using act() that cause the items to reorder in alternating directions (for example, one update that moves an item to the top, followed immediately by another that moves it to the bottom), and then verify using waitFor() that the final rendered order matches the expected result without any stale intermediate orders appearing. This will ensure the hook properly handles async race conditions where multiple reordering operations occur in quick succession.Source: Coding guidelines
🤖 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/live-query-orderby-reorder.test.ts`:
- Line 4: The `flush` function uses a hardcoded setTimeout with a fixed 20ms
delay, making tests timing-sensitive under CI load. Instead of relying on this
sleep-based approach, replace the `flush` function implementation to use
condition-based waits by leveraging testing utilities like `waitFor` to
explicitly wait on observable state changes. Identify the specific order or
event expectations that each test needs to verify after calling `flush`, and
replace the fixed sleep with explicit waits on those observable state conditions
to ensure reliable test execution regardless of system load.
---
Nitpick comments:
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 1024-1030: The code directly accesses the private _changes
property on the collection object through any type casting to call emitEvents,
which breaks encapsulation and type safety. Instead of reaching into _changes
via any, create an internal delegate method on the Collection class (such as
emitChangesForced or similar) that encapsulates the _changes.emitEvents call,
then replace the direct changesManager assignment and emitEvents invocation in
the current code with a call to this new internal method on the collection
object. This maintains clean boundaries between components while preserving the
forced emit behavior.
In `@packages/db/tests/live-query-orderby-reorder.test.ts`:
- Around line 128-153: The current test covers a standard limit case with
limit(3). Add two additional test cases following the same pattern: one testing
the reorder behavior when limit(0) is used to create an empty result window, and
another testing when offset exceeds the data length to verify empty-window
semantics during reorder updates. Use the same manualCollection,
createLiveQueryCollection, write, and subscribeChanges pattern to verify that
reorder updates are handled correctly when the query window is empty or beyond
available data boundaries.
- Line 9: Replace all instances of the `any` type throughout the file (found at
the variable declaration `let fns: any` and at the other locations noted in
lines 54-55, 87, 95, 124, 140, 152) with explicit type annotations. For test
helper functions and projection row types, define appropriate test-local types
that clearly express what these variables and functions should return, rather
than using `any` or `unknown`. This ensures better type safety and makes
assertions more robust by allowing the compiler to catch type-related
regressions.
In `@packages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx`:
- Line 37: The map function in the result.current.data transformation uses `any`
type annotation for the row parameter, which reduces type safety. Identify the
concrete row type that useLiveQuery returns (likely defined in your test setup
or data models), and replace the `any` type annotation with that concrete type
in both the mapping operation at line 37 and the additional occurrence at line
44. This ensures type-safe assertions and prevents masking any contract drift in
the useLiveQuery data shape.
- Around line 21-47: Add a new test case within the existing describe block for
useLiveQuery orderBy + select(id-only) that tests rapid consecutive updates with
reordering in opposite directions. This test should perform multiple
back-to-back writes using act() that cause the items to reorder in alternating
directions (for example, one update that moves an item to the top, followed
immediately by another that moves it to the bottom), and then verify using
waitFor() that the final rendered order matches the expected result without any
stale intermediate orders appearing. This will ensure the hook properly handles
async race conditions where multiple reordering operations occur in quick
succession.
🪄 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: 45978f9c-2f09-4ecd-b14c-7c28e43ad837
📒 Files selected for processing (5)
.changeset/orderby-reorder-emission.mdpackages/db/src/query/live/collection-config-builder.tspackages/db/src/query/live/types.tspackages/db/tests/live-query-orderby-reorder.test.tspackages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx
…ary cases - Drop fixed setTimeout flushes (live query is synchronous on commit) - Type the sync helper and projections instead of any - Add limit(0) and offset-beyond-length empty-window reorder cases
|
Thanks @coderabbitai — addressed the test feedback in
On the |
|
Tip For best results, initiate chat on the files or code changes.
On the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/live-query-orderby-reorder.test.ts (1)
49-203: ⚡ Quick winAdd missing corner-case tests required by test guidelines (single-element + async race).
This suite now covers the main repro and limit/offset edges, but it still misses guideline-required corner coverage for a single-element collection and an async race/ordering scenario (e.g., back-to-back updates that reorder in opposite directions and assert final order + emitted events are not stale).
As per coding guidelines, "
**/*.test.{ts,tsx,js}: Test corner cases including: ... single-element collections ... async race conditions ... 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/live-query-orderby-reorder.test.ts` around lines 49 - 203, Add two new test cases to the test suite that cover the required corner cases. First, add a single-element collection test using the same pattern as the existing tests (manualCollection with one item, createLiveQueryCollection with orderBy and select, write an update that triggers a reorder, and verify the collection remains correct and emits appropriate events). Second, add an async race condition test that writes two back-to-back updates causing opposite reorders (for example, first update moves an item to the front, then immediately updates another item to push the first item back), and verify that the final order is correct and that the emitted events reflect the actual state transitions without stale or duplicate events.Source: Coding guidelines
🤖 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/live-query-orderby-reorder.test.ts`:
- Around line 49-203: Add two new test cases to the test suite that cover the
required corner cases. First, add a single-element collection test using the
same pattern as the existing tests (manualCollection with one item,
createLiveQueryCollection with orderBy and select, write an update that triggers
a reorder, and verify the collection remains correct and emits appropriate
events). Second, add an async race condition test that writes two back-to-back
updates causing opposite reorders (for example, first update moves an item to
the front, then immediately updates another item to push the first item back),
and verify that the final order is correct and that the emitted events reflect
the actual state transitions without stale or duplicate events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcc9819d-c473-496c-89f0-93da1040c68e
📒 Files selected for processing (1)
packages/db/tests/live-query-orderby-reorder.test.ts
1 participant
🎯 Changes
A live query with
orderBydoes not emit a change when a row's position changes but its projected value does not. For example:When a row's
valuechanges so its position moves, the projected value ({ id }) is byte-identical, so the collection's value-diff (deepEqualsinstate.ts) suppresses the update. The live query'sSortedMapre-sorts internally, butsubscribeChangesemits nothing — souseLiveQuerykeeps rendering the stale order. Any projection that omits the changingorderByfield hits this (it is not specific to id-only).Fix
The
orderByoperator already streams the move as a retract (old value + oldorderByIndex) plus an insert (new value + new index). We capture the retracted side inaccumulateChangesand, after each flush, emit anupdatedirectly only when the index changed and the value is unchanged — the exact case the value-diff swallows. This mirrors the existing includes-materialization direct-emit pattern and leaves generic collection code untouched.orderBy(the gate short-circuits onundefinedindex).state.ts's emit (value changed).✅ Checklist
pnpm test.New tests:
packages/db/tests/live-query-orderby-reorder.test.ts(id-only reorder emits; exactly-one-emit when sort field projected; no spurious emit;limitwindow) andpackages/react-db/tests/useLiveQuery-orderby-reorder.test.tsx(useLiveQueryrenders correct order). Full@tanstack/db(2391) and@tanstack/react-db(95) suites pass.🚀 Release Impact
@tanstack/dbpatch).Summary by CodeRabbit
Bug Fixes
orderByto correctly detect and emit updates when rows are reordered (order-only moves), ensuring UI/order changes are reflected even if projected values don’t change.Tests
orderByreorder test coverage, including projection andlimitedge cases.useLiveQueryhook to verify reordered results update in the rendered output even when the sort field isn’t selected.