-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
React: Use self-closing tag for code snippets #33342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React: Use self-closing tag for code snippets #33342
Conversation
|
View your CI Pipeline Execution ↗ for commit 883720a
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughMake JSX snippet generation emit self-closing elements when there are no children, precompute JSX children into a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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📓 Path-based instructions (8)**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
Files:
**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-28T14:50:24.889ZApplied to files:
📚 Learning: 2025-09-24T09:39:39.233ZApplied to files:
📚 Learning: 2025-11-05T09:38:47.712ZApplied to files:
⏰ 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)
🔇 Additional comments (6)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (1)
53-66: Prefer reusingwithCSF3()/withCSF4()helpers (and consider aligning test name vs CSF version).This test inlines CSF boilerplate that the file already centralizes in helpers, and despite the PR description mentioning CSF4 it’s currently exercising the CSF3-style default export meta. Consider rewriting via
withCSF3()(or switching towithCSF4()if that’s the intended coverage) to reduce duplication and keep intent crisp.code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
225-234: Self-closing synthesis looks correct; consider treatingchildren: ''as “no children” (if desired).Current logic won’t self-close when args set
childrento an empty string, becausetoJsxChildren()produces aJSXText(''). If you want<Button />for that case too, one low-risk place to encode it istoJsxChildren():const toJsxChildren = (node: t.Node | null | undefined) => !node ? [] : t.isStringLiteral(node) - ? [t.jsxText(node.value)] + ? node.value === '' + ? [] + : [t.jsxText(node.value)] : t.isJSXElement(node) || t.isJSXFragment(node) ? [node] : t.isExpression(node) ? [t.jsxExpressionContainer(node)] : [];(Only do this if product expectation is that empty-string children should render self-closing.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx(1 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
🧬 Code graph analysis (1)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (1)
scripts/utils/tools.ts (1)
dedent(119-119)
⏰ 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: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 128 | 🚨 +1 🚨 |
| Self size | 1.12 MB | 1.12 MB | 0 B |
| Dependency size | 21.97 MB | 21.96 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 159 | 160 | 🚨 +1 🚨 |
| Self size | 30 KB | 30 KB | 🚨 +18 B 🚨 |
| Dependency size | 23.15 MB | 23.14 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 117 | 118 | 🚨 +1 🚨 |
| Self size | 35 KB | 35 KB | 🎉 -1 B 🎉 |
| Dependency size | 19.76 MB | 19.75 MB | 🎉 -11 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
Closes #
What I did
Added self-closing fallback rendering for synthesized snippets with no children in React code snippets, and added a regression test verifying the no-children case now outputs .
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!
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
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.