perf: checkbox optimization — CSS indicators and flattened tree#28
Merged
Conversation
…lass Replace .DefaultCheckIcon/.DefaultMinusIcon icon rules with CSS pseudo-elements (::before for checkmark, ::after for minus). Add .ControlIndicator combined class merging Control + Indicator styles for the flattened render path. Add .Indicator:empty fallback for compound-part consumers.
Remove LuCheck/LuMinus imports. CheckboxItem now uses a flattened tree (single BaseCheckbox.Indicator with .ControlIndicator class) when no custom indicator prop is passed and keepIndicatorMounted is true. Falls back to nested Control > Indicator structure for custom indicators or keepIndicatorMounted=false.
…rMounted fallback
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCI benchmark runner switched to codspeed-macro. Checkbox component adds ControlIndicator styles and a flattened rendering path; tests for checkbox behaviors added. Benchmarks config stubs react-syntax-highlighter and updates Vitest aliasing; a DataTable benchmark JSX wrapper adjusted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
react-syntax-highlighter imports refractor (ESM-only) via require() in the forked CJS process, causing CI failures. Configure Vitest deps optimizer to inline both packages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/base-ui/src/components/checkbox/Checkbox.module.css`:
- Around line 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.
In `@packages/base-ui/src/components/checkbox/Checkbox.test.tsx`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81a42fef-04bb-4c29-b293-42e9aca4ccf5
📒 Files selected for processing (5)
.github/workflows/codspeed.ymlpackages/base-ui/src/components/checkbox/Checkbox.module.csspackages/base-ui/src/components/checkbox/Checkbox.test.tsxpackages/base-ui/src/components/checkbox/Checkbox.tsxpackages/benchmarks/vitest.config.ts
Comment on lines
+220
to
+221
| border-bottom: 2px solid currentColor; | ||
| border-right: 2px solid currentColor; |
There was a problem hiding this comment.
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.
Comment on lines
+58
to
+62
| it('renders indeterminate state in flattened path', () => { | ||
| renderWithTheme(<Checkbox.Item indeterminate />); | ||
| const checkbox = screen.getByRole('checkbox'); | ||
| expect(checkbox).toHaveAttribute('data-indeterminate'); | ||
| }); |
There was a problem hiding this comment.
🧹 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:
- Adding an assertion that differentiates this test (e.g., verifying no SVGs like in the flattened path test)
- 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.
The deps.optimizer.web approach was insufficient on the CodSpeed ARM64 runner. Switch to server.deps.inline which forces Vite to transform react-syntax-highlighter and refractor before the forked process attempts to require() them.
CodSpeed's forked process uses CJS require() which fails on refractor (ESM-only), imported transitively via CodeBlock barrel. No benchmark uses CodeBlock, so stub the module in setup. Revert ineffective server.deps.inline approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/setup.ts`:
- Around line 4-12: Currently the global vi.mock calls for
'react-syntax-highlighter/dist/esm/prism-light' and
'react-syntax-highlighter/dist/cjs/prism-light' live in shared setup
(vi.mock(...)), which can silently hide future real usage; move these mocks out
of the shared setup and into a CodSpeed-specific test config or into only the
affected suites that need them (e.g., the CodSpeed runner test file or a
suite-level setup file). Locate the two vi.mock calls and either (a) wrap them
behind a CodSpeed-only conditional (check a dedicated env var like CODSPEED or a
test-runner flag) so they execute only for CodSpeed runs, or (b) remove them
from the global setup and add identical vi.mock calls to the specific test
suites/config that actually require stubbing the 'react-syntax-highlighter'
modules. Ensure the module names
('react-syntax-highlighter/dist/esm/prism-light' and
'react-syntax-highlighter/dist/cjs/prism-light') and the mock factory (default:
() => null) are preserved when relocating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3684c85f-393f-4ab0-9ee3-29b40d3ad31a
📒 Files selected for processing (1)
packages/benchmarks/src/setup.ts
Comment on lines
+4
to
+12
| // react-syntax-highlighter imports refractor (ESM-only) via require(). | ||
| // In CodSpeed's forked runner process the CJS require() of ESM fails. | ||
| // No benchmark uses CodeBlock, so stub the module out entirely. | ||
| vi.mock('react-syntax-highlighter/dist/esm/prism-light', () => ({ | ||
| default: () => null, | ||
| })); | ||
| vi.mock('react-syntax-highlighter/dist/cjs/prism-light', () => ({ | ||
| default: () => null, | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any benchmark currently touches components that may depend on react-syntax-highlighter.
# Expected result if this global mock is safe today: no benchmark matches below.
rg -n -C2 --glob 'packages/benchmarks/**/*.{ts,tsx}' \
'CodeBlock|MarkdownPreview|AIMarkdown|react-syntax-highlighter'
# Also locate app/component files that directly import the syntax highlighter,
# so you can compare them against benchmark imports.
rg -n -C2 --glob 'packages/**/*.{ts,tsx}' \
'react-syntax-highlighter'Repository: omniviewdev/ui
Length of output: 2453
Scope this mock to CodSpeed-only or to the affected suites (defensive best practice).
No current benchmark uses CodeBlock or react-syntax-highlighter, so the global mock in shared setup is safe today. However, if a future benchmark adds syntax-highlighting coverage, the mock would silently suppress it rather than fail loudly. To guard against this, move the mock into a CodSpeed-specific config or only mock it in suites that actually need it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/benchmarks/src/setup.ts` around lines 4 - 12, Currently the global
vi.mock calls for 'react-syntax-highlighter/dist/esm/prism-light' and
'react-syntax-highlighter/dist/cjs/prism-light' live in shared setup
(vi.mock(...)), which can silently hide future real usage; move these mocks out
of the shared setup and into a CodSpeed-specific test config or into only the
affected suites that need them (e.g., the CodSpeed runner test file or a
suite-level setup file). Locate the two vi.mock calls and either (a) wrap them
behind a CodSpeed-only conditional (check a dedicated env var like CODSPEED or a
test-runner flag) so they execute only for CodSpeed runs, or (b) remove them
from the global setup and add identical vi.mock calls to the specific test
suites/config that actually require stubbing the 'react-syntax-highlighter'
modules. Ensure the module names
('react-syntax-highlighter/dist/esm/prism-light' and
'react-syntax-highlighter/dist/cjs/prism-light') and the mock factory (default:
() => null) are preserved when relocating.
Previous approaches (deps.optimizer, server.deps.inline, vi.mock) failed because CodSpeed's forked process loads modules via Node's require() before Vitest transforms apply. A Vite resolve alias redirects all react-syntax-highlighter imports (including sub-paths like dist/esm/styles/prism) to a stub before module resolution.
DataTable.Header and DataTable.Body were rendered directly inside the Root div, causing <thead>/<tbody> to appear outside a <table>. Adding DataTable.Container provides the required <table> wrapper element.
Merging this PR will degrade performance by 86.83%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | mount |
1.8 ms | 5.4 ms | -65.99% |
| ❌ | WallTime | mount 100 rows |
41.7 ms | 218.6 ms | -80.94% |
| ❌ | WallTime | mount section |
393.4 µs | 1,882.7 µs | -79.11% |
| ❌ | WallTime | checked toggle |
1.2 ms | 4.8 ms | -74.29% |
| ❌ | WallTime | mount 1000 |
810.3 ms | 1,964.1 ms | -58.75% |
| ❌ | WallTime | variant change |
213.5 µs | 1,621.9 µs | -86.83% |
| ❌ | WallTime | mount div |
434.9 µs | 2,018.2 µs | -78.45% |
| ❌ | WallTime | mount 1000 |
35.3 ms | 192.8 ms | -81.68% |
| ❌ | WallTime | mount 1000 |
98.1 ms | 442.9 ms | -77.84% |
| ❌ | WallTime | mount 1000 rows |
436.4 ms | 2,060.9 ms | -78.83% |
| ❌ | WallTime | mount |
560.7 µs | 2,718.6 µs | -79.38% |
| ❌ | WallTime | mount with decorators |
614.5 µs | 3,265.6 µs | -81.18% |
Comparing feat/checkbox-optimization (68fc1f1) with main (ea3c565)
This was referenced Mar 12, 2026
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
LuCheck/LuMinusReact icon components with CSS pseudo-elements (::beforefor checkmark,::afterfor minus) — eliminates 2 React component instances per checkboxCheckboxItemcomponent tree when no customindicatorprop is passed — mergesCheckboxControlspan andCheckboxIndicatorwrapper into a singleBaseCheckbox.Indicatorelement with combined.ControlIndicatorstylescodspeed-macrorunners, enabling execution profiling (flame graphs) that were previously unavailableBenchmark Results
Design Details
.ControlIndicator(flattened path) and.Indicator:empty(compound-part fallback) — customindicatorprop content is never overlappedkeepIndicatorMountedistrue(the default) — falls back to nested structure otherwiseMarkdownPreview,AIMarkdown) get CSS indicators via.Indicator:emptyfallbackTest plan
Summary by CodeRabbit
Style
Tests
Chores
UI