fix(solid-db): set key option to $key when calling reconcile#1598
fix(solid-db): set key option to $key when calling reconcile#1598Leonabcd123 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesuseLiveQuery reconciliation fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 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 |
Leonabcd123
changed the title
![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
🤖 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 @.changeset/solid-id-rendering.md:
- Line 5: The release note in the changeset file contains two typos that need
correction on line 5: the phrase "that that" appears as a duplicate word and
should be simplified to "that", and the word "reconcilation" is misspelled and
should be corrected to "reconciliation". Update the text to fix both instances
so the release note reads clearly and professionally.
🪄 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: e433e653-ee95-4104-8db2-eb84d69e6a44
📒 Files selected for processing (3)
.changeset/solid-id-rendering.mdpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/useLiveQuery.test.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/solid-db/tests/useLiveQuery.test.tsx (1)
2609-2639:⚠️ Potential issue | 🟠 MajorWrap
useLiveQueryin a managed reactive root to avoid leaked subscriptions.This test directly instantiates
useLiveQuerywithout a reactive owner context. In Solid.js, effects and signals created byuseLiveQuerymust be disposed properly to prevent subscription leakage between tests. UserenderHook(the established pattern throughout this file) orcreateRootwith cleanup to ensure proper lifecycle management.Suggested fix
- const query = useLiveQuery((q) => - q - .from({ items: collection }) - .orderBy(({ items }) => items.name, `asc`), - ) + const rendered = renderHook(() => + useLiveQuery((q) => + q + .from({ items: collection }) + .orderBy(({ items }) => items.name, `asc`), + ), + ) await waitFor(() => { - expect(query.isReady).toBe(true) + expect(rendered.result.isReady).toBe(true) }) expect( - Array.from(query()).map((item) => item.name), + Array.from(rendered.result()).map((item) => item.name), ).toEqual([`Bob`, `Kevin`, `Stuart`]) collection.utils.begin() collection.utils.write({ type: `update`, value: { _id: `stuart1`, name: `Alvin`, }, }) collection.utils.commit() await waitFor(() => { expect( - Array.from(query()).map((item) => item.name), + Array.from(rendered.result()).map((item) => item.name), ).toEqual([`Alvin`, `Bob`, `Kevin`]) })🤖 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/solid-db/tests/useLiveQuery.test.tsx` around lines 2609 - 2639, The test is directly calling useLiveQuery without a managed reactive root context, which can leak subscriptions between tests in Solid.js. Wrap the useLiveQuery call and all subsequent query operations in either renderHook (the established pattern used elsewhere in this test file) or createRoot with proper cleanup to ensure effects and signals are disposed correctly. This ensures the reactive context is properly initialized and cleaned up after the test completes, preventing subscription leakage.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.
Outside diff comments:
In `@packages/solid-db/tests/useLiveQuery.test.tsx`:
- Around line 2609-2639: The test is directly calling useLiveQuery without a
managed reactive root context, which can leak subscriptions between tests in
Solid.js. Wrap the useLiveQuery call and all subsequent query operations in
either renderHook (the established pattern used elsewhere in this test file) or
createRoot with proper cleanup to ensure effects and signals are disposed
correctly. This ensures the reactive context is properly initialized and cleaned
up after the test completes, preventing subscription leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c00179c5-f4e2-4a12-8731-4007dfcd934c
📒 Files selected for processing (1)
packages/solid-db/tests/useLiveQuery.test.tsx
1 participant
Fixes #1524.
🎯 Changes
Add
keyoption when callingreconcilewhich is set to$key, which makes it so items are matched correctly even when theidproperty doesn't exist.✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
useLiveQueryreconciliation so list items are matched using the correct key field, preventing incorrect item reuse during updates and reordering.Tests
useLiveQuerywith a custom item key, confirming that sorted results update to the expected order after an in-collection change.