Test: Introduce disable user event feature flag#33108
Conversation
valentinpalkovic
requested review from
jonniebigodes and
kylegach
as code owners
valentinpalkovic
added
bug
ci:normal
valentinpalkovic
force-pushed
the
valentin/introduce-disable-user-event-feature-flag
branch
2 times, most recently
from
ee96cfa to
c56c659
Compare
📝 WalkthroughWalkthroughAdds a new feature flag/config option to control automatic Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Preview as Preview runtime
participant Global as globalThis.FEATURES
participant Clipboard as Clipboard API check
participant UserEvent as `@testing-library/user-event` setup
Note over Global,Preview: At startup Preview evaluates feature flag
Preview->>Global: read FEATURES.userEventSetup
alt userEventSetup === false
Preview->>UserEvent: SKIP setup (flag disables)
Note right of UserEvent: userEvent not initialized
else userEventSetup !== false
Preview->>Clipboard: check clipboard availability
alt clipboard available
Preview->>UserEvent: initialize userEvent (setup)
Note right of UserEvent: userEvent available to stories/tests
else clipboard unavailable
Preview->>UserEvent: SKIP setup (clipboard guard)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
🧹 Nitpick comments (2)
code/core/src/test/preview.ts (1)
163-164: Document the precedence between global and story-level settings.The
disableUserEventparameter allows story-level control, but the relationship with the globalfeatures.userEventSetupflag should be documented. Consider adding a JSDoc comment explaining that this parameter overrides the global setting.test?: { /** Ignore unhandled errors during test execution */ dangerouslyIgnoreUnhandledErrors?: boolean; /** Whether to throw exceptions coming from the play function */ throwPlayFunctionExceptions?: boolean; - /** Disable setting up @testing-library/user-event (browser-only) */ + /** + * Disable setting up @testing-library/user-event (browser-only). + * Overrides the global features.userEventSetup configuration for this story. + */ disableUserEvent?: boolean; };docs/api/main-config/main-config-features.mdx (1)
152-164: Clarify default behavior and improve sentence structure.The documentation states "Defaults to enabled for web renderers" but doesn't explain what happens when the property is
undefined. Consider being more explicit about the default behavior.Additionally, the static analysis tool flags the sentence structure on line 158.
Consider this revision for clarity:
-Control automatic setup of Testing Library's `userEvent` in Storybook's preview runtime. When enabled, Storybook sets up `userEvent` and wires clipboard helpers for web-based environments. Disable this in non-DOM environments (e.g., React Native) or when you want full control over interaction utilities. +Control automatic setup of Testing Library's `userEvent` in Storybook's preview runtime. When enabled (or `undefined` for web renderers), Storybook automatically sets up `userEvent` and wires clipboard helpers. Disable this in non-DOM environments (e.g., React Native) or when you want full control over interaction utilities. -Defaults to enabled for web renderers. Can be turned off globally via `features`. +Defaults to `true` for web renderers when not explicitly set. Set to `false` to disable globally via the `features` configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/component-testing/components/test-fn.stories.tsx(1 hunks)code/core/src/test/preview.ts(2 hunks)code/core/src/types/modules/core-common.ts(1 hunks)docs/_snippets/main-config-features-user-event-instrumentation.md(1 hunks)docs/api/main-config/main-config-features.mdx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/component-testing/components/test-fn.stories.tsxdocs/_snippets/main-config-features-user-event-instrumentation.mdcode/core/src/types/modules/core-common.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/types/modules/core-common.ts
🧬 Code graph analysis (1)
code/core/src/component-testing/components/test-fn.stories.tsx (1)
code/core/src/test/testing-library.ts (1)
userEvent(123-126)
🪛 LanguageTool
docs/api/main-config/main-config-features.mdx
[style] ~158-~158: To form a complete sentence, be sure to include a subject.
Context: ... Defaults to enabled for web renderers. Can be turned off globally via features. ...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/types/modules/core-common.ts (1)
454-458: LGTM! Clear documentation and type definition.The new
userEventSetupfeature flag is well-documented and correctly typed. The JSDoc clearly explains when to disable this feature (non-DOM environments like React Native, or manual control scenarios).docs/_snippets/main-config-features-user-event-instrumentation.md (1)
1-51: LGTM! Comprehensive configuration examples.The documentation snippets clearly demonstrate how to disable
userEventSetupacross different configuration formats (CSF 3 and CSF Next, both JS and TS). The examples are consistent and use appropriate placeholders.docs/api/main-config/main-config-features.mdx (1)
22-22: Verify alphabetical ordering in the type block.The
userEventSetup?: booleanproperty appears afterexperimentalTestSyntaxbut beforehighlight. Please verify this follows the intended alphabetical order for the type definition.
| // Verifies that disableUserEvent parameter prevents userEvent from being set up | ||
| export const UserEventDisabled = meta.story({ | ||
| parameters: { test: { disableUserEvent: true } }, | ||
| play: async ({ userEvent }) => { | ||
| // userEvent should be undefined when disableUserEvent is true | ||
| await expect(userEvent).toBeUndefined(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Test depends on incomplete implementation in preview.ts.
This test story expects userEvent to be undefined when parameters.test.disableUserEvent is true, but the parameter is not currently consumed in the enhanceContext function in code/core/src/test/preview.ts (line 98). The test will fail until that implementation is completed.
See the review comment on code/core/src/test/preview.ts lines 98-102 for the required fix.
🤖 Prompt for AI Agents
In code/core/src/test/preview.ts around lines 98-102, enhanceContext currently
ignores the parameters.test.disableUserEvent flag; update it to read
context.parameters?.test?.disableUserEvent and, when true, skip
initializing/attaching userEvent to the enhanced context (leave userEvent
undefined), otherwise proceed with the existing userEvent setup; ensure any
existing conditional paths and typings handle userEvent possibly being undefined
so the story's play can assert it.
| const userEventDisabled = globalThis?.FEATURES?.userEventSetup === false; | ||
|
|
||
| const clipboard = globalThis.window?.navigator?.clipboard; | ||
| if (clipboard) { | ||
| // TODO: Remove clipboard check in SB 11 | ||
| if (!userEventDisabled && clipboard) { |
There was a problem hiding this comment.
Critical: Story-level disableUserEvent parameter is not consumed.
The code defines userEventDisabled based solely on the global feature flag globalThis?.FEATURES?.userEventSetup, but the story-level parameter parameters.test.disableUserEvent (defined at lines 163-164) is never checked or used in the enhanceContext function.
The UserEventDisabled story in test-fn.stories.tsx sets parameters: { test: { disableUserEvent: true } } and expects userEvent to be undefined, but this won't work because the parameter is not being consumed.
Apply this diff to check the story-level parameter:
const enhanceContext: LoaderFunction = async (context) => {
if (globalThis.HTMLElement && context.canvasElement instanceof globalThis.HTMLElement) {
context.canvas = within(context.canvasElement);
}
// userEvent.setup() cannot be called in non browser environment and will attempt to access window.navigator.clipboard
// which will throw an error in react native for example.
- const userEventDisabled = globalThis?.FEATURES?.userEventSetup === false;
+ const userEventDisabled =
+ globalThis?.FEATURES?.userEventSetup === false ||
+ context.parameters?.test?.disableUserEvent === true;
const clipboard = globalThis.window?.navigator?.clipboard;
// TODO: Remove clipboard check in SB 11
if (!userEventDisabled && clipboard) {📝 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 userEventDisabled = globalThis?.FEATURES?.userEventSetup === false; | |
| const clipboard = globalThis.window?.navigator?.clipboard; | |
| if (clipboard) { | |
| // TODO: Remove clipboard check in SB 11 | |
| if (!userEventDisabled && clipboard) { | |
| const userEventDisabled = | |
| globalThis?.FEATURES?.userEventSetup === false || | |
| context.parameters?.test?.disableUserEvent === true; | |
| const clipboard = globalThis.window?.navigator?.clipboard; | |
| // TODO: Remove clipboard check in SB 11 | |
| if (!userEventDisabled && clipboard) { |
🤖 Prompt for AI Agents
In code/core/src/test/preview.ts around lines 98 to 102, the current
userEventDisabled boolean only checks the global feature flag and ignores the
story-level parameter parameters.test.disableUserEvent — update the logic in
enhanceContext to read ctx.parameters?.test?.disableUserEvent (or equivalent for
the story context) and treat the story-level flag as authoritative (i.e.,
userEventDisabled = ctx.parameters?.test?.disableUserEvent === true ||
globalThis?.FEATURES?.userEventSetup === false), then use that combined value
when deciding whether to initialize userEvent/clipboard so stories that set
parameters.test.disableUserEvent:true correctly get userEvent undefined.
|
View your CI Pipeline Execution ↗ for commit 827e931
☁️ Nx Cloud last updated this comment at |
|
Thanks Valentin. If you can make a canary version I'm happy to test this out 🙏 |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
valentinpalkovic
force-pushed
the
valentin/introduce-disable-user-event-feature-flag
branch
from
c56c659 to
a22663f
Compare
valentinpalkovic
force-pushed
the
valentin/introduce-disable-user-event-feature-flag
branch
from
a22663f to
1bc0c49
Compare
|
@dannyhw Here we go: It seems like we have some issues creating canaries. Taking a look |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/api/main-config/main-config-features.mdx (1)
156-158: Minor style improvement: Add subject to sentence.The sentence "Can be turned off globally via
features." is missing a subject. Consider revising for clarity.Apply this diff:
-Control automatic setup of Testing Library's `userEvent` in Storybook's preview runtime. When enabled, Storybook sets up `userEvent` and wires clipboard helpers for web-based environments. Disable this in non-DOM environments (e.g., React Native) or when you want full control over interaction utilities. +Control automatic setup of Testing Library's `userEvent` in Storybook's preview runtime. When enabled, Storybook sets up `userEvent` and wires clipboard helpers for web-based environments. Disable this in non-DOM environments (e.g., React Native) or when you want full control over interaction utilities. -Defaults to enabled for web renderers. Can be turned off globally via `features`. +Defaults to enabled for web renderers. It can be turned off globally via `features`.As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/component-testing/components/test-fn.stories.tsx(0 hunks)code/core/src/test/preview.ts(1 hunks)code/core/src/types/modules/core-common.ts(1 hunks)docs/_snippets/main-config-features-user-event-instrumentation.md(1 hunks)docs/api/main-config/main-config-features.mdx(4 hunks)
💤 Files with no reviewable changes (1)
- code/core/src/component-testing/components/test-fn.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/_snippets/main-config-features-user-event-instrumentation.md
- code/core/src/types/modules/core-common.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/test/preview.ts
🪛 LanguageTool
docs/api/main-config/main-config-features.mdx
[style] ~158-~158: To form a complete sentence, be sure to include a subject.
Context: ... Defaults to enabled for web renderers. Can be turned off globally via features. ...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/test/preview.ts (2)
98-102: Global flag implementation looks correct; verify story-level control decision.The implementation correctly checks the global feature flag and defaults to enabled when undefined. However, the past review comment mentioned a story-level
parameters.test.disableUserEventparameter that would allow per-story control. The current code does not consume any such parameter, and the test story (test-fn.stories.tsx) that would have demonstrated this capability was removed in this PR.If the decision is to only support global configuration (not per-story), please confirm this is intentional. If per-story control is still desired, the
disableUserEventproperty should be added to theTestParametersinterface (lines 156-164) and checked in theuserEventDisabledlogic.Based on past review comments and the removal of the test story, please confirm:
- Is global-only control the intended design, or should per-story control be supported?
- If per-story control was intentionally removed, consider updating or closing the past review comment to reflect this decision.
98-102: Consider adding verification for the feature flag behavior.The test story (
test-fn.stories.tsx) that would have verified this feature was removed. Given that this flag prevents crashes in React Native environments (per issue #33107), consider:
- Adding a new test to verify that when
FEATURES.userEventSetupis false,context.userEventremains undefined- Documenting manual testing steps for React Native environments
- Confirming that the canary build mentioned in the PR comments has been tested
The PR author offered to create a canary version for testing. Can you confirm this has been tested in the reproduction repository (https://github.com/dannyhw/clipboard-repro)?
docs/api/main-config/main-config-features.mdx (1)
22-22: LGTM! Type definitions are consistent across all renderer variants.The
userEventSetup?: booleanproperty has been correctly added to all three renderer-specific type blocks (React, Angular, and fallback).Also applies to: 44-44, 65-65
valentinpalkovic
changed the title
|
Alright. This one will work: |
|
@valentinpalkovic its working! would be great to get this in a patch 🙏 |
|
something like a feature flag for turning off all dom features? |
Yes, we had that in mind, but the problem is that some DOM features are working thanks to polyfills, and React Native might expose more and more Web APIs. So it is tough to solve this problem holistically with a single flag. |
|
Yeah I think that the goal for meta will be to make as many web apis compatible as possible. I'm not sure the correct solution here but often I just don't want to load certain code paths. Maybe being able to overide things like enhanceContext with a framework implementation would be more extendable. Though not sure if thats simple to do. |
|
Could also potentially revisit making the RN ui a webview |
2 participants
Closes #33107
What I did
features.userEventSetupto control automatic setup of Testing Library’s userEvent in the preview.Motivation
In non-DOM environments (e.g., React Native), automatically calling userEvent.setup() can lead to crashes due to window access.navigator.clipboard and related DOM APIs. A consistent, global toggle provides a safe, predictable way to disable userEvent setup where it’s not applicable.
Default Behavior
features.userEventSetup: false.Usage example
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33108-sha-827e9314. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33108-sha-827e9314 sandboxor in an existing project withnpx storybook@0.0.0-pr-33108-sha-827e9314 upgrade.More information
0.0.0-pr-33108-sha-827e9314valentin/introduce-disable-user-event-feature-flag827e93141763717522)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=33108Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.