Manager: Keep local preview iframe on host URL when a ref story loads first#35078
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFramesRenderer.tsx stops passing ChangesLocal preview iframe composed-ref fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/FramesRenderer.stories.tsx (1)
71-78: ⚡ Quick winAdd interaction and accessibility checks to the
playtest.Line 71-78 currently validates behavior only (iframe URL). Per repo story guidelines, this
playshould also include at least one interaction assertion and one accessibility assertion.As per coding guidelines: “For React components, write Storybook stories with
playfunctions containing behavior, accessibility, and interaction assertions — do NOT write separate*.test.tsxunit tests for components”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/preview/FramesRenderer.stories.tsx` around lines 71 - 78, The current play function only checks the iframe URL; extend the play in FramesRenderer.stories.tsx to also perform an interaction and an accessibility assertion: use the existing canvasElement and localFrame (selector '`#storybook-preview-iframe`') to query into the iframe document (e.g., its contentDocument/body) and perform a user interaction against a visible control inside the iframe (e.g., click a button or trigger keyboard focus) using the same play context, then assert the expected change (interaction assertion); finally run an a11y check (e.g., call the storybook testing-library or axe accessibility check) against the iframe document and assert no violations (accessibility assertion). Ensure these checks live inside the same play async function and reuse localFrame/canvasElement identifiers already present.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/core/src/manager/components/preview/FramesRenderer.stories.tsx`:
- Around line 71-78: The current play function only checks the iframe URL;
extend the play in FramesRenderer.stories.tsx to also perform an interaction and
an accessibility assertion: use the existing canvasElement and localFrame
(selector '`#storybook-preview-iframe`') to query into the iframe document (e.g.,
its contentDocument/body) and perform a user interaction against a visible
control inside the iframe (e.g., click a button or trigger keyboard focus) using
the same play context, then assert the expected change (interaction assertion);
finally run an a11y check (e.g., call the storybook testing-library or axe
accessibility check) against the iframe document and assert no violations
(accessibility assertion). Ensure these checks live inside the same play async
function and reuse localFrame/canvasElement identifiers already present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93bbc829-b9ae-4494-858d-651aee9a5a1f
📒 Files selected for processing (2)
code/core/src/manager/components/preview/FramesRenderer.stories.tsxcode/core/src/manager/components/preview/FramesRenderer.tsx
|
@Sidnioulz this is the fix we discussed on #34553 — it drops |
|
@coderabbitai This story is a targeted regression test for which iframe URL the local preview frame is built with. FramesRenderer only renders |
|
@coderabbitai see above — FramesRenderer renders only preview iframes (mocked, cross-origin here), so there's no inner DOM to interact with or run axe against; the play asserts the URL-construction contract, which is the regression. Keeping it focused. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Sidnioulz
added
bug
ci:normal
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 74 | 🚨 +2 🚨 |
| Self size | 20.74 MB | 20.44 MB | 🎉 -303 KB 🎉 |
| Dependency size | 36.11 MB | 36.65 MB | 🚨 +539 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 189 | 188 | 🎉 -1 🎉 |
| Self size | 15 KB | 15 KB | 0 B |
| Dependency size | 30.77 MB | 30.67 MB | 🎉 -101 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 205 | 🚨 +2 🚨 |
| Self size | 908 KB | 908 KB | 0 B |
| Dependency size | 88.98 MB | 89.22 MB | 🚨 +235 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 198 | 🚨 +2 🚨 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 87.47 MB | 87.71 MB | 🚨 +235 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 75 | 🚨 +2 🚨 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 56.85 MB | 57.08 MB | 🚨 +235 KB 🚨 |
| Bundle Size Analyzer | node | node |
|
Verified
The local preview frame no longer gets stuck on the ref's Storybook on a ref-first load. |
Sidnioulz
added
qa:needed
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
LGTM! Just a small improvement to make on the story to avoid maintainer confusion or Chromatic warnings.
|
@TheSeydiCharyyev could you please check the CI errors as well? LMK if some of them look wrong/unrelated. Thanks! |
|
Switched the story to the existing Icons ref so the active frame renders a real story instead of a non-existent URL — that was the react-…-chromatic failure. On the rest of the CI: the other four (angular-cli build, next-js vite/webpack build, and the normal-generated aggregate) look unrelated to this change — it's a manager-runtime one-liner that can't affect framework builds, and the same jobs are green on #35037, which is rebased onto current next. This branch is a few days behind next, so a rebase should clear them. Want me to rebase it (like you did for #35037), or would you prefer to? |
3 participants
Closes #34553
What I did
Follow-up to #35050. That PR fixed Controls for stories from a composed ref, but surfaced a separate, pre-existing bug: after loading a composed-ref story first, navigating to a host story left the host preview stuck (
Couldn't find story matching id …→ spinner), so host stories never rendered and their Controls stayed in the loading state.The root cause is in
FramesRenderer. The local preview frame is initialized once, and since v10.3.0 (thegetStoryHrefsrefactor,2a864fdb) that init passes the current story'srefId:For a non-empty
refId,getStoryHrefsreturns the ref's iframe URL, so on a ref-first load the local frame is pointed at the composed ref's Storybook and — because it is only set once (theif (!frames[...])guard) — never re-pointed. Host stories can then never load. Host-first avoids it because the first call has norefId.This drops
refIdfrom that one call so the local preview frame always uses the host's own iframe URL (the behavior before v10.3.0), and adds a story covering it. Verified on the repro that ref-first → host renders correctly, with host-first still working.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
storybook-refs-issue.zipfrom [Bug]: Controls never load with two Storybooks (refs) if initial page is a ref story #34553.?path=/story/react_my-button--default). Its Controls should load.Couldn't find story matching id …).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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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 pull request has been released as version
0.0.0-pr-35078-sha-e5f7408f. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35078-sha-e5f7408f sandboxor in an existing project withnpx storybook@0.0.0-pr-35078-sha-e5f7408f upgrade.More information
0.0.0-pr-35078-sha-e5f7408ffix/framesrenderer-ref-first-host-previewe5f7408f1780747525)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35078Summary by CodeRabbit
Bug Fixes
Tests