Draft
Conversation
Contributor
|
ndelangen
added
maintenance
|
View your CI Pipeline Execution ↗ for commit b818039
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive React Native Storybook support, including entrypoint file generation, platform-specific npm script derivation for iOS and Android, automated Metro configuration patching with AST transformation and fallback mechanisms, and integration into the React Native generator module with coordinated orchestration and messaging. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
✨ Finishing Touches📝 Generate docstrings
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: 3
🧹 Nitpick comments (2)
code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.test.ts (1)
5-48: Consider adding edge case tests.The current tests cover the main scenarios well. Consider adding tests for edge cases like
undefinedscripts, empty string values, and non-string script values (e.g.,{ ios: 123 }), which the implementation handles but aren't explicitly tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.test.ts` around lines 5 - 48, Add edge-case unit tests for deriveStorybookPlatformScripts: add cases where input scripts contain undefined values (e.g., { ios: undefined }), empty strings ({ android: '' }), and non-string values ({ ios: 123 }) and assert the function treats those as missing base scripts (appear in missingBaseScripts) and does not add corresponding storybook:* entries to scriptsToAdd; also confirm the function does not mutate the original input object (compare against a snapshot) and that scriptsToAdd only contains entries for valid string commands. Use deriveStorybookPlatformScripts, scriptsToAdd, and missingBaseScripts in the new tests to mirror existing test style.code/lib/create-storybook/src/generators/REACT_NATIVE/index.test.ts (1)
99-120: Consider making the output assertions more robust.The test relies on checking the last logged message via
mock.calls.at(-1). If additional log calls are added in the future, this could become fragile.Consider using
toHaveBeenCalledWith(expect.stringContaining(...))or iterating over all calls to find relevant messages:const allLogs = vi.mocked(logger.log).mock.calls.map(c => String(c[0])); expect(allLogs.some(log => log.includes('npm run storybook:ios'))).toBe(true);That said, testing the final output message is reasonable for verifying user-facing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/create-storybook/src/generators/REACT_NATIVE/index.test.ts` around lines 99 - 120, The test "postConfigure removes legacy entry replacement copy" is brittle because it asserts on the last logger.log call via vi.mocked(logger.log).mock.calls.at(-1); update the assertions to inspect all logger.log calls instead: map vi.mocked(logger.log).mock.calls to strings (or use expect(vi.mocked(logger.log)).toHaveBeenCalledWith(expect.stringContaining(...))) and assert that some log includes 'npm run storybook:ios' and 'npm run storybook:android' and that none include 'Replace the contents of your app entry'; update the assertions around logger.log in this test and keep the rest of the test (including invoking reactNativeGenerator.configure/postConfigure) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.ts`:
- Around line 8-12: The generated Storybook scripts use the STORYBOOK_ENV_PREFIX
(which includes "cross-env") via withStorybookEnv, but the configure function in
REACT_NATIVE/index.ts never adds cross-env to devDependencies; update the
configure function to add "cross-env" to the devDependencies/packages list
that's installed for React Native projects so the generated scripts will run.
Locate the configure function in REACT_NATIVE/index.ts and include "cross-env"
alongside the other dev packages added during setup (ensure it's added as a
devDependency, not a runtime dependency).
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/index.ts`:
- Around line 19-20: The module-level variables lastMetroCodemodResult and
lastScriptDerivationResult can leak state between generator runs; update the
implementation so state is not stored at module scope—either clear these
variables at the start of configure or move them into per-run storage (e.g.,
capture them in a closure/class instance or return them from configure for
postConfigure to consume). Concretely, remove reliance on the module-level
lastMetroCodemodResult/lastScriptDerivationResult and ensure configure
initializes fresh per-run state (or returns a state object) that postConfigure
uses, updating references in configure and postConfigure accordingly (symbols:
lastMetroCodemodResult, lastScriptDerivationResult, configure, postConfigure,
GeneratorModule).
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/metroConfig.ts`:
- Around line 8-14: Replace the placeholder constants that will be exposed to
users: update METRO_SETUP_DOCS_LINK to point to the real React Native Metro docs
URL and replace EXPO_CREATE_METRO_COMMAND (the command and args in the
EXPO_CREATE_METRO_COMMAND constant) with the actual Expo CLI command and
arguments used to generate a metro config; ensure the values are accurate,
user-friendly, and tested (references: METRO_SETUP_DOCS_LINK and
EXPO_CREATE_METRO_COMMAND in this file), and keep METRO_CONFIG_CANDIDATES and
METRO_FALLBACK_COMMENT_MARKER unchanged unless you need to add another candidate
or alter the marker text for clarity.
---
Nitpick comments:
In
`@code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.test.ts`:
- Around line 5-48: Add edge-case unit tests for deriveStorybookPlatformScripts:
add cases where input scripts contain undefined values (e.g., { ios: undefined
}), empty strings ({ android: '' }), and non-string values ({ ios: 123 }) and
assert the function treats those as missing base scripts (appear in
missingBaseScripts) and does not add corresponding storybook:* entries to
scriptsToAdd; also confirm the function does not mutate the original input
object (compare against a snapshot) and that scriptsToAdd only contains entries
for valid string commands. Use deriveStorybookPlatformScripts, scriptsToAdd, and
missingBaseScripts in the new tests to mirror existing test style.
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/index.test.ts`:
- Around line 99-120: The test "postConfigure removes legacy entry replacement
copy" is brittle because it asserts on the last logger.log call via
vi.mocked(logger.log).mock.calls.at(-1); update the assertions to inspect all
logger.log calls instead: map vi.mocked(logger.log).mock.calls to strings (or
use
expect(vi.mocked(logger.log)).toHaveBeenCalledWith(expect.stringContaining(...)))
and assert that some log includes 'npm run storybook:ios' and 'npm run
storybook:android' and that none include 'Replace the contents of your app
entry'; update the assertions around logger.log in this test and keep the rest
of the test (including invoking reactNativeGenerator.configure/postConfigure)
unchanged.
🪄 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: 6cf731fa-ff73-4e11-a696-0e40bc249414
📒 Files selected for processing (9)
code/lib/create-storybook/src/generators/REACT_NATIVE/generateEntrypoint.test.tscode/lib/create-storybook/src/generators/REACT_NATIVE/generateEntrypoint.tscode/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.test.tscode/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.tscode/lib/create-storybook/src/generators/REACT_NATIVE/index.test.tscode/lib/create-storybook/src/generators/REACT_NATIVE/index.tscode/lib/create-storybook/src/generators/REACT_NATIVE/metroConfig.test.tscode/lib/create-storybook/src/generators/REACT_NATIVE/metroConfig.tscode/lib/create-storybook/src/generators/REACT_NATIVE/templates/index.js
Comment on lines
+8
to
+12
| const STORYBOOK_ENV_PREFIX = 'cross-env STORYBOOK_ENABLED=true'; | ||
|
|
||
| const withStorybookEnv = (scriptValue: string) => { | ||
| return `${STORYBOOK_ENV_PREFIX} ${scriptValue}`.trim(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if cross-env is added as a dependency during RN Storybook setup
rg -n 'cross-env' --type=ts -g '!*.test.*' code/lib/create-storybook/Repository: storybookjs/storybook
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Read the full generateScripts.ts file to understand context
cat -n code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.ts | head -60Repository: storybookjs/storybook
Length of output: 1514
🏁 Script executed:
#!/bin/bash
# Search for where cross-env might be added as a dependency
rg -n 'cross-env' code/lib/create-storybook/ -t ts -t js -A 3 -B 3Repository: storybookjs/storybook
Length of output: 5207
🏁 Script executed:
#!/bin/bash
# Check if cross-env is in any package.json in create-storybook
find code/lib/create-storybook -name 'package.json' -exec grep -l 'cross-env' {} \;Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check core package.json to see if cross-env is a dependency
grep -n 'cross-env' code/core/package.json || echo "Not found in core package.json"Repository: storybookjs/storybook
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read the REACT_NATIVE generator index file to see if cross-env is added
cat -n code/lib/create-storybook/src/generators/REACT_NATIVE/index.ts | head -100Repository: storybookjs/storybook
Length of output: 4402
🏁 Script executed:
#!/bin/bash
# Search for addDependency or addDevDependency calls in REACT_NATIVE
rg -n 'addDependency|addDevDependency|installDependencies' code/lib/create-storybook/src/generators/REACT_NATIVE/ -A 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check other generators to see the pattern for adding dependencies
rg -n 'addDependency.*cross-env|cross-env.*Dependency' code/lib/create-storybook/src/generators/ -t tsRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for cross-env in template files that might be copied
find code/lib/create-storybook/src/templates -name '*.json' -o -name '*.md' | xargs grep -l 'cross-env' 2>/dev/null || echo "No cross-env found in templates"Repository: storybookjs/storybook
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Check if react-native has cross-env as a transitive dependency
rg -n 'cross-env' code/core/package.json code/lib/create-storybook/package.json 2>/dev/null || echo "Not in primary package files"Repository: storybookjs/storybook
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Look at what packages are in the REACT_NATIVE templates that get copied
ls -la code/lib/create-storybook/src/templates/REACT_NATIVE/ 2>/dev/null | head -20Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about cross-env in React Native files
rg -n 'cross-env' code/lib/create-storybook/src/generators/REACT_NATIVE/ -B 2 -A 2 --type ts --type mdRepository: storybookjs/storybook
Length of output: 4003
🏁 Script executed:
#!/bin/bash
# Check if there are any setup instructions or documentation
find code/lib/create-storybook -name 'README*' -o -name 'SETUP*' | xargs cat 2>/dev/null | head -50Repository: storybookjs/storybook
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Look at index.test.ts to see if there are hints about expected package.json
cat -n code/lib/create-storybook/src/generators/REACT_NATIVE/index.test.ts | head -80Repository: storybookjs/storybook
Length of output: 3646
Add cross-env as a devDependency in React Native setup.
The generated scripts for storybook:ios and storybook:android reference cross-env on line 8, but the dependency is never installed. Update the configure function in REACT_NATIVE/index.ts to include cross-env in the packages added to the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/generateScripts.ts`
around lines 8 - 12, The generated Storybook scripts use the
STORYBOOK_ENV_PREFIX (which includes "cross-env") via withStorybookEnv, but the
configure function in REACT_NATIVE/index.ts never adds cross-env to
devDependencies; update the configure function to add "cross-env" to the
devDependencies/packages list that's installed for React Native projects so the
generated scripts will run. Locate the configure function in
REACT_NATIVE/index.ts and include "cross-env" alongside the other dev packages
added during setup (ensure it's added as a devDependency, not a runtime
dependency).
Comment on lines
+19
to
+20
| let lastMetroCodemodResult: MetroCodemodResult | undefined; | ||
| let lastScriptDerivationResult: StorybookPlatformScriptDerivationResult | undefined; |
Contributor
There was a problem hiding this comment.
Module-level state may leak between generator invocations.
lastMetroCodemodResult and lastScriptDerivationResult persist across calls. While configure and postConfigure are called as a pair per execution, if the module is reused without cache clearing (e.g., in tests or multiple sequential init commands in the same process), stale state from a previous run could affect subsequent runs. Consider passing state through the generator's return value or using a closure/class instance instead.
Alternative: Return state from configure for postConfigure consumption
One approach is to store the state in a property that configure returns and postConfigure receives, though this requires changes to the GeneratorModule interface. A simpler fix is to clear the state at the start of configure:
let lastMetroCodemodResult: MetroCodemodResult | undefined;
let lastScriptDerivationResult: StorybookPlatformScriptDerivationResult | undefined;
export default defineGeneratorModule({
// ...
configure: async (packageManager, context) => {
+ // Reset state from any previous run
+ lastMetroCodemodResult = undefined;
+ lastScriptDerivationResult = undefined;
+
const missingReactDom = !packageManager.getDependencyVersion('react-dom');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/index.ts` around lines
19 - 20, The module-level variables lastMetroCodemodResult and
lastScriptDerivationResult can leak state between generator runs; update the
implementation so state is not stored at module scope—either clear these
variables at the start of configure or move them into per-run storage (e.g.,
capture them in a closure/class instance or return them from configure for
postConfigure to consume). Concretely, remove reliance on the module-level
lastMetroCodemodResult/lastScriptDerivationResult and ensure configure
initializes fresh per-run state (or returns a state object) that postConfigure
uses, updating references in configure and postConfigure accordingly (symbols:
lastMetroCodemodResult, lastScriptDerivationResult, configure, postConfigure,
GeneratorModule).
Comment on lines
+8
to
+14
| export const METRO_CONFIG_CANDIDATES = ['metro.config.ts', 'metro.config.js', 'metro.config.cjs']; | ||
| export const METRO_SETUP_DOCS_LINK = 'TODO_REPLACE_WITH_REACT_NATIVE_METRO_DOCS_LINK'; | ||
| export const METRO_FALLBACK_COMMENT_MARKER = 'storybook-react-native-metro-codemod-fallback'; | ||
| export const EXPO_CREATE_METRO_COMMAND = { | ||
| command: 'TODO_REPLACE_WITH_EXPO_METRO_CREATE_COMMAND', | ||
| args: [] as string[], | ||
| } as const; |
Contributor
There was a problem hiding this comment.
TODO placeholders need resolution before release.
METRO_SETUP_DOCS_LINK and EXPO_CREATE_METRO_COMMAND contain placeholder values that will appear in user-facing output. These should be replaced with actual documentation links and commands before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/create-storybook/src/generators/REACT_NATIVE/metroConfig.ts` around
lines 8 - 14, Replace the placeholder constants that will be exposed to users:
update METRO_SETUP_DOCS_LINK to point to the real React Native Metro docs URL
and replace EXPO_CREATE_METRO_COMMAND (the command and args in the
EXPO_CREATE_METRO_COMMAND constant) with the actual Expo CLI command and
arguments used to generate a metro config; ensure the values are accurate,
user-friendly, and tested (references: METRO_SETUP_DOCS_LINK and
EXPO_CREATE_METRO_COMMAND in this file), and keep METRO_CONFIG_CANDIDATES and
METRO_FALLBACK_COMMENT_MARKER unchanged unless you need to add another candidate
or alter the marker text for clarity.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.47 MB | 20.46 MB | 🎉 -12 KB 🎉 |
| Dependency size | 16.55 MB | 16.55 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -36 B 🎉 |
| Dependency size | 22.76 MB | 22.73 MB | 🎉 -33 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 121 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.83 MB | 23.80 MB | 🎉 -32 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -18 B 🎉 |
| Dependency size | 20.55 MB | 20.51 MB | 🎉 -33 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 66.22 MB | 66.21 MB | 🎉 -12 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.06 MB | 🚨 +14 KB 🚨 |
| Dependency size | 37.03 MB | 37.01 MB | 🎉 -12 KB 🎉 |
| Bundle Size Analyzer | node | node |
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
1 participant
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.
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 pull request has been released as version
0.0.0-pr-34333-sha-6f6a0f1a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34333-sha-6f6a0f1a sandboxor in an existing project withnpx storybook@0.0.0-pr-34333-sha-6f6a0f1a upgrade.More information
0.0.0-pr-34333-sha-6f6a0f1anorbert/rn-init-overhaul6f6a0f1a1774521256)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34333Summary by CodeRabbit
New Features
.rnstorybookdirectoryTests