Fix reactivity misuses in the motion component body - #79
Conversation
The merged config/props object was built with `$state` inside a `$derived`. The derived already recomputes on dependency changes, so the inner `$state` added no reactivity - it only allocated a fresh deep proxy over the whole props graph on every recomputation, churning the identity of structures VisualElement holds by reference and deep-proxying user objects. The visual element was also assigned onto the object a `$derived` owns, so a variant change rebuilt that object and left `context.visualElement` undefined until the watcher reassigned it. It now lives in init-time state the context reads through a getter. Also removes a stale commented-out `$effect` block: the visual element is unmounted by the render attachment's teardown, so the note it carried no longer holds. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 54c63fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
commit: |
1 participant
Two reactivity misuses in
createRendererMotionComponent/renderMotionComponent, plus a dead comment block.1.
$state({...})created inside a$derived.byconfigAndPropsbuilt its result with$state. The$derivedalready re-runs whenmotionConfig/motionPropschange, so the inner$statebought nothing — it just allocated a fresh deepProxyover the entire props graph (style,variants,animate, ...) on every recomputation. That:VisualElementholds by reference, andNothing mutates the resulting object —
VisualElementonly ever replacesthis.propswholesale inupdate()— so it is now a plain object literal.2.
context.visualElementmutated onto a derived-owned objectuseCreateMotionContextreturns a$derivedobject, and the watcher wrotevisualElementonto it. Any variant or tracked prop change rebuilt that object, socontext.visualElementread asundefineduntil the watcher ran again. In that windowUseRenderloses the element'slistenersandmeasureProps.visualElementgoes undefined.The visual element now lives in init-time
$state, andcontextis a stable object exposinginitial/animatefrom the derived through getters andvisualElementthrough a getter/setter pair. The existingwatch.preassignment is unchanged.3. Dead code
The commented-out
$effectblock claimed "useMotionRef is not called on destroy" — it is:UseRender'smotionRefattachment teardown callsref(null), which unmounts the visual element. The note is stale, so the block is removed.Tests
packages/motion-start/src/motion/__tests__/reactivity.svelte.spec.tsdrives the realcreateRendererMotionComponentwith a stub renderer/visual element (probe-motion-component.svelte.ts+ReactivityFixture.svelte) and asserts:style,variants) reach the visual element by reference rather than as proxy clones;context.visualElementstays defined when read immediately after a variant change, before the commit flushes.All three fail on
mainand pass with this change.Verification
vitest run(package): 621 passed. Two pre-existing failures on this machine (package-imports,reorder-production-ssr) are 30s in-test timeouts on disk/build-heavy work and fail identically without this change.svelte-check: 0 errors, 0 warnings.biome checkon the changed files: clean apart from the same Svelte-template false positives (noUnusedVariablesfor template-only bindings) that existing fixtures produce.Scope note:
useLayoutIdinside the$derived.byand the broadercreate-factory/create-proxy/use-render/MotionScoperestructure are deliberately untouched.