Skip to content

Fix spring sub-frame sampling, MotionValue reactive-read refcount, and PresenceChild map - #81

Open
JonathonRP wants to merge 2 commits into
mainfrom
jonathonrp-spring-sample-and-value-fixes
Open

Fix spring sub-frame sampling, MotionValue reactive-read refcount, and PresenceChild map#81
JonathonRP wants to merge 2 commits into
mainfrom
jonathonrp-spring-sample-and-value-fixes

Conversation

@JonathonRP

@JonathonRP JonathonRP commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Three small, independent correctness fixes against framer-motion v11.11.11 behaviour.

Note

File overlap with #80. PR #80 (jonathonrp-backport-upstream-bug-fixes) also edits packages/motion-start/src/value/index.ts, widening Owner['getProps'] (line 50) to declare transformTemplate. That is written as a method shorthand on purpose — it relies on parameter bivariance for the AcceleratedAnimation.supports() transformTemplate guard, so it must not be "tidied" into a property signature.

This PR's hunks in that file are confined to the MotionValue class body (the #reactiveSubscribers/#subscribe fields, a stale commented-out line, and the notify branch of updateAndNotify); Owner/getProps is untouched, nothing was reformatted or reorganised, and the change to how change subscribers are stored is additive with no bearing on the Owner typing. The two diffs do not overlap and should merge cleanly in either order. Likely order is #80 first, then this rebased on top.

1. value/use-spring.ts — restore dropped sub-frame sampling

Upstream calls animation.sample(frameData.delta) when retargeting a spring whose previous animation has not yet rendered a frame, so the replacement spring starts from the correct interpolated position. The port dropped that call, so a spring retargeted before its first render restarted from the stale position (a visible "snap back" on rapid retargets).

Restored the upstream branch, adapted to this file's structure (activeSpringAnimation is a plain closure variable rather than a React ref), and added frameData to the existing frameloop import.

Test: src/value/__tests__/use-spring.svelte.spec.ts mounts a spring, sets a target, and retargets it before the first animation has ticked. It drives the spring with an idle driver (never ticks) so the precondition animation.time === 0 is deterministic rather than dependent on frameloop queue ordering, spies on animateValue to capture the keyframes each animation is started with, and asserts the retargeted animation starts from the sampled position rather than 0. Verified failing before the fix (expected 0 to be greater than 0).

2. value/index.ts — reactive .current reads no longer cancel animations

.current was made reactive with createSubscriber((update) => this.on('change', update)), which registers a genuine change listener. MotionValue.on('change', …)'s unsubscribe schedules a frame.read that calls this.stop() when events.change.getSize() hits zero. So for a standalone MotionValue (not owned by a VisualElement), unmounting the last component that merely read .current reactively cancelled an in-flight animation. Upstream has no such coupling — there, subscriber count and animation lifetime are independent in this case.

Fix: Svelte's signal subscriber is now held in its own SubscriptionManager (#reactiveSubscribers), notified from updateAndNotify under the same _current !== prev condition as the change event, immediately after events.change.notify(...). It is therefore invisible to the refcount that gates auto-stop. Reactive behaviour of .current is unchanged, and the existing "when all change listeners removed, stop animation" behaviour for real on('change') subscribers still holds.

Lazy allocation

#reactiveSubscribers is declared as an optional field and constructed on first reactive subscription (inside the createSubscriber start callback), with #reactiveSubscribers?.notify() at the notify site.

This matters because SubscriptionManager is not free: constructing one allocates the instance, its backing subscriptions: Handler[] = [] array, and two bound functions — add and notify are instance fields, not prototype methods — so roughly four allocations each time. MotionValues are created per animated property per element, so on a list of a few hundred animated items an eager class-field initializer would be a meaningful and permanent cost, and the overwhelming majority of MotionValues are never read inside a $derived/$effect, leaving the manager empty for its entire life. Lazy allocation keeps the port aligned with upstream's practice of lazily creating events.change on first on() rather than allocating event managers up front.

