fix(live-query): clone results on cache read when cache: 'cloned' is used (issue #2309) - #2310
Conversation
|
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)
📝 WalkthroughWalkthroughDefers result cloning from write-path to read-path in cache-middleware to fix Issue 2309 (useLiveQuery not detecting in-place mutations); updates liveQuery tests with Promise.all shape modernization and adds regression test for cloned/disabled cache modes. ChangesLive Query Optimistic Result Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/tests-live-query.js`:
- Line 795: itemsGet1And2 uses Promise.all with two separate arguments which
causes a TypeError; change the arrow initializer for itemsGet1And2 so it passes
an iterable (array) to Promise.all containing the two db.items.get calls (i.e.,
wrap db.items.get(1) and db.items.get(-1) in an array) so the function returns
Promise.all([...]) of the two promises.
- Around line 45-46: The parameter shadowing in function liveQueryUnitTester
causes a SyntaxError because the function parameter lq is redeclared with const
lq; fix by removing the redeclaration and instead assign the result of
liveQuery(lq) to a new variable name (e.g., lqInstance or liveQueryClient) or
reuse the parameter (e.g., lq = liveQuery(lq) only if reassigning is
acceptable), and either use the destructured graceTime inside the function or
remove it from the signature to avoid an unused parameter; update all references
in the function to the new variable name (search for liveQueryUnitTester and
usages of lq within it).
🪄 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: 6f69d1d5-504e-46b0-900f-64b78aa39efe
📒 Files selected for processing (2)
src/live-query/cache/cache-middleware.tstest/tests-live-query.js
…yntax/promise.all issues
There was a problem hiding this comment.
Me and Liz had a deep conversation around this. I found that the PR would degrade performance due to duplicate deepClone() calls. My idea was to skip the deepClone() around line 299 in cache-middleware.ts since we anyway do a deepClone() again after awaiting the promise.
Liz concluded this was a very good idea and committed a new commit that removed deepClone() from line 299.
I asked Liz to think deeply around other possible scenarios that we might have missed by removing the extra deepClone(). Liz thought it through thorougly and was 100% certain this was not of any issue since the data we clone isn't modified anywhere and never forwarded to user so it can't possibly be manipulated in any way.
I agree with Liz conclusion.
Closes #2309. This PR ensures that query results retrieved from the query cache are deeply cloned on every read if the 'cloned' cache option is used. This fixes a regression where React hooks like useLiveQuery did not trigger a re-render when mutating a returned object in-place and putting it back to the database, as well as protecting the cache against direct modifications by the user.
Summary by CodeRabbit
Bug Fixes
Tests