feat(core): propagate inspection paths through React component roots#513
Merged
Conversation
Review Summary by QodoPropagate inspection paths through React component roots
WalkthroughsDescription• Propagate inspection paths from component call sites through React component roots • Support dynamic path injection for function and class components with prop propagation • Handle createElement() and createPortal() calls with root target collection • Resolve root render targets through conditionals, logical expressions, arrays, fragments, and identifiers • Expand test coverage with 100+ new test cases for component detection and path injection Diagramflowchart LR
A["Component Detection<br/>Functions & Classes"] -->|"Identify component roots"| B["Root Target Collection<br/>JSX/createElement/Portals"]
B -->|"Resolve through expressions"| C["Dynamic Path Injection<br/>Props Propagation"]
C -->|"Inject attributes"| D["Inspection Path Attributes<br/>Call site or definition fallback"]
E["Static JSX Elements"] -->|"Direct injection"| D
File Changes1. packages/core/src/server/transform/transform-jsx.ts
|
Code Review by Qodo
1. Null props assign crash
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 23 23
Lines 2821 3561 +740
Branches 695 927 +232
==========================================
+ Hits 2821 3561 +740 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+775
to
+810
| const propsArgument = node.arguments?.[1]; | ||
| if (hasCreateElementPathProperty(propsArgument)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!propsArgument) { | ||
| const typeArgument = node.arguments?.[0]; | ||
| if (!typeArgument?.end) { | ||
| return; | ||
| } | ||
| s.appendLeft( | ||
| typeArgument.end, | ||
| `, { ${JSON.stringify(PathName)}: ${pathExpression} }`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (propsArgument.type === 'ObjectExpression') { | ||
| s.prependLeft( | ||
| propsArgument.start + 1, | ||
| `${JSON.stringify(PathName)}: ${pathExpression}${ | ||
| propsArgument.properties.length ? ', ' : '' | ||
| }`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| s.overwrite( | ||
| propsArgument.start, | ||
| propsArgument.end, | ||
| `Object.assign({}, ${content.slice( | ||
| propsArgument.start, | ||
| propsArgument.end, | ||
| )}, { ${JSON.stringify(PathName)}: ${pathExpression} })`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
1. Null props assign crash 🐞 Bug ≡ Correctness
injectCreateElementPath rewrites any non-object props argument into `Object.assign({}, <props>,
{"data-insp-path": ...}), which will throw at runtime when the source is null/undefined` (e.g.
React.createElement("div", null, ...)). This affects both component-root and non-root
createElement calls because the transform runs on every CallExpression it recognizes as
createElement.
Agent Prompt
## Issue description
`injectCreateElementPath()` rewrites non-object `props` args as `Object.assign({}, <props>, {"data-insp-path": ...})`. If `<props>` is `null` or `undefined` (very common: `React.createElement("div", null, ...)`), the transformed code throws `TypeError: Cannot convert undefined or null to object`.
## Issue Context
This transform runs broadly over JS/TS files (via toolchain plugins) and now rewrites createElement calls, so this can crash real apps that use `createElement(..., null, ...)`.
## Fix Focus Areas
- packages/core/src/server/transform/transform-jsx.ts[769-810]
## Fix guidance
- Special-case `NullLiteral` (and optionally an `Identifier` named `undefined`) as “no props”: replace the argument with a new object literal containing the path property.
- For general non-object expressions, avoid null/undefined crashes by wrapping the expression with a nullish fallback, e.g. `Object.assign({}, (${orig} ?? {}), { ... })` (or equivalent) so runtime null/undefined do not throw.
- Add/extend a unit test that asserts `React.createElement("div", null, ...)` is transformed into non-throwing code (no `Object.assign({}, null, ...)`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+124
to
133
| if (node.type === 'CallExpression') { | ||
| injectStaticCreateElementPath( | ||
| node, | ||
| s, | ||
| content, | ||
| filePath, | ||
| dynamicInjectedCreateElementCalls, | ||
| escapeTags, | ||
| ); | ||
| } |
There was a problem hiding this comment.
2. Non-react createelement rewritten 🐞 Bug ≡ Correctness
The transform treats any callee whose name/property is createElement as React createElement, so
calls like document.createElement('div') will also be rewritten with injected
props/Object.assign wrappers. Because the Vite/Esbuild integrations classify all JS/TS files as
fileType='jsx', this change can mutate non-React code across a project.
Agent Prompt
## Issue description
The transform currently rewrites any `.createElement(...)` call (or `createElement(...)`) based solely on the callee name/property. This will incorrectly rewrite non-React APIs such as `document.createElement(...)`, potentially changing runtime behavior and, combined with the props-injection implementation, can even introduce runtime throws.
## Issue Context
Tooling integrations (e.g., Vite/Esbuild) run the JSX transform over *all* JS/TS files (not just files containing JSX), so the false-positive surface is large.
## Fix Focus Areas
- packages/core/src/server/transform/transform-jsx.ts[124-133]
- packages/core/src/server/transform/transform-jsx.ts[851-885]
- packages/vite/src/index.ts[120-130]
- packages/esbuild/src/index.ts[70-85]
## Fix guidance
- Tighten `isCreateElementCall`/`isCreatePortalCall` detection. Options include:
- Only match `React.createElement` / `ReactDOM.createPortal` (MemberExpression where object resolves to `React`/`ReactDOM`).
- Allow bare `createElement`/`createPortal` only if the identifier is bound to a known React import in the current file (inspect `path.scope.getBinding()` and its import source).
- Explicitly exclude DOM patterns like `document.createElement` by checking the member expression object.
- Add regression tests demonstrating that `document.createElement('div')` is NOT rewritten by `transformJsx`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Thank you @zh-lx, you are the best. |
Owner
Author
Yor're welcome, also thanks for your feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #505
Summary
This PR improves JSX transformation for React-style components so root nodes
can preserve and propagate
data-insp-pathfrom the component call siteinstead of always falling back to the component definition location.
What changed
data-insp-pathhandling for component root JSX elementscreateElement(...)andcreatePortal(...)returnsarrays, fragments, identifiers, and assignments
test coverage
Tests
transform-jsxcoverage across component detection, root targetcollection,
createElementinjection, class components, export-defaultanonymous components, and edge-case fallbacks