fix: UI audit Chunk 5 — low severity token and a11y fixes#24
Merged
Conversation
…okens Font-size: 6 files updated to use --ov-font-size-caption/body tokens Z-index: 12 global z-index values tokenized (--ov-z-modal, --ov-z-popup, --ov-z-popover, --ov-z-tooltip, --ov-z-toast) with fallbacks Opacity: 17 disabled-state opacity values replaced with --ov-opacity-disabled
Add tabIndex and onKeyDown (Enter/Space) to sortable header cells. Other findings verified as false positives: backdrop overlays use Escape key dismissal, list items use active-descendant keyboard patterns.
|
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)
📝 WalkthroughWalkthroughReplaced many hard-coded CSS values with design-token CSS variables (z-index, opacity, font-size), reformatted audit-findings documentation and reporting output, and added keyboard accessibility to sortable RowList headers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ai-ui/src/components/content/AICodeBlock.module.css (1)
45-57: 🧹 Nitpick | 🔵 TrivialConsider adding a fallback value for consistency.
The tokenization from
14pxtovar(--ov-font-size-body)is correct per the PR mapping. Otherfont-sizeproperties in this file include fallbacks (lines 26, 34, 77). For consistency:font-size: var(--ov-font-size-body, 0.875rem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-ui/src/components/content/AICodeBlock.module.css` around lines 45 - 57, The .CopyButton rule is missing a fallback for font-size; update the .CopyButton CSS (the selector named .CopyButton) to use the CSS variable with a fallback (e.g., change the font-size declaration to use var(--ov-font-size-body, 0.875rem)) so it matches other rules in the file and provides a consistent fallback value.packages/ai-ui/src/components/agent/AgentStatusItem.module.css (1)
8-14: 🧹 Nitpick | 🔵 TrivialConsider adding a fallback value for consistency.
The tokenization from
12pxtovar(--ov-font-size-caption)is correct. Otherfont-sizeproperties in this file (lines 37, 47) include fallbacks. For defensive CSS consistency:font-size: var(--ov-font-size-caption, 0.75rem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-ui/src/components/agent/AgentStatusItem.module.css` around lines 8 - 14, Update the .Icon CSS rule so its font-size uses a fallback like the other rules; replace the current font-size value with a var that includes a fallback (e.g., var(--ov-font-size-caption, 0.75rem)) to ensure consistent defensive behavior in the .Icon selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/audit-findings.md`:
- Around line 39-42: The markdown generator/template that renders the audit
report is emitting fenced code blocks without blank lines (causing MD031);
update the template that emits the offending snippet (the generator that outputs
entries like the CSS block containing "--_ov-fallback-bg: `#0b8f5a`;" and the
surrounding ```css fences) so it inserts a blank line both before the opening
fence and after the closing fence for every fenced block (e.g., ensure there's a
newline before the line that writes "```css" and a newline after the line that
writes "```"), and add a small unit/regression check to the template output to
prevent future regressions.
- Around line 4-5: The audit report is stale: it still lists
packages/base-ui/src/components/row-list/RowList.tsx as missing a keyboard
handler even though the RowList component's handler was added in this PR; rerun
the audit/regeneration job against the updated source tree (after this PR is
merged), refresh the affected section that mentions RowList and the totals on
lines 4-5, and commit the regenerated report so the document reflects the
current state (search for the RowList component/RowList.tsx references and the
totals block to validate the fixes).
In `@packages/ai-ui/src/components/tool-calls/ToolCall.module.css`:
- Around line 31-35: The .StatusIcon rule is missing a fallback for the CSS
variable; update the font-size declaration in the .StatusIcon selector to use a
fallback with var(--ov-font-size-caption, 0.75rem) so it matches the defensive
pattern used elsewhere (e.g., other font-size rules) and avoids layout
regressions when the variable is undefined.
In `@packages/base-ui/src/components/accordion/Accordion.module.css`:
- Around line 67-69: The rule .Item[data-ov-disabled='true'] uses the CSS
variable --ov-opacity-disabled without a fallback; update its value to use a
fallback literal so styles degrade gracefully, e.g. change opacity:
var(--ov-opacity-disabled) to opacity: var(--ov-opacity-disabled, 0.5) (or
replace 0.5 with the original literal used elsewhere in this file) so the
fallback matches existing token defaults.
In `@packages/base-ui/src/components/button/Button.module.css`:
- Around line 51-54: The disabled-state rule ".Root:is([disabled],
[data-disabled])" uses the undefined CSS variable --ov-opacity-disabled; update
this declaration to include a safe fallback (e.g., change opacity:
var(--ov-opacity-disabled) to opacity: var(--ov-opacity-disabled, 0.5)) in
Button.module.css and apply the same fallback pattern to the same selector in
the other 22 affected component CSS files (Checkbox.module.css,
Input.module.css, TextField.module.css, etc.) so the disabled styling remains
valid when the variable is not set.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.module.css`:
- Around line 50-53: The disabled rule for .Root uses the CSS variable
--ov-opacity-disabled without a fallback; update the .Root:disabled declaration
to provide a fallback value (e.g., var(--ov-opacity-disabled, 0.5) or the
project's default disabled opacity) so the component still renders correctly
when the token is missing; modify the .Root:disabled opacity line to use the
fallback and keep the existing cursor: not-allowed rule unchanged.
In `@packages/base-ui/src/components/radio/Radio.module.css`:
- Around line 122-124: The disabled-state CSS uses the custom property
--ov-opacity-disabled without a fallback, so update the selector
.Root:is([data-disabled], [disabled]) in Radio.module.css to supply a fallback
(use 0.5 like ChatInput.module.css) and mirror this change across all other
base-ui component CSS files that set opacity from --ov-opacity-disabled; locate
every rule that references --ov-opacity-disabled (e.g., opacity:
var(--ov-opacity-disabled)) and change it to opacity: var(--ov-opacity-disabled,
0.5) to ensure consistent disabled opacity.
In `@packages/base-ui/src/components/row-list/RowList.tsx`:
- Around line 118-125: The header cell currently sets interactive props based
only on col.sortable which can create dead tab stops because
handleHeaderClick/handleHeaderKeyDown additionally require onSortChange; change
the gating so tabIndex, onClick and onKeyDown are applied only when col.sortable
&& onSortChange are both present (update the JSX where tabIndex, onClick and
onKeyDown are set and references to col.sortable), and better yet move the
click/key handlers (handleHeaderClick, handleHeaderKeyDown) onto a real <button>
rendered inside the columnheader element (or replace the header content with a
button) so assistive tech gets proper button semantics and avoid focusable
non-actionable headers.
In `@packages/base-ui/src/components/split-button/SplitButton.module.css`:
- Around line 13-16: The disabled opacity rule using var(--ov-opacity-disabled)
should include a defensive fallback so disabled elements don't render fully
opaque if the token is undefined; update the selector
.Root[data-ov-disabled='true'] (and the corresponding disabled selector around
lines 68-72) to use the token with a fallback value (e.g., 0.45) so opacity
becomes var(--ov-opacity-disabled, 0.45).
---
Outside diff comments:
In `@packages/ai-ui/src/components/agent/AgentStatusItem.module.css`:
- Around line 8-14: Update the .Icon CSS rule so its font-size uses a fallback
like the other rules; replace the current font-size value with a var that
includes a fallback (e.g., var(--ov-font-size-caption, 0.75rem)) to ensure
consistent defensive behavior in the .Icon selector.
In `@packages/ai-ui/src/components/content/AICodeBlock.module.css`:
- Around line 45-57: The .CopyButton rule is missing a fallback for font-size;
update the .CopyButton CSS (the selector named .CopyButton) to use the CSS
variable with a fallback (e.g., change the font-size declaration to use
var(--ov-font-size-body, 0.875rem)) so it matches other rules in the file and
provides a consistent fallback value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa681076-5325-49c6-8b9d-193898787124
📒 Files selected for processing (34)
docs/superpowers/specs/audit-findings.mdpackages/ai-ui/src/components/agent/AgentStatusItem.module.csspackages/ai-ui/src/components/artifact/AIArtifact.module.csspackages/ai-ui/src/components/branching/AIBranch.module.csspackages/ai-ui/src/components/content/AICodeBlock.module.csspackages/ai-ui/src/components/tool-calls/ToolCall.module.csspackages/base-ui/src/components/accordion/Accordion.module.csspackages/base-ui/src/components/alert-dialog/AlertDialog.module.csspackages/base-ui/src/components/autocomplete/Autocomplete.module.csspackages/base-ui/src/components/button/Button.module.csspackages/base-ui/src/components/checkbox/Checkbox.module.csspackages/base-ui/src/components/chip/Chip.module.csspackages/base-ui/src/components/collapsible/Collapsible.module.csspackages/base-ui/src/components/combobox/Combobox.module.csspackages/base-ui/src/components/confirm-button/ConfirmButton.module.csspackages/base-ui/src/components/context-menu/ContextMenu.module.csspackages/base-ui/src/components/dialog/Dialog.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/input/Input.module.csspackages/base-ui/src/components/menu/Menu.module.csspackages/base-ui/src/components/multi-select/MultiSelect.module.csspackages/base-ui/src/components/number-input/NumberInput.module.csspackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/popover/Popover.module.csspackages/base-ui/src/components/radio/Radio.module.csspackages/base-ui/src/components/row-list/RowList.tsxpackages/base-ui/src/components/select/Select.module.csspackages/base-ui/src/components/split-button/SplitButton.module.csspackages/base-ui/src/components/stepper/Stepper.module.csspackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/text-area/TextArea.module.csspackages/base-ui/src/components/text-field/TextField.module.csspackages/base-ui/src/components/toast/Toast.module.csspackages/base-ui/src/components/tooltip/Tooltip.module.css
Comment on lines
51
to
54
| .Root:is([disabled], [data-disabled]) { | ||
| opacity: 0.45; | ||
| opacity: var(--ov-opacity-disabled); | ||
| cursor: not-allowed; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for --ov-opacity-disabled definition in the codebase
rg -n '--ov-opacity-disabled\s*:' --type cssRepository: omniviewdev/ui
Length of output: 485
🏁 Script executed:
#!/bin/bash
# Search for --ov-opacity-disabled definition (correct syntax)
echo "=== Searching for --ov-opacity-disabled definition ==="
rg '\-\-ov-opacity-disabled\s*:' --type css
echo ""
echo "=== Searching for --ov-opacity-disabled usage ==="
rg '\-\-ov-opacity-disabled' --type css
echo ""
echo "=== Searching for CSS variable patterns with fallbacks ==="
rg 'var\([^,)]+,' --type css | head -20
echo ""
echo "=== Checking Button.module.css for context ==="
cat packages/base-ui/src/components/button/Button.module.css 2>/dev/null || echo "File not found, searching for it..."Repository: omniviewdev/ui
Length of output: 10569
Add fallback value for --ov-opacity-disabled in 23 components.
The CSS variable --ov-opacity-disabled is not defined anywhere in the codebase. Without a fallback, the opacity property will be invalid, causing the disabled state styling to fail across all affected components. This inconsistency is evident since ChatInput.module.css already includes a fallback (var(--ov-opacity-disabled, 0.5)), while 22 other components lack one.
Affected files:
- Button.module.css
- Checkbox.module.css
- Input.module.css
- TextField.module.css
- And 19 others
♻️ Suggested fix
.Root:is([disabled], [data-disabled]) {
- opacity: var(--ov-opacity-disabled);
+ opacity: var(--ov-opacity-disabled, 0.5);
cursor: not-allowed;
}📝 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
| .Root:is([disabled], [data-disabled]) { | |
| opacity: 0.45; | |
| opacity: var(--ov-opacity-disabled); | |
| cursor: not-allowed; | |
| } | |
| .Root:is([disabled], [data-disabled]) { | |
| opacity: var(--ov-opacity-disabled, 0.5); | |
| cursor: not-allowed; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/button/Button.module.css` around lines 51 -
54, The disabled-state rule ".Root:is([disabled], [data-disabled])" uses the
undefined CSS variable --ov-opacity-disabled; update this declaration to include
a safe fallback (e.g., change opacity: var(--ov-opacity-disabled) to opacity:
var(--ov-opacity-disabled, 0.5)) in Button.module.css and apply the same
fallback pattern to the same selector in the other 22 affected component CSS
files (Checkbox.module.css, Input.module.css, TextField.module.css, etc.) so the
disabled styling remains valid when the variable is not set.
Comment on lines
122
to
124
| .Root:is([data-disabled], [disabled]) { | ||
| opacity: 0.45; | ||
| opacity: var(--ov-opacity-disabled); | ||
| cursor: not-allowed; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Definitions of --ov-opacity-disabled:"
rg -nP -- '--ov-opacity-disabled\s*:' || true
echo
echo "Usages of --ov-opacity-disabled:"
rg -nP -- 'var\(--ov-opacity-disabled\b'Repository: omniviewdev/ui
Length of output: 2546
Add fallback values to --ov-opacity-disabled across all disabled-state declarations.
The --ov-opacity-disabled token is not defined anywhere in the codebase. Without a fallback, disabled controls across 22 component modules will render fully opaque, creating accessibility and UX issues. The ChatInput.module.css already sets a fallback of 0.5—establish this as the standard pattern across all components.
Affected files:
packages/base-ui/src/components/{button,checkbox,radio,input,...}/[Component].module.cssand 19 others
For the Radio component specifically:
.Root:is([data-disabled], [disabled]) {
- opacity: var(--ov-opacity-disabled);
+ opacity: var(--ov-opacity-disabled, 0.5);
cursor: not-allowed;
}Apply the same fix to all other disabled-state declarations in the base-ui suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/radio/Radio.module.css` around lines 122 -
124, The disabled-state CSS uses the custom property --ov-opacity-disabled
without a fallback, so update the selector .Root:is([data-disabled], [disabled])
in Radio.module.css to supply a fallback (use 0.5 like ChatInput.module.css) and
mirror this change across all other base-ui component CSS files that set opacity
from --ov-opacity-disabled; locate every rule that references
--ov-opacity-disabled (e.g., opacity: var(--ov-opacity-disabled)) and change it
to opacity: var(--ov-opacity-disabled, 0.5) to ensure consistent disabled
opacity.
- Add blank lines around fenced code blocks in report generator (MD031) - Add 0.75rem/0.875rem fallbacks to font-size token usages - Add 0.45 fallback to all var(--ov-opacity-disabled) usages (21 files) - Gate RowList sort interactivity on onSortChange presence - Regenerate audit findings report (806 findings, down from 845)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/base-ui/src/components/row-list/RowList.tsx (1)
116-125: 🧹 Nitpick | 🔵 Trivial
data-sortableattribute is still set without checkingonSortChange.The interactive props (
tabIndex,onClick,onKeyDown) are now correctly gated oncol.sortable && onSortChange, addressing the dead tab-stop concern. However,data-sortableon line 116 still only checkscol.sortable, which could apply visual "sortable" styling to headers that aren't actually interactive.🔧 Suggested consistency fix
+ const isInteractive = col.sortable && !!onSortChange; + return ( <div key={col.id} role="columnheader" className={styles.HeaderCell} - data-sortable={col.sortable ? '' : undefined} + data-sortable={isInteractive ? '' : undefined} aria-sort={ariaSortValue} - tabIndex={col.sortable && onSortChange ? 0 : undefined} + tabIndex={isInteractive ? 0 : undefined} style={ { '--_row-header-cell-justify': col.align ?? undefined, } as CSSProperties } - onClick={col.sortable && onSortChange ? () => handleHeaderClick(col) : undefined} - onKeyDown={col.sortable && onSortChange ? (e) => handleHeaderKeyDown(col, e) : undefined} + onClick={isInteractive ? () => handleHeaderClick(col) : undefined} + onKeyDown={isInteractive ? (e) => handleHeaderKeyDown(col, e) : undefined} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/row-list/RowList.tsx` around lines 116 - 125, The data-sortable attribute is being set based only on col.sortable which can cause non-interactive headers to receive sortable styling; update the JSX that sets data-sortable so it mirrors the interactivity gating used for tabIndex/onClick/onKeyDown (i.e., use col.sortable && onSortChange) so styling only applies when the header is actually interactive—locate the data-sortable assignment in RowList.tsx alongside handleHeaderClick and handleHeaderKeyDown and change its condition to include onSortChange.
🤖 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/base-ui/src/components/row-list/RowList.tsx`:
- Around line 116-125: The data-sortable attribute is being set based only on
col.sortable which can cause non-interactive headers to receive sortable
styling; update the JSX that sets data-sortable so it mirrors the interactivity
gating used for tabIndex/onClick/onKeyDown (i.e., use col.sortable &&
onSortChange) so styling only applies when the header is actually
interactive—locate the data-sortable assignment in RowList.tsx alongside
handleHeaderClick and handleHeaderKeyDown and change its condition to include
onSortChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ba1a755-f5ce-4f20-a760-a405ee27ae87
📒 Files selected for processing (27)
docs/superpowers/specs/audit-findings.mdpackages/ai-ui/src/components/agent/AgentStatusItem.module.csspackages/ai-ui/src/components/content/AICodeBlock.module.csspackages/ai-ui/src/components/tool-calls/ToolCall.module.csspackages/base-ui/src/components/accordion/Accordion.module.csspackages/base-ui/src/components/button/Button.module.csspackages/base-ui/src/components/checkbox/Checkbox.module.csspackages/base-ui/src/components/chip/Chip.module.csspackages/base-ui/src/components/collapsible/Collapsible.module.csspackages/base-ui/src/components/command-list/CommandList.module.csspackages/base-ui/src/components/confirm-button/ConfirmButton.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/input/Input.module.csspackages/base-ui/src/components/list/List.module.csspackages/base-ui/src/components/number-input/NumberInput.module.csspackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/radio/Radio.module.csspackages/base-ui/src/components/row-list/RowList.tsxpackages/base-ui/src/components/select/Select.module.csspackages/base-ui/src/components/selectable-list/SelectableList.module.csspackages/base-ui/src/components/split-button/SplitButton.module.csspackages/base-ui/src/components/stepper/Stepper.module.csspackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/text-area/TextArea.module.csspackages/base-ui/src/components/text-field/TextField.module.csspackages/base-ui/src/components/tree-list/TreeList.module.cssscripts/audit/report.mjs
Aligns data-sortable attribute with tabIndex/onClick/onKeyDown gating so headers don't receive sortable styling when non-interactive.
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
Token fixes
12px→--ov-font-size-caption,14px/0.875rem→--ov-font-size-body--ov-z-modal,--ov-z-popup,--ov-z-popover,--ov-z-tooltip,--ov-z-toast). Local stacking contexts (0/1/2/3) intentionally left as literals.0.45values →--ov-opacity-disabled. Binary 0/1 and design-specific values intentionally left as literals.Accessibility
tabIndex={0}andonKeyDownhandlerTest plan
pnpm --filter @omniview/base-ui typecheckpassesSummary by CodeRabbit
New Features
Style
Documentation