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 35cf95a
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughRewrote getUsedProps in mount-utils.ts to parse destructured properties from a function's source (inline params or first-body destructuring) with a new splitByComma helper and stricter unknown types; expanded tests with fixtures covering multiple destructuring forms and updated snapshots. (46 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ 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.
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts
Outdated
Show resolved
Hide resolved
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? |
| const LateDestructuring = { | ||
| play: async (a: any) => { | ||
| console.log(a); | ||
| const { | ||
| mount, | ||
| veryLongTranspiledDefinitionnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn, | ||
| over, | ||
| multiple, | ||
| lines, | ||
| } = a; | ||
| await mount(); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Hopefully I'm understanding the code correctly, if not my comment might not make sense:
You're testing a 2 happy paths of your code:
- the old methodology, where de-structure happens in the function arguments
- the new, where de-structuring happens on the first line, I suppose this is what SWC outputs when the
bugfixesis turned off?
Then you test a single "non-happy" path, by adding a console.log-call above the de-structure, and your test correctly shows, it's not detected.
There are cases possible where the code can get "confused", and get triggered in an happy path incorrectly, or potentially crash?
Here are some scenarios, that would be worth having a test for:
// I'd expect this to return ['mount']
play: async (context: any) => {
const {
mount, // a comment
} = context;
await mount();
},
// No trailing comma! I'd expect this to still return ['mount']
play: async (context: any) => {
const {
mount // a comment
} = context;
await mount();
},
// I'd expect this to return ['mount']
play: async (context: any) => {
const {
// a comment
mount,
} = context;
await mount();
},
// I'd expect this to return [], even though "mount" is destructured, it's not from the context
play: async (context: any) => {
const { mount } = await import('unrelated-module');
const {
mount: sbMount,
} = context;
await sbMount();
},
// I'm unsure what to expect, but the preferred return is ['mount']
play: async (context: any) => {
const {
mount: sbMount,
} = context;
await sbMount();
},
I don't know if these would work correctly with the implementation,
I understand that there's only so much we can do with a stringified fn and regexp.
Perhaps we should add a bit more resilience to more failure cases, and I suggest adding a try-catch block around getUsedProps. That is unless any thrown errors would already be handled further up the code chain, which I'm not too familiar with.
There was a problem hiding this comment.
Comments are disregarded, they don't appear in the code we're operating on.
There was a problem hiding this comment.
I agree, I'm fairly sure that by default comments are discarded.
But, code of stories files are bundled by the preview builders, which are configurable, I think we can't make the assumption that comments will always be removed.
Can you point me to some code that guarantees they will be discarded irregardless of the preview builder configuration?
If the point on code-comments I made above is not an issue, was not my only example.
The code can produce false-positives, false-negatives as well as throw errors.
My key suggestion is to add resilience with a try-catch (unless this makes no sense), and add more tests.
|
@ndelangen I have not verified this, but we obviously should. It would be ideal if we'd no longer need the |
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
✏️ Tip: You can customize this high-level summary in your review settings.