Core: Storybook failed to load iframe.html when publishing#33896
Conversation
…subpath When Storybook is hosted at a subpath without a trailing slash (e.g. `/design-system`), the previous regex `/\/[^/]*$/` would strip the entire last path segment and replace it with `/`, losing the subpath prefix and producing `/iframe.html` instead of `/design-system/iframe.html`. Fix: only strip the last segment if it looks like an HTML file (ends in `.html`), then ensure a trailing slash before appending `iframe.html`. This correctly handles all cases: - `/` → `/iframe.html` - `/index.html` → `/iframe.html` - `/design-system` → `/design-system/iframe.html` - `/design-system/` → `/design-system/iframe.html` - `/design-system/index.html` → `/design-system/iframe.html` Fixes storybookjs#33848
📝 WalkthroughWalkthroughModified URL construction logic in manager-api to conditionally strip only HTML-suffixed path segments before appending iframe.html when no explicit PREVIEW_URL is configured. Added test coverage for subpath hosting scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing Touches
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/manager-api/modules/url.ts (1)
255-258: LGTM — fix is correct; consider an inline comment for the regex chain.The two-step regex is correct for all cases:
replace(/\/[^/]*\.html$/, '')removes the trailing.htmlpath segment (e.g.,index.html) only when present.replace(/\/?$/, '/')normalises the result to always end with exactly one slash beforeiframe.htmlis appended.Optionally, a brief comment would help future readers understand the intent without having to trace through the regexes:
💬 Suggested comment
const previewBase = refId ? refs[refId].url + '/iframe.html' : global.PREVIEW_URL || + // Strip trailing .html segment (e.g. index.html), ensure trailing slash, then append iframe.html `${managerBase.replace(/\/[^/]*\.html$/, '').replace(/\/?$/, '/')}iframe.html`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/url.ts` around lines 255 - 258, Add a brief inline comment next to the previewBase assignment explaining the two-step regex on managerBase: note that replace(/\/[^/]*\.html$/, '') strips a trailing .html segment (e.g., index.html) if present and replace(/\/?$/, '/') normalizes the URL to end with exactly one slash before appending 'iframe.html'; place this comment alongside the previewBase declaration (referencing previewBase, managerBase, refId, refs) so future readers immediately understand the intent without parsing the regexes.code/core/src/manager-api/tests/url.test.js (1)
490-533: Three new tests correctly cover the bug scenarios — consider adding subpath + absolute-URL coverage.The three added tests map cleanly to the three deployment configurations mentioned in the issue (no trailing slash, trailing slash,
index.html) and their assertions are correct against the updated regex logic.One gap: the existing
'supports returning absolute URLs using the base option'test (lines 421–438) only exercisespathname: '/'. The samebase: 'origin'/base: 'network'path through the code now also runs the regex chain, so a subpath variant would guard against regressions when both options are combined.✅ Suggested additional test
it('correctly links with base option when hosted at a subpath', () => { const { api, state } = initURL({ store, provider: { channel: new EventEmitter() }, state: { location: { pathname: '/design-system/', search: '' } }, navigate: vi.fn(), fullAPI: { getCurrentStoryData: () => ({ id: 'test--story' }) }, }); store.setState(state); const origin = api.getStoryHrefs('test--story', { base: 'origin' }); expect(origin.previewHref).toContain('http://localhost:6006/design-system/iframe.html'); const network = api.getStoryHrefs('test--story', { base: 'network' }); expect(network.previewHref).toContain('http://192.168.1.1:6006/design-system/iframe.html'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/tests/url.test.js` around lines 490 - 533, Add a new test (or extend the existing "supports returning absolute URLs using the base option" test) that exercises api.getStoryHrefs with base: 'origin' and base: 'network' while the initURL state uses a subpath (e.g. state.location.pathname = '/design-system/' or '/design-system') so the regex logic for trimming/keeping subpaths runs for absolute URLs; use initURL and api.getStoryHrefs('test--story', { base: 'origin' }) / ({ base: 'network' }) and assert that previewHref contains the absolute origin/network host plus the subpath + '/iframe.html' (e.g. contains 'http://localhost:6006/design-system/iframe.html') to guard against regressions in getStoryHrefs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager-api/modules/url.ts`:
- Around line 255-258: Add a brief inline comment next to the previewBase
assignment explaining the two-step regex on managerBase: note that
replace(/\/[^/]*\.html$/, '') strips a trailing .html segment (e.g., index.html)
if present and replace(/\/?$/, '/') normalizes the URL to end with exactly one
slash before appending 'iframe.html'; place this comment alongside the
previewBase declaration (referencing previewBase, managerBase, refId, refs) so
future readers immediately understand the intent without parsing the regexes.
In `@code/core/src/manager-api/tests/url.test.js`:
- Around line 490-533: Add a new test (or extend the existing "supports
returning absolute URLs using the base option" test) that exercises
api.getStoryHrefs with base: 'origin' and base: 'network' while the initURL
state uses a subpath (e.g. state.location.pathname = '/design-system/' or
'/design-system') so the regex logic for trimming/keeping subpaths runs for
absolute URLs; use initURL and api.getStoryHrefs('test--story', { base: 'origin'
}) / ({ base: 'network' }) and assert that previewHref contains the absolute
origin/network host plus the subpath + '/iframe.html' (e.g. contains
'http://localhost:6006/design-system/iframe.html') to guard against regressions
in getStoryHrefs.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Core: Storybook failed to load iframe.html when publishing (cherry picked from commit 344ac52)
Fixes #33848
Summary
This PR fixes: [Bug]: Storybook failed to load iframe.html when publishing
Changes
Testing
Please review the changes carefully. The fix was verified against the existing test suite.
This PR was created with the assistance of Claude Sonnet 4.6 by Anthropic | effort: low. Happy to make any adjustments!
Summary by CodeRabbit
Bug Fixes
Tests