Skip to content

Controls: Allow resetting the Select control #33289

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.

Sign up for GitHub

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

Merged
valentinpalkovic merged 1 commit into from
Dec 9, 2025
Merged

Controls: Allow resetting the Select control #33289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/addons/docs/src/blocks/controls/options/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ const SingleSelect: FC<SelectProps> = ({ name, value, options, onChange, argType
{name}
</label>
<OptionsSelect disabled={readonly} id={controlId} value={selection} onChange={handleChange}>
<option key="no-selection" disabled>
<option disabled={selection === NO_SELECTION} key="no-selection">
{NO_SELECTION}
</option>
{Object.keys(options).map((key) => (
Expand Down
24 changes: 23 additions & 1 deletion code/addons/docs/src/blocks/controls/options/SelectOptions.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Meta, StoryObj } from '@storybook/react-vite';

import { fn } from 'storybook/test';
import { expect, fn, userEvent } from 'storybook/test';

import { OptionsControl } from './Options';

Expand Down Expand Up @@ -70,6 +70,28 @@ export const ArrayUndefined: Story = {
},
};

export const ArrayResettable: Story = {
args: {
value: undefined,
},
play: async ({ canvas, args }) => {
const select = canvas.getByRole('combobox');
expect(select).toHaveValue('Choose option...');

await userEvent.click(select);
await userEvent.selectOptions(select, arrayOptions[2]);

expect(args.onChange).toHaveBeenCalledWith(arrayOptions[2]);
expect(select).toHaveValue(arrayOptions[2]);

await userEvent.click(select);
await userEvent.selectOptions(select, 'Choose option...');

expect(args.onChange).toHaveBeenCalledWith(undefined);
expect(select).toHaveValue('Choose option...');
},
};
Comment on lines +73 to +93
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, verify the file exists and check the content at the specified lines
cat -n code/addons/docs/src/blocks/controls/options/SelectOptions.stories.tsx | sed -n '65,100p'

Repository: storybookjs/storybook

Length of output: 1179

🏁 Script executed:

# Find and examine the SelectOptions component that's being tested
find code -name "SelectOptions.*" -type f | grep -v stories | grep -v test

Repository: storybookjs/storybook

Length of output: 47

🏁 Script executed:

# Let's look at the beginning of the story file to understand the setup
head -n 80 code/addons/docs/src/blocks/controls/options/SelectOptions.stories.tsx

Repository: storybookjs/storybook

Length of output: 1652

🏁 Script executed:

# Find and examine the OptionsControl component
cat code/addons/docs/src/blocks/controls/options/Options.tsx

Repository: storybookjs/storybook

Length of output: 1782

🏁 Script executed:

# Find SelectControl implementation
cat code/addons/docs/src/blocks/controls/options/Select.tsx

Repository: storybookjs/storybook

Length of output: 4658

🏁 Script executed:

# Search for other play function tests in stories that test controlled components
# Look for patterns where onChange is called and then value is checked
rg -l "play:.*async" code/addons/docs/src/blocks/controls/options/*.stories.tsx

Repository: storybookjs/storybook

Length of output: 135

🏁 Script executed:

# Search for play functions that test onChange and then check values/props
rg -n "play:.*async.*\{" code --type=tsx -A 20 | rg -B 5 -A 15 "onChange.*\n.*expect.*\(.*\)" | head -60

Repository: storybookjs/storybook

Length of output: 284

🏁 Script executed:

# Let me search for similar test patterns more carefully
rg -U "play.*async.*\{[\s\S]*?onChange[\s\S]*?expect.*toHaveValue" code --type ts --type tsx -A 5 -B 2 | head -80

Repository: storybookjs/storybook

Length of output: 92

🏁 Script executed:

# Search more broadly for play function patterns without file type restrictions
rg -A 30 "export const ArrayResettable" code/addons/docs/src/blocks/controls/options/SelectOptions.stories.tsx

Repository: storybookjs/storybook

Length of output: 883

🏁 Script executed:

# Check the Storybook version and look for test utilities documentation
cat code/package.json | grep -A 2 -B 2 '"storybook"'

Repository: storybookjs/storybook

Length of output: 234

🏁 Script executed:

# Check for test setup and jest configuration
find code/addons/docs -name "*.config.*" -o -name "jest.setup.*" -o -name "vitest.config.*" | head -10

Repository: storybookjs/storybook

Length of output: 98

🏁 Script executed:

# Check if there are any test run configurations or results
# Also look for any mocking of onChange that might auto-update args
rg -B 10 "ArrayResettable" code/addons/docs/src/blocks/controls/options/SelectOptions.stories.tsx | head -20

Repository: storybookjs/storybook

Length of output: 218

🏁 Script executed:

# Look for any test execution scripts or configurations that run these story tests
find code/addons/docs -name "*.test.*" -o -name "*.spec.*" | head -20

Repository: storybookjs/storybook

Length of output: 323

🏁 Script executed:

# Search for Storybook's play function or test runner documentation in the repo
find code -name "*.md" -path "*/docs/*" | xargs grep -l "play.*function" | head -5

Repository: storybookjs/storybook

Length of output: 126

🏁 Script executed:

# Read the ESLint rule documentation to understand play function context
cat code/lib/eslint-plugin/docs/rules/context-in-play-function.md

Repository: storybookjs/storybook

Length of output: 2068

🏁 Script executed:

# Search for other play function examples in stories to understand the pattern
rg -l "play.*async.*{" code --type ts -g "*stories.ts" | head -10

Repository: storybookjs/storybook

Length of output: 156

The play function tests user interactions but has a logical flaw in the controlled component pattern.

The test verifies that onChange is called correctly (line 84, 90), but the assumptions about component state updates are incorrect. The SelectControl component is fully controlled—it receives value as a prop and does not maintain internal state. After calling onChange, the args.value is never updated, so the component won't re-render with the new selected value. The assertions on lines 85 and 91 (expect(select).toHaveValue(...)) may pass due to DOM state changes from userEvent.selectOptions(), but they don't reflect the actual rendered state of the controlled component. To properly test a controlled component in a story, either:

  • Update args.value in the play function after calling onChange, or
  • Use a story setup that automatically syncs onChange callbacks back to args (if Storybook provides such a mechanism).

Verify the actual behavior by running this story's play function to confirm whether the select element visually updates or if the test passes due to DOM-level state diverging from the React component's rendered value.

Copy link
Member Author

@Sidnioulz Sidnioulz Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to put it is that the transient states before re-rendering are correct, so there is no flash of incorrect state :p Do you buy that, @ghengeveld? :P

Ultimately the test is testing some irrelevant things that only matter in edge cases, but it is also testing the relevant things so I don't mind leaving it the way it is.


export const ArrayMultiUndefined: Story = {
args: {
type: 'multi-select',
Expand Down
Loading