Core: Add getStoryHrefs manager API and add hotkey for "open in isolation"
#33416
Conversation
|
View your CI Pipeline Execution ↗ for commit e5d6628
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 84c18fa ☁️ Nx Cloud last updated this comment at |
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
code/core/src/manager-api/modules/url.ts (2)
34-48: Consider adding input validation to parseSerializedParam.The
parseSerializedParamfunction splits on semicolons and colons without validating the input format. Malformed input (e.g.,"a:b:c","key:",":value") could produce unexpected entries in the returned object.🔎 Suggested improvement with validation
const parseSerializedParam = (param: string) => Object.fromEntries( param .split(';') .map((pair) => pair.split(':')) - .filter(([key, value]) => key && value) + .filter(([key, value]) => key && value !== undefined && pair.split(':').length === 2) );Note: The current filter already checks
key && value, which should handle most edge cases. Consider if additional validation is needed based on expected usage patterns.
227-272: Document the asymmetric globals handling for refs.Line 270 conditionally excludes globals from the preview URL when a
refIdis present (${refId ? '' : globalsParam}). This asymmetry between manager and preview URLs for refs could be confusing to consumers.Consider adding a comment explaining why globals are dropped for ref previews:
return { managerHref: `${managerBase}?path=/${viewMode}/${refId ? `${refId}_` : ''}${storyId}${argsParam}${globalsParam}${customParams}`, + // Globals are omitted from ref preview URLs as they are managed by the external Storybook instance previewHref: `${previewBase}?id=${storyId}&viewMode=${viewMode}${argsParam}${refId ? '' : globalsParam}${customParams}`, };code/addons/docs/src/blocks/getStoryHref.ts (1)
15-24: Consider edge case handling in parseQuery.The
parseQueryfunction doesn't handle edge cases like:
- Missing values (e.g.,
key=or justkey)- Multiple
=signs in a pair (e.g.,key=value=extra)- Empty pairs from consecutive
&charactersWhile these might be unlikely in practice, they could cause subtle bugs.
🔎 More robust parsing
function parseQuery(queryString: string) { const query: Record<string, string> = {}; const pairs = queryString.split('&'); for (let i = 0; i < pairs.length; i++) { + if (!pairs[i]) continue; const pair = pairs[i].split('='); - query[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1] || ''); + const key = decodeURIComponent(pair[0]); + const value = decodeURIComponent(pair.slice(1).join('=') || ''); + if (key) query[key] = value; } return query; }This handles empty pairs, missing values, and multiple
=signs more gracefully.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
code/addons/docs/src/blocks/components/Preview.tsxcode/addons/docs/src/blocks/components/Story.tsxcode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/components/components/utils/getStoryHref.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/utils/types.tsx
💤 Files with no reviewable changes (1)
- code/addons/docs/src/blocks/components/Preview.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/manager-api/tests/url.test.js
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager/components/preview/Preview.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager/components/preview/Preview.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager/components/preview/Preview.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/manager-api/tests/url.test.js
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager/components/preview/Preview.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager/components/preview/Preview.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
🧠 Learnings (7)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Story.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager-api/modules/url.tscode/addons/docs/src/blocks/components/Toolbar.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/core/src/manager-api/tests/url.test.jscode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/addons/docs/src/blocks/components/Toolbar.tsxcode/core/src/components/components/utils/getStoryHref.tscode/addons/docs/src/blocks/components/Story.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/addons/docs/src/blocks/components/Toolbar.tsx
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/node-logger` for server-side logging in Node.js code
Applied to files:
code/core/src/components/components/utils/getStoryHref.ts
🧬 Code graph analysis (6)
code/core/src/manager/components/preview/FramesRenderer.tsx (1)
code/core/src/manager-api/version.ts (1)
version(1-1)
code/core/src/manager-api/modules/url.ts (1)
code/core/src/manager-api/tests/url.test.js (2)
store(155-163)store(254-259)
code/addons/docs/src/blocks/components/Toolbar.tsx (2)
code/addons/docs/src/blocks/getStoryHref.ts (1)
getStoryHref(4-13)code/core/src/components/components/utils/getStoryHref.ts (1)
getStoryHref(15-37)
code/addons/docs/src/blocks/getStoryHref.ts (1)
code/core/src/components/components/utils/getStoryHref.ts (1)
getStoryHref(15-37)
code/core/src/components/components/utils/getStoryHref.ts (2)
code/addons/docs/src/blocks/getStoryHref.ts (1)
getStoryHref(4-13)code/core/src/node-logger/index.ts (1)
deprecate(95-95)
code/addons/docs/src/blocks/components/Story.tsx (2)
code/addons/docs/src/blocks/getStoryHref.ts (1)
getStoryHref(4-13)code/core/src/components/components/utils/getStoryHref.ts (1)
getStoryHref(15-37)
⏰ 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). (5)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
- GitHub Check: nx
🔇 Additional comments (10)
code/addons/docs/src/blocks/components/Story.tsx (1)
5-5: LGTM! Clean migration to local getStoryHref utility.The changes correctly update the import path and simplify the function call signature by removing the baseUrl parameter, aligning with the new URL generation pattern.
Also applies to: 10-10, 99-99
code/core/src/components/components/utils/getStoryHref.ts (1)
1-1: LGTM! Proper deprecation pattern implemented.The deprecation follows best practices with both JSDoc annotation and runtime warning, providing a clear migration path to
api.getStoryHrefs. The existing implementation is preserved for backward compatibility.Also applies to: 14-22
code/core/src/manager-api/modules/url.ts (1)
343-353: LGTM! Consistent use of global.window.The changes to use
global.windowinstead of a standalonewindowreference are consistent with the global imports at the top of the file.code/core/src/manager-api/tests/url.test.js (1)
9-27: LGTM! Comprehensive test coverage for getStoryHrefs.The test suite thoroughly covers the new API including:
- Basic URL generation
- Args and globals inheritance and merging
- Base URL options (origin/network)
- Ref handling
- PREVIEW_URL overrides
The mock setup for
@storybook/globalis appropriate for testing absolute URL generation.Also applies to: 252-408
code/core/src/manager/components/preview/utils/types.tsx (1)
52-52: LGTM! Type updated to include the API.The addition of
api: APItoFramesRendererPropscorrectly reflects the new prop being passed from Preview.tsx to FramesRenderer.code/addons/docs/src/blocks/components/Toolbar.tsx (1)
4-4: LGTM! Clean migration to local getStoryHref utility.The changes consistently remove the
baseUrldependency and switch to the localgetStoryHrefutility, simplifying the component's props and call signatures.Also applies to: 10-10, 56-56, 117-117
code/core/src/manager/components/preview/Preview.tsx (1)
22-33: LGTM! API correctly passed through to FramesRenderer.The changes properly thread the
apithrough the component tree viacanvasMapperand the Canvas consumer, enabling FramesRenderer to use the newapi.getStoryHrefsmethod for URL generation.Also applies to: 156-166, 206-216
code/core/src/manager/components/preview/FramesRenderer.tsx (3)
4-4: Import cleanup looks good.The removal of
getStoryHreffrom imports aligns with the deprecation strategy outlined in the PR.
56-56: Refactoring to use the new API is implemented correctly.The transition from
getStoryHreftoapi.getStoryHrefs(...).previewis clean and maintains the same functionality. The API correctly receives all necessary parameters (storyId,queryParamswith version,refId,viewMode), and the.previewaccessor appropriately extracts the preview URL from the returned object.Also applies to: 77-81
76-91: Appropriate handling of main vs. external ref iframes.The implementation correctly distinguishes between the main preview iframe (which uses the new API) and external ref iframes (which require manual URL construction with their own base URLs). This architectural decision is sound given that external refs point to separate Storybook instances.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.19 MB | 20.18 MB | 🎉 -14 KB 🎉 |
| Dependency size | 16.52 MB | 16.52 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 0 B |
| Dependency size | 67.24 MB | 67.23 MB | 🎉 -14 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 65.82 MB | 65.80 MB | 🎉 -14 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 999 KB | 999 KB | 0 B |
| Dependency size | 36.71 MB | 36.69 MB | 🎉 -14 KB 🎉 |
| Bundle Size Analyzer | node | node |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
code/core/src/manager/components/preview/tools/share.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/core/src/manager/components/preview/tools/share.stories.tsx (1)
18-21: Fix shortcut key case sensitivity in mock: use uppercase letters.The mock shortcut keys use lowercase letters (
'l'and'i'), but the actual implementation inshortcuts.tsuses uppercase ('L'and'I'). Update lines 18-21 to:getShortcutKeys: () => ({ copyStoryLink: ['alt', 'shift', 'L'], openInIsolation: ['alt', 'shift', 'I'], }),code/core/src/manager-api/modules/url.ts (2)
35-43: Handle colons in parameter values.The
split(':')on line 39 splits on every colon. If a value contains colons (e.g.,"theme:dark:blue"), only the first two segments are captured by the destructuring[key, value], discarding the rest (e.g.,":blue"). This causes data loss when args or globals contain colon-separated values.🔎 Proposed fix using split with limit
const parseSerializedParam = (param: string) => Object.fromEntries( param .split(';') - .map((pair) => pair.split(':')) + .map((pair) => pair.split(':', 2)) // Encoding values ensures we don't break already encoded args/globals but also don't encode our own special characters like ; and :. .map(([key, value]) => [key, encodeURIComponent(value)]) .filter(([key, value]) => key && value) );This limits the split to at most 2 parts, preserving colons in the value.
269-275: URL-encode path segments and query parameters.The serialized
argsParamandglobalsParamstrings (containing;and:delimiters) are concatenated directly into query strings without URL encoding the entire serialized string. While individual values within these strings are already encoded byparseSerializedParam, the structural characters could still conflict with the&delimiter in the query string. Additionally,refIdandstoryIdin themanagerHrefpath (line 274) andstoryIdin thepreviewHrefquery (line 275) are not URL-encoded.🔎 Proposed fix to add URL encoding
- argsParam = argsParam && `&args=${argsParam}`; - globalsParam = globalsParam && `&globals=${globalsParam}`; + argsParam = argsParam && `&args=${encodeURIComponent(argsParam)}`; + globalsParam = globalsParam && `&globals=${encodeURIComponent(globalsParam)}`; customParams = customParams && `&${customParams}`; return { - managerHref: `${managerBase}?path=/${viewMode}/${refId ? `${refId}_` : ''}${storyId}${argsParam}${globalsParam}${customParams}`, - previewHref: `${previewBase}?id=${storyId}&viewMode=${viewMode}${refParam}${argsParam}${refId ? '' : globalsParam}${customParams}`, + managerHref: `${managerBase}?path=/${viewMode}/${refId ? `${encodeURIComponent(refId)}_` : ''}${encodeURIComponent(storyId)}${argsParam}${globalsParam}${customParams}`, + previewHref: `${previewBase}?id=${encodeURIComponent(storyId)}&viewMode=${viewMode}${refParam}${argsParam}${refId ? '' : globalsParam}${customParams}`, };Note: Verify that
stringify()from picoquery already handles encoding forcustomParams.
🧹 Nitpick comments (1)
code/core/src/manager-api/tests/url.test.js (1)
16-27: Consider using thespy: trueoption for consistency.The coding guidelines recommend using
vi.mock()with thespy: trueoption for all package mocks. While this mock only provides static values rather than functions to spy on, addingspy: truewould maintain consistency with the testing guidelines.🔎 Proposed change
-vi.mock('@storybook/global', () => ({ +vi.mock('@storybook/global', { spy: true }, () => ({ global: { window: { location: { hash: '', href: 'http://localhost:6006', origin: 'http://localhost:6006', }, }, STORYBOOK_NETWORK_ADDRESS: 'http://192.168.1.1:6006/', }, }));Note: If
spy: truecauses issues with this static value mock, the current approach is acceptable.Based on coding guidelines.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/addons/docs/src/blocks/getStoryHref.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/tools/share.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/docs/src/blocks/getStoryHref.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/modules/url.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/modules/url.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/modules/url.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/modules/url.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/modules/url.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/manager-api/tests/url.test.js
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/manager-api/tests/url.test.js
🧠 Learnings (7)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.js
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/modules/url.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/tests/url.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/manager-api/tests/url.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/core/src/manager-api/tests/url.test.js
🧬 Code graph analysis (1)
code/core/src/manager-api/modules/url.ts (1)
code/core/src/manager-api/tests/url.test.js (2)
store(155-163)store(254-259)
⏰ 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: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/manager/components/preview/tools/share.stories.tsx (1)
22-25: Mock correctly implements the API property names.The
getStoryHrefsmock now correctly returns{ managerHref, previewHref }, addressing the issue flagged in a previous review. The property names match the actual API implementation.code/core/src/manager-api/modules/url.ts (3)
45-51: LGTM!The merge logic correctly parses both parameter strings, merges them with proper precedence (extra params override existing), and re-serializes back to the expected format.
141-167: LGTM!The JSDoc documentation is comprehensive and clearly describes the method signature, all options, and return value.
348-352: LGTM!The change from
globalWindowtoglobal.windowis consistent with the pattern used elsewhere in this file and correctly accesses the idle callback APIs.code/core/src/manager-api/tests/url.test.js (1)
252-457: Excellent test coverage!The test suite comprehensively covers the
getStoryHrefsfunctionality with well-structured tests for:
- Basic URL generation
- Args/globals inheritance and merging behavior
- Special value preservation (e.g.,
!null,!hex())- Query parameter encoding and preservation
- Absolute URL generation with base options
- Ref-based URL generation
- Custom nested query parameters
The tests are well-organized, have clear naming, and cover both happy paths and edge cases. The test at lines 388-403 specifically addresses the encoding of custom query params, which was mentioned in past review comments.
…nal getStoryHref helper
Sidnioulz
force-pushed
the
isolation-mode-shortcut
branch
from
68aae62 to
e5d6628
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
code/core/src/manager-api/modules/url.ts (2)
35-43: Colon handling in serialized values may truncate data.
split(':')at line 39 splits on every colon. If a value contains colons (e.g., a URL likehttp://example.com), only the first segment after the key is captured, discarding the rest.The previous review flagged this issue. Consider using
split(':', 2)to limit the split:🔎 Proposed fix
const parseSerializedParam = (param: string) => Object.fromEntries( param .split(';') - .map((pair) => pair.split(':')) + .map((pair) => { + const colonIndex = pair.indexOf(':'); + return colonIndex === -1 ? [pair, ''] : [pair.slice(0, colonIndex), pair.slice(colonIndex + 1)]; + }) .map(([key, value]) => [key, encodeURIComponent(value)]) .filter(([key, value]) => key && value) );
269-276: URL encoding for args/globals parameters.The
argsParamandglobalsParamvalues contain serialized data with special characters (;,:) that are concatenated directly into the URL. While the values within are encoded viaencodeURIComponentinparseSerializedParam, the overall structure with;and:delimiters remains unencoded.This was flagged in a previous review. Verify whether the current encoding strategy is intentional to preserve the custom serialization format, or if full URL encoding is needed.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/utils/types.tsx (1)
51-60: Remove unusedbaseUrlfromFramesRendererProps.The
baseUrlprop (line 56) is defined in the interface but unused in theFramesRenderercomponent, which now uses API-based URL generation viaapi.getStoryHrefs(). Removing it will eliminate dead code and avoid confusion about its purpose.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
code/addons/docs/src/blocks/components/Preview.tsxcode/addons/docs/src/blocks/components/Story.tsxcode/addons/docs/src/blocks/components/Toolbar.tsxcode/addons/docs/src/blocks/getStoryHref.tscode/core/src/components/components/utils/getStoryHref.tscode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/utils/stringifyQueryParams.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager/settings/defaultShortcuts.tsxcode/core/src/manager/settings/shortcuts.tsxcode/core/src/typings.d.ts
💤 Files with no reviewable changes (2)
- code/addons/docs/src/blocks/components/Preview.tsx
- code/core/src/manager/components/preview/utils/stringifyQueryParams.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- code/addons/docs/src/blocks/getStoryHref.ts
- code/core/src/manager-api/tests/url.test.js
- code/core/src/manager-api/modules/shortcuts.ts
- code/core/src/components/components/utils/getStoryHref.ts
- code/core/src/manager/components/preview/Preview.tsx
- code/core/src/manager/settings/shortcuts.tsx
- code/core/src/typings.d.ts
- code/addons/docs/src/blocks/components/Story.tsx
- code/core/src/manager/settings/defaultShortcuts.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/utils/types.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
🧠 Learnings (9)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/tools/share.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/tools/share.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/addons/docs/src/blocks/components/Toolbar.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/tools/share.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/core/src/manager/components/preview/tools/share.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/addons/docs/src/blocks/components/Toolbar.tsx
🧬 Code graph analysis (4)
code/core/src/manager-api/modules/url.ts (1)
code/core/src/manager-api/tests/url.test.js (2)
store(155-163)store(254-259)
code/core/src/manager/components/preview/FramesRenderer.tsx (1)
code/core/src/manager-api/version.ts (1)
version(1-1)
code/core/src/manager/components/preview/tools/share.tsx (3)
code/core/src/components/index.ts (3)
TooltipLinkList(86-86)PopoverProvider(71-71)Button(56-56)code/core/src/types/modules/addons.ts (1)
Addon_BaseType(329-391)code/core/src/components/components/Popover/PopoverProvider.tsx (1)
PopoverProvider(55-102)
code/addons/docs/src/blocks/components/Toolbar.tsx (2)
code/addons/docs/src/blocks/getStoryHref.ts (1)
getStoryHref(3-15)code/core/src/components/components/utils/getStoryHref.ts (1)
getStoryHref(15-37)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (14)
code/addons/docs/src/blocks/components/Toolbar.tsx (3)
4-10: LGTM! Clean migration to local utility.The import change correctly switches from the deprecated
getStoryHrefin internal components to the new local utility that handles URL construction without requiring abaseUrlprop.
56-56: Props cleanup looks correct.The
baseUrlprop removal from the component signature aligns with the interface change inEjectProps(lines 17-19) and the switch to the localgetStoryHrefutility.
117-117: Verify the localgetStoryHrefhandles edge cases.The simplified call
getStoryHref(storyId)relies on the local utility to derive the base URL fromglobalThis.PREVIEW_URL || 'iframe.html'. Ensure this works correctly in all docs contexts (standalone docs, embedded docs, etc.).code/core/src/manager/components/preview/tools/share.stories.tsx (2)
18-25: Mock API correctly updated.The mock now returns
{ managerHref, previewHref }matching the actualgetStoryHrefsAPI signature. ThegetShortcutKeysmock includes the newopenInIsolationshortcut.
47-62: Good test coverage for development mode.The story correctly sets up globals (
STORYBOOK_NETWORK_ADDRESS,CONFIG_TYPE) and cleans them up via thebeforeEachreturn function. The play function validates the QR code section renders correctly.code/core/src/manager-api/modules/url.ts (2)
230-246: Solid implementation ofgetStoryHrefs.The method correctly:
- Determines if viewing the current story to default
inheritArgs- Validates
refIdagainst available refs- Handles origin vs network base URL resolution
The logic flow is clear and well-documented.
348-352: Consistent use ofglobal.window.The change from
globalWindowtoglobal.windowaligns with the pattern used elsewhere in this file and improves consistency.code/core/src/manager/components/preview/FramesRenderer.tsx (3)
54-62: Clean API-based refactor.The component now accepts
apiinstead ofbaseUrl, centralizing URL generation logic. The destructuring is clean and the signature change aligns withFramesRendererProps.
71-77: Main iframe URL generation looks correct.The URL is generated once (guarded by the
if (!frames['storybook-preview-iframe'])check) usingapi.getStoryHrefswith proper parameters including version and viewMode.
79-88: Ref iframe URL invalidation logic.The check
!frames[id]?.startsWith(ref.url)ensures frames are regenerated when the ref URL changes. This is a reasonable cache invalidation strategy.However, note that this only checks if the cached URL starts with the ref URL—it won't invalidate if
queryParams,storyId, orviewModechange. Since the component is typically re-mounted or the entire frames ref is reset on story changes, this should be acceptable.code/core/src/manager/components/preview/tools/share.tsx (4)
17-20: Clean mapper pattern.The mapper extracts exactly what ShareMenu needs from the Combo, keeping the component decoupled from the full state shape.
59-67: ShareMenu signature is well-typed.The component correctly accepts
api,storyId, andrefIdprops with appropriate types. UsingReact.memois a good optimization for the tooltip content.
74-121: Well-structured links with proper URL sources.The implementation correctly:
- Uses
originHrefsfor copy/open actions (same-origin use case)- Uses
networkHrefsfor QR code (cross-device scanning)- Includes the new
openInIsolationshortcut- Has complete useMemo dependencies
The QR description adapts based on
CONFIG_TYPE, providing appropriate user guidance.
131-148: Render function uses Consumer pattern correctly.The Consumer with mapper pattern cleanly provides the required props to ShareMenu. The conditional rendering (
storyId ?) ensures the share button only appears when a story is selected.
|
Everything looks great to me! The one thing I noticed that the keyboard shortcut also works for a docs page, should it? Considering the share menu is absent in the UI when viewing docs pages. |
I don't think it hurts to have it. People might want to embed such a docs page in another website, or debug rare interactions between manager-head scripts and their docs. |
|
Yeah, it doesn't hurt to have it as an undocumented feature. |
What I did
getStoryHrefsmethod tourlsmanager API moduleOption-Shift-I/Alt-Shift-I), usingapi.getStoryHrefsapi.getStoryHrefsShareMenucomponent to useapi.getStoryHrefsfor all URLs, including QR codegetStoryHrefutil in manager componentsgetStoryHrefinto addon-docs source, because that's the only place it's still used and cannot be replaced withapi.getStoryHrefsdue to being in the preview. Yet another reason to not render docs pages in the preview.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor
Deprecations
✏️ Tip: You can customize this high-level summary in your review settings.