perf: expand benchmark coverage to ~68 BaseUI components#31
Merged
Conversation
Add mount, rerender, and mountMany benchmarks for TreeList, CommandList, BasicList, SelectableList, and RowList components.
📝 WalkthroughWalkthroughAdds a comprehensive performance benchmarking suite under packages/benchmarks: many per-component Vitest bench files, Reassure perf tests, shared data factories and tiered bench options, jsdom fallbacks, perf scripts/config, and utilities to run and compare baseline/current runs. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/benchmarks/src/base-ui/Breadcrumbs.bench.tsx`:
- Around line 6-38: The benchmark currently reuses prebuilt JSX nodes
(threeItems and fourItems) which skips allocation cost; move construction of
Breadcrumbs.Item elements into each timed case instead: remove the top-level
threeItems/fourItems constants and create the children inline inside the
benchRender callback and inside the benchRerender initialProps/updatedProps
builders so that benchRender and benchRerender measure per-run JSX allocation
for <Breadcrumbs> children; update references to threeItems/fourItems to the
newly created inline children in the bench cases (symbols: threeItems,
fourItems, benchRender, benchRerender, Breadcrumbs).
In `@packages/benchmarks/src/base-ui/Checkbox.bench.tsx`:
- Line 14: Reduce the heavy Tier 2 benchmark by lowering the instance count
passed to benchMountMany in Checkbox.bench.tsx: modify the call to
benchMountMany('mount 1000', 1000, (i) => <Checkbox key={i} />, TIER_2_OPTIONS)
to use a smaller number (e.g., match TagInput's 100 or a reasonable Tier 2 value
like 200) and update the description string accordingly; ensure you only change
the numeric count and the human-readable label while keeping the same
benchMountMany function and TIER_2_OPTIONS.
In `@packages/benchmarks/src/base-ui/Collapsible.bench.tsx`:
- Around line 21-22: The initialProps and updatedProps object literals use
redundant type assertions ("open: false as boolean" and "open: true as
boolean"); remove the "as boolean" casts and leave the boolean literals (e.g.,
"open: false" and "open: true") in the initialProps/updatedProps definitions
inside Collapsible.bench (look for the initialProps and updatedProps symbols) to
simplify types without changing behavior.
In `@packages/benchmarks/src/base-ui/Combobox.bench.tsx`:
- Around line 13-33: The Combobox in the benchmark never opens so the Popup/List
children are not mounted; update the Combobox.Root used in these benchmarks to
include the defaultOpen prop so the popup is mounted by default and the full
option list (Combobox.Popup, Combobox.List, Combobox.Item) is rendered for the
100/200/300 option benchmarks; add defaultOpen to the Combobox.Root declaration
that wraps Combobox.Input/Trigger/Portal so the benchmarks measure actual
option-list rendering.
In `@packages/benchmarks/src/base-ui/DataTable.bench.tsx`:
- Around line 54-58: The benchmark currently slices rows100 inside the closure
passed to benchMountMany, causing a new 10-row array allocation on every
iteration; precompute the 10-row fixture once before calling benchMountMany
(e.g., const tenRows = rows100.slice(0, 10)) and pass tenRows into the closure
so DataTableBench receives the precomputed fixture instead of creating it
per-iteration; update the call sites referencing benchMountMany and
DataTableBench to use that tenRows variable.
In `@packages/benchmarks/src/base-ui/DescriptionList.bench.tsx`:
- Around line 7-39: The benchmark prebuilds React fragments threeItems/fourItems
at module load which skips JSX allocation and skews results; instead construct
the DescriptionList.Item fragments inline inside each benchmark case—replace
uses of threeItems/fourItems in benchRender and in benchRerender's
initialProps/updatedProps with freshly created JSX fragments (the same JSX
currently in threeItems/fourItems) so each run allocates the item tree; update
references in the benchRender/benchRerender calls that pass children to
DescriptionList accordingly and remove the module-level threeItems/fourItems
constants.
In `@packages/benchmarks/src/base-ui/DockLayout.bench.tsx`:
- Around line 6-55: The prebuilt DockNode fixtures twoPanel and
threePanelVertical (including their content JSX) are created at module load,
skewing benchmark results; change them into factory functions (e.g.,
makeTwoPanel() and makeThreePanelVertical()) or construct the DockNode trees
inline inside the measured render/callback so all JSX (content: <div>...</div>)
is created during the timed render path; update any references to
twoPanel/threePanelVertical to call the factories to ensure fixture creation
cost is included in the benchmark.
In `@packages/benchmarks/src/base-ui/Drawer.bench.tsx`:
- Around line 19-30: The rerender benchmark uses benchRerender rendering
<Drawer> with animations enabled, causing variability; update the render
callback to pass animate={false} to the Drawer component (the instance created
inside the benchRerender callback that currently renders <Drawer
open={props.open} onOpenChange={noop} anchor="bottom" defaultSize={320}>), so
the open→closed rerender uses the same non-animated behavior as the mount-many
case.
In `@packages/benchmarks/src/base-ui/EditableList.bench.tsx`:
- Around line 9-65: Extract the duplicated three-item EditableList JSX into a
shared helper or fixture (e.g., a function renderThreeItems or constant
ITEMS_FIXTURE) and reuse it in both benchRender('mount with items', ...) and
benchRerender('editable toggle', ...). Replace the inline EditableList tree (the
repeated EditableList with EditableList.Item, EditableList.ItemView,
EditableList.ItemEditor, and EditableList.ItemField nodes and
noop/TIER_2_OPTIONS usage) with a single call/reference to the shared helper so
both benchmarks render the exact same item tree.
In `@packages/benchmarks/src/base-ui/EditorTabs.bench.tsx`:
- Around line 4-15: Remove the local makeTabs helper and instead import and use
the shared factory makeEditorTabs (which returns TabDescriptor[]) from the
benchmark factories module; replace calls to makeTabs(20) and makeTabs(40) with
makeEditorTabs(20) and makeEditorTabs(40), and remove the makeTabs function
declaration so tab fixture generation is centralized (references: makeTabs,
makeEditorTabs, TabDescriptor, tabs20, tabs40, EditorTabs).
In `@packages/benchmarks/src/base-ui/Menu.bench.tsx`:
- Around line 7-24: The mount benchmark for Menu currently measures the
closed-state trigger only; update the benchRender call in Menu.bench.tsx to
either add defaultOpen to the rendered <Menu> (so the Portal/Positioner/Popup
subtree is mounted and the test matches the Popover "mount (open)" case) or
rename the benchmark label to explicitly state it measures the closed-state
mount; modify the JSX passed to benchRender (the <Menu> element) and/or the
first argument label string accordingly (referencing Menu, Menu.Trigger,
Menu.Portal, Menu.Positioner, and Menu.Popup).
In `@packages/benchmarks/src/base-ui/NavList.bench.tsx`:
- Around line 27-30: Remove the unnecessary "as string" type assertions on the
string literals assigned to activeKey inside the initialProps and updatedProps
objects; update the objects (initialProps: { activeKey: 'home' } and
updatedProps: { activeKey: 'settings' }) so the literals are used directly
without casting, keeping the same shape and types for the NavList bench setup.
In `@packages/benchmarks/src/base-ui/Pagination.bench.tsx`:
- Around line 18-19: Remove the redundant "as number" type assertions on the
page values in the bench config: update the initialProps and updatedProps
objects (keys initialProps and updatedProps in Pagination.bench.tsx) to use
plain numeric literals (e.g., page: 1 and page: 5) instead of "page: 1 as
number" / "page: 5 as number".
In `@packages/benchmarks/src/base-ui/ScrollArea.bench.tsx`:
- Line 14: Remove the inline style prop on the ScrollArea instances (e.g., the
<ScrollArea style={{ height: 200 }}> occurrences) and instead apply the fixed
height via a CSS-module fixture class or the approved data-attribute styling
approach; update the JSX to remove the style prop and add the fixture class or
data-attribute name, and add the corresponding rule in the component's CSS
module so both occurrences are lint-compliant.
In `@packages/benchmarks/src/base-ui/SearchInput.bench.tsx`:
- Around line 7-9: The benchmarks recreate an inline noop for SearchInput's
onValueChange on each iteration, adding callback identity churn; define a stable
module-level noop (e.g., const NOOP_ON_VALUE_CHANGE = () => {}) and use that
identifier in both benchRender calls instead of the inline () => {} so
onValueChange remains referentially stable across renders; update the tests that
render <SearchInput placeholder="Search..." value="" onValueChange={...} /> and
any other instances in this file (lines ~11-16) to pass NOOP_ON_VALUE_CHANGE.
In `@packages/benchmarks/src/base-ui/Timeline.bench.tsx`:
- Around line 7-39: The benchmark reuses prebuilt fragments (threeItems and
fourItems), undercounting work; change the bench cases so they construct the
Timeline children per iteration instead of referencing the shared fragments —
i.e., inline-create the JSX for Timeline.Item in the benchRender and
benchRerender callbacks (replace references to threeItems/fourItems with fresh
JSX each call) so that Timeline, benchRender, and benchRerender see newly built
fragments on every iteration.
In `@packages/benchmarks/src/base-ui/ToggleButton.bench.tsx`:
- Around line 15-18: Remove the redundant type assertions on the boolean
literals in the props object: in ToggleButton.bench.tsx update the object with
keys initialProps and updatedProps (the pressed property) to use plain
true/false literals instead of "false as boolean" and "true as boolean"; ensure
only the pressed property name is used as a boolean literal so TypeScript can
infer the type without the casts.
In `@packages/benchmarks/src/base-ui/Toolbar.bench.tsx`:
- Around line 23-26: The test's prop setup uses a redundant type cast: remove
the unnecessary "as undefined" on initialProps ('aria-disabled') and simply set
it to undefined, and if a boolean disabled prop exists on the component prefer
using that (e.g., initialProps: { disabled: false } / updatedProps: { disabled:
true }) instead of string 'true'; update the objects named initialProps and
updatedProps in Toolbar.bench.tsx accordingly and keep the updatedProps 'true'
value as either a boolean true or the string 'true' consistently without the
redundant "as undefined" cast.
In `@packages/benchmarks/src/utils/bench-options.ts`:
- Around line 17-21: TIER_1_OPTIONS duplicates DEFAULT_OPTIONS; replace the
duplicate object with a reference to DEFAULT_OPTIONS to avoid duplication and
keep them in sync (either assign TIER_1_OPTIONS = DEFAULT_OPTIONS or use a
shallow copy like TIER_1_OPTIONS: BenchRenderOptions = { ...DEFAULT_OPTIONS }),
updating the symbol TIER_1_OPTIONS and ensuring BenchRenderOptions typing
remains correct.
In `@packages/benchmarks/src/utils/factories.ts`:
- Around line 32-40: The current makeTreeNodes (and its inner build) treats
`count` as per-level breadth causing exponential growth; replace the recursive
build with a breadth-first generator that respects a total-node budget: keep a
created counter, start with the root node, use a queue to pop nodes and attach
up to N children until created >= count or depth reached, and only enqueue
children when depth allows; update references to makeTreeNodes and
remove/replace the inner build function accordingly so the total returned nodes
never exceed the `count` parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc805e72-80c0-478e-a31c-d6db249aa4d2
📒 Files selected for processing (76)
packages/benchmarks/src/base-ui/Accordion.bench.tsxpackages/benchmarks/src/base-ui/ActionList.bench.tsxpackages/benchmarks/src/base-ui/AlertDialog.bench.tsxpackages/benchmarks/src/base-ui/AppShell.bench.tsxpackages/benchmarks/src/base-ui/Autocomplete.bench.tsxpackages/benchmarks/src/base-ui/Banner.bench.tsxpackages/benchmarks/src/base-ui/BasicList.bench.tsxpackages/benchmarks/src/base-ui/Box.bench.tsxpackages/benchmarks/src/base-ui/Breadcrumbs.bench.tsxpackages/benchmarks/src/base-ui/Button.bench.tsxpackages/benchmarks/src/base-ui/ButtonGroup.bench.tsxpackages/benchmarks/src/base-ui/Card.bench.tsxpackages/benchmarks/src/base-ui/Checkbox.bench.tsxpackages/benchmarks/src/base-ui/CheckboxGroup.bench.tsxpackages/benchmarks/src/base-ui/Chip.bench.tsxpackages/benchmarks/src/base-ui/ClipboardText.bench.tsxpackages/benchmarks/src/base-ui/CodeBlock.bench.tsxpackages/benchmarks/src/base-ui/Collapsible.bench.tsxpackages/benchmarks/src/base-ui/Combobox.bench.tsxpackages/benchmarks/src/base-ui/CommandList.bench.tsxpackages/benchmarks/src/base-ui/ConfirmButton.bench.tsxpackages/benchmarks/src/base-ui/ContextMenu.bench.tsxpackages/benchmarks/src/base-ui/DataTable.bench.tsxpackages/benchmarks/src/base-ui/DescriptionList.bench.tsxpackages/benchmarks/src/base-ui/Dialog.bench.tsxpackages/benchmarks/src/base-ui/DockLayout.bench.tsxpackages/benchmarks/src/base-ui/Drawer.bench.tsxpackages/benchmarks/src/base-ui/EditableList.bench.tsxpackages/benchmarks/src/base-ui/EditorTabs.bench.tsxpackages/benchmarks/src/base-ui/EmptyState.bench.tsxpackages/benchmarks/src/base-ui/FilterBar.bench.tsxpackages/benchmarks/src/base-ui/FindBar.bench.tsxpackages/benchmarks/src/base-ui/FormField.bench.tsxpackages/benchmarks/src/base-ui/IconButton.bench.tsxpackages/benchmarks/src/base-ui/Input.bench.tsxpackages/benchmarks/src/base-ui/List.bench.tsxpackages/benchmarks/src/base-ui/Menu.bench.tsxpackages/benchmarks/src/base-ui/Meter.bench.tsxpackages/benchmarks/src/base-ui/MultiSelect.bench.tsxpackages/benchmarks/src/base-ui/NavList.bench.tsxpackages/benchmarks/src/base-ui/NumberInput.bench.tsxpackages/benchmarks/src/base-ui/Pagination.bench.tsxpackages/benchmarks/src/base-ui/Popover.bench.tsxpackages/benchmarks/src/base-ui/Progress.bench.tsxpackages/benchmarks/src/base-ui/Radio.bench.tsxpackages/benchmarks/src/base-ui/RadioGroup.bench.tsxpackages/benchmarks/src/base-ui/ResizableSplitPane.bench.tsxpackages/benchmarks/src/base-ui/RowList.bench.tsxpackages/benchmarks/src/base-ui/ScrollArea.bench.tsxpackages/benchmarks/src/base-ui/SearchInput.bench.tsxpackages/benchmarks/src/base-ui/SegmentedControl.bench.tsxpackages/benchmarks/src/base-ui/Select.bench.tsxpackages/benchmarks/src/base-ui/SelectableList.bench.tsxpackages/benchmarks/src/base-ui/Sheet.bench.tsxpackages/benchmarks/src/base-ui/Slider.bench.tsxpackages/benchmarks/src/base-ui/Spinner.bench.tsxpackages/benchmarks/src/base-ui/SplitButton.bench.tsxpackages/benchmarks/src/base-ui/StatRow.bench.tsxpackages/benchmarks/src/base-ui/StatusBar.bench.tsxpackages/benchmarks/src/base-ui/Stepper.bench.tsxpackages/benchmarks/src/base-ui/Switch.bench.tsxpackages/benchmarks/src/base-ui/Table.bench.tsxpackages/benchmarks/src/base-ui/Tabs.bench.tsxpackages/benchmarks/src/base-ui/TagInput.bench.tsxpackages/benchmarks/src/base-ui/TextArea.bench.tsxpackages/benchmarks/src/base-ui/TextField.bench.tsxpackages/benchmarks/src/base-ui/Timeline.bench.tsxpackages/benchmarks/src/base-ui/Toast.bench.tsxpackages/benchmarks/src/base-ui/ToggleButton.bench.tsxpackages/benchmarks/src/base-ui/ToggleButtonGroup.bench.tsxpackages/benchmarks/src/base-ui/Toolbar.bench.tsxpackages/benchmarks/src/base-ui/Tooltip.bench.tsxpackages/benchmarks/src/base-ui/TreeList.bench.tsxpackages/benchmarks/src/setup.tspackages/benchmarks/src/utils/bench-options.tspackages/benchmarks/src/utils/factories.ts
Comment on lines
+6
to
+38
| const threeItems = ( | ||
| <> | ||
| <Breadcrumbs.Item href="/home">Home</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item href="/docs">Docs</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item active>Current</Breadcrumbs.Item> | ||
| </> | ||
| ); | ||
|
|
||
| const fourItems = ( | ||
| <> | ||
| <Breadcrumbs.Item href="/home">Home</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item href="/docs">Docs</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item href="/api">API</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item active>Current</Breadcrumbs.Item> | ||
| </> | ||
| ); | ||
|
|
||
| describe('Breadcrumbs', () => { | ||
| benchRender( | ||
| 'mount with 3 items', | ||
| () => <Breadcrumbs>{threeItems}</Breadcrumbs>, | ||
| TIER_2_OPTIONS, | ||
| ); | ||
|
|
||
| benchRerender( | ||
| 'items change', | ||
| { | ||
| initialProps: { children: threeItems }, | ||
| updatedProps: { children: fourItems }, | ||
| }, | ||
| (props) => <Breadcrumbs {...props} />, | ||
| TIER_2_OPTIONS, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Build the breadcrumb items inside the timed cases.
threeItems/fourItems are prebuilt once, so this benchmark excludes the child JSX allocation work that many of the other suites still measure. Generating the items per case will keep these numbers more comparable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/Breadcrumbs.bench.tsx` around lines 6 - 38,
The benchmark currently reuses prebuilt JSX nodes (threeItems and fourItems)
which skips allocation cost; move construction of Breadcrumbs.Item elements into
each timed case instead: remove the top-level threeItems/fourItems constants and
create the children inline inside the benchRender callback and inside the
benchRerender initialProps/updatedProps builders so that benchRender and
benchRerender measure per-run JSX allocation for <Breadcrumbs> children; update
references to threeItems/fourItems to the newly created inline children in the
bench cases (symbols: threeItems, fourItems, benchRender, benchRerender,
Breadcrumbs).
Comment on lines
+54
to
+58
| benchMountMany( | ||
| 'mount 50 tables (10 rows each)', | ||
| 50, | ||
| (i) => <DataTableBench key={i} data={rows100.slice(0, 10)} />, | ||
| TIER_1_OPTIONS, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Precompute the 10-row fixture outside the measured closure.
Line 57 allocates a new array for every mountMany instance on every iteration, so this benchmark includes fixture setup noise in the measured path.
Proposed fix
const rows100 = makeRows(100);
const rows500 = makeRows(500);
const rows600 = makeRows(600);
+const rows10 = rows100.slice(0, 10);
@@
benchMountMany(
'mount 50 tables (10 rows each)',
50,
- (i) => <DataTableBench key={i} data={rows100.slice(0, 10)} />,
+ (i) => <DataTableBench key={i} data={rows10} />,
TIER_1_OPTIONS,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/DataTable.bench.tsx` around lines 54 - 58,
The benchmark currently slices rows100 inside the closure passed to
benchMountMany, causing a new 10-row array allocation on every iteration;
precompute the 10-row fixture once before calling benchMountMany (e.g., const
tenRows = rows100.slice(0, 10)) and pass tenRows into the closure so
DataTableBench receives the precomputed fixture instead of creating it
per-iteration; update the call sites referencing benchMountMany and
DataTableBench to use that tenRows variable.
Comment on lines
+17
to
+21
| /** Tier 1: deep benchmarks (mount + rerender + mountMany). */ | ||
| export const TIER_1_OPTIONS: BenchRenderOptions = { | ||
| iterations: 100, | ||
| warmupIterations: 5, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reusing DEFAULT_OPTIONS for TIER_1_OPTIONS.
TIER_1_OPTIONS has identical values to DEFAULT_OPTIONS. You could reference the default to reduce duplication and ensure they stay in sync.
♻️ Suggested simplification
/** Tier 1: deep benchmarks (mount + rerender + mountMany). */
-export const TIER_1_OPTIONS: BenchRenderOptions = {
- iterations: 100,
- warmupIterations: 5,
-};
+export const TIER_1_OPTIONS: BenchRenderOptions = { ...DEFAULT_OPTIONS };📝 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.
Suggested change
| /** Tier 1: deep benchmarks (mount + rerender + mountMany). */ | |
| export const TIER_1_OPTIONS: BenchRenderOptions = { | |
| iterations: 100, | |
| warmupIterations: 5, | |
| }; | |
| /** Tier 1: deep benchmarks (mount + rerender + mountMany). */ | |
| export const TIER_1_OPTIONS: BenchRenderOptions = { ...DEFAULT_OPTIONS }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/utils/bench-options.ts` around lines 17 - 21,
TIER_1_OPTIONS duplicates DEFAULT_OPTIONS; replace the duplicate object with a
reference to DEFAULT_OPTIONS to avoid duplication and keep them in sync (either
assign TIER_1_OPTIONS = DEFAULT_OPTIONS or use a shallow copy like
TIER_1_OPTIONS: BenchRenderOptions = { ...DEFAULT_OPTIONS }), updating the
symbol TIER_1_OPTIONS and ensuring BenchRenderOptions typing remains correct.
…erage - Remove unnecessary type casts (as boolean, as const, as undefined) across all benchmark files - Convert prebuilt JSX constants to factory functions (Breadcrumbs, DescriptionList, DockLayout, Timeline) - Extract shared helpers (EditableList threeItems, SearchInput noop) - Add defaultOpen to Combobox, animate=false to Drawer rerender - Rename Menu 'mount' to 'mount (closed)' for clarity - Reduce iteration counts (Tier 1: 30/3, Tier 2: 20/2) for faster local runs - Remove benchMountMany from heavy containers (Popover, DataTable, *List) - Reduce item counts from 500 to 200 for list benchmarks - Fix makeTreeNodes exponential growth with breadth-first budget approach - Add benchMountMany to 21 components commonly mounted many times in DOM: IconButton, Chip, Card, Switch, ToggleButton, Input, TextField, TextArea, NumberInput, FormField, Tooltip, StatRow, Progress, Meter, Spinner, ClipboardText, Collapsible, Radio, Banner, Select, Breadcrumbs
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
packages/benchmarks/src/base-ui/Timeline.bench.tsx (1)
34-40:⚠️ Potential issue | 🟠 MajorBuild the rerender
childreninside the render callback.Lines 37-38 still create the
childrentrees once during benchmark registration, so every rerender iteration reuses the same ReactNode instances. That undercounts the rerender path versus the mount case, which now rebuilds the items per iteration. Pass a primitive prop and generate theTimeline.Itemnodes in the callback instead.♻️ Proposed fix
benchRerender( 'items change', { - initialProps: { children: threeItems() }, - updatedProps: { children: fourItems() }, + initialProps: { itemCount: 3 }, + updatedProps: { itemCount: 4 }, }, - (props) => <Timeline {...props} />, + ({ itemCount }) => ( + <Timeline> + {itemCount >= 1 ? <Timeline.Item timestamp="10:00">Deployed v1.0</Timeline.Item> : null} + {itemCount >= 2 ? <Timeline.Item timestamp="10:15">Health check passed</Timeline.Item> : null} + {itemCount >= 3 ? <Timeline.Item timestamp="10:30">Rollout complete</Timeline.Item> : null} + {itemCount >= 4 ? <Timeline.Item timestamp="10:45">Metrics stabilized</Timeline.Item> : null} + </Timeline> + ), TIER_2_OPTIONS, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/benchmarks/src/base-ui/Timeline.bench.tsx` around lines 34 - 40, The benchmark currently creates ReactNode trees once via threeItems()/fourItems() and passes them as children to benchRerender, causing rerender iterations to reuse the same nodes; change benchRerender usage to pass a primitive prop (e.g., itemCount or state key) and in the render callback (the (props) => <Timeline {...props} /> function) generate Timeline.Item nodes each render (using the current primitive to decide between three vs four items) so each rerender rebuilds the children; update any references to threeItems/fourItems to be used only inside that callback or remove them if replaced by an inline builder.packages/benchmarks/src/base-ui/DockLayout.bench.tsx (1)
68-75:⚠️ Potential issue | 🟠 MajorBuild rerender layouts inside the measured callback too.
Line 71 and Line 72 still precompute both
DockNodetrees before the benchmark runs, so the rerender result continues to exclude layout/JSX fixture creation cost. That keeps this suite under-reporting work relative to the other benchmarks and to your own mount case above.📏 Suggested fix
benchRerender( 'layout change (2-panel → 3-panel vertical)', { - initialProps: { layout: makeTwoPanel() }, - updatedProps: { layout: makeThreePanelVertical() }, + initialProps: { variant: 'two' as const }, + updatedProps: { variant: 'three' as const }, }, - (props) => <DockLayout layout={props.layout} />, + (props) => ( + <DockLayout + layout={props.variant === 'two' ? makeTwoPanel() : makeThreePanelVertical()} + /> + ), TIER_1_OPTIONS, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/benchmarks/src/base-ui/DockLayout.bench.tsx` around lines 68 - 75, The benchmark currently precomputes both DockNode trees (makeTwoPanel and makeThreePanelVertical) before timing, so layout/JSX creation cost is excluded; change the setup to build those layouts inside the measured callback instead of as precomputed props — for example, stop passing preconstructed layout objects to benchRerender and instead invoke makeTwoPanel() and makeThreePanelVertical() within the callback used to render DockLayout (or provide factory functions as initialProps/updatedProps) so that benchRerender measures the full cost of creating the DockNode trees for DockLayout; update references to benchRerender, makeTwoPanel, makeThreePanelVertical, DockLayout, and TIER_1_OPTIONS accordingly.packages/benchmarks/src/base-ui/Breadcrumbs.bench.tsx (1)
34-40: 🧹 Nitpick | 🔵 TrivialRerender still prebuilds the breadcrumb tree.
Lines 37-38 evaluate
threeItems()/fourItems()once at suite setup, so the timed rerender path skips child allocation. If you want this case to stay comparable with the inline mount benches, pass a small shape prop and build the items inside the render callback.♻️ Suggested rewrite
benchRerender( 'items change', { - initialProps: { children: threeItems() }, - updatedProps: { children: fourItems() }, + initialProps: { itemCount: 3 as const }, + updatedProps: { itemCount: 4 as const }, }, - (props) => <Breadcrumbs {...props} />, + ({ itemCount }) => ( + <Breadcrumbs> + <Breadcrumbs.Item href="/home">Home</Breadcrumbs.Item> + <Breadcrumbs.Item href="/docs">Docs</Breadcrumbs.Item> + {itemCount === 4 && <Breadcrumbs.Item href="/api">API</Breadcrumbs.Item>} + <Breadcrumbs.Item active>Current</Breadcrumbs.Item> + </Breadcrumbs> + ), TIER_2_OPTIONS, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/benchmarks/src/base-ui/Breadcrumbs.bench.tsx` around lines 34 - 40, The rerender benchmark precomputes children by calling threeItems()/fourItems() at suite setup, so the timed path skips allocation; change benchRerender to pass a small shape prop (e.g., count or size) via initialProps/updatedProps and move the item construction into the render callback so children are built on each render; update the render callback that currently returns <Breadcrumbs {...props} /> to construct the items inside (using props.count to choose/build three vs four items or build dynamically) so the benchmark measures child allocation like the inline mount benches.packages/benchmarks/src/base-ui/DescriptionList.bench.tsx (1)
34-40: 🧹 Nitpick | 🔵 TrivialRerender still precomputes the item fragment.
Lines 37-38 create the
childrentree once during suite setup, so the timed rerender path does not includeDescriptionList.Itemallocation. Build the fragment from a small prop inside the render callback instead.♻️ Suggested rewrite
benchRerender( 'items change', { - initialProps: { children: threeItems() }, - updatedProps: { children: fourItems() }, + initialProps: { itemCount: 3 as const }, + updatedProps: { itemCount: 4 as const }, }, - (props) => <DescriptionList {...props} />, + ({ itemCount }) => ( + <DescriptionList> + <DescriptionList.Item label="Name">Widget</DescriptionList.Item> + <DescriptionList.Item label="Status">Active</DescriptionList.Item> + <DescriptionList.Item label="Version">1.0.0</DescriptionList.Item> + {itemCount === 4 && ( + <DescriptionList.Item label="Region">US-East</DescriptionList.Item> + )} + </DescriptionList> + ), TIER_2_OPTIONS, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/benchmarks/src/base-ui/DescriptionList.bench.tsx` around lines 34 - 40, The benchmark currently precomputes the children fragment (threeItems/fourItems) during suite setup so the benchRerender timed path skips allocating DescriptionList.Item; change the bench to build the fragment inside the render callback passed to benchRerender (use a small prop like itemCount or an items array length) and create the DescriptionList.Item elements there (referencing benchRerender and the DescriptionList component) so each rerender actually allocates the item fragments during the timed run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/benchmarks/src/base-ui/ActionList.bench.tsx`:
- Around line 23-26: In ActionList.bench.tsx remove the trailing spaces inside
the props object literals so they read { disabled: false } and { disabled: true
} instead of { disabled: false } / { disabled: true }; update the occurrences
for initialProps and updatedProps in the benchmark config to eliminate the extra
whitespace before the closing braces.
In `@packages/benchmarks/src/base-ui/EditorTabs.bench.tsx`:
- Around line 27-33: The benchmark currently measures makeEditorTabs inside the
timed benchMountMany callback; precompute an array of 30 tab sets before calling
benchMountMany (using makeEditorTabs to build each set once), then change the
callback passed to benchMountMany to simply index into that precomputed array
and render <EditorTabs key={i} tabs={precomputed[i]}
activeId={precomputed[i][0]!.id} /> so only EditorTabs rendering is timed; refer
to benchMountMany, makeEditorTabs, and EditorTabs when implementing this change.
In `@packages/benchmarks/src/base-ui/MultiSelect.bench.tsx`:
- Around line 11-21: The benchmark is not rendering the popup list because
MultiSelectBench does not pass the defaultOpen prop; update the MultiSelect call
inside the MultiSelectBench function to include defaultOpen so the
BaseCombobox.Portal mounts and the full option list is rendered during
benchmarks (add defaultOpen to the <MultiSelect<Option> ... /> invocation in
MultiSelectBench).
In `@packages/benchmarks/src/base-ui/Popover.bench.tsx`:
- Around line 6-46: The benchmark file lacks a bulk-mount case required for Tier
1 coverage; add a new benchmark using the existing harness (e.g., benchMountMany
or a similarly named helper used elsewhere) to repeatedly mount many Popover
instances to capture portal setup costs. Insert a new test similar in shape to
the 'mount (open)' case but using the many-mount helper (referencing Popover,
Popover.Trigger, Popover.Portal, Popover.Positioner, Popover.Popup,
Popover.Title, Popover.Description) and use TIER_1_OPTIONS so the file includes
single mount, rerender toggle (benchRerender), and the required mountMany bulk
case.
- Around line 26-46: The current benchRerender named "open/close toggle" only
measures teardown (open: true -> false); update tests to benchmark the open path
by adding a new benchRerender that uses initialProps: { open: false } and
updatedProps: { open: true } rendering the same Popover subtree (same Popover,
Popover.Trigger, Popover.Portal, Popover.Positioner, Popover.Popup,
Popover.Title, Popover.Description) with TIER_1_OPTIONS, or alternatively rename
the existing case to "close" if you intend to keep only the teardown
measurement.
In `@packages/benchmarks/src/base-ui/Select.bench.tsx`:
- Around line 57-74: The benchmark uses Select with defaultValue="option-0"
which does not match any Select.Item values ("a","b","c"), so no item will be
selected; update the defaultValue in the benchMountMany case to one of the
existing item values (e.g., "a") or change the Select.Item value(s) to include
"option-0" so they match; locate the benchMountMany call creating the Select
(function/variable names: benchMountMany, Select, Select.Item) and make the
defaultValue and item values consistent.
- Around line 9-29: The benchmark mounts the Select but never opens its popup,
so add the Select prop defaultOpen to render the option list on mount; update
the benchRender usage that renders <Select defaultValue={options[0]!.value}> (in
the Select bench component) to include defaultOpen so the popup/List/Item
rendering (the Select.Portal/Select.Positioner/Select.Popup/Select.List and
options map) is measured under TIER_2_OPTIONS.
In `@packages/benchmarks/src/utils/factories.ts`:
- Around line 32-46: makeTreeNodes currently consumes the entire count creating
only root nodes (created reaches count) before the breadth-first descendant loop
runs, producing a flat list; fix by reserving some of the count for children so
the BFS loop can attach descendants: when creating roots in makeTreeNodes, don't
always increment created to equal count at once—push fewer roots (or decrement
remaining = count - created) and then in the queue-processing loop (the queue
variable and its {node, depth} entries) create child TreeNode objects until
created reaches count, attach them to a children array on the parent node,
increment created for each child, and enqueue children with incremented depth
while respecting the depth parameter so nested nodes are produced instead of a
flat list.
---
Duplicate comments:
In `@packages/benchmarks/src/base-ui/Breadcrumbs.bench.tsx`:
- Around line 34-40: The rerender benchmark precomputes children by calling
threeItems()/fourItems() at suite setup, so the timed path skips allocation;
change benchRerender to pass a small shape prop (e.g., count or size) via
initialProps/updatedProps and move the item construction into the render
callback so children are built on each render; update the render callback that
currently returns <Breadcrumbs {...props} /> to construct the items inside
(using props.count to choose/build three vs four items or build dynamically) so
the benchmark measures child allocation like the inline mount benches.
In `@packages/benchmarks/src/base-ui/DescriptionList.bench.tsx`:
- Around line 34-40: The benchmark currently precomputes the children fragment
(threeItems/fourItems) during suite setup so the benchRerender timed path skips
allocating DescriptionList.Item; change the bench to build the fragment inside
the render callback passed to benchRerender (use a small prop like itemCount or
an items array length) and create the DescriptionList.Item elements there
(referencing benchRerender and the DescriptionList component) so each rerender
actually allocates the item fragments during the timed run.
In `@packages/benchmarks/src/base-ui/DockLayout.bench.tsx`:
- Around line 68-75: The benchmark currently precomputes both DockNode trees
(makeTwoPanel and makeThreePanelVertical) before timing, so layout/JSX creation
cost is excluded; change the setup to build those layouts inside the measured
callback instead of as precomputed props — for example, stop passing
preconstructed layout objects to benchRerender and instead invoke makeTwoPanel()
and makeThreePanelVertical() within the callback used to render DockLayout (or
provide factory functions as initialProps/updatedProps) so that benchRerender
measures the full cost of creating the DockNode trees for DockLayout; update
references to benchRerender, makeTwoPanel, makeThreePanelVertical, DockLayout,
and TIER_1_OPTIONS accordingly.
In `@packages/benchmarks/src/base-ui/Timeline.bench.tsx`:
- Around line 34-40: The benchmark currently creates ReactNode trees once via
threeItems()/fourItems() and passes them as children to benchRerender, causing
rerender iterations to reuse the same nodes; change benchRerender usage to pass
a primitive prop (e.g., itemCount or state key) and in the render callback (the
(props) => <Timeline {...props} /> function) generate Timeline.Item nodes each
render (using the current primitive to decide between three vs four items) so
each rerender rebuilds the children; update any references to
threeItems/fourItems to be used only inside that callback or remove them if
replaced by an inline builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51ad686e-4900-433c-a300-651b51089b22
📒 Files selected for processing (50)
packages/benchmarks/src/base-ui/ActionList.bench.tsxpackages/benchmarks/src/base-ui/AlertDialog.bench.tsxpackages/benchmarks/src/base-ui/AppShell.bench.tsxpackages/benchmarks/src/base-ui/Banner.bench.tsxpackages/benchmarks/src/base-ui/BasicList.bench.tsxpackages/benchmarks/src/base-ui/Breadcrumbs.bench.tsxpackages/benchmarks/src/base-ui/Card.bench.tsxpackages/benchmarks/src/base-ui/Checkbox.bench.tsxpackages/benchmarks/src/base-ui/Chip.bench.tsxpackages/benchmarks/src/base-ui/ClipboardText.bench.tsxpackages/benchmarks/src/base-ui/Collapsible.bench.tsxpackages/benchmarks/src/base-ui/Combobox.bench.tsxpackages/benchmarks/src/base-ui/CommandList.bench.tsxpackages/benchmarks/src/base-ui/ConfirmButton.bench.tsxpackages/benchmarks/src/base-ui/DataTable.bench.tsxpackages/benchmarks/src/base-ui/DescriptionList.bench.tsxpackages/benchmarks/src/base-ui/Dialog.bench.tsxpackages/benchmarks/src/base-ui/DockLayout.bench.tsxpackages/benchmarks/src/base-ui/Drawer.bench.tsxpackages/benchmarks/src/base-ui/EditableList.bench.tsxpackages/benchmarks/src/base-ui/EditorTabs.bench.tsxpackages/benchmarks/src/base-ui/FormField.bench.tsxpackages/benchmarks/src/base-ui/IconButton.bench.tsxpackages/benchmarks/src/base-ui/Input.bench.tsxpackages/benchmarks/src/base-ui/Menu.bench.tsxpackages/benchmarks/src/base-ui/Meter.bench.tsxpackages/benchmarks/src/base-ui/MultiSelect.bench.tsxpackages/benchmarks/src/base-ui/NavList.bench.tsxpackages/benchmarks/src/base-ui/NumberInput.bench.tsxpackages/benchmarks/src/base-ui/Pagination.bench.tsxpackages/benchmarks/src/base-ui/Popover.bench.tsxpackages/benchmarks/src/base-ui/Progress.bench.tsxpackages/benchmarks/src/base-ui/Radio.bench.tsxpackages/benchmarks/src/base-ui/RowList.bench.tsxpackages/benchmarks/src/base-ui/SearchInput.bench.tsxpackages/benchmarks/src/base-ui/Select.bench.tsxpackages/benchmarks/src/base-ui/SelectableList.bench.tsxpackages/benchmarks/src/base-ui/Spinner.bench.tsxpackages/benchmarks/src/base-ui/SplitButton.bench.tsxpackages/benchmarks/src/base-ui/StatRow.bench.tsxpackages/benchmarks/src/base-ui/Switch.bench.tsxpackages/benchmarks/src/base-ui/TextArea.bench.tsxpackages/benchmarks/src/base-ui/TextField.bench.tsxpackages/benchmarks/src/base-ui/Timeline.bench.tsxpackages/benchmarks/src/base-ui/ToggleButton.bench.tsxpackages/benchmarks/src/base-ui/Toolbar.bench.tsxpackages/benchmarks/src/base-ui/Tooltip.bench.tsxpackages/benchmarks/src/base-ui/TreeList.bench.tsxpackages/benchmarks/src/utils/bench-options.tspackages/benchmarks/src/utils/factories.ts
Comment on lines
+23
to
+26
| { | ||
| initialProps: { disabled: false }, | ||
| updatedProps: { disabled: true }, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Trailing whitespace.
There's extra whitespace before the closing braces in the props objects.
🧹 Suggested fix
{
- initialProps: { disabled: false },
- updatedProps: { disabled: true },
+ initialProps: { disabled: false },
+ updatedProps: { disabled: true },
},📝 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.
Suggested change
| { | |
| initialProps: { disabled: false }, | |
| updatedProps: { disabled: true }, | |
| }, | |
| { | |
| initialProps: { disabled: false }, | |
| updatedProps: { disabled: true }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/ActionList.bench.tsx` around lines 23 - 26,
In ActionList.bench.tsx remove the trailing spaces inside the props object
literals so they read { disabled: false } and { disabled: true } instead of {
disabled: false } / { disabled: true }; update the occurrences for
initialProps and updatedProps in the benchmark config to eliminate the extra
whitespace before the closing braces.
Comment on lines
+27
to
+33
| benchMountMany( | ||
| 'mount 30 tab bars (5 tabs each)', | ||
| 30, | ||
| (i) => { | ||
| const tabs = makeEditorTabs(5, `bar${i}-`); | ||
| return <EditorTabs key={i} tabs={tabs} activeId={tabs[0]!.id} />; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
mountMany is timing tab factory work too.
Lines 31-32 allocate a fresh tab array inside the timed callback, so this case measures makeEditorTabs as well as EditorTabs. Precompute the 30 tab sets once and index into them here.
♻️ Suggested rewrite
const tabs20 = makeEditorTabs(20);
const tabs40 = makeEditorTabs(40);
+const mountManyTabs = Array.from({ length: 30 }, (_, i) => makeEditorTabs(5, `bar${i}-`));
describe('EditorTabs', () => {
@@
benchMountMany(
'mount 30 tab bars (5 tabs each)',
30,
(i) => {
- const tabs = makeEditorTabs(5, `bar${i}-`);
+ const tabs = mountManyTabs[i]!;
return <EditorTabs key={i} tabs={tabs} activeId={tabs[0]!.id} />;
},
TIER_1_OPTIONS,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/EditorTabs.bench.tsx` around lines 27 - 33,
The benchmark currently measures makeEditorTabs inside the timed benchMountMany
callback; precompute an array of 30 tab sets before calling benchMountMany
(using makeEditorTabs to build each set once), then change the callback passed
to benchMountMany to simply index into that precomputed array and render
<EditorTabs key={i} tabs={precomputed[i]} activeId={precomputed[i][0]!.id} /> so
only EditorTabs rendering is timed; refer to benchMountMany, makeEditorTabs, and
EditorTabs when implementing this change.
Comment on lines
+11
to
+21
| function MultiSelectBench({ options, value }: { options: Option[]; value?: Option[] }) { | ||
| return ( | ||
| <MultiSelect<Option> | ||
| items={options} | ||
| value={value} | ||
| getItemLabel={(item) => item.label} | ||
| getItemValue={(item) => item.value} | ||
| placeholder="Select options" | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MultiSelect has a defaultOpen prop and how it handles popup mounting
rg -n -C5 'defaultOpen|keepMounted|Popup|Portal' packages/base-ui/src/components --include='*[Mm]ulti[Ss]elect*' 2>/dev/null || \
rg -n -C5 'defaultOpen|keepMounted|Popup|Portal' packages -g '*[Mm]ulti[Ss]elect*.tsx' -g '*[Mm]ulti[Ss]elect*.ts'Repository: omniviewdev/ui
Length of output: 5489
🏁 Script executed:
cat -n packages/benchmarks/src/base-ui/MultiSelect.bench.tsxRepository: omniviewdev/ui
Length of output: 1462
🏁 Script executed:
rg -n 'export.*Portal|const.*Portal' packages/base-ui/src/components/combobox -A 3 | head -30Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
find packages/base-ui/src/components/combobox -name '*.tsx' -o -name '*.ts' | head -10Repository: omniviewdev/ui
Length of output: 282
🏁 Script executed:
rg -n 'Portal.*=|Popup.*=' packages/base-ui/src/components/combobox/Combobox.tsx | head -20Repository: omniviewdev/ui
Length of output: 204
🏁 Script executed:
rg -n 'import.*BaseCombobox|from.*@base-ui' packages/base-ui/src/components/combobox/Combobox.tsx | head -10Repository: omniviewdev/ui
Length of output: 127
🏁 Script executed:
rg -n 'open|Open' packages/base-ui/src/components/multi-select/MultiSelect.tsx | grep -E '(if|condition|render)' | head -15Repository: omniviewdev/ui
Length of output: 80
🏁 Script executed:
sed -n '215,260p' packages/base-ui/src/components/multi-select/MultiSelect.tsxRepository: omniviewdev/ui
Length of output: 1481
🏁 Script executed:
sed -n '330,360p' packages/base-ui/src/components/multi-select/MultiSelect.tsxRepository: omniviewdev/ui
Length of output: 1044
🏁 Script executed:
sed -n '1,15p' packages/base-ui/src/components/multi-select/MultiSelect.tsxRepository: omniviewdev/ui
Length of output: 658
🏁 Script executed:
rg -n 'interface.*MultiSelectProps|type.*MultiSelectProps' packages/base-ui/src/components/multi-select/MultiSelect.tsx -A 20Repository: omniviewdev/ui
Length of output: 797
Add defaultOpen to ensure benchmarks measure full option-list rendering performance.
The MultiSelect component uses BaseCombobox.Portal to conditionally mount its popup content, similar to Combobox. Currently, the benchmark measurements only capture the closed state (trigger/input and selected chips) since defaultOpen is not specified. Without this prop, the popup remains closed and its content is not rendered during benchmarking.
Both the test file (line 44) and stories file (line 247) explicitly use defaultOpen when measuring or displaying the popup. Update the MultiSelectBench component to include defaultOpen so the benchmarks accurately measure the performance of rendering the full option list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/MultiSelect.bench.tsx` around lines 11 - 21,
The benchmark is not rendering the popup list because MultiSelectBench does not
pass the defaultOpen prop; update the MultiSelect call inside the
MultiSelectBench function to include defaultOpen so the BaseCombobox.Portal
mounts and the full option list is rendered during benchmarks (add defaultOpen
to the <MultiSelect<Option> ... /> invocation in MultiSelectBench).
Comment on lines
+6
to
+46
| describe('Popover', () => { | ||
| benchRender( | ||
| 'mount (open)', | ||
| () => ( | ||
| <Popover defaultOpen> | ||
| <Popover.Trigger>Open</Popover.Trigger> | ||
| <Popover.Portal> | ||
| <Popover.Positioner> | ||
| <Popover.Popup> | ||
| <Popover.Title>Info</Popover.Title> | ||
| <Popover.Description>Popover content here.</Popover.Description> | ||
| <Popover.Close>Close</Popover.Close> | ||
| </Popover.Popup> | ||
| </Popover.Positioner> | ||
| </Popover.Portal> | ||
| </Popover> | ||
| ), | ||
| TIER_1_OPTIONS, | ||
| ); | ||
|
|
||
| benchRerender( | ||
| 'open/close toggle', | ||
| { | ||
| initialProps: { open: true }, | ||
| updatedProps: { open: false }, | ||
| }, | ||
| (props) => ( | ||
| <Popover open={props.open}> | ||
| <Popover.Trigger>Open</Popover.Trigger> | ||
| <Popover.Portal> | ||
| <Popover.Positioner> | ||
| <Popover.Popup> | ||
| <Popover.Title>Info</Popover.Title> | ||
| <Popover.Description>Content</Popover.Description> | ||
| </Popover.Popup> | ||
| </Popover.Positioner> | ||
| </Popover.Portal> | ||
| </Popover> | ||
| ), | ||
| TIER_1_OPTIONS, | ||
| ); |
There was a problem hiding this comment.
Tier 1 coverage is incomplete without a mountMany case.
Popover is called out as a Tier 1 target in this PR, but this file only covers single mount and rerender. Without a bulk-mount benchmark, repeated portal setup cost is untracked here, so this file does not meet the stated Tier 1 benchmark shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/Popover.bench.tsx` around lines 6 - 46, The
benchmark file lacks a bulk-mount case required for Tier 1 coverage; add a new
benchmark using the existing harness (e.g., benchMountMany or a similarly named
helper used elsewhere) to repeatedly mount many Popover instances to capture
portal setup costs. Insert a new test similar in shape to the 'mount (open)'
case but using the many-mount helper (referencing Popover, Popover.Trigger,
Popover.Portal, Popover.Positioner, Popover.Popup, Popover.Title,
Popover.Description) and use TIER_1_OPTIONS so the file includes single mount,
rerender toggle (benchRerender), and the required mountMany bulk case.
Comment on lines
+26
to
+46
| benchRerender( | ||
| 'open/close toggle', | ||
| { | ||
| initialProps: { open: true }, | ||
| updatedProps: { open: false }, | ||
| }, | ||
| (props) => ( | ||
| <Popover open={props.open}> | ||
| <Popover.Trigger>Open</Popover.Trigger> | ||
| <Popover.Portal> | ||
| <Popover.Positioner> | ||
| <Popover.Popup> | ||
| <Popover.Title>Info</Popover.Title> | ||
| <Popover.Description>Content</Popover.Description> | ||
| </Popover.Popup> | ||
| </Popover.Positioner> | ||
| </Popover.Portal> | ||
| </Popover> | ||
| ), | ||
| TIER_1_OPTIONS, | ||
| ); |
There was a problem hiding this comment.
Benchmark the open path instead of only the close path.
This case only rerenders open: true -> false, so it measures teardown, not a real open/close toggle. For Popover, the more regression-prone work is usually on open when the portal subtree is created and positioned. Add a separate false -> true rerender bench, or rename this one to close so the results are not misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/Popover.bench.tsx` around lines 26 - 46, The
current benchRerender named "open/close toggle" only measures teardown (open:
true -> false); update tests to benchmark the open path by adding a new
benchRerender that uses initialProps: { open: false } and updatedProps: { open:
true } rendering the same Popover subtree (same Popover, Popover.Trigger,
Popover.Portal, Popover.Positioner, Popover.Popup, Popover.Title,
Popover.Description) with TIER_1_OPTIONS, or alternatively rename the existing
case to "close" if you intend to keep only the teardown measurement.
Comment on lines
+9
to
+29
| describe('Select', () => { | ||
| benchRender('mount with 20 options', () => ( | ||
| <Select defaultValue={options[0]!.value}> | ||
| <Select.Trigger> | ||
| <Select.Value placeholder="Choose..." /> | ||
| </Select.Trigger> | ||
| <Select.Portal> | ||
| <Select.Positioner> | ||
| <Select.Popup> | ||
| <Select.List> | ||
| {options.map((opt) => ( | ||
| <Select.Item key={opt.value} value={opt.value}> | ||
| {opt.label} | ||
| </Select.Item> | ||
| ))} | ||
| </Select.List> | ||
| </Select.Popup> | ||
| </Select.Positioner> | ||
| </Select.Portal> | ||
| </Select> | ||
| ), TIER_2_OPTIONS); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Select has a defaultOpen prop and how it handles popup mounting
rg -n -C5 'defaultOpen|keepMounted' packages/base-ui/src/components -g '*[Ss]elect*.tsx' -g '*[Ss]elect*.ts' | head -50Repository: omniviewdev/ui
Length of output: 3377
🏁 Script executed:
#!/bin/bash
# Find Select component definition to check available props
fd -e tsx -e ts 'Select' packages/base-ui/src/components --max-depth 2 | grep -E 'Select\.(tsx|ts)$'Repository: omniviewdev/ui
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check the Select component props and definition
cat -n packages/base-ui/src/components/select/Select.tsx | head -100Repository: omniviewdev/ui
Length of output: 3755
🏁 Script executed:
#!/bin/bash
# Search for defaultOpen in Select files
rg -n 'defaultOpen' packages/base-ui/src/components/select/ -g '*.tsx' -g '*.ts'Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if `@base-ui/react/select` is in package.json or node_modules
cat packages/benchmarks/package.json | grep -A5 -B5 '@base-ui'Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the full benchmark file to understand the structure
cat -n packages/benchmarks/src/base-ui/Select.bench.tsxRepository: omniviewdev/ui
Length of output: 2872
🏁 Script executed:
#!/bin/bash
# Find and check Combobox benchmark
cat -n packages/benchmarks/src/base-ui/Combobox.bench.tsxRepository: omniviewdev/ui
Length of output: 2146
🏁 Script executed:
#!/bin/bash
# Check if Select is exported from base-ui and review its types
rg -n 'export.*Select' packages/base-ui/src/index.ts -A5 -B5Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at the Select Root component definition more carefully
sed -n '96,150p' packages/base-ui/src/components/select/Select.tsxRepository: omniviewdev/ui
Length of output: 1808
🏁 Script executed:
#!/bin/bash
# Search for defaultOpen in BaseSelect or upstream components
rg -n 'defaultOpen' packages/base-ui/src/components/ --type-listRepository: omniviewdev/ui
Length of output: 447
🏁 Script executed:
#!/bin/bash
# Check if defaultOpen is used in any Select-related component
rg -n 'defaultOpen' packages/base-ui/src/ -C3Repository: omniviewdev/ui
Length of output: 17366
🏁 Script executed:
#!/bin/bash
# Get the complete SelectRootProps type definition
sed -n '62,68p' packages/base-ui/src/components/select/Select.tsxRepository: omniviewdev/ui
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if there's any type definition or documentation mentioning defaultOpen for Select
find packages/base-ui/src/components/select -type f \( -name '*.ts' -o -name '*.tsx' -o -name '*.md' \) -exec cat {} \; | grep -i 'defaultopen' || echo "No defaultOpen found in Select component files"Repository: omniviewdev/ui
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check the `@base-ui/react` package to understand what props BaseSelect supports
# First, let's see if there's node_modules or lock file info
ls -la | grep -E 'package-lock|yarn.lock|pnpm-lock'Repository: omniviewdev/ui
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Check the `@base-ui/react` version in pnpm-lock.yaml
grep -A3 '@base-ui/react' pnpm-lock.yaml | head -20Repository: omniviewdev/ui
Length of output: 673
🏁 Script executed:
#!/bin/bash
# Check if there are any type definitions or stubs for BaseSelect in the node_modules or elsewhere
find . -name "*.d.ts" -o -name "*.ts" | xargs grep -l "BaseSelect.*Root.*Props" 2>/dev/null | head -5Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or usage patterns showing how Select behaves with defaultOpen
rg -n 'Select.*defaultOpen|defaultOpen.*Select' packages/ --type-list=all 2>/dev/null | head -10Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check `@base-ui/react` documentation or types by searching online resources or looking at package info
# Let's see if there are any comments or docs in the benchmark that explain the Portal/Popup behavior
grep -B5 -A15 'Portal\|Popup' packages/benchmarks/src/base-ui/Select.bench.tsx | head -30Repository: omniviewdev/ui
Length of output: 989
🏁 Script executed:
#!/bin/bash
# Compare with Combobox implementation to understand the difference
head -100 packages/base-ui/src/components/combobox/Combobox.tsx | grep -A30 'ComboboxRoot\|defaultOpen'Repository: omniviewdev/ui
Length of output: 1172
🌐 Web query:
@base-ui/react Select component defaultOpen prop documentation
💡 Result:
In Base UI’s Select (@base-ui/react/select), defaultOpen is a prop on <Select.Root>:
defaultOpen?: boolean(default:false) — “Whether the select popup is initially open.”- If you need to control the open state, use
open(andonOpenChange) instead ofdefaultOpen. [1]
Example
import { Select } from '@base-ui/react/select';
<Select.Root defaultOpen items={[{ value: 'a', label: 'A' }]}>
<Select.Trigger>
<Select.Value />
</Select.Trigger>
<Select.Portal>
<Select.Positioner>
<Select.Popup>
<Select.List>
<Select.Item value="a">
<Select.ItemText />
</Select.Item>
</Select.List>
</Select.Popup>
</Select.Positioner>
</Select.Portal>
</Select.Root>Source: Base UI Select → API reference → Root props (defaultOpen). [1]
🌐 Web query:
@base-ui/react Select Portal Popup conditional rendering closed state
💡 Result:
In Base UI @base-ui/react/select, the open/closed state is owned by <Select.Root> via the open + onOpenChange props. (base-ui.com)
Conditionally render the portalled popup only when open
const [open, setOpen] = React.useState(false);
<Select.Root open={open} onOpenChange={setOpen}>
<Select.Trigger>
<Select.Value />
<Select.Icon />
</Select.Trigger>
{open && (
<Select.Portal>
<Select.Backdrop />
<Select.Positioner>
<Select.Popup>
<Select.List>{/* items... */}</Select.List>
</Select.Popup>
</Select.Positioner>
</Select.Portal>
)}
</Select.Root>(These parts are the standard Select anatomy.) (base-ui.com)
If you need it to stay mounted after closing (e.g., exit animations)
Use actionsRef and its unmount action: when provided, the Select won’t unmount on close; you call unmount() yourself (e.g., after the animation finishes). (base-ui.com)
Citations:
- 1: https://base-ui.com/react/components/select
- 2: https://base-ui.com/react/components/select
- 3: https://base-ui.com/react/components/select
🏁 Script executed:
#!/bin/bash
# Check if the Select component in `@omniview/base-ui` properly passes defaultOpen through SelectRootProps
grep -A10 'BaseSelect.Root<Value, Multiple>' packages/base-ui/src/components/select/Select.tsx | head -15Repository: omniviewdev/ui
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Verify BaseSelect.Root actually receives and uses the props including defaultOpen
sed -n '96,130p' packages/base-ui/src/components/select/Select.tsxRepository: omniviewdev/ui
Length of output: 1196
Add defaultOpen to measure the full option-list rendering.
The Select component supports defaultOpen to render the popup content on mount. Without it, the benchmark measures only the closed trigger state. To capture the performance of rendering all 20 options, add defaultOpen:
Proposed fix
- benchRender('mount with 20 options', () => (
- <Select defaultValue={options[0]!.value}>
+ benchRender('mount with 20 options', () => (
+ <Select defaultValue={options[0]!.value} defaultOpen>
<Select.Trigger>📝 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.
Suggested change
| describe('Select', () => { | |
| benchRender('mount with 20 options', () => ( | |
| <Select defaultValue={options[0]!.value}> | |
| <Select.Trigger> | |
| <Select.Value placeholder="Choose..." /> | |
| </Select.Trigger> | |
| <Select.Portal> | |
| <Select.Positioner> | |
| <Select.Popup> | |
| <Select.List> | |
| {options.map((opt) => ( | |
| <Select.Item key={opt.value} value={opt.value}> | |
| {opt.label} | |
| </Select.Item> | |
| ))} | |
| </Select.List> | |
| </Select.Popup> | |
| </Select.Positioner> | |
| </Select.Portal> | |
| </Select> | |
| ), TIER_2_OPTIONS); | |
| describe('Select', () => { | |
| benchRender('mount with 20 options', () => ( | |
| <Select defaultValue={options[0]!.value} defaultOpen> | |
| <Select.Trigger> | |
| <Select.Value placeholder="Choose..." /> | |
| </Select.Trigger> | |
| <Select.Portal> | |
| <Select.Positioner> | |
| <Select.Popup> | |
| <Select.List> | |
| {options.map((opt) => ( | |
| <Select.Item key={opt.value} value={opt.value}> | |
| {opt.label} | |
| </Select.Item> | |
| ))} | |
| </Select.List> | |
| </Select.Popup> | |
| </Select.Positioner> | |
| </Select.Portal> | |
| </Select> | |
| ), TIER_2_OPTIONS); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/Select.bench.tsx` around lines 9 - 29, The
benchmark mounts the Select but never opens its popup, so add the Select prop
defaultOpen to render the option list on mount; update the benchRender usage
that renders <Select defaultValue={options[0]!.value}> (in the Select bench
component) to include defaultOpen so the popup/List/Item rendering (the
Select.Portal/Select.Positioner/Select.Popup/Select.List and options map) is
measured under TIER_2_OPTIONS.
Comment on lines
+57
to
+74
| benchMountMany('mount 50', 50, (i) => ( | ||
| <Select key={i} defaultValue="option-0"> | ||
| <Select.Trigger> | ||
| <Select.Value placeholder="Choose..." /> | ||
| </Select.Trigger> | ||
| <Select.Portal> | ||
| <Select.Positioner> | ||
| <Select.Popup> | ||
| <Select.List> | ||
| <Select.Item value="a">A</Select.Item> | ||
| <Select.Item value="b">B</Select.Item> | ||
| <Select.Item value="c">C</Select.Item> | ||
| </Select.List> | ||
| </Select.Popup> | ||
| </Select.Positioner> | ||
| </Select.Portal> | ||
| </Select> | ||
| ), TIER_2_OPTIONS); |
There was a problem hiding this comment.
defaultValue references non-existent option value.
Line 58 uses defaultValue="option-0" but the Select items are hardcoded as "a", "b", "c". This mismatch means no option will be initially selected, which may or may not be intentional for the benchmark.
🐛 Proposed fix
benchMountMany('mount 50', 50, (i) => (
- <Select key={i} defaultValue="option-0">
+ <Select key={i} defaultValue="a">
<Select.Trigger>📝 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.
Suggested change
| benchMountMany('mount 50', 50, (i) => ( | |
| <Select key={i} defaultValue="option-0"> | |
| <Select.Trigger> | |
| <Select.Value placeholder="Choose..." /> | |
| </Select.Trigger> | |
| <Select.Portal> | |
| <Select.Positioner> | |
| <Select.Popup> | |
| <Select.List> | |
| <Select.Item value="a">A</Select.Item> | |
| <Select.Item value="b">B</Select.Item> | |
| <Select.Item value="c">C</Select.Item> | |
| </Select.List> | |
| </Select.Popup> | |
| </Select.Positioner> | |
| </Select.Portal> | |
| </Select> | |
| ), TIER_2_OPTIONS); | |
| benchMountMany('mount 50', 50, (i) => ( | |
| <Select key={i} defaultValue="a"> | |
| <Select.Trigger> | |
| <Select.Value placeholder="Choose..." /> | |
| </Select.Trigger> | |
| <Select.Portal> | |
| <Select.Positioner> | |
| <Select.Popup> | |
| <Select.List> | |
| <Select.Item value="a">A</Select.Item> | |
| <Select.Item value="b">B</Select.Item> | |
| <Select.Item value="c">C</Select.Item> | |
| </Select.List> | |
| </Select.Popup> | |
| </Select.Positioner> | |
| </Select.Portal> | |
| </Select> | |
| ), TIER_2_OPTIONS); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/base-ui/Select.bench.tsx` around lines 57 - 74, The
benchmark uses Select with defaultValue="option-0" which does not match any
Select.Item values ("a","b","c"), so no item will be selected; update the
defaultValue in the benchMountMany case to one of the existing item values
(e.g., "a") or change the Select.Item value(s) to include "option-0" so they
match; locate the benchMountMany call creating the Select (function/variable
names: benchMountMany, Select, Select.Item) and make the defaultValue and item
values consistent.
Comment on lines
+32
to
+46
| export function makeTreeNodes(count: number, depth: number = 2): TreeNode[] { | ||
| let created = 0; | ||
| const roots: TreeNode[] = []; | ||
| const queue: { node: TreeNode; depth: number }[] = []; | ||
|
|
||
| // Create root-level nodes | ||
| while (created < count) { | ||
| const node: TreeNode = { | ||
| id: `node-${created}`, | ||
| label: `Node ${created}`, | ||
| }; | ||
| created++; | ||
| roots.push(node); | ||
| if (depth > 0) queue.push({ node, depth: 1 }); | ||
| } |
There was a problem hiding this comment.
makeTreeNodes never builds descendants.
Lines 38-46 spend the entire count budget on roots, so created === count before the breadth-first loop starts. The result is a flat list, not a tree, which invalidates any benchmark meant to exercise nested rendering.
🌲 Suggested fix
export function makeTreeNodes(count: number, depth: number = 2): TreeNode[] {
- let created = 0;
- const roots: TreeNode[] = [];
- const queue: { node: TreeNode; depth: number }[] = [];
-
- // Create root-level nodes
- while (created < count) {
- const node: TreeNode = {
- id: `node-${created}`,
- label: `Node ${created}`,
- };
- created++;
- roots.push(node);
- if (depth > 0) queue.push({ node, depth: 1 });
- }
+ if (count <= 0) return [];
+
+ const root: TreeNode = {
+ id: 'node-0',
+ label: 'Node 0',
+ };
+ let created = 1;
+ const roots: TreeNode[] = [root];
+ const queue: { node: TreeNode; depth: number }[] =
+ depth > 0 ? [{ node: root, depth: 1 }] : [];
// Breadth-first: attach children until budget exhausted
while (queue.length > 0 && created < count) {📝 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.
Suggested change
| export function makeTreeNodes(count: number, depth: number = 2): TreeNode[] { | |
| let created = 0; | |
| const roots: TreeNode[] = []; | |
| const queue: { node: TreeNode; depth: number }[] = []; | |
| // Create root-level nodes | |
| while (created < count) { | |
| const node: TreeNode = { | |
| id: `node-${created}`, | |
| label: `Node ${created}`, | |
| }; | |
| created++; | |
| roots.push(node); | |
| if (depth > 0) queue.push({ node, depth: 1 }); | |
| } | |
| export function makeTreeNodes(count: number, depth: number = 2): TreeNode[] { | |
| if (count <= 0) return []; | |
| const root: TreeNode = { | |
| id: 'node-0', | |
| label: 'Node 0', | |
| }; | |
| let created = 1; | |
| const roots: TreeNode[] = [root]; | |
| const queue: { node: TreeNode; depth: number }[] = | |
| depth > 0 ? [{ node: root, depth: 1 }] : []; | |
| // Breadth-first: attach children until budget exhausted | |
| while (queue.length > 0 && created < count) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/utils/factories.ts` around lines 32 - 46,
makeTreeNodes currently consumes the entire count creating only root nodes
(created reaches count) before the breadth-first descendant loop runs, producing
a flat list; fix by reserving some of the count for children so the BFS loop can
attach descendants: when creating roots in makeTreeNodes, don't always increment
created to equal count at once—push fewer roots (or decrement remaining = count
- created) and then in the queue-processing loop (the queue variable and its
{node, depth} entries) create child TreeNode objects until created reaches
count, attach them to a children array on the parent node, increment created for
each child, and enqueue children with incremented depth while respecting the
depth parameter so nested nodes are produced instead of a flat list.
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
Add Reassure-based performance tests that measure React render counts and durations per interaction scenario — the dimension our vitest bench microbenchmarks don't cover. Infrastructure: - vitest.config.perf.ts with JIT-disabled V8 flags for stable measurement - setup-perf.ts configures Reassure for @testing-library/react (20 runs, 3 warmup) - scripts/perf-compare.mjs for cross-branch comparison reporting - PointerEvent jsdom stub (required by @base-ui/react click handlers) Initial perf tests (7 files, 18 scenarios): - Button: mount baseline (1 render, 0 issues) - Checkbox: mount, toggle, toggle-one-in-list-of-20 - Select: mount with 20 options, open dropdown - Tabs: mount 5 tabs, switch active tab - Accordion: mount 5 items, expand, expand+collapse - Dialog: mount closed, open, open+close - Input/TextField: mount, type 10 characters Scripts: pnpm perf, pnpm perf:baseline, pnpm perf:compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/benchmarks/package.json`:
- Line 12: The perf:compare script references `@callstack/reassure-compare` but
the package is missing from package.json; update package.json by adding
"@callstack/reassure-compare" to devDependencies (choose an appropriate semver
version or set to latest) so the "perf:compare" script can resolve, then run the
package manager install to update lockfiles.
In `@packages/benchmarks/src/__perf__/base-ui/Input.perf-test.tsx`:
- Around line 27-45: The test currently fires a single change event with the
full string which collapses 10 keystrokes; instead make the benchmark use a
small controlled harness and dispatch sequential per-character changes so each
keystroke goes through the controlled onChange path. Concretely, render a
controlled Input (use Input, Input.Control inside the measureRenders wrapper
ThemeWrapper) with a value state and onChange handler, then in the scenario loop
over each character of "helloworld" and call fireEvent.change (or
fireEvent.input) on the element for each incremental value so each keystroke
triggers React updates measured by measureRenders.
In `@packages/benchmarks/src/__perf__/base-ui/Select.perf-test.tsx`:
- Around line 58-64: The scenario currently clicks the trigger (trigger =
screen.getByRole('combobox')) and returns immediately, missing async rendering
of the popup; update the scenario in Select.perf-test.tsx to await the opened
listbox/popup after fireEvent.click(trigger) (e.g. use await
screen.findByRole('listbox') or await waitFor(() =>
screen.getByRole('listbox'))) so the benchmark measures the full "open dropdown"
path.
In `@packages/benchmarks/src/setup-perf.ts`:
- Around line 5-9: The code calculates reassureDir using __dirname which is
undefined in ESM; update the directory resolution to use import.meta.url with
the URL constructor (e.g., new URL('../.reassure', import.meta.url)) and then
pass its pathname into path.resolve or path.fromFileUrl equivalent; keep the
existing checks and creation logic (fs.existsSync(reassureDir) and
fs.mkdirSync(reassureDir, { recursive: true })) but replace any references to
__dirname with the import.meta.url-based URL resolution so the reassureDir
variable is correctly computed in ESM.
In `@packages/benchmarks/vitest.config.perf.ts`:
- Around line 14-58: The export default is using an unnecessary async factory
and a type assertion; change the default export to a synchronous factory by
removing the async keyword and the trailing "as ReturnType<typeof defineConfig>"
cast, returning the same config object directly from defineConfig; locate the
export default block that calls defineConfig and update it so it is a plain
(non-async) function returning the config object and drop the type assertion.
- Around line 26-31: The file uses __dirname in path.resolve calls (e.g., the
replacement entries for '@omniview/base-ui', '@omniview/editors', and the
react-syntax-highlighter stub) but __dirname is not available in ESM; derive it
from import.meta.url by creating a __filename via fileURLToPath(import.meta.url)
and then __dirname via dirname(__filename), and then use that __dirname with
path.resolve (or replace path.resolve(__dirname, ...) with resolve(__dirname,
...)) so the existing replacement entries continue to work in ESM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef038476-1b1e-4412-ac47-d94c36382d56
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/benchmarks/.gitignorepackages/benchmarks/package.jsonpackages/benchmarks/scripts/perf-compare.mjspackages/benchmarks/src/__perf__/base-ui/Accordion.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Button.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Checkbox.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Dialog.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Input.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Select.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Tabs.perf-test.tsxpackages/benchmarks/src/__perf__/utils/theme-wrapper.tsxpackages/benchmarks/src/setup-perf.tspackages/benchmarks/src/setup.tspackages/benchmarks/vitest.config.perf.ts
Comment on lines
+14
to
+58
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export default defineConfig(async () => { | ||
| return { | ||
| plugins: [ | ||
| react({ | ||
| babel: { | ||
| plugins: [['babel-plugin-react-compiler', {}]], | ||
| }, | ||
| }), | ||
| ], | ||
| resolve: { | ||
| alias: [ | ||
| { find: '@omniview/base-ui', replacement: path.resolve(__dirname, '../base-ui/src/index.ts') }, | ||
| { find: '@omniview/editors', replacement: path.resolve(__dirname, '../editors/src/index.ts') }, | ||
| { | ||
| find: /^react-syntax-highlighter(\/.*)?$/, | ||
| replacement: path.resolve(__dirname, 'src/stubs/react-syntax-highlighter.ts'), | ||
| }, | ||
| ], | ||
| }, | ||
| test: { | ||
| include: ['src/**/*.perf-test.{ts,tsx}'], | ||
| environment: 'jsdom', | ||
| // Reassure's writeTestStats uses expect.getState().currentTestName | ||
| globals: true, | ||
| setupFiles: ['./src/setup.ts', './src/setup-perf.ts'], | ||
| pool: 'forks', | ||
| fileParallelism: false, | ||
| // Reassure recommends --no-turbofan --no-sparkplug for stable measurements. | ||
| // --expose-gc lets Reassure force GC between runs to reduce noise. | ||
| forks: { | ||
| execArgv: [ | ||
| '--expose-gc', | ||
| '--no-turbofan', | ||
| '--no-sparkplug', | ||
| '--hash-seed=1', | ||
| '--random-seed=1', | ||
| '--max-old-space-size=4096', | ||
| ], | ||
| }, | ||
| // Reassure tests are slow (20 runs + warmup per test). Generous timeout. | ||
| testTimeout: 60_000, | ||
| }, | ||
| }; | ||
| }) as ReturnType<typeof defineConfig>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Async config factory with type assertion is acceptable but consider simplifying.
The as ReturnType<typeof defineConfig> cast works around TypeScript's handling of the async factory pattern. This is a known workaround and functions correctly, but you could simplify by not using async since nothing inside awaits:
-export default defineConfig(async () => {
- return {
+export default defineConfig({
plugins: [
// ...
],
// ...
- };
-}) as ReturnType<typeof defineConfig>;
+});📝 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.
Suggested change
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| export default defineConfig(async () => { | |
| return { | |
| plugins: [ | |
| react({ | |
| babel: { | |
| plugins: [['babel-plugin-react-compiler', {}]], | |
| }, | |
| }), | |
| ], | |
| resolve: { | |
| alias: [ | |
| { find: '@omniview/base-ui', replacement: path.resolve(__dirname, '../base-ui/src/index.ts') }, | |
| { find: '@omniview/editors', replacement: path.resolve(__dirname, '../editors/src/index.ts') }, | |
| { | |
| find: /^react-syntax-highlighter(\/.*)?$/, | |
| replacement: path.resolve(__dirname, 'src/stubs/react-syntax-highlighter.ts'), | |
| }, | |
| ], | |
| }, | |
| test: { | |
| include: ['src/**/*.perf-test.{ts,tsx}'], | |
| environment: 'jsdom', | |
| // Reassure's writeTestStats uses expect.getState().currentTestName | |
| globals: true, | |
| setupFiles: ['./src/setup.ts', './src/setup-perf.ts'], | |
| pool: 'forks', | |
| fileParallelism: false, | |
| // Reassure recommends --no-turbofan --no-sparkplug for stable measurements. | |
| // --expose-gc lets Reassure force GC between runs to reduce noise. | |
| forks: { | |
| execArgv: [ | |
| '--expose-gc', | |
| '--no-turbofan', | |
| '--no-sparkplug', | |
| '--hash-seed=1', | |
| '--random-seed=1', | |
| '--max-old-space-size=4096', | |
| ], | |
| }, | |
| // Reassure tests are slow (20 runs + warmup per test). Generous timeout. | |
| testTimeout: 60_000, | |
| }, | |
| }; | |
| }) as ReturnType<typeof defineConfig>; | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` | |
| export default defineConfig({ | |
| plugins: [ | |
| react({ | |
| babel: { | |
| plugins: [['babel-plugin-react-compiler', {}]], | |
| }, | |
| }), | |
| ], | |
| resolve: { | |
| alias: [ | |
| { find: '@omniview/base-ui', replacement: path.resolve(__dirname, '../base-ui/src/index.ts') }, | |
| { find: '@omniview/editors', replacement: path.resolve(__dirname, '../editors/src/index.ts') }, | |
| { | |
| find: /^react-syntax-highlighter(\/.*)?$/, | |
| replacement: path.resolve(__dirname, 'src/stubs/react-syntax-highlighter.ts'), | |
| }, | |
| ], | |
| }, | |
| test: { | |
| include: ['src/**/*.perf-test.{ts,tsx}'], | |
| environment: 'jsdom', | |
| // Reassure's writeTestStats uses expect.getState().currentTestName | |
| globals: true, | |
| setupFiles: ['./src/setup.ts', './src/setup-perf.ts'], | |
| pool: 'forks', | |
| fileParallelism: false, | |
| // Reassure recommends --no-turbofan --no-sparkplug for stable measurements. | |
| // --expose-gc lets Reassure force GC between runs to reduce noise. | |
| forks: { | |
| execArgv: [ | |
| '--expose-gc', | |
| '--no-turbofan', | |
| '--no-sparkplug', | |
| '--hash-seed=1', | |
| '--random-seed=1', | |
| '--max-old-space-size=4096', | |
| ], | |
| }, | |
| // Reassure tests are slow (20 runs + warmup per test). Generous timeout. | |
| testTimeout: 60_000, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/vitest.config.perf.ts` around lines 14 - 58, The export
default is using an unnecessary async factory and a type assertion; change the
default export to a synchronous factory by removing the async keyword and the
trailing "as ReturnType<typeof defineConfig>" cast, returning the same config
object directly from defineConfig; locate the export default block that calls
defineConfig and update it so it is a plain (non-async) function returning the
config object and drop the type assertion.
- Add @callstack/reassure-compare to devDependencies (pnpm strict mode) - Rewrite Input typing test with controlled component and per-keystroke changes - Await listbox render after Select dropdown open - Replace __dirname with import.meta.url derivation in ESM files - Document async factory + cast rationale in vitest.config.perf.ts
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 `@packages/benchmarks/package.json`:
- Around line 10-11: Replace the POSIX-only package.json script commands ("perf"
and "perf:baseline") with a cross-platform Node script: create a script file
(e.g., packages/benchmarks/scripts/run-perf.mjs) that uses rmSync to remove the
target file, sets REASSURE_OUTPUT_FILE in the env when mode is "baseline", and
spawnSync to run vitest (detecting Windows to call "pnpm.cmd" vs "pnpm"); then
update the "perf" script to call node run-perf.mjs and the "perf:baseline"
script to call node run-perf.mjs baseline so the cleanup and env setup are
platform-independent.
In `@packages/benchmarks/vitest.config.perf.ts`:
- Around line 18-20: The Vitest config is placing Node flags under the
unsupported key test.forks (and possibly test.poolOptions.forks.execArgv); move
those V8/Node flags up into the Vitest 4.0 supported property by setting
test.execArgv to the array of flags (replace any test.forks or
test.poolOptions.forks.execArgv usage), ensuring the deterministic V8 flags you
intended are provided via test.execArgv in the exported config (update
references in the module that build the config object).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 784fa40b-673e-4cf4-8be7-5c0ab86bd14f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/benchmarks/package.jsonpackages/benchmarks/src/__perf__/base-ui/Input.perf-test.tsxpackages/benchmarks/src/__perf__/base-ui/Select.perf-test.tsxpackages/benchmarks/src/setup-perf.tspackages/benchmarks/vitest.config.perf.ts
Comment on lines
+10
to
+11
| "perf": "rm -f .reassure/current.perf && vitest --run --config vitest.config.perf.ts", | ||
| "perf:baseline": "rm -f .reassure/baseline.perf && REASSURE_OUTPUT_FILE=.reassure/baseline.perf vitest --run --config vitest.config.perf.ts", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/benchmarks/package.json | head -30Repository: omniviewdev/ui
Length of output: 1449
🏁 Script executed:
# Check if there are other scripts in the project that handle cross-platform concerns
rg "scripts" -A 20 package.json | head -100Repository: omniviewdev/ui
Length of output: 1675
🏁 Script executed:
# Look for existing cross-platform patterns (e.g., cross-env, cross-spawn usage)
rg "cross-env|cross-spawn|node scripts" --type json -B 2 -A 2Repository: omniviewdev/ui
Length of output: 977
🏁 Script executed:
# Check root package.json to understand project's approach
cat -n package.json | head -50Repository: omniviewdev/ui
Length of output: 2164
🏁 Script executed:
cat packages/benchmarks/scripts/perf-compare.mjs 2>/dev/null | head -50Repository: omniviewdev/ui
Length of output: 589
🏁 Script executed:
# Check if there's a Node version requirement or Windows-specific handling in the project
git log --oneline -1 && head -5 .gitignoreRepository: omniviewdev/ui
Length of output: 186
Use cross-platform approach for perf scripts.
Lines 10-11 use POSIX-only shell syntax (rm -f and REASSURE_OUTPUT_FILE=... env assignment) that will fail on Windows. Since the project already uses Node scripts for similar tooling (perf-compare.mjs), move the cleanup and environment setup into a Node script to ensure perf and perf:baseline work consistently across platforms.
Cross-platform solution
"scripts": {
"bench": "vitest bench --run",
"bench:browser": "vitest bench --config vitest.config.browser.ts --run",
- "perf": "rm -f .reassure/current.perf && vitest --run --config vitest.config.perf.ts",
- "perf:baseline": "rm -f .reassure/baseline.perf && REASSURE_OUTPUT_FILE=.reassure/baseline.perf vitest --run --config vitest.config.perf.ts",
+ "perf": "node scripts/run-perf.mjs current",
+ "perf:baseline": "node scripts/run-perf.mjs baseline",
"perf:compare": "node scripts/perf-compare.mjs"
},// packages/benchmarks/scripts/run-perf.mjs
import { rmSync } from 'node:fs';
import { spawnSync } from 'node:child_process';
const mode = process.argv[2];
const output =
mode === 'baseline'
? '.reassure/baseline.perf'
: '.reassure/current.perf';
rmSync(output, { force: true });
const env =
mode === 'baseline'
? { ...process.env, REASSURE_OUTPUT_FILE: output }
: process.env;
const cmd = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm';
const { status } = spawnSync(
cmd,
['exec', 'vitest', '--run', '--config', 'vitest.config.perf.ts'],
{ stdio: 'inherit', env }
);
process.exit(status ?? 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "perf": "rm -f .reassure/current.perf && vitest --run --config vitest.config.perf.ts", | |
| "perf:baseline": "rm -f .reassure/baseline.perf && REASSURE_OUTPUT_FILE=.reassure/baseline.perf vitest --run --config vitest.config.perf.ts", | |
| "perf": "node scripts/run-perf.mjs current", | |
| "perf:baseline": "node scripts/run-perf.mjs baseline", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/package.json` around lines 10 - 11, Replace the
POSIX-only package.json script commands ("perf" and "perf:baseline") with a
cross-platform Node script: create a script file (e.g.,
packages/benchmarks/scripts/run-perf.mjs) that uses rmSync to remove the target
file, sets REASSURE_OUTPUT_FILE in the env when mode is "baseline", and
spawnSync to run vitest (detecting Windows to call "pnpm.cmd" vs "pnpm"); then
update the "perf" script to call node run-perf.mjs and the "perf:baseline"
script to call node run-perf.mjs baseline so the cleanup and env setup are
platform-independent.
Comment on lines
+18
to
+20
| * Note: async factory + cast mirrors vitest.config.ts — needed because Vitest's | ||
| * InlineConfig type doesn't include `forks` even though the runtime supports it. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Vitest versions declared in the repo ==="
rg -n --glob 'package.json' '"vitest"|"@vitest/' .
echo
echo "=== Current perf config block ==="
sed -n '40,62p' packages/benchmarks/vitest.config.perf.tsRepository: omniviewdev/ui
Length of output: 1356
🌐 Web query:
Vitest 4.0 configuration poolOptions forks execArgv
💡 Result:
In Vitest 4.0, test.poolOptions (including poolOptions.forks.execArgv) was removed. What used to be configured under poolOptions.{pool}.* is now configured with top-level test options. Specifically, to pass Node flags to the forks pool, use test.execArgv. [1]
Vitest 4.x config (replacement for poolOptions.forks.execArgv)
// vitest.config.ts
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
pool: 'forks', // default is 'forks'
execArgv: ['--expose-gc'], // <-- replaces poolOptions.forks.execArgv
},
})execArgv “Pass[es] additional arguments to node in the runner worker” (applies when spawning worker processes such as child_process for forks). [2]
CLI equivalent
vitest --pool forks --execArgv --expose-gcVitest exposes this as --execArgv <option> and --pool <pool>. [3]
Related poolOptions migrations you might also need
singleFork→maxWorkers: 1andisolate: false[1]- VM pool
memoryLimit→vmMemoryLimit[1]
Sources: Migration guide (Vitest 4.0) [1], execArgv config reference [2], CLI reference [3].
Move the Node flags out of test.forks and use the correct Vitest 4.0 syntax.
test.forks is not a supported Vitest config key. In Vitest 4.0, worker options are configured directly under test — specifically, use test.execArgv to pass Node flags to the forks pool, not test.poolOptions.forks.execArgv (which was removed in v4.0). Without this fix, the deterministic V8 settings you intended will not be applied. (Vitest 4.0 Migration Guide)
🛠️ Proposed fix
test: {
include: ['src/**/*.perf-test.{ts,tsx}'],
environment: 'jsdom',
// Reassure's writeTestStats uses expect.getState().currentTestName
globals: true,
setupFiles: ['./src/setup.ts', './src/setup-perf.ts'],
pool: 'forks',
fileParallelism: false,
// Reassure recommends --no-turbofan --no-sparkplug for stable measurements.
// --expose-gc lets Reassure force GC between runs to reduce noise.
- forks: {
- execArgv: [
- '--expose-gc',
- '--no-turbofan',
- '--no-sparkplug',
- '--hash-seed=1',
- '--random-seed=1',
- '--max-old-space-size=4096',
- ],
- },
+ execArgv: [
+ '--expose-gc',
+ '--no-turbofan',
+ '--no-sparkplug',
+ '--hash-seed=1',
+ '--random-seed=1',
+ '--max-old-space-size=4096',
+ ],
// Reassure tests are slow (20 runs + warmup per test). Generous timeout.
testTimeout: 60_000,
},📝 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.
Suggested change
| * Note: async factory + cast mirrors vitest.config.ts — needed because Vitest's | |
| * InlineConfig type doesn't include `forks` even though the runtime supports it. | |
| */ | |
| test: { | |
| include: ['src/**/*.perf-test.{ts,tsx}'], | |
| environment: 'jsdom', | |
| // Reassure's writeTestStats uses expect.getState().currentTestName | |
| globals: true, | |
| setupFiles: ['./src/setup.ts', './src/setup-perf.ts'], | |
| pool: 'forks', | |
| fileParallelism: false, | |
| // Reassure recommends --no-turbofan --no-sparkplug for stable measurements. | |
| // --expose-gc lets Reassure force GC between runs to reduce noise. | |
| execArgv: [ | |
| '--expose-gc', | |
| '--no-turbofan', | |
| '--no-sparkplug', | |
| '--hash-seed=1', | |
| '--random-seed=1', | |
| '--max-old-space-size=4096', | |
| ], | |
| // Reassure tests are slow (20 runs + warmup per test). Generous timeout. | |
| testTimeout: 60_000, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/vitest.config.perf.ts` around lines 18 - 20, The Vitest
config is placing Node flags under the unsupported key test.forks (and possibly
test.poolOptions.forks.execArgv); move those V8/Node flags up into the Vitest
4.0 supported property by setting test.execArgv to the array of flags (replace
any test.forks or test.poolOptions.forks.execArgv usage), ensuring the
deterministic V8 flags you intended are provided via test.execArgv in the
exported config (update references in the module that build the config object).
5 tasks
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
Tier 1 components (mount + rerender + mountMany)
DataTable, TreeList, CommandList, BasicList, SelectableList, RowList, Autocomplete, Combobox, MultiSelect, FilterBar, TagInput, Dialog, Drawer, Sheet, ContextMenu, Popover, DockLayout, ResizableSplitPane, EditorTabs
Early optimization targets identified
Test plan
pnpm bench)Summary by CodeRabbit
Tests
New Features
Chores