createSubscriber itself is still constructed eagerly per instance, unchanged from before this PR — for the record it only allocates a closure, a source(0) signal and the returned function, and registers no listeners or effects until the first tracked read.

Also removed a stale // #subscribe: ReturnType<typeof createSubscriber> | null = null; left over from an earlier approach, which was misleading next to the real #subscribe defined above it.

Tests in src/value/__tests__/motion-value-reactivity.svelte.spec.ts:

  • animate a standalone MotionValue, mount and unmount a component that reads .current, assert the animation is still running after the frameloop has processed the teardown. Verified failing before the fix (expected false to be true).
  • assert lazy allocation: SubscriptionManager construction is counted via a vi.mock Proxy construct trap (the field itself is a real #private and can't be read from a test, so this avoids weakening the source to make it observable). A MotionValue that is only set() constructs zero managers; mounting a component that reads .current constructs exactly one. Verified failing with an eager field initializer (expected 1 to be +0).

3. components/AnimatePresence/PresenceChild/PresenceChild.svelte — remove no-op $state(new Map())

Chose: drop the $state wrapper (not SvelteMap).

presenceChildren is only ever read imperatively: handleExitComplete and register are context callbacks invoked by descendants, and the two $effects that touch it are driven by isPresent — the forEach/size reads happen inside an effect body already tracking isPresent, and the size read is inside a tick().then() callback where nothing is tracked anyway. Nothing renders from the map or derives from its contents, so it was never meant to drive reactivity, and $state around a native Map is a no-op in Svelte 5 regardless (only plain objects and arrays are proxied). Switching to SvelteMap would have added reactive overhead for no consumer. Added a comment recording the reasoning. The map is local to the component and never passed out, so nothing depended on the wrapper.

Verification

  • vitest --run src/value: 74 passed (11 files), including motion-value-reactivity.svelte.spec.ts and use-spring.svelte.spec.ts.
  • vitest --run over src/value, src/components/AnimatePresence and src/render: 200 passed.
  • Full suite: 621 passed. The failures seen under full-suite parallel load (package-imports.spec.ts, Reorder production SSR) are known pre-existing flakes that pass in isolation, including on a clean stashed tree.
  • bun run --filter motion-start check (svelte-check): 0 errors, 0 warnings.
  • Biome: no new lint findings on the changed files (repo-wide biome check is already failing on pre-existing formatting/line-ending issues; baseline compared file-by-file).
  • Changeset added: .changeset/tidy-spiders-jam.md (patch).

…d PresenceChild map

- useSpring: restore upstream's animation.sample(frameData.delta) when retargeting a spring whose previous animation has not yet rendered a frame.
- MotionValue: track Svelte's signal subscriber separately from user on('change') subscribers so reactive .current reads no longer participate in the refcount that auto-stops animations.
- PresenceChild: drop the no-op \ wrapper around the registered-children Map.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@changeset-bot

changeset-bot Bot commented Aug 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d13d8ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
motion-start Patch

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

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@JonathonRP, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 581475fd-5fec-4ba5-8e86-fbd8c77bd927

📥 Commits

Reviewing files that changed from the base of the PR and between 0adcef2 and d13d8ea.

📒 Files selected for processing (7)
  • .changeset/tidy-spiders-jam.md
  • packages/motion-start/src/components/AnimatePresence/PresenceChild/PresenceChild.svelte
  • packages/motion-start/src/value/__tests__/UseSpringFixture.svelte
  • packages/motion-start/src/value/__tests__/motion-value-reactivity.svelte.spec.ts
  • packages/motion-start/src/value/__tests__/use-spring.svelte.spec.ts
  • packages/motion-start/src/value/index.ts
  • packages/motion-start/src/value/use-spring.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Aug 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/motion-start@81

commit: d13d8ea

Only create the SubscriptionManager backing Svelte's signal subscriber on the
first reactive read, mirroring how events managers are created on first on().
Most MotionValues are never read inside a \/\, so this avoids ~4
allocations per value. Also drops a stale commented-out #subscribe declaration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant