Skip to content

perf: checkbox optimization — CSS indicators and flattened tree #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joshuapare merged 9 commits into from
Mar 12, 2026
Merged

perf: checkbox optimization — CSS indicators and flattened tree #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2929bd8
perf(checkbox): CSS-only indicators and .ControlIndicator flattened c…
joshuapare Mar 12, 2026
f48ef2a
perf(checkbox): remove icon components and add flattened render path
joshuapare Mar 12, 2026
993a662
test(checkbox): add tests for flattened default path and keepIndicato…
joshuapare Mar 12, 2026
50e7b4b
ci: use codspeed-macro runner for walltime execution profiling
joshuapare Mar 12, 2026
2ae7797
fix(benchmarks): inline ESM-only deps for CodSpeed CI runner
joshuapare Mar 12, 2026
11817fa
fix(benchmarks): use server.deps.inline for ESM-only deps in CI
joshuapare Mar 12, 2026
9e280e4
fix(benchmarks): mock react-syntax-highlighter for CodSpeed runner
joshuapare Mar 12, 2026
1756962
fix(benchmarks): stub react-syntax-highlighter via Vite resolve alias
joshuapare Mar 12, 2026
68fc1f1
fix(benchmarks): wrap DataTable bench with Container for valid HTML
joshuapare Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/codspeed.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ concurrency:

jobs:
benchmarks:
runs-on: ubuntu-latest
runs-on: codspeed-macro
timeout-minutes: 30
permissions:
contents: read
Expand Down
103 changes: 91 additions & 12 deletions packages/base-ui/src/components/checkbox/Checkbox.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
grid-template-columns: minmax(0, 1fr) var(--_ov-control-size);
}

.Root[data-ov-label-position='start'] .Control {
.Root[data-ov-label-position='start'] .Control,
.Root[data-ov-label-position='start'] .ControlIndicator {
grid-column: 2;
}

Expand Down Expand Up @@ -168,24 +169,28 @@
box-shadow var(--ov-duration-interactive) var(--ov-ease-standard);
}

.Root:not([data-disabled], [disabled]):hover .Control {
.Root:not([data-disabled], [disabled]):hover .Control,
.Root:not([data-disabled], [disabled]):hover .ControlIndicator {
background: var(--_ov-control-bg-hover);
border-color: var(--_ov-control-border-hover);
}

.Root:is([data-checked], [data-indeterminate]) .Control {
.Root:is([data-checked], [data-indeterminate]) .Control,
.Root:is([data-checked], [data-indeterminate]) .ControlIndicator {
background: var(--_ov-control-bg-checked);
border-color: var(--_ov-control-border-checked);
color: var(--_ov-control-fg-checked);
}

.Root:focus-visible .Control {
.Root:focus-visible .Control,
.Root:focus-visible .ControlIndicator {
border-color: var(--_ov-focus-border);
box-shadow: 0 0 0 1px var(--ov-color-state-focus-ring);
}

.Indicator {
display: inline-flex;
position: relative;
align-items: center;
justify-content: center;
inline-size: 100%;
Expand All @@ -202,22 +207,96 @@
transform: scale(1);
}

.DefaultCheckIcon,
.DefaultMinusIcon {
inline-size: var(--_ov-indicator-size);
block-size: var(--_ov-indicator-size);
/* ── Fallback pseudo-elements for compound-part consumers using .Indicator with no children ── */

.Indicator:empty::before {
content: '';
display: none;
position: absolute;
top: 50%;
left: 50%;
width: 35%;
height: 60%;
border-bottom: 2px solid currentColor;
border-right: 2px solid currentColor;
Comment on lines +220 to +221
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use lowercase currentcolor for Stylelint compliance.

Stylelint reports value-keyword-case errors. While both currentColor and currentcolor are valid CSS, the project's linting configuration prefers lowercase.

🔧 Proposed fix 
 .Indicator:empty::before {
   content: '';
   display: none;
   position: absolute;
   top: 50%;
   left: 50%;
   width: 35%;
   height: 60%;
-  border-bottom: 2px solid currentColor;
-  border-right: 2px solid currentColor;
+  border-bottom: 2px solid currentcolor;
+  border-right: 2px solid currentcolor;
   transform: translate(-50%, -60%) rotate(45deg);
 }
 .Indicator:empty::after {
   content: '';
   display: none;
   position: absolute;
   top: 50%;
   left: 50%;
   width: 55%;
   height: 2px;
-  background: currentColor;
+  background: currentcolor;
   border-radius: 1px;
   transform: translate(-50%, -50%);
 }
 .ControlIndicator::before {
   content: '';
   display: none;
   position: absolute;
   top: 50%;
   left: 50%;
   width: 35%;
   height: 60%;
-  border-bottom: 2px solid currentColor;
-  border-right: 2px solid currentColor;
+  border-bottom: 2px solid currentcolor;
+  border-right: 2px solid currentcolor;
   transform: translate(-50%, -60%) rotate(45deg);
 }
 .ControlIndicator::after {
   content: '';
   display: none;
   position: absolute;
   top: 50%;
   left: 50%;
   width: 55%;
   height: 2px;
-  background: currentColor;
+  background: currentcolor;
   border-radius: 1px;
   transform: translate(-50%, -50%);
 }

Also applies to: 237-237, 276-277, 293-293

🧰 Tools
🪛 Stylelint (17.4.0)

[error] 220-220: Expected "currentColor" to be "currentcolor" (value-keyword-case)

(value-keyword-case)

[error] 221-221: Expected "currentColor" to be "currentcolor" (value-keyword-case)

(value-keyword-case)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/checkbox/Checkbox.module.css` around lines
220 - 221, Replace the capitalized CSS keyword occurrences of "currentColor"
with the lowercase "currentcolor" in the Checkbox styles; specifically update
the declarations using border-bottom and border-right (and any other
declarations in the same module where currentColor appears) inside
Checkbox.module.css so they use "currentcolor" to satisfy Stylelint's
value-keyword-case rule.

transform: translate(-50%, -60%) rotate(45deg);
}

.Root[data-checked]:not([data-indeterminate]) .Indicator:empty::before {
display: block;
}

.DefaultMinusIcon {
.Indicator:empty::after {
content: '';
display: none;
position: absolute;
top: 50%;
left: 50%;
width: 55%;
height: 2px;
background: currentColor;
border-radius: 1px;
transform: translate(-50%, -50%);
}

.Root[data-indeterminate] .DefaultCheckIcon {
.Root[data-indeterminate] .Indicator:empty::after {
display: block;
}

/* ── Combined Control + Indicator (flattened path: no custom indicator, keepMounted=true) ── */

.ControlIndicator {
display: inline-flex;
position: relative;
align-items: center;
justify-content: center;
inline-size: var(--_ov-control-size);
block-size: var(--_ov-control-size);
border-radius: var(--ov-size-choice-radius);
border: 1px solid var(--_ov-control-border);
background: var(--_ov-control-bg);
color: var(--_ov-control-fg);
transition:
background-color var(--ov-duration-interactive) var(--ov-ease-standard),
border-color var(--ov-duration-interactive) var(--ov-ease-standard),
color var(--ov-duration-interactive) var(--ov-ease-standard),
box-shadow var(--ov-duration-interactive) var(--ov-ease-standard);
}

/* ── Pseudo-element indicators on .ControlIndicator ── */

.ControlIndicator::before {
content: '';
display: none;
position: absolute;
top: 50%;
left: 50%;
width: 35%;
height: 60%;
border-bottom: 2px solid currentColor;
border-right: 2px solid currentColor;
transform: translate(-50%, -60%) rotate(45deg);
}

.Root[data-indeterminate] .DefaultMinusIcon {
display: inline;
.Root[data-checked]:not([data-indeterminate]) .ControlIndicator::before {
display: block;
}

.ControlIndicator::after {
content: '';
display: none;
position: absolute;
top: 50%;
left: 50%;
width: 55%;
height: 2px;
background: currentColor;
border-radius: 1px;
transform: translate(-50%, -50%);
}

.Root[data-indeterminate] .ControlIndicator::after {
display: block;
}

.Content {
Expand Down
26 changes: 26 additions & 0 deletions packages/base-ui/src/components/checkbox/Checkbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,32 @@ describe('Checkbox', () => {
expect(screen.getByTestId('indicator')).toBeInTheDocument();
});

it('renders default checkbox without icon child elements', () => {
const { container } = renderWithTheme(<Checkbox.Item defaultChecked />);
const checkbox = screen.getByRole('checkbox');
expect(checkbox).toHaveAttribute('data-checked');

// Flattened path: no LuCheck/LuMinus SVGs
const svgs = container.querySelectorAll('svg');
expect(svgs).toHaveLength(0);
});

it('falls back to nested structure when keepIndicatorMounted is false', () => {
renderWithTheme(
<Checkbox.Item keepIndicatorMounted={false} defaultChecked>
Nested path
</Checkbox.Item>,
);
const checkbox = screen.getByRole('checkbox', { name: 'Nested path' });
expect(checkbox).toHaveAttribute('data-checked');
});

it('renders indeterminate state in flattened path', () => {
renderWithTheme(<Checkbox.Item indeterminate />);
const checkbox = screen.getByRole('checkbox');
expect(checkbox).toHaveAttribute('data-indeterminate');
});
Comment on lines +58 to +62
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider combining with existing indeterminate test or adding differentiating assertions.

This test is very similar to the existing test at lines 21-26 (supports indeterminate state with default indicator). The only difference is the absence of label text. Consider either:

  1. Adding an assertion that differentiates this test (e.g., verifying no SVGs like in the flattened path test)
  2. Combining with the existing test if the behavior is identical
💡 Suggested enhancement to differentiate the test 
 it('renders indeterminate state in flattened path', () => {
-  renderWithTheme(<Checkbox.Item indeterminate />);
+  const { container } = renderWithTheme(<Checkbox.Item indeterminate />);
   const checkbox = screen.getByRole('checkbox');
   expect(checkbox).toHaveAttribute('data-indeterminate');
+  // Flattened path: no LuMinus SVGs, uses CSS pseudo-element
+  expect(container.querySelectorAll('svg')).toHaveLength(0);
 });
📝 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
it('renders indeterminate state in flattened path', () => {
renderWithTheme(<Checkbox.Item indeterminate />);
const checkbox = screen.getByRole('checkbox');
expect(checkbox).toHaveAttribute('data-indeterminate');
});
it('renders indeterminate state in flattened path', () => {
const { container } = renderWithTheme(<Checkbox.Item indeterminate />);
const checkbox = screen.getByRole('checkbox');
expect(checkbox).toHaveAttribute('data-indeterminate');
// Flattened path: no LuMinus SVGs, uses CSS pseudo-element
expect(container.querySelectorAll('svg')).toHaveLength(0);
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/checkbox/Checkbox.test.tsx` around lines 58 -
62, The new test "renders indeterminate state in flattened path" duplicates the
existing "supports indeterminate state with default indicator" for
Checkbox.Item; either merge them or make this test assert a distinguishing
behavior: confirm the element still has data-indeterminate and also assert the
flattened-path variant renders without the SVG indicator (e.g., verify no SVG
indicator is present or that label text is absent), or remove the duplicate if
behaviors are identical—update the test name to reflect the differentiated
assertion if you add one.


it('applies label position and spread layout attributes', () => {
renderWithTheme(
<Checkbox.Item labelPosition="start" layout="spread" defaultChecked>
Expand Down
24 changes: 13 additions & 11 deletions packages/base-ui/src/components/checkbox/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
type HTMLAttributes,
type ReactNode,
} from 'react';
import { LuCheck, LuMinus } from 'react-icons/lu';
import { cn, withBaseClassName } from '../../system/classnames';
import { styleDataAttributes } from '../../system/styleProps';
import type { StyledComponentProps } from '../../system/types';
Expand Down Expand Up @@ -98,6 +97,7 @@ const CheckboxItem = forwardRef<ElementRef<typeof BaseCheckbox.Root>, CheckboxIt
ref,
) {
const hasContent = Boolean(children) || Boolean(description);
const useFlattened = indicator === undefined && keepIndicatorMounted;

return (
<CheckboxRoot
Expand All @@ -110,16 +110,18 @@ const CheckboxItem = forwardRef<ElementRef<typeof BaseCheckbox.Root>, CheckboxIt
data-ov-layout={layout}
{...props}
>
<CheckboxControl>
<CheckboxIndicator keepMounted={keepIndicatorMounted}>
{indicator ?? (
<>
<LuCheck aria-hidden className={styles.DefaultCheckIcon} />
<LuMinus aria-hidden className={styles.DefaultMinusIcon} />
</>
)}
</CheckboxIndicator>
</CheckboxControl>
{useFlattened ? (
<BaseCheckbox.Indicator
keepMounted
className={styles.ControlIndicator}
/>
) : (
<CheckboxControl>
<CheckboxIndicator keepMounted={keepIndicatorMounted}>
{indicator}
</CheckboxIndicator>
</CheckboxControl>
)}
{hasContent ? (
<span className={styles.Content}>
{children ? <CheckboxLabel>{children}</CheckboxLabel> : null}
Expand Down
6 changes: 4 additions & 2 deletions packages/benchmarks/src/base-ui/DataTable.bench.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ function DataTableBench({ data }: { data: Row[] }) {

return (
<DataTable table={table}>
<DataTable.Header />
<DataTable.Body />
<DataTable.Container>
<DataTable.Header />
<DataTable.Body />
</DataTable.Container>
</DataTable>
);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/benchmarks/src/stubs/react-syntax-highlighter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Stub: react-syntax-highlighter is imported transitively via the base-ui
// barrel (CodeBlock component) but no benchmark uses it. The real package
// fails in CodSpeed's forked CJS process because refractor is ESM-only.
export const Prism = () => null;
export default () => null;
16 changes: 12 additions & 4 deletions packages/benchmarks/vitest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,18 @@ export default defineConfig(async () => {
return {
plugins,
resolve: {
alias: {
'@omniview/base-ui': path.resolve(__dirname, '../base-ui/src/index.ts'),
'@omniview/editors': path.resolve(__dirname, '../editors/src/index.ts'),
},
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') },
// Stub out react-syntax-highlighter and all sub-path imports (e.g.
// react-syntax-highlighter/dist/esm/styles/prism). Imported transitively
// via CodeBlock barrel but unused by benchmarks. The real package fails
// in CodSpeed's forked CJS process because refractor is ESM-only.
{
find: /^react-syntax-highlighter(\/.*)?$/,
replacement: path.resolve(__dirname, 'src/stubs/react-syntax-highlighter.ts'),
},
],
},
test: {
benchmark: {
Expand Down
Loading