Core: Fix event source URL based on refId when multiple iframes share the same origin#34105
Open
ghengeveld wants to merge 1 commit intonextfrom
Open
Core: Fix event source URL based on refId when multiple iframes share the same origin#34105ghengeveld wants to merge 1 commit intonextfrom
ghengeveld wants to merge 1 commit intonextfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit d0a83f8
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughAdded an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/channels/postmessage/getEventSourceUrl.ts (1)
59-64: Consider wrapping URL parsing in try-catch for robustness.The
new URL()call at line 62 is outside any try-catch block. While iframesrcattributes are typically valid URLs, a malformed value could cause an uncaught exception. This is a minor edge case, but for consistency with the defensive approach used elsewhere in this function, you may want to handle it.🛡️ Optional fix to add defensive error handling
const src = pickFrameByRefId(candidates, refId)?.getAttribute('src'); if (src) { + try { const { protocol, host, pathname } = new URL(src, document.location.toString()); return `${protocol}//${host}${pathname}`; + } catch { + // Malformed src, fall through to return null + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/channels/postmessage/getEventSourceUrl.ts` around lines 59 - 64, In getEventSourceUrl, wrap the new URL(...) call (where you deconstruct protocol, host, pathname) in a try-catch so a malformed iframe src from pickFrameByRefId(...) does not throw; on exception log or silently handle and return undefined (preserving the existing behavior when src is absent). Ensure you still pass document.location.toString() as the base to URL and return `${protocol}//${host}${pathname}` only on success.
🤖 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/channels/postmessage/getEventSourceUrl.ts`:
- Around line 59-64: In getEventSourceUrl, wrap the new URL(...) call (where you
deconstruct protocol, host, pathname) in a try-catch so a malformed iframe src
from pickFrameByRefId(...) does not throw; on exception log or silently handle
and return undefined (preserving the existing behavior when src is absent).
Ensure you still pass document.location.toString() as the base to URL and return
`${protocol}//${host}${pathname}` only on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a63a365c-dbaa-44f6-b3f6-8a349218b126
📒 Files selected for processing (2)
code/core/src/channels/postmessage/getEventSourceUrl.tscode/core/src/channels/postmessage/index.ts
Contributor
|
Failed to publish canary version of this pull request, triggered by @ghengeveld. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22962473056 |
Contributor
|
Failed to publish canary version of this pull request, triggered by @Sidnioulz. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22962473056 |
1 similar comment
Contributor
|
Failed to publish canary version of this pull request, triggered by @Sidnioulz. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22962473056 |
ghengeveld
added
the
patch:yes
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 76 KB | 76 KB | 0 B |
| Dependency size | 32.18 MB | 32.19 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 140 KB | 140 KB | 0 B |
| Dependency size | 30.39 MB | 30.40 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 189 | 189 | 0 |
| Self size | 15 KB | 15 KB | 🚨 +18 B 🚨 |
| Dependency size | 28.89 MB | 28.90 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 650 KB | 650 KB | 0 B |
| Dependency size | 59.89 MB | 59.91 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -36 B 🎉 |
| Dependency size | 22.45 MB | 22.47 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.74 MB | 23.75 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 🚨 +18 B 🚨 |
| Dependency size | 20.24 MB | 20.25 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 24 KB | 24 KB | 🎉 -12 B 🎉 |
| Dependency size | 44.52 MB | 44.53 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 16 KB | 16 KB | 0 B |
| Dependency size | 33.44 MB | 33.45 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 0 B |
| Dependency size | 67.35 MB | 67.36 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 65.88 MB | 65.89 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 167 | 167 | 0 |
| Self size | 18 KB | 18 KB | 0 B |
| Dependency size | 31.39 MB | 31.40 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 58 | 0 |
| Self size | 1.19 MB | 1.19 MB | 🚨 +6 B 🚨 |
| Dependency size | 13.19 MB | 13.20 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34094
What I did
Updated channel event handling to consider
refIdwhen multiple iframes with the same origin exist.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
npx storybook@0.0.0-pr-34105-sha-d0a83f8a upgradestorybook-staticstorybook-staticfolder into thepublicfolder..storybook/main.tswith the belowrefsconfig.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 pull request has been released as version
0.0.0-pr-34105-sha-d0a83f8a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34105-sha-d0a83f8a sandboxor in an existing project withnpx storybook@0.0.0-pr-34105-sha-d0a83f8a upgrade.More information
0.0.0-pr-34105-sha-d0a83f8afix-composition-on-common-base-urld0a83f8a1773284776)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=34105Summary by CodeRabbit