improve bundle size benchmarks and add initial skill#7450
improve bundle size benchmarks and add initial skill#7450schiller-manuel wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughPR expands bundle-size benchmarking with scenario filtering, per-file sizing, sourcemap-based source attribution, and Nx-integrated package builds in measure.mjs. Adds four query/diff/history/analyze CLI tools for result inspection and reporting. Updates data parsing to use VM sandbox execution. Introduces ts-symbol-references CLI for TypeScript reference location discovery. Updates configuration, documentation, and npm scripts. ChangesBundle-Size Benchmarking Tools
TypeScript Symbol References Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 1f28eef
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 8 bumped as dependents. 🟩 Patch bumps
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/benchmarks/bundle-size/analyze.mjs`:
- Around line 15-16: Validate the `--top-sources` option (key 'top-sources')
before using it with Array.prototype.slice: parse the value to a number (e.g.,
using parseInt or Number), check Number.isInteger(value) and value > 0, and if
the check fails output a clear error and exit non-zero; update the code paths
where 'top-sources' is read (and where it's applied with slice around the slice
usage at the current slice call) to use the validated integer variable so bad
inputs fail explicitly.
In `@scripts/benchmarks/bundle-size/diff.mjs`:
- Around line 34-37: The code currently builds ids from values.id or from union
of baselineById/currentById, which hides typos when a user passes --id that
doesn't exist; add a guard: if values.id is truthy and neither
baselineById.has(values.id) nor currentById.has(values.id), throw a clear error
(or exit non‑zero) indicating the requested id is unknown; update the logic
around the ids assignment (referencing values.id, baselineById, currentById, and
the ids variable) to perform this existence check before proceeding.
In `@scripts/benchmarks/bundle-size/history.mjs`:
- Around line 15-16: The CLI option 'top-deltas' must be validated before being
used for array slicing: locate the option definition (key 'top-deltas') and the
code that reads argv['top-deltas'] (and any usage like .slice(0, topDeltas)
around the history processing) and add a check that the parsed value is a finite
positive integer (Number.isFinite and Number.isInteger and > 0). If the check
fails, throw or exit with a clear error message stating that --top-deltas must
be a positive integer. Apply the same validation at both usage sites referenced
in the diff (the option definition and the slicing usage).
- Around line 22-25: The code in parseHistory uses vm.runInNewContext to
evaluate user-provided payloads (the block where
trimmed.startsWith('window.BENCHMARK_DATA') creates sandbox and calls
vm.runInNewContext), which allows arbitrary code execution; instead, strip the
optional "window.BENCHMARK_DATA =" prefix and any trailing semicolon from the
trimmed string and parse the remaining content with JSON.parse to return the
benchmark object (i.e., remove the sandbox/vm.runInNewContext logic and replace
it with safe string extraction + JSON.parse).
In `@scripts/benchmarks/bundle-size/measure.mjs`:
- Around line 158-161: The code builds the requested array from the filter
string but doesn't reject cases where the input was only commas/whitespace,
producing an empty requested list; update the logic after the requested
calculation in measure.mjs to detect requested.length === 0 (or equivalently
!requested.length) and fail fast by emitting a clear error mentioning the
--scenario selector was empty and exiting non‑zero (e.g., throw an Error or call
process.exit(1)); reference the requested variable and the code path that
consumes it so the script never proceeds to publish an empty snapshot when no
scenarios were selected.
- Around line 739-743: The code forces at least 1 byte for tail mappings (bytes
= Math.max(1, ...)) which overcounts zero-width mappings; change the calculation
in the block that computes bytes (the variables: previousSource, previousColumn,
generatedLines, lineIndex) to allow zero-width results by using Math.max(0,
(generatedLines[lineIndex]?.length || previousColumn) - previousColumn) (or
equivalent non-negative clamp) so zero-length tail mappings aren't rounded up.
- Around line 954-956: status.command stores a shell command but inserts
args.scenario raw, causing multi-token values like "react-router.minimal,
solid-router.full" to split; update the interpolation in measure.mjs where
status.command is built so args.scenario is shell-quoted or escaped (e.g., wrap
in a quoted string via JSON.stringify or a proper shell-escape helper) before
concatenation, ensuring the persisted rerun command can be pasted verbatim;
apply the same quoting approach to any other arg interpolations in the same
expression if they can contain spaces or punctuation.
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 71-74: The code currently executes untrusted text via
vm.runInNewContext(trimmed, sandbox...) to obtain window.BENCHMARK_DATA; replace
this with a safe extraction: use a regular expression against trimmed to capture
the JSON assigned to window.BENCHMARK_DATA, validate the match, and then parse
the captured JSON with JSON.parse() to return the data instead of executing
code. Locate the block using trimmed and vm.runInNewContext and update it to
extract the RHS of the window.BENCHMARK_DATA assignment via regex, check for a
match, and call JSON.parse on the captured string (handle parse errors) before
returning the resulting object.
🪄 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: 54471d3d-46c0-47c9-bda8-09944273e2f3
📒 Files selected for processing (11)
benchmarks/bundle-size/README.mdbenchmarks/bundle-size/package.jsonpackage.jsonscripts/benchmarks/bundle-size/analyze.mjsscripts/benchmarks/bundle-size/diff.mjsscripts/benchmarks/bundle-size/history.mjsscripts/benchmarks/bundle-size/measure.mjsscripts/benchmarks/bundle-size/pr-report.mjsscripts/benchmarks/bundle-size/query.mjsscripts/ts-symbol-references.mjsskills/bundle-size-optimization/SKILL.md
| 'top-sources': { type: 'string', default: '30' }, | ||
| json: { type: 'boolean' }, |
There was a problem hiding this comment.
Add validation for --top-sources.
Please validate that --top-sources is a positive integer before applying slice, so bad inputs fail explicitly instead of producing confusing output.
Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/analyze.mjs` around lines 15 - 16, Validate
the `--top-sources` option (key 'top-sources') before using it with
Array.prototype.slice: parse the value to a number (e.g., using parseInt or
Number), check Number.isInteger(value) and value > 0, and if the check fails
output a clear error and exit non-zero; update the code paths where
'top-sources' is read (and where it's applied with slice around the slice usage
at the current slice call) to use the validated integer variable so bad inputs
fail explicitly.
| const ids = values.id | ||
| ? [values.id] | ||
| : [...new Set([...baselineById.keys(), ...currentById.keys()])].sort() | ||
|
|
There was a problem hiding this comment.
Fail fast on unknown --id.
When --id is provided but absent from both baseline and current, returning an n/a row hides typos. Throwing a clear error matches the behavior of the other bundle-size CLIs.
Suggested guard
const baselineById = byId(readCurrent(values.baseline))
const currentById = byId(readCurrent(values.current))
+
+if (
+ values.id &&
+ !baselineById.has(values.id) &&
+ !currentById.has(values.id)
+) {
+ throw new Error(`Unknown bundle-size metric: ${values.id}`)
+}
+
const ids = values.id
? [values.id]
: [...new Set([...baselineById.keys(), ...currentById.keys()])].sort()📝 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.
| const ids = values.id | |
| ? [values.id] | |
| : [...new Set([...baselineById.keys(), ...currentById.keys()])].sort() | |
| const baselineById = byId(readCurrent(values.baseline)) | |
| const currentById = byId(readCurrent(values.current)) | |
| if ( | |
| values.id && | |
| !baselineById.has(values.id) && | |
| !currentById.has(values.id) | |
| ) { | |
| throw new Error(`Unknown bundle-size metric: ${values.id}`) | |
| } | |
| const ids = values.id | |
| ? [values.id] | |
| : [...new Set([...baselineById.keys(), ...currentById.keys()])].sort() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/diff.mjs` around lines 34 - 37, The code
currently builds ids from values.id or from union of baselineById/currentById,
which hides typos when a user passes --id that doesn't exist; add a guard: if
values.id is truthy and neither baselineById.has(values.id) nor
currentById.has(values.id), throw a clear error (or exit non‑zero) indicating
the requested id is unknown; update the logic around the ids assignment
(referencing values.id, baselineById, currentById, and the ids variable) to
perform this existence check before proceeding.
| 'top-deltas': { type: 'string', default: '20' }, | ||
| json: { type: 'boolean' }, |
There was a problem hiding this comment.
Validate --top-deltas input before slicing.
Invalid or negative values currently degrade into ambiguous output. Add a finite positive-integer check and throw a clear error.
Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/history.mjs` around lines 15 - 16, The CLI
option 'top-deltas' must be validated before being used for array slicing:
locate the option definition (key 'top-deltas') and the code that reads
argv['top-deltas'] (and any usage like .slice(0, topDeltas) around the history
processing) and add a check that the parsed value is a finite positive integer
(Number.isFinite and Number.isInteger and > 0). If the check fails, throw or
exit with a clear error message stating that --top-deltas must be a positive
integer. Apply the same validation at both usage sites referenced in the diff
(the option definition and the slicing usage).
| if (trimmed.startsWith('window.BENCHMARK_DATA')) { | ||
| const sandbox = { window: {} } | ||
| vm.runInNewContext(trimmed, sandbox, { timeout: 1000 }) | ||
| return sandbox.window.BENCHMARK_DATA |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What do Node.js docs say about using node:vm with untrusted code, and is it recommended as a security boundary?
💡 Result:
The Node.js documentation explicitly states that the node:vm module is not a security mechanism and should not be used to run untrusted code [1][2]. The official documentation repeatedly emphasizes this warning, clarifying that the term "sandbox" is used in the context of the module only to refer to a separate execution context, not to provide security guarantees [3][4]. It is not recommended as a security boundary because it does not provide true isolation [5]. The node:vm module allows code to be compiled and run within V8 virtual machine contexts, which is primarily a tool for scoping variables and creating separate global environments [1][2]. Malicious code can easily escape these contexts by walking up the prototype chain to access host-realm objects or the host process, effectively bypassing the intended isolation [5]. Consequently, attempting to use node:vm to safely execute untrusted code is considered inherently insecure and is not a valid approach for building secure sandboxes [6][5].
Citations:
- 1: https://nodejs.org/docs/v22.11.0/api/vm.html
- 2: https://nodejs.org/api/vm.html
- 3: https://nodejs.org/dist/v10.0.0/docs/api/vm.html
- 4: https://nodejs.org/dist/v11.0.0/docs/api/vm.html
- 5: https://dev.to/dendrite_soup/nodevm-is-not-a-sandbox-stop-using-it-like-one-2f74
- 6: docs: Clarification around real world risks and use cases of VM module nodejs/node#40718
🏁 Script executed:
# Find the file and examine the parseHistory function and surrounding context
cat -n scripts/benchmarks/bundle-size/history.mjsRepository: TanStack/router
Length of output: 2937
🏁 Script executed:
# Search for CLI argument handling related to history
rg -i "history" scripts/benchmarks/bundle-size/ -A 2 -B 2Repository: TanStack/router
Length of output: 13412
🏁 Script executed:
# Search for how the history file is used in the benchmark script
rg -i "\.mjs|history" --type javascript | grep -i benchmark | head -20Repository: TanStack/router
Length of output: 94
Do not execute history file payloads with vm.runInNewContext.
The --history CLI argument (line 44-45) accepts user-provided files and passes them to parseHistory, which evaluates code via vm.runInNewContext (lines 22-25). Node.js documentation explicitly warns that the vm module is not a security mechanism and should not be used to run untrusted code—it does not provide true isolation.
Since the benchmark data is JSON (optionally wrapped with window.BENCHMARK_DATA = ), replace the code execution with string extraction and JSON.parse:
if (trimmed.startsWith('window.BENCHMARK_DATA')) {
const jsonStr = trimmed.replace(/^window\.BENCHMARK_DATA\s*=\s*/, '').replace(/;$/, '')
return JSON.parse(jsonStr)
}This eliminates arbitrary code execution while parsing the data safely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/history.mjs` around lines 22 - 25, The code in
parseHistory uses vm.runInNewContext to evaluate user-provided payloads (the
block where trimmed.startsWith('window.BENCHMARK_DATA') creates sandbox and
calls vm.runInNewContext), which allows arbitrary code execution; instead, strip
the optional "window.BENCHMARK_DATA =" prefix and any trailing semicolon from
the trimmed string and parse the remaining content with JSON.parse to return the
benchmark object (i.e., remove the sandbox/vm.runInNewContext logic and replace
it with safe string extraction + JSON.parse).
| const requested = filter | ||
| .split(',') | ||
| .map((value) => value.trim()) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
Reject empty --scenario selector lists.
If the user passes only commas or whitespace, this collapses to an empty array and the script reports success with zero measured scenarios. Failing fast here avoids publishing an empty snapshot by accident.
Suggested fix
const requested = filter
.split(',')
.map((value) => value.trim())
.filter(Boolean)
+ if (requested.length === 0) {
+ throw new Error('--scenario must include at least one non-empty selector')
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/measure.mjs` around lines 158 - 161, The code
builds the requested array from the filter string but doesn't reject cases where
the input was only commas/whitespace, producing an empty requested list; update
the logic after the requested calculation in measure.mjs to detect
requested.length === 0 (or equivalently !requested.length) and fail fast by
emitting a clear error mentioning the --scenario selector was empty and exiting
non‑zero (e.g., throw an Error or call process.exit(1)); reference the requested
variable and the code path that consumes it so the script never proceeds to
publish an empty snapshot when no scenarios were selected.
| status: { | ||
| state: 'success', | ||
| command: `node ${path.relative(repoRoot, fileURLToPath(import.meta.url))}${args.scenario ? ` --scenario ${args.scenario}` : ''}${args.analysis ? ' --analysis' : ''}${args.sourcemap ? ' --sourcemap' : ''}${args.skipPackageBuilds ? ' --skip-package-builds' : ''}`, |
There was a problem hiding this comment.
Quote the persisted rerun command.
status.command stores a shell command, but args.scenario is interpolated raw. A common value like react-router.minimal, solid-router.full gets recorded as two argv tokens and cannot be pasted back verbatim.
Suggested fix
- command: `node ${path.relative(repoRoot, fileURLToPath(import.meta.url))}${args.scenario ? ` --scenario ${args.scenario}` : ''}${args.analysis ? ' --analysis' : ''}${args.sourcemap ? ' --sourcemap' : ''}${args.skipPackageBuilds ? ' --skip-package-builds' : ''}`,
+ command: `node ${path.relative(repoRoot, fileURLToPath(import.meta.url))}${args.scenario ? ` --scenario ${JSON.stringify(args.scenario)}` : ''}${args.analysis ? ' --analysis' : ''}${args.sourcemap ? ' --sourcemap' : ''}${args.skipPackageBuilds ? ' --skip-package-builds' : ''}`,📝 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.
| status: { | |
| state: 'success', | |
| command: `node ${path.relative(repoRoot, fileURLToPath(import.meta.url))}${args.scenario ? ` --scenario ${args.scenario}` : ''}${args.analysis ? ' --analysis' : ''}${args.sourcemap ? ' --sourcemap' : ''}${args.skipPackageBuilds ? ' --skip-package-builds' : ''}`, | |
| status: { | |
| state: 'success', | |
| command: `node ${path.relative(repoRoot, fileURLToPath(import.meta.url))}${args.scenario ? ` --scenario ${JSON.stringify(args.scenario)}` : ''}${args.analysis ? ' --analysis' : ''}${args.sourcemap ? ' --sourcemap' : ''}${args.skipPackageBuilds ? ' --skip-package-builds' : ''}`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/measure.mjs` around lines 954 - 956,
status.command stores a shell command but inserts args.scenario raw, causing
multi-token values like "react-router.minimal, solid-router.full" to split;
update the interpolation in measure.mjs where status.command is built so
args.scenario is shell-quoted or escaped (e.g., wrap in a quoted string via
JSON.stringify or a proper shell-escape helper) before concatenation, ensuring
the persisted rerun command can be pasted verbatim; apply the same quoting
approach to any other arg interpolations in the same expression if they can
contain spaces or punctuation.
| if (trimmed.startsWith('window.BENCHMARK_DATA')) { | ||
| return JSON.parse( | ||
| trimmed | ||
| .replace(/^window\.BENCHMARK_DATA\s*=\s*/, '') | ||
| .replace(/;\s*$/, ''), | ||
| ) | ||
| const sandbox = { window: {} } | ||
| vm.runInNewContext(trimmed, sandbox, { timeout: 1000 }) | ||
| return sandbox.window.BENCHMARK_DATA |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is node:vm (vm.runInNewContext) considered a security boundary for untrusted JavaScript input in Node.js?
💡 Result:
No, the node:vm module is explicitly not considered a security boundary, and it should never be used to run untrusted JavaScript code [1][2][3]. Official Node.js documentation consistently includes a prominent warning that the module is not a security mechanism and cautions against using it for untrusted input [1][4][3]. While the module uses V8 contexts to provide an isolated global environment for code execution, this is intended for scoping and convenience rather than security [1][5][3]. The primary reasons include: * Lack of true isolation: The module is designed to separate scopes, not to provide a hardened security boundary [5][6]. A malicious script can easily access the host environment by traversing the prototype chain—for example, by accessing constructors—to reach the host's process or global objects [5]. * Misconceptions about sandboxing: The term "sandbox" is sometimes used in informal contexts to describe a separate execution environment, but it does not imply that the code is protected or restricted from interacting with the host [4][5]. * Historical and practical failures: Numerous efforts to build secure "sandboxing" libraries on top of node:vm or similar mechanisms have historically failed, often resulting in critical vulnerabilities and sandbox escape exploits that allow for remote code execution on the host [5][7][8][9]. Because node:vm operates within the same runtime process as the host application, any compromise of the execution context can lead to full host takeover [8][10][6]. For scenarios requiring the execution of untrusted code, experts and documentation recommend using true isolation boundaries, such as separate operating system processes, containers, or dedicated virtual machines/sandboxes that operate outside the host's primary runtime environment [5][8][10].
Citations:
- 1: https://nodejs.org/dist/latest-v19.x/docs/api/vm.html
- 2: https://nodejs.org/download/release/v18.20.4/docs/api/vm.html
- 3: https://nodejs.org/api/vm.html
- 4: https://nodejs.org/dist/v11.0.0/docs/api/vm.html
- 5: https://dev.to/dendrite_soup/nodevm-is-not-a-sandbox-stop-using-it-like-one-2f74
- 6: https://snyk.io/blog/security-concerns-javascript-sandbox-node-js-vm-module/
- 7: https://www.csoonline.com/article/4168568/13-new-critical-holes-in-javascript-sandbox-allow-execution-of-arbitrary-code.html
- 8: https://undercodenews.com/critical-vm2-sandbox-disaster-exposes-millions-of-nodejs-systems-to-remote-code-execution/
- 9: https://undercodenews.com/critical-vm2-sandbox-escape-vulnerabilities-expose-nodejs-systems-to-full-remote-code-execution/
- 10: https://b2bdaily.com/it/can-a-single-sandboxjs-flaw-lead-to-full-host-takeover/
🏁 Script executed:
# Find and read the pr-report.mjs file
fd -t f "pr-report.mjs" --exec cat -n {} \;Repository: TanStack/router
Length of output: 11479
🏁 Script executed:
# Look for context around lines 71-74 and check data source
fd -t f "pr-report.mjs" --exec sed -n '1,80p' {} \;Repository: TanStack/router
Length of output: 2125
🏁 Script executed:
# Search for other vm module usage in the codebase
rg "vm\." --type javascript --type typescript -B 2 -A 2Repository: TanStack/router
Length of output: 94
Replace vm.runInNewContext() with regex extraction and JSON.parse().
The node:vm module provides no security boundary against untrusted code execution. Data files read from external sources should be parsed safely without code execution.
Safer parsing approach
function parseMaybeDataJs(raw) {
const trimmed = raw.trim()
if (trimmed.startsWith('window.BENCHMARK_DATA')) {
- const sandbox = { window: {} }
- vm.runInNewContext(trimmed, sandbox, { timeout: 1000 })
- return sandbox.window.BENCHMARK_DATA
+ const match = trimmed.match(
+ /^window\.BENCHMARK_DATA\s*=\s*(\{[\s\S]*\})\s*;?\s*$/,
+ )
+ if (!match) {
+ throw new Error('Invalid data.js format for window.BENCHMARK_DATA')
+ }
+ return JSON.parse(match[1])
}
return JSON.parse(trimmed)
}📝 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.
| if (trimmed.startsWith('window.BENCHMARK_DATA')) { | |
| return JSON.parse( | |
| trimmed | |
| .replace(/^window\.BENCHMARK_DATA\s*=\s*/, '') | |
| .replace(/;\s*$/, ''), | |
| ) | |
| const sandbox = { window: {} } | |
| vm.runInNewContext(trimmed, sandbox, { timeout: 1000 }) | |
| return sandbox.window.BENCHMARK_DATA | |
| if (trimmed.startsWith('window.BENCHMARK_DATA')) { | |
| const match = trimmed.match( | |
| /^window\.BENCHMARK_DATA\s*=\s*(\{[\s\S]*\})\s*;?\s*$/, | |
| ) | |
| if (!match) { | |
| throw new Error('Invalid data.js format for window.BENCHMARK_DATA') | |
| } | |
| return JSON.parse(match[1]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 71 - 74, The code
currently executes untrusted text via vm.runInNewContext(trimmed, sandbox...) to
obtain window.BENCHMARK_DATA; replace this with a safe extraction: use a regular
expression against trimmed to capture the JSON assigned to
window.BENCHMARK_DATA, validate the match, and then parse the captured JSON with
JSON.parse() to return the data instead of executing code. Locate the block
using trimmed and vm.runInNewContext and update it to extract the RHS of the
window.BENCHMARK_DATA assignment via regex, check for a match, and call
JSON.parse on the captured string (handle parse errors) before returning the
resulting object.
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
schiller-manuel
force-pushed
the
bundle-size-skill
branch
from
3dda1ba to
1f28eef
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/benchmarks/bundle-size/query.mjs (1)
25-27: ⚡ Quick winConsider listing available metric IDs in the error message.
When an unknown ID is provided, showing the available options would improve discoverability.
💡 Proposed enhancement
if (values.id && metrics.length === 0) { - throw new Error(`Unknown bundle-size metric: ${values.id}`) + const availableIds = (current.metrics || []).map((m) => m.id).join(', ') + throw new Error( + `Unknown bundle-size metric: ${values.id}\nAvailable: ${availableIds}` + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/benchmarks/bundle-size/query.mjs` around lines 25 - 27, The error thrown for an unknown metric currently uses only values.id; update the throw in the check that uses values.id and metrics (the block that currently does throw new Error(`Unknown bundle-size metric: ${values.id}`)) to include the available metric IDs by deriving them from the metrics collection (e.g., map metrics to their id strings and join them), and include that list in the Error message so it reads like "Unknown bundle-size metric: <id>. Available metrics: <id1, id2, ...>".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/benchmarks/bundle-size/query.mjs`:
- Around line 19-20: Wrap the synchronous file read and JSON.parse around a
try-catch to provide clear error handling: when resolving currentPath, reading
via fs.readFileSync and parsing into current, catch errors (file not found,
permission, or invalid JSON), log a descriptive message including currentPath
and the caught error (e.g., via console.error) and exit with a non-zero code;
optionally pre-check with fs.existsSync(currentPath) to give a specific "file
not found" message before parsing.
In `@scripts/ts-symbol-references.mjs`:
- Around line 152-160: The current CLI parses line/column into
lineNumber/columnNumber and only checks Number.isInteger, allowing 0 or
negatives which become invalid after decrement; update the validation around the
conversion of line and column to ensure both are integers >= 1 and call fail
with a clear message if not (e.g., "--line and --column must be positive 1-based
integers"), before calling sourceFile.getPositionOfLineAndCharacter (so adjust
the checks around lineNumber, columnNumber, Number.isInteger and the fail
invocation).
In `@skills/bundle-size-optimization/SKILL.md`:
- Line 31: Replace the machine-specific hardcoded temp path in SKILL.md ("
/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode") with a generic
placeholder (e.g., "<temp-dir>/opencode" or "/tmp/opencode") so instructions are
portable; update the sentence that references the path to read something like
"run the same scenario in a separate worktree under <temp-dir>/opencode and diff
the two `current.json` files" and keep the backticked `current.json` reference
unchanged.
---
Nitpick comments:
In `@scripts/benchmarks/bundle-size/query.mjs`:
- Around line 25-27: The error thrown for an unknown metric currently uses only
values.id; update the throw in the check that uses values.id and metrics (the
block that currently does throw new Error(`Unknown bundle-size metric:
${values.id}`)) to include the available metric IDs by deriving them from the
metrics collection (e.g., map metrics to their id strings and join them), and
include that list in the Error message so it reads like "Unknown bundle-size
metric: <id>. Available metrics: <id1, id2, ...>".
🪄 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: 52f96b00-26a7-4e1e-9499-d00234ba6c2b
📒 Files selected for processing (12)
AGENTS.mdbenchmarks/bundle-size/README.mdbenchmarks/bundle-size/package.jsonpackage.jsonscripts/benchmarks/bundle-size/analyze.mjsscripts/benchmarks/bundle-size/diff.mjsscripts/benchmarks/bundle-size/history.mjsscripts/benchmarks/bundle-size/measure.mjsscripts/benchmarks/bundle-size/pr-report.mjsscripts/benchmarks/bundle-size/query.mjsscripts/ts-symbol-references.mjsskills/bundle-size-optimization/SKILL.md
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- benchmarks/bundle-size/README.md
| const currentPath = path.resolve(values.current) | ||
| const current = JSON.parse(fs.readFileSync(currentPath, 'utf8')) |
There was a problem hiding this comment.
Add error handling for file read and JSON parse operations.
The script will crash with a generic Node.js error if the file doesn't exist or contains invalid JSON. Adding a try-catch block would provide clearer feedback.
🛡️ Proposed fix to add error handling
-const currentPath = path.resolve(values.current)
-const current = JSON.parse(fs.readFileSync(currentPath, 'utf8'))
+const currentPath = path.resolve(values.current)
+let current
+try {
+ current = JSON.parse(fs.readFileSync(currentPath, 'utf8'))
+} catch (error) {
+ console.error(`Failed to read results from ${currentPath}: ${error.message}`)
+ process.exit(1)
+}📝 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.
| const currentPath = path.resolve(values.current) | |
| const current = JSON.parse(fs.readFileSync(currentPath, 'utf8')) | |
| const currentPath = path.resolve(values.current) | |
| let current | |
| try { | |
| current = JSON.parse(fs.readFileSync(currentPath, 'utf8')) | |
| } catch (error) { | |
| console.error(`Failed to read results from ${currentPath}: ${error.message}`) | |
| process.exit(1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/benchmarks/bundle-size/query.mjs` around lines 19 - 20, Wrap the
synchronous file read and JSON.parse around a try-catch to provide clear error
handling: when resolving currentPath, reading via fs.readFileSync and parsing
into current, catch errors (file not found, permission, or invalid JSON), log a
descriptive message including currentPath and the caught error (e.g., via
console.error) and exit with a non-zero code; optionally pre-check with
fs.existsSync(currentPath) to give a specific "file not found" message before
parsing.
| const lineNumber = Number(line) | ||
| const columnNumber = Number(column) | ||
| if (!Number.isInteger(lineNumber) || !Number.isInteger(columnNumber)) { | ||
| fail('--line and --column must be integers.') | ||
| } | ||
|
|
||
| const position = sourceFile.getPositionOfLineAndCharacter( | ||
| lineNumber - 1, | ||
| columnNumber - 1, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript 6.x, what are the expected input constraints for SourceFile.getPositionOfLineAndCharacter(line, character), and how does it behave for negative line/character values?
💡 Result:
In TypeScript 6.x, SourceFile.getPositionOfLineAndCharacter(line, character) expects zero-based line and character inputs, with constraints that the line and character must be within the bounds of the source file; if an input is outside those bounds, the behavior is not defined in the public docs and internal callers typically guard against invalid line/character values. For negative line/character values: TypeScript does not provide a documented/guaranteed contract for negative line or character. The method is implemented in a way that assumes non-negative inputs (the underlying line-start lookup/assertions treat negative positions as invalid). As a result, passing negative values can lead to incorrect results or runtime failures depending on the exact TypeScript version/build and whether internal assertions are enabled. Evidence: internal TypeScript code paths that compute positions enforce non-negative constraints (e.g., asserts like “generatedCharacter cannot be negative” in the sourcemap pipeline) and the general compiler API guidance assumes valid, non-negative line/character usage when converting between lines/characters and positions. [1] [2]
Citations:
- 1: https://github.com/microsoft/TypeScript/blob/main/src/compiler/sourcemap.ts
- 2: https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API
🏁 Script executed:
# Check if the file exists and view the specific lines
if [ -f "scripts/ts-symbol-references.mjs" ]; then
echo "=== File exists ==="
wc -l scripts/ts-symbol-references.mjs
echo ""
echo "=== Lines 145-170 (context around lines 152-160) ==="
sed -n '145,170p' scripts/ts-symbol-references.mjs
else
echo "File not found at scripts/ts-symbol-references.mjs"
# Try to find it
find . -name "ts-symbol-references.mjs" -type f 2>/dev/null
fiRepository: TanStack/router
Length of output: 980
🏁 Script executed:
# Search for similar input validation patterns in the codebase to match style
echo "=== Search for Number.isInteger validation patterns ==="
rg "Number\.isInteger" -A 3 -B 1 scripts/Repository: TanStack/router
Length of output: 848
Validate --line/--column as 1-based positive integers.
0 or negative values currently pass integer checks and then get decremented, which can lead to invalid position lookup behavior instead of a clear CLI error.
Suggested fix
const lineNumber = Number(line)
const columnNumber = Number(column)
- if (!Number.isInteger(lineNumber) || !Number.isInteger(columnNumber)) {
- fail('--line and --column must be integers.')
+ if (
+ !Number.isInteger(lineNumber) ||
+ !Number.isInteger(columnNumber) ||
+ lineNumber < 1 ||
+ columnNumber < 1
+ ) {
+ fail('--line and --column must be positive integers (1-based).')
}📝 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.
| const lineNumber = Number(line) | |
| const columnNumber = Number(column) | |
| if (!Number.isInteger(lineNumber) || !Number.isInteger(columnNumber)) { | |
| fail('--line and --column must be integers.') | |
| } | |
| const position = sourceFile.getPositionOfLineAndCharacter( | |
| lineNumber - 1, | |
| columnNumber - 1, | |
| const lineNumber = Number(line) | |
| const columnNumber = Number(column) | |
| if ( | |
| !Number.isInteger(lineNumber) || | |
| !Number.isInteger(columnNumber) || | |
| lineNumber < 1 || | |
| columnNumber < 1 | |
| ) { | |
| fail('--line and --column must be positive integers (1-based).') | |
| } | |
| const position = sourceFile.getPositionOfLineAndCharacter( | |
| lineNumber - 1, | |
| columnNumber - 1, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ts-symbol-references.mjs` around lines 152 - 160, The current CLI
parses line/column into lineNumber/columnNumber and only checks
Number.isInteger, allowing 0 or negatives which become invalid after decrement;
update the validation around the conversion of line and column to ensure both
are integers >= 1 and call fail with a clear message if not (e.g., "--line and
--column must be positive 1-based integers"), before calling
sourceFile.getPositionOfLineAndCharacter (so adjust the checks around
lineNumber, columnNumber, Number.isInteger and the fail invocation).
| - Track `gzipBytes` first; also inspect `initialGzipBytes`, `rawBytes`, `brotliBytes`, `jsFiles`, and per-file `files`. | ||
| - Dist paths use `scenarioDir`/`outDir`, not metric ids: `react-router.minimal` maps to `dist/react-router-minimal/`. | ||
| - For tiny changes, measure after each candidate; gzip can move opposite raw bytes. | ||
| - To compare a base commit, run the same scenario in a separate worktree under `/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode` and diff the two `current.json` files. |
There was a problem hiding this comment.
Replace machine-specific path with a generic placeholder.
The hardcoded path /var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode appears to be a macOS-specific temporary directory from a particular machine. This could confuse users trying to follow the instructions literally.
📝 Proposed fix
-- To compare a base commit, run the same scenario in a separate worktree under `/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode` and diff the two `current.json` files.
+- To compare a base commit, run the same scenario in a separate worktree (e.g., `/tmp/base-worktree`) and diff the two `current.json` files.📝 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.
| - To compare a base commit, run the same scenario in a separate worktree under `/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode` and diff the two `current.json` files. | |
| - To compare a base commit, run the same scenario in a separate worktree (e.g., `/tmp/base-worktree`) and diff the two `current.json` files. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/bundle-size-optimization/SKILL.md` at line 31, Replace the
machine-specific hardcoded temp path in SKILL.md ("
/var/folders/6f/2t42ntqs4yv4h6qwzbh5pmcm0000gn/T/opencode") with a generic
placeholder (e.g., "<temp-dir>/opencode" or "/tmp/opencode") so instructions are
portable; update the sentence that references the path to read something like
"run the same scenario in a separate worktree under <temp-dir>/opencode and diff
the two `current.json` files" and keep the backticked `current.json` reference
unchanged.
1 participant
Summary by CodeRabbit
New Features
Documentation