test: broaden client-nav benchmark coverage#7010
Conversation
Exercise fast and rich client-navigation paths across React, Solid, and Vue so performance regressions surface across more router code paths.
📝 WalkthroughWalkthroughAdded a JSDOM MouseEvent global, reduced perf-loop iterations, replaced single Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 31001ee
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
benchmarks/client-nav/react/setup.ts (1)
7-49: Consider extracting shared helpers to reduce cross-framework duplication.The
getRequiredLinkandwaitForRequiredLinkfunctions are copied verbatim acrossreact/setup.ts,solid/setup.ts, andvue/setup.ts. These are framework-agnostic DOM utilities that could be extracted to a shared module likebenchmarks/client-nav/helpers.ts.♻️ Proposed shared module
Create
benchmarks/client-nav/helpers.ts:export function getRequiredLink( container: ParentNode, testId: string, cache?: Map<string, HTMLAnchorElement>, ): HTMLAnchorElement { const cachedLink = cache?.get(testId) if (cachedLink) { return cachedLink } const link = container.querySelector<HTMLAnchorElement>( `[data-testid="${testId}"]`, ) if (!link) { throw new Error(`Unable to find benchmark link: ${testId}`) } cache?.set(testId, link) return link } export async function waitForRequiredLink( container: ParentNode, testId: string, cache?: Map<string, HTMLAnchorElement>, ): Promise<HTMLAnchorElement> { for (let attempt = 0; attempt < 10; attempt++) { const link = container.querySelector<HTMLAnchorElement>( `[data-testid="${testId}"]`, ) if (link) { cache?.set(testId, link) return link } await new Promise<void>((resolve) => { requestAnimationFrame(() => resolve()) }) } return getRequiredLink(container, testId, cache) }Based on learnings: "Separate framework-agnostic core logic from React/Solid bindings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/client-nav/react/setup.ts` around lines 7 - 49, Extract the duplicated DOM utilities getRequiredLink and waitForRequiredLink into a new shared module (e.g., benchmarks/client-nav/helpers.ts) that exports both functions with the same signatures and return types, then replace the local copies in react/setup.ts (and similarly in solid/setup.ts and vue/setup.ts) with imports from that module; ensure the new helpers export types for ParentNode and HTMLAnchorElement usage and update any references to cache or call sites to use the imported getRequiredLink and waitForRequiredLink functions.
🤖 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/app.tsx`:
- Around line 185-197: The declared return type for useLoaderDeps is incorrect:
update its signature so it returns a Vue.Ref wrapping UseLoaderDepsResult (i.e.,
Vue.Ref<UseLoaderDepsResult<TRouter, TFrom, TSelected>>) because the
implementation calls useMatch() and returns a ref; change the type definition
for useLoaderDeps to match useSearch's pattern, removing the need for the local
workaround (the cast to Vue.Ref<number>) and ensure UseLoaderDepsResult,
useLoaderDeps, useMatch and useSearch are referenced in the updated type
signature.
---
Nitpick comments:
In `@benchmarks/client-nav/react/setup.ts`:
- Around line 7-49: Extract the duplicated DOM utilities getRequiredLink and
waitForRequiredLink into a new shared module (e.g.,
benchmarks/client-nav/helpers.ts) that exports both functions with the same
signatures and return types, then replace the local copies in react/setup.ts
(and similarly in solid/setup.ts and vue/setup.ts) with imports from that
module; ensure the new helpers export types for ParentNode and HTMLAnchorElement
usage and update any references to cache or call sites to use the imported
getRequiredLink and waitForRequiredLink functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2317288d-f196-4022-bd1b-18f7c87fa3e3
📒 Files selected for processing (7)
benchmarks/client-nav/jsdom.tsbenchmarks/client-nav/react/app.tsxbenchmarks/client-nav/react/setup.tsbenchmarks/client-nav/solid/app.tsxbenchmarks/client-nav/solid/setup.tsbenchmarks/client-nav/vue/app.tsxbenchmarks/client-nav/vue/setup.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83cbdb7eae
ℹ️ 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".
Merging this PR will degrade performance by 35.47%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| 👁 | client-side navigation loop (solid) |
82.6 ms | 116.3 ms | -28.94% |
| 👁 | client-side navigation loop (react) |
45.6 ms | 61.3 ms | -25.63% |
| 👁 | client-side navigation loop (vue) |
58.6 ms | 90.9 ms | -35.47% |
Comparing opencode/kind-comet (31001ee) with main (c9e1855)
Keep benchmark navigations steady-state across frameworks and align Vue loader deps typing with the ref-based runtime API.
|
Addressed the review feedback in commit 31001ee:
Re-ran:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1-3: The import order violates ESLint: move the value imports
(getRequiredLink, waitForRequiredLink) before the type-only imports;
specifically, place the import of getRequiredLink and waitForRequiredLink ahead
of the type imports for NavigateOptions and App so the file imports read with
value imports first and type imports (NavigateOptions, App) afterward.
In `@benchmarks/client-nav/solid/setup.ts`:
- Around line 1-3: Reorder the imports so runtime/value imports come before
type-only imports: move the import of getRequiredLink and waitForRequiredLink
(from ../setup-helpers) above the type imports for NavigateOptions and App;
ensure the final order is value imports first (getRequiredLink,
waitForRequiredLink) followed by type imports (import type { NavigateOptions }
from '@tanstack/router-core' and import type * as App from './app').
In `@benchmarks/client-nav/vue/setup.ts`:
- Around line 1-3: Reorder the import statements so value imports come before
type-only imports: move the import of getRequiredLink and waitForRequiredLink
from '../setup-helpers' to be above the type imports (NavigateOptions and App)
so that value imports precede type imports; update the import order in the
module containing the getRequiredLink/waitForRequiredLink usage to satisfy
ESLint's import-order rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f69168d-7f1d-46c5-903a-01c849498f14
📒 Files selected for processing (9)
benchmarks/client-nav/react/app.tsxbenchmarks/client-nav/react/setup.tsbenchmarks/client-nav/setup-helpers.tsbenchmarks/client-nav/solid/app.tsxbenchmarks/client-nav/solid/setup.tsbenchmarks/client-nav/vue/app.tsxbenchmarks/client-nav/vue/setup.tspackages/vue-router/src/useLoaderDeps.tsxpackages/vue-router/tests/routeApi.test-d.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- benchmarks/client-nav/vue/app.tsx
| import type { NavigateOptions } from '@tanstack/router-core' | ||
| import type * as App from './app' | ||
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' |
There was a problem hiding this comment.
Fix import order: value imports before type imports.
ESLint reports that ../setup-helpers should be imported before type-only imports.
Proposed fix
+import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'
import type { NavigateOptions } from '@tanstack/router-core'
import type * as App from './app'
-import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'📝 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 type * as App from './app' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import type { NavigateOptions } from '@tanstack/router-core' | |
| import type * as App from './app' |
🧰 Tools
🪛 ESLint
[error] 3-3: ../setup-helpers 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/react/setup.ts` around lines 1 - 3, The import order
violates ESLint: move the value imports (getRequiredLink, waitForRequiredLink)
before the type-only imports; specifically, place the import of getRequiredLink
and waitForRequiredLink ahead of the type imports for NavigateOptions and App so
the file imports read with value imports first and type imports
(NavigateOptions, App) afterward.
| import type { NavigateOptions } from '@tanstack/router-core' | ||
| import type * as App from './app' | ||
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' |
There was a problem hiding this comment.
Fix import order: value imports before type imports.
ESLint reports that ../setup-helpers should be imported before type-only imports.
Proposed fix
+import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'
import type { NavigateOptions } from '@tanstack/router-core'
import type * as App from './app'
-import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'📝 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 type * as App from './app' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import type { NavigateOptions } from '@tanstack/router-core' | |
| import type * as App from './app' |
🧰 Tools
🪛 ESLint
[error] 3-3: ../setup-helpers 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 - 3, Reorder the imports
so runtime/value imports come before type-only imports: move the import of
getRequiredLink and waitForRequiredLink (from ../setup-helpers) above the type
imports for NavigateOptions and App; ensure the final order is value imports
first (getRequiredLink, waitForRequiredLink) followed by type imports (import
type { NavigateOptions } from '@tanstack/router-core' and import type * as App
from './app').
| import type { NavigateOptions } from '@tanstack/router-core' | ||
| import type * as App from './app' | ||
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' |
There was a problem hiding this comment.
Fix import order: value imports before type imports.
ESLint reports that ../setup-helpers should be imported before type-only imports.
Proposed fix
+import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'
import type { NavigateOptions } from '@tanstack/router-core'
import type * as App from './app'
-import { getRequiredLink, waitForRequiredLink } from '../setup-helpers'📝 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 type * as App from './app' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import { getRequiredLink, waitForRequiredLink } from '../setup-helpers' | |
| import type { NavigateOptions } from '@tanstack/router-core' | |
| import type * as App from './app' |
🧰 Tools
🪛 ESLint
[error] 3-3: ../setup-helpers 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/vue/setup.ts` around lines 1 - 3, Reorder the import
statements so value imports come before type-only imports: move the import of
getRequiredLink and waitForRequiredLink from '../setup-helpers' to be above the
type imports (NavigateOptions and App) so that value imports precede type
imports; update the import order in the module containing the
getRequiredLink/waitForRequiredLink usage to satisfy ESLint's import-order rule.
Summary
MouseEventto the shared jsdom benchmark environmentTesting
Summary by CodeRabbit
Tests
New Features
Chores
Chores