Core: Fix play function mount detection when destructuring in the function body#33367
Conversation
|
Note this should run |
|
View your CI Pipeline Execution ↗ for commit 7311877
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughRewrote getUsedProps to parse destructured property names from a function's source (inline params, first-body destructuring, or const/let/var destructuring), added splitByComma and comment-stripping helpers, tightened types to unknown; expanded tests with multiple destructuring fixtures and updated snapshots. (35 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (2)
30-31: EscapefirstArgbefore using it inRegExpto prevent potential ReDoS.The static analysis correctly identifies that
firstArgis interpolated directly into a regex without escaping. While in practicefirstArgshould be a valid JS identifier, defensive coding would escape regex metacharacters.+function escapeRegExp(s: string) { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + const [, destructuredArg] = - body?.trim()?.match(new RegExp(`^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${firstArg};`)) || []; + body?.trim()?.match(new RegExp(`^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapeRegExp(firstArg)};`)) || [];
48-71: Consider handling parentheses insplitByCommafor robustness.The function handles
{and[but not(. This could cause incorrect splitting for destructured patterns with function-call defaults like{ mount, foo = someFunc(1, 2) }, where the comma inside the parentheses would be treated as a top-level separator.Given that the result is post-processed with
.replace(/:.*|=.*/g, '')to strip defaults, the impact may be limited, but it could still produce unexpected tokens.function splitByComma(s: string) { const result = []; const stack = []; let start = 0; for (let i = 0; i < s.length; i++) { - if (s[i] === '{' || s[i] === '[') { - stack.push(s[i] === '{' ? '}' : ']'); + if (s[i] === '{' || s[i] === '[' || s[i] === '(') { + stack.push(s[i] === '{' ? '}' : s[i] === '[' ? ']' : ')'); } else if (s[i] === stack[stack.length - 1]) { stack.pop(); } else if (!stack.length && s[i] === ',') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts(2 hunks)code/core/src/preview-api/modules/preview-web/render/mount-utils.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/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
**/*.{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/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
**/*.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/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
🧠 Learnings (10)
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
🧬 Code graph analysis (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts (2)
code/renderers/react/src/entry-preview.tsx (1)
mount(12-12)code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (1)
getUsedProps(14-37)
🪛 ast-grep (0.40.0)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[warning] 30-30: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${firstArg};)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[error] 15-15: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 25-25: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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). (5)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (3)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (1)
7-14: Good improvements to type safety and documentation.The signature change from
anytounknownand the added JSDoc improve maintainability.code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts (2)
35-72: Well-designed test fixtures covering key scenarios.The fixtures properly cover:
- Method shorthand syntax (
MethodProperty)- First-line body destructuring (
TranspiledDefinition)- Negative case where destructuring is not on the first line (
LateDestructuring)This aligns well with the PR objectives to support transpiled destructuring patterns.
109-121: Good coverage of the transpiled destructuring detection.The test correctly validates that:
TranspiledDefinition.play(first-line destructuring) returns the expected propsLateDestructuring.play(non-first-line destructuring) returns[]This matches the documented behavior that only first-line body destructuring is supported.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (1)
14-14: Good TypeScript improvement.Changing from
anytounknownaligns with strict mode best practices. For consistency, consider also updating line 3'smountDestructuredsignature to useunknowninstead ofany.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
🪛 ast-grep (0.40.0)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[warning] 34-34: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${firstArg};)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[error] 15-15: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 25-25: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (4)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (4)
7-13: Excellent documentation.The JSDoc clearly explains the function's behavior, including both inline and first-statement destructuring scenarios.
15-15: Regex works correctly despite linter warning.The
[^]pattern (negated empty character class) is valid JavaScript that matches any character including newlines. However, Biome flags it as an error. Consider using[\s\S]instead for clarity and to silence the linter, or add a Biome ignore comment if the current syntax is preferred.Note: This regex assumes the function string has matching parentheses and a single level of nesting. Complex cases (e.g., functions with parentheses in default parameter values) might not parse correctly, but this is likely acceptable given the controlled context of play functions.
Based on static analysis hints from Biome.
20-28: Inline destructuring extraction looks correct.The logic properly:
- Splits arguments by comma (handling nested structures)
- Detects inline destructuring pattern
{...}- Extracts property names while stripping renaming and default values
Same note as line 15: consider replacing
[^]with[\s\S]on line 25 to address the Biome warning.
43-76: Solid implementation with excellent documentation.The
splitByCommafunction correctly handles nested structures (braces and brackets) when splitting by top-level commas. The JSDoc clearly explains its purpose with a helpful example.Minor note: This implementation doesn't handle commas inside string literals or template literals (e.g.,
a, "b, c", d). However, this is acceptable for the current use case of parsing function parameters and destructuring patterns where string literals in these positions would be unusual.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
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/core/src/preview-api/modules/preview-web/render/mount-utils.ts
🪛 ast-grep (0.40.0)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[warning] 35-35: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[error] 15-15: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 25-25: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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
🔇 Additional comments (5)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (5)
7-13: Good documentation of the enhanced detection logic.The JSDoc clearly describes both inline and first-statement destructuring detection, which aligns with the PR objectives.
14-14: Type safety improvement aligns with strict mode guidelines.Changing from
anytounknownprovides better type safety while still allowing any function signature to be passed.
15-28: Inline destructuring parsing is correct; Biome warnings are false positives.The regex patterns using
[^]on lines 15 and 25 are flagged by Biome but are actually correct JavaScript syntax. The pattern[^]matches any character including newlines (unlike.which excludes newlines by default). This is an established idiom in JavaScript regular expressions.The logic correctly:
- Extracts function arguments and body
- Uses
splitByCommato handle nested structures- Strips property renaming (
:) and defaults (=)Note: The Biome linter warnings can be safely ignored as they represent false positives for this valid regex pattern.
30-32: Previous identifier validation concern has been addressed.The regex now correctly validates that
firstArgis a valid JavaScript identifier by ensuring it starts with a letter, underscore, or dollar sign, followed by any number of alphanumeric characters, underscores, or dollar signs. This addresses the incomplete validation flagged in the previous review.
44-77: Well-implemented helper with clear documentation.The
splitByCommafunction correctly handles nested structures using a stack-based approach to track brace and bracket depth. The implementation properly:
- Pushes opening brackets/braces onto the stack
- Pops matching closing brackets/braces
- Only splits on commas at depth 0
- Trims and filters empty tokens
The JSDoc clearly explains the purpose and behavior with a helpful example.
|
@ghengeveld The issue mentions I would expect Is performing this check a manual test that needs to be done during QA? |
|
@ndelangen I have not verified this, but we obviously should. It would be ideal if we'd no longer need the |
…e alternatively destructured syntax in our play-function parsing/detection of mount
|
I've checked the codebase for I removed the positive values, or changed the defaults to I'm also keen to hear @kasperpeulen's has feedback on this change. I did not do any manual local testing yet. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts (1)
35-209: Good fixture coverage for “moved destructuring” + comment variants
The added cases (method shorthand, first-statement destructuring, late destructuring negative, and comment stripping) look well-targeted and should prevent regressions.Avoid
console.login fixtures (use a no-op expression instead) (As per coding guidelines, avoid direct console usage.)Proposed diff
const LateDestructuring = { play: async (a: any) => { - console.log(a); + void a; const { mount, veryLongTranspiledDefinitionnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn, over, multiple, lines, } = a; await mount(); }, };
🧹 Nitpick comments (1)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (1)
48-95: Tighten helper typing (and consider comma-splitting in parentheses as a follow-up)With TS strictness, it’s better if these helpers are explicitly typed. Also,
splitByComma()currently ignores nested parentheses, so defaults likefoo = bar(a, b)can split incorrectly—might be fine for now, but worth considering if you see false positives/negatives.Proposed diff
function stripComments(s: string): string { // Remove single-line comments (// ...) s = s.replace(/\/\/.*$/gm, ''); // Remove multi-line comments (/* ... */) s = s.replace(/\/\*[\s\S]*?\*\//g, ''); return s; } +function splitByComma(s: string): string[] { - function splitByComma(s: string) { - const result = []; - const stack = []; + const result: string[] = []; + const stack: string[] = []; let start = 0; for (let i = 0; i < s.length; i++) { if (s[i] === '{' || s[i] === '[') { stack.push(s[i] === '{' ? '}' : ']'); } else if (s[i] === stack[stack.length - 1]) { stack.pop(); } else if (!stack.length && s[i] === ',') { const token = s.substring(start, i).trim(); if (token) { result.push(token); } start = i + 1; } } const lastToken = s.substring(start).trim(); if (lastToken) { result.push(lastToken); } return result; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/core-server/presets/common-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.tscode/frameworks/nextjs/src/babel/preset.tscode/frameworks/nextjs/src/preset.tscode/frameworks/nextjs/src/swc/next-swc-loader-patch.ts
💤 Files with no reviewable changes (4)
- code/core/src/core-server/presets/common-preset.ts
- code/frameworks/nextjs/src/babel/preset.ts
- code/frameworks/nextjs/src/swc/next-swc-loader-patch.ts
- code/frameworks/nextjs/src/preset.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/builders/builder-webpack5/src/presets/custom-webpack-preset.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.test.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{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/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
**/*.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/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
🧠 Learnings (14)
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Export functions from modules if they need to be tested
Applied to files:
code/core/src/preview-api/modules/preview-web/render/mount-utils.test.ts
🪛 ast-grep (0.40.4)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[warning] 37-37: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
[error] 15-15: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 25-25: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
⏰ 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
🔇 Additional comments (1)
code/builders/builder-webpack5/src/presets/custom-webpack-preset.ts (1)
16-29: Revert concern about regression—settingbugfixes: falseis intentional and conservativeThis change was deliberate (commit 7311877): bugfixes was disabled because Storybook improved its play-function parsing and no longer needs SWC's bugfix transforms. Setting
bugfixes: falseis more conservative (broader downleveling), not riskier. Safari 15 issues reported in the ecosystem were caused by SWC's minifier, not the bugfixes option. The change is safe for Storybook's supported targets and is already documented in the commit message. No further action needed.
| export function getUsedProps(fn: (...args: unknown[]) => unknown) { | ||
| const [, args, body] = fn.toString().match(/[^(]*\(([^)]+)\)(?:.*{([^]+)})?/) || []; | ||
| if (!args) { | ||
| return []; | ||
| } | ||
|
|
||
| const args = splitByComma(match[1]); | ||
|
|
||
| if (!args.length) { | ||
| const [firstArg] = splitByComma(args); | ||
| if (!firstArg) { | ||
| return []; | ||
| } | ||
|
|
||
| const first = args[0]; | ||
| const [, destructuredProps] = firstArg.match(/^{([^]+)}$/) || []; | ||
| if (destructuredProps) { | ||
| return splitByComma(stripComments(destructuredProps)).map((prop) => | ||
| prop.replace(/:.*|=.*/g, '').trim() | ||
| ); | ||
| } | ||
|
|
||
| if (!(first.startsWith('{') && first.endsWith('}'))) { | ||
| if (!firstArg.match(/^[a-z_$][0-9a-z_$]*$/i)) { | ||
| return []; | ||
| } | ||
|
|
||
| const props = splitByComma(first.slice(1, -1).replace(/\s/g, '')).map((prop) => { | ||
| return prop.replace(/:.*|=.*/g, ''); | ||
| }); | ||
| const escapedArg = firstArg.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const [, destructuredArg] = | ||
| body?.trim()?.match(new RegExp(`^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};`)) || []; | ||
| if (destructuredArg) { | ||
| return splitByComma(stripComments(destructuredArg)).map((prop) => | ||
| prop.replace(/:.*|=.*/g, '').trim() | ||
| ); | ||
| } | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
Fix Biome regex errors + make body-capture robust for single-line play functions
[^]+trips Biome (noEmptyCharacterClassInRegex) at Line 15 and Line 25.- Current body capture can fail when the entire function is on one line and contains additional
{}(e.g.,const { mount } = context;).
Proposed diff
-export function getUsedProps(fn: (...args: unknown[]) => unknown) {
- const [, args, body] = fn.toString().match(/[^(]*\(([^)]+)\)(?:.*{([^]+)})?/) || [];
+export function getUsedProps(fn: (...args: unknown[]) => unknown): string[] {
+ const src = fn.toString();
+ const [, args] = src.match(/^[^(]*\(([^)]*)\)/) || [];
+ const [, body] = src.match(/^[^(]*\([^)]*\)[^{]*{([\s\S]*)}\s*$/) || [];
if (!args) {
return [];
}
const [firstArg] = splitByComma(args);
if (!firstArg) {
return [];
}
- const [, destructuredProps] = firstArg.match(/^{([^]+)}$/) || [];
+ const [, destructuredProps] = firstArg.match(/^{([\s\S]*)}$/) || [];
if (destructuredProps) {
return splitByComma(stripComments(destructuredProps)).map((prop) =>
prop.replace(/:.*|=.*/g, '').trim()
);
}
if (!firstArg.match(/^[a-z_$][0-9a-z_$]*$/i)) {
return [];
}
const escapedArg = firstArg.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const [, destructuredArg] =
- body?.trim()?.match(new RegExp(`^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};`)) || [];
+ body?.trim()?.match(
+ new RegExp(`^(?:const|let|var)\\s*{([\\s\\S]*?)}\\s*=\\s*${escapedArg}\\s*;?`)
+ ) || [];
if (destructuredArg) {
return splitByComma(stripComments(destructuredArg)).map((prop) =>
prop.replace(/:.*|=.*/g, '').trim()
);
}
return [];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getUsedProps(fn: (...args: unknown[]) => unknown) { | |
| const [, args, body] = fn.toString().match(/[^(]*\(([^)]+)\)(?:.*{([^]+)})?/) || []; | |
| if (!args) { | |
| return []; | |
| } | |
| const args = splitByComma(match[1]); | |
| if (!args.length) { | |
| const [firstArg] = splitByComma(args); | |
| if (!firstArg) { | |
| return []; | |
| } | |
| const first = args[0]; | |
| const [, destructuredProps] = firstArg.match(/^{([^]+)}$/) || []; | |
| if (destructuredProps) { | |
| return splitByComma(stripComments(destructuredProps)).map((prop) => | |
| prop.replace(/:.*|=.*/g, '').trim() | |
| ); | |
| } | |
| if (!(first.startsWith('{') && first.endsWith('}'))) { | |
| if (!firstArg.match(/^[a-z_$][0-9a-z_$]*$/i)) { | |
| return []; | |
| } | |
| const props = splitByComma(first.slice(1, -1).replace(/\s/g, '')).map((prop) => { | |
| return prop.replace(/:.*|=.*/g, ''); | |
| }); | |
| const escapedArg = firstArg.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const [, destructuredArg] = | |
| body?.trim()?.match(new RegExp(`^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};`)) || []; | |
| if (destructuredArg) { | |
| return splitByComma(stripComments(destructuredArg)).map((prop) => | |
| prop.replace(/:.*|=.*/g, '').trim() | |
| ); | |
| } | |
| return []; | |
| } | |
| export function getUsedProps(fn: (...args: unknown[]) => unknown): string[] { | |
| const src = fn.toString(); | |
| const [, args] = src.match(/^[^(]*\(([^)]*)\)/) || []; | |
| const [, body] = src.match(/^[^(]*\([^)]*\)[^{]*{([\s\S]*)}\s*$/) || []; | |
| if (!args) { | |
| return []; | |
| } | |
| const [firstArg] = splitByComma(args); | |
| if (!firstArg) { | |
| return []; | |
| } | |
| const [, destructuredProps] = firstArg.match(/^{([\s\S]*)}$/) || []; | |
| if (destructuredProps) { | |
| return splitByComma(stripComments(destructuredProps)).map((prop) => | |
| prop.replace(/:.*|=.*/g, '').trim() | |
| ); | |
| } | |
| if (!firstArg.match(/^[a-z_$][0-9a-z_$]*$/i)) { | |
| return []; | |
| } | |
| const escapedArg = firstArg.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const [, destructuredArg] = | |
| body?.trim()?.match( | |
| new RegExp(`^(?:const|let|var)\\s*{([\\s\\S]*?)}\\s*=\\s*${escapedArg}\\s*;?`) | |
| ) || []; | |
| if (destructuredArg) { | |
| return splitByComma(stripComments(destructuredArg)).map((prop) => | |
| prop.replace(/:.*|=.*/g, '').trim() | |
| ); | |
| } | |
| return []; | |
| } |
🧰 Tools
🪛 ast-grep (0.40.4)
[warning] 37-37: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:const|let|var)\\s*{([^}]+)}\\s*=\\s*${escapedArg};)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
[error] 15-15: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 25-25: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
|
I manually verified the behavior using a local sandbox in Manually adding a mount to the |
2 participants
Closes #33366
What I did
Besides looking for
mountin an inline-destructured play function argument, I added support for destructuringmounton the first line of the play function body.Previously only this was legal:
Now also this is legal:
This is still not legal (destructuring is required):
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
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.