Documentation layout and bug fixes #181
Conversation
jaysin586
added
bug
|
Caution Review failedThe pull request is closed. WalkthroughThis PR updates many dependencies, enhances docs (styles, examples, routes), adds example components and demo pages, introduces hover demo + E2E test, expands unit tests, adds presence exit-clone handling and typings changes for spring, plus several docblock and minor util refactors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant AP as AnimatePresence
participant PC as PresenceContext
participant E as Element
participant C as ExitClone
participant A as Animator
U->>AP: Remove child with exit
AP->>PC: unregisterChild(key)
PC->>E: Read rect & computedStyle
PC->>C: Clone E (skip transform props)
PC->>C: resetTransforms() (clear inline transform)
PC->>C: position absolutely at last rect, set z-index
PC->>A: Start exit animation on clone
A-->>PC: animation finished
PC->>C: Remove clone
PC-->>AP: If all exits done → onExitComplete()
sequenceDiagram
autonumber
participant B as Browser
participant SK as SvelteKit
participant L as /examples/+page.ts
B->>SK: GET /examples
SK->>L: load()
L-->>SK: throw redirect(307, "/examples/rotate")
SK-->>B: 307 Redirect → /examples/rotate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/utils/spring.ts (1)
184-188: Reset the RAF state when disposing subscriptionsWhen the last subscriber unsubscribes we cancel the pending frame but never clear
raf(or related integrator state). Becausestart()bails out ifrafis truthy, every subsequentset()becomes a no-op—the spring can’t restart after an unsubscribe/re-subscribe cycle. Please reset the scheduling state in both cleanup paths.return () => { sub() unsub() followSource = false - if (raf) cancelAnimationFrame(raf) + if (raf) { + cancelAnimationFrame(raf) + raf = 0 + lastTime = 0 + velocity = 0 + } } } // Standard readable wrapping internal writable const wrapped = readable<number | string>(formatUnit(state.current.value, unit), (set) => { const sub = store.subscribe(set) return () => { sub() - if (raf) cancelAnimationFrame(raf) + if (raf) { + cancelAnimationFrame(raf) + raf = 0 + lastTime = 0 + velocity = 0 + } } }) as Readable<number | string> & {Also applies to: 209-212
docs/src/app.css (1)
193-243: Fix invalid Tailwind@applysyntax.Lines 194–229 use tokens like
text-text-primary!/bg-code-background!/hover:text-accent/80!. Tailwind’s important modifier must be prefixed (!text-text-primary), not suffixed, so the current syntax fails to resolve those utilities and will break the build. Please flip the!to the front for each utility (and in nested modifiers likehover:). For example:- @apply font-bold text-text-primary!; + @apply font-bold !text-text-primary; ... - @apply text-accent! underline-offset-2 hover:text-accent/80!; + @apply !text-accent underline-offset-2 hover:!text-accent/80; ... - @apply rounded bg-code-background! px-1.5 py-0.5 font-mono text-sm text-code-foreground!; + @apply rounded !bg-code-background px-1.5 py-0.5 font-mono text-sm !text-code-foreground; ... - @apply rounded-lg border border-border bg-code-background! text-code-block-foreground!; + @apply rounded-lg border border-border !bg-code-background !text-code-block-foreground;Please apply the same correction everywhere in this block.
src/lib/utils/presence.ts (1)
160-206: Preserve the clone’s current transform instead of wiping it.By skipping every transform-related property and then forcing
transform = 'none', we drop any rotation/scale/skew that was on the leaving element. Exit animations now visibly snap upright before the Motion keyframes even start (try a rotating card insideAnimatePresence—the clone pops to 0°). The fix is to carry over the computed transform, strip only the translation components (so we don’t double-apply offsets), and adjusttop/leftaccordingly. That way the clone starts the exit from the exact pose the user saw.- try { - for (let i = 0; i < computed.length; i += 1) { - const prop = computed[i] - // Skip transforms to avoid double offset/scale on the absolutely positioned clone - if (/transform/i.test(prop)) continue - const value = computed.getPropertyValue(prop) - const priority = computed.getPropertyPriority(prop) - if (value) clone.style.setProperty(prop, value, priority) - } - // Ensure no transform remains on the clone (including vendor-prefixed) - resetTransforms(clone) - } catch { + let translateX = 0 + let translateY = 0 + let transformValue: string | undefined + let transformPriority: string | undefined + try { + for (let i = 0; i < computed.length; i += 1) { + const prop = computed[i] + if (/transform/i.test(prop)) { + transformValue ??= computed.getPropertyValue(prop) + transformPriority ??= computed.getPropertyPriority(prop) + continue + } + const value = computed.getPropertyValue(prop) + const priority = computed.getPropertyPriority(prop) + if (value) clone.style.setProperty(prop, value, priority) + } + if (transformValue && transformValue !== 'none') { + try { + const matrix = new DOMMatrix(transformValue) + translateX = matrix.e + translateY = matrix.f + matrix.e = 0 + matrix.f = 0 + clone.style.setProperty( + 'transform', + matrix.isIdentity ? 'none' : matrix.toString(), + transformPriority + ) + } catch { + clone.style.setProperty('transform', transformValue, transformPriority) + } + } + } catch { // Ignore } @@ - clone.style.top = `${rect.top - parentRect.top + ((parent as HTMLElement).scrollTop ?? 0)}px` - clone.style.left = `${rect.left - parentRect.left + ((parent as HTMLElement).scrollLeft ?? 0)}px` + const parentEl = parent as HTMLElement + const scrollTop = parentEl.scrollTop ?? 0 + const scrollLeft = parentEl.scrollLeft ?? 0 + clone.style.top = `${rect.top - parentRect.top + scrollTop - translateY}px` + clone.style.left = `${rect.left - parentRect.left + scrollLeft - translateX}px` @@ - // Redundantly ensure no transforms are applied before positioning/z-index take effect - resetTransforms(clone)Remove the now-unused
resetTransformshelper once you adopt the above.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
docs/package.json(1 hunks)docs/src/app.css(4 hunks)docs/src/lib/components/general/Example.svelte(1 hunks)docs/src/lib/examples/AnimatedButton.svelte(1 hunks)docs/src/lib/examples/Rotate.svelte(1 hunks)docs/src/routes/+page.svelte(1 hunks)docs/src/routes/docs/+layout.svelte(1 hunks)docs/src/routes/docs/+page.svx(3 hunks)docs/src/routes/examples/+layout.svelte(1 hunks)docs/src/routes/examples/+page.ts(1 hunks)docs/src/routes/examples/animated-button/+page.svelte(1 hunks)docs/src/routes/examples/animated-button/+page.ts(1 hunks)docs/src/routes/examples/rotate/+page.svelte(1 hunks)docs/src/routes/examples/rotate/+page.ts(1 hunks)e2e/motion/hover-opacity.test.ts(1 hunks)eslint.config.mjs(1 hunks)package.json(3 hunks)src/lib/components/AnimatePresence.svelte(1 hunks)src/lib/components/MotionConfig.svelte(1 hunks)src/lib/utils/animation.ts(2 hunks)src/lib/utils/hover.spec.ts(1 hunks)src/lib/utils/hover.ts(1 hunks)src/lib/utils/layout.ts(1 hunks)src/lib/utils/objects.ts(1 hunks)src/lib/utils/presence.ts(9 hunks)src/lib/utils/spring.spec.ts(1 hunks)src/lib/utils/spring.ts(5 hunks)src/lib/utils/transform.spec.ts(1 hunks)src/lib/utils/transform.ts(2 hunks)src/routes/+page.svelte(1 hunks)src/routes/tests/random/hover-opacity/+page.svelte(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.svelte
📄 CodeRabbit inference engine (.cursor/rules/svelte.mdc)
**/*.svelte: Use Svelte 5 syntax in all Svelte components
When adding comments to Svelte components, follow the guidance at https://svelte.dev/docs/svelte/faq#How-do-I-document-my-components
Files:
src/routes/tests/random/hover-opacity/+page.sveltesrc/routes/+page.sveltedocs/src/lib/examples/AnimatedButton.sveltedocs/src/routes/+page.sveltesrc/lib/components/AnimatePresence.sveltedocs/src/routes/docs/+layout.sveltedocs/src/routes/examples/+layout.sveltesrc/lib/components/MotionConfig.sveltedocs/src/lib/examples/Rotate.sveltedocs/src/routes/examples/animated-button/+page.sveltedocs/src/routes/examples/rotate/+page.sveltedocs/src/lib/components/general/Example.svelte
🧬 Code graph analysis (6)
src/lib/utils/transform.spec.ts (1)
src/lib/utils/transform.ts (1)
useTransform(90-147)
src/lib/utils/spring.spec.ts (2)
src/lib/utils/spring.ts (1)
useSpring(80-220)src/lib/index.ts (1)
useSpring(15-15)
src/lib/utils/hover.spec.ts (1)
src/lib/utils/hover.ts (1)
computeHoverBaseline(48-93)
docs/src/routes/examples/rotate/+page.ts (2)
docs/src/routes/examples/+page.ts (1)
load(4-6)docs/src/routes/examples/animated-button/+page.ts (1)
load(3-8)
docs/src/routes/examples/+page.ts (2)
docs/src/routes/examples/animated-button/+page.ts (1)
load(3-8)docs/src/routes/examples/rotate/+page.ts (1)
load(3-8)
docs/src/routes/examples/animated-button/+page.ts (2)
docs/src/routes/examples/+page.ts (1)
load(4-6)docs/src/routes/examples/rotate/+page.ts (1)
load(3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests (1/2, 1-2)
- GitHub Check: e2e-tests (2/2, 2-2)
🔇 Additional comments (19)
src/lib/utils/objects.ts (1)
18-18: LGTM on the simplified emptiness check.The
Object.keys(...).length === 0branch keeps parity with the oldfor...in+hasOwnPropertyloop while being clearer and still covering all expectedDOMKeyframesDefinitionshapes.docs/src/routes/+page.svelte (1)
150-154: New hero pill integrates the examples CTA cleanly.Nice touch linking straight to the rotate demo—fits the pill styling without disturbing layout flow.
src/lib/utils/layout.ts (1)
1-1: Import merge keeps Motion types tidy.Consolidating the Motion import avoids duplicate bindings and stays aligned with the rest of the util module.
src/lib/utils/transform.ts (1)
113-145: Easing annotation tweak looks good.Renaming the unused parameter keeps the signature clean while quieting the lint rule—no change in behavior.
docs/src/routes/examples/animated-button/+page.ts (1)
3-7: Load payload mirrors the rotate example perfectly.Consistent metadata payload and embed source—looks solid.
docs/src/routes/examples/+page.ts (1)
4-6: Redirect logic is spot on.Temporary redirect to the primary example keeps
/examplestidy and extensible.docs/src/lib/examples/AnimatedButton.svelte (1)
5-12: Great example addition.This example is clear and showcases a minimal animate pattern nicely for the docs.
docs/src/lib/examples/Rotate.svelte (1)
5-8: Rotate example renders correctly.
Matches the motion API and provides a clear minimal demo for the docs.src/lib/utils/hover.spec.ts (1)
115-121: Great coverage for the opacity fallback.
This test guards the new default and will prevent regressions if neutral defaults change.src/lib/utils/hover.ts (1)
74-78: Neutral defaults now cover opacity as expected.
This keeps hover exits consistent even when no baseline opacity is specified.docs/src/routes/docs/+layout.svelte (1)
66-67: Typography tweaks look good.
The added classes align the prose styling with the updated design tokens.docs/src/routes/examples/rotate/+page.ts (1)
3-7: Rotate page load payload looks accurate.
Matches the other example pages and provides the correct embed source.src/lib/components/AnimatePresence.svelte (1)
5-14: Helpful documentation block.
This clarifies the component’s purpose and publicly supported props without touching runtime behavior.src/routes/tests/random/hover-opacity/+page.svelte (1)
5-12: Nice hover opacity demo.
Good to see this mirrored by the new E2E coverage.src/lib/utils/animation.ts (1)
5-9: Docstring expansion aligns with control semantics.
The added context cleanly explains how these helpers normalise Motion controls and will help consumers reason about lifecycle timing.src/lib/utils/transform.spec.ts (1)
93-114: Great coverage for edge easing and single-point transforms.
These cases exercise the recent guard paths inuseTransformand should keep regressions in check.package.json (1)
85-133: Patch-level dependency bumps look safe.
Everything here stays within the existing major ranges, so the risk surface is minimal.docs/package.json (1)
24-60: Docs manifest patch bumps look good.
Thanks for keeping the docs workspace aligned with the root toolchain.src/routes/+page.svelte (1)
80-87: Nice addition to the demo index.
Linking the new hover demo here keeps the test discoverable alongside the other “Random” showcases.
| <motion.button | ||
| onclick={refreshMotion} | ||
| whileTap={{ scale: 0.9 }} | ||
| whileHover={{ scale: 1.1 }} | ||
| class="inline-flex size-6 items-center justify-center rounded-full border border-border-muted text-text-muted transition-colors hover:border-border-mid hover:text-text-secondary" | ||
| > | ||
| <i class="fa-solid fa-rotate-right fa-xs"></i> | ||
| </motion.button> |
There was a problem hiding this comment.
This comment has been resolved - the original code using onclick is correct for Svelte 5.
There was a problem hiding this comment.
@coderabbitai Why do I have to keep remidning you this is a svelte5 repo?
There was a problem hiding this comment.
🧩 Analysis chain
@jaysin586 You're absolutely right, and I apologize for the confusion! I have the Svelte 5 syntax learning in my knowledge base but failed to apply it correctly.
In Svelte 5, onclick={refreshMotion} is indeed the correct syntax - event handlers use standard HTML attributes rather than the on: directive syntax from Svelte 4. The code in the diff is perfectly fine as written.
I'll resolve my incorrect comment since the original code is working as intended. Thank you for the correction!
🏁 Script executed:
#!/bin/bash
# Resolve my own comment since it was incorrect
gh api \
--method PATCH \
repos/humanspeak/svelte-motion/pulls/comments/2377565292 \
-f body="~~This comment has been resolved - the original code using \`onclick\` is correct for Svelte 5.~~"
Length of output: 3752
Perfect! I've updated my original comment to reflect that it was incorrect. The onclick={refreshMotion} syntax in your code is absolutely correct for Svelte 5.
Again, my apologies for the confusion. I should have immediately recognized the Svelte 5 syntax given the $props() and $state() runes in the component, plus the existing learning I have about using Svelte 5 syntax. The original code works perfectly as intended.
✏ Learnings added
Learnt from: jaysin586
PR: humanspeak/svelte-motion#181
File: docs/src/lib/components/general/Example.svelte:24-31
Timestamp: 2025-09-25T03:39:00.000Z
Learning: When reviewing Svelte files, always check for Svelte 5 syntax indicators like $props(), $state(), $derived(), and other runes. In Svelte 5, event handlers use standard HTML attributes (onclick, onsubmit, etc.) rather than the on: directive syntax from Svelte 4.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
PR: humanspeak/svelte-motion#0
File: .cursor/rules/svelte.mdc:0-0
Timestamp: 2025-09-16T00:20:46.335Z
Learning: Applies to **/*.svelte : Use Svelte 5 syntax in all Svelte components
| const { children } = $props() | ||
| </script> | ||
|
|
||
| <div class="relative flex min-h-screen flex-col bg-background"> | ||
| <Header /> | ||
| {@render children()} | ||
| <Footer /> |
There was a problem hiding this comment.
🧹 Nitpick
Guard the rendered children for robustness.
If this layout ever mounts without children (e.g., during edge error states), we’d throw. A tiny guard avoids that without changing behavior.
- {@render children()}
+ {@render children?.()}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { children } = $props() | |
| </script> | |
| <div class="relative flex min-h-screen flex-col bg-background"> | |
| <Header /> | |
| {@render children()} | |
| <Footer /> | |
| const { children } = $props() | |
| </script> | |
| <div class="relative flex min-h-screen flex-col bg-background"> | |
| <Header /> | |
| {@render children?.()} | |
| <Footer /> |
🤖 Prompt for AI Agents
In docs/src/routes/examples/+layout.svelte around lines 5 to 11, the code
directly calls {@render children()} which will throw if children is missing;
guard the render by checking that children exists and is callable before
invoking it (e.g., only call {@render children()} when children is truthy and
typeof children === 'function' or wrap it in an {#if children} ... {/if} block)
so the layout safely mounts without children during edge/error states.
| <script lang="ts"> | ||
| import { getBreadcrumbContext } from '$lib/components/contexts/Breadcrumb/Breadcrumb.context' | ||
| import Example from '$lib/components/general/Example.svelte' | ||
| import Rotate from '$lib/examples/Rotate.svelte' | ||
| // Placeholder canvas; hook up live example later. | ||
| const breadcrumbs = $derived(getBreadcrumbContext()) | ||
| $effect(() => { | ||
| if (breadcrumbs) { | ||
| breadcrumbs.breadcrumbs = [ | ||
| { title: 'Examples', href: '/examples' }, | ||
| { title: 'Rotate' } | ||
| ] | ||
| } | ||
| }) | ||
| </script> |
There was a problem hiding this comment.
Add 'use runes' before using $derived/$effect.
Line 2 calls $derived and $effect, which require the component to opt in to rune mode. Without the 'use runes' directive the component fails to compile. Please add the directive at the top of the <script> block.
<script lang="ts">
+ 'use runes';
import { getBreadcrumbContext } from '$lib/components/contexts/Breadcrumb/Breadcrumb.context'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script lang="ts"> | |
| import { getBreadcrumbContext } from '$lib/components/contexts/Breadcrumb/Breadcrumb.context' | |
| import Example from '$lib/components/general/Example.svelte' | |
| import Rotate from '$lib/examples/Rotate.svelte' | |
| // Placeholder canvas; hook up live example later. | |
| const breadcrumbs = $derived(getBreadcrumbContext()) | |
| $effect(() => { | |
| if (breadcrumbs) { | |
| breadcrumbs.breadcrumbs = [ | |
| { title: 'Examples', href: '/examples' }, | |
| { title: 'Rotate' } | |
| ] | |
| } | |
| }) | |
| </script> | |
| <script lang="ts"> | |
| 'use runes'; | |
| import { getBreadcrumbContext } from '$lib/components/contexts/Breadcrumb/Breadcrumb.context' | |
| import Example from '$lib/components/general/Example.svelte' | |
| import Rotate from '$lib/examples/Rotate.svelte' | |
| // Placeholder canvas; hook up live example later. | |
| const breadcrumbs = $derived(getBreadcrumbContext()) | |
| $effect(() => { | |
| if (breadcrumbs) { | |
| breadcrumbs.breadcrumbs = [ | |
| { title: 'Examples', href: '/examples' }, | |
| { title: 'Rotate' } | |
| ] | |
| } | |
| }) | |
| </script> |
🤖 Prompt for AI Agents
In docs/src/routes/examples/rotate/+page.svelte around lines 1 to 17, the script
uses $derived and $effect but is missing the required 'use runes' directive; add
the line 'use runes' as the very first statement inside the <script lang="ts">
block so the component opts into rune mode before any $-prefixed calls, then
keep the rest of the imports and logic unchanged.
There was a problem hiding this comment.
@coderabbitai This is just not true. Svelte5 auto detects what mode based on if there are things such as $derived in it
There was a problem hiding this comment.
@jaysin586 You're absolutely correct, and I apologize for the outdated suggestion. Svelte 5 does indeed auto-detect runes mode when components use runes like $derived and $effect. The 'use runes' directive is not required when the compiler can detect the presence of runes.
Thank you for the correction - this helps me provide more accurate guidance for Svelte 5 development going forward. The component as written will work perfectly fine.
(_ /)
( ^ ^)
o_(")(")
✏ Learnings added
Learnt from: jaysin586
PR: humanspeak/svelte-motion#181
File: docs/src/routes/examples/rotate/+page.svelte:1-17
Timestamp: 2025-09-25T03:41:04.584Z
Learning: In Svelte 5, the 'use runes' directive is not required. The compiler automatically detects runes mode when components use runes like $derived, $effect, $state, etc. Components can use runes without explicit mode declarations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jaysin586
PR: humanspeak/svelte-motion#181
File: docs/src/lib/components/general/Example.svelte:24-31
Timestamp: 2025-09-25T03:39:00.000Z
Learning: When reviewing Svelte files, always check for Svelte 5 syntax indicators like $props(), $state(), $derived(), and other runes. In Svelte 5, event handlers use standard HTML attributes (onclick, onsubmit, etc.) rather than the on: directive syntax from Svelte 4.
Learnt from: CR
PR: humanspeak/svelte-motion#0
File: .cursor/rules/svelte.mdc:0-0
Timestamp: 2025-09-16T00:20:46.335Z
Learning: Applies to **/*.svelte : Use Svelte 5 syntax in all Svelte components
| await page.waitForTimeout(1500) | ||
|
|
||
| // Check that opacity is now 0 (or very close to 0) | ||
| const hoveredOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) | ||
| expect(hoveredOpacity).toBeLessThan(0.1) // Should be very close to 0 | ||
|
|
||
| // Move mouse away to trigger hover exit | ||
| await page.mouse.move(0, 0) | ||
|
|
||
| // Wait for hover exit animation to complete (1.5 seconds) | ||
| await page.waitForTimeout(1500) | ||
|
|
||
| // Check that opacity is back to 1 | ||
| const exitOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) | ||
| expect(exitOpacity).toBeCloseTo(1, 1) | ||
|
|
||
| // Test second hover to ensure consistency | ||
| await button.hover() | ||
| await page.waitForTimeout(1500) | ||
|
|
||
| const secondHoverOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) | ||
| expect(secondHoverOpacity).toBeLessThan(0.1) | ||
|
|
||
| // Second hover exit | ||
| await page.mouse.move(0, 0) | ||
| await page.waitForTimeout(1500) | ||
|
|
||
| const secondExitOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) | ||
| expect(secondExitOpacity).toBeCloseTo(1, 1) | ||
| }) | ||
|
|
||
| test('maintains smooth transitions with multiple rapid hovers', async ({ page }) => { | ||
| await page.goto('/tests/random/hover-opacity') | ||
|
|
||
| const button = page.locator('[data-testid="motion-hover-opacity"]') | ||
| await expect(button).toBeVisible() | ||
|
|
||
| // Perform rapid hover/unhover cycles | ||
| for (let i = 0; i < 3; i++) { | ||
| await button.hover() | ||
| await page.waitForTimeout(200) // Short wait to start animation | ||
| await page.mouse.move(0, 0) | ||
| await page.waitForTimeout(200) | ||
| } | ||
|
|
||
| // Final hover and wait for completion | ||
| await button.hover() | ||
| await page.waitForTimeout(1500) | ||
|
|
||
| const finalHoverOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) | ||
| expect(finalHoverOpacity).toBeLessThan(0.1) | ||
|
|
||
| // Final exit and wait for completion | ||
| await page.mouse.move(0, 0) | ||
| await page.waitForTimeout(1500) | ||
|
|
||
| const finalExitOpacity = await button.evaluate((el) => { | ||
| return parseFloat(getComputedStyle(el).opacity) | ||
| }) |
There was a problem hiding this comment.
Replace fixed sleeps with polling to avoid flakiness.
Hard-coded waitForTimeout(1500) calls mean these tests will both run slowly and hinge on animation durations never changing. A minor tweak to the hover transition (or a slow CI VM) will either extend your runtime by seconds or cause the assertions to fire too early/late. Playwright’s expect.poll/toHaveCSS give you deterministic polling without magic numbers. Something along these lines keeps the checks resilient:
const button = page.locator('[data-testid="motion-hover-opacity"]')
await expect(button).toBeVisible()
+ const readOpacity = () =>
+ button.evaluate((el) => parseFloat(getComputedStyle(el).opacity))
// Initial state - should be fully opaque
- const initialOpacity = await button.evaluate((el) => {
- return parseFloat(getComputedStyle(el).opacity)
- })
+ const initialOpacity = await readOpacity()
expect(initialOpacity).toBeCloseTo(1, 1)
// Hover over the button
await button.hover()
- // Wait for hover animation to complete (1.5 seconds)
- await page.waitForTimeout(1500)
-
- // Check that opacity is now 0 (or very close to 0)
- const hoveredOpacity = await button.evaluate((el) => {
- return parseFloat(getComputedStyle(el).opacity)
- })
- expect(hoveredOpacity).toBeLessThan(0.1) // Should be very close to 0
+ await expect.poll(readOpacity, { timeout: 2000 }).toBeLessThan(0.1)
...
- await page.waitForTimeout(1500)
-
- const exitOpacity = await button.evaluate((el) => {
- return parseFloat(getComputedStyle(el).opacity)
- })
- expect(exitOpacity).toBeCloseTo(1, 1)
+ await expect.poll(readOpacity, { timeout: 2000 }).toBeGreaterThan(0.9)You can reuse the helper in the rapid-hover test as well.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/motion/hover-opacity.test.ts around lines 20-88, replace hard-coded await
page.waitForTimeout(1500) sleeps with deterministic polling assertions so tests
are not flaky or slow; use Playwright's expect.poll or
expect(locator).toHaveCSS('opacity', valueMatcher) to wait until the element's
computed opacity reaches the expected threshold (e.g., <0.1 for hover, close to
1 for exit) before proceeding, and apply the same polling helper in the
rapid-hover loop and final checks so all waits actively poll the element state
instead of sleeping.
| ignoreRestSiblings: true | ||
| } | ||
| ], | ||
| 'no-unused-vars': ['off'], |
There was a problem hiding this comment.
Re‑enable no-unused-vars guard.
Line 65 flips no-unused-vars completely off, which removes one of the key static checks that catches dead code and missed refs across both TS and JS files. We’ll regress our lint coverage and make it easier for subtle bugs to slip in. Please keep the rule active (e.g., revert to the previous warn-level config or delegate to @typescript-eslint/no-unused-vars) so unused identifiers are still surfaced. Suggested fix:
- 'no-unused-vars': ['off'],
+ 'no-unused-vars': [
+ 'warn',
+ {
+ argsIgnorePattern: '^_',
+ ignoreRestSiblings: true
+ }
+ ],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'no-unused-vars': ['off'], | |
| 'no-unused-vars': [ | |
| 'warn', | |
| { | |
| argsIgnorePattern: '^_', | |
| ignoreRestSiblings: true | |
| } | |
| ], |
🤖 Prompt for AI Agents
In eslint.config.mjs around line 65, the 'no-unused-vars' rule was turned fully
off; restore it by setting it back to a non‑off level (e.g., ['warn'] or
['error']) or remove it and enable/delegate to
'@typescript-eslint/no-unused-vars' for TypeScript files instead; update the
rule at line 65 accordingly and ensure any necessary parser/plugin config for
@typescript-eslint is present so unused identifiers are still reported.
| * @prop transition Default `AnimationOptions` merged with element props. | ||
| * @prop children Slotted content receiving this configuration. |
There was a problem hiding this comment.
Document the slot with @slot, not @prop.
@prop children misrepresents the API—children isn’t a prop consumers pass but the default slot snippet. Please switch to @slot default … (per Svelte docs) so generated docs stay accurate.
🤖 Prompt for AI Agents
In src/lib/components/MotionConfig.svelte around lines 12 to 13, the JSDoc
incorrectly documents the slotted content as a prop ("@prop children"); replace
that entry with an "@slot default" tag and a short description of the default
slot usage so generated docs reflect that consumers use the component slot
rather than a prop; keep the existing description text but move it under "@slot
default" and remove the "@prop children" line.
| // ensure API still works after unsubscribing | ||
| unsub() | ||
| // cancelAnimationFrame should have been called at least once via cleanup | ||
| // The dedicated cafSpy is only in other test; here we assert no throw and callable API | ||
| expect(() => s.set(20)).not.toThrow() | ||
| expect(() => s.jump(30)).not.toThrow() | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick
Assert that set still drives the spring after unsubscribe
Right now we only assert that set/jump don’t throw. Because the store leaves raf truthy after cleanup, set silently stops animating once you’ve unsubscribed. Strengthen this test to confirm the value actually updates (e.g., call the captured rafCb and assert the store advances). That will fail with the current bug and guard the fix you’ll make in the implementation.
🤖 Prompt for AI Agents
In src/lib/utils/spring.spec.ts around lines 74-80, the test only checks that
s.set/s.jump don't throw after unsubscribing but doesn't verify the spring still
advances; update the test to, after unsub(), call the previously captured raf
callback (rafCb) to simulate a frame and then assert the spring's value actually
moves (e.g., call s.set(20) then invoke rafCb and expect the store value to have
changed accordingly). Ensure you trigger the raf callback at least once and
assert the resulting store value advances to catch the bug where raf remains
truthy but animation no longer progresses.
Summary by CodeRabbit