Actions: Warn instead of throw for implicit actions in docs renders#34505
Open
EricMChavez wants to merge 2 commits intostorybookjs:nextfrom
Open
Actions: Warn instead of throw for implicit actions in docs renders#34505EricMChavez wants to merge 2 commits intostorybookjs:nextfrom
EricMChavez wants to merge 2 commits 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughModify implicit-action handling so that when a story renderer exists with 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
- Use storybook client logger instead of raw console.warn - Add explicit .ts extension to relative import - Move vi.spyOn to beforeEach for consistent mock setup - Restore globals by original presence in test teardown Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Hi @EricMChavez, Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the fix works, optimally with before-and-after comparisons. Thank you for your understanding! |
Author
Screenshot Before: implicit actions block render in docs
After: implicit actions throw warning, does not block render in docs
Note: Story view still throws error for implicit actions |
valentinpalkovic
moved this from Human verification
to Empathy Queue (prioritized)
in Core Team Projects
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #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