test: add cross-framework client navigation benchmarks#6800
Merged
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 new cross-framework client-side navigation benchmarking suite under Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI / GitHub Actions
participant NX as pnpm / nx
participant Vitest as Vitest (runner)
participant App as RouterProvider / App
participant Router as MemoryHistory Router
CI->>NX: pnpm nx run `@benchmarks/client-nav`:test:perf
NX->>Vitest: start project (react / solid / vue)
Vitest->>App: mount RouterProvider
App->>Router: createTestRouter (memoryHistory)
loop navigations
Vitest->>Router: navigate to /{id}?{q} (replace)
Router->>App: render updates / onRendered
App->>Vitest: notify render complete
end
Vitest->>CI: emit results / CodSpeed upload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @benchmarks/client-nav:test:perf:solid |
❌ Failed | 1m 14s | View ↗ |
nx run @benchmarks/client-nav:test:perf:vue |
✅ Succeeded | 1m 23s | View ↗ |
nx run @benchmarks/client-nav:test:perf:react |
✅ Succeeded | 1m 26s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-02 16:51:04 UTC
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c2386154
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
benchmarks/client-nav/vitest.setup.ts (1)
3-4: Avoid@ts-expect-errorfor global setup typing.At Lines 3–4, declare the global type instead of suppressing errors to maintain strict typing compliance.
♻️ Suggested typed alternative
import { vi } from 'vitest' -// `@ts-expect-error` -global.IS_REACT_ACT_ENVIRONMENT = true +declare global { + // eslint-disable-next-line no-var + var IS_REACT_ACT_ENVIRONMENT: boolean +} + +globalThis.IS_REACT_ACT_ENVIRONMENT = true window.scrollTo = vi.fn()This follows the established pattern in the codebase and aligns with the coding guideline requiring strict TypeScript type safety for
**/*.{ts,tsx}files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/vitest.setup.ts` around lines 3 - 4, Replace the // `@ts-expect-error` hack by adding a proper global type declaration for IS_REACT_ACT_ENVIRONMENT: declare global { var IS_REACT_ACT_ENVIRONMENT: boolean } (or property on interface Global if preferred) in this module, ensure the file is treated as a module by adding export {} if needed, then remove the `@ts-expect-error` and assign global.IS_REACT_ACT_ENVIRONMENT = true; this keeps strict typing while preserving the existing runtime assignment.benchmarks/client-nav/react/speed.bench.tsx (1)
85-85: Prefer a non-rejecting default fornext.Using
Promise.reject()here can produce noisy unhandled rejections if the callback is touched beforesetupreassigns it. A resolved no-op default is safer for harness stability.♻️ Proposed tweak
- let next: () => Promise<void> = () => Promise.reject() + let next: () => Promise<void> = async () => {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/speed.bench.tsx` at line 85, The default `next` callback is initialized to a rejecting promise which can trigger unhandled rejections; change the initializer for the `next` variable (declared as `let next: () => Promise<void>`) to a non-rejecting no-op (e.g., a resolved promise or an async no-op) so callers touching `next` before `setup` runs won't cause runtime unhandled rejection noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/client-nav/solid/speed.bench.tsx`:
- Line 77: Remove the unpaired console.time('render') call in the route
component (it currently has no matching console.timeEnd('render')), so delete
that console.time('render') statement from the component to avoid adding extra
overhead to the navigation benchmark; if timing was intended instead, add a
matching console.timeEnd('render') at the end of the component render logic, but
otherwise simply remove the console.time('render') line.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Around line 114-120: The benchmark currently uses a numeric nextId when
calling navigate, causing Vue to pass numbers into params and search; change the
navigate call in speed.bench.tsx to pass string IDs by converting nextId to a
string (e.g., use nextId.toString() or template `${nextId}`) for both params and
search so route params/search are normalized to strings; locate the usage around
the nextId variable and the navigate({ to: '/$id', params: { id: ... }, search:
{ id: ... }, replace: true }) call and update both places to use the stringified
id.
- Line 75: Remove the unnecessary type cast on the route definition by replacing
the "component: Root as any" usage with "component: Root" so the route's
component property uses its existing unknown-compatible type instead of
bypassing type safety; update the route object where the component field is set
(look for the "component" property assigned from the Root identifier in
speed.bench.tsx) and remove the "as any" cast to comply with TypeScript strict
mode.
In `@e2e/bundle-size/package.json`:
- Around line 24-27: The package.json lists an incompatible pair:
"@vitejs/plugin-vue" at 5.2.3 and "vite" at ^7.3.1; update either dependency so
they match supported peer ranges — either upgrade "@vitejs/plugin-vue" to a
version that declares support for Vite 7 (replace 5.2.3 with the plugin's
Vite-7-compatible release) or downgrade "vite" to a 6.x range (e.g., ^6.x) so it
satisfies "@vitejs/plugin-vue@5.2.3"; ensure any related plugins like
"@vitejs/plugin-vue-jsx" are also bumped/downgraded to compatible versions and
run install to verify peer deps resolve.
---
Nitpick comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Line 85: The default `next` callback is initialized to a rejecting promise
which can trigger unhandled rejections; change the initializer for the `next`
variable (declared as `let next: () => Promise<void>`) to a non-rejecting no-op
(e.g., a resolved promise or an async no-op) so callers touching `next` before
`setup` runs won't cause runtime unhandled rejection noise.
In `@benchmarks/client-nav/vitest.setup.ts`:
- Around line 3-4: Replace the // `@ts-expect-error` hack by adding a proper
global type declaration for IS_REACT_ACT_ENVIRONMENT: declare global { var
IS_REACT_ACT_ENVIRONMENT: boolean } (or property on interface Global if
preferred) in this module, ensure the file is treated as a module by adding
export {} if needed, then remove the `@ts-expect-error` and assign
global.IS_REACT_ACT_ENVIRONMENT = true; this keeps strict typing while
preserving the existing runtime assignment.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
benchmarks/client-nav/README.mdbenchmarks/client-nav/package.jsonbenchmarks/client-nav/react/speed.bench.tsxbenchmarks/client-nav/react/tsconfig.jsonbenchmarks/client-nav/react/vitest.config.tsbenchmarks/client-nav/solid/speed.bench.tsxbenchmarks/client-nav/solid/tsconfig.jsonbenchmarks/client-nav/solid/vitest.config.tsbenchmarks/client-nav/tsconfig.jsonbenchmarks/client-nav/vitest.config.tsbenchmarks/client-nav/vitest.setup.tsbenchmarks/client-nav/vue/speed.bench.tsxbenchmarks/client-nav/vue/tsconfig.jsonbenchmarks/client-nav/vue/vitest.config.tse2e/bundle-size/package.jsonpackage.jsonpnpm-workspace.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
benchmarks/client-nav/vue/speed.bench.tsx (1)
93-93: 🛠️ Refactor suggestion | 🟠 MajorDrop
as anyon the root route component.
Please avoid bypassing types here; usecomponent: Rootdirectly.Suggested fix
- component: Root as any, + component: Root,As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/vue/speed.bench.tsx` at line 93, The route entry currently casts the Root component via "component: Root as any" which bypasses type safety; change it to "component: Root" and update the route typing or the Root export so they match (e.g., ensure Root is typed as a Vue component or the route config uses the correct generic/RouteRecordRaw-compatible type). Locate the route object where "component: Root as any" appears, remove the "as any" cast, and if TypeScript errors appear, fix the Root export signature or adjust the route type declaration so Root's actual type is accepted without casting.benchmarks/bundle-size/package.json (1)
24-27:⚠️ Potential issue | 🟠 MajorResolve the Vue plugin/Vite version mismatch.
@vitejs/plugin-vue@^5.2.3is typically paired with Vite 5/6, while this package pinsvite@^7.3.1. Please align these versions to avoid peer-resolution/build issues.What is the peer dependency range for vite in `@vitejs/plugin-vue`@5.2.3 and `@vitejs/plugin-vue-jsx`@4.1.2?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/bundle-size/package.json` around lines 24 - 27, package.json currently mixes "@vitejs/plugin-vue" and "@vitejs/plugin-vue-jsx" versions that target an older Vite with vite@^7.3.1, causing a peer-dependency mismatch; check the peer dependency ranges for `@vitejs/plugin-vue`@5.2.3 and `@vitejs/plugin-vue-jsx`@4.1.2 (via npm info or the registry) and then either (A) upgrade the two plugins to releases whose peerDependencies include vite@^7 (update "@vitejs/plugin-vue" and "@vitejs/plugin-vue-jsx" entries) or (B) downgrade the "vite" version to the range required by the plugin versions; after updating package.json run install and verify with npm ls/pnpm ls to ensure no peer conflicts remain.benchmarks/client-nav/solid/speed.bench.tsx (1)
88-90:⚠️ Potential issue | 🟠 MajorRemove unpaired
console.time('render')from benchmark path.
console.time('render')at Line 89 has no matchingconsole.timeEnd, which adds non-target overhead/noise to navigation measurements.Suggested fix
component: () => { - console.time('render') return <div /> },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/solid/speed.bench.tsx` around lines 88 - 90, The benchmark's component function contains an unpaired console.time('render') call (in the component returned by component: () => { ... }) which skews navigation measurements; remove the standalone console.time('render') from the component (or if measuring, add a matching console.timeEnd('render') in the corresponding lifecycle/after-render hook) so that console.time('render') is either fully paired with console.timeEnd('render') or omitted entirely from the component render path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/client-nav/package.json`:
- Around line 31-35: Update the `@vitejs/plugin-vue` dependency in package.json
from "^5.2.3" to a 6.x release (e.g. "^6.0.0") so it is compatible with
vite@^7.3.1; adjust the "@vitejs/plugin-vue" entry in package.json, then
reinstall dependencies (npm/yarn/pnpm install) and regenerate the lockfile to
resolve the peer dependency conflict; verify project builds using the updated
`@vitejs/plugin-vue` version.
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Line 4: The import rootRouteId is unused and causing lint failures; remove
rootRouteId from the import statement (the import line that currently imports
rootRouteId from '@tanstack/router-core') or replace it by using the identifier
where needed; specifically delete rootRouteId from the import so only actually
used exports remain.
In `@benchmarks/client-nav/solid/speed.bench.tsx`:
- Around line 67-75: Replace the three uses of selectors.map(...) rendering
Params, Search, and Links with Solid's <For> control-flow to comply with Solid
best practices: import For from 'solid-js' (or use the existing import) and
change each block to <For each={selectors}>{() => <Params />}</For>, similarly
for Search and Links, ensuring you pass the items via the each prop (and use the
item/index callback if you need unique keys or item data) instead of using
.map().
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Line 4: The file imports rootRouteId which is unused; remove the unused import
by deleting the import specifier rootRouteId from the import statement (the
import line "import { rootRouteId } from '@tanstack/router-core'") or remove the
entire import if nothing else is imported, ensuring no other references to
rootRouteId remain in the file.
---
Duplicate comments:
In `@benchmarks/bundle-size/package.json`:
- Around line 24-27: package.json currently mixes "@vitejs/plugin-vue" and
"@vitejs/plugin-vue-jsx" versions that target an older Vite with vite@^7.3.1,
causing a peer-dependency mismatch; check the peer dependency ranges for
`@vitejs/plugin-vue`@5.2.3 and `@vitejs/plugin-vue-jsx`@4.1.2 (via npm info or the
registry) and then either (A) upgrade the two plugins to releases whose
peerDependencies include vite@^7 (update "@vitejs/plugin-vue" and
"@vitejs/plugin-vue-jsx" entries) or (B) downgrade the "vite" version to the
range required by the plugin versions; after updating package.json run install
and verify with npm ls/pnpm ls to ensure no peer conflicts remain.
In `@benchmarks/client-nav/solid/speed.bench.tsx`:
- Around line 88-90: The benchmark's component function contains an unpaired
console.time('render') call (in the component returned by component: () => { ...
}) which skews navigation measurements; remove the standalone
console.time('render') from the component (or if measuring, add a matching
console.timeEnd('render') in the corresponding lifecycle/after-render hook) so
that console.time('render') is either fully paired with
console.timeEnd('render') or omitted entirely from the component render path.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Line 93: The route entry currently casts the Root component via "component:
Root as any" which bypasses type safety; change it to "component: Root" and
update the route typing or the Root export so they match (e.g., ensure Root is
typed as a Vue component or the route config uses the correct
generic/RouteRecordRaw-compatible type). Locate the route object where
"component: Root as any" appears, remove the "as any" cast, and if TypeScript
errors appear, fix the Root export signature or adjust the route type
declaration so Root's actual type is accepted without casting.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
.github/workflows/bundle-size.yml.github/workflows/client-nav-benchmarks.ymlbenchmarks/bundle-size/.gitignorebenchmarks/bundle-size/README.mdbenchmarks/bundle-size/package.jsonbenchmarks/bundle-size/results/.gitkeepbenchmarks/bundle-size/scenarios/react-router-full/index.htmlbenchmarks/bundle-size/scenarios/react-router-full/src/main.tsxbenchmarks/bundle-size/scenarios/react-router-full/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/react-router-full/src/routes/index.tsxbenchmarks/bundle-size/scenarios/react-router-full/vite.config.tsbenchmarks/bundle-size/scenarios/react-router-minimal/index.htmlbenchmarks/bundle-size/scenarios/react-router-minimal/src/main.tsxbenchmarks/bundle-size/scenarios/react-router-minimal/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/react-router-minimal/src/routes/index.tsxbenchmarks/bundle-size/scenarios/react-router-minimal/vite.config.tsbenchmarks/bundle-size/scenarios/react-start-full/src/router.tsxbenchmarks/bundle-size/scenarios/react-start-full/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/react-start-full/src/routes/index.tsxbenchmarks/bundle-size/scenarios/react-start-full/vite.config.tsbenchmarks/bundle-size/scenarios/react-start-minimal/src/router.tsxbenchmarks/bundle-size/scenarios/react-start-minimal/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/react-start-minimal/src/routes/index.tsxbenchmarks/bundle-size/scenarios/react-start-minimal/vite.config.tsbenchmarks/bundle-size/scenarios/solid-router-full/index.htmlbenchmarks/bundle-size/scenarios/solid-router-full/src/main.tsxbenchmarks/bundle-size/scenarios/solid-router-full/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/solid-router-full/src/routes/index.tsxbenchmarks/bundle-size/scenarios/solid-router-full/vite.config.tsbenchmarks/bundle-size/scenarios/solid-router-minimal/index.htmlbenchmarks/bundle-size/scenarios/solid-router-minimal/src/main.tsxbenchmarks/bundle-size/scenarios/solid-router-minimal/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/solid-router-minimal/src/routes/index.tsxbenchmarks/bundle-size/scenarios/solid-router-minimal/vite.config.tsbenchmarks/bundle-size/scenarios/solid-start-full/src/router.tsxbenchmarks/bundle-size/scenarios/solid-start-full/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/solid-start-full/src/routes/index.tsxbenchmarks/bundle-size/scenarios/solid-start-full/vite.config.tsbenchmarks/bundle-size/scenarios/solid-start-minimal/src/router.tsxbenchmarks/bundle-size/scenarios/solid-start-minimal/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/solid-start-minimal/src/routes/index.tsxbenchmarks/bundle-size/scenarios/solid-start-minimal/vite.config.tsbenchmarks/bundle-size/scenarios/vue-router-full/index.htmlbenchmarks/bundle-size/scenarios/vue-router-full/src/main.tsxbenchmarks/bundle-size/scenarios/vue-router-full/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/vue-router-full/src/routes/index.tsxbenchmarks/bundle-size/scenarios/vue-router-full/vite.config.tsbenchmarks/bundle-size/scenarios/vue-router-minimal/index.htmlbenchmarks/bundle-size/scenarios/vue-router-minimal/src/main.tsxbenchmarks/bundle-size/scenarios/vue-router-minimal/src/routes/__root.tsxbenchmarks/bundle-size/scenarios/vue-router-minimal/src/routes/index.tsxbenchmarks/bundle-size/scenarios/vue-router-minimal/vite.config.tsbenchmarks/bundle-size/tsconfig.jsonbenchmarks/client-nav/package.jsonbenchmarks/client-nav/react/speed.bench.tsxbenchmarks/client-nav/react/vitest.config.tsbenchmarks/client-nav/solid/speed.bench.tsxbenchmarks/client-nav/solid/vitest.config.tsbenchmarks/client-nav/vue/speed.bench.tsxbenchmarks/client-nav/vue/vitest.config.tspackage.jsonpnpm-workspace.yamlscripts/benchmarks/bundle-size/measure.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- benchmarks/client-nav/react/vitest.config.ts
- pnpm-workspace.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
benchmarks/client-nav/react/speed.bench.tsx (1)
109-116: NormalizenextIdto string for cross-framework comparability.Line 110 uses numeric IDs, while the Solid benchmark uses string IDs. Normalizing this removes inconsistent coercion cost from framework comparisons.
🔧 Suggested fix
- const nextId = id++ + const nextId = String(id++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/speed.bench.tsx` around lines 109 - 116, The benchmark's next function uses a numeric nextId which differs from Solid's string IDs; change the construction of nextId inside next (where id is incremented) to normalize it to a string (e.g., use String(id++) or id.toString()) and ensure the same string-valued nextId is passed into navigate's params and search in the next function so both params and search use string IDs for consistent cross-framework comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Around line 129-137: The suite currently registers lifecycle twice: remove
per-benchmark lifecycle from the bench options so setup/teardown run only once
via the suite-level beforeAll and afterAll; specifically, keep beforeAll(setup)
and afterAll(teardown) as-is and delete the setup and teardown properties from
the bench('navigate', () => act(next), { ... }) options (which reference setup
and teardown), leaving warmupIterations, time, and the benchmark callback
(act(next)) intact to avoid repeated expensive mounts.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Around line 143-151: The bench call is passing setup/teardown in its options
while the suite already runs beforeAll(setup) and afterAll(teardown), causing
setup to run per iteration; remove setup and teardown from the bench options in
the bench('navigate', () => next(), {...}) invocation so only warmupIterations
and time (and any other per-benchmark settings) remain, leaving beforeAll(setup)
and afterAll(teardown) to handle one-time initialization/cleanup for the suite
and keeping the next() benchmark body unchanged.
---
Nitpick comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Around line 109-116: The benchmark's next function uses a numeric nextId which
differs from Solid's string IDs; change the construction of nextId inside next
(where id is incremented) to normalize it to a string (e.g., use String(id++) or
id.toString()) and ensure the same string-valued nextId is passed into
navigate's params and search in the next function so both params and search use
string IDs for consistent cross-framework comparison.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/client-nav-benchmarks.ymlbenchmarks/client-nav/react/speed.bench.tsxbenchmarks/client-nav/react/vitest.config.tsbenchmarks/client-nav/solid/speed.bench.tsxbenchmarks/client-nav/solid/vitest.config.tsbenchmarks/client-nav/vue/speed.bench.tsxbenchmarks/client-nav/vue/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/client-nav/solid/vitest.config.ts
- .github/workflows/client-nav-benchmarks.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
benchmarks/client-nav/vue/speed.bench.tsx (1)
143-151:⚠️ Potential issue | 🟠 MajorRemove duplicated benchmark lifecycle wiring.
Line 143 and Line 144 already handle one-time suite lifecycle. Re-registering
setup/teardownin Line 149 and Line 150 runs mount/subscription work per benchmark cycle, which skews the navigation benchmark.Suggested fix
bench('navigate', () => next(), { warmupIterations: 1000, time: 10_000, - setup, - teardown, })For Vitest v4, does `bench(name, fn, { setup, teardown })` run `setup`/`teardown` for each benchmark cycle, while `beforeAll`/`afterAll` run once per suite?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/vue/speed.bench.tsx` around lines 143 - 151, The test registers suite-level lifecycle twice: beforeAll(setup)/afterAll(teardown) and again in bench('navigate', ..., { setup, teardown }); remove the per-benchmark setup/teardown from the bench call so the bench uses the suite-level beforeAll/afterAll; locate the bench invocation named "navigate" and delete the setup and teardown properties from its options (or alternatively replace suite-level beforeAll/afterAll with beforeEach/afterEach if you actually want per-iteration mounting, but do not keep both).benchmarks/client-nav/react/speed.bench.tsx (1)
129-137:⚠️ Potential issue | 🟠 MajorDrop per-benchmark
setup/teardownto avoid measurement skew.Line 129 and Line 130 already run suite lifecycle once. Keeping
setup/teardownin Line 135 and Line 136 makes render/cleanup execute every cycle and distorts the benchmark.Suggested fix
bench('navigate', () => act(next), { warmupIterations: 1000, time: 10_000, - setup, - teardown, })In Vitest 4, are `setup` and `teardown` passed to `bench()` executed per benchmark cycle (not once per suite)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/speed.bench.tsx` around lines 129 - 137, The per-benchmark options currently pass setup and teardown into bench(...) which causes setup/teardown to run every iteration and skew results; remove setup and teardown from the bench call (leave beforeAll(setup) and afterAll(teardown) as the suite lifecycle) so bench('navigate', () => act(next), { warmupIterations: 1000, time: 10_000 }) runs without per-cycle render/cleanup and preserves accurate timing for the navigate benchmark.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Line 3: The import members from 'vitest' are not alphabetically sorted; update
the import that currently lists bench, describe, beforeAll, afterAll to an
alphabetically ordered list (afterAll, beforeAll, bench, describe) so the
symbols (afterAll, beforeAll, bench, describe) are sorted to satisfy the
sort-imports lint rule.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Line 2: The named imports from vitest are not alphabetically sorted causing
the `sort-imports` lint error; reorder the import specifiers in the import
statement so the members are alphabetized (e.g., afterAll, beforeAll, bench,
describe) to satisfy the linter and keep the import as import { afterAll,
beforeAll, bench, describe } from 'vitest'.
---
Duplicate comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Around line 129-137: The per-benchmark options currently pass setup and
teardown into bench(...) which causes setup/teardown to run every iteration and
skew results; remove setup and teardown from the bench call (leave
beforeAll(setup) and afterAll(teardown) as the suite lifecycle) so
bench('navigate', () => act(next), { warmupIterations: 1000, time: 10_000 })
runs without per-cycle render/cleanup and preserves accurate timing for the
navigate benchmark.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Around line 143-151: The test registers suite-level lifecycle twice:
beforeAll(setup)/afterAll(teardown) and again in bench('navigate', ..., { setup,
teardown }); remove the per-benchmark setup/teardown from the bench call so the
bench uses the suite-level beforeAll/afterAll; locate the bench invocation named
"navigate" and delete the setup and teardown properties from its options (or
alternatively replace suite-level beforeAll/afterAll with beforeEach/afterEach
if you actually want per-iteration mounting, but do not keep both).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
benchmarks/client-nav/vue/speed.bench.tsx (1)
2-2:⚠️ Potential issue | 🟡 MinorSort
vitestnamed imports to keep lint green.Line 2 still violates
sort-imports.Suggested fix
-import { bench, describe, beforeAll, afterAll } from 'vitest' +import { afterAll, beforeAll, bench, describe } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/vue/speed.bench.tsx` at line 2, The named imports from 'vitest' in the import statement (bench, describe, beforeAll, afterAll) are not sorted and trigger the sort-imports lint rule; reorder them alphabetically (afterAll, beforeAll, bench, describe) in the import declaration so the import line for 'vitest' is sorted and the lint error is resolved.benchmarks/client-nav/react/speed.bench.tsx (1)
3-3:⚠️ Potential issue | 🟡 MinorSort
vitestnamed imports to satisfy lint.Line 3 still violates
sort-imports.Suggested fix
-import { bench, describe, beforeAll, afterAll } from 'vitest' +import { afterAll, beforeAll, bench, describe } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/speed.bench.tsx` at line 3, The named imports from vitest are not sorted; reorder the import specifiers for bench, describe, beforeAll, and afterAll into alphabetical order (afterAll, beforeAll, bench, describe) so the import line for vitest satisfies the sort-imports lint rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Around line 30-49: The linter is flagging valid Vue composable usage inside
the setup functions as hook-rule false positives; update the implementations of
Params and Search (the Vue.defineComponent calls with setup using useParams and
useSearch and their select callbacks) to satisfy the hook rule by ensuring the
calls to useParams and useSearch occur unconditionally at the top of the setup
function body (i.e., do not wrap them in conditional logic and avoid late
returns) and keep the select callbacks as pure functions; specifically modify
the setup blocks for Params and Search so useParams(...) and useSearch(...) are
the first statements in setup (refer to the Params and Search component
definitions and the useParams/useSearch usages) to prevent the lint rule from
reporting false positives.
---
Duplicate comments:
In `@benchmarks/client-nav/react/speed.bench.tsx`:
- Line 3: The named imports from vitest are not sorted; reorder the import
specifiers for bench, describe, beforeAll, and afterAll into alphabetical order
(afterAll, beforeAll, bench, describe) so the import line for vitest satisfies
the sort-imports lint rule.
In `@benchmarks/client-nav/vue/speed.bench.tsx`:
- Line 2: The named imports from 'vitest' in the import statement (bench,
describe, beforeAll, afterAll) are not sorted and trigger the sort-imports lint
rule; reorder them alphabetically (afterAll, beforeAll, bench, describe) in the
import declaration so the import line for 'vitest' is sorted and the lint error
is resolved.
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aa595dba3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 835a825c70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
new dedicated
@benchmarks/client-navworkspace package, w/ React, Solid, and Vue "client navigation" benchmarkspnpm nx run @benchmarks/client-nav:test:perf(orpnpm benchmark:client-nav)pnpm nx run @benchmarks/client-nav:test:perf:solidbenchmark details
useParams, 20useSearch, 20Linkon the__root, and navigate many times the/$idroute by changing its params / searchmoved existing "bundle size" benchmark to the same root
/benchmarksfolder, and in a later PR will add an SSR setup toopnpm nx run @benchmarks/bundle-size:build(or still like beforepnpm benchmark:bundle-size)Warning
I couldn't get the Solid version of this new test to work. We could merge without it and get to it later...
Summary by CodeRabbit
New Features
Documentation
Chores