Core: Add SHARE_PROVIDER addon type and migrate QR Code sharing to it#33653
Core: Add SHARE_PROVIDER addon type and migrate QR Code sharing to it#33653ghengeveld wants to merge 81 commits into
SHARE_PROVIDER addon type and migrate QR Code sharing to it#33653Conversation
…e addon that uses the new API
|
View your CI Pipeline Execution ↗ for commit 830979a
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThis PR introduces a QR code manager addon, extends the addon system with experimental share providers, refactors the share tool to support provider-driven flows, adds reactive globals support to the Vue 3 renderer with generic updateArgs, and includes Vue 3 template decorators and documentation examples. Changes
Sequence DiagramssequenceDiagram
actor User
participant Manager as Manager UI
participant AddonSystem as Addon System
participant ShareModal as Share Modal
participant Provider as Share Provider
User->>Manager: Click Share Tool
Manager->>AddonSystem: getElements(experimental_SHARE_PROVIDER)
AddonSystem-->>Manager: Return registered share providers
Manager->>Manager: Map providers via shareMapper
Manager->>ShareModal: Render modal with provider tabs
ShareModal-->>User: Display provider list
User->>ShareModal: Select provider
ShareModal->>Provider: Invoke provider.render()
Provider-->>ShareModal: Return provider UI
ShareModal-->>User: Display provider content
sequenceDiagram
actor User
participant Story as Vue3 Story
participant Decorator as Reactive Decorator
participant Toolbar as Storybook Toolbar
participant Watcher as Computed Watcher
Story->>Decorator: Initialize with reactiveGlobals
Decorator->>Watcher: Setup computed greeting from locale
Decorator-->>User: Render story with initial greeting
User->>Toolbar: Change locale global
Toolbar->>Decorator: updateArgs(Globals, {locale: 'es'})
Decorator->>Watcher: Update reactive globals object
Watcher-->>Story: Trigger computed re-evaluation
Watcher-->>User: Display updated greeting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing touches
Comment |
![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
🤖 Fix all issues with AI agents
In `@code/core/src/qrcode/manager.tsx`:
- Around line 48-73: The anonymous lowercased shareMenu function uses hooks
(useStorybookApi) directly which violates hook usage conventions; extract it
into a capitalized React component (e.g., ShareMenu) that calls useStorybookApi
and returns the same JSX (QRContainer/QRImage/QRContent/QRTitle/QRDescription)
and then pass that component reference instead of the function to addons.add via
the shareMenu property; ensure you import React if needed and keep the same
logic to read storyId, refId and networkHrefs (api.getCurrentStoryData,
api.getStoryHrefs) and return null when no storyId.
🧹 Nitpick comments (1)
code/core/src/manager-api/modules/addons.ts (1)
23-25: Consider updating the JSDoc example.The JSDoc comment on line 23 mentions only
experimental_PAGEas an example type parameter. Consider updating it to also mentionexperimental_SHARE_PROVIDERandexperimental_TEST_PROVIDERfor completeness.📝 Suggested documentation update
/** * Returns a collection of elements of a specific type. * * `@template` T - The type of the elements in the collection. - * `@param` {Addon_Types | Addon_TypesEnum.experimental_PAGE} type - The type of the elements to + * `@param` {Addon_Types | Addon_TypesEnum.experimental_PAGE | Addon_TypesEnum.experimental_SHARE_PROVIDER | Addon_TypesEnum.experimental_TEST_PROVIDER} type - The type of the elements to * retrieve.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
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/21373763000 |
| 'storybook/test': './src/test', | ||
| 'storybook/internal': './src', | ||
| 'storybook/outline': './src/outline', | ||
| 'storybook/qrcode': './src/qrcode', |
There was a problem hiding this comment.
| 'storybook/qrcode': './src/qrcode', |
This interface has to be exported to prevent "Default export of the module has or is using private name" errors when attempting to use `meta.story()`.
The CSF parser was incorrectly identifying Zod v4's `.meta()` method calls as Storybook's CSF Factories `meta()` function when the object calling `.meta()` was a directly imported variable. This caused the error: ``` BadMetaError: CSF: meta() factory must be imported from .storybook/preview configuration ``` The fix changes the detection logic to only throw BadMetaError when the variable calling `.meta()` is specifically named "preview". This is the canonical name used in CSF Factories (`preview.meta()`), so if someone has a variable with this name but imports it from the wrong path, they get a helpful error. For all other variable names (like `mySchema` from Zod), the `.meta()` call is silently ignored since it's clearly not attempting to use CSF Factories. Closes #33654
The csf-factories automigration now respects the `--yes` flag to skip
interactive prompts. When `--yes` is passed:
- Uses default glob: `**/*.{stories,story}.{js,jsx,ts,tsx,mjs,mjsx,mts,mtsx}`
- Uses default import type: subpath imports
This enables running the automigration in CI or scripts without user input.
Example:
```bash
npx storybook automigrate csf-factories --yes
```
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Sandbox always uses limited glob, non-interactive mode uses default glob. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
…ck out to the tab bar
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/_snippets/automocked-modules-in-story.md (1)
200-200:⚠️ Potential issue | 🟡 MinorInconsistent Svelte import syntax.
This line still uses named import syntax
import { AuthButton } from './AuthButton.svelte';, while the other Svelte snippets in this file (lines 133, 166, 231) were updated to use default import syntax. For consistency and correctness, this should also use a default import.📝 Proposed fix for consistency
- import { AuthButton } from './AuthButton.svelte'; + import AuthButton from './AuthButton.svelte';code/core/src/common/js-package-manager/PNPMProxy.ts (1)
105-117:⚠️ Potential issue | 🟠 MajorFix npm version incompatibility across package manager proxies.
The
-ws=falseand-iwrflags require npm 8.0.0+, but test coverage includes npm 7.24.0, indicating support for npm 7.x is expected. Without a fallback,getRegistryURL()will throw on npm 7.x. This affects PNPMProxy, NPMProxy, and BUNProxy identically.Add error handling with fallback to retry without workspace flags:
Suggested fallback implementation
public async getRegistryURL() { // pnpm 10.7.1+ falls back to npm for certain config keys (including registry) // https://github.com/pnpm/pnpm/pull/9346 // "npm config" commands are not allowed in workspaces per default // https://github.com/npm/cli/issues/6099#issuecomment-1847584792 - const childProcess = await executeCommand({ - command: 'npm', - cwd: this.cwd, - args: ['config', 'get', 'registry', '-ws=false', '-iwr'], - }); - const url = (typeof childProcess.stdout === 'string' ? childProcess.stdout : '').trim(); - return url === 'undefined' ? undefined : url; + try { + const childProcess = await executeCommand({ + command: 'npm', + cwd: this.cwd, + args: ['config', 'get', 'registry', '-ws=false', '-iwr'], + }); + const url = (typeof childProcess.stdout === 'string' ? childProcess.stdout : '').trim(); + return url === 'undefined' ? undefined : url; + } catch (error) { + logger.debug( + `npm config get registry failed with workspace flags, retrying without: ${String(error)}` + ); + const childProcess = await executeCommand({ + command: 'npm', + cwd: this.cwd, + args: ['config', 'get', 'registry'], + }); + const url = (typeof childProcess.stdout === 'string' ? childProcess.stdout : '').trim(); + return url === 'undefined' ? undefined : url; + } }Apply the same pattern to NPMProxy and BUNProxy.
code/addons/vitest/src/postinstall.ts (1)
5-35:⚠️ Potential issue | 🟡 MinorAvoid treating an unrelated
storybookTest()call as “already configured.”
callee.name === 'storybookTest'can match a local helper that isn’t imported from the addon source. Prefer requiring an import match to avoid false positives.🛠️ Suggested adjustment
- if ( - callee.type === 'Identifier' && - (pluginIdentifiers.has(callee.name) || callee.name === 'storybookTest') - ) { + if (callee.type === 'Identifier' && pluginIdentifiers.has(callee.name)) { pluginReferenced = true; path.stop(); }Also applies to: 427-468
code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
17-35:⚠️ Potential issue | 🟠 MajorGuard against infinite retries when no files match.
In non‑interactive runs (yes/glob/sandbox),No files matchedre-enters with the same glob and can loop indefinitely. Bail out or surface the error when prompting isn’t possible.🐛 Suggested fix
async function runStoriesCodemod(options: { dryRun: boolean | undefined; packageManager: JsPackageManager; useSubPathImports: boolean; previewConfigPath: string; yes: boolean | undefined; glob: string | undefined; }) { - const { dryRun, packageManager, yes, glob, ...codemodOptions } = options; + const { dryRun, packageManager, yes, glob, ...codemodOptions } = options; + const inSandbox = optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX) ?? false; try { - const inSandbox = optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX) ?? false; let globString = glob ?? '**/*.{stories,story}.{js,jsx,ts,tsx,mjs,mjsx,mts,mtsx}'; @@ } catch (err: any) { if (err.message === 'No files matched') { - await runStoriesCodemod(options); + if (glob || yes || inSandbox) { + throw err; + } + await runStoriesCodemod(options); } else { throw err; } } }Also applies to: 51-52
🤖 Fix all issues with AI agents
In `@CHANGELOG.prerelease.md`:
- Around line 1-19: The changelog is missing a release-note for the new
share-provider and QR migration; add a concise bullet under the 10.3.0-alpha.2
section that mentions the introduction of experimental_SHARE_PROVIDER (or
"share-provider") and that QR sharing was migrated into a provider (include the
change in behavior and how users can opt-in via experimental_SHARE_PROVIDER),
referencing the feature names exactly (experimental_SHARE_PROVIDER, QR sharing,
share-provider) so users can discover the new addon type and behavioral change.
In `@code/addons/vitest/src/postinstall.ts`:
- Around line 176-183: The code computes vitestSetupFile using fileExtension
even when an existingSetupFile with a different extension is present, causing
the generated config to point at a non-existent file; change the logic so that
after computing existingSetupFile (from EXTENSIONS and options.configDir) you
set vitestSetupFile = existingSetupFile when one is found (otherwise fall back
to resolve(options.configDir, `vitest.setup.${fileExtension}`)), ensuring
vitestSetupFile always references the actual existing file and not a hard-coded
default.
- Around line 242-250: The code currently returns early when
isConfigAlreadySetup(vitestWorkspaceFile, workspaceFileContent) is true, which
skips subsequent steps (a11y setup and final summary); remove the early return
and instead log the success message via logger.step(CLI_COLORS.success(...)) but
allow execution to continue so later steps run; apply the same change for the
other occurrence around lines 298-315 (the other isConfigAlreadySetup check) so
both branches only log and do not return early.
In `@code/addons/vitest/src/updateVitestFile.ts`:
- Around line 14-17: The forEach callback currently uses a parenthesized
assignment (template = ...) which makes the arrow function implicitly return a
value; change the callback to a statement body so it does not return anything:
update the Object.entries(replacements).forEach(([key, value]) => ...) callback
to use a block body and perform template = template.replace(key,
normalize(value)); inside it (or replace the forEach with a for...of loop) while
keeping the use of replacements, template, and normalize intact.
In `@code/core/src/telemetry/detect-agent.ts`:
- Around line 77-79: The comment above the stdoutIsTTY check in detect-agent.ts
is misleading—either update the comment to remove "exclude CI" or actually guard
for CI; to fix, choose one: (A) change the comment to reflect that the current
check only returns undefined when stdoutIsTTY is true, or (B) add a CI guard
(e.g., detect common CI env vars like CI, GITHUB_ACTIONS, GITLAB_CI) alongside
the stdoutIsTTY condition so detect-agent.ts returns undefined in CI too;
reference the stdoutIsTTY check and the surrounding comment to locate where to
apply the change.
- Around line 1-8: detectExplicitAgent is missing a branch for the 'opencode'
KnownAgentName; add an explicit detection that checks process.env.OPENCODE (or
equivalent env var) and returns 'opencode' when present. Update
detectExplicitAgent to inspect OPENCODE alongside existing checks (e.g., for
CLAUDE_CLI, GEMINI_CLI) and return the string 'opencode' so it matches the
KnownAgentName union.
In `@code/e2e-tests/addon-mcp.spec.ts`:
- Line 11: Replace the non-null assertion on process.env.STORYBOOK_SANDBOX_DIR
(assigned to sandboxDir) with an explicit guard that throws a clear Error when
the env var is missing; locate the assignment to sandboxDir in addon-mcp.spec.ts
and any other usages (the similar occurrences around the second block) and
change them to check if process.env.STORYBOOK_SANDBOX_DIR is truthy, set
sandboxDir from it, otherwise throw an Error with a descriptive message like
"STORYBOOK_SANDBOX_DIR must be set (set by e2e-tests-build.ts)". This mirrors
the defensive pattern used in storybook-hooks.spec.ts and ensures consistent,
informative failures.
In `@docs/_snippets/component-story-with-custom-render-function.md`:
- Around line 497-506: The module script tag is missing TypeScript mode; update
the <script module> tag to include lang="ts" so the example is treated as
TypeScript (affecting the snippet that imports defineMeta, Layout, and
MyComponent and defines Story via defineMeta).
In `@docs/_snippets/decorator-with-reactive-globals.md`:
- Around line 10-18: The Vue template attribute binding uses invalid moustache
braces; update the template in the snippet (the template string that returns
greeting and globals) to use proper Vue binding syntax by replacing
:lang={{globals?.locale || 'en'}} with :lang="globals?.locale || 'en'" (and make
the same change for the other occurrence referenced at lines 40-44); keep the
computed greeting and globals symbols as-is.
In `@docs/_snippets/decorator-with-updateArgs.md`:
- Around line 30-34: The snippet references MyComponent in the type
StoryObj<Meta<typeof MyComponent>> but MyComponent is not imported or defined;
update the type to avoid the missing symbol by replacing the Meta<typeof
MyComponent> usage with a generic shape (e.g., StoryObj<{ args: { counter:
number } }>) or add an import for MyComponent; specifically edit the declaration
of WithIncrementDecorator (and the import list for types) so StoryObj no longer
depends on the undefined MyComponent symbol.
🧹 Nitpick comments (9)
docs/_snippets/nextjs-router-override-in-story.md (1)
82-82: Consider broader consistency if updating snippet formatting.This blank line addition makes the CSF Next JS snippet consistent with the CSF Next TS snippet (line 55), but the CSF 3 snippets (lines 1-22 and 24-51) don't have blank lines between their imports. If you're standardizing formatting across these snippets, consider applying the same blank-line style to all four examples for consistency.
Additionally, this change appears unrelated to the PR's stated purpose of adding the
SHARE_PROVIDERaddon type and migrating QR Code sharing.docs/_snippets/loader-story.md (1)
537-537: Minor formatting change unrelated to PR objectives.An empty line has been added after the import statement. This appears to be a formatting adjustment, possibly from running Prettier. Like the Angular import changes, this is unrelated to the SHARE_PROVIDER addon feature.
Consider whether formatting-only changes to documentation snippets should be included in this feature PR or deferred to a separate cleanup pass.
scripts/bench/bench-packages.ts (1)
447-447: Consider refining the type annotation for accuracy.The actual runtime types for these options are broader than annotated due to commander's optional value behavior:
pullRequestcan benumber | true | null | undefinedbaseBranchcan bestring | true | undefinedWhile the runtime checks on lines 449 and 504 handle all cases correctly, aligning the type annotation would improve type safety and documentation.
Suggested type refinement
- const options = program.opts<{ pullRequest?: number; baseBranch?: string; upload?: boolean }>(); + const options = program.opts<{ + pullRequest?: number | true | null; + baseBranch?: string | true; + upload?: boolean; + }>();code/core/src/core-server/utils/strip-comments-and-strings.test.ts (1)
1-30: Test coverage looks good for basic cases.The tests cover the primary use cases well. Consider adding edge case tests to improve robustness:
💡 Suggested additional edge case tests
it('should preserve code outside strings and comments', () => { expect(stripCommentsAndStrings('module.exports = {}')).toBe('module.exports = {}'); expect(stripCommentsAndStrings('exports.foo = bar')).toBe('exports.foo = bar'); }); + + it('should handle escaped quotes inside strings', () => { + expect(stripCommentsAndStrings('const s = "hello \\"world\\""')).toBe('const s = ""'); + }); + + it('should handle multiple strings on one line', () => { + expect(stripCommentsAndStrings('import { a } from "a"; import { b } from "b"')).toBe( + 'import { a } from ""; import { b } from ""' + ); + }); + + it('should handle multiline block comments', () => { + expect(stripCommentsAndStrings('const a = 1;\n/* comment\nspanning\nlines */\nconst b = 2;')).toBe( + 'const a = 1;\n\nconst b = 2;' + ); + }); });code/core/src/qrcode/manager.tsx (1)
54-58: Minor: Consider clearing timeout on unmount.If the component unmounts before the 2-second timeout completes,
setCopied(false)will be called on an unmounted component. While React 18 suppresses the warning, it's cleaner to useuseEffectcleanup.♻️ Optional fix using useEffect for cleanup
+import React, { useState, useEffect, useRef } from 'react'; + const ShareProviderRender = () => { const api = useStorybookApi(); const theme = useTheme(); const [copied, setCopied] = useState(false); + const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); + + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); // ... existing code ... const copyLink = () => { copy(networkHrefs.managerHref); setCopied(true); - setTimeout(() => setCopied(false), 2000); + timeoutRef.current = setTimeout(() => setCopied(false), 2000); };code/core/src/manager/index.stories.tsx (2)
99-104: Shared provider instance may cause state leakage between stories.The
ReactProvideris instantiated once at the meta level and shared across all stories. SinceReactProvidermaintains mutable state (wsDisconnected, channel listeners), stories may interfere with each other when run in sequence. For example,ConnectionLostsetswsDisconnected = true, which persists for subsequent stories.Consider using a factory function to create a fresh provider per story:
♻️ Suggested refactor
const meta = preview.meta({ title: 'Main', component: Main, - args: { - provider: new ReactProvider(), - }, + render: () => { + const provider = new ReactProvider(); + return <Main provider={provider} />; + },
82-95: Channel listener is never removed.The
CHANNEL_WS_DISCONNECTlistener is registered inhandleAPIbut never cleaned up. IfhandleAPIwere called multiple times, this would register duplicate listeners. While this may be acceptable for the current test setup, consider storing and cleaning up the listener for robustness.code/core/src/common/utils/get-storybook-refs.test.ts (1)
5-33: Move fetch mocking into a beforeEach-driven setup (avoid inline mocks).
Inlinevi.spyOn(...).mockResolvedValue/mockRejectedValueinside each test breaks the Vitest mocking rules; parameterize the cases and set the fetch behavior inbeforeEach.As per coding guidelines: Avoid inline mock implementations within test cases in Vitest tests; Implement mock behaviors in `beforeEach` blocks in Vitest tests; Group related mocks together in Vitest tests.♻️ Possible refactor
describe('checkRef', () => { afterEach(() => vi.restoreAllMocks()); - - it('returns true when fetch returns 200', async () => { - vi.spyOn(global, 'fetch').mockResolvedValue({ - ok: true, - status: 200, - json: async () => ({}), - } as Response); - expect(await checkRef('https://chromatic.com')).toBe(true); - }); - - it('returns false when fetch returns 401', async () => { - vi.spyOn(global, 'fetch').mockResolvedValue({ ok: false, status: 401 } as Response); - expect(await checkRef('https://chromatic.com')).toBe(false); - }); - - it('returns false when fetch returns 200 with loginUrl', async () => { - vi.spyOn(global, 'fetch').mockResolvedValue({ - ok: true, - status: 200, - json: async () => ({ loginUrl: 'https://chromatic.com/login' }), - } as Response); - expect(await checkRef('https://chromatic.com')).toBe(false); - }); - - it('returns false when fetch fails', async () => { - vi.spyOn(global, 'fetch').mockRejectedValue(new Error('Network error')); - expect(await checkRef('https://chromatic.com')).toBe(false); - }); + const cases = [ + { + name: 'returns true when fetch returns 200', + response: { ok: true, status: 200, json: async () => ({}) }, + expected: true, + }, + { name: 'returns false when fetch returns 401', response: { ok: false, status: 401 }, expected: false }, + { + name: 'returns false when fetch returns 200 with loginUrl', + response: { ok: true, status: 200, json: async () => ({ loginUrl: 'https://chromatic.com/login' }) }, + expected: false, + }, + { name: 'returns false when fetch fails', error: new Error('Network error'), expected: false }, + ] as const; + + describe.each(cases)('$name', ({ response, error, expected }) => { + beforeEach(() => { + const fetchMock = vi.spyOn(global, 'fetch'); + if (error) { + fetchMock.mockRejectedValue(error); + } else { + fetchMock.mockResolvedValue(response as Response); + } + }); + + it('matches expectation', async () => { + expect(await checkRef('https://chromatic.com')).toBe(expected); + }); + }); });code/addons/vitest/src/updateVitestFile.ts (1)
238-294: Well-structured logic for handling multi-project Vitest configs.The
hasProjectsProptype guard and the branching logic to append to existingtest.projectsarrays is a solid approach that avoids the double-nesting issue mentioned in the AI summary.One minor nit: on line 276,
templateProp.type === 'ObjectProperty'is redundant since it was already checked on line 266.🧹 Optional: Remove redundant type check
if (!existingProp && templateProp.type === 'ObjectProperty') { + if (!existingProp) { existingTestProp.value.properties.push(templateProp); }
| ## 10.3.0-alpha.2 | ||
|
|
||
| - Addon Vitest: Support simple vite.config without defineConfig helper - [#33694](https://github.com/storybookjs/storybook/pull/33694), thanks @valentinpalkovic! | ||
| - Addon-Docs: Add support for `sourceState: 'none'` to canvas block parameters - [#33627](https://github.com/storybookjs/storybook/pull/33627), thanks @quisido! | ||
| - Addon-Vitest: Append Storybook project to existing test.projects array without double nesting - [#33708](https://github.com/storybookjs/storybook/pull/33708), thanks @valentinpalkovic! | ||
| - Addon-Vitest: Normalize Windows paths in addon-vitest automigration - [#33340](https://github.com/storybookjs/storybook/pull/33340), thanks @tanujbhaud! | ||
| - Addon-Vitest: Skip postinstall setup when configured - [#33712](https://github.com/storybookjs/storybook/pull/33712), thanks @valentinpalkovic! | ||
| - Addon-Vitest: Update Vitest plugin configuration to disable requireAssertions for expect - [#33693](https://github.com/storybookjs/storybook/pull/33693), thanks @valentinpalkovic! | ||
| - CSF-Factories: Fix codemod for preview files without exports - [#33673](https://github.com/storybookjs/storybook/pull/33673), thanks @kasperpeulen! | ||
| - CSF: Fix false positive detection of Zod v4 .meta() as CSF Factory - [#33666](https://github.com/storybookjs/storybook/pull/33666), thanks @kasperpeulen! | ||
| - CSFFactories: Add non-interactive mode and --glob flag - [#33648](https://github.com/storybookjs/storybook/pull/33648), thanks @kasperpeulen! | ||
| - CSFFactories: Preserve leading comments when adding imports - [#33645](https://github.com/storybookjs/storybook/pull/33645), thanks @kasperpeulen! | ||
| - Cli: Use npm for registry URL in PNPMProxy to avoid workspace errors - [#33571](https://github.com/storybookjs/storybook/pull/33571), thanks @ia319! | ||
| - Codemod: Fix csf-2-to-3 failing due to quoted filenames - [#33646](https://github.com/storybookjs/storybook/pull/33646), thanks @kasperpeulen! | ||
| - Codemod: Fix glob pattern handling on Windows - [#33714](https://github.com/storybookjs/storybook/pull/33714), thanks @kasperpeulen! | ||
| - Composition: Handle 401 responses with loginUrl from Chromatic - [#33705](https://github.com/storybookjs/storybook/pull/33705), thanks @kasperpeulen! | ||
| - Core: Fix false-positive CJS warning when 'exports' appears in strings or comments - [#33572](https://github.com/storybookjs/storybook/pull/33572), thanks @reeseo3o! | ||
| - Telemetry: Add agent detection - [#33675](https://github.com/storybookjs/storybook/pull/33675), thanks @valentinpalkovic! | ||
|
|
There was a problem hiding this comment.
Add a release-note entry for the new share-provider + QR migration.
This PR introduces experimental_SHARE_PROVIDER and migrates QR sharing into a provider, but the alpha.2 notes don’t mention it. Please add a bullet so users can discover the new addon type and behavior change.
✍️ Suggested changelog entry
## 10.3.0-alpha.2
+- Core: Add experimental share-provider addon type and migrate QR share UI into a provider📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 10.3.0-alpha.2 | |
| - Addon Vitest: Support simple vite.config without defineConfig helper - [#33694](https://github.com/storybookjs/storybook/pull/33694), thanks @valentinpalkovic! | |
| - Addon-Docs: Add support for `sourceState: 'none'` to canvas block parameters - [#33627](https://github.com/storybookjs/storybook/pull/33627), thanks @quisido! | |
| - Addon-Vitest: Append Storybook project to existing test.projects array without double nesting - [#33708](https://github.com/storybookjs/storybook/pull/33708), thanks @valentinpalkovic! | |
| - Addon-Vitest: Normalize Windows paths in addon-vitest automigration - [#33340](https://github.com/storybookjs/storybook/pull/33340), thanks @tanujbhaud! | |
| - Addon-Vitest: Skip postinstall setup when configured - [#33712](https://github.com/storybookjs/storybook/pull/33712), thanks @valentinpalkovic! | |
| - Addon-Vitest: Update Vitest plugin configuration to disable requireAssertions for expect - [#33693](https://github.com/storybookjs/storybook/pull/33693), thanks @valentinpalkovic! | |
| - CSF-Factories: Fix codemod for preview files without exports - [#33673](https://github.com/storybookjs/storybook/pull/33673), thanks @kasperpeulen! | |
| - CSF: Fix false positive detection of Zod v4 .meta() as CSF Factory - [#33666](https://github.com/storybookjs/storybook/pull/33666), thanks @kasperpeulen! | |
| - CSFFactories: Add non-interactive mode and --glob flag - [#33648](https://github.com/storybookjs/storybook/pull/33648), thanks @kasperpeulen! | |
| - CSFFactories: Preserve leading comments when adding imports - [#33645](https://github.com/storybookjs/storybook/pull/33645), thanks @kasperpeulen! | |
| - Cli: Use npm for registry URL in PNPMProxy to avoid workspace errors - [#33571](https://github.com/storybookjs/storybook/pull/33571), thanks @ia319! | |
| - Codemod: Fix csf-2-to-3 failing due to quoted filenames - [#33646](https://github.com/storybookjs/storybook/pull/33646), thanks @kasperpeulen! | |
| - Codemod: Fix glob pattern handling on Windows - [#33714](https://github.com/storybookjs/storybook/pull/33714), thanks @kasperpeulen! | |
| - Composition: Handle 401 responses with loginUrl from Chromatic - [#33705](https://github.com/storybookjs/storybook/pull/33705), thanks @kasperpeulen! | |
| - Core: Fix false-positive CJS warning when 'exports' appears in strings or comments - [#33572](https://github.com/storybookjs/storybook/pull/33572), thanks @reeseo3o! | |
| - Telemetry: Add agent detection - [#33675](https://github.com/storybookjs/storybook/pull/33675), thanks @valentinpalkovic! | |
| ## 10.3.0-alpha.2 | |
| - Core: Add experimental share-provider addon type and migrate QR share UI into a provider | |
| - Addon Vitest: Support simple vite.config without defineConfig helper - [`#33694`](https://github.com/storybookjs/storybook/pull/33694), thanks `@valentinpalkovic`! | |
| - Addon-Docs: Add support for `sourceState: 'none'` to canvas block parameters - [`#33627`](https://github.com/storybookjs/storybook/pull/33627), thanks `@quisido`! | |
| - Addon-Vitest: Append Storybook project to existing test.projects array without double nesting - [`#33708`](https://github.com/storybookjs/storybook/pull/33708), thanks `@valentinpalkovic`! | |
| - Addon-Vitest: Normalize Windows paths in addon-vitest automigration - [`#33340`](https://github.com/storybookjs/storybook/pull/33340), thanks `@tanujbhaud`! | |
| - Addon-Vitest: Skip postinstall setup when configured - [`#33712`](https://github.com/storybookjs/storybook/pull/33712), thanks `@valentinpalkovic`! | |
| - Addon-Vitest: Update Vitest plugin configuration to disable requireAssertions for expect - [`#33693`](https://github.com/storybookjs/storybook/pull/33693), thanks `@valentinpalkovic`! | |
| - CSF-Factories: Fix codemod for preview files without exports - [`#33673`](https://github.com/storybookjs/storybook/pull/33673), thanks `@kasperpeulen`! | |
| - CSF: Fix false positive detection of Zod v4 .meta() as CSF Factory - [`#33666`](https://github.com/storybookjs/storybook/pull/33666), thanks `@kasperpeulen`! | |
| - CSFFactories: Add non-interactive mode and --glob flag - [`#33648`](https://github.com/storybookjs/storybook/pull/33648), thanks `@kasperpeulen`! | |
| - CSFFactories: Preserve leading comments when adding imports - [`#33645`](https://github.com/storybookjs/storybook/pull/33645), thanks `@kasperpeulen`! | |
| - Cli: Use npm for registry URL in PNPMProxy to avoid workspace errors - [`#33571`](https://github.com/storybookjs/storybook/pull/33571), thanks `@ia319`! | |
| - Codemod: Fix csf-2-to-3 failing due to quoted filenames - [`#33646`](https://github.com/storybookjs/storybook/pull/33646), thanks `@kasperpeulen`! | |
| - Codemod: Fix glob pattern handling on Windows - [`#33714`](https://github.com/storybookjs/storybook/pull/33714), thanks `@kasperpeulen`! | |
| - Composition: Handle 401 responses with loginUrl from Chromatic - [`#33705`](https://github.com/storybookjs/storybook/pull/33705), thanks `@kasperpeulen`! | |
| - Core: Fix false-positive CJS warning when 'exports' appears in strings or comments - [`#33572`](https://github.com/storybookjs/storybook/pull/33572), thanks `@reeseo3o`! | |
| - Telemetry: Add agent detection - [`#33675`](https://github.com/storybookjs/storybook/pull/33675), thanks `@valentinpalkovic`! |
🤖 Prompt for AI Agents
In `@CHANGELOG.prerelease.md` around lines 1 - 19, The changelog is missing a
release-note for the new share-provider and QR migration; add a concise bullet
under the 10.3.0-alpha.2 section that mentions the introduction of
experimental_SHARE_PROVIDER (or "share-provider") and that QR sharing was
migrated into a provider (include the change in behavior and how users can
opt-in via experimental_SHARE_PROVIDER), referencing the feature names exactly
(experimental_SHARE_PROVIDER, QR sharing, share-provider) so users can discover
the new addon type and behavioral change.
| const vitestSetupFile = resolve(options.configDir, `vitest.setup.${fileExtension}`); | ||
| const existingSetupFile = | ||
| EXTENSIONS.map((ext) => resolve(options.configDir, `vitest.setup${ext}`)).find(existsSync) || | ||
| null; | ||
|
|
||
| if (existsSync(vitestSetupFile)) { | ||
| const errorMessage = dedent` | ||
| Found an existing Vitest setup file: | ||
| ${vitestSetupFile} | ||
| Please refer to the documentation to complete the setup manually: | ||
| https://storybook.js.org/docs/next/${DOCUMENTATION_LINK}#manual-setup-advanced | ||
| `; | ||
| logger.line(); | ||
| logger.error(`${errorMessage}\n`); | ||
| errors.push(new AddonVitestPostinstallExistingSetupFileError({ filePath: vitestSetupFile })); | ||
| if (existingSetupFile) { | ||
| logger.step(`Found existing Vitest setup file, reusing:`); | ||
| logger.log(`${existingSetupFile}\n`); |
There was a problem hiding this comment.
Reuse an existing setup file when generating a brand‑new config.
If an existing setup file has a non-default extension, the “no existing config” branch still hard-codes vitestSetupFile and can point to a non-existent file.
🛠️ Suggested tweak
- const configTemplate = await loadTemplate(getTemplateName(), {
- CONFIG_DIR: options.configDir,
- SETUP_FILE: relative(dirname(newConfigFile), vitestSetupFile),
- });
+ const configTemplate = await loadTemplate(getTemplateName(), {
+ CONFIG_DIR: options.configDir,
+ SETUP_FILE: relative(dirname(newConfigFile), existingSetupFile ?? vitestSetupFile),
+ });🤖 Prompt for AI Agents
In `@code/addons/vitest/src/postinstall.ts` around lines 176 - 183, The code
computes vitestSetupFile using fileExtension even when an existingSetupFile with
a different extension is present, causing the generated config to point at a
non-existent file; change the logic so that after computing existingSetupFile
(from EXTENSIONS and options.configDir) you set vitestSetupFile =
existingSetupFile when one is found (otherwise fall back to
resolve(options.configDir, `vitest.setup.${fileExtension}`)), ensuring
vitestSetupFile always references the actual existing file and not a hard-coded
default.
| const workspaceFileContent = await fs.readFile(vitestWorkspaceFile, 'utf8'); | ||
| const alreadyConfigured = isConfigAlreadySetup(vitestWorkspaceFile, workspaceFileContent); | ||
|
|
||
| if (alreadyConfigured) { | ||
| logger.step( | ||
| CLI_COLORS.success('Vitest for Storybook is already properly configured. Skipping setup.') | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don’t return early when a workspace is already configured.
The early return skips later steps (a11y setup + final summary) and is inconsistent with the root-config branch that just logs and continues.
🐛 Suggested fix
- if (alreadyConfigured) {
- logger.step(
- CLI_COLORS.success('Vitest for Storybook is already properly configured. Skipping setup.')
- );
- return;
- }
-
- const workspaceTemplate = await loadTemplate('vitest.workspace.template.ts', {
- EXTENDS_WORKSPACE: viteConfigFile
- ? relative(dirname(vitestWorkspaceFile), viteConfigFile)
- : '',
- CONFIG_DIR: options.configDir,
- SETUP_FILE: relative(dirname(vitestWorkspaceFile), existingSetupFile ?? vitestSetupFile),
- }).then((t) => t.replace(`\n 'ROOT_CONFIG',`, '').replace(/\s+extends: '',/, ''));
- const source = babelParse(workspaceTemplate);
- const target = babelParse(workspaceFileContent);
-
- const updated = updateWorkspaceFile(source, target);
- if (updated) {
- logger.step(`Updating your Vitest workspace file...`);
- logger.log(`${vitestWorkspaceFile}`);
- const formattedContent = await formatFileContent(vitestWorkspaceFile, generate(target).code);
- await writeFile(vitestWorkspaceFile, formattedContent);
- } else {
- logger.error(/* ... */);
- errors.push(new AddonVitestPostinstallWorkspaceUpdateError({ filePath: vitestWorkspaceFile }));
- }
+ if (alreadyConfigured) {
+ logger.step(
+ CLI_COLORS.success('Vitest for Storybook is already properly configured. Skipping setup.')
+ );
+ } else {
+ const workspaceTemplate = await loadTemplate('vitest.workspace.template.ts', {
+ EXTENDS_WORKSPACE: viteConfigFile
+ ? relative(dirname(vitestWorkspaceFile), viteConfigFile)
+ : '',
+ CONFIG_DIR: options.configDir,
+ SETUP_FILE: relative(dirname(vitestWorkspaceFile), existingSetupFile ?? vitestSetupFile),
+ }).then((t) => t.replace(`\n 'ROOT_CONFIG',`, '').replace(/\s+extends: '',/, ''));
+ const source = babelParse(workspaceTemplate);
+ const target = babelParse(workspaceFileContent);
+
+ const updated = updateWorkspaceFile(source, target);
+ if (updated) {
+ logger.step(`Updating your Vitest workspace file...`);
+ logger.log(`${vitestWorkspaceFile}`);
+ const formattedContent = await formatFileContent(
+ vitestWorkspaceFile,
+ generate(target).code
+ );
+ await writeFile(vitestWorkspaceFile, formattedContent);
+ } else {
+ logger.error(/* ... */);
+ errors.push(
+ new AddonVitestPostinstallWorkspaceUpdateError({ filePath: vitestWorkspaceFile })
+ );
+ }
+ }Also applies to: 298-315
🤖 Prompt for AI Agents
In `@code/addons/vitest/src/postinstall.ts` around lines 242 - 250, The code
currently returns early when isConfigAlreadySetup(vitestWorkspaceFile,
workspaceFileContent) is true, which skips subsequent steps (a11y setup and
final summary); remove the early return and instead log the success message via
logger.step(CLI_COLORS.success(...)) but allow execution to continue so later
steps run; apply the same change for the other occurrence around lines 298-315
(the other isConfigAlreadySetup check) so both branches only log and do not
return early.
| // Normalize Windows paths (backslashes) to forward slashes for JavaScript string compatibility | ||
| Object.entries(replacements).forEach( | ||
| ([key, value]) => (template = template.replace(key, normalize(value))) | ||
| ); |
There was a problem hiding this comment.
Fix forEach callback to not return a value.
The parenthesized assignment (template = ...) causes the arrow function to implicitly return the assigned value. While functionally harmless, forEach ignores return values, making this misleading and flagged by static analysis.
🔧 Proposed fix using block body syntax
// Normalize Windows paths (backslashes) to forward slashes for JavaScript string compatibility
- Object.entries(replacements).forEach(
- ([key, value]) => (template = template.replace(key, normalize(value)))
- );
+ Object.entries(replacements).forEach(([key, value]) => {
+ template = template.replace(key, normalize(value));
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Normalize Windows paths (backslashes) to forward slashes for JavaScript string compatibility | |
| Object.entries(replacements).forEach( | |
| ([key, value]) => (template = template.replace(key, normalize(value))) | |
| ); | |
| // Normalize Windows paths (backslashes) to forward slashes for JavaScript string compatibility | |
| Object.entries(replacements).forEach(([key, value]) => { | |
| template = template.replace(key, normalize(value)); | |
| }); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 15-15: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@code/addons/vitest/src/updateVitestFile.ts` around lines 14 - 17, The forEach
callback currently uses a parenthesized assignment (template = ...) which makes
the arrow function implicitly return a value; change the callback to a statement
body so it does not return anything: update the
Object.entries(replacements).forEach(([key, value]) => ...) callback to use a
block body and perform template = template.replace(key, normalize(value));
inside it (or replace the forEach with a for...of loop) while keeping the use of
replacements, template, and normalize intact.
| export type KnownAgentName = | ||
| | 'claude-code' | ||
| | 'gemini-cli' | ||
| | 'cursor' | ||
| | 'codex' | ||
| | 'opencode' | ||
| | 'amp' | ||
| | 'unknown'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/telemetry/detect-agent.tsRepository: storybookjs/storybook
Length of output: 2441
🌐 Web query:
OpenCode CLI environment variable identification marker
💡 Result:
OpenCode CLI sets an environment variable OPENCODE=1 as an identification marker to indicate the current process (and its child processes) are running in an OpenCode context. [1] (deepwiki.com)
Citations:
🏁 Script executed:
rg -i 'opencode' code/core/src/telemetry/Repository: storybookjs/storybook
Length of output: 119
Add detection for OpenCode agent.
KnownAgentName includes 'opencode', but detectExplicitAgent has no corresponding detection branch. OpenCode CLI sets the OPENCODE environment variable to identify itself. Add explicit detection for it:
Suggested fix
// Cursor Agent (proposed / best-effort; Cursor often sets VSCode env vars too)
if (env.CURSOR_AGENT) {
return {
name: 'cursor',
};
}
+ // OpenCode
+ if (env.OPENCODE) {
+ return {
+ name: 'opencode',
+ };
+ }
// Generic "AGENT" marker (unknown implementation)
if (env.AGENT) {
return { name: 'unknown' };
}🤖 Prompt for AI Agents
In `@code/core/src/telemetry/detect-agent.ts` around lines 1 - 8,
detectExplicitAgent is missing a branch for the 'opencode' KnownAgentName; add
an explicit detection that checks process.env.OPENCODE (or equivalent env var)
and returns 'opencode' when present. Update detectExplicitAgent to inspect
OPENCODE alongside existing checks (e.g., for CLAUDE_CLI, GEMINI_CLI) and return
the string 'opencode' so it matches the KnownAgentName union.
| // 2) Behavioral / fingerprint heuristics (exclude CI to reduce false positives) | ||
| if (stdoutIsTTY) { | ||
| return undefined; |
There was a problem hiding this comment.
Comment contradicts behavior (CI not excluded).
The comment says CI is excluded, but the code does not check CI. Update the comment or add a CI guard.
💡 Suggested comment update
- // 2) Behavioral / fingerprint heuristics (exclude CI to reduce false positives)
+ // 2) Behavioral / fingerprint heuristics (only when stdout is not a TTY)🤖 Prompt for AI Agents
In `@code/core/src/telemetry/detect-agent.ts` around lines 77 - 79, The comment
above the stdoutIsTTY check in detect-agent.ts is misleading—either update the
comment to remove "exclude CI" or actually guard for CI; to fix, choose one: (A)
change the comment to reflect that the current check only returns undefined when
stdoutIsTTY is true, or (B) add a CI guard (e.g., detect common CI env vars like
CI, GITHUB_ACTIONS, GITLAB_CI) alongside the stdoutIsTTY condition so
detect-agent.ts returns undefined in CI too; reference the stdoutIsTTY check and
the surrounding comment to locate where to apply the change.
| const templateName = process.env.STORYBOOK_TEMPLATE_NAME || ''; | ||
| const type = process.env.STORYBOOK_TYPE || 'dev'; | ||
| const sandboxDir = process.env.STORYBOOK_SANDBOX_DIR; | ||
| const sandboxDir = process.env.STORYBOOK_SANDBOX_DIR!; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the addon-mcp.spec.ts file around the flagged lines
fd addon-mcp.spec.tsRepository: storybookjs/storybook
Length of output: 97
🏁 Script executed:
# Read the file around lines 11 and 205-206 to understand the context
cat -n code/e2e-tests/addon-mcp.spec.ts | sed -n '1,30p'Repository: storybookjs/storybook
Length of output: 1198
🏁 Script executed:
# Check lines 200-210 as well
cat -n code/e2e-tests/addon-mcp.spec.ts | sed -n '200,210p'Repository: storybookjs/storybook
Length of output: 432
🏁 Script executed:
# Search for where STORYBOOK_SANDBOX_DIR is defined, set, or documented
rg -n "STORYBOOK_SANDBOX_DIR" -S --type-list | head -20Repository: storybookjs/storybook
Length of output: 454
🏁 Script executed:
# Check CI workflows and test setup scripts
rg -n "STORYBOOK_SANDBOX_DIR" -SRepository: storybookjs/storybook
Length of output: 367
🏁 Script executed:
# Look for test setup or configuration files that might set this env var
fd -e yml -e yaml -e js -e ts | xargs rg -l "STORYBOOK_SANDBOX_DIR" 2>/dev/null | head -10Repository: storybookjs/storybook
Length of output: 169
🏁 Script executed:
# Check e2e-tests-build.ts to see how STORYBOOK_SANDBOX_DIR is set
cat -n scripts/tasks/e2e-tests-build.ts | sed -n '40,60p'Repository: storybookjs/storybook
Length of output: 715
🏁 Script executed:
# Also check the storybook-hooks.spec.ts to see how it handles the env var
cat -n code/e2e-tests/storybook-hooks.spec.ts | sed -n '15,25p'Repository: storybookjs/storybook
Length of output: 594
🏁 Script executed:
# Check if STORYBOOK_SANDBOX_DIR is set in CI/test configuration
fd -e yml -e yaml | xargs rg -l "STORYBOOK_SANDBOX_DIR" 2>/dev/nullRepository: storybookjs/storybook
Length of output: 49
Add explicit guard for STORYBOOK_SANDBOX_DIR for consistency and clarity.
The env var is set by the build script (e2e-tests-build.ts), but the other test file (storybook-hooks.spec.ts) uses a defensive pattern with fallback. Use an explicit guard with a clear error message instead of the non-null assertion to match the codebase pattern and improve code robustness:
Suggested change
-const sandboxDir = process.env.STORYBOOK_SANDBOX_DIR!;
+const sandboxDir = process.env.STORYBOOK_SANDBOX_DIR;
+if (!sandboxDir) {
+ throw new Error('STORYBOOK_SANDBOX_DIR must be set to run addon-mcp e2e tests');
+}Also applies to: 205-206
🤖 Prompt for AI Agents
In `@code/e2e-tests/addon-mcp.spec.ts` at line 11, Replace the non-null assertion
on process.env.STORYBOOK_SANDBOX_DIR (assigned to sandboxDir) with an explicit
guard that throws a clear Error when the env var is missing; locate the
assignment to sandboxDir in addon-mcp.spec.ts and any other usages (the similar
occurrences around the second block) and change them to check if
process.env.STORYBOOK_SANDBOX_DIR is truthy, set sandboxDir from it, otherwise
throw an Error with a descriptive message like "STORYBOOK_SANDBOX_DIR must be
set (set by e2e-tests-build.ts)". This mirrors the defensive pattern used in
storybook-hooks.spec.ts and ensures consistent, informative failures.
| <script module> | ||
| import { defineMeta } from '@storybook/addon-svelte-csf'; | ||
|
|
||
| import Layout from './Layout.svelte'; | ||
| import MyComponent from './MyComponent.svelte'; | ||
|
|
||
| const { Story } = defineMeta({ | ||
| component: MyComponent, | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and check its content around lines 497-506
fd "component-story-with-custom-render-function.md" --type fRepository: storybookjs/storybook
Length of output: 126
🏁 Script executed:
# Get the file and check line count and content around the specified lines
wc -l docs/_snippets/component-story-with-custom-render-function.mdRepository: storybookjs/storybook
Length of output: 130
🏁 Script executed:
# Read the relevant section to verify the issue
sed -n '490,515p' docs/_snippets/component-story-with-custom-render-function.mdRepository: storybookjs/storybook
Length of output: 572
🌐 Web query:
Svelte TypeScript module script syntax lang="ts"
💡 Result:
In Svelte, TypeScript is enabled per <script> tag by adding lang="ts". (svelte.dev)
Svelte 5 (current syntax)
Use a module script with the module attribute, and add lang="ts" the same way:
<script module lang="ts">
// runs once per module (not per component instance)
export const answer: number = 42;
</script>
<script lang="ts">
// runs per component instance
</script>Svelte’s docs define the module script as <script module>. (svelte.dev)
Svelte 4 (legacy syntax)
The module script was written with context="module":
<script context="module" lang="ts">
// module-level code
</script>Svelte 5 docs explicitly call out this Svelte 4 legacy form. (svelte.dev)
Citations:
- 1: https://svelte.dev/docs/typescript?utm_source=openai
- 2: https://svelte.dev/docs/svelte/svelte-files
- 3: https://svelte.dev/docs/svelte/svelte-files
Add lang="ts" to the module script for this TypeScript example.
The code fence declares language="ts" but the <script module> tag lacks lang="ts", preventing TypeScript from being recognized when this snippet is used. According to Svelte documentation, the correct syntax is <script module lang="ts">.
Suggested fix
-<script module>
+<script module lang="ts">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script module> | |
| import { defineMeta } from '@storybook/addon-svelte-csf'; | |
| import Layout from './Layout.svelte'; | |
| import MyComponent from './MyComponent.svelte'; | |
| const { Story } = defineMeta({ | |
| component: MyComponent, | |
| }); | |
| </script> | |
| <script module lang="ts"> | |
| import { defineMeta } from '@storybook/addon-svelte-csf'; | |
| import Layout from './Layout.svelte'; | |
| import MyComponent from './MyComponent.svelte'; | |
| const { Story } = defineMeta({ | |
| component: MyComponent, | |
| }); | |
| </script> |
🤖 Prompt for AI Agents
In `@docs/_snippets/component-story-with-custom-render-function.md` around lines
497 - 506, The module script tag is missing TypeScript mode; update the <script
module> tag to include lang="ts" so the example is treated as TypeScript
(affecting the snippet that imports defineMeta, Layout, and MyComponent and
defines Story via defineMeta).
| const greeting = computed(() => globals?.locale === 'en' ? 'Hello!' : '¡Hola!'); | ||
|
|
||
| return { greeting, globals }; | ||
| }, | ||
| template: ` | ||
| <div :lang={{globals?.locale || 'en'}}> | ||
| <p>Greeting: {{greeting}}</p> | ||
| <story /> | ||
| </div> |
There was a problem hiding this comment.
Fix Vue attribute binding syntax in the snippet.
:lang={{...}} is invalid in Vue templates; it should be :lang="...".
💡 Suggested fix
- <div :lang={{globals?.locale || 'en'}}>
+ <div :lang="globals?.locale || 'en'">- <div :lang={{globals?.locale || 'en'}}>
+ <div :lang="globals?.locale || 'en'">Also applies to: 40-44
🤖 Prompt for AI Agents
In `@docs/_snippets/decorator-with-reactive-globals.md` around lines 10 - 18, The
Vue template attribute binding uses invalid moustache braces; update the
template in the snippet (the template string that returns greeting and globals)
to use proper Vue binding syntax by replacing :lang={{globals?.locale || 'en'}}
with :lang="globals?.locale || 'en'" (and make the same change for the other
occurrence referenced at lines 40-44); keep the computed greeting and globals
symbols as-is.
| ```ts filename=".storybook/preview.ts" renderer="vue" language="ts" | ||
| import { useArgs } from 'storybook/preview-api'; | ||
| import type { Meta, StoryObj } from '@storybook/vue3'; | ||
|
|
||
| const WithIncrementDecorator: StoryObj<Meta<typeof MyComponent>> = { |
There was a problem hiding this comment.
MyComponent is referenced but not imported.
The TypeScript snippet references MyComponent in the type annotation StoryObj<Meta<typeof MyComponent>>, but this component is neither imported nor defined in the snippet. This will cause a TypeScript error when users copy this example.
Consider either:
- Adding an import for
MyComponent, or - Using a generic placeholder type that doesn't require an import (e.g.,
StoryObj<{ args: { counter: number } }>)
📝 Suggested fix (option 2)
```ts filename=".storybook/preview.ts" renderer="vue" language="ts"
import { useArgs } from 'storybook/preview-api';
-import type { Meta, StoryObj } from '@storybook/vue3';
+import type { StoryObj } from '@storybook/vue3';
-const WithIncrementDecorator: StoryObj<Meta<typeof MyComponent>> = {
+const WithIncrementDecorator: StoryObj<{ args: { counter: number } }> = {
args: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```ts filename=".storybook/preview.ts" renderer="vue" language="ts" | |
| import { useArgs } from 'storybook/preview-api'; | |
| import type { Meta, StoryObj } from '@storybook/vue3'; | |
| const WithIncrementDecorator: StoryObj<Meta<typeof MyComponent>> = { |
🤖 Prompt for AI Agents
In `@docs/_snippets/decorator-with-updateArgs.md` around lines 30 - 34, The
snippet references MyComponent in the type StoryObj<Meta<typeof MyComponent>>
but MyComponent is not imported or defined; update the type to avoid the missing
symbol by replacing the Meta<typeof MyComponent> usage with a generic shape
(e.g., StoryObj<{ args: { counter: number } }>) or add an import for
MyComponent; specifically edit the declaration of WithIncrementDecorator (and
the import list for types) so StoryObj no longer depends on the undefined
MyComponent symbol.
ndelangen
force-pushed
the
share-provider
branch
from
90dbf96 to
5b82dd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@code/core/src/qrcode/manager.tsx`:
- Around line 53-57: The copyLink handler sets a 2000ms timeout but never clears
it on unmount, risking setCopied(false) on an unmounted component; modify the
component that declares copyLink to store the timeout ID (from setTimeout) in a
ref or state and add a useEffect cleanup that calls clearTimeout on that ID (or
clear any active timeout) when the component unmounts so setCopied is not called
after unmount; ensure you reference the existing copyLink function and the
setCopied state updater when implementing the cleanup.
In `@docs/plans/eliminate-dts-mappers.md`:
- Around line 123-143: Two sections are both titled "Option 4" which confuses
readers; update the second heading ("### Option 4: Hybrid Approach -
`declarationMap` + `paths`") to "### Option 5: Hybrid Approach -
`declarationMap` + `paths`" (and adjust any internal references if present) so
headings are uniquely numbered; ensure the surrounding text remains unchanged
and any cross-references to the hybrid approach use the new Option 5 label.
- Around line 21-28: Update the "Option 0: Use `customConditions`" section to
state that `customConditions` requires TypeScript 5.0+ and only works with
moduleResolution set to `node16`, `nodenext`, or `bundler`; and correct the
resolution description to explain that TypeScript adds the custom condition to
its active condition set but resolution follows the key order in the
package.json `exports` object (the first matching key wins), so you must place
`"source"` before `"types"` in `exports` if you want it selected; apply this
clarification to the "Option 0" header and the other mentions of
`customConditions` and resolution behavior.
🧹 Nitpick comments (2)
code/core/src/manager/components/preview/tools/share.tsx (1)
19-24: Redundant nullish coalescing operator.Line 22 uses
order ?? 0butorderalready has a default value of0from destructuring on line 21, making the nullish coalescing unnecessary.♻️ Simplified code
const items = Object.values(api.getElements(Addon_TypesEnum.experimental_SHARE_PROVIDER)) .map((registeredShareProvider) => { const { id, title, render, order = 0 } = registeredShareProvider; - return { id, title, children: <>{render()}</>, order: order ?? 0 }; + return { id, title, children: <>{render()}</>, order }; }) .sort((a, b) => a.order - b.order);docs/plans/eliminate-dts-mappers.md (1)
72-72: Reduce repeated “exactly” phrasing.The repeated “exactly what you suggested” reads promotional and distracts from the plan. Consider rephrasing or trimming it to keep the doc neutral and concise.
Also applies to: 157-157, 203-203, 352-352, 362-362
| const copyLink = () => { | ||
| copy(networkHrefs.managerHref); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
There was a problem hiding this comment.
Memory leak: setTimeout not cleared on unmount.
If the component unmounts before the 2000ms timeout completes, setCopied(false) will attempt to update state on an unmounted component. Use useEffect cleanup or store the timeout ID to clear it.
🛠️ Proposed fix using useEffect for cleanup
-import React, { useState } from 'react';
+import React, { useEffect, useState } from 'react';+ useEffect(() => {
+ if (!copied) return;
+ const timer = setTimeout(() => setCopied(false), 2000);
+ return () => clearTimeout(timer);
+ }, [copied]);
+
const copyLink = () => {
copy(networkHrefs.managerHref);
setCopied(true);
- setTimeout(() => setCopied(false), 2000);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const copyLink = () => { | |
| copy(networkHrefs.managerHref); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }; | |
| import React, { useEffect, useState } from 'react'; | |
| const [copied, setCopied] = useState(false); | |
| useEffect(() => { | |
| if (!copied) return; | |
| const timer = setTimeout(() => setCopied(false), 2000); | |
| return () => clearTimeout(timer); | |
| }, [copied]); | |
| const copyLink = () => { | |
| copy(networkHrefs.managerHref); | |
| setCopied(true); | |
| }; |
🤖 Prompt for AI Agents
In `@code/core/src/qrcode/manager.tsx` around lines 53 - 57, The copyLink handler
sets a 2000ms timeout but never clears it on unmount, risking setCopied(false)
on an unmounted component; modify the component that declares copyLink to store
the timeout ID (from setTimeout) in a ref or state and add a useEffect cleanup
that calls clearTimeout on that ID (or clear any active timeout) when the
component unmounts so setCopied is not called after unmount; ensure you
reference the existing copyLink function and the setCopied state updater when
implementing the cleanup.
| ### Option 0: Use `customConditions` in tsconfig.json (Perfect Solution!) ⭐⭐⭐ | ||
|
|
||
| **How it works:** | ||
| - Add a custom condition (e.g., `"source"` or `"development"`) to your `package.json` exports pointing to source files | ||
| - Configure TypeScript's `customConditions` compiler option to recognize this condition | ||
| - TypeScript will prefer the custom condition over `"types"` when resolving imports | ||
| - When publishing, only `dist/` is published, so consumers fall back to `"types"` or `"default"` conditions | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript customConditions compiler option minimum version support
💡 Result:
The compilerOptions.customConditions option is supported starting in TypeScript 5.0. It was introduced as part of the TypeScript 5.0 release (--customConditions) for package.json exports/imports conditional resolution. [1][2]
Notes:
- It’s only valid/used when
moduleResolutionisnode16,nodenext, orbundler. [1][2]
Sources:
[1] TypeScript TSConfig option docs: customConditions https://www.typescriptlang.org/tsconfig/customConditions.html
[2] TypeScript 5.0 release notes / announcement (customConditions section) https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html / https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/
🏁 Script executed:
# First, let's examine the markdown file to see the current content
cd /tmp && find . -name "eliminate-dts-mappers.md" 2>/dev/null || echo "File might not exist in sandbox"Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# List the structure to find the file
find . -type f -name "*.md" -path "*/docs/*" 2>/dev/null | head -20Repository: storybookjs/storybook
Length of output: 950
🌐 Web query:
TypeScript package.json exports customConditions resolution precedence order
💡 Result:
Precedence is driven by the key order in the exports (or imports) object, not by TypeScript. During conditional matching, the resolver walks the conditions in the order they appear in package.json and selects the first condition whose name is “active” for that resolution; "default" is the fallback and should be last. [1]
What conditions are “active” in TypeScript?
- TypeScript always adds its own condition set (depending on
moduleResolutionand whether it’s resolving animport-like orrequire-like request), and then adds yourcompilerOptions.customConditionsto that set. [2] - In
node16/nodenext, when resolving via:- ESM
import: matchestypes,node,import - CJS
require: matchestypes,node,require[3]
- ESM
- In
bundler, it matchestypesplusimport/requiredepending on syntax (and doesn’t use thenodecondition the same way). [3]
So the effective rule is:
- Build the active condition set = TypeScript defaults (
types, plusnode(node16/nodenext), plusimport/require) +customConditions. [2] [3] - Evaluate the
exports["."](or subpath) object top-to-bottom in JSON key order. The first key that’s in the active set wins;"default"only wins if nothing earlier matched. [1]
Example: if customConditions: ["my-condition"] is set, and your exports lists "my-condition" before "import"/"require"/"node", TypeScript will pick "my-condition" (because it’s active and earlier). [2] [1]
Sources: Node conditional exports ordering rules [1]; TypeScript customConditions behavior [2]; TypeScript default export conditions by resolver mode/context [3].
[1] https://nodejs.org/api/packages.html
[2] https://www.typescriptlang.org/tsconfig/customConditions.html
[3] https://www.typescriptlang.org/docs/handbook/modules/reference.html
🏁 Script executed:
# Check if we can access the repository structure
git ls-files | grep -E "(docs|README)" | head -20Repository: storybookjs/storybook
Length of output: 820
🏁 Script executed:
# Find the markdown file in the repository
find . -name "eliminate-dts-mappers.md" -type fRepository: storybookjs/storybook
Length of output: 102
🏁 Script executed:
# Read the specific file and lines to see current documentation
cat -n docs/plans/eliminate-dts-mappers.md | head -80Repository: storybookjs/storybook
Length of output: 4244
🏁 Script executed:
# Read more lines to see the other affected sections (52-57, 199-203, 362-368)
cat -n docs/plans/eliminate-dts-mappers.md | sed -n '50,60p'Repository: storybookjs/storybook
Length of output: 777
🏁 Script executed:
# Check lines 199-203
cat -n docs/plans/eliminate-dts-mappers.md | sed -n '195,210p'Repository: storybookjs/storybook
Length of output: 904
🏁 Script executed:
# Check lines 362-368
cat -n docs/plans/eliminate-dts-mappers.md | sed -n '360,370p'Repository: storybookjs/storybook
Length of output: 898
🏁 Script executed:
# Get full file length to understand scope
wc -l docs/plans/eliminate-dts-mappers.mdRepository: storybookjs/storybook
Length of output: 104
Document TypeScript 5.0 minimum requirement and clarify customConditions resolution behavior.
The plan lacks critical constraints and contains an inaccuracy about resolution precedence:
-
Add minimum TypeScript version:
customConditionsrequires TypeScript 5.0+ and only works withmoduleResolution: node16,nodenext, orbundler. -
Correct the resolution description: The document states "TypeScript will prefer the custom condition over
types" (lines 26, 200, 365), but this is inaccurate. Resolution is determined by key order inexports, not automatic preference. TypeScript addscustomConditionsto its active condition set and uses the first matching key in theexportsobject. If"source"appears before"types", it wins; the order must be explicit in the package.json.
Update lines 21–28, 51–57, 199–203, and 362–368 to clarify these constraints so implementers understand the requirements and expected behavior.
🤖 Prompt for AI Agents
In `@docs/plans/eliminate-dts-mappers.md` around lines 21 - 28, Update the "Option
0: Use `customConditions`" section to state that `customConditions` requires
TypeScript 5.0+ and only works with moduleResolution set to `node16`,
`nodenext`, or `bundler`; and correct the resolution description to explain that
TypeScript adds the custom condition to its active condition set but resolution
follows the key order in the package.json `exports` object (the first matching
key wins), so you must place `"source"` before `"types"` in `exports` if you
want it selected; apply this clarification to the "Option 0" header and the
other mentions of `customConditions` and resolution behavior.
| ### Option 4: Use `rootDirs` for Virtual Directory Merging | ||
| **How it works:** | ||
| - Configure `rootDirs` to treat `src/` and `dist/` as virtual siblings | ||
| - TypeScript resolves imports as if they're in the same location | ||
| - Allows relative imports to work across virtual directories | ||
|
|
||
| **Pros:** | ||
| - Native TypeScript feature | ||
| - Handles complex directory structures | ||
| - Good for monorepos | ||
|
|
||
| **Cons:** | ||
| - May be complex to configure correctly | ||
| - Less commonly used, so less documentation/examples | ||
|
|
||
| ### Option 4: Hybrid Approach - `declarationMap` + `paths` | ||
| **How it works:** | ||
| - Enable `declarationMap` for source mapping | ||
| - Use `paths` for module resolution during compilation | ||
| - Best of both worlds | ||
|
|
There was a problem hiding this comment.
Fix duplicate “Option 4” numbering.
There are two “Option 4” sections (rootDirs and hybrid), which is confusing for readers and cross-references. Please renumber one of them (e.g., make the hybrid approach Option 5).
🤖 Prompt for AI Agents
In `@docs/plans/eliminate-dts-mappers.md` around lines 123 - 143, Two sections are
both titled "Option 4" which confuses readers; update the second heading ("###
Option 4: Hybrid Approach - `declarationMap` + `paths`") to "### Option 5:
Hybrid Approach - `declarationMap` + `paths`" (and adjust any internal
references if present) so headings are uniquely numbered; ensure the surrounding
text remains unchanged and any cross-references to the hybrid approach use the
new Option 5 label.
There was a problem hiding this comment.
Actionable comments posted: 2
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.stories.tsx (1)
12-27:⚠️ Potential issue | 🟡 MinorAdd
getElementsmock to prevent TypeError in shareTool render.The shareTool.render calls
api.getElements()to retrieve registered share providers. Without this method on the mock, the story will crash withCannot convert undefined or null to object. Add a provider mock so the component can render and the play step can proceed.🧪 Suggested mock update
api: { emit: fn().mockName('api::emit'), + getElements: () => ({ + 'storybook/qrcode': { + id: 'storybook/qrcode', + title: 'QR Code', + render: () => <div>Scan to open</div>, + order: 0, + }, + }), getShortcutKeys: () => ({ copyStoryLink: ['alt', 'shift', 'l'], openInIsolation: ['alt', 'shift', 'i'], }),
🤖 Fix all issues with AI agents
In `@code/core/src/core-server/server-channel/telemetry-channel.ts`:
- Line 44: Replace the empty catch block after the try that retrieves the
session ID / fetches events with error logging: import logger from
'storybook/internal/node-logger', change catch {} to catch (err) {
logger.error("telemetry-channel: failed to retrieve session ID or fetch events",
err); } so the failure is recorded with context and the error object is included
for debugging.
In `@code/core/src/manager/components/preview/tools/share.tsx`:
- Around line 34-61: Move the useEffect call in the Share component so it runs
before the early return (i.e., place the useEffect immediately after the
useState declaration) to preserve Hook ordering, and update its dependency array
to include api (useEffect(() => { if (isOpen) api.emit(SHARE_POPOVER_OPENED); },
[isOpen, api])). Ensure you keep the existing isOpen state and the
api.emit(SHARE_POPOVER_OPENED) call intact.
🧹 Nitpick comments (1)
code/core/src/core-server/server-channel/telemetry-channel.ts (1)
46-54: Unnecessaryasyncand missing error handling for share telemetry handlers.These handlers are marked
asyncbut don't useawait, making the keyword unnecessary. More importantly, unlike thePREVIEW_INITIALIZEDhandler, these lack error handling. Iftelemetry()rejects, it will result in unhandled promise rejections.Consider adding minimal error handling for consistency with the existing pattern.
♻️ Proposed refactor for consistency
- channel.on(SHARE_POPOVER_OPENED, async () => { - telemetry('share', { action: 'popover-opened' }); - }); - channel.on(SHARE_STORY_LINK, async () => { - telemetry('share', { action: 'story-link-copied' }); - }); - channel.on(SHARE_ISOLATE_MODE, async () => { - telemetry('share', { action: 'isolate-mode-opened' }); - }); + channel.on(SHARE_POPOVER_OPENED, () => { + telemetry('share', { action: 'popover-opened' }).catch(() => {}); + }); + channel.on(SHARE_STORY_LINK, () => { + telemetry('share', { action: 'story-link-copied' }).catch(() => {}); + }); + channel.on(SHARE_ISOLATE_MODE, () => { + telemetry('share', { action: 'isolate-mode-opened' }).catch(() => {}); + });Or, to match the
PREVIEW_INITIALIZEDpattern with logging:- channel.on(SHARE_POPOVER_OPENED, async () => { - telemetry('share', { action: 'popover-opened' }); - }); + channel.on(SHARE_POPOVER_OPENED, async () => { + try { + await telemetry('share', { action: 'popover-opened' }); + } catch {} + });
| } | ||
| } | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
Empty catch block silently swallows errors.
The empty catch {} hides potential failures in session ID retrieval or event fetching, making debugging difficult. As per coding guidelines, server-side code should use logger from storybook/internal/node-logger for logging.
🔧 Proposed fix to log errors
+import { logger } from 'storybook/internal/node-logger';
+
// ... in the handler
- } catch {}
+ } catch (error) {
+ logger.warn('Telemetry preview-first-load failed:', error);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch {} | |
| } catch (error) { | |
| logger.warn('Telemetry preview-first-load failed:', error); | |
| } |
🤖 Prompt for AI Agents
In `@code/core/src/core-server/server-channel/telemetry-channel.ts` at line 44,
Replace the empty catch block after the try that retrieves the session ID /
fetches events with error logging: import logger from
'storybook/internal/node-logger', change catch {} to catch (err) {
logger.error("telemetry-channel: failed to retrieve session ID or fetch events",
err); } so the failure is recorded with context and the error object is included
for debugging.
| function Share({ items, api }: { items: ReturnType<typeof shareMapper>['items']; api: API }) { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| const links = useMemo(() => { | ||
| const copyTitle = copied ? 'Copied!' : 'Copy story link'; | ||
| const originHrefs = api.getStoryHrefs(storyId, { base: 'origin', refId }); | ||
| const networkHrefs = api.getStoryHrefs(storyId, { base: 'network', refId }); | ||
| if (items.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| return [ | ||
| [ | ||
| { | ||
| id: 'copy-link', | ||
| title: copyTitle, | ||
| icon: <LinkIcon />, | ||
| right: enableShortcuts ? <Shortcut keys={copyStoryLink} /> : null, | ||
| onClick: () => { | ||
| copy(originHrefs.managerHref); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }, | ||
| }, | ||
| { | ||
| id: 'open-new-tab', | ||
| title: 'Open in isolation mode', | ||
| icon: <ShareAltIcon />, | ||
| right: enableShortcuts ? <Shortcut keys={openInIsolation} /> : null, | ||
| href: originHrefs.previewHref, | ||
| target: '_blank', | ||
| rel: 'noopener noreferrer', | ||
| }, | ||
| ], | ||
| [ | ||
| { | ||
| id: 'qr-section', | ||
| content: ( | ||
| <QRContainer> | ||
| <QRImage value={networkHrefs.managerHref} /> | ||
| <QRContent> | ||
| <QRTitle>Scan to open</QRTitle> | ||
| <QRDescription> | ||
| {global.CONFIG_TYPE === 'DEVELOPMENT' | ||
| ? 'Device must be on the same network.' | ||
| : 'View story on another device.'} | ||
| </QRDescription> | ||
| </QRContent> | ||
| </QRContainer> | ||
| ), | ||
| }, | ||
| ], | ||
| ]; | ||
| }, [api, storyId, refId, copied, enableShortcuts, copyStoryLink, openInIsolation]); | ||
| useEffect(() => { | ||
| if (isOpen) { | ||
| api.emit(SHARE_POPOVER_OPENED); | ||
| } | ||
| }, [isOpen]); | ||
|
|
||
| return <TooltipLinkList links={links} style={{ width: 240 }} />; | ||
| }); | ||
| return ( | ||
| <> | ||
| <Button padding="small" variant="solid" ariaLabel={false} onClick={() => setIsOpen(true)}> | ||
| Share | ||
| </Button> | ||
| <Modal open={isOpen} onOpenChange={(isOpenNow) => setIsOpen(isOpenNow)}> | ||
| {items.length > 1 ? ( | ||
| <TabsView tabs={items} backgroundColor="transparent" /> | ||
| ) : ( | ||
| items[0].children | ||
| )} | ||
| </Modal> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React Rules of Hooks conditional return early useEffect order violation
💡 Result:
In React, an “early return” that happens before a Hook call makes later Hooks conditional, which violates the Rules of Hooks because React must see the same Hooks in the same order on every render. This is why ESLint reports things like: “Did you accidentally call a React Hook after an early return?” and why you can get runtime errors such as “Rendered fewer hooks than expected.” [1][2]
What’s going wrong
function MyComp({ loading }) {
if (loading) return <Spinner />; // <-- early return on some renders
useEffect(() => { ... }, []); // <-- runs only when loading is false
return <div />;
}On renders where loading === true, useEffect is not called, so the Hook order/count differs across renders → violation. [1][2]
Correct patterns
A) Call Hooks first, then conditionally return
function MyComp({ loading }) {
useEffect(() => {
if (loading) return; // gate the effect *inside* the effect
// do effect work
}, [loading]);
if (loading) return <Spinner />;
return <div />;
}Hooks are always called; only the effect’s contents are conditional. [1][2]
B) Split into two components
function MyComp(props) {
if (props.loading) return <Spinner />;
return <MyCompLoaded {...props} />;
}
function MyCompLoaded(props) {
useEffect(() => { /* ... */ }, []);
return <div />;
}Each component has a consistent Hook order. [1]
Rule of thumb: All Hooks must run before any conditional return in that component function. [1][2]
Sources: React “Rules of Hooks” and the ESLint rules-of-hooks lint documentation. [1][2]
🏁 Script executed:
# Check the actual file to see the complete context and confirm the issue
cat -n code/core/src/manager/components/preview/tools/share.tsx | head -70Repository: storybookjs/storybook
Length of output: 2557
Move useEffect before the early return and add api to the dependency array.
The useEffect is placed after an early return (line 37), violating the Rules of Hooks. React requires all Hooks to be called before any conditional return to maintain consistent Hook ordering across renders. Additionally, api is used in the effect but missing from the dependency array, which can cause stale reference bugs.
🔧 Proposed fix
function Share({ items, api }: { items: ReturnType<typeof shareMapper>['items']; api: API }) {
const [isOpen, setIsOpen] = useState(false);
- if (items.length === 0) {
- return null;
- }
-
useEffect(() => {
if (isOpen) {
api.emit(SHARE_POPOVER_OPENED);
}
- }, [isOpen]);
+ }, [api, isOpen]);
+
+ if (items.length === 0) {
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Share({ items, api }: { items: ReturnType<typeof shareMapper>['items']; api: API }) { | |
| const [isOpen, setIsOpen] = useState(false); | |
| const links = useMemo(() => { | |
| const copyTitle = copied ? 'Copied!' : 'Copy story link'; | |
| const originHrefs = api.getStoryHrefs(storyId, { base: 'origin', refId }); | |
| const networkHrefs = api.getStoryHrefs(storyId, { base: 'network', refId }); | |
| if (items.length === 0) { | |
| return null; | |
| } | |
| return [ | |
| [ | |
| { | |
| id: 'copy-link', | |
| title: copyTitle, | |
| icon: <LinkIcon />, | |
| right: enableShortcuts ? <Shortcut keys={copyStoryLink} /> : null, | |
| onClick: () => { | |
| copy(originHrefs.managerHref); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }, | |
| }, | |
| { | |
| id: 'open-new-tab', | |
| title: 'Open in isolation mode', | |
| icon: <ShareAltIcon />, | |
| right: enableShortcuts ? <Shortcut keys={openInIsolation} /> : null, | |
| href: originHrefs.previewHref, | |
| target: '_blank', | |
| rel: 'noopener noreferrer', | |
| }, | |
| ], | |
| [ | |
| { | |
| id: 'qr-section', | |
| content: ( | |
| <QRContainer> | |
| <QRImage value={networkHrefs.managerHref} /> | |
| <QRContent> | |
| <QRTitle>Scan to open</QRTitle> | |
| <QRDescription> | |
| {global.CONFIG_TYPE === 'DEVELOPMENT' | |
| ? 'Device must be on the same network.' | |
| : 'View story on another device.'} | |
| </QRDescription> | |
| </QRContent> | |
| </QRContainer> | |
| ), | |
| }, | |
| ], | |
| ]; | |
| }, [api, storyId, refId, copied, enableShortcuts, copyStoryLink, openInIsolation]); | |
| useEffect(() => { | |
| if (isOpen) { | |
| api.emit(SHARE_POPOVER_OPENED); | |
| } | |
| }, [isOpen]); | |
| return <TooltipLinkList links={links} style={{ width: 240 }} />; | |
| }); | |
| return ( | |
| <> | |
| <Button padding="small" variant="solid" ariaLabel={false} onClick={() => setIsOpen(true)}> | |
| Share | |
| </Button> | |
| <Modal open={isOpen} onOpenChange={(isOpenNow) => setIsOpen(isOpenNow)}> | |
| {items.length > 1 ? ( | |
| <TabsView tabs={items} backgroundColor="transparent" /> | |
| ) : ( | |
| items[0].children | |
| )} | |
| </Modal> | |
| </> | |
| ); | |
| } | |
| function Share({ items, api }: { items: ReturnType<typeof shareMapper>['items']; api: API }) { | |
| const [isOpen, setIsOpen] = useState(false); | |
| useEffect(() => { | |
| if (isOpen) { | |
| api.emit(SHARE_POPOVER_OPENED); | |
| } | |
| }, [api, isOpen]); | |
| if (items.length === 0) { | |
| return null; | |
| } | |
| return ( | |
| <> | |
| <Button padding="small" variant="solid" ariaLabel={false} onClick={() => setIsOpen(true)}> | |
| Share | |
| </Button> | |
| <Modal open={isOpen} onOpenChange={(isOpenNow) => setIsOpen(isOpenNow)}> | |
| {items.length > 1 ? ( | |
| <TabsView tabs={items} backgroundColor="transparent" /> | |
| ) : ( | |
| items[0].children | |
| )} | |
| </Modal> | |
| </> | |
| ); | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 41-41: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In `@code/core/src/manager/components/preview/tools/share.tsx` around lines 34 -
61, Move the useEffect call in the Share component so it runs before the early
return (i.e., place the useEffect immediately after the useState declaration) to
preserve Hook ordering, and update its dependency array to include api
(useEffect(() => { if (isOpen) api.emit(SHARE_POPOVER_OPENED); }, [isOpen,
api])). Ensure you keep the existing isOpen state and the
api.emit(SHARE_POPOVER_OPENED) call intact.
…ng displayed by share
…pleProviders story with placeholder content
15 participants
What I did
experimental_SHARE_PROVIDERaddon type.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
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 pull request has been released as version
0.0.0-pr-33653-sha-6047da63. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33653-sha-6047da63 sandboxor in an existing project withnpx storybook@0.0.0-pr-33653-sha-6047da63 upgrade.More information
0.0.0-pr-33653-sha-6047da63share-provider6047da631770221520)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=33653Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation