fix(svelte-query): preserve array length updates in createRawRef#10836
fix(svelte-query): preserve array length updates in createRawRef#10836raashish1601 wants to merge 5 commits into
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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughBroadcast 'added' messages now include the query ChangesQuery Broadcast Protocol
Svelte Query Array Ref Handling
Solid Query resource-start tracking
Sequence DiagramsequenceDiagram
participant QueryClientSender
participant BroadcastChannel
participant QueryClientReceiver
QueryClientSender->>BroadcastChannel: post { type: "added", queryKey, state }
BroadcastChannel->>QueryClientReceiver: deliver message
QueryClientReceiver->>QueryClientReceiver: if state present -> build/update cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
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/svelte-query/src/containers.svelte.ts (1)
83-96:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix
deletePropertyarray truncation when deleting multiple indicesIn
packages/svelte-query/src/containers.svelte.ts,deleteProperty()decrementstarget.length--for every numeric index delete.update()then removes stale indices by doingdelete out[key]for each key inkeysToRemove; for arrays these keys are processed in ascending index order, so deleting multiple indices truncates the current tail each time. The provided repro ([1,2,3,4] -> [1]) results inlength: 2(with remaining keys[ '0' ]) instead oflength: 1, proving the proxy ends with the wrong array length.Existing tests for
createRawRefonly cover shrinking arrays step-by-step by a single trailing decrement, so the multi-element shrink case is uncovered.The repro in the comment already demonstrates the bug in code matching this implementation; any
createQueriesupdate where thequeriesarray length drops by more than one element will go through the samecreateRawRef.update()deletion path.Want me to draft a
deletePropertyfix that recomputeslengthfrom the remaining visible indices instead of blindly decrementing?🤖 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/svelte-query/src/containers.svelte.ts` around lines 83 - 96, The deleteProperty trap in createRawRef currently decrements target.length for every numeric index delete, causing incorrect lengths when multiple indices are removed; change deleteProperty (and any related logic in createRawRef.update) so that when deleting an array index (isArrayIndex(prop)) you do NOT blindly do target.length-- but instead recompute the correct length from the remaining visible numeric indices: consider target's own keys minus hiddenKeys, find the maximum numeric index (or -1 if none) and set target.length = maxIndex + 1 (or 0), ensuring hiddenKeys are excluded from the computation; keep the existing behavior of setting target[prop] = undefined and adding hiddenKeys.add(prop) but replace the decrement with the recompute step so multi-index removals produce the correct final length.
🧹 Nitpick comments (1)
packages/svelte-query/src/containers.svelte.ts (1)
34-38: ⚡ Quick winHarden
isArrayIndexagainst non-canonical numeric strings
Number.isInteger(Number(value))currently returnstruefor non-canonical inputs like'1e3','0x10', and''/' '(coerced to1000,16, and0), so the current check can treat them as valid array indices. Use a round-tripString(n) === valuecheck.♻️ Canonical index detection
- const isArrayIndex = (value: PropertyKey): value is `${number}` => - typeof value === 'string' && - value !== 'length' && - Number.isInteger(Number(value)) && - Number(value) >= 0 + const isArrayIndex = (value: PropertyKey): value is `${number}` => { + if (typeof value !== 'string') return false + const n = Number(value) + return Number.isInteger(n) && n >= 0 && String(n) === value + }🤖 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/svelte-query/src/containers.svelte.ts` around lines 34 - 38, The isArrayIndex predicate currently treats non-canonical numeric strings as valid (e.g. "1e3", "0x10", ""), so change its logic to parse Number(value) into a variable (e.g., const n = Number(value)) and then require Number.isInteger(n) && n >= 0 && String(n) === value (and retain the typeof value === 'string' && value !== 'length' checks); update the isArrayIndex implementation to use that round-trip String(n) === value equality to ensure only canonical decimal integer strings are accepted.
🤖 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/svelte-query/src/containers.svelte.ts`:
- Around line 83-96: The deleteProperty trap in createRawRef currently
decrements target.length for every numeric index delete, causing incorrect
lengths when multiple indices are removed; change deleteProperty (and any
related logic in createRawRef.update) so that when deleting an array index
(isArrayIndex(prop)) you do NOT blindly do target.length-- but instead recompute
the correct length from the remaining visible numeric indices: consider target's
own keys minus hiddenKeys, find the maximum numeric index (or -1 if none) and
set target.length = maxIndex + 1 (or 0), ensuring hiddenKeys are excluded from
the computation; keep the existing behavior of setting target[prop] = undefined
and adding hiddenKeys.add(prop) but replace the decrement with the recompute
step so multi-index removals produce the correct final length.
---
Nitpick comments:
In `@packages/svelte-query/src/containers.svelte.ts`:
- Around line 34-38: The isArrayIndex predicate currently treats non-canonical
numeric strings as valid (e.g. "1e3", "0x10", ""), so change its logic to parse
Number(value) into a variable (e.g., const n = Number(value)) and then require
Number.isInteger(n) && n >= 0 && String(n) === value (and retain the typeof
value === 'string' && value !== 'length' checks); update the isArrayIndex
implementation to use that round-trip String(n) === value equality to ensure
only canonical decimal integer strings are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8b0cd51-7cbb-4e46-8beb-9dfebd1e63fb
📒 Files selected for processing (5)
packages/query-broadcast-client-experimental/src/__tests__/index.test.tspackages/query-broadcast-client-experimental/src/index.tspackages/svelte-query/src/containers.svelte.tspackages/svelte-query/tests/containers.svelte.test.tspackages/svelte-query/tests/createQueries/createQueries.svelte.test.ts
1 participant
Summary: keep array length in sync when createRawRef adds numeric indices during update; this fixes createQueries activating from an empty array. Validation: pnpm --filter @tanstack/svelte-query test:lib -- --run tests/containers.svelte.test.ts tests/createQueries/createQueries.svelte.test.ts
Summary by CodeRabbit
Bug Fixes
Behavior Change
Tests