CLI: In csf-factories codemod only remove types which are unused #33020
Conversation
|
View your CI Pipeline Execution ↗ for commit 8ece7fd
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughExtracts inline logic that removed unused Storybook-specific TypeScript types into a new exported helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Factory as story-to-csf-factory
participant Remover as removeUnusedTypes
participant Program as ProgramNode
participant AST as CSF_AST
participant Cleanup as cleanupTypeImports
Factory->>Remover: removeUnusedTypes(ProgramNode, CSF_AST)
activate Remover
Remover->>AST: scan for declared type aliases & interfaces
Remover->>AST: scan for referenced type identifiers
Remover->>Remover: mark declarations referencing disallowed Storybook types
Remover->>Program: mutate program.body to remove unused marked declarations
Remover->>Cleanup: cleanupTypeImports(AST, removedNames)
deactivate Remover
Remover-->>Factory: return (void) — AST mutated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-05T09:38:47.712ZApplied to files:
📚 Learning: 2025-11-05T09:37:25.920ZApplied to files:
📚 Learning: 2025-09-24T09:39:39.233ZApplied to files:
🧬 Code graph analysis (1)code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts (2)
🪛 Biome (2.1.2)code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts[error] 15-15: Do not shadow the global "unescape" property. Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global. (lint/suspicious/noShadowRestrictedNames) ⏰ 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)
🔇 Additional comments (3)
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: 1
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
670-702: Consider adding test coverage for transitive type alias usage.The test validates that used type aliases are preserved when referenced in variable declarations. However, it doesn't cover the case where a Storybook type alias is used by another type alias:
type Story = StoryObj<Props>; type MyStory = Story; // Story used hereThis edge case might not be handled correctly by the current implementation (see related comment on the production code).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts(1 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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/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/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/codemod/helpers/story-to-csf-factory.test.ts
🧠 Learnings (16)
📓 Common learnings
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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
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: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 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.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 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.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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
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
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
626-668: LGTM! Test effectively validates the fix for issue #32932.This test confirms that user-defined generic types (like
Data = Record<string, any>) are preserved when used, while unused Storybook-specific type aliases are correctly removed. The inline snapshot provides clear validation of the expected transformation.code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (3)
26-73: LGTM! Properly identifies Storybook-related type aliases.The function correctly traverses TSTypeAliasDeclarations and identifies those referencing Storybook types, including handling for qualified names (e.g.,
Namespace.Meta). The early stopping withinnerPath.stop()is a good optimization.
114-138: LGTM! Clean separation of concerns and correct removal logic.The function properly orchestrates the detection and removal process. The filter logic correctly preserves all nodes except the identified unused Storybook type aliases.
444-449: LGTM! Clear integration with well-documented two-step cleanup process.The comments effectively distinguish between removing unused type aliases (new functionality) and cleaning up type imports (existing functionality). The sequencing is logical.
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
Outdated
Show resolved
Hide resolved
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (1)
11-19: Centralize the disallow list.With type cleanup delegated to
removeUnusedTypes, this localtypesDisallowListcopy is now redundant and duplicates the list maintained inside the helper. Let’s drop it (or import the shared one) so we only maintain the list in one place and avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts(1 hunks)code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts(1 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
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.
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.
📚 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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
📚 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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
📚 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/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts (2)
code/core/src/csf-tools/CsfFile.ts (2)
loadCsf(1060-1064)printCsf(1079-1081)code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts (1)
removeUnusedTypes(53-105)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (1)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts (1)
removeUnusedTypes(53-105)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts (2)
24-85: Test logic is sound; remove unnecessaryasync.The test correctly verifies that unused Storybook type aliases and interfaces are removed while preserving imports and exports. However, the
asynckeyword is unnecessary since no asynchronous operations occur.Apply this diff:
- it('should remove unused Storybook types', async () => { + it('should remove unused Storybook types', () => {
87-121: Remove the unnecessaryasynckeyword from the test function.The test function is marked
asyncbut performs no asynchronous operations. The test expectations are correct—Alias2is properly preserved because theremoveUnusedTypesimplementation only removes unused types that directly reference disallowed Storybook types. SinceAlias2only indirectly referencesStoryFn(throughAlias), it is correctly kept.- it('should not remove used Storybook types', async () => { + it('should not remove used Storybook types', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
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.
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.
📚 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/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts (2)
code/core/src/csf-tools/CsfFile.ts (2)
loadCsf(1060-1064)printCsf(1079-1081)code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts (1)
removeUnusedTypes(53-105)
🪛 Biome (2.1.2)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts
[error] 15-15: Do not shadow the global "unescape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts (3)
1-8: LGTM!All imports are necessary and properly used in the test suite.
10-13: Custom snapshot serializer looks good.The serializer ensures all snapshot values are converted to strings. While
test: () => trueis broad, it's acceptable here since all snapshots in this file are string-based.
17-22: LGTM!The
getTransformedhelper correctly implements the transformation pipeline: load → parse → transform → print. The flow matches the expected usage pattern for CSF transformations.
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.test.ts
Outdated
Show resolved
Hide resolved
…com:storybookjs/storybook into yann/do-not-remove-used-types-csf-factories
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts (2)
54-72: Consider simplifying the type extraction.The comment on line 55 notes that the
idproperty is always anIdentifier, yet lines 56-58 defensively check for this. Given TypeScript's type guarantees for these AST nodes, you can simplify:- if (path.isTSTypeAliasDeclaration() || path.isTSInterfaceDeclaration()) { - // These always have an `id` property that's an Identifier - const idNode = (node as t.TSTypeAliasDeclaration | t.TSInterfaceDeclaration).id; - const name = idNode && t.isIdentifier(idNode) ? idNode.name : undefined; - if (name) { - declaredTypes.add(name); + if (path.isTSTypeAliasDeclaration() || path.isTSInterfaceDeclaration()) { + const name = node.id.name; + declaredTypes.add(name); - // If we previously saw identifiers with this name before the declaration, - // count them now as references (handles reference-before-declaration). - if (pendingIdentifierNames.has(name)) { - referencedTypes.add(name); - } + // If we previously saw identifiers with this name before the declaration, + // count them now as references (handles reference-before-declaration). + if (pendingIdentifierNames.has(name)) { + referencedTypes.add(name); } - - // No need to traverse into the id itself here; we still want to traverse the - // declaration body so that disallowed-type references inside are detected - // by the TSTypeReference/TSExpressionWithTypeArguments handlers below. - return; - } + return; + }This matches the approach already used at line 151 and aligns with ndelangen's past review suggestion.
Based on learnings
107-144: Consider simplifying the owner extraction.Similar to the earlier suggestion, the defensive type checking can be simplified. Since we've already confirmed the owner is a type declaration via type guards, the
idis guaranteed to be anIdentifier:if (path.isTSTypeReference()) { const typeRefNode = node as t.TSTypeReference; const typeNameNode = typeRefNode.typeName; if (t.isIdentifier(typeNameNode) && disallowedTypesSet.has(typeNameNode.name)) { const owner = path.findParent( (p) => p.isTSTypeAliasDeclaration() || p.isTSInterfaceDeclaration() ); - if (owner && (owner.isTSTypeAliasDeclaration() || owner.isTSInterfaceDeclaration())) { - const ownerId = (owner.node as t.TSTypeAliasDeclaration | t.TSInterfaceDeclaration).id; - const ownerName = t.isIdentifier(ownerId) ? ownerId.name : undefined; - if (ownerName) { - typeDeclReferencesDisallowed.add(ownerName); - } + if (owner) { + typeDeclReferencesDisallowed.add(owner.node.id.name); } } return; }Apply the same simplification to the
TSExpressionWithTypeArgumentshandler (lines 128-144).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
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.
⏰ 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 (5)
code/lib/cli-storybook/src/codemod/helpers/remove-unused-types.ts (5)
1-16: LGTM! Clean imports and efficient type tracking setup.The disallowed types list covers the key Storybook-specific types that should be removed when unused, and using a Set for fast lookups is appropriate.
35-48: Excellent approach with forward reference handling.The single-pass traversal with four tracking sets is efficient and elegant. The
pendingIdentifierNamesset correctly handles the case where a type is referenced before its declaration, which can occur in TypeScript files.
75-103: LGTM! Identifier tracking correctly handles references and forward declarations.The logic properly skips declaration identifiers (lines 82-92) and correctly promotes pending identifiers to references when their declaration is encountered. This ensures that types used before they're declared aren't incorrectly marked as unused.
149-164: Filter logic correctly implements the removal conditions.The three-part check ensures only unused Storybook-related type declarations are removed, which directly addresses the bug described in issue #32932. Note that line 151 uses
node.id.namedirectly (correctly), which contrasts with the more defensive approach elsewhere—this simpler pattern could be applied throughout the file.
166-168: LGTM! Proper cleanup of orphaned imports.Delegating to
cleanupTypeImportsafter removing the type declarations ensures that any now-unused Storybook type imports are also removed, keeping the file clean.
yannbf
added
the
needs qa
Error ErrorDangerfile |
yannbf
removed
the
needs qa
Closes #32932
What I did
The CSF factories codemod was not properly checking usage of types before removing them, now it does.
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!
This is well tested in unit tests, but it is a good idea to give it a pass at the csf-factories codemod. An example code would be this:
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 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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Tests
Refactor