feat(benchmarks): competitive benchmarks — @omniview/base-ui vs MUI vs raw HTML#30
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds competitive benchmarks and tooling: new Taskfile/npm scripts and Vitest competitive config, multiple competitive suites and utils, MUI theme customizations and wrappers, and a report generator that converts Vitest JSON results into a Markdown report. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Vitest as Vitest Runner
participant BenchUtils as bench-compare
participant Wrappers as wrapOv/wrapMui/wrapRaw
participant JSDOM as JSDOM Renderer
participant FS as Filesystem
participant Reporter as generate-competitive-report.js
Vitest->>BenchUtils: run benchmark suites
BenchUtils->>Wrappers: request wrapped render trees
Wrappers->>JSDOM: mount wrapped elements
JSDOM-->>BenchUtils: return measurements (ops/s)
BenchUtils-->>Vitest: report results
Vitest->>FS: write JSON results (`results/competitive/latest.json`)
Reporter->>FS: read `results/competitive/latest.json`
Reporter->>Reporter: parse, compute overheads & geo-means
Reporter->>FS: write `results/competitive/report.md`
Reporter-->>Vitest: log completion
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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
🤖 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/scripts/generate-competitive-report.js`:
- Around line 82-90: The code sets rawHz, ovHz, and muiHz to 0 when a benchmark
is missing, which yields misleading finite ratios; change the defaults to
Number.NaN (i.e., use g.impls['raw'] ?? Number.NaN, g.impls['@omniview/base-ui']
?? Number.NaN, g.impls['@mui/material'] ?? Number.NaN) so missing values
propagate as NaN and are filtered out by the existing Number.isFinite() checks
before pushing into overheadValues.ov and overheadValues.mui.
In
`@packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsx`:
- Around line 342-347: In inputs.tsx update the transition value in the checkbox
style object so the color properties animate over 120ms with ease-in; replace
the current shorthand "transition" entry (the object containing border,
borderColor, backgroundColor, boxShadow, transition and '&:hover') with a proper
explicit transition string like "border-color 120ms ease-in, background-color
120ms ease-in" so both borderColor and backgroundColor animate instead of
parsing as separate zero-duration transitions.
In
`@packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx`:
- Around line 65-74: The MuiSelect customization in
createNavigationCustomizations() is overwriting the MuiSelect from
createInputsCustomizations() when both objects are shallow-spread in theme.tsx;
fix by consolidating or deep-merging the two so styleOverrides.select and
defaultProps are combined (e.g., in createInputsCustomizations() or a new shared
merge step, merge the MuiSelect entries to include both styleOverrides.select
keys: display/alignItems from createNavigationCustomizations and
overflow/flexWrap from createInputsCustomizations, and preserve the
IconComponent defaultProp you want to keep). Update theme.tsx to spread a single
merged object (or perform a deep merge of createInputsCustomizations() and
createNavigationCustomizations()) so no MuiSelect keys are lost.
- Around line 43-46: In MuiMenu.styleOverrides.list the selector
`&.${dividerClasses.root}` targets the list element itself but dividers are
child nodes, so change the selector to use a descendant combinator (e.g. `&
.${dividerClasses.root}`) so the margin rule applies to Divider children; update
the selector where `list` is defined in the navigation customizations to
reference `dividerClasses.root` as a descendant.
In `@packages/benchmarks/src/competitive/implementations/mui-theme/primitives.ts`:
- Line 28: Replace the empty interface declaration to satisfy the
no-empty-object-type lint rule: change the declaration of PaletteColor that
currently reads "interface PaletteColor extends ColorRange {}" to a non-empty
construct—preferably convert it to a type alias "type PaletteColor = ColorRange"
(or alternatively add a property or add an inline eslint-disable-next-line
comment). Update the symbol PaletteColor accordingly and ensure references to
PaletteColor continue to work with the new type alias.
In `@packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx`:
- Around line 9-10: Replace the unconventional direct import of the declaration
file (the statement importing './mui.d.ts' in theme.tsx) with a proper
TypeScript declaration inclusion: remove the import from
packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx and
either add a triple-slash reference directive to mui.d.ts at the top of files
that need it or (preferably) add mui.d.ts to your tsconfig.json "include" so the
module augmentation in mui.d.ts is picked up automatically; ensure the symbol
name mui.d.ts is used when updating tsconfig or the reference so the custom
variants (soft, link, xs, xl) remain available.
In `@packages/benchmarks/src/competitive/Switch.competitive.tsx`:
- Line 9: The raw benchmark case using role="switch" is missing the required
aria-checked attribute; update the 'raw' entry that returns wrapRaw(...) so the
input includes an explicit aria-checked value (e.g., aria-checked="false" for
the unchecked baseline) — locate the 'raw' mapping and add aria-checked="false"
to the <input> element returned by wrapRaw.
In `@packages/benchmarks/src/utils/bench-compare.ts`:
- Around line 50-62: The current benchCompareMany loop creates children and
wrapped elements outside the bench callback (using implementations, wrappers,
createElement and wrap) so element construction is excluded from timing, unlike
benchCompare which calls renderFn() inside the callback; either document this
behavioral difference or move element creation into the bench callback so timing
includes creation: update the loop around bench(`${suiteName} [${label}]`, ...)
to call factory(i), wrap(createElement(...)) and build the children array inside
the bench callback (where render and cleanup are called), or add a clear comment
next to the implementations/wrappers usage indicating that creation is
intentionally done once to measure only mount cost.
In `@packages/benchmarks/vitest.config.competitive.ts`:
- Around line 9-44: The export includes an unnecessary type assertion "as
ReturnType<typeof defineConfig>"—remove that cast and simply export the result
of defineConfig (i.e. change "export default defineConfig(async () => { ... })
as ReturnType<typeof defineConfig>;" to "export default defineConfig(async () =>
{ ... });") so the module uses defineConfig's inferred return type directly;
keep the existing async callback, plugins, resolve, and test configuration
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1316868f-cbc4-4f67-be7c-a5c4984b28eb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
Taskfile.ymlpackage.jsonpackages/benchmarks/README.mdpackages/benchmarks/package.jsonpackages/benchmarks/scripts/generate-competitive-report.jspackages/benchmarks/src/competitive/Button.competitive.tsxpackages/benchmarks/src/competitive/Checkbox.competitive.tsxpackages/benchmarks/src/competitive/Popover.competitive.tsxpackages/benchmarks/src/competitive/Select.competitive.tsxpackages/benchmarks/src/competitive/Switch.competitive.tsxpackages/benchmarks/src/competitive/TextField.competitive.tsxpackages/benchmarks/src/competitive/Tooltip.competitive.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/dataDisplay.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/feedback.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/mui.d.tspackages/benchmarks/src/competitive/implementations/mui-theme/primitives.tspackages/benchmarks/src/competitive/implementations/mui-theme/theme.tsxpackages/benchmarks/src/competitive/implementations/wrappers.tsxpackages/benchmarks/src/utils/bench-compare.tspackages/benchmarks/src/utils/index.tspackages/benchmarks/vitest.config.competitive.ts
Comment on lines
+65
to
+74
| MuiSelect: { | ||
| defaultProps: { | ||
| IconComponent: React.forwardRef<SVGSVGElement, SvgIconProps>((props, ref) => ( | ||
| <UnfoldMoreRoundedIcon fontSize="small" {...props} ref={ref} /> | ||
| )), | ||
| }, | ||
| styleOverrides: { | ||
| select: { display: 'flex', alignItems: 'center' }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- theme composition ---\n'
sed -n '1,220p' packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx
printf '\n--- navigation MuiSelect ---\n'
sed -n '65,90p' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- inputs MuiSelect ---\n'
sed -n '360,385p' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxRepository: omniviewdev/ui
Length of output: 2819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- createNavigationCustomizations return structure ---\n'
rg -A 3 'export.*createNavigationCustomizations' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- createInputsCustomizations return structure ---\n'
rg -A 3 'export.*createInputsCustomizations' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxRepository: omniviewdev/ui
Length of output: 712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Full createNavigationCustomizations function ---\n'
rg -B 2 'export.*createNavigationCustomizations' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx | head -1
# Get line number and show full function
line_num=$(rg -n 'export.*createNavigationCustomizations' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx | cut -d: -f1)
start_line=$((line_num - 5))
wc_lines=$(wc -l < packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx)
sed -n "${start_line},${wc_lines}p" packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx | head -20Repository: omniviewdev/ui
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Full navigation.tsx file ---\n'
wc -l packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- Full inputs.tsx file ---\n'
wc -l packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsx
printf '\n--- Last 30 lines of navigation.tsx ---\n'
tail -30 packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- Last 30 lines of inputs.tsx ---\n'
tail -30 packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxRepository: omniviewdev/ui
Length of output: 2339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Return statement in navigation.tsx ---\n'
rg -n 'return' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- Return statement in inputs.tsx ---\n'
rg -n 'return' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsx
printf '\n--- navigation.tsx lines 5-50 (to see function body start) ---\n'
sed -n '5,50p' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx
printf '\n--- inputs.tsx lines 5-50 (to see function body start) ---\n'
sed -n '5,50p' packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxRepository: omniviewdev/ui
Length of output: 3732
MuiSelect is being overwritten — inputs' styleOverrides will be lost.
This file's MuiSelect (lines 65-74) and packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsx's MuiSelect (lines 364-377) both export objects merged in theme.tsx via shallow spreads: ...createInputsCustomizations() then ...createNavigationCustomizations(). The second spread completely replaces the first, so inputs' styleOverrides.select rules (overflow: 'hidden' and the nested flexWrap: 'nowrap' rule) are discarded.
Consolidate both MuiSelect customizations into a single definition, or use a deep merge strategy (e.g., combine the returned objects before spreading).
🧰 Tools
🪛 ESLint
[error] 67-69: Component definition is missing display name
(react/display-name)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsx`
around lines 65 - 74, The MuiSelect customization in
createNavigationCustomizations() is overwriting the MuiSelect from
createInputsCustomizations() when both objects are shallow-spread in theme.tsx;
fix by consolidating or deep-merging the two so styleOverrides.select and
defaultProps are combined (e.g., in createInputsCustomizations() or a new shared
merge step, merge the MuiSelect entries to include both styleOverrides.select
keys: display/alignItems from createNavigationCustomizations and
overflow/flexWrap from createInputsCustomizations, and preserve the
IconComponent defaultProp you want to keep). Update theme.tsx to spread a single
merged object (or perform a deep merge of createInputsCustomizations() and
createNavigationCustomizations()) so no MuiSelect keys are lost.
Merging this PR will degrade performance by 12.22%
Performance Changes
Comparing 
Footnotes
|
…-ui vs MUI vs raw HTML Introduces side-by-side performance comparison benchmarks with: - benchCompare/benchCompareMany utilities for cross-library benchmarks - 7 component suites: Button, Checkbox, Switch, Select, TextField, Tooltip, Popover - Vendored production MUI theme matching IDE environment - Separate vitest.config.competitive.ts and Taskfile tasks - Report generator with per-component overhead multipliers and geometric mean
- Use NaN defaults for missing benchmark impls to avoid misleading ratios - Fix CSS transition shorthand in checkbox styles - Consolidate MuiSelect customizations into inputs.tsx (was split across two files) - Fix divider selector to use descendant combinator in MuiMenu - Add eslint-disable for required empty interface in MUI module augmentation - Replace direct .d.ts import with triple-slash reference directive - Add aria-checked to raw switch benchmark for a11y correctness - Document intentional element pre-creation in benchCompareMany - Remove unnecessary type assertion from competitive vitest config
joshuapare
force-pushed
the
feat/benchmarks-comparative
branch
from
14e85d4 to
9c00cfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/scripts/generate-competitive-report.js`:
- Around line 81-95: The report rows are emitted in the iteration order of the
Map/objects in groups which makes output non-deterministic; before building
lines, convert the groups iterable into an array and sort it by the composite
key (e.g. `${g.suite}|${g.benchName}`) so rows are deterministic, then iterate
the sorted array to compute rawHz/ovHz/muiHz, push into overheadValues (ov/mui)
and lines using the existing formatHz and formatOverhead helpers; ensure you
reference the same identifiers (groups, overheadValues, lines, formatHz,
formatOverhead) and replace the existing for-loop over groups with a sorted
iteration.
In
`@packages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsx`:
- Around line 366-371: The inline anonymous forwardRef used for
MuiSelect.defaultProps.IconComponent (currently React.forwardRef((props, ref) =>
<UnfoldMoreRoundedIcon ... />)) causes react/display-name; extract it to a named
forwarded component (e.g., SelectIcon or SelectIconForwardRef) defined outside
the file-scope where MuiSelect is configured, implement it with
React.forwardRef<SVGSVGElement, SvgIconProps>(function SelectIcon(props, ref) {
... }), and then set MuiSelect.defaultProps.IconComponent to that named
component so the linter can pick up the displayName.
- Around line 386-389: Replace the explicit any cast in the MuiAutocomplete
inputRoot style (currently "flexWrap: 'nowrap !important' as any") by using
MUI's selector specificity pattern; update the inputRoot entry (in the
inputs.tsx customization for MuiAutocomplete) to remove the type assertion and
instead wrap the rule under the '&&' key (e.g., '&&': { flexWrap: 'nowrap' }) so
you get higher specificity without using any casts.
In `@packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx`:
- Line 9: Move the triple-slash reference directive /// <reference
path="./mui.d.ts" /> to the very top of the file before any imports so
TypeScript recognizes the module augmentation; locate the directive in theme.tsx
(currently after imports) and cut/paste it to the first line of the file,
ensuring it precedes all import statements so the custom MUI variants (soft,
link, xs, xl) from mui.d.ts are applied.
In `@packages/benchmarks/vitest.config.competitive.ts`:
- Around line 19-24: The competitive Vitest config's resolve.alias block (the
object that currently maps '@omniview/base-ui' and '@omniview/editors') is
missing the stub alias for 'react-syntax-highlighter'; add an alias entry for
'react-syntax-highlighter' that points to the same stub module used in the
standard config (the runtime/no-op stub) so the alias set in the resolve.alias
object matches the one in vitest.config and prevents future transitive imports
from breaking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b6addce-f8f5-4dea-b895-5b5e0c49dbff
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
Taskfile.ymlpackage.jsonpackages/benchmarks/README.mdpackages/benchmarks/package.jsonpackages/benchmarks/scripts/generate-competitive-report.jspackages/benchmarks/src/competitive/Button.competitive.tsxpackages/benchmarks/src/competitive/Checkbox.competitive.tsxpackages/benchmarks/src/competitive/Popover.competitive.tsxpackages/benchmarks/src/competitive/Select.competitive.tsxpackages/benchmarks/src/competitive/Switch.competitive.tsxpackages/benchmarks/src/competitive/TextField.competitive.tsxpackages/benchmarks/src/competitive/Tooltip.competitive.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/dataDisplay.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/feedback.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/customizations/navigation.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/mui.d.tspackages/benchmarks/src/competitive/implementations/mui-theme/primitives.tspackages/benchmarks/src/competitive/implementations/mui-theme/theme.tsxpackages/benchmarks/src/competitive/implementations/wrappers.tsxpackages/benchmarks/src/utils/bench-compare.tspackages/benchmarks/src/utils/index.tspackages/benchmarks/vitest.config.competitive.ts
- Sort report rows by suite|benchName for deterministic output - Extract named SelectIcon forwardRef component for displayName - Replace `as any` cast with MUI '&&' specificity pattern - Move triple-slash reference directive before imports - Add react-syntax-highlighter stub alias to competitive config
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/scripts/generate-competitive-report.js`:
- Around line 93-105: The geometric-mean is being broken by including zero
overheads because Number.isFinite allows 0 and Math.log(0) => -Infinity; update
the collection and/or aggregation to only use strictly positive overheads: in
the block that currently does if (Number.isFinite(ovOverhead))
overheadValues.ov.push(ovOverhead) and similarly for mui, change the condition
to test ovOverhead > 0 and muiOverhead > 0 (or alternatively filter
overheadValues.ov and overheadValues.mui to only values > 0 inside geoMean
before computing logSum); ensure the geoMean(values) implementation still
handles empty arrays by returning NaN as it does now.
In `@packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx`:
- Line 1: Replace the triple-slash directive in theme.tsx with a type-only
import to satisfy `@typescript-eslint/triple-slash-reference`: remove the ///
<reference ... /> line and add a type import that loads the module augmentation
(e.g., import type {} from './mui';) so mui.d.ts is still applied;
alternatively, ensure mui.d.ts is included via tsconfig.json's include array and
delete the reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2f4c09b-a0d9-4888-b540-f14841eb21da
📒 Files selected for processing (4)
packages/benchmarks/scripts/generate-competitive-report.jspackages/benchmarks/src/competitive/implementations/mui-theme/customizations/inputs.tsxpackages/benchmarks/src/competitive/implementations/mui-theme/theme.tsxpackages/benchmarks/vitest.config.competitive.ts
…-slash with type import - Filter overhead values to > 0 to prevent Math.log(0) poisoning geoMean - Replace /// <reference> directive with type-only import for mui.d.ts
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 `@packages/benchmarks/scripts/generate-competitive-report.js`:
- Around line 25-27: The formatOverhead function currently only checks
Number.isFinite and lib === 0, causing it to render "0.0x" when raw is
non-positive; update formatOverhead to treat any non-positive throughput as
unavailable by returning 'N/A' when raw <= 0 or lib <= 0 (in addition to the
existing Number.isFinite checks), so both raw and lib must be finite and
strictly > 0 before computing and returning `${(raw / lib).toFixed(1)}x`.
- Around line 43-53: The current loop in generate-competitive-report.js silently
skips malformed benchmark names and accepts any impl label; update the parsing
in the for (const bench of group.benchmarks ?? []) block to fail fast: after
const match = bench.name.match(/^(.+?)\s+\[(.+)]$/) throw an Error if !match
(include bench.name in the message) instead of continuing, validate match[2]
(the impl) against an explicit allowedImpls set you add (e.g., const
ALLOWED_IMPLS = new Set([...])) and throw if the impl is unknown, and after
collecting results (the results array) throw an Error if results.length === 0 so
the script exits non-zero for empty extraction; reference the variables match,
bench.name, match[1]/match[2], results and suite.replace when implementing these
checks.
In `@packages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx`:
- Around line 10-33: The cssVariables.colorSchemeSelector value in the
createTheme call (muiTheme) is incorrect — replace the literal
'data-mui-color-scheme' with a supported pattern: either the shorthand 'data' or
a pattern including the %s placeholder like '[data-mui-color-scheme=%s]'; update
the cssVariables object inside the createTheme invocation (refer to cssVariables
and colorSchemeSelector in muiTheme) to use one of these valid values so MUI v7
will generate proper selectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95b78230-0dad-456e-99d6-8827979d282f
📒 Files selected for processing (2)
packages/benchmarks/scripts/generate-competitive-report.jspackages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx
Comment on lines
+43
to
+53
| for (const bench of group.benchmarks ?? []) { | ||
| // Parse "mount [raw]" → benchName="mount", impl="raw" | ||
| const match = bench.name.match(/^(.+?)\s+\[(.+)]$/); | ||
| if (!match) continue; | ||
|
|
||
| results.push({ | ||
| suite: suite.replace(' competitive', ''), | ||
| benchName: match[1], | ||
| impl: match[2], | ||
| hz: bench.hz, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, pathlib, re, sys
path = pathlib.Path("packages/benchmarks/results/competitive/latest.json")
if not path.exists():
sys.exit("Generate packages/benchmarks/results/competitive/latest.json before running this check.")
data = json.loads(path.read_text())
pattern = re.compile(r'^(.+?)\s+\[(.+)\]$')
expected = {"raw", "@omniview/base-ui", "@mui/material"}
total = 0
bad_names = []
unknown_impls = []
for file in data.get("files", []):
for group in file.get("groups", []):
for bench in group.get("benchmarks", []):
total += 1
name = bench.get("name", "")
match = pattern.match(name)
if not match:
bad_names.append((group.get("fullName"), name))
continue
if match.group(2) not in expected:
unknown_impls.append((group.get("fullName"), name, match.group(2)))
print({"total_benchmarks": total, "bad_names": len(bad_names), "unknown_impls": len(unknown_impls)})
for row in bad_names[:20]:
print("bad_name:", row)
for row in unknown_impls[:20]:
print("unknown_impl:", row)
PYRepository: omniviewdev/ui
Length of output: 145
🏁 Script executed:
cat -n packages/benchmarks/scripts/generate-competitive-report.js | head -80Repository: omniviewdev/ui
Length of output: 3158
🏁 Script executed:
cat -n packages/benchmarks/scripts/generate-competitive-report.js | sed -n '80,120p'Repository: omniviewdev/ui
Length of output: 1757
🏁 Script executed:
cat -n packages/benchmarks/scripts/generate-competitive-report.js | sed -n '120,160p'Repository: omniviewdev/ui
Length of output: 602
🏁 Script executed:
# Check if there are any tests for this script
find packages/benchmarks -type f -name "*.test.*" -o -name "*.spec.*" | head -20Repository: omniviewdev/ui
Length of output: 108
🏁 Script executed:
# Look for example or actual benchmark results
find packages/benchmarks/results -type f -name "*.json" 2>/dev/null | head -10Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Check the test file to see if there are validation tests
cat packages/benchmarks/src/utils/bench-render.test.tsRepository: omniviewdev/ui
Length of output: 1890
🏁 Script executed:
# Look for vitest configuration and benchmark files
find packages/benchmarks -name "*.config.*" -o -name "vitest.config.*" | head -10Repository: omniviewdev/ui
Length of output: 230
🏁 Script executed:
# Check the competitive vitest config
cat packages/benchmarks/vitest.config.competitive.tsRepository: omniviewdev/ui
Length of output: 1310
🏁 Script executed:
# Look for competitive benchmark files
find packages/benchmarks/src/competitive -name "*.competitive.*" 2>/dev/null | head -5Repository: omniviewdev/ui
Length of output: 355
🏁 Script executed:
# Examine one benchmark file to see the naming pattern
cat packages/benchmarks/src/competitive/Button.competitive.tsx | head -60Repository: omniviewdev/ui
Length of output: 954
🏁 Script executed:
# Check the bench-compare utilities to see how benchmark names are generated
find packages/benchmarks -name "*bench-compare*" -type fRepository: omniviewdev/ui
Length of output: 104
🏁 Script executed:
cat packages/benchmarks/src/utils/bench-compare.tsRepository: omniviewdev/ui
Length of output: 2192
Add validation to fail fast on unexpected benchmark names and implementation labels.
The parser silently skips any benchmarks that don't match the naming pattern, accepts any implementation label without validation, and can produce an empty report without signaling an error. This creates multiple degradation paths: typos in implementation labels result in N/A cells, malformed benchmark names are silently dropped, and an empty report (if no benchmarks match) still exits 0. Add explicit validation to throw on malformed benchmark names, unknown implementation labels, and empty extraction results.
Proposed fix
for (const bench of group.benchmarks ?? []) {
// Parse "mount [raw]" → benchName="mount", impl="raw"
const match = bench.name.match(/^(.+?)\s+\[(.+)]$/);
- if (!match) continue;
+ if (!match) {
+ throw new Error(
+ `Unsupported benchmark name "${bench.name}" in "${group.fullName ?? 'unknown'}". Expected "<metric> [<implementation>]".`,
+ );
+ }
+ if (!['raw', '@omniview/base-ui', '@mui/material'].includes(match[2])) {
+ throw new Error(
+ `Unsupported implementation label "${match[2]}" in "${bench.name}".`,
+ );
+ }
results.push({
suite: suite.replace(' competitive', ''),
benchName: match[1],
impl: match[2],
hz: bench.hz,
});
}
@@
function generateReport(data) {
const benchmarks = extractCompetitiveBenchmarks(data);
+ if (benchmarks.length === 0) {
+ throw new Error(
+ 'No competitive benchmarks were extracted from latest.json. Verify the Vitest JSON schema and benchmark naming convention.',
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/scripts/generate-competitive-report.js` around lines 43 -
53, The current loop in generate-competitive-report.js silently skips malformed
benchmark names and accepts any impl label; update the parsing in the for (const
bench of group.benchmarks ?? []) block to fail fast: after const match =
bench.name.match(/^(.+?)\s+\[(.+)]$/) throw an Error if !match (include
bench.name in the message) instead of continuing, validate match[2] (the impl)
against an explicit allowedImpls set you add (e.g., const ALLOWED_IMPLS = new
Set([...])) and throw if the impl is unknown, and after collecting results (the
results array) throw an Error if results.length === 0 so the script exits
non-zero for empty extraction; reference the variables match, bench.name,
match[1]/match[2], results and suite.replace when implementing these checks.
…x colorSchemeSelector - Return N/A when raw or lib throughput is <= 0 to avoid misleading 0.0x - Use MUI v7 %s placeholder pattern for colorSchemeSelector
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/benchmarks/scripts/generate-competitive-report.js (1)
43-47:⚠️ Potential issue | 🟠 MajorFail fast on malformed benchmark rows and unknown implementation labels.
At Line 46, malformed benchmark names are silently skipped, and
match[2]is never validated. This can generate a partial/empty report with exit code 0 instead of surfacing data-shape regressions.🔧 Proposed fix
+const ALLOWED_IMPLS = new Set(['raw', '@omniview/base-ui', '@mui/material']); + function extractCompetitiveBenchmarks(data) { const results = []; for (const file of data.files ?? []) { for (const group of file.groups ?? []) { @@ for (const bench of group.benchmarks ?? []) { // Parse "mount [raw]" → benchName="mount", impl="raw" const match = bench.name.match(/^(.+?)\s+\[(.+)]$/); - if (!match) continue; + if (!match) { + throw new Error( + `Unsupported benchmark name "${bench.name}" in "${group.fullName ?? 'unknown'}". Expected "<metric> [<implementation>]".`, + ); + } + if (!ALLOWED_IMPLS.has(match[2])) { + throw new Error(`Unsupported implementation label "${match[2]}" in "${bench.name}".`); + } @@ } + if (results.length === 0) { + throw new Error( + 'No competitive benchmarks were extracted from latest.json. Verify Vitest JSON shape and benchmark naming convention.', + ); + } return results; }#!/bin/bash set -euo pipefail python - <<'PY' import json, pathlib, re, sys p = pathlib.Path("packages/benchmarks/results/competitive/latest.json") if not p.exists(): sys.exit("Missing packages/benchmarks/results/competitive/latest.json") data = json.loads(p.read_text()) name_pat = re.compile(r'^(.+?)\s+\[(.+)]$') allowed = {"raw", "@omniview/base-ui", "@mui/material"} total = 0 bad_name = [] bad_impl = [] for f in data.get("files", []): for g in f.get("groups", []): for b in g.get("benchmarks", []): total += 1 n = b.get("name", "") m = name_pat.match(n) if not m: bad_name.append((g.get("fullName"), n)) continue if m.group(2) not in allowed: bad_impl.append((g.get("fullName"), n, m.group(2))) print({"total": total, "bad_name": len(bad_name), "bad_impl": len(bad_impl)}) for row in bad_name[:10]: print("bad_name:", row) for row in bad_impl[:10]: print("bad_impl:", row) if total == 0: print("No benchmarks found in JSON.") PYAlso applies to: 48-53, 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/benchmarks/scripts/generate-competitive-report.js` around lines 43 - 47, The loop that parses bench.name using const match = bench.name.match(/^(.+?)\s+\[(.+)]$/) silently skips malformed names and never validates match[2], which can hide data-shape regressions; update the code in the for (const bench of group.benchmarks ?? []) block to fail fast: if !match, throw or exit with a clear error including the problematic group.fullName and bench.name, and validate the implementation label (match[2]) against an allowed set (e.g., {"raw","@omniview/base-ui","@mui/material"})—on unknown labels also throw/exit with a descriptive error; apply the same strict checks where bench name parsing is repeated (the similar blocks around the later ranges) so malformed rows or unknown implementations surface immediately instead of producing a partial/empty report.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/benchmarks/scripts/generate-competitive-report.js`:
- Around line 43-47: The loop that parses bench.name using const match =
bench.name.match(/^(.+?)\s+\[(.+)]$/) silently skips malformed names and never
validates match[2], which can hide data-shape regressions; update the code in
the for (const bench of group.benchmarks ?? []) block to fail fast: if !match,
throw or exit with a clear error including the problematic group.fullName and
bench.name, and validate the implementation label (match[2]) against an allowed
set (e.g., {"raw","@omniview/base-ui","@mui/material"})—on unknown labels also
throw/exit with a descriptive error; apply the same strict checks where bench
name parsing is repeated (the similar blocks around the later ranges) so
malformed rows or unknown implementations surface immediately instead of
producing a partial/empty report.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5411dab-81b6-4152-874a-5c275c39e2e8
📒 Files selected for processing (2)
packages/benchmarks/scripts/generate-competitive-report.jspackages/benchmarks/src/competitive/implementations/mui-theme/theme.tsx
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
benchCompare/benchCompareManyutilities for cross-library benchmark suitesResults (JSDOM, local)
Test plan
pnpm bench:competitiveruns all 7 suites to completionpnpm bench:competitive:jsonwritesresults/competitive/latest.jsontask bench:competitive:reportgeneratesresults/competitive/report.mdpnpm bench) still passSummary by CodeRabbit
New Features
Documentation
Chores