test: refactor client-nav benchmarks for flame profiling#6816
Conversation
📝 WalkthroughWalkthroughRefactors benchmarks/client-nav: adds a shared JSDOM environment, replaces .tsx benches with framework-specific setup modules and Vitest bench + flame scripts for React, Solid, and Vue, updates package.json (flame tooling, scripts, NX targets), and adjusts TypeScript configs to include new files. Changes
Sequence Diagram(s)sequenceDiagram
participant Flame as Flame Runtime
participant Setup as setup()
participant Router as Router
participant Component as Component
participant Window as JSDOM_Window
Flame->>Setup: initialize test harness / call setup()
Setup->>Router: createTestRouter()
Router-->>Setup: router ready
Setup->>Component: render app (mount)
Component-->>Setup: mounted / ready
Flame->>Setup: test.before()
activate Setup
Setup->>Router: subscribe onRendered
Setup->>Component: ready for ticks
deactivate Setup
loop until 10s elapsed
Flame->>Setup: test.tick()
activate Setup
Setup->>Router: navigate to next route
Router->>Component: trigger render
Component-->>Router: render complete
deactivate Setup
end
Flame->>Setup: test.after()
activate Setup
Setup->>Component: unmount / dispose
Setup->>Window: cleanup
Setup->>Window: window.close()
Window-->>Flame: closed
deactivate Setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 17s | View ↗ |
nx run @benchmarks/ssr:test:perf:vue |
✅ Succeeded | 2m 9s | View ↗ |
nx run @benchmarks/ssr:test:perf:solid |
✅ Succeeded | 1m 54s | View ↗ |
nx run @benchmarks/ssr:test:perf:react |
✅ Succeeded | 2m 3s | View ↗ |
nx run @benchmarks/client-nav:test:perf:vue |
✅ Succeeded | 1m 24s | View ↗ |
nx run @benchmarks/client-nav:test:perf:react |
✅ Succeeded | 1m 23s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-03 23:41:55 UTC
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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/client-nav/package.json (1)
31-42:⚠️ Potential issue | 🟡 MinorRemove the unused
@testing-library/reactdependency.The PR objectives remove React/Solid/Vue testing-library dependencies, but
@testing-library/react(line 34) remains in devDependencies. All benchmark setup files use direct DOM/framework APIs (react-dom/client,solid-js/web,vue) without testing-library patterns. This dependency is unused and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/package.json` around lines 31 - 42, Remove the unused `@testing-library/react` entry from the devDependencies block in package.json: locate the "devDependencies" object and delete the "@testing-library/react" key/value so only actually used packages (vite, vitest, typescript, framework plugins like "@vitejs/plugin-react", "vite-plugin-solid", etc.) remain.
🧹 Nitpick comments (4)
benchmarks/client-nav/solid/speed.flame.ts (1)
15-18: Ensurewindow.close()always runs on teardown errors.Line 16 can throw and skip Line 17. Use nested
try/finallyso JSDOM cleanup is guaranteed.♻️ Proposed cleanup hardening
} finally { - test.after() - window.close() + try { + await test.after() + } finally { + window.close() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/solid/speed.flame.ts` around lines 15 - 18, The teardown currently calls test.after() and then window.close() in a single finally block so if test.after() throws, window.close() may be skipped; change to a nested try/finally where you call test.after() inside an inner try and call window.close() in that inner finally (or vice versa) so window.close() is guaranteed to run even when test.after() throws; locate the block containing test.after() and window.close() and wrap test.after() in its own try/finally to ensure window.close() always executes.benchmarks/client-nav/vue/speed.flame.ts (1)
15-18: Harden final cleanup ordering.On Line 16, an exception from
test.after()preventswindow.close()on Line 17. Make close run in an innerfinally.♻️ Proposed cleanup hardening
} finally { - test.after() - window.close() + try { + await test.after() + } finally { + window.close() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/vue/speed.flame.ts` around lines 15 - 18, The finally block can abort window.close() if test.after() throws; change the cleanup so test.after() is called in an outer try and window.close() in an inner finally (i.e., call test.after() inside a try { test.after() } finally { window.close() }) so that window.close() always runs even when test.after() throws; update the block around test.after() and window.close() accordingly.benchmarks/client-nav/react/speed.flame.ts (1)
15-18: Make window cleanup unconditional.If
test.after()throws on Line 16, Line 17 won’t run, leaving JSDOM resources open. Wraptest.after()sowindow.close()always executes.♻️ Proposed cleanup hardening
} finally { - test.after() - window.close() + try { + await test.after() + } finally { + window.close() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/speed.flame.ts` around lines 15 - 18, The finally block currently calls test.after() then window.close() so if test.after() throws the window won't be closed; change the finally to capture any error from test.after() (e.g., let afterErr), call test.after() inside a try/catch that stores the error, always call window.close() in the finally, and rethrow afterErr if present so cleanup is unconditional; reference the existing test.after() and window.close() calls when making the change.benchmarks/client-nav/solid/setup.ts (1)
1-3: Fix import ordering per ESLint rules.The static analysis indicates that the
solid-js/webruntime import should occur before the type-only import of@tanstack/router-core.♻️ Proposed fix for import order
+import { render } from 'solid-js/web' import type { NavigateOptions } from '@tanstack/router-core' -import { render } from 'solid-js/web' import type * as App from './app'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/solid/setup.ts` around lines 1 - 3, Reorder the imports so runtime import comes before type-only imports: place the "import { render } from 'solid-js/web'" line above the "import type { NavigateOptions } from '@tanstack/router-core'" line; keep the "import type * as App from './app'" type import wherever appropriate (after runtime import is fine) to satisfy ESLint import-order rules and preserve the same imported symbols (NavigateOptions, render, App).
🤖 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/setup.ts`:
- Around line 14-16: The router and component are being created once at module
scope via createTestRouter() (variables router and component), causing reuse
across benchmark iterations; move the createTestRouter() call into the setup
lifecycle (match Solid/Vue) so each iteration gets a fresh router: declare let
router and component (and keep id/root as needed) at top, then call ({ router,
component } = createTestRouter()) inside the before/setup function (e.g.,
beforeEach or before), ensuring any existing references in functions like
setup() use the newly assigned router/component for each run.
In `@benchmarks/client-nav/README.md`:
- Line 31: Update the README text that currently reads "10 second loop" to use a
hyphenated compound modifier by changing it to "10-second loop" in the sentence
"Run framework-specific flame benchmarks (10 second loop, profiled with
`@platformatic/flame`, forced to `NODE_ENV=production`)" so the phrase becomes
"10-second loop" for correct grammar.
---
Outside diff comments:
In `@benchmarks/client-nav/package.json`:
- Around line 31-42: Remove the unused `@testing-library/react` entry from the
devDependencies block in package.json: locate the "devDependencies" object and
delete the "@testing-library/react" key/value so only actually used packages
(vite, vitest, typescript, framework plugins like "@vitejs/plugin-react",
"vite-plugin-solid", etc.) remain.
---
Nitpick comments:
In `@benchmarks/client-nav/react/speed.flame.ts`:
- Around line 15-18: The finally block currently calls test.after() then
window.close() so if test.after() throws the window won't be closed; change the
finally to capture any error from test.after() (e.g., let afterErr), call
test.after() inside a try/catch that stores the error, always call
window.close() in the finally, and rethrow afterErr if present so cleanup is
unconditional; reference the existing test.after() and window.close() calls when
making the change.
In `@benchmarks/client-nav/solid/setup.ts`:
- Around line 1-3: Reorder the imports so runtime import comes before type-only
imports: place the "import { render } from 'solid-js/web'" line above the
"import type { NavigateOptions } from '@tanstack/router-core'" line; keep the
"import type * as App from './app'" type import wherever appropriate (after
runtime import is fine) to satisfy ESLint import-order rules and preserve the
same imported symbols (NavigateOptions, render, App).
In `@benchmarks/client-nav/solid/speed.flame.ts`:
- Around line 15-18: The teardown currently calls test.after() and then
window.close() in a single finally block so if test.after() throws,
window.close() may be skipped; change to a nested try/finally where you call
test.after() inside an inner try and call window.close() in that inner finally
(or vice versa) so window.close() is guaranteed to run even when test.after()
throws; locate the block containing test.after() and window.close() and wrap
test.after() in its own try/finally to ensure window.close() always executes.
In `@benchmarks/client-nav/vue/speed.flame.ts`:
- Around line 15-18: The finally block can abort window.close() if test.after()
throws; change the cleanup so test.after() is called in an outer try and
window.close() in an inner finally (i.e., call test.after() inside a try {
test.after() } finally { window.close() }) so that window.close() always runs
even when test.after() throws; update the block around test.after() and
window.close() accordingly.
ℹ️ 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/jsdom.tsbenchmarks/client-nav/package.jsonbenchmarks/client-nav/react/setup.tsbenchmarks/client-nav/react/speed.bench.tsbenchmarks/client-nav/react/speed.bench.tsxbenchmarks/client-nav/react/speed.flame.tsbenchmarks/client-nav/react/tsconfig.jsonbenchmarks/client-nav/solid/setup.tsbenchmarks/client-nav/solid/speed.bench.tsbenchmarks/client-nav/solid/speed.bench.tsxbenchmarks/client-nav/solid/speed.flame.tsbenchmarks/client-nav/solid/tsconfig.jsonbenchmarks/client-nav/vue/setup.tsbenchmarks/client-nav/vue/speed.bench.tsbenchmarks/client-nav/vue/speed.flame.tsbenchmarks/client-nav/vue/tsconfig.json
💤 Files with no reviewable changes (2)
- benchmarks/client-nav/react/speed.bench.tsx
- benchmarks/client-nav/solid/speed.bench.tsx
![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: b996126da1
ℹ️ 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".
| "@vitejs/plugin-react": "^4.3.4", | ||
| "@vitejs/plugin-vue": "^5.2.3", | ||
| "@vitejs/plugin-vue-jsx": "^4.1.2", | ||
| "@types/jsdom": "28.0.0", |
There was a problem hiding this comment.
The new flame harness now imports jsdom at runtime (benchmarks/client-nav/jsdom.ts), but this package only adds @types/jsdom here and never declares jsdom itself. This works only as long as a parent workspace happens to provide jsdom; in filtered/isolated installs (for example, running only @benchmarks/client-nav tasks) the flame scripts can fail with ERR_MODULE_NOT_FOUND when loading ../jsdom.ts.
Useful? React with 👍 / 👎.
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/setup.ts`:
- Around line 1-2: The imports are out of order per ESLint import/order: move
the value import "render" so it appears before the type-only import
"NavigateOptions"; update the import statements involving NavigateOptions and
render in benchmarks/client-nav/solid/setup.ts so the line importing render from
'solid-js/web' is placed above the line importing type { NavigateOptions } from
'@tanstack/router-core'.
- Line 49: Remove the unnecessary "as any" cast from the render call: update the
dispose assignment that calls render(() => component() as any, container) to
call render(() => component(), container) so the RouterProvider element returned
by component() keeps its proper typing; modify the expression where dispose is
assigned (the render(...) call using component() and container) to drop the cast
and rely on existing types.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
benchmarks/client-nav/react/setup.tsbenchmarks/client-nav/solid/setup.tsbenchmarks/client-nav/vue/setup.ts
✅ Files skipped from review due to trivial changes (1)
- benchmarks/client-nav/react/setup.ts
| import type { NavigateOptions } from '@tanstack/router-core' | ||
| import { render } from 'solid-js/web' |
There was a problem hiding this comment.
Fix import order to satisfy ESLint import/order.
Line 2 should be ordered before the type import on Line 1 to match the enforced rule.
Suggested diff
-import type { NavigateOptions } from '@tanstack/router-core'
import { render } from 'solid-js/web'
+import type { NavigateOptions } from '@tanstack/router-core'📝 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.
| import type { NavigateOptions } from '@tanstack/router-core' | |
| import { render } from 'solid-js/web' | |
| import { render } from 'solid-js/web' | |
| import type { NavigateOptions } from '@tanstack/router-core' |
🧰 Tools
🪛 ESLint
[error] 2-2: solid-js/web import should occur before type import of @tanstack/router-core
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/client-nav/solid/setup.ts` around lines 1 - 2, The imports are out
of order per ESLint import/order: move the value import "render" so it appears
before the type-only import "NavigateOptions"; update the import statements
involving NavigateOptions and render in benchmarks/client-nav/solid/setup.ts so
the line importing render from 'solid-js/web' is placed above the line importing
type { NavigateOptions } from '@tanstack/router-core'.
|
|
||
| container = document.createElement('div') | ||
| document.body.append(container) | ||
| dispose = render(() => component() as any, container) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the typing of component and createTestRouter
rg -n -C3 'export function createTestRouter|component' benchmarks/client-nav/solid/app.ts benchmarks/client-nav/solid/setup.tsRepository: TanStack/router
Length of output: 1098
🏁 Script executed:
#!/bin/bash
# Find createTestRouter definition
rg -n 'export.*createTestRouter|function createTestRouter' benchmarks/client-nav/solid/Repository: TanStack/router
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Read the createTestRouter function implementation
cat -n benchmarks/client-nav/solid/app.tsx | sed -n '83,150p'Repository: TanStack/router
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Check imports and render usage in setup.ts
head -n 30 benchmarks/client-nav/solid/setup.tsRepository: TanStack/router
Length of output: 1055
🏁 Script executed:
#!/bin/bash
# Check if there are type issues or if RouterProvider might not be properly typed
# Look for imports and RouterProvider definition
rg -n 'RouterProvider|from.*router' benchmarks/client-nav/solid/app.tsx | head -20Repository: TanStack/router
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Check TypeScript configuration for the benchmark
find benchmarks/client-nav/solid -name "tsconfig.json" -o -name "tsconfig*.json"Repository: TanStack/router
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check TypeScript configuration
cat -n benchmarks/client-nav/solid/tsconfig.jsonRepository: TanStack/router
Length of output: 546
🏁 Script executed:
#!/bin/bash
# Check package.json to see `@tanstack/solid-router` version and typing
cat benchmarks/client-nav/solid/package.json 2>/dev/null || \
cat package.json | grep -A 5 -B 5 "solid-router"Repository: TanStack/router
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check RouterProvider typing in solid-router
rg -n 'export.*RouterProvider|const RouterProvider' packages/solid-router/src/ -A 5Repository: TanStack/router
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Check what solid-js/web render expects
rg -n 'function render|export.*render' packages/ -A 3 | grep -A 3 'solid-js/web' | head -20Repository: TanStack/router
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Let's check the actual render signature expectations by looking at how
# Solid typically types render and what component type it expects
# Also check if there are other uses of render in the codebase without `as any`
rg -n 'render\(' benchmarks/client-nav/solid/ --type ts --type tsxRepository: TanStack/router
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Check other render usages in benchmarks and e2e tests
rg -n 'render\(' benchmarks/ e2e/ -A 1 | head -40Repository: TanStack/router
Length of output: 2649
🏁 Script executed:
#!/bin/bash
# Let's verify the complete setup.ts context around line 49
sed -n '45,55p' benchmarks/client-nav/solid/setup.tsRepository: TanStack/router
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Let's check the full app.tsx context to see component definition and any type annotations
sed -n '1,100p' benchmarks/client-nav/solid/app.tsx | head -100Repository: TanStack/router
Length of output: 1829
Remove as any from the render call to strengthen type safety.
Line 49 uses as any unnecessarily. Since component() returns a properly-typed RouterProvider element from @tanstack/solid-router, and other similar code in the codebase (e.g., e2e/solid-router/ and benchmarks/bundle-size/ files) calls render() without this cast, the type cast can be safely removed:
dispose = render(() => component(), container)
This improves type safety in the benchmark harness and aligns with the guideline for strict-mode type checking in TypeScript files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/client-nav/solid/setup.ts` at line 49, Remove the unnecessary "as
any" cast from the render call: update the dispose assignment that calls
render(() => component() as any, container) to call render(() => component(),
container) so the RouterProvider element returned by component() keeps its
proper typing; modify the expression where dispose is assigned (the render(...)
call using component() and container) to drop the cast and rely on existing
types.
1 participant
Summary
setupmodules and.tsbenchmark entriestest:flame:*benchmarks/client-nav/jsdom.tsSummary by CodeRabbit
New Features
Chores