fix(layout): resolve LayoutGroup id at init and stop swallowing context errors - #77
fix(layout): resolve LayoutGroup id at init and stop swallowing context errors#77JonathonRP wants to merge 1 commit into
Conversation
…xt errors `motion` components read their enclosing `LayoutGroup` id from inside the `$derived` that assembles `configAndProps`. `getContext` is an initialisation-only API, so keeping it inside a value that recomputes on every props change is a latent hazard: any recompute that lands outside a reaction throws, and the blanket `try`/`catch` in `useLayoutGroupContext` turned that throw into `null`, silently dropping the group prefix from `layoutId` and breaking shared-layout animations. - Resolve the layout group id once, during component initialisation, as a plain `const`; the derived now reads the already-resolved value. - Replace the `useLayoutId` hook with the pure `getLayoutId` helper. - Detect a missing `LayoutGroup` provider with `hasContext` instead of catching everything, so genuine misuse throws loudly. - Delete the unused `createLayoutGroupContext` rune store and `LayoutGroupContextType`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 57478b4 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: 47 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 (7)
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: |
The problem
packages/motion-start/src/motion/index.svelte.tsassembledconfigAndPropslike this:useLayoutIdcallsuseLayoutGroupContext(), which reaches into Svelte's context system.getContext/hasContextare component-initialisation APIs — they throwlifecycle_outside_componentwhenevercomponent_contextis null. Putting one inside a value that recomputes on every props change means the context lookup is re-entered long after initialisation, from whatever call site happens to pull on the derived.That was made invisible by a blanket catch in
packages/motion-start/src/context/LayoutGroupContext.svelte.ts:The catch existed for a legitimate reason — motion elements are routinely rendered with no
LayoutGroupancestor, andcreateContext's getter throwsmissing_contextin that case. But it caught everything, so a lifecycle violation was indistinguishable from "no provider" and degraded tolayoutGroupId === undefined. The observable symptom would belayoutIdlosing itsLayoutGroupprefix after a prop change, which silently breaks shared-layout animations between grouped elements.Honest scoping note
I wrote the regression test first and it passed against the unfixed code. Digging into
svelte@5.56.8,update_reactionrestores the reaction's captured component context before recomputing (set_component_context(reaction.ctx)ininternal/client/runtime.js), and$derivedsignals capturectx: component_contextat creation (internal/client/reactivity/deriveds.js). BecauserenderMotionComponentruns insideMotionScope's initialisation, the derived captures a non-null ctx andgetContextstill resolves on recompute. So on this Svelte version the prefix does not actually drop today.What remains real, and is what this PR fixes:
tick().then(), a rAF callback, a microtask render) or the derived is created without a component context.catchgenuinely does erase that failure mode. That is the part that made the hazard undiagnosable, and it is removed.I've left the reproduction test in as a regression guard rather than deleting it, and added a direct test that the context helper throws outside initialisation.
The fix
const layoutGroupId = useLayoutGroupContext()?.id;now sits in the component body, outside any$derived. The derived reads the already-resolved value.useLayoutId→getLayoutId. The olduse*hook was the footgun; it's replaced by a puregetLayoutId(props, layoutGroupId)that is safe to call from a derived. Nothing else in the repo referenceduseLayoutId.useLayoutGroupContextnow uses an explicithasContextcheck for the documented "no provider" case and lets everything else throw. I checked all four call sites first:LayoutGroup.svelte—useLayoutGroupContext() || { id: oldId }, legitimately runs with no provider at the root of a group. Covered byhasContext.MeasureLayout.svelte—?? { forceRender: () => {} }, same.motion/index.svelte.ts— now init-only.LayoutGroupConsumer.svelte(test fixture) — now has explicit coverage for the no-provider case.createLayoutGroupContextandLayoutGroupContextType(the rune-store implementation) had no importers anywhere in the repo — only doc/plan files mention them. Deleted, along with their re-export from thesrc/contextbarrel. The barrel isn't inpackage.json#exportsand isn't imported anywhere, so this isn't a public API break.I kept the
.svelte.tsextension onLayoutGroupContext.svelte.tseven though it no longer contains runes, becausesrc/index.tsre-exports theLayoutGroupContexttype from that path.Tests
layout-group-motion.svelte.spec.ts(new): mountsmotion.divwithlayoutId="box"inside<LayoutGroup id="group">, changes a style prop, and asserts bothvisualElement.getProps().layoutIdandprojection.options.layoutIdare stillgroup-box. It asserts the props recompute actually happened (style.width === '200px') so it can't pass vacuously.layout-group.svelte.spec.ts: added coverage thatuseLayoutGroupContext()returnsnullwith no provider and throws when called outside component initialisation.Verification
bun run test:run— 620 passed, 1 skipped. The single failure (package-imports.spec.ts"fully specifies relative JavaScript imports") is a 5s-timeout flake on this machine and reproduces identically on a cleangit stashed tree; it's unrelated to this change.bun run check(svelte-check) — 0 errors, 0 warnings.biome linton the touched files — no new diagnostics (remaining ones are pre-existingnoExplicitAny/CRLF noise shared withmain).Follow-up, not done here
use-visual-element.svelte.tshas the same shape —$derived(useSwitchLayoutGroupContext())— andSwitchLayoutGroupContext.ts/DeprecatedLayoutGroupContext.tsboth still use blankettry/catch. Left alone deliberately to keep this PR scoped; worth folding into the planned context refactor.