CLI: Narrow CSF Factories Subpath Imports#34889
Closed
alan747271363-art wants to merge 3 commits into
Closed
Conversation
Contributor
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds POSIX path helpers to compute ChangesScoped Storybook subpath imports with file extensions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/cli-storybook/src/codemod/csf-factories.ts`:
- Around line 123-126: The code currently forwards the raw configDir into
getStorybookSubpathImportTarget (and the similar helper at the later call) which
lets relative configDir be resolved from process.cwd(); normalize configDir
against the package operation dir first by resolving it with
packageManager.primaryPackageJson.operationDir (e.g., use
path.resolve(operationDir, configDir) or equivalent) and pass that resolved path
into getStorybookSubpathImportTarget and the other helper call so both
targets/imports are computed relative to the package's operationDir.
- Around line 130-133: The current logic only sets imports['`#storybook/`*'] when
it is missing, leaving stale mappings in package.json; change the code so
imports['`#storybook/`*'] is assigned storybookImportTarget unconditionally (or at
least when its value !== storybookImportTarget) and set shouldWritePackageJson =
true when you overwrite it, using the existing imports['`#storybook/`*'] and
shouldWritePackageJson symbols to detect and replace stale mappings so package
resolution is updated on reruns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12ddf8a9-47b0-4821-966d-9c3bf235b549
📒 Files selected for processing (5)
code/core/src/csf-tools/CsfFile.test.tscode/lib/cli-storybook/src/codemod/csf-factories.test.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts
This comment was marked as low quality.
This comment was marked as low quality.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/codemod/csf-factories.test.ts (1)
14-26: 💤 Low valueMocks should use
spy: truewith behaviors defined inbeforeEach.As per coding guidelines, Vitest mocks should use
vi.mock()with thespy: trueoption and implement mock behaviors inbeforeEachblocks rather than inline factory implementations.♻️ Suggested refactor for guideline compliance
-vi.mock('../automigrate/codemod.ts', () => ({ - runCodemod: vi.fn(), -})); -vi.mock('./helpers/story-to-csf-factory.ts', () => ({ - storyToCsfFactory: vi.fn(async () => 'transformed story'), -})); -vi.mock('./helpers/config-to-csf-factory.ts', () => ({ - configToCsfFactory: vi.fn(async () => 'transformed config'), -})); -vi.mock('storybook/internal/common', async (importOriginal) => ({ - ...(await importOriginal<typeof import('storybook/internal/common')>()), - syncStorybookAddons: vi.fn(), -})); +vi.mock('../automigrate/codemod.ts', { spy: true }); +vi.mock('./helpers/story-to-csf-factory.ts', { spy: true }); +vi.mock('./helpers/config-to-csf-factory.ts', { spy: true }); +vi.mock('storybook/internal/common', { spy: true }); + +import { configToCsfFactory } from './helpers/config-to-csf-factory.ts'; +import { syncStorybookAddons } from 'storybook/internal/common'; beforeEach(() => { vi.clearAllMocks(); process.env.IN_STORYBOOK_SANDBOX = '1'; + vi.mocked(runCodemod).mockResolvedValue(undefined); + vi.mocked(storyToCsfFactory).mockResolvedValue('transformed story'); + vi.mocked(configToCsfFactory).mockResolvedValue('transformed config'); + vi.mocked(syncStorybookAddons).mockResolvedValue(undefined); });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Implement mock behaviors inbeforeEachblocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/cli-storybook/src/codemod/csf-factories.test.ts` around lines 14 - 26, Change the inline vi.mock factory implementations to use spy: true and move the mock behaviors into a beforeEach setup: call vi.mock(...) with { spy: true } for the modules currently mocked (the module exporting runCodemod, './helpers/story-to-csf-factory' exporting storyToCsfFactory, './helpers/config-to-csf-factory' exporting configToCsfFactory, and 'storybook/internal/common' exposing syncStorybookAddons), then in beforeEach set expectations with the mocked functions (e.g., vi.mocked(runCodemod).mockImplementation / .mockResolvedValue, vi.mocked(storyToCsfFactory).mockResolvedValue('transformed story'), vi.mocked(configToCsfFactory).mockResolvedValue('transformed config'), and vi.mocked(syncStorybookAddons).mockImplementation) so mocks follow the spy pattern and behaviors are initialized per-test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/lib/cli-storybook/src/codemod/csf-factories.test.ts`:
- Around line 14-26: Change the inline vi.mock factory implementations to use
spy: true and move the mock behaviors into a beforeEach setup: call vi.mock(...)
with { spy: true } for the modules currently mocked (the module exporting
runCodemod, './helpers/story-to-csf-factory' exporting storyToCsfFactory,
'./helpers/config-to-csf-factory' exporting configToCsfFactory, and
'storybook/internal/common' exposing syncStorybookAddons), then in beforeEach
set expectations with the mocked functions (e.g.,
vi.mocked(runCodemod).mockImplementation / .mockResolvedValue,
vi.mocked(storyToCsfFactory).mockResolvedValue('transformed story'),
vi.mocked(configToCsfFactory).mockResolvedValue('transformed config'), and
vi.mocked(syncStorybookAddons).mockImplementation) so mocks follow the spy
pattern and behaviors are initialized per-test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 559a3ffb-cf46-425b-8592-a9cfae7f1af3
📒 Files selected for processing (2)
code/lib/cli-storybook/src/codemod/csf-factories.test.tscode/lib/cli-storybook/src/codemod/csf-factories.ts
This comment was marked as low quality.
This comment was marked as low quality.
Contributor
|
Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the fix works, optimally with before-and-after comparisons. Thank you for your understanding! |
This comment was marked as low quality.
This comment was marked as low quality.
Contributor
|
Closing due to purely AI-generated content. |
github-project-automation
Bot
moved this from Human verification
to Done
in Core Team Projects
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
#*) with a scoped#storybook/*import target based on the resolved Storybook config directory.#storybook/<preview filename>with the preview file extension when subpath imports are selected.Fixes #32935.
Testing
node .yarn\releases\yarn-4.10.3.cjs install --immutablenode .yarn\releases\yarn-4.10.3.cjs nx run-many -t compile -p core cli$env:CI='true'; node .yarn\releases\yarn-4.10.3.cjs vitest run code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts code/lib/cli-storybook/src/codemod/csf-factories.test.ts code/core/src/csf-tools/CsfFile.test.tsnode .yarn\releases\yarn-4.10.3.cjs exec oxfmt --check code/lib/cli-storybook/src/codemod/csf-factories.ts code/lib/cli-storybook/src/codemod/csf-factories.test.ts code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts code/core/src/csf-tools/CsfFile.test.tsnode .yarn\releases\yarn-4.10.3.cjs nx run-many -t check -p core cliManual testing
Not run. This changes the CSF factories automigration path generation and package.json imports map logic, covered by targeted unit tests.
Summary by CodeRabbit
New Features
Tests