Core: Ensure valid QR code URL #32661
Conversation
📝 WalkthroughWalkthroughThreaded an isDevelopment boolean (derived from global.CONFIG_TYPE) into ShareMenu; changed QR title/description and memoization; prefer network address when building story URL; set TooltipLinkList width to 210; updated stories to set/restore CONFIG_TYPE and added a Production story. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Global as global (CONFIG_TYPE, STORYBOOK_NETWORK_ADDRESS)
participant Tool as Share Tool
participant Menu as ShareMenu
participant User as User
Note over Global,Tool: render share tool at runtime
Global->>Tool: provide CONFIG_TYPE & STORYBOOK_NETWORK_ADDRESS
Tool->>Tool: isDevelopment = (CONFIG_TYPE === "DEVELOPMENT")
alt network address present
Tool->>Tool: url = new URL(window.location.search, STORYBOOK_NETWORK_ADDRESS).href
else no network address
Tool->>Tool: url = window.location.search (fallback)
end
Tool->>Menu: render({ isDevelopment, url, ..., tooltipStyle: { width: 210 } })
alt isDevelopment == true
Menu->>User: show QR + "Scan to open" + "Device must be on the same network."
else isDevelopment == false
Menu->>User: show QR + "Scan to open" + "View story on another device."
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/share.tsx (1)
155-157: LGTM! The URL constructor correctly fixes the invalid URL bug.The change from string concatenation to using the
URLconstructor properly handles the case whereSTORYBOOK_NETWORK_ADDRESSalready contains query parameters. When you pass a query string (e.g.,?path=/story/...) as the first argument to the URL constructor, it correctly replaces any existing query string in the base URL, preventing malformed URLs likehttp://host/?old=query?new=query.Consider adding error handling for robustness.
The
URLconstructor throws aTypeErrorifSTORYBOOK_NETWORK_ADDRESSis malformed. While this is likely a configuration error that should surface, consider wrapping this in a try-catch to provide a more graceful fallback:const storyUrl = global.STORYBOOK_NETWORK_ADDRESS - ? new URL(window.location.search, global.STORYBOOK_NETWORK_ADDRESS).href + ? (() => { + try { + return new URL(window.location.search, global.STORYBOOK_NETWORK_ADDRESS).href; + } catch { + return window.location.href; + } + })() : window.location.href;Consider adding tests for this fix.
Given that this addresses a specific bug (#32658), adding tests would help prevent regressions:
- Test with
STORYBOOK_NETWORK_ADDRESScontaining query params- Test with empty
window.location.search- Test with malformed URLs (if error handling is added)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/preview/tools/share.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/manager/components/preview/tools/share.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/manager/components/preview/tools/share.tsx
⏰ 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
|
View your CI Pipeline Execution ↗ for commit 3614922
☁️ Nx Cloud last updated this comment at |
|
Awesome! While you're here, I think it would make sense to change the text when in production mode, because "Must be on the same network as this device" doesn't make a lot of sense |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/manager/components/preview/tools/share.tsx (1)
147-147: AddisDevelopmentto the useMemo dependency array.The
isDevelopmentvariable is used inside theuseMemohook (line 126) but is missing from the dependency array. This will cause React to use stale values whenCONFIG_TYPEchanges, resulting in incorrect QR section text.Apply this diff:
- }, [baseUrl, storyId, queryParams, copied, qrUrl, enableShortcuts, copyStoryLink]); + }, [baseUrl, storyId, queryParams, copied, qrUrl, enableShortcuts, copyStoryLink, isDevelopment]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/tools/share.stories.tsx(2 hunks)code/core/src/manager/components/preview/tools/share.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/tools/share.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/tools/share.tsx
⏰ 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/core/src/manager/components/preview/tools/share.tsx (5)
85-91: LGTM!The
isDevelopmentparameter is correctly typed and integrated into the function signature.
126-139: LGTM!The conditional QR section text improves the user experience by providing context-appropriate messaging for development vs. production environments. This directly addresses the feedback about the network requirement message not making sense in production.
162-162: LGTM!Using the URL constructor correctly handles cases where
STORYBOOK_NETWORK_ADDRESSincludes query paths, preventing the generation of malformed QR code URLs. This directly addresses the issue described in #32658.
164-164: LGTM!The
isDevelopmentflag is correctly computed fromglobal.CONFIG_TYPE.
170-172: LGTM!The
isDevelopmentprop is correctly passed toShareMenuvia the spread syntax.code/core/src/manager/components/preview/tools/share.stories.tsx (1)
44-50: LGTM!The cleanup pattern correctly captures and restores the original
CONFIG_TYPE, ensuring proper test isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/share.tsx (1)
149-149: Consider using styled component for consistency.While the inline
styleprop works, the rest of this file uses styled components (e.g.,QRContainer,QRTitle). For consistency, consider creating a styled wrapper or adding the width constraint to the styled component pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/preview/tools/share.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/manager/components/preview/tools/share.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/manager/components/preview/tools/share.tsx
⏰ 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/core/src/manager/components/preview/tools/share.tsx (6)
85-85: LGTM: isDevelopment prop properly typed.The new prop is correctly typed and clearly named, supporting the conditional QR description logic.
Also applies to: 91-91
134-139: LGTM: Conditional QR text improves UX.The conditional description appropriately distinguishes between development (network requirement) and production (general device access) contexts, addressing the feedback about production messaging making more sense.
147-147: LGTM: useMemo dependencies correctly updated.The
isDevelopmentdependency is correctly included since it affects the QR description rendering.
161-161: LGTM: isDevelopment computation is straightforward.The equality check against
'DEVELOPMENT'is clear. IfCONFIG_TYPEis undefined or any other value,isDevelopmentdefaults tofalse, which is a sensible fallback for production-like behavior.
162-164: Excellent fix using URL constructor!This correctly resolves the bug where string concatenation produced malformed QR URLs when
STORYBOOK_NETWORK_ADDRESScontained query parameters (e.g.,?path=/onboarding).The URL constructor properly handles:
- Replacing the base URL's query with
window.location.searchwhen present- Preserving the network address scheme, host, and port
- Avoiding malformed URLs like
http://host:port/?path=/onboarding?path=/story/exampleExample scenarios:
- Base:
http://192.168.1.100:6006/?path=/onboarding, Search:?path=/story/example--default
→ Result:http://192.168.1.100:6006/?path=/story/example--default✓- Base:
http://192.168.1.100:6006, Search:?path=/story/example--default
→ Result:http://192.168.1.100:6006/?path=/story/example--default✓
170-172: LGTM: isDevelopment correctly passed to ShareMenu.The prop is properly threaded through the tooltip to
ShareMenu, completing the feature implementation.
Closes #32658
What I did
Ensure the QR code URL is a valid URL by using URL constructor rather than concatenating strings.
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>Summary by CodeRabbit
UI
Tests