Actions: Warn instead of throw for implicit actions in docs renders#34505
Open
EricMChavez wants to merge 1 commit intostorybookjs:nextfrom
Open
Actions: Warn instead of throw for implicit actions in docs renders#34505EricMChavez wants to merge 1 commit intostorybookjs:nextfrom
EricMChavez wants to merge 1 commit intostorybookjs:nextfrom
Conversation
Stories embedded in a docs page render with viewMode === 'docs' and have no play function semantics, so an implicit action firing during a lifecycle hook (e.g. Vue 3 onMounted emitting an event) should warn rather than crash the entire docs page. Fixes storybookjs#33758 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughChanges update implicit-action error handling in Storybook's action handler. When stories render in docs mode ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/actions/runtime/action.ts (1)
67-80:⚠️ Potential issue | 🟠 MajorUse Storybook client logger instead of
console.warnin runtime code.Line 79 uses raw
console.warnin a client-side runtime code path. Replace it withstorybook/internal/client-loggerfor consistency with the repository's logging standards.Proposed fix
import { ImplicitActionsDuringRendering } from 'storybook/internal/preview-errors'; +import { logger } from 'storybook/internal/client-logger'; import type { Renderer } from 'storybook/internal/types'; @@ if (deprecated) { - console.warn(error); + logger.warn(error); } else { throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/runtime/action.ts` around lines 67 - 80, Replace the raw console.warn call with Storybook's client logger: import the logger from 'storybook/internal/client-logger' and call its warn method with the ImplicitActionsDuringRendering error instead of console.warn; update the block where inDocs/deprecated and error (created from ImplicitActionsDuringRendering using storyRenderer.phase and name) are defined to use clientLogger.warn(error) so runtime logging follows the repo standard.
🧹 Nitpick comments (1)
code/core/src/actions/runtime/action.test.ts (1)
12-13: Restore global keys by original presence, not only original value.Lines 20-21 always reassign properties, which can leave keys present with
undefinedeven if they were absent originally. That can affect later tests that use'key' in globalchecks.Proposed fix
describe('action handler — implicit actions', () => { + const hadPreview = '__STORYBOOK_PREVIEW__' in (global as any); + const hadFeatures = 'FEATURES' in (globalThis as any); const originalPreview = (global as any).__STORYBOOK_PREVIEW__; const originalFeatures = (globalThis as any).FEATURES; @@ afterEach(() => { - (global as any).__STORYBOOK_PREVIEW__ = originalPreview; - (globalThis as any).FEATURES = originalFeatures; + if (hadPreview) { + (global as any).__STORYBOOK_PREVIEW__ = originalPreview; + } else { + delete (global as any).__STORYBOOK_PREVIEW__; + } + if (hadFeatures) { + (globalThis as any).FEATURES = originalFeatures; + } else { + delete (globalThis as any).FEATURES; + } vi.restoreAllMocks(); });Also applies to: 19-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/runtime/action.test.ts` around lines 12 - 13, The test saves originalPreview and originalFeatures but restores them unconditionally, which can leave properties set to undefined when they were originally absent; update the teardown to record presence (e.g., hadPreview/hadFeatures) when capturing (global as any).__STORYBOOK_PREVIEW__ and (globalThis as any).FEATURES and then on restore set the property back only if it was originally present (assign originalPreview/originalFeatures) otherwise delete the property (use delete (global as any).__STORYBOOK_PREVIEW__ and delete (globalThis as any).FEATURES) so key presence matches original state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/actions/runtime/action.test.ts`:
- Line 5: Change the relative import in action.test.ts to include the explicit
.ts extension: locate the import statement "import { action } from './action';"
and update it to import from './action.ts' so it follows the repository's import
rule; ensure any other imports in this test file follow the same explicit .ts
extension convention.
- Around line 7-9: The test uses vi.mock('storybook/preview-api') without spy:
true and has an inline vi.spyOn(...) inside a test; update the module mock to
vi.mock('storybook/preview-api', { spy: true }, ...) and move any inline
vi.spyOn(...) mockImplementation calls out of the test into the test suite's
beforeEach so all mock behaviors for the file (action.test.ts) are declared in
beforeEach; ensure the spy created via vi.mock supports spyOn and the prior
inline mockImplementation is replicated in that beforeEach setup.
---
Outside diff comments:
In `@code/core/src/actions/runtime/action.ts`:
- Around line 67-80: Replace the raw console.warn call with Storybook's client
logger: import the logger from 'storybook/internal/client-logger' and call its
warn method with the ImplicitActionsDuringRendering error instead of
console.warn; update the block where inDocs/deprecated and error (created from
ImplicitActionsDuringRendering using storyRenderer.phase and name) are defined
to use clientLogger.warn(error) so runtime logging follows the repo standard.
---
Nitpick comments:
In `@code/core/src/actions/runtime/action.test.ts`:
- Around line 12-13: The test saves originalPreview and originalFeatures but
restores them unconditionally, which can leave properties set to undefined when
they were originally absent; update the teardown to record presence (e.g.,
hadPreview/hadFeatures) when capturing (global as any).__STORYBOOK_PREVIEW__ and
(globalThis as any).FEATURES and then on restore set the property back only if
it was originally present (assign originalPreview/originalFeatures) otherwise
delete the property (use delete (global as any).__STORYBOOK_PREVIEW__ and delete
(globalThis as any).FEATURES) so key presence matches original state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c3dc644-3264-4743-9082-a60e99d976e9
📒 Files selected for processing (2)
code/core/src/actions/runtime/action.test.tscode/core/src/actions/runtime/action.ts
|
|
||
| import { global } from '@storybook/global'; | ||
|
|
||
| import { action } from './action'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the test file and check its contents around line 5
fd -t f "action.test.ts" code/core/src/actions/runtime/Repository: storybookjs/storybook
Length of output: 109
🏁 Script executed:
# Read the test file to verify the import statement
head -n 15 code/core/src/actions/runtime/action.test.tsRepository: storybookjs/storybook
Length of output: 519
🏁 Script executed:
# Check if the target module (action.ts) exists in the same directory
ls -la code/core/src/actions/runtime/ | grep -E "^-.*action\.(ts|tsx|js|jsx)$"Repository: storybookjs/storybook
Length of output: 123
Add explicit .ts extension for the relative import.
Line 5 should use an explicit extension (./action.ts) to match repository import rules.
Proposed fix
-import { action } from './action';
+import { action } from './action.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.
Suggested change
| import { action } from './action'; | |
| import { action } from './action.ts'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/actions/runtime/action.test.ts` at line 5, Change the relative
import in action.test.ts to include the explicit .ts extension: locate the
import statement "import { action } from './action';" and update it to import
from './action.ts' so it follows the repository's import rule; ensure any other
imports in this test file follow the same explicit .ts extension convention.
Comment on lines
+7
to
+9
| vi.mock('storybook/preview-api', () => ({ | ||
| addons: { getChannel: () => ({ emit: vi.fn() }) }, | ||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/actions/runtime/action.test.ts | head -50Repository: storybookjs/storybook
Length of output: 2103
Align Vitest mocks with the repo's spy-mocking conventions.
Lines 7-9 use vi.mock() without the spy: true option, and line 39 has an inline vi.spyOn() mock implementation inside a test case. Both violate the coding guidelines:
- "Use
vi.mock()with thespy: trueoption for all package and file mocks" - "Avoid inline mock implementations within test cases"
- "Implement mock behaviors in
beforeEachblocks"
Proposed refactor
+import { addons } from 'storybook/preview-api';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { global } from '@storybook/global';
import { action } from './action';
-vi.mock('storybook/preview-api', () => ({
- addons: { getChannel: () => ({ emit: vi.fn() }) },
-}));
+vi.mock('storybook/preview-api', { spy: true });
describe('action handler — implicit actions', () => {
const originalPreview = (global as any).__STORYBOOK_PREVIEW__;
const originalFeatures = (globalThis as any).FEATURES;
+ const mockChannel = { emit: vi.fn() };
+ let warnSpy: ReturnType<typeof vi.spyOn>;
beforeEach(() => {
(globalThis as any).FEATURES = { disallowImplicitActionsInRenderV8: true };
+ vi.mocked(addons.getChannel).mockReturnValue(mockChannel as any);
+ warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
});
// ... other test setup ...
it('warns instead of throwing when the render is in docs viewMode', () => {
setRender({ phase: 'rendering', viewMode: 'docs' });
- const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
const handler = action('onBlabla', { implicit: true });
expect(() => handler()).not.toThrow();
- expect(warn).toHaveBeenCalled();
+ expect(warnSpy).toHaveBeenCalled();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/actions/runtime/action.test.ts` around lines 7 - 9, The test
uses vi.mock('storybook/preview-api') without spy: true and has an inline
vi.spyOn(...) inside a test; update the module mock to
vi.mock('storybook/preview-api', { spy: true }, ...) and move any inline
vi.spyOn(...) mockImplementation calls out of the test into the test suite's
beforeEach so all mock behaviors for the file (action.test.ts) are declared in
beforeEach; ensure the spy created via vi.mock supports spyOn and the prior
inline mockImplementation is replicated in that beforeEach setup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #33758
What I did
When a story is embedded in a docs page (
viewMode: 'docs') and an arg matched byactions.argTypesRegex(e.g.^on.*) fires during render — for example a Vue 3 component emitting an event fromonMounted— the implicit action handler currently throwsImplicitActionsDuringRendering, which crashes the entire docs page.The throw exists to protect play functions from accidentally relying on implicit actions, but docs renders have no play-function semantics. This PR downgrades the throw to a
console.warnwhen the activeStoryRenderhasviewMode === 'docs'. Story view (viewMode === 'story') is unchanged, so play-function safety is preserved.The discriminator is reliable: docs-embedded stories are mounted via
Preview.renderStoryToElement, which hard-codes'docs'as theStoryRenderview mode, andViewModeis exhaustively'story' | 'docs'.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
New unit tests in
code/core/src/actions/runtime/action.test.ts:viewMode: 'story'+phase: 'rendering'→ throws (preserves play-function safety)viewMode: 'docs'+phase: 'rendering'→ warns, no throwManual testing
yarn task sandbox --template vue3-vite/default-ts --start-from autoonMountedand a story with anonBlablaargType (no matchingargsentry).ImplicitActionsDuringRendering, after this fix the docs page renders and the warning shows in the console.Documentation
This is a behavior fix, not a deprecation — no migration entry added. Happy to add a note if reviewers prefer.
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes