-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Interactions: Add disable parameter for interactions panel #33368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interactions: Add disable parameter for interactions panel #33368
Conversation
|
Hi @jeevikar14! Thanks for your PR. Until it gets assigned and reviewed, one thing you can do is to add documentation for this change. You can look at how other addons are documented and copy that in the |
|
View your CI Pipeline Execution ↗ for commit 652efe3 ☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/component-testing/manager.tsx (1)
29-31: Implementation is correct, but consider extracting for testability.The disabled callback logic correctly uses optional chaining and handles edge cases. However, the inline implementation cannot be directly unit tested.
As noted in the review of
manager.test.tsx, consider extracting this logic into an exported function (e.g.,isInteractionsDisabled) to enable proper unit testing of the actual implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/component-testing/constants.ts(1 hunks)code/core/src/component-testing/manager.test.tsx(1 hunks)code/core/src/component-testing/manager.tsx(2 hunks)code/core/src/component-testing/types.ts(1 hunks)code/core/template/stories/component-play.stories.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/component-testing/manager.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/component-testing/manager.test.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/component-testing/manager.test.tsxcode/core/template/stories/component-play.stories.tscode/core/src/component-testing/types.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/component-testing/manager.test.tsxcode/core/template/stories/component-play.stories.tscode/core/src/component-testing/types.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/component-testing/manager.test.tsxcode/core/template/stories/component-play.stories.tscode/core/src/component-testing/types.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/component-testing/manager.test.tsxcode/core/template/stories/component-play.stories.tscode/core/src/component-testing/types.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/component-testing/manager.test.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/component-testing/manager.test.tsxcode/core/template/stories/component-play.stories.tscode/core/src/component-testing/types.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
🧠 Learnings (15)
📓 Common learnings
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.
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest
Applied to files:
code/core/src/component-testing/manager.test.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/template/stories/component-play.stories.tscode/core/src/component-testing/manager.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/template/stories/component-play.stories.ts
📚 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/template/stories/component-play.stories.tscode/core/src/component-testing/constants.tscode/core/src/component-testing/manager.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/template/stories/component-play.stories.tscode/core/src/component-testing/manager.tsx
🧬 Code graph analysis (1)
code/core/src/component-testing/manager.tsx (1)
code/core/src/component-testing/constants.ts (1)
PARAM_KEY(3-3)
⏰ 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: Core Unit Tests, windows-latest
- GitHub Check: normal
🔇 Additional comments (5)
code/core/src/component-testing/constants.ts (1)
3-3: LGTM! Good practice to centralize the parameter key.Adding a constant for the parameter key promotes consistency and maintainability across the codebase.
code/core/src/component-testing/types.ts (1)
1-21: LGTM! Well-documented TypeScript interfaces.The type definitions are clear and the JSDoc comments provide helpful context with documentation links. The structure aligns well with Storybook's parameter patterns.
code/core/template/stories/component-play.stories.ts (1)
28-34: LGTM! Good example of the disable parameter usage.This story serves as both documentation and a manual test case for the new disable functionality.
code/core/src/component-testing/manager.tsx (2)
6-6: LGTM! Appropriate type import.The
API_StoryEntrytype import is necessary for typing the disabled callback's parameters.
10-10: LGTM! Using the centralized constant.Good practice to import and use
PARAM_KEYfor consistency across the codebase.
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import type { InteractionsParameters } from './types'; | ||
| import { PARAM_KEY } from './constants'; | ||
|
|
||
| describe('Interactions Panel Disable Parameter', () => { | ||
| it('should return true when interactions.disable is true', () => { | ||
| const parameters: InteractionsParameters = { | ||
| [PARAM_KEY]: { | ||
| disable: true, | ||
| }, | ||
| }; | ||
|
|
||
| const disabled = !!parameters?.[PARAM_KEY]?.disable; | ||
| expect(disabled).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when interactions.disable is false', () => { | ||
| const parameters: InteractionsParameters = { | ||
| [PARAM_KEY]: { | ||
| disable: false, | ||
| }, | ||
| }; | ||
|
|
||
| const disabled = !!parameters?.[PARAM_KEY]?.disable; | ||
| expect(disabled).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when interactions parameter is not provided', () => { | ||
| const parameters: InteractionsParameters = {}; | ||
|
|
||
| const disabled = !!parameters?.[PARAM_KEY]?.disable; | ||
| expect(disabled).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when disable is undefined', () => { | ||
| const parameters: InteractionsParameters = { | ||
| [PARAM_KEY]: {}, | ||
| }; | ||
|
|
||
| const disabled = !!parameters?.[PARAM_KEY]?.disable; | ||
| expect(disabled).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should import and verify the actual implementation.
These tests replicate the disable logic inline (lines 14, 25, 32, 41) rather than testing the actual disabled callback from manager.tsx. If the implementation in manager.tsx changes, these tests would still pass, providing false confidence.
Recommended approach:
- Extract the disabled logic from
manager.tsxinto an exported function:
// In manager.tsx
export const isInteractionsDisabled = (parameters: API_StoryEntry['parameters']): boolean => {
return !!parameters?.[PARAM_KEY]?.disable;
};- Update the test file to import and test the actual function:
import { describe, expect, it } from 'vitest';
import { isInteractionsDisabled } from './manager';
import type { InteractionsParameters } from './types';
import { PARAM_KEY } from './constants';
describe('Interactions Panel Disable Parameter', () => {
it('should return true when interactions.disable is true', () => {
const parameters: InteractionsParameters = {
[PARAM_KEY]: { disable: true },
};
expect(isInteractionsDisabled(parameters)).toBe(true);
});
// ... other tests using isInteractionsDisabled(parameters)
});- Use the exported function in the panel registration:
disabled: isInteractionsDisabled,This ensures tests verify the actual implementation and catch regressions.
As per coding guidelines: "Export functions that need to be tested from their modules" and "Write meaningful unit tests that actually import and call the functions being tested."
🤖 Prompt for AI Agents
In code/core/src/component-testing/manager.test.tsx lines 1-44: the tests inline
the disable logic instead of calling the real implementation; export a function
from manager.tsx (e.g., isInteractionsDisabled(parameters): boolean { return
!!parameters?.[PARAM_KEY]?.disable; }), update manager.tsx to use that exported
function for the panel's disabled callback, then change this test file to import
isInteractionsDisabled and assert its return value for each case instead of
repeating the inline logic; ensure types/import paths match existing module
structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - Extracted disable logic to utils.ts as isInteractionsDisabled() and updated tests to verify the actual implementation instead of replicating logic inline. All tests passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/component-testing/utils.ts (1)
10-12: Consider adding JSDoc for better developer experience.The implementation is correct and handles edge cases properly using optional chaining. However, adding JSDoc documentation would improve discoverability and clarify the function's purpose.
For example:
+/** + * Checks if the interactions panel should be disabled for a story. + * @param parameters - Story parameters containing interaction configuration + * @returns true if interactions are disabled via parameters.interactions.disable + */ export function isInteractionsDisabled(parameters: API_StoryEntry['parameters']): boolean { return !!parameters?.[PARAM_KEY]?.disable; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/component-testing/manager.test.tsx(1 hunks)code/core/src/component-testing/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/component-testing/manager.test.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/component-testing/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/component-testing/utils.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/component-testing/utils.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/component-testing/utils.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/component-testing/utils.ts
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/component-testing/utils.ts (2)
1-2: LGTM!The import of
API_StoryEntrytype provides proper type safety for the new utility function.
8-9: LGTM!Using the
PARAM_KEYconstant ensures consistency across the codebase when accessing the interactions parameter.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
Trade-off analysis:
This is well within acceptable limits for adding a new feature. |
|
@ndelangen Hi sir , I wanted to follow up on this PR. |
|
@jeevikar14 thank you so much! |
Adds support for disabling the interactions panel on a per-story basis using the
interactions.disableparameter.Fixes #33339
Problem
The interactions panel could not be disabled for specific stories, unlike other addons (a11y, viewport, backgrounds, etc.). This caused performance issues when the panel tried to pretty-print large objects in visual regression tests.
Solution
Implemented the
interactions.disableparameter following the same pattern as other Storybook addons:InteractionsParametersinterface withdisableoptionPARAM_KEYconstant for consistencyUsage