Svelte: Improve support for async components #31476
Conversation
…ories for async components
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
|
View your CI Pipeline Execution ↗ for commit 0c7723f
☁️ Nx Cloud last updated this comment at |
JReinhold
changed the title
tick() after mounttick() after mount
…rybookjs/storybook into jeppe/svelte-async-component-support
There was a problem hiding this comment.
6 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
|
With the release of Svelte 5.36.0 and experimental support for async components, this just became way more relevant, and we should prioritize getting this cleaned up and merged. https://github.com/sveltejs/svelte/releases/tag/svelte%405.36.0 |
…elte-async-component-support
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Renderer as renderToCanvas
participant Svelte as Svelte Runtime
participant Registry as Instance Registry
Caller->>Renderer: renderToCanvas(story, args)
alt No existing instance
Renderer->>Svelte: new Component({ target, props: args })
Note over Svelte,Renderer #D6EAF8: Component mounts
Renderer->>Svelte: await svelte.tick()
Note over Renderer #D6EAF8: Ensure mount effects settle
Renderer->>Registry: record(instance)
else Existing instance
Renderer->>Svelte: instance.$set(args)
Renderer->>Svelte: await svelte.tick()
Note over Renderer #D6EAF8: Ensure prop update settles
end
Renderer-->>Caller: Promise resolves
sequenceDiagram
autonumber
participant Storybook
participant Preview as PreviewRender.svelte
participant Boundary as Svelte Boundary
participant Decorator as DecoratorHandler
participant Comp as Story Component
Storybook->>Preview: render(story)
Preview->>Boundary: mount boundary
Note over Boundary #F9E79F: show pending notice (id='sb-pending-async-component-notice') while unresolved
Boundary->>Decorator: render DecoratorHandler inside boundary
Decorator->>Comp: mount/render story component
alt Component effects complete
Boundary-->>Storybook: pending notice removed, settled UI shown
else Still pending
Boundary-->>Storybook: pending notice remains visible
end
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/renderers/svelte/src/render.ts (1)
38-49: Annotate Svelte renderToCanvas with Promise<() => void> and await its callersrenderToCanvas is async — add an explicit return type and make callers that delegate/return its result await it so they receive the unmount function (not a Promise).
Files to change:
- code/renderers/svelte/src/render.ts — change signature to return Promise<() => void>
- code/renderers/svelte/src/portable-stories.ts — make INTERNAL_DEFAULT_PROJECT_ANNOTATIONS.renderToCanvas async and await svelteProjectAnnotations.renderToCanvas(...)
- code/renderers/vue3/src/portable-stories.ts — make renderToCanvas async and await defaultProjectAnnotations.renderToCanvas(...)
- code/renderers/react/src/portable-stories.tsx — add await when delegating to reactProjectAnnotations.renderToCanvas(...) (function already async; make explicit)
Also audit other portable-stories/delegation sites that return projectAnnotations.renderToCanvas to ensure consistent Promise<() => void> handling.
🧹 Nitpick comments (2)
code/renderers/svelte/src/render.ts (1)
79-85: Set the mounted component before awaiting tick to avoid re-entrant mountsMinor race: another render could start before tick resolves and think nothing is mounted. Set the map first.
Apply this diff:
const mountedComponent = svelte.mount(PreviewRender, { target: canvasElement, props, }); - await svelte.tick(); - componentsByDomElement.set(canvasElement, { mountedComponent, props }); + componentsByDomElement.set(canvasElement, { mountedComponent, props }); + await svelte.tick();code/renderers/svelte/template/stories/settled.stories.ts (1)
3-7: Remove unused imports until Async story is enabledAvoid lints on unused symbols.
Apply this diff:
-import { expect, fn, waitFor } from 'storybook/test'; - -import AsyncComponent from './AsyncComponent.svelte'; +import { expect, fn } from 'storybook/test'; + import SyncComponent from './SyncComponent.svelte';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
code/renderers/svelte/src/__test__/composeStories/__snapshots__/portable-stories.test.ts.snapis excluded by!**/*.snaptest-storybooks/portable-stories-kitchen-sink/svelte/stories/__snapshots__/Button.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
code/renderers/svelte/src/render.ts(3 hunks)code/renderers/svelte/static/PreviewRender.svelte(1 hunks)code/renderers/svelte/template/stories/AsyncComponent.svelte(1 hunks)code/renderers/svelte/template/stories/SyncComponent.svelte(1 hunks)code/renderers/svelte/template/stories/settled.stories.ts(1 hunks)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/renderers/svelte/static/PreviewRender.svelte (2)
29-34: Use data-testid instead of an ID; add live-region semanticsAvoid global IDs (duplication across canvases) and prefer test IDs. Also make the pending notice accessible.
Apply this diff:
<svelte:boundary> {#snippet pending()} - <div id="sb-pending-async-component-notice">Pending async component...</div> + <div data-testid="sb-pending-async-component-notice" role="status" aria-live="polite"> + Pending async component... + </div> {/snippet} <DecoratorHandler {Component} {props} /> </svelte:boundary>
29-34: Confirm Svelte version compatibility for svelte:boundaryEnsure this file is only compiled by Svelte 5+ in all environments (Preview/Docs). If older Svelte compilers are still in use anywhere, this tag will fail to compile.
code/renderers/svelte/template/stories/SyncComponent.svelte (1)
1-13: LGTM — simple post-mount effect with test hookEffect runs once and reveals the element; aligns with the new tick() sequencing.
code/renderers/svelte/template/stories/AsyncComponent.svelte (1)
1-25: OK to keep synchronous for now; TODO is clearGiven the async story is disabled, this stub is fine until native async components are enabled.
When enabling real async behavior, prefer removing the timeout and relying on Svelte’s async component semantics plus the Preview boundary.
code/renderers/svelte/src/render.ts (1)
83-84: Good: tick() after mount and prop mutationThese awaits align the play phase with completed effects and reactivity.
Also applies to: 94-95
code/renderers/svelte/template/stories/settled.stories.ts (1)
26-51: When enabling Async story, prefer test id over element idMatches the boundary change; keeps tests resilient.
Apply this diff when uncommenting:
- await expect(canvas.getElementById('sb-pending-async-component-notice')).toBeInTheDocument(); + await expect(canvas.getByTestId('sb-pending-async-component-notice')).toBeInTheDocument();
JReinhold
changed the title
tick() after mount
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
code/renderers/svelte/template/stories/settled.stories.ts (3)
41-43: Optional: link the tracked Svelte bug or TODO issue.Add a reference to the Svelte issue tracking settled() never resolving to make this actionable for future cleanup.
I can add a comment with the exact issue link if you share it.
44-47: Assert the pending notice is removed once settled (flakiness guard).After the component settles, also assert that the pending notice disappears. This strengthens the signal that the boundary completes.
await waitFor(() => { expect(args.onEffect).toHaveBeenCalledTimes(1); expect(canvas.getByTestId('after-effect')).toBeInTheDocument(); + // Pending notice should be gone once the async component has settled + expect( + canvasElement.querySelector('#sb-pending-async-component-notice') + ).toBeNull(); });
5-5: Remove commented import.Dead/commented code adds noise.
-// import * as svelte from 'svelte';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.locktest-storybooks/portable-stories-kitchen-sink/svelte/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
code/frameworks/svelte-vite/package.json(1 hunks)code/package.json(0 hunks)code/renderers/svelte/package.json(1 hunks)code/renderers/svelte/svelte.config.js(1 hunks)code/renderers/svelte/template/stories/AsyncComponent.svelte(1 hunks)code/renderers/svelte/template/stories/SyncComponent.svelte(1 hunks)code/renderers/svelte/template/stories/settled.stories.ts(1 hunks)test-storybooks/portable-stories-kitchen-sink/svelte/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- code/package.json
✅ Files skipped from review due to trivial changes (1)
- code/renderers/svelte/svelte.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- code/renderers/svelte/template/stories/AsyncComponent.svelte
- code/renderers/svelte/template/stories/SyncComponent.svelte
⏰ 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). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
test-storybooks/portable-stories-kitchen-sink/svelte/package.json (1)
57-66: Dependency bumps look consistent with renderer/framework changes.The Svelte/Vite/plugin/test libs are aligned; no issues from this diff.
code/renderers/svelte/template/stories/settled.stories.ts (1)
37-39: Prefer data-testid/getByTestId over querying by id.Switching to test IDs improves test robustness and consistency with Testing Library best practices.
As a follow‑up:
- Add data-testid="sb-pending-async-component-notice" on the notice element (PreviewRender.svelte).
- Then change this assertion to use canvas.getByTestId('sb-pending-async-component-notice').
code/renderers/svelte/package.json (1)
62-67: Version bumps look good — verify TS and plugin compatibility
- @sveltejs/vite-plugin-svelte ^6.2.0 requires Node >=20.19 and has a peer Vite range ^6.3.0 || ^7.0.0 — ensure CI/dev Node and Vite versions match this matrix.
- TypeScript: current stable is 5.9.2; confirm your toolchain mirrors provide ^5.8.3 (or upgrade to 5.9) and run TS type-checks/builds to validate repo compatibility.
…rybookjs/storybook into jeppe/svelte-async-component-support
|
Self-merged |
Closes #
What I did
tick()after mounting and re-rendering Svelte components, to ensure that any effects are completed before calling the render phase complete.@testing-library/svelteusesflushSync, but because we're in an async context we canawait tick()instead, which is supposedly better (source: the man-bun-man in Barcelona).Originally I did this to test out the
await settled()call, but concluded that we should not be calling that automatically similar totick(). The reason being that it would block users from testing their components in the pending state, as it would causeplayto wait for all promises to settled, not allowing users to make assertions before so. If users want to, they can callsettled()in theirplay-function for maximum flexibility.<svelte:boundary>, so that if users try to render async components without wrapping them in a boundary, it won't crash, it will just use our top-level boundary instead. Users can add their own lower-level boundaries if they want to. This is a no-op in current Svelte versions so it should be safe to add now.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Adds proper Svelte async component support to Storybook, ensuring component effects complete before rendering and state transitions are handled correctly.
await svelte.tick()after mounting/re-rendering to properly complete effects before proceeding<svelte:boundary>for graceful async component fallback instead of crashingAsyncComponent.svelte,SyncComponent.svelte) to verify effect handlingsettled()to preserve ability to test pending statesSummary by CodeRabbit
New Features
Documentation
Chores