refactor(devtools-vite): migrate from Babel to oxc-parser + MagicString#397
refactor(devtools-vite): migrate from Babel to oxc-parser + MagicString#397dake3601 wants to merge 4 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughThe devtools-vite package migrates from Babel-based AST transforms to an oxc-parser + MagicString pipeline. Core transformers, utilities, and tests were rewritten to operate on parsed oxc ASTs and string edits; package dependencies and documentation were updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/devtools/skills/devtools-production/SKILL.md (1)
34-39:⚠️ Potential issue | 🟡 MinorAlign stripped-package docs with current implementation.
This section says
@tanstack/preact-devtoolsis auto-stripped, butpackages/devtools-vite/src/remove-devtools.ts(Line 6-9 in provided snippet) currently matches only React, core devtools, and Solid imports. Please either add preact handling inremove-devtools.tsor remove preact from this list to avoid incorrect production guidance.Suggested doc-only correction (if implementation is intentional)
- `@tanstack/react-devtools` -- `@tanstack/preact-devtools` - `@tanstack/solid-devtools` - `@tanstack/devtools`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/skills/devtools-production/SKILL.md` around lines 34 - 39, The docs list includes `@tanstack/preact-devtools` but the implementation in remove-devtools.ts currently only matches React, core devtools, and Solid imports; either add preact handling to the matcher in remove-devtools.ts (e.g., include '@tanstack/preact-devtools' in the PACKAGES/regex used to detect imports and JSX removal in the remove logic) or remove preact from the SKILL.md list so docs reflect behavior—modify whichever side is intended so the list in SKILL.md and the import-matching array/regex in remove-devtools.ts stay in sync.
🧹 Nitpick comments (3)
packages/devtools-vite/src/inject-source.ts (1)
144-161: VariableDeclaration handling may miss deeply nested patterns.The code handles
const Foo = () => {}andconst Foo = function() {}but doesn't recursively check for more complex patterns likeconst Foo = memo(() => {})orconst Foo = forwardRef((props, ref) => {}). These wrapped components won't have source injection.This is a known limitation - supporting HOC-wrapped components would require more complex analysis. Consider documenting this limitation or adding a TODO for future enhancement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/inject-source.ts` around lines 144 - 161, VariableDeclaration handling currently only checks decl.init for ArrowFunctionExpression or FunctionExpression, so HOC-wrapped patterns like memo(() => {}) or forwardRef((props, ref) => {}) are missed; update the VariableDeclaration branch (where decl.init is inspected) to unwrap CallExpression wrappers by checking if decl.init.type === 'CallExpression' and then examine its arguments (recursively if needed) for ArrowFunctionExpression or FunctionExpression before calling processFunction, or if you don't want to implement unwrapping now, add a clear TODO comment and a short documentation note near processFunction/VariableDeclaration explaining this limitation and linking to a future task to implement recursive CallExpression unwrapping.packages/devtools-vite/src/remove-devtools.ts (1)
162-172: Import reconstruction may lose original formatting.When rebuilding import statements with remaining specifiers (line 171), the reconstruction creates a normalized format that may differ from the original (e.g., loses type imports, multiline formatting, or trailing commas). This is acceptable for production builds but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/remove-devtools.ts` around lines 162 - 172, The current overwrite reconstructs imports via `specTexts.join` which normalizes formatting and can drop `type` modifiers, trailing commas, and multiline layout; instead preserve the original import text by using the AST ranges and original code slices to only remove the ranges for deleted specifiers: compute the removal ranges from `node.specifiers` vs `remaining`, splice those ranges out of `code.slice(node.start, node.end)` (or use `s.remove` on each removed specifier range) and then `s.overwrite` or `s.appendRight` the resulting original-import string—this keeps `importKind`, whitespace, comments, trailing commas and multiline formatting intact while still removing the unwanted specifiers (refer to `remaining`, `specTexts`, `node`, and `s.overwrite` in the diff).packages/devtools-vite/src/enhance-logs.ts (1)
51-57: Parenthesis scan could fail on unusual formatting.The scan from
callee.endto find(works for typicalconsole.log(...)calls but could fail if there's a comment between the callee and the parenthesis (e.g.,console.log /* comment */ (...)). While rare in practice, consider using the CallExpression's argument span instead.🔧 Alternative approach using CallExpression structure
If oxc-parser provides the arguments array with position info, you could use:
- // Find the opening '(' of the call by scanning forward from callee end - let parenOffset = callee.end - while (parenOffset < code.length && code[parenOffset] !== '(') { - parenOffset++ - } - // Insert right after '(' - s.appendRight(parenOffset + 1, spreadStr) + // Insert at the start of the first argument, or right after callee if no args + const firstArg = node.arguments[0] + if (firstArg) { + s.appendRight(firstArg.start, spreadStr) + } else { + // No arguments: console.log() -> console.log(spreadStr) + s.appendRight(node.end - 1, spreadStr) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/enhance-logs.ts` around lines 51 - 57, The current scan from callee.end to find '(' (using parenOffset and s.appendRight) can fail with comments or weird whitespace between callee and the paren; instead compute the insertion point from the CallExpression node's arguments: if node.arguments.length > 0 use node.arguments[0].start to insert spreadStr before the first argument, otherwise fall back to using the CallExpression's end/span (e.g., search between callee.end and node.end for the '(' or insert just before node.end-1) so you avoid relying solely on callee.end and make the insertion robust; update references to parenOffset, callee.end, s.appendRight and spreadStr accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools-vite/package.json`:
- Around line 63-64: The dependency oxc-parser is pinned at ^0.72.0 and should
be reviewed and updated; open packages/devtools-vite/package.json, evaluate
compatibility of oxc-parser (symbol: "oxc-parser") against the current latest
(≈0.120.0), run the package upgrade (e.g., npm/yarn/pnpm) to a newer safe
version, run the test suite and local build to catch breaking changes, and if
you must keep 0.72.0 document the compatibility rationale beside the dependency
(or in CHANGELOG/README) so future reviewers know why it is pinned; also
consider aligning or noting magic-string (symbol: "magic-string") remains
current.
In `@packages/devtools-vite/src/inject-plugin.ts`:
- Around line 8-14: The devtools package list is duplicated and inconsistent
(see devtoolsPackages in inject-plugin.ts vs checks in remove-devtools.ts);
extract a single exported constant (e.g. TANSTACK_DEVTOOLS_PACKAGES) from a new
constants module and replace the local devtoolsPackages array in
inject-plugin.ts and any package checks in remove-devtools.ts to import and use
TANSTACK_DEVTOOLS_PACKAGES so both modules share the exact same list.
In `@packages/devtools-vite/src/offset-to-loc.ts`:
- Around line 9-37: The current createLocMapper uses JS string code-unit
indexing (charCodeAt/length) but callers pass oxc-parser byte offsets, causing
wrong positions for non-ASCII text; update the call sites (e.g., where
enhance-logs.ts calls createLocMapper) to convert oxc byte offsets to JS
code-unit indices before calling createLocMapper OR modify createLocMapper to
accept byte offsets by first mapping byte offset → code-unit index (decode UTF-8
byte positions into JS string indices) and then perform the existing binary
search; finally add a regression test that feeds source with multi-byte
characters (emoji/CJK) and verifies line/column results from createLocMapper
match expected positions to prevent regressions.
In `@packages/devtools-vite/src/remove-devtools.ts`:
- Around line 6-9: The function isTanStackDevtoolsImport currently only checks
'@tanstack/react-devtools', '@tanstack/devtools', and
'@tanstack/solid-devtools', so imports of '@tanstack/preact-devtools' and
'@tanstack/vue-devtools' are not detected and thus not removed; update
isTanStackDevtoolsImport to include '@tanstack/preact-devtools' and
'@tanstack/vue-devtools' (the same package names listed in devtoolPackages in
plugin.ts) so those imports are properly recognized and stripped during
production builds.
---
Outside diff comments:
In `@packages/devtools/skills/devtools-production/SKILL.md`:
- Around line 34-39: The docs list includes `@tanstack/preact-devtools` but the
implementation in remove-devtools.ts currently only matches React, core
devtools, and Solid imports; either add preact handling to the matcher in
remove-devtools.ts (e.g., include '@tanstack/preact-devtools' in the
PACKAGES/regex used to detect imports and JSX removal in the remove logic) or
remove preact from the SKILL.md list so docs reflect behavior—modify whichever
side is intended so the list in SKILL.md and the import-matching array/regex in
remove-devtools.ts stay in sync.
---
Nitpick comments:
In `@packages/devtools-vite/src/enhance-logs.ts`:
- Around line 51-57: The current scan from callee.end to find '(' (using
parenOffset and s.appendRight) can fail with comments or weird whitespace
between callee and the paren; instead compute the insertion point from the
CallExpression node's arguments: if node.arguments.length > 0 use
node.arguments[0].start to insert spreadStr before the first argument, otherwise
fall back to using the CallExpression's end/span (e.g., search between
callee.end and node.end for the '(' or insert just before node.end-1) so you
avoid relying solely on callee.end and make the insertion robust; update
references to parenOffset, callee.end, s.appendRight and spreadStr accordingly.
In `@packages/devtools-vite/src/inject-source.ts`:
- Around line 144-161: VariableDeclaration handling currently only checks
decl.init for ArrowFunctionExpression or FunctionExpression, so HOC-wrapped
patterns like memo(() => {}) or forwardRef((props, ref) => {}) are missed;
update the VariableDeclaration branch (where decl.init is inspected) to unwrap
CallExpression wrappers by checking if decl.init.type === 'CallExpression' and
then examine its arguments (recursively if needed) for ArrowFunctionExpression
or FunctionExpression before calling processFunction, or if you don't want to
implement unwrapping now, add a clear TODO comment and a short documentation
note near processFunction/VariableDeclaration explaining this limitation and
linking to a future task to implement recursive CallExpression unwrapping.
In `@packages/devtools-vite/src/remove-devtools.ts`:
- Around line 162-172: The current overwrite reconstructs imports via
`specTexts.join` which normalizes formatting and can drop `type` modifiers,
trailing commas, and multiline layout; instead preserve the original import text
by using the AST ranges and original code slices to only remove the ranges for
deleted specifiers: compute the removal ranges from `node.specifiers` vs
`remaining`, splice those ranges out of `code.slice(node.start, node.end)` (or
use `s.remove` on each removed specifier range) and then `s.overwrite` or
`s.appendRight` the resulting original-import string—this keeps `importKind`,
whitespace, comments, trailing commas and multiline formatting intact while
still removing the unwanted specifiers (refer to `remaining`, `specTexts`,
`node`, and `s.overwrite` in the diff).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efb58404-8540-40fe-a2a1-4521664de5be
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/common-bottles-move.md_artifacts/domain_map.yamldocs/architecture.mddocs/source-inspector.mddocs/vite-plugin.mdpackages/devtools-vite/package.jsonpackages/devtools-vite/skills/devtools-vite-plugin/SKILL.mdpackages/devtools-vite/skills/devtools-vite-plugin/references/vite-options.mdpackages/devtools-vite/src/ast-utils.tspackages/devtools-vite/src/babel.tspackages/devtools-vite/src/enhance-logs.tspackages/devtools-vite/src/inject-plugin.test.tspackages/devtools-vite/src/inject-plugin.tspackages/devtools-vite/src/inject-source.test.tspackages/devtools-vite/src/inject-source.tspackages/devtools-vite/src/offset-to-loc.tspackages/devtools-vite/src/remove-devtools.test.tspackages/devtools-vite/src/remove-devtools.tspackages/devtools/skills/devtools-marketplace/SKILL.mdpackages/devtools/skills/devtools-production/SKILL.md
💤 Files with no reviewable changes (1)
- packages/devtools-vite/src/babel.ts
| "magic-string": "^0.30.0", | ||
| "oxc-parser": "^0.72.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions and security advisories for new dependencies
echo "=== Checking magic-string ==="
curl -s https://registry.npmjs.org/magic-string/latest | jq '.version'
echo "=== Checking oxc-parser ==="
curl -s https://registry.npmjs.org/oxc-parser/latest | jq '.version'
# Check if 0.72.0 exists for oxc-parser
echo "=== Checking oxc-parser@0.72.0 exists ==="
curl -s https://registry.npmjs.org/oxc-parser/0.72.0 | jq '.version // "NOT FOUND"'Repository: TanStack/devtools
Length of output: 189
Consider updating oxc-parser to a more recent version.
While magic-string@^0.30.0 is current (latest is 0.30.21), oxc-parser@0.72.0 is significantly outdated; the latest version is 0.120.0. Check the changelog between 0.72.0 and a more recent release to ensure no critical fixes, breaking changes, or performance improvements are missed. If 0.72.0 is intentionally pinned for compatibility, document the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/package.json` around lines 63 - 64, The dependency
oxc-parser is pinned at ^0.72.0 and should be reviewed and updated; open
packages/devtools-vite/package.json, evaluate compatibility of oxc-parser
(symbol: "oxc-parser") against the current latest (≈0.120.0), run the package
upgrade (e.g., npm/yarn/pnpm) to a newer safe version, run the test suite and
local build to catch breaking changes, and if you must keep 0.72.0 document the
compatibility rationale beside the dependency (or in CHANGELOG/README) so future
reviewers know why it is pinned; also consider aligning or noting magic-string
(symbol: "magic-string") remains current.
| const devtoolsPackages = [ | ||
| '@tanstack/react-devtools', | ||
| '@tanstack/solid-devtools', | ||
| '@tanstack/vue-devtools', | ||
| '@tanstack/svelte-devtools', | ||
| '@tanstack/angular-devtools', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare devtools package lists across files
echo "=== inject-plugin.ts packages ==="
rg -A8 'devtoolsPackages\s*=' packages/devtools-vite/src/inject-plugin.ts
echo ""
echo "=== remove-devtools.ts packages ==="
rg -A5 'isTanStackDevtoolsImport' packages/devtools-vite/src/remove-devtools.ts
echo ""
echo "=== plugin.ts packages ==="
rg -A8 'devtoolPackages\s*=' packages/devtools-vite/src/plugin.tsRepository: TanStack/devtools
Length of output: 1230
Inconsistent devtools package handling across plugin files.
Different files define or check for different devtools packages: inject-plugin.ts includes svelte and angular (plus react, solid, vue), while remove-devtools.ts only checks for react, solid, and devtools. This inconsistency could cause issues if one file is updated to support a new framework but the other isn't. Extracting a shared constant would ensure consistency.
🛠️ Suggested approach: create shared constant
Create a shared constants file:
// packages/devtools-vite/src/constants.ts
export const TANSTACK_DEVTOOLS_PACKAGES = [
'@tanstack/react-devtools',
'@tanstack/preact-devtools',
'@tanstack/solid-devtools',
'@tanstack/vue-devtools',
'@tanstack/svelte-devtools',
'@tanstack/angular-devtools',
'@tanstack/devtools',
] as constThen import and use in both remove-devtools.ts and inject-plugin.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/inject-plugin.ts` around lines 8 - 14, The
devtools package list is duplicated and inconsistent (see devtoolsPackages in
inject-plugin.ts vs checks in remove-devtools.ts); extract a single exported
constant (e.g. TANSTACK_DEVTOOLS_PACKAGES) from a new constants module and
replace the local devtoolsPackages array in inject-plugin.ts and any package
checks in remove-devtools.ts to import and use TANSTACK_DEVTOOLS_PACKAGES so
both modules share the exact same list.
| * Pre-compute line start offsets for fast byte-offset to line/column conversion. | ||
| * Returns a function that maps a byte offset to `{ line, column }`. | ||
| * | ||
| * Build: O(n) single pass over the source string. | ||
| * Lookup: O(log n) binary search. | ||
| */ | ||
| export function createLocMapper(source: string): (offset: number) => Loc { | ||
| const lineStarts: Array<number> = [0] | ||
| for (let i = 0; i < source.length; i++) { | ||
| if (source.charCodeAt(i) === 10 /* \n */) { | ||
| lineStarts.push(i + 1) | ||
| } | ||
| } | ||
|
|
||
| return (offset: number): Loc => { | ||
| let lo = 0 | ||
| let hi = lineStarts.length - 1 | ||
| while (lo < hi) { | ||
| const mid = (lo + hi + 1) >> 1 | ||
| if (lineStarts[mid]! <= offset) { | ||
| lo = mid | ||
| } else { | ||
| hi = mid - 1 | ||
| } | ||
| } | ||
| return { | ||
| line: lo + 1, | ||
| column: offset - lineStarts[lo]!, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In oxc-parserv0.72.0 JavaScript bindings, areNode.startandNode.end measured as UTF-8 byte offsets or UTF-16 code-unit indices?
💡 Result:
In oxc-parser v0.72.0 JavaScript bindings, Node.start and Node.end are measured as UTF-8 byte offsets.
Citations:
- 1: parser,codegen: comment attachment and print all comments oxc-project/oxc#19671
- 2: feat(parser): utf16 spans oxc-project/oxc#959
- 3: https://crates.io/crates/oxc_span
- 4: AST nodes with loc and/or range fields. oxc-project/oxc#10307
- 5: unicode characters result in incorrect start/end values oxc-project/oxc#7508
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect call sites and offset sources.
rg -nP --type=ts -C3 '\bcreateLocMapper\s*\('
rg -nP --type=ts -C3 '\bnode\.start\b|\bnode\.end\b'Repository: TanStack/devtools
Length of output: 5877
Critical: offset-unit mismatch causes incorrect locations for non-ASCII source files.
The implementation uses UTF-16 code-unit indexing (via source.charCodeAt(), source.length, string offset arithmetic), but oxc-parser byte offsets are passed directly at call sites (e.g., enhance-logs.ts:41). For ASCII text this works by coincidence; for non-ASCII characters (emoji, multi-byte UTF-8), byte offsets and code-unit offsets diverge, producing wrong line/column mappings.
Convert oxc-parser byte offsets to JavaScript string indices before passing to createLocMapper, or add byte-to-code-unit conversion within the mapper itself. Add a regression test with non-ASCII characters (e.g., emoji, CJK) to prevent silent data corruption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/offset-to-loc.ts` around lines 9 - 37, The current
createLocMapper uses JS string code-unit indexing (charCodeAt/length) but
callers pass oxc-parser byte offsets, causing wrong positions for non-ASCII
text; update the call sites (e.g., where enhance-logs.ts calls createLocMapper)
to convert oxc byte offsets to JS code-unit indices before calling
createLocMapper OR modify createLocMapper to accept byte offsets by first
mapping byte offset → code-unit index (decode UTF-8 byte positions into JS
string indices) and then perform the existing binary search; finally add a
regression test that feeds source with multi-byte characters (emoji/CJK) and
verifies line/column results from createLocMapper match expected positions to
prevent regressions.
| const isTanStackDevtoolsImport = (source: string) => | ||
| source === '@tanstack/react-devtools' || | ||
| source === '@tanstack/devtools' || | ||
| source === '@tanstack/solid-devtools' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the devtools packages list in plugin.ts for consistency
echo "=== Packages in plugin.ts transform hook ==="
rg -A10 'devtoolPackages\s*=' packages/devtools-vite/src/plugin.ts
echo ""
echo "=== All tanstack devtools package references ==="
rg '@tanstack/(react|preact|solid|vue|angular)-devtools|@tanstack/devtools' packages/devtools-vite/src/Repository: TanStack/devtools
Length of output: 12965
🏁 Script executed:
rg -B5 -A5 'preact-devtools|vue-devtools' packages/devtools-vite/src/remove-devtools.test.tsRepository: TanStack/devtools
Length of output: 43
🏁 Script executed:
head -50 packages/devtools-vite/src/remove-devtools.test.tsRepository: TanStack/devtools
Length of output: 1415
Add missing TanStack devtools packages to the removal check.
isTanStackDevtoolsImport omits @tanstack/preact-devtools and @tanstack/vue-devtools, which are included in the devtoolPackages array in plugin.ts. This causes these imports to bypass removal during production builds.
🐛 Proposed fix
const isTanStackDevtoolsImport = (source: string) =>
source === '@tanstack/react-devtools' ||
source === '@tanstack/devtools' ||
- source === '@tanstack/solid-devtools'
+ source === '@tanstack/solid-devtools' ||
+ source === '@tanstack/preact-devtools' ||
+ source === '@tanstack/vue-devtools'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/remove-devtools.ts` around lines 6 - 9, The
function isTanStackDevtoolsImport currently only checks
'@tanstack/react-devtools', '@tanstack/devtools', and
'@tanstack/solid-devtools', so imports of '@tanstack/preact-devtools' and
'@tanstack/vue-devtools' are not detected and thus not removed; update
isTanStackDevtoolsImport to include '@tanstack/preact-devtools' and
'@tanstack/vue-devtools' (the same package names listed in devtoolPackages in
plugin.ts) so those imports are properly recognized and stripped during
production builds.
|
@dake3601 mind just updating the branch? |
|
View your CI Pipeline Execution ↗ for commit 9b1b486 ☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/devtools-vite/src/inject-source.ts (1)
152-178: Avoid walking variable-initialized functions twice.For
const Foo = () => <div />, theVariableDeclarationbranch processesdecl.init, and Lines 164-178 recurse into the sameArrowFunctionExpression/FunctionExpressionagain. That doubles thecollectJsx()work for the most common component shape in the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-vite/src/inject-source.ts` around lines 152 - 178, The VariableDeclaration branch processes decl.init (ArrowFunctionExpression/FunctionExpression) but the later forEachChild recursion re-visits the same function; to fix, ensure visitFunctions does not walk the init node twice by signaling that a child was handled: after calling processFunction for decl.init, mark didTransform/return true for that branch or exclude decl.init when calling forEachChild so the child visit is skipped; update visitFunctions (and any callers) to short-circuit/skip recursion for nodes already handled (references: visitFunctions, processFunction, forEachChild, VariableDeclaration, decl.init, ArrowFunctionExpression/FunctionExpression).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools-vite/src/inject-plugin.ts`:
- Around line 130-139: The buildPluginString function injects displayName and
importName raw into generated code, which breaks JS/TS if they contain
quotes/backslashes; update buildPluginString to escape dynamic string literals
before emitting (e.g., serialize displayName and packageName using a safe
literal serializer such as JSON.stringify or a dedicated escape function) so the
returned strings produce valid code for both the object form and JSX-like form;
apply the same escaping where packageName is interpolated elsewhere in the file
(the other occurrence noted in the review) and ensure importName is
validated/escaped when used in template output.
- Around line 233-243: When adding the import after a transform (the block that
runs when s.hasChanged()), avoid appending if the symbol is already imported:
scan result.program's ImportDeclaration nodes (the same walk over
result.program) and check each declaration's source.value ===
injection.packageName and its specifiers for an ImportSpecifier whose
imported.name === importName (also consider
ImportDefaultSpecifier/ImportNamespaceSpecifier if your importName could be
default or *); only call s.appendRight(lastImportEnd, importStr) when no
existing specifier for importName from injection.packageName is found. Ensure
you still compute lastImportEnd as before so a new import is inserted in the
correct place when needed.
In `@packages/devtools-vite/src/inject-source.ts`:
- Around line 92-100: collectJsx is traversing into nested function bodies and
collecting inner JSX with the outer props context; stop classifying
nested-function JSX by treating function/method nodes as traversal boundaries.
Update collectJsx (and the same logic referenced around lines 118-125) so that
when encountering nodes whose type is a function/method (e.g.,
FunctionDeclaration, FunctionExpression, ArrowFunctionExpression,
MethodDefinition, or similar AST function node types used in this project) it
does not recurse into their body — only inspect the function node itself if
needed but skip calling forEachChild on its body — so inner components like
Inner retain their own propsName processing instead of being annotated by the
outer scope.
- Around line 127-136: The code currently uses oxc-parser byte offsets
(jsx.start / jsx.end) directly with offsetToLoc and magic-string appendLeft, but
oxc-parser 0.72.0 returns UTF-8 byte offsets while offsetToLoc/createLocMapper
and magic-string expect JS string indices (UTF-16); convert the UTF-8 byte
offsets to JavaScript string indices before calling offsetToLoc(jsx.start) and
before s.appendLeft(jsx.end - 2 / jsx.end - 1). Add or reuse a helper that maps
UTF-8 byte offsets to JS string offsets and apply it to jsx.start and jsx.end
(and any other places like where createLocMapper is called around lines 204–205)
so the inserted data-tsd-source positions and line/column values are correct for
multibyte characters.
---
Nitpick comments:
In `@packages/devtools-vite/src/inject-source.ts`:
- Around line 152-178: The VariableDeclaration branch processes decl.init
(ArrowFunctionExpression/FunctionExpression) but the later forEachChild
recursion re-visits the same function; to fix, ensure visitFunctions does not
walk the init node twice by signaling that a child was handled: after calling
processFunction for decl.init, mark didTransform/return true for that branch or
exclude decl.init when calling forEachChild so the child visit is skipped;
update visitFunctions (and any callers) to short-circuit/skip recursion for
nodes already handled (references: visitFunctions, processFunction,
forEachChild, VariableDeclaration, decl.init,
ArrowFunctionExpression/FunctionExpression).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3a88f68-43d1-4cb4-94e2-d3a1fdd1cdaf
📒 Files selected for processing (4)
packages/devtools-vite/skills/devtools-vite-plugin/SKILL.mdpackages/devtools-vite/src/enhance-logs.tspackages/devtools-vite/src/inject-plugin.tspackages/devtools-vite/src/inject-source.ts
✅ Files skipped from review due to trivial changes (1)
- packages/devtools-vite/skills/devtools-vite-plugin/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/devtools-vite/src/enhance-logs.ts
| function buildPluginString( | ||
| importName: string, | ||
| displayName: string, | ||
| pluginType: string, | ||
| ): string { | ||
| if (pluginType === 'function') { | ||
| return `${importName}()` | ||
| } | ||
| return `{ name: "${displayName}", render: <${importName} /> }` | ||
| } |
There was a problem hiding this comment.
Escape dynamic string literals before emitting transformed code.
displayName and packageName are injected into generated source via raw interpolation. If either contains quotes/backslashes, the output can become invalid TS/JS (or inject unintended code).
Proposed fix
function buildPluginString(
importName: string,
displayName: string,
pluginType: string,
): string {
if (pluginType === 'function') {
return `${importName}()`
}
- return `{ name: "${displayName}", render: <${importName} /> }`
+ return `{ name: ${JSON.stringify(displayName)}, render: <${importName} /> }`
}
@@
- const importStr = `\nimport { ${importName} } from "${injection.packageName}";`
+ const importStr = `\nimport { ${importName} } from ${JSON.stringify(injection.packageName)};`Also applies to: 235-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/inject-plugin.ts` around lines 130 - 139, The
buildPluginString function injects displayName and importName raw into generated
code, which breaks JS/TS if they contain quotes/backslashes; update
buildPluginString to escape dynamic string literals before emitting (e.g.,
serialize displayName and packageName using a safe literal serializer such as
JSON.stringify or a dedicated escape function) so the returned strings produce
valid code for both the object form and JSX-like form; apply the same escaping
where packageName is interpolated elsewhere in the file (the other occurrence
noted in the review) and ensure importName is validated/escaped when used in
template output.
| // Add import at the top of the file if transform happened | ||
| if (s.hasChanged()) { | ||
| const importStr = `\nimport { ${importName} } from "${injection.packageName}";` | ||
| let lastImportEnd = 0 | ||
| walk(result.program, (n) => { | ||
| if (n.type === 'ImportDeclaration' && n.end > lastImportEnd) { | ||
| lastImportEnd = n.end | ||
| } | ||
| }) | ||
| s.appendRight(lastImportEnd, importStr) | ||
| } |
There was a problem hiding this comment.
Avoid appending an import when the symbol is already imported.
After any transform, a new import is always appended. If importName is already imported, this can introduce duplicate bindings and fail module parsing.
Proposed fix
if (s.hasChanged()) {
- const importStr = `\nimport { ${importName} } from ${JSON.stringify(injection.packageName)};`
+ let alreadyImported = false
let lastImportEnd = 0
walk(result.program, (n) => {
if (n.type === 'ImportDeclaration' && n.end > lastImportEnd) {
lastImportEnd = n.end
}
+ if (
+ n.type === 'ImportDeclaration' &&
+ n.source.value === injection.packageName
+ ) {
+ for (const spec of n.specifiers) {
+ if (
+ spec.type === 'ImportSpecifier' &&
+ spec.local.name === importName
+ ) {
+ alreadyImported = true
+ }
+ }
+ }
})
- s.appendRight(lastImportEnd, importStr)
+ if (!alreadyImported) {
+ const importStr = `\nimport { ${importName} } from ${JSON.stringify(injection.packageName)};`
+ s.appendRight(lastImportEnd, importStr)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/inject-plugin.ts` around lines 233 - 243, When
adding the import after a transform (the block that runs when s.hasChanged()),
avoid appending if the symbol is already imported: scan result.program's
ImportDeclaration nodes (the same walk over result.program) and check each
declaration's source.value === injection.packageName and its specifiers for an
ImportSpecifier whose imported.name === importName (also consider
ImportDefaultSpecifier/ImportNamespaceSpecifier if your importName could be
default or *); only call s.appendRight(lastImportEnd, importStr) when no
existing specifier for importName from injection.packageName is found. Ensure
you still compute lastImportEnd as before so a new import is inserted in the
correct place when needed.
| /** | ||
| * Walk a subtree collecting every JSXOpeningElement (crosses into nested functions). | ||
| */ | ||
| function collectJsx(node: Node, out: Array<JSXOpeningElement>) { | ||
| if (node.type === 'JSXOpeningElement') { | ||
| out.push(node) | ||
| return | ||
| } | ||
| forEachChild(node, (child) => collectJsx(child, out)) |
There was a problem hiding this comment.
Stop classifying nested-function JSX with the outer propsName.
Line 121 collects JSX from the entire body, including nested functions, but nested functions are also revisited later. That lets an outer function inject into inner JSX with the wrong props context. For example, const Inner = (innerProps) => <div {...innerProps} /> inside another function gets annotated before Inner is evaluated, so the forwarded data-tsd-source is overwritten instead of skipped.
Proposed fix
+function isFunctionLike(node: Node): boolean {
+ return (
+ node.type === 'FunctionDeclaration' ||
+ node.type === 'FunctionExpression' ||
+ node.type === 'ArrowFunctionExpression'
+ )
+}
+
/**
- * Walk a subtree collecting every JSXOpeningElement (crosses into nested functions).
+ * Walk a subtree collecting JSXOpeningElements owned by the current function.
*/
function collectJsx(node: Node, out: Array<JSXOpeningElement>) {
if (node.type === 'JSXOpeningElement') {
out.push(node)
return
}
+ if (isFunctionLike(node)) {
+ return
+ }
forEachChild(node, (child) => collectJsx(child, out))
}Also applies to: 118-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/inject-source.ts` around lines 92 - 100,
collectJsx is traversing into nested function bodies and collecting inner JSX
with the outer props context; stop classifying nested-function JSX by treating
function/method nodes as traversal boundaries. Update collectJsx (and the same
logic referenced around lines 118-125) so that when encountering nodes whose
type is a function/method (e.g., FunctionDeclaration, FunctionExpression,
ArrowFunctionExpression, MethodDefinition, or similar AST function node types
used in this project) it does not recurse into their body — only inspect the
function node itself if needed but skip calling forEachChild on its body — so
inner components like Inner retain their own propsName processing instead of
being annotated by the outer scope.
| const loc = offsetToLoc(jsx.start) | ||
| const attrStr = ` data-tsd-source="${file}:${loc.line}:${loc.column + 1}"` | ||
|
|
||
| // Insert before '>' or '/>' at the end of the opening element | ||
| if (jsx.selfClosing) { | ||
| // ends with '/>' — insert before '/>' | ||
| s.appendLeft(jsx.end - 2, attrStr) | ||
| } else { | ||
| // ends with '>' — insert before '>' | ||
| s.appendLeft(jsx.end - 1, attrStr) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In oxc-parserversion0.72.0JavaScript bindings, are AST nodestart/endspans reported as UTF-8 byte offsets or JavaScript string/code-unit indices, and are they intended to be used directly withmagic-string edit positions?
💡 Result:
In oxc-parser 0.72.0’s JS bindings, AST/node start/end (i.e. Span) positions are UTF-8 byte offsets into the source text (matching Oxc’s core Span design as byte offsets) rather than JavaScript UTF-16/code-unit indices. [1][2]
Because magic-string APIs take character indices referring to the original JS string (i.e. JS string indices / UTF-16 code units), you should not use Oxc’s start/end directly as magic-string edit positions when the source may contain non-ASCII characters (emoji, etc.). You must convert UTF-8 byte offsets → UTF-16 indices first (Oxc’s own docs/examples call out this UTF-8 vs UTF-16 mismatch explicitly). [3][4]
Sources: [1] [2] [3] [4]
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
wc -l packages/devtools-vite/src/inject-source.tsRepository: TanStack/devtools
Length of output: 108
🏁 Script executed:
# Check the exact code at lines 127-136
sed -n '120,140p' packages/devtools-vite/src/inject-source.tsRepository: TanStack/devtools
Length of output: 771
🏁 Script executed:
# Check lines 204-205
sed -n '200,210p' packages/devtools-vite/src/inject-source.tsRepository: TanStack/devtools
Length of output: 331
🏁 Script executed:
# Search for any UTF-8 to UTF-16 conversion utilities in the codebase
rg -i "utf|byte.*index|string.*index" --type ts --type jsRepository: TanStack/devtools
Length of output: 1163
🏁 Script executed:
# Check if there's any offset conversion happening with oxc-parser elsewhere
rg "appendLeft|appendRight" --type ts --type js -B 3 -A 3Repository: TanStack/devtools
Length of output: 3020
🏁 Script executed:
cat -n packages/devtools-vite/src/inject-source.tsRepository: TanStack/devtools
Length of output: 7451
🏁 Script executed:
# Look for createLocMapper definition
rg "createLocMapper" --type ts -B 5 -A 15Repository: TanStack/devtools
Length of output: 7291
🏁 Script executed:
# Check what imports are used
head -20 packages/devtools-vite/src/inject-source.tsRepository: TanStack/devtools
Length of output: 682
🏁 Script executed:
cat -n packages/devtools-vite/src/offset-to-loc.tsRepository: TanStack/devtools
Length of output: 1283
🏁 Script executed:
# Check if there's any handling of UTF-8 byte conversion
rg "byte|utf|encode|decode" packages/devtools-vite/src/ --type tsRepository: TanStack/devtools
Length of output: 1085
Convert oxc-parser UTF-8 byte offsets to JavaScript string indices before using with magic-string and createLocMapper.
Lines 127–136 (and 204–205) use jsx.start / jsx.end from oxc-parser directly. In oxc-parser 0.72.0, these are UTF-8 byte offsets, but magic-string APIs and createLocMapper expect JavaScript string indices (UTF-16 code units). This mismatch causes incorrect appendLeft() insertion positions and wrong line/column values in the emitted data-tsd-source attribute whenever the source contains multibyte UTF-8 characters (emoji, non-ASCII text, etc.).
A UTF-8 to UTF-16 conversion is needed before passing offsets to either function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools-vite/src/inject-source.ts` around lines 127 - 136, The code
currently uses oxc-parser byte offsets (jsx.start / jsx.end) directly with
offsetToLoc and magic-string appendLeft, but oxc-parser 0.72.0 returns UTF-8
byte offsets while offsetToLoc/createLocMapper and magic-string expect JS string
indices (UTF-16); convert the UTF-8 byte offsets to JavaScript string indices
before calling offsetToLoc(jsx.start) and before s.appendLeft(jsx.end - 2 /
jsx.end - 1). Add or reuse a helper that maps UTF-8 byte offsets to JS string
offsets and apply it to jsx.start and jsx.end (and any other places like where
createLocMapper is called around lines 204–205) so the inserted data-tsd-source
positions and line/column values are correct for multibyte characters.
2 participants
🎯 Changes
Replace Babel toolchain with oxc-parser for parsing and MagicString for source transformation in devtools-vite.
This improves vite dev performance. Having the plugin in our vite config increase inner loop with ~20 sec with these changes it drops down to around ~7 in my machine.
✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
Documentation
Chores
Improvements