A11y: Make search clear button keyboard accessible#32590
Conversation
📝 WalkthroughWalkthroughThe Search.tsx component now renders the clear button based on the presence of input text (inputValue) instead of the menu open state. The button includes an aria-label and, when activated, calls Downshift's reset({ inputValue: '' }) and then closeMenu(), replacing the previous clearSelection behavior. The button remains a CloseIcon and is only rendered while input text exists. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SearchInput
participant ClearButton
participant Downshift
participant Menu
rect rgb(245,245,255)
note right of SearchInput: Clear button visible only when inputValue !== ''
User->>SearchInput: Type query
SearchInput-->>ClearButton: Becomes visible
end
alt Activate clear (click/Enter/Space)
User->>ClearButton: Activate
ClearButton->>Downshift: reset({ inputValue: '' })
Downshift-->>SearchInput: input cleared
ClearButton->>Menu: closeMenu()
Menu-->>User: Menu closed
SearchInput-->>ClearButton: Hides (inputValue === '')
else No input present
ClearButton--x User: Not rendered / not reachable
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: wide-ranging changes across many subsystems (addon removal, AST-based vitest merges, module-loading semantics, automigration heuristics, manager API navigation) with heterogeneous logic requiring focused review per area; multiple deleted modules (Jest) and AST/merge logic demand careful validation. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
ritoban23
force-pushed
the
fix/search-clear-button-keyboard-accessibility
branch
from
d44b51a to
818d81f
Compare
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for attempting a fix on this issue, @ritoban23!
Unfortunately, we've since noticed that the issue is a regression on a specific branch (a11y-consolidation) and we've found and fixed the root cause for this regression. So, I'll be closing the GitHub issue.
I want to discuss your changes to the event handler before closing the PR though. Could you please walk me through what it does?
|
@Sidnioulz hello , apologies for the late reply....I tried to explain my proposed changes to the best of my understanding hope it helps! |
Sidnioulz
added
bug
ui
accessibility
ci:normal
Sidnioulz
changed the title
Absolutely, thank you for this contribution! |
|
View your CI Pipeline Execution ↗ for commit d8e0026
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/_snippets/addon-a11y-config-in-preview.md (1)
25-30: ---Revert
initialGlobalstoglobals— this is the incorrect property name.According to Storybook Accessibility docs, the correct property for global a11y configuration is
globals, notinitialGlobals. The documentation snippet shows:globals: { a11y: { // Optional flag to prevent the automatic check manual: true, }, },This appears to affect all four code examples in the file (lines 25, 61, 99, 137). Revert these changes to use the correct
globalsproperty name.code/lib/cli-storybook/src/automigrate/multi-project.test.ts (1)
10-27: Usespy: trueoption for package mocks.The mock setup doesn't use the
spy: trueoption, which violates the project's coding guidelines. As per coding guidelinesApply this pattern instead:
-vi.mock('storybook/internal/node-logger', async (importOriginal) => { - return { - ...(await importOriginal<typeof import('storybook/internal/node-logger')>()), - prompt: { - multiselect: vi.fn(), - error: vi.fn(), - }, - logger: { - log: vi.fn(), - error: vi.fn(), - debug: vi.fn(), - SYMBOLS: { - success: '✔', - error: '✕', - }, - }, - }; -}); +vi.mock('storybook/internal/node-logger', { spy: true });Then access and configure mocks using
vi.mocked()in your tests (which you're already doing correctly at lines 128, 159, 188, and 225).code/core/src/common/utils/get-addon-annotations.ts (1)
35-46: Critical: Logic bug—checking wrong endpoint for core addons.For core addons,
data.importPathis set toaddon(line 30), but the resolution check on line 43 always verifies${addon}/preview. According to the test expectations and the comment on line 29, core addons export from the index entrypoint, not/preview. This causes core addons to fail the check and returnnull, preventing them from being added todefinePrevieweven though they should be included.Apply this fix:
// If the preview endpoint doesn't exist, we don't need to add the addon to definePreview try { const require = createRequire(import.meta.url); - require.resolve(`${addon}/preview`, { paths: [configDir] }); + require.resolve(data.importPath, { paths: [configDir] }); } catch (err) { return null; }code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (1)
8-12: Add{ spy: true }to vi.mock() per coding guidelines.Vitest 3.2.4 supports
{ spy: true }option, and your coding guidelines require it for all package/file mocks. Update the mock at lines 8-12:vi.mock('node:fs/promises', async (importOriginal) => ({ ...(await importOriginal<typeof import('node:fs/promises')>()), readFile: vi.fn(), writeFile: vi.fn(), }), { spy: true });code/addons/vitest/src/updateVitestFile.ts (1)
142-153: Import dedupe compares only the first specifier and ignores module source. Can silently skip required imports.If the target already imports a specifier with the same local name from a different module (e.g., defineConfig from 'vite' vs 'vitest/config'), this check treats it as “present” and won’t add the correct import. Also fails for multi-specifier and side‑effect imports.
- if ( - !target.program.body.some( - (targetNode) => - targetNode.type === sourceNode.type && - targetNode.specifiers.some((s) => s.local.name === sourceNode.specifiers[0].local.name) - ) - ) { + const src = sourceNode; + const srcNames = new Set(src.specifiers.map((s) => s.local.name)); + const exists = target.program.body.some( + (n) => + n.type === 'ImportDeclaration' && + n.source.value === src.source.value && + ( + (src.specifiers.length === 0 && n.specifiers.length === 0) || + n.specifiers.some((s) => srcNames.has(s.local.name)) + ) + ); + if (!exists) { const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration'); target.program.body.splice(lastImport + 1, 0, sourceNode); }
🧹 Nitpick comments (23)
code/renderers/server/package.json (1)
49-49: Consider updatingts-dedentto v2.2.0 for ESM support.The dependency
ts-dedentis pinned to^2.0.0. Version 2.2.0 (latest stable) added ESM module support and includes stability fixes. Given that Storybook 10 has transitioned to ESM-only (per learnings), updating to 2.2.0 would align the package with the modern build architecture.Consider bumping the version:
- "ts-dedent": "^2.0.0", + "ts-dedent": "^2.2.0",Based on learnings: Storybook 10 moved to ESM-only; ts-dedent 2.2.0 includes ESM support and is stable/low-activity with no breaking changes.
docs/_snippets/addon-a11y-config-in-preview.md (1)
1-144: Consider adding version or deprecation context to the documentation snippet.This file documents configuration for addon-a11y, which was updated in beta versions (10.0.0-beta.10 → 10.0.0-beta.13). The property name change from
globalstoinitialGlobalsrepresents a breaking API change. If users are upgrading from older Storybook versions or the beta versions mentioned, they should understand:
- Which version(s) require
initialGlobalsvs. the deprecatedglobals- Whether there is a migration path or automatic tooling support (e.g., via codemod)
- Whether other parts of the codebase or user guides reference the old property name
Consider adding a callout or note within or adjacent to this snippet file clarifying the version requirement and any migration guidance (e.g., "As of addon-a11y version X, use
initialGlobalsinstead ofglobals"). Per the coding guidelines fordocs/**, significant changes should include migration guides for breaking changes. As per coding guidelines.code/core/src/common/utils/get-addon-annotations.ts (1)
26-26: Consider addressing the TODO and adding tests.Given the complexity of the resolution logic and the potential for edge cases (core vs. non-core addons, ESM vs. CommonJS resolution), tests would be valuable here.
Do you want me to help generate unit tests for this function or open an issue to track this task?
code/frameworks/nextjs/src/utils.ts (1)
28-32: Consider validating the input parameter.The function handles coercion failure gracefully by returning
false, which is good defensive programming. However, if an invalidversionparameter is passed,semver.gte()may throw an error. Consider validating the input or documenting the expected format.Optionally, add input validation:
export const isNextVersionGte = (version: string): boolean => { const currentVersion = getNextjsVersion(); const coercedVersion = semver.coerce(currentVersion); - return coercedVersion ? semver.gte(coercedVersion, version) : false; + if (!coercedVersion) return false; + const targetVersion = semver.coerce(version); + if (!targetVersion) return false; + return semver.gte(coercedVersion, targetVersion); };Alternatively, document that
versionmust be a valid semver string.code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
58-60: Broaden callee detection for mergeConfig.Callee is assumed to be an Identifier; real-world configs sometimes use MemberExpression or aliased imports. Consider handling both Identifier and MemberExpression for resilience.
Example:
-const isMergeConfigExpression = (path: Declaration) => - isCallExpression(path) && path.callee.name === 'mergeConfig'; +const isMergeConfigExpression = (path: Declaration) => + isCallExpression(path) && + ((path.callee as any)?.type === 'Identifier' && (path.callee as any).name === 'mergeConfig' || + ((path.callee as any)?.type === 'MemberExpression' && + (path.callee as any).property?.name === 'mergeConfig'));code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (3)
16-28: Good coverage for mergeConfig + defineConfig. Add object-literal variant.Please also cover mergeConfig(base, { test: { environment: 'jsdom' } }) to match common patterns and the suggested code change.
Apply:
@@ fileMocks 'vitest.merge-config.ts': ` @@ `, + 'vitest.merge-config-object.ts': ` + import { mergeConfig } from 'vitest/config' + import viteConfig from './vite.config' + + export default mergeConfig( + viteConfig, + { + test: { + environment: 'jsdom', + }, + }, + ) + `,
29-42: Multi-arg case covered. Consider nested mergeConfig too (optional).Nice to see position-agnostic coverage. Optionally add mergeConfig(mergeConfig(base, {}), defineConfig({...})) to ensure deep args still pass.
189-203: Add test for object-literal config; reduce per-test boilerplate.Add a test for the new object-literal mock and consider centralizing default mockImplementation in beforeEach to reduce repetition.
Example:
describe('Check Vitest config files', () => { @@ it('should allow existing test config option with mergeConfig and defineConfig as argument in a different position', async () => { @@ }); + + it('should allow existing test config option with mergeConfig using an object literal', async () => { + vi.mocked(find.any).mockImplementation(coerce('config', 'vitest.merge-config-object.ts')); + const result = await vitestConfigFiles.condition(mockContext, state); + expect(result).toEqual({ type: 'compatible' }); + });And optionally:
beforeEach(() => { vi.mocked(find.any).mockReset(); });Also confirm whether your Vitest version supports vi.mock(..., { spy: true }) and update mocks accordingly to align with team guidelines.
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
231-258: Assert banner write on the final writeFile call.run() writes AST-changes first, then prefixes banner and writes again. Tests currently assert only the first call; add an assertion that the last call includes bannerComment to verify the banner step.
- const writtenContent = mockWriteFile.mock.calls[0][1]; + // First write: AST changes + const firstWrite = mockWriteFile.mock.calls[0]?.[1]; + expect(firstWrite).toMatchInlineSnapshot(/* ...existing snapshot... */); + + // Final write: banner + content + const lastWrite = mockWriteFile.mock.calls.at(-1)?.[1]; + expect(lastWrite).toContain(bannerComment);Also applies to: 281-298, 300-341, 357-398, 400-434
146-229: Add a case for template‑literal expressions.containsDirnameUsage should detect
${__dirname}inside template expressions. Add a positive test:it('should handle mixed quote types', () => { // ... expect(containsDirnameUsage(content)).toBe(true); }); + +it('should detect __dirname inside template literal expressions', () => { + const content = ` + const p = \`\${__dirname}/config.js\`; + `; + expect(containsDirnameUsage(content)).toBe(true); +});code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
223-264: Broaden detection tests: template expressions and false positives.Add coverage for:
- __dirname inside template literal expressions (should be true).
- hasDirnameDefined when definition appears only in comments/strings (should be false).
describe('containsDirnameUsage', () => { it('should detect __dirname usage', () => { /* ... */ }); + it('should detect __dirname in template literal expressions', () => { + const content = ` + const out = \`\${__dirname}/dist\`; + `; + expect(containsDirnameUsage(content)).toBe(true); + }); // ... }); describe('hasDirnameDefined', () => { it('should detect const __dirname definition', () => { /* ... */ }); + it('should not treat comments/strings as definitions', () => { + const content = ` + // const __dirname = "fake"; + const msg = "var __dirname = 'fake'"; + `; + expect(hasDirnameDefined(content)).toBe(false); + }); });Also applies to: 266-298, 300-332
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
149-169: Remove unused insertOffset.insertOffset is never used; drop it to reduce noise.
- const declarationsToInsert: t.Statement[] = []; - let insertOffset = 0; + const declarationsToInsert: t.Statement[] = []; @@ - declarationsToInsert.push(filenameDeclaration); - insertOffset++; + declarationsToInsert.push(filenameDeclaration); @@ - declarationsToInsert.push(dirnameDeclaration); - insertOffset++; + declarationsToInsert.push(dirnameDeclaration);Also applies to: 170-186
212-215: Consider folding banner insertion into updateMainConfig to avoid double I/O.You can prepend bannerComment within the same write pass (e.g., via a pre/post hook in updateMainConfig), reducing two file writes and simplifying tests. Optional.
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
250-256: Align containsRequireUsage with comment/string stripping.Prevent false positives from comments/strings by delegating to containsPatternUsage.
-export function containsRequireUsage(content: string): boolean { - // Check for require() calls - const requireCallRegex = /\brequire\(/; - const requireDotRegex = /\brequire\./; - return requireCallRegex.test(content) || requireDotRegex.test(content); -} +export function containsRequireUsage(content: string): boolean { + return containsPatternUsage(content, /\brequire\s*\(|\brequire\./); +}
289-290: Remove stray doc comment (no implementation).Comment “Check if a specific import already exists...” is orphaned. Remove to avoid confusion.
-/** Check if a specific import already exists in the file */ -code/addons/vitest/src/updateVitestFile.ts (5)
82-108: Broaden unsupported function-notation check (include FunctionExpression).You only detect an ArrowFunctionExpression. defineConfig(() => ({})) and defineConfig(function () { return {} }) are both function notations. Extend the guard to include FunctionExpression.
- targetExportDefault.declaration.arguments[0].type === 'ArrowFunctionExpression' + (targetExportDefault.declaration.arguments[0].type === 'ArrowFunctionExpression' || + targetExportDefault.declaration.arguments[0].type === 'FunctionExpression')
155-171: Variable dedupe checks only the first declarator; misses multi-declarator statements.If a template declares multiple vars in one statement, duplicates can slip in.
- targetNode.declarations.some( - (d) => - 'name' in d.id && - 'name' in sourceNode.declarations[0].id && - d.id.name === sourceNode.declarations[0].id.name - ) + targetNode.declarations.some((d) => + sourceNode.declarations.some( + (sd) => 'name' in d.id && 'name' in sd.id && d.id.name === sd.id.name + ) + )
191-296: Avoid mutating the source AST; ensure idempotent workspace/projects merges.
- You unshift into the template’s array (workspaceOrProjectsProp.value.elements.unshift(...)), mutating the source AST. If the same template AST is reused, later updates accumulate entries.
- Re-running update can duplicate the injected “storybook” project.
Refactor to:
- Clone the template’s properties (structuredClone or t.cloneNode deep) before modifying.
- Skip adding a project if the target already contains a project with test.name === 'storybook'.
- // Add the existing test project to the template's array - workspaceOrProjectsProp.value.elements.unshift(existingTestProject); + // Work on a cloned template to avoid mutating the source AST + const clonedProps = structuredClone(properties) as t.ObjectExpression['properties']; + const clonedTestProp = clonedProps.find( + (p) => p.type === 'ObjectProperty' && p.key.type === 'Identifier' && p.key.name === 'test' + ) as t.ObjectProperty; + const clonedArrProp = (clonedTestProp.value as t.ObjectExpression).properties.find( + (p) => + p.type === 'ObjectProperty' && + p.key.type === 'Identifier' && + (p.key.name === 'workspace' || p.key.name === 'projects') + ) as t.ObjectProperty; + const clonedArr = clonedArrProp.value as t.ArrayExpression; + // Idempotency: avoid duplicate 'storybook' project + const hasStorybook = clonedArr.elements.some( + (el) => + el && + el.type === 'ObjectExpression' && + el.properties.some( + (pp) => + pp.type === 'ObjectProperty' && + pp.key.type === 'Identifier' && + pp.key.name === 'test' && + pp.value.type === 'ObjectExpression' && + pp.value.properties.some( + (np) => + np.type === 'ObjectProperty' && + np.key.type === 'Identifier' && + np.key.name === 'name' && + np.value.type === 'StringLiteral' && + np.value.value === 'storybook' + ) + ) + ); + clonedArr.elements.unshift(existingTestProject); // Remove the existing test property from defineConfig since we're moving it to the array defineConfigProps.properties = defineConfigProps.properties.filter( (p) => p !== existingTestProp ); - // Merge the template properties (which now include our existing test project in the array) - mergeProperties(properties, defineConfigProps.properties); + // Merge cloned template props (with existing project) and skip duplicate storybook project later + mergeProperties(clonedProps, defineConfigProps.properties);Optionally, dedupe array merges generically in mergeProperties for ObjectExpression elements keyed by test.name.
Based on learnings
40-46: Array merge is not idempotent; repeated runs duplicate entries.Concatenating arrays blindly duplicates workspace/projects and plugins (storybookTest). Make merges set-like for known structures.
- targetProp.value.elements.push(...sourceProp.value.elements); + const toAdd = sourceProp.value.elements.filter((el) => { + if (!el || el.type !== 'ObjectExpression') return true; + // Deduplicate by test.name for workspace/projects arrays + const testObj = el.properties.find( + (p) => p.type === 'ObjectProperty' && p.key.type === 'Identifier' && p.key.name === 'test' + ) as t.ObjectProperty | undefined; + const nameProp = + testObj && testObj.value.type === 'ObjectExpression' + ? (testObj.value.properties.find( + (p) => p.type === 'ObjectProperty' && p.key.type === 'Identifier' && p.key.name === 'name' + ) as t.ObjectProperty | undefined) + : undefined; + if (nameProp && nameProp.value.type === 'StringLiteral') { + return !targetProp.value.elements.some( + (e) => + e && + e.type === 'ObjectExpression' && + e.properties.some( + (pp) => + pp.type === 'ObjectProperty' && + pp.key.type === 'Identifier' && + pp.key.name === 'test' && + pp.value.type === 'ObjectExpression' && + pp.value.properties.some( + (np) => + np.type === 'ObjectProperty' && + np.key.type === 'Identifier' && + np.key.name === 'name' && + np.value.type === 'StringLiteral' && + np.value.value === nameProp.value.value + ) + ) + ); + } + return true; + }); + targetProp.value.elements.push(...toAdd);
109-139: No aliased or member expressionmergeConfigcalls detected—refactor improves robustness but is not addressing current issues.The codebase shows no instances of aliased
mergeConfigimports (e.g.,import { mergeConfig as merge }) or member expression calls (e.g.,vite.mergeConfig()). All ~80 uses importmergeConfigdirectly and call it asmergeConfig(...). The suggested enhancement would add defensive support forMemberExpressionpatterns, making the code more resilient to alternative usage styles that don't currently exist in this repository.This is a robustness improvement rather than a fix for existing code.
code/addons/vitest/src/updateVitestFile.test.ts (3)
18-20: Avoid __dirname in ESM tests; derive from import.meta.url.Vitest runs ESM; __dirname may be undefined. Use fileURLToPath for portability.
-import { join } from 'node:path'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; ... -vi.mock('../../../core/src/shared/utils/module', () => ({ - resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')), -})); +vi.mock('../../../core/src/shared/utils/module', () => ({ + resolvePackageDir: vi.fn().mockImplementation(() => + join(fileURLToPath(new URL('..', import.meta.url))) + ), +}));
10-16: Use spy: true and restore mocks between tests.Aligns with our mocking guidance and prevents cross‑test leakage.
-vi.mock('storybook/internal/node-logger', () => ({ +vi.mock('storybook/internal/node-logger', () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), }, -})); +}), { spy: true }); -vi.mock('../../../core/src/shared/utils/module', () => ({ +vi.mock('../../../core/src/shared/utils/module', () => ({ resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')), -})); +}), { spy: true }); + +afterEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); +});Based on learnings
Also applies to: 18-20
46-56: DRY the before/after/diff pattern.Extract a small helper to reduce repetition and make failures easier to read.
function expectTransform( ast: babel.BabelFile['ast'], transform: () => boolean, snapshot: string | RegExp ) { const before = babel.generate(ast).code; const updated = transform(); expect(updated).toBe(true); const after = babel.generate(ast).code; expect(after).not.toBe(before); expect(getDiff(before, after)).toMatchInlineSnapshot(snapshot as any); }Then use: expectTransform(target, () => updateConfigFile(source, target),
...snapshot...);Also applies to: 120-130, 227-237, 302-313, 376-386, 447-457, 524-535, 601-612, 675-686, 745-756, 807-818
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
code/addons/jest/docs/storybook-addon-jest.gifis excluded by!**/*.gifcode/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (91)
CHANGELOG.md(1 hunks)CHANGELOG.prerelease.md(1 hunks)MIGRATION.md(3 hunks)code/addons/a11y/package.json(1 hunks)code/addons/docs/package.json(1 hunks)code/addons/jest/README.md(0 hunks)code/addons/jest/build-config.ts(0 hunks)code/addons/jest/manager.js(0 hunks)code/addons/jest/package.json(0 hunks)code/addons/jest/project.json(0 hunks)code/addons/jest/src/components/Message.tsx(0 hunks)code/addons/jest/src/components/Panel.tsx(0 hunks)code/addons/jest/src/components/Result.tsx(0 hunks)code/addons/jest/src/hoc/provideJestResult.tsx(0 hunks)code/addons/jest/src/index.ts(0 hunks)code/addons/jest/src/manager.tsx(0 hunks)code/addons/jest/src/shared.test.ts(0 hunks)code/addons/jest/src/shared.ts(0 hunks)code/addons/jest/src/types.ts(0 hunks)code/addons/jest/tsconfig.json(0 hunks)code/addons/jest/vitest.config.ts(0 hunks)code/addons/links/package.json(1 hunks)code/addons/onboarding/package.json(1 hunks)code/addons/pseudo-states/package.json(1 hunks)code/addons/themes/package.json(1 hunks)code/addons/vitest/package.json(1 hunks)code/addons/vitest/src/updateVitestFile.test.ts(11 hunks)code/addons/vitest/src/updateVitestFile.ts(2 hunks)code/builders/builder-vite/package.json(1 hunks)code/builders/builder-webpack5/package.json(1 hunks)code/core/package.json(1 hunks)code/core/src/cli/detect.ts(1 hunks)code/core/src/common/utils/get-addon-annotations.ts(2 hunks)code/core/src/common/utils/load-main-config.ts(1 hunks)code/core/src/common/versions.ts(1 hunks)code/core/src/core-server/__for-testing__/main.ts(0 hunks)code/core/src/manager-api/modules/stories.ts(4 hunks)code/core/src/manager-api/tests/stories.test.ts(1 hunks)code/core/src/manager-api/version.ts(1 hunks)code/core/src/manager/components/sidebar/TagsFilterPanel.tsx(1 hunks)code/core/src/node-logger/prompts/prompt-provider-clack.ts(1 hunks)code/core/src/shared/utils/module.ts(2 hunks)code/frameworks/angular/package.json(1 hunks)code/frameworks/ember/package.json(1 hunks)code/frameworks/html-vite/package.json(1 hunks)code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs/package.json(1 hunks)code/frameworks/nextjs/src/config/webpack.ts(1 hunks)code/frameworks/nextjs/src/preset.ts(2 hunks)code/frameworks/nextjs/src/utils.ts(2 hunks)code/frameworks/preact-vite/package.json(1 hunks)code/frameworks/react-native-web-vite/package.json(1 hunks)code/frameworks/react-vite/package.json(1 hunks)code/frameworks/react-webpack5/package.json(1 hunks)code/frameworks/server-webpack5/package.json(1 hunks)code/frameworks/svelte-vite/package.json(1 hunks)code/frameworks/sveltekit/package.json(1 hunks)code/frameworks/vue3-vite/package.json(1 hunks)code/frameworks/web-components-vite/package.json(1 hunks)code/lib/cli-sb/package.json(1 hunks)code/lib/cli-storybook/package.json(1 hunks)code/lib/cli-storybook/src/add.ts(2 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)code/lib/cli-storybook/src/automigrate/multi-project.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/multi-project.ts(1 hunks)code/lib/cli-storybook/src/codemod/csf-factories.ts(2 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts(9 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts(1 hunks)code/lib/codemod/package.json(1 hunks)code/lib/core-webpack/package.json(1 hunks)code/lib/create-storybook/package.json(1 hunks)code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts(2 hunks)code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx(2 hunks)code/lib/csf-plugin/package.json(1 hunks)code/lib/eslint-plugin/package.json(1 hunks)code/lib/react-dom-shim/package.json(1 hunks)code/package.json(1 hunks)code/presets/create-react-app/package.json(1 hunks)code/presets/react-webpack/package.json(1 hunks)code/presets/server-webpack/package.json(1 hunks)code/renderers/html/package.json(1 hunks)code/renderers/preact/package.json(1 hunks)code/renderers/react/package.json(1 hunks)code/renderers/server/package.json(1 hunks)code/renderers/svelte/package.json(1 hunks)code/renderers/vue3/package.json(1 hunks)code/renderers/web-components/package.json(1 hunks)docs/_snippets/addon-a11y-config-in-preview.md(4 hunks)
💤 Files with no reviewable changes (17)
- code/addons/jest/README.md
- code/addons/jest/src/types.ts
- code/addons/jest/project.json
- code/addons/jest/vitest.config.ts
- code/addons/jest/tsconfig.json
- code/addons/jest/src/hoc/provideJestResult.tsx
- code/addons/jest/src/index.ts
- code/addons/jest/src/shared.ts
- code/addons/jest/manager.js
- code/core/src/core-server/for-testing/main.ts
- code/addons/jest/src/shared.test.ts
- code/addons/jest/build-config.ts
- code/addons/jest/src/components/Result.tsx
- code/addons/jest/package.json
- code/addons/jest/src/components/Message.tsx
- code/addons/jest/src/components/Panel.tsx
- code/addons/jest/src/manager.tsx
✅ Files skipped from review due to trivial changes (37)
- code/addons/onboarding/package.json
- code/addons/vitest/package.json
- code/lib/cli-storybook/package.json
- code/frameworks/react-native-web-vite/package.json
- code/renderers/svelte/package.json
- code/renderers/html/package.json
- code/frameworks/vue3-vite/package.json
- code/frameworks/web-components-vite/package.json
- code/frameworks/react-webpack5/package.json
- code/frameworks/nextjs/package.json
- code/core/package.json
- code/lib/csf-plugin/package.json
- code/frameworks/nextjs-vite/package.json
- code/frameworks/react-vite/package.json
- code/lib/create-storybook/package.json
- code/addons/a11y/package.json
- code/renderers/web-components/package.json
- code/frameworks/angular/package.json
- code/builders/builder-vite/package.json
- code/builders/builder-webpack5/package.json
- code/lib/codemod/package.json
- code/addons/pseudo-states/package.json
- code/frameworks/server-webpack5/package.json
- CHANGELOG.prerelease.md
- code/lib/cli-sb/package.json
- code/addons/docs/package.json
- code/addons/themes/package.json
- code/presets/react-webpack/package.json
- code/addons/links/package.json
- CHANGELOG.md
- code/frameworks/sveltekit/package.json
- code/presets/server-webpack/package.json
- code/lib/eslint-plugin/package.json
- code/lib/core-webpack/package.json
- code/renderers/vue3/package.json
- code/frameworks/svelte-vite/package.json
- code/frameworks/html-vite/package.json
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/frameworks/nextjs/src/utils.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/frameworks/preact-vite/package.jsoncode/renderers/react/package.jsoncode/frameworks/ember/package.jsoncode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/presets/create-react-app/package.jsoncode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/renderers/preact/package.jsoncode/core/src/node-logger/prompts/prompt-provider-clack.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/react-dom-shim/package.jsoncode/package.jsoncode/core/src/cli/detect.tscode/lib/cli-storybook/src/add.tscode/core/src/manager-api/version.tscode/renderers/server/package.jsoncode/core/src/manager-api/tests/stories.test.tscode/core/src/common/utils/get-addon-annotations.tscode/core/src/shared/utils/module.tscode/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/core/src/common/utils/load-main-config.tscode/addons/vitest/src/updateVitestFile.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/core/src/manager-api/modules/stories.tscode/frameworks/nextjs/src/config/webpack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/frameworks/nextjs/src/utils.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/cli/detect.tscode/lib/cli-storybook/src/add.tscode/core/src/manager-api/version.tscode/core/src/manager-api/tests/stories.test.tscode/core/src/common/utils/get-addon-annotations.tscode/core/src/shared/utils/module.tscode/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/core/src/common/utils/load-main-config.tscode/addons/vitest/src/updateVitestFile.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/core/src/manager-api/modules/stories.tscode/frameworks/nextjs/src/config/webpack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/frameworks/nextjs/src/utils.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/cli/detect.tscode/lib/cli-storybook/src/add.tscode/core/src/manager-api/version.tscode/core/src/manager-api/tests/stories.test.tscode/core/src/common/utils/get-addon-annotations.tscode/core/src/shared/utils/module.tscode/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/core/src/common/utils/load-main-config.tscode/addons/vitest/src/updateVitestFile.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/core/src/manager-api/modules/stories.tscode/frameworks/nextjs/src/config/webpack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/frameworks/nextjs/src/utils.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/cli/detect.tscode/lib/cli-storybook/src/add.tscode/core/src/manager-api/version.tscode/core/src/manager-api/tests/stories.test.tscode/core/src/common/utils/get-addon-annotations.tscode/core/src/shared/utils/module.tscode/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/core/src/common/utils/load-main-config.tscode/addons/vitest/src/updateVitestFile.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/core/src/manager-api/modules/stories.tscode/frameworks/nextjs/src/config/webpack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update documentation under
docs/for significant changes, including migration guides for breaking changes
Files:
docs/_snippets/addon-a11y-config-in-preview.md
🧠 Learnings (7)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/addons/vitest/src/updateVitestFile.test.ts
📚 Learning: 2025-09-17T08:11:04.287Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.287Z
Learning: Applies to code/vitest.workspace.ts : Keep and use the Vitest workspace configuration at code/vitest.workspace.ts
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/addons/vitest/src/updateVitestFile.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to type and access mocked functions
Applied to files:
code/addons/vitest/src/updateVitestFile.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to scripts/**/*.{ts,js,mjs} : In Node.js scripts, use `storybook/internal/node-logger` instead of `console.*`
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : In application code, use Storybook loggers instead of `console.*` (client code: `storybook/internal/client-logger`; server code: `storybook/internal/node-logger`)
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
🧬 Code graph analysis (12)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(67-216)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
containsDirnameUsage(274-276)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsPatternUsage(259-271)containsDirnameUsage(274-276)hasDirnameDefined(283-287)
code/lib/cli-storybook/src/add.ts (2)
code/core/src/common/utils/load-main-config.ts (1)
loadMainConfig(14-71)code/core/src/common/utils/sync-main-preview-addons.ts (1)
syncStorybookAddons(16-25)
code/core/src/manager-api/tests/stories.test.ts (2)
code/core/src/manager-api/tests/mockStoriesEntries.ts (1)
navigationEntries(71-120)code/core/src/manager-api/tests/url.test.js (1)
store(143-151)
code/frameworks/nextjs/src/preset.ts (1)
code/frameworks/nextjs/src/utils.ts (1)
isNextVersionGte(28-32)
code/lib/cli-storybook/src/automigrate/multi-project.test.ts (1)
code/lib/cli-storybook/src/automigrate/multi-project.ts (1)
promptForAutomigrations(197-250)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
vitestConfigFiles(95-146)
code/core/src/common/utils/load-main-config.ts (1)
code/core/src/shared/utils/module.ts (1)
importModule(54-89)
code/core/src/manager-api/modules/stories.ts (2)
code/core/src/manager-api/tests/url.test.js (1)
store(143-151)code/core/src/csf/index.ts (2)
sanitize(8-16)toId(27-28)
code/frameworks/nextjs/src/config/webpack.ts (1)
code/frameworks/nextjs/src/utils.ts (1)
isNextVersionGte(28-32)
code/addons/vitest/src/updateVitestFile.test.ts (1)
code/addons/vitest/src/updateVitestFile.ts (3)
updateConfigFile(79-301)loadTemplate(9-16)updateWorkspaceFile(303-364)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (6)
containsRequireUsage(251-256)containsFilenameUsage(278-280)containsDirnameUsage(274-276)updateMainConfig(207-230)bannerComment(298-299)BannerConfig(292-296)
| const result = await clack.multiselect<T>({ | ||
| ...options, | ||
| required: options.required, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify: This change appears unrelated to the PR objectives.
This PR is focused on making the search clear button keyboard accessible (UI component), but this file handles CLI prompts (Node.js infrastructure). The change spreads options and then explicitly sets required: options.required, which is redundant unless there's a specific requirement from the @clack/prompts API.
Additionally, other methods in this class (text, confirm, select) pass options directly without this pattern, suggesting inconsistency.
Please clarify:
- Is this change intentional and related to this PR?
- If yes, why is the explicit
requiredproperty needed after the spread? - Should this be in a separate PR or commit?
To verify whether @clack/prompts has specific requirements for the required property in multiselect:
🌐 Web query:
Does @clack/prompts multiselect require the required property to be explicitly set?
💡 Result:
No — you don't have to set required. multiselect's required defaults to false; only set required: true (or a message) if you want to force at least one selection. [1][2]
Sources:
[1] @clack/prompts (npm) — multiselect example showing required: false.
[2] go-clack/prompts docs — MultiSelect Required (default: false).
🏁 Script executed:
#!/bin/bash
# Examine the full multiselect method and compare with other methods in the same class
cat code/core/src/node-logger/prompts/prompt-provider-clack.tsLength of output: 3381
🏁 Script executed:
#!/bin/bash
# Search for similar patterns of spreading options with explicit required in the codebase
rg -n 'required:\s*' code/core/src/node-logger/prompts/ -A2 -B2Length of output: 463
🏁 Script executed:
#!/bin/bash
# Find the MultiSelectPromptOptions type definition
fd -e ts -e tsx | xargs rg -n "type MultiSelectPromptOptions|interface MultiSelectPromptOptions" -A5Length of output: 510
🏁 Script executed:
#!/bin/bash
# Check git history for this specific line to see when it was changed
git log -p --follow -S "required: options.required" -- code/core/src/node-logger/prompts/prompt-provider-clack.ts | head -100Length of output: 59
Remove unnecessary pattern: pass options directly to clack.multiselect.
The { ...options, required: options.required } pattern is redundant. The MultiSelectPromptOptions interface defines required as an optional property (required?: boolean), and @clack/prompts handles the default value internally. This pattern serves no purpose and creates inconsistency with other methods in the class (text, confirm, select), which all pass options directly.
Change lines 56-59 from:
const result = await clack.multiselect<T>({
...options,
required: options.required,
});
To:
const result = await clack.multiselect<T>(options);
🤖 Prompt for AI Agents
In code/core/src/node-logger/prompts/prompt-provider-clack.ts around lines 56 to
59, the call to clack.multiselect() spreads options and reassigns required
redundantly; replace the object literal with a direct pass-through of options by
calling clack.multiselect<T>(options) so it matches the other prompt methods and
lets @clack/prompts handle defaults.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
5 participants
Closes #32582
What I did
Fixed the search clear button keyboard accessibility issue by changing the button's visibility condition from
isOpentoinput. This ensures the clear button remains visible and focusable even when the search input loses focus, allowing keyboard users to tab to and activate it. Also added properaria-labelfor screen readers and improved the clear functionality.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!
cd code && yarn storybook:uiDocumentation
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Bug Fixes
Documentation
Breaking Change