chore: UI library audit — scanner, triage, and high-severity fixes#22
Merged
Conversation
Defines the automated scan + manual review + fixes + guardrails approach for auditing all 3 UI packages for token consistency, convention compliance, React performance, and accessibility.
20-task plan covering automated scanning, manual review, fixes by severity, ESLint guardrails, and final verification across all 3 packages.
Scanners for token consistency, theme coverage, convention violations, React performance, and accessibility across all 3 packages. Initial scan: 1135 findings (164 High, 789 Medium, 182 Low).
- Identified false positives (avatar colors, legitimate inline styles) - Documented systemic patterns (397 theme gaps, 130 hardcoded transitions) - Manual performance review of 12 complex components - Critical: CommandList meta object allocation in render loop - Important: DataTable non-virtualized row rendering lacks memoization
- Switch thumb: #ffffff → var(--ov-color-fg-inverse) - EditorTabs: remove hardcoded rgb fallback from var(--ov-color-state-hover) - Annotate elevation shadow rgb() values that have no semantic token equivalent
99 violations across 26 CSS Module files. Primitive --ov-primitive-* references replaced with semantic equivalents (--ov-color-*, --ov-space-*, --ov-radius-*, --ov-shadow-*, --ov-font-size-*). Primitives with no exact semantic match use literal values with explanatory comments.
Convert style={{}} to CSS variable pass-through pattern where possible.
Dynamic values use --_ prefixed CSS custom properties. Virtualizer
positioning styles left as-is (required by @tanstack/react-virtual).
Static styles moved to CSS Module classes.
|
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 a Node.js UI Library Audit (design + plan), a scripts/audit toolchain (orchestrator, report, utils, five scanners), an npm audit script, and numerous CSS/TSX token, CSS-variable, accessibility, and React-performance edits across UI packages. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "npm script (audit:ui)"
participant Orch as "scripts/audit/index.mjs"
participant Scanner as "scripts/audit/scanners/*"
participant FS as "Filesystem (packages/*)"
participant Reporter as "scripts/audit/report.mjs"
CLI->>Orch: run audit
Orch->>Scanner: invoke scans (token, theme, conventions, react-perf, accessibility)
Scanner->>FS: findFiles / readLines
FS-->>Scanner: file contents
Scanner-->>Orch: findings[]
Orch->>Reporter: generateReport(allFindings)
Reporter->>FS: write docs/superpowers/specs/audit-findings.md
Reporter-->>Orch: outPath + counts
Orch-->>CLI: exit (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/editors/src/components/diff-viewer/DiffViewer.module.css (1)
1-8:⚠️ Potential issue | 🟡 MinorApply
border-boxbefore sizing the bordered root.Line 7 now sizes the outer
.Rootelement directly. Because.Rootstill uses the default content-box model, the 1px border is added on top of--_diff-height, so the default100%height can overflow its parent by 2px.Suggested fix
.Root { + box-sizing: border-box; position: relative; overflow: hidden; border-radius: var(--ov-radius-sm, 4px); border: 1px solid var(--ov-color-editor-border, var(--ov-color-border-default)); background: var(--ov-color-editor-bg, var(--ov-color-bg-surface)); height: var(--_diff-height); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/diff-viewer/DiffViewer.module.css` around lines 1 - 8, The .Root selector is using the content-box model so its 1px border is added outside --_diff-height causing overflow; update the .Root rules to use box-sizing: border-box (add the box-sizing declaration in the .Root block before/alongside the height declaration) so the border is included in the element's size and the height no longer overflows its parent.packages/editors/src/components/code-editor/CodeEditor.module.css (1)
1-9:⚠️ Potential issue | 🟠 Major
width: 100%now overflows by the border width.Lines 7-8 moved sizing to the bordered
.Root, but the element is still content-box. With the defaultwidth="100%", the editor becomes 2px wider than its container, which is enough to trigger horizontal overflow in tight layouts.Suggested fix
.Root { + box-sizing: border-box; position: relative; overflow: hidden; border-radius: var(--ov-radius-sm, 4px); border: 1px solid var(--ov-color-editor-border, var(--ov-color-border-default)); background: var(--ov-color-editor-bg, var(--ov-color-bg-surface)); height: var(--_editor-height); width: var(--_editor-width); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/code-editor/CodeEditor.module.css` around lines 1 - 9, The .Root element is using content-box sizing so width: var(--_editor-width) (often 100%) overflows the container by the border width; update the .Root CSS (selector .Root) to use box-sizing: border-box so the border is included in the specified width (or alternatively adjust --_editor-width to subtract the border), ensuring the editor no longer exceeds its container.packages/base-ui/src/components/chip/Chip.module.css (1)
4-29:⚠️ Potential issue | 🟡 MinorInitialize
--_ov-group-spacingon.Groupto prevent inheritance leaks.CSS custom properties inherit by default. Without a local initialization, nested
.Groupinstances will reuse an ancestor's--_ov-group-spacingvalue instead of defaulting to 4px, since the fallback invar(--_ov-group-spacing, 4px)only applies when the property is undefined—not when it's inherited. Initialize the variable on.Groupitself to ensure proper encapsulation.Suggested fix
.Group { + --_ov-group-spacing: 4px; display: inline-flex; align-items: center; - gap: var(--_ov-group-spacing, 4px); /* fallback: --ov-space-stack-xs; no semantic token for 4px */ + gap: var(--_ov-group-spacing); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/chip/Chip.module.css` around lines 4 - 29, The .Group rule needs a local initialization of the CSS custom property to avoid inherited values from ancestor groups; add an explicit declaration like setting --_ov-group-spacing: 4px on the .Group selector so nested .Group instances start with the intended default (and keep the existing data-ov-spacing overrides such as .Group[data-ov-spacing='1'] etc. to continue to change it).packages/base-ui/src/components/data-table/DataTableHeader.tsx (1)
30-50:⚠️ Potential issue | 🟠 MajorUse a real interactive control for sorting and expose
aria-sort.Line 40 makes the
<th>clickable, but<th>is not keyboard-operable and assistive tech never receives the current sort state. Keep the cell semantic as a header, setaria-sorton it, and move the toggle onto a<button>inside the cell.Suggested shape
<th key={header.id} className={styles.HeaderCell} + aria-sort={ + sorted === 'asc' + ? 'ascending' + : sorted === 'desc' + ? 'descending' + : canSort + ? 'none' + : undefined + } style={{ // eslint-disable-line react/forbid-component-props -- TanStack Table column size/pinning CSS variable injection ...sizeStyles, ...pinningStyles, }} data-ov-sortable={canSort ? 'true' : 'false'} data-ov-pinned={header.column.getIsPinned() || undefined} data-ov-align={align} - onClick={canSort ? header.column.getToggleSortingHandler() : undefined} > - {header.isPlaceholder - ? null - : flexRender(header.column.columnDef.header, header.getContext())} + {header.isPlaceholder ? null : canSort ? ( + <button + type="button" + className={styles.SortButton} + onClick={header.column.getToggleSortingHandler()} + > + {flexRender(header.column.columnDef.header, header.getContext())} + </button> + ) : ( + flexRender(header.column.columnDef.header, header.getContext()) + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx` around lines 30 - 50, The header cell is clickable but not keyboard-accessible and lacks aria-sort; change the implementation to keep the <th> as a semantic header, set aria-sort on it based on the existing sorted value (map 'asc' -> 'ascending', 'desc' -> 'descending', else undefined), and move the sorting toggle handler to a real interactive element: render a <button> inside the cell that uses header.column.getToggleSortingHandler() (and header.column.getCanSort()/canSort) for onClick/onKey handlers, contains the header content and sort indicator, and includes an accessible label/visually-hidden text for screen readers; ensure the <th> retains data-ov-* attributes (data-ov-sortable, data-ov-pinned, data-ov-align) but no onClick so keyboard/assistive tech get correct semantics.
🤖 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/plans/2026-03-10-ui-audit.md`:
- Around line 1404-1411: Update the final verification block so the `pnpm
typecheck` expectation reflects the branch reality: state that `pnpm typecheck`
currently passes only for `base-ui` and that there are known cross-package type
issues in `editors` and `ai-ui`, rather than claiming full-monorepo success;
edit the lines mentioning `pnpm typecheck` and the list of package checks to
explicitly note the limited scope (base-ui) and the outstanding failures for
`editors` and `ai-ui`.
- Around line 459-468: The scanner currently flags any JSX style={{...}} via
INLINE_STYLE; change the logic so it skips/report-exempts style objects that
only set CSS custom properties (keys beginning with --, e.g. '--_dynamic-value')
to allow the prescribed CSS-variable pass-through. Implement a helper (e.g.
isCssVarOnlyStyle(line) or similar) used in the loop that parses the matched
style object string and returns true when every key in the object is a CSS
custom property (match /^--[A-Za-z0-9\-_]+$/ allowing quoted/unquoted keys and
any expression/value), then only call finding(...) when INLINE_STYLE.test(line)
&& !isCssVarOnlyStyle(line). Ensure the change references INLINE_STYLE,
isCssVarOnlyStyle (new helper), finding, tsxFiles, findFiles, and readLines so
the exempted styles are not reported.
- Around line 32-33: Replace the hardcoded absolute path string "cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm add -Dw glob" with
a repo-relative instruction so the plan is runnable by others; update the line
in docs/superpowers/plans/2026-03-10-ui-audit.md to use a path relative to the
repository root (or a single-step package manager command that runs from the
repo root) and remove any user-specific home directory references so
contributors can execute the steps without editing the path.
In `@packages/base-ui/src/components/aspect-ratio/AspectRatio.module.css`:
- Line 5: The aspect-ratio declaration in AspectRatio.module.css uses
var(--_aspect-ratio) without a fallback; update that property to include a
defensive fallback (e.g., var(--_aspect-ratio, 1)) so the aspect ratio remains
predictable if the CSS variable isn't set by the TSX component or when the class
is used elsewhere.
In `@packages/base-ui/src/components/card/Card.module.css`:
- Around line 97-105: The three CSS rules .Root[data-ov-elevation='1'],
.Root[data-ov-elevation='2'], and .Root[data-ov-elevation='3'] currently all use
the same token (--ov-shadow-surface) so elevation no longer indicates depth;
update these selectors to apply distinct shadow tokens (or temporary
component-local aliases) for each elevation (e.g., use a small/medium/large
variant or create --card-shadow-sm / --card-shadow-md / --card-shadow-lg
aliases) so .Root[data-ov-elevation='1'], .Root[data-ov-elevation='2'], and
.Root[data-ov-elevation='3'] render different box-shadow values while you
migrate to the semantic shadow scale.
In `@packages/base-ui/src/components/checkbox/Checkbox.module.css`:
- Around line 246-247: The lg variant lost its larger label size; update the
.Root[data-ov-size='lg'] .Label rule to restore a distinct larger font-size
(either set font-size: 1rem; to match the previous 16px behavior or reference a
new semantic token such as --ov-font-size-title or a new token like
--ov-font-size-label-lg) so the lg variant renders larger label text than the
default .Label; change the value in the .Root[data-ov-size='lg'] .Label selector
accordingly.
In `@packages/base-ui/src/components/code-block/CodeBlock.module.css`:
- Around line 32-35: The lg size variant lost its distinct type scale because
.Root[data-ov-size='lg'] sets --_ov-font-size to --ov-font-size-body; restore a
unique 16px step by changing .Root[data-ov-size='lg'] to use a distinct semantic
token or component alias (e.g., map it to --ov-font-size-title or define a new
--ov-font-size-code-lg that equals the previous 16px value) so the lg selector
and its --_ov-font-size variable produce a larger text size than the default
.Root.
In `@packages/base-ui/src/components/data-table/DataTableFooter.tsx`:
- Around line 27-30: The eslint suppression using "react/forbid-component-props"
in DataTableFooter.tsx is a no-op because that rule isn't enabled globally;
either enable/configure react/forbid-component-props in the ESLint config or
remove the suppression comments and handle the underlying issue. Update the
ESLint config (where react.configs.recommended.rules are extended) to add
react/forbid-component-props with the desired options, or delete the inline "//
eslint-disable-line react/forbid-component-props" comments in components that
set style props (e.g., the style block using header.column.id and the flex/width
CSS variable injections) and, if needed, refactor those components
(DataTableFooter, DataTable, CommandList, TreeList, TabDragBroker,
ChatMessageList) to avoid passing raw style props on components so they comply
with the rule.
In `@packages/base-ui/src/components/form-field/FormField.module.css`:
- Line 8: Define a single private CSS custom property for the 4px escape-hatch
and use it wherever the hardcoded 4px/3px values appear: add something like
--_space-stack-xs: 4px at the top of FormField.module.css (scoped to the
module/class used, e.g., the top-level selector in this file), replace the
repeated literal gaps with var(--_space-stack-xs), and derive the `sm` variant
from it (e.g., calc(var(--_space-stack-xs) - 1px) or similar) so the three
occurrences (the current gap: 4px entries) are centralized and consistent.
In `@packages/base-ui/src/components/grid/Grid.module.css`:
- Around line 55-69: Add a short TODO comment above the
.Root[data-ov-spacing='1'] rule (and the two other analogous rules that hardcode
4px) noting that these hardcoded values are temporary until a
--ov-space-stack-xs semantic token is introduced; reference the specific
selectors (.Root[data-ov-spacing='1'] and the other similar
.Root[data-ov-spacing='...'] rules) and state "update to use --ov-space-stack-xs
when available" and optionally add a ticket/reference ID for the audit backlog.
In `@packages/base-ui/src/components/meter/Meter.module.css`:
- Line 7: The CSS rule in Meter.module.css is using a hardcoded gap: 4px which
violates the spacing scale; update the gap declaration to use the project
spacing token instead (replace gap: 4px; with the appropriate semantic token,
e.g. gap: var(--space-xs) or the project's equivalent token) so the Meter
component uses the normalized spacing scale rather than a raw pixel value.
In `@packages/base-ui/src/components/pagination/Pagination.module.css`:
- Line 5: The CSS in Pagination.module.css sets a hardcoded spacing literal on
the custom property --_ov-gap (currently "4px"), which bypasses the design token
system; change the assignment to use the existing semantic token
--ov-space-stack-xs (or create and use a new semantic token if a different size
is required) so --_ov-gap reads from that token instead and stays consistent
with theme/density overrides.
In `@packages/base-ui/src/components/radio/Radio.module.css`:
- Around line 205-207: The lg variant selector .Root[data-ov-size='lg'] .Label
currently sets font-size to --ov-font-size-body, which matches the base .Label
and flattens the size hierarchy; change that declaration to use a larger
semantic token (e.g. --ov-font-size-title) or a dedicated token like
--ov-font-size-body-lg so the lg label is larger than md and restores size
progression. Locate the .Root[data-ov-size='lg'] .Label rule and replace
--ov-font-size-body with the chosen larger semantic token, ensuring consistency
with other token usage in the file.
In `@packages/base-ui/src/components/row-list/RowList.tsx`:
- Line 1: Replace the incorrect use of the React namespace in the style cast on
line 61 by changing the type from React.CSSProperties to the imported
CSSProperties (so the cast matches other uses in this file such as the style
casts in the blocks around lines 109–110 and 155); update the cast in the
component or variable in RowList (the location using React.CSSProperties) to use
CSSProperties instead.
In `@packages/base-ui/src/components/sheet/Sheet.module.css`:
- Around line 51-60: The elevation entries in the Sheet CSS have been
flattened—.Root[data-ov-elevation='1'], .Root[data-ov-elevation='2'], and
.Root[data-ov-elevation='3'] all set the same --_ov-shadow; restore distinct
low/medium/high shadow tokens for each selector (e.g., use the appropriate
semantic shadow tokens for small/medium/large or the original
--ov-shadow-surface-sm / --ov-shadow-surface / --ov-shadow-surface-lg
equivalents) so that the Sheet component's data-ov-elevation='1'/'2'/'3' produce
visually distinct shadows and stacked sheets remain distinguishable.
In `@packages/base-ui/src/components/skeleton/Skeleton.module.css`:
- Around line 4-5: The width/height CSS variables set at the top are being
overridden by later selectors for text variant, so update Skeleton.module.css so
variant- and size-specific rules set the CSS variables rather than hard
`width`/`height`: move or duplicate the `--_sk-width`/`--_sk-height` assignments
into `.Root[data-ov-variant='text']` and change all size-specific text selectors
that currently assign `width`/`height` to instead assign
`--_sk-width`/`--_sk-height`; keep Skeleton.tsx props logic unchanged so
`width`/`height` from the component (and the multiline 80% last-line behavior)
flow through the variables and are not stomped by later rules.
In `@packages/base-ui/src/components/stack/Stack.module.css`:
- Around line 20-21: The 4px literal in .Root[data-ov-spacing='1'] (setting
--_ov-spacing: 4px) should be replaced with a shared semantic spacing token; add
a new token (e.g., --ov-space-stack-xs or --ov-space-xs) in the design tokens
layer and use that token here instead of the hardcoded 4px (update
.Root[data-ov-spacing='1'] to set --_ov-spacing: var(--ov-space-stack-xs) and
ensure the new token is defined once in the tokens file so all layout primitives
can reference it).
In `@packages/base-ui/src/components/status-bar/StatusBar.module.css`:
- Line 31: The CSS file repeats the hardcoded "4px" spacing across multiple
selectors; define a local CSS variable (e.g., --statusbar-space-xs) on the .Root
rule and replace all occurrences of "4px" in .Root, .Item, .Indicator,
.Progress, and .IconItem with that variable (var(--statusbar-space-xs)) so the
spacing is centralized and easier to update/migrate to a semantic token later.
Ensure every instance mentioned in the review (around lines 31, 51-52, 64, 87,
98-99, 122-123, 136) is updated to use the new variable.
In `@packages/base-ui/src/components/switch/Switch.module.css`:
- Line 20: In Switch.module.css update the custom property used for the switch
thumb fill: replace the foreground token --ov-color-fg-inverse with the
background/surface token --ov-color-bg-surface-raised for --_ov-thumb-bg so the
thumb uses a surface fill (use var(--ov-color-bg-surface-raised) for
--_ov-thumb-bg); locate and change the declaration of --_ov-thumb-bg in the
Switch.module.css file where it currently references --ov-color-fg-inverse.
In `@packages/base-ui/src/components/toast/Toast.module.css`:
- Around line 129-138: The current CSS sets the same shadow for all elevations
(.Container[data-ov-elevation='1'] .Toast, .Container[data-ov-elevation='2']
.Toast, .Container[data-ov-elevation='3'] .Toast), so elevation no longer
affects visual depth; update each selector to assign a distinct shadow token
(e.g. set elevation '1' to --_ov-surface-shadow: var(--ov-shadow-surface-sm),
elevation '2' to var(--ov-shadow-surface) or --ov-shadow-surface-md, and
elevation '3' to var(--ov-shadow-surface-lg)) so each Toast elevation produces a
different box-shadow.
In `@packages/base-ui/src/components/tooltip/Tooltip.module.css`:
- Around line 121-125: The .Popup[data-ov-size='sm'] rule incorrectly sets
--_ov-font-size to the same token used by the base popup, removing the smaller
"sm" text size; update the .Popup[data-ov-size='sm'] declaration to restore the
previous 11px override (e.g., set --_ov-font-size: 0.6875rem) until a smaller
semantic token exists so the small size remains visually distinct (modify the
rule using the .Popup[data-ov-size='sm'] selector and the --_ov-font-size
variable).
In `@scripts/audit/scanners/accessibility.mjs`:
- Around line 23-25: The current check uses /onKeyDown|onKeyUp|role=/ on the
context and suppresses findings if any role attribute exists; change it so role=
alone does not suppress the warning. Update the condition around
results.push(finding(...)) to require either explicit keyboard handlers
(onKeyDown or onKeyUp present in context) OR a combination of role="button" (or
role includes "button") plus an explicit tabIndex (or tabindex) present — do not
treat any role= by itself as sufficient; reference the surrounding variables
context and the results.push(finding(...)) call when making this change.
- Line 19: The current per-line regex in
scripts/audit/scanners/accessibility.mjs (the if that tests /onClick/ and
/<(?:div|span|li|td|tr|img|a(?!\s))/) misses multi-line JSX props; change the
scanner to collect/buffer the current tag text until the tag closes (until the
matching '>' or '/>') and then run the regex against that buffered tag string,
or better yet feed the file into a simple JSX/HTML parser and detect elements
with onClick/onPress attributes; update the code paths that perform the per-line
test to use the buffered/tag-level string check so interactive elements split
across lines are caught.
In `@scripts/audit/scanners/react-performance.mjs`:
- Around line 57-63: The current guard that limits inline arrow-prop detection
to cases where the JSX tag and prop share a line (the /<[A-Z]/ and /<[a-z]/
checks) misses multiline TSX props; update the detection around ARROW_FN_PROP
and ARROW_FN_PROP_SHORT so that when an arrow-fn prop is found you also scan a
small window of surrounding lines (e.g., previous 3–5 lines and/or following
lines) to find an unclosed JSX opening tag like /<\s*[A-Za-z]/ that has no
matching closing '>' before the prop, and only then call
results.push(finding(...)) — this preserves the existing finding call site while
catching multiline handler props without requiring a full AST parse.
- Around line 20-33: The current heuristic uses file-level flags (hasExport,
hasMemo, hasForwardRef, isLeaf) which suppresses warnings for all exports if any
of those tokens appear anywhere; change the logic to evaluate memo eligibility
per exported component: locate each export declaration (patterns matching export
const|function followed by the component name), extract the component's source
slice (the declaration and its body), and for each component check
component-scoped patterns (React.memo wrap or memo(<ComponentName>), forwardRef
usage in that slice, and any usage/destructuring of "children" within that
slice) and push a finding via results.push(finding(...)) only for components
that are exported, not memo/forwardRef-wrapped, and are leaf components; keep
existing symbols (hasExport, hasMemo, hasForwardRef, isLeaf, finding,
results.push) but apply them per exported component instead of once per file.
In `@scripts/audit/scanners/theme-coverage.mjs`:
- Around line 76-100: The current token regex and directory check are
misclassifying names: update the token capture (varMatch) to allow hyphens in
the token tail (e.g. use /var\((--ov-color-(?:bg|fg|border)-[\w-]+)\)/) so
tokens like --ov-color-bg-surface-raised are matched, and replace the broad
substring check (componentDir.includes(dir)) with a segment-aware match (e.g.
split componentDir on /[-_]/ and check if the resulting segments include the
IDE_DIR_MAP key) so names like data-table won’t be mistaken for tab; keep using
IDE_DIR_MAP and results.push as-is.
- Around line 15-23: The regex for extracting theme blocks is using double
quotes and thus fails to match CSS selectors that use single quotes; update the
pattern used to build blockRegex (the code that iterates over cssContent and
assigns into blocks[mode]) so it accepts single-quoted theme attributes (e.g.,
change the selector part from [data-ov-theme="..."] to [data-ov-theme='...'] in
the regex), then re-run the existing parsing loop (the while ((match =
blockRegex.exec(cssContent))) that populates blocks and uses varRegex) to
correctly capture theme modes and their CSS variables.
In `@scripts/audit/scanners/token-consistency.mjs`:
- Around line 8-9: The regexes RAW_SPACING and RAW_RADIUS only match shorthand
properties (margin, padding, gap, border-radius) and therefore miss directional
and corner-specific variants (e.g., padding-top, padding-inline-start, row-gap,
column-gap, border-top-left-radius). Update RAW_SPACING and RAW_RADIUS to
include directional and corner variants (or use a pattern that matches property
names that start with or contain margin/padding/gap and border-...-radius), then
update the corresponding duplicated patterns around lines 55-65 to the same
expanded patterns so properties like padding-top: 4px and border-top-left-radius
are detected.
- Line 12: The current RAW_TRANSITION regex in
scripts/audit/scanners/token-consistency.mjs flags multi-line declarations as
violations because the scanner tests one line at a time; fix by making the check
multiline-aware: update RAW_TRANSITION so it matches the entire declaration span
(across newlines) up to the terminating semicolon or block boundary and only
flags when that full span does not contain var(--ov) — e.g. use the dotAll (/s)
modifier or join lines until a semicolon/} and change the negative lookahead to
search that full span; refer to RAW_TRANSITION in token-consistency.mjs and the
scanner logic that applies it when implementing this change.
In `@scripts/audit/utils.mjs`:
- Around line 4-6: Replace the current ROOT definition that uses new
URL(...).pathname with fileURLToPath(new URL(...)) to obtain a proper filesystem
path; add an import for fileURLToPath from 'url' and preserve the existing
.replace(/\/$/, '') trimming of a trailing slash so existing uses of ROOT
(glob(), relative(), and report output) work correctly on Windows and with
percent-encoded paths.
---
Outside diff comments:
In `@packages/base-ui/src/components/chip/Chip.module.css`:
- Around line 4-29: The .Group rule needs a local initialization of the CSS
custom property to avoid inherited values from ancestor groups; add an explicit
declaration like setting --_ov-group-spacing: 4px on the .Group selector so
nested .Group instances start with the intended default (and keep the existing
data-ov-spacing overrides such as .Group[data-ov-spacing='1'] etc. to continue
to change it).
In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx`:
- Around line 30-50: The header cell is clickable but not keyboard-accessible
and lacks aria-sort; change the implementation to keep the <th> as a semantic
header, set aria-sort on it based on the existing sorted value (map 'asc' ->
'ascending', 'desc' -> 'descending', else undefined), and move the sorting
toggle handler to a real interactive element: render a <button> inside the cell
that uses header.column.getToggleSortingHandler() (and
header.column.getCanSort()/canSort) for onClick/onKey handlers, contains the
header content and sort indicator, and includes an accessible
label/visually-hidden text for screen readers; ensure the <th> retains data-ov-*
attributes (data-ov-sortable, data-ov-pinned, data-ov-align) but no onClick so
keyboard/assistive tech get correct semantics.
In `@packages/editors/src/components/code-editor/CodeEditor.module.css`:
- Around line 1-9: The .Root element is using content-box sizing so width:
var(--_editor-width) (often 100%) overflows the container by the border width;
update the .Root CSS (selector .Root) to use box-sizing: border-box so the
border is included in the specified width (or alternatively adjust
--_editor-width to subtract the border), ensuring the editor no longer exceeds
its container.
In `@packages/editors/src/components/diff-viewer/DiffViewer.module.css`:
- Around line 1-8: The .Root selector is using the content-box model so its 1px
border is added outside --_diff-height causing overflow; update the .Root rules
to use box-sizing: border-box (add the box-sizing declaration in the .Root block
before/alongside the height declaration) so the border is included in the
element's size and the height no longer overflows its parent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 798e3c3d-e4cb-4f15-b2a2-a0375fbd33ac
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (71)
docs/superpowers/plans/2026-03-10-ui-audit.mddocs/superpowers/specs/2026-03-10-ui-audit-design.mddocs/superpowers/specs/audit-findings.mdpackage.jsonpackages/ai-ui/src/components/artifact/AIArtifact.module.csspackages/ai-ui/src/components/artifact/AIArtifact.tsxpackages/ai-ui/src/components/chat/ChatMessageList.tsxpackages/ai-ui/src/components/content/AIImageGeneration.module.csspackages/ai-ui/src/components/content/AIImageGeneration.tsxpackages/base-ui/src/components/aspect-ratio/AspectRatio.module.csspackages/base-ui/src/components/aspect-ratio/AspectRatio.tsxpackages/base-ui/src/components/basic-list/BasicList.module.csspackages/base-ui/src/components/card/Card.module.csspackages/base-ui/src/components/checkbox/Checkbox.module.csspackages/base-ui/src/components/chip/Chip.module.csspackages/base-ui/src/components/code-block/CodeBlock.module.csspackages/base-ui/src/components/command-list/CommandList.tsxpackages/base-ui/src/components/data-table/DataTable.module.csspackages/base-ui/src/components/data-table/DataTableBody.tsxpackages/base-ui/src/components/data-table/DataTableContainer.tsxpackages/base-ui/src/components/data-table/DataTableFooter.tsxpackages/base-ui/src/components/data-table/DataTableHeader.tsxpackages/base-ui/src/components/data-table/DataTableLoading.tsxpackages/base-ui/src/components/data-table/DataTableVirtualBody.tsxpackages/base-ui/src/components/drawer/Drawer.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsxpackages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsxpackages/base-ui/src/components/filter-bar/FilterBar.module.csspackages/base-ui/src/components/find-bar/FindBar.module.csspackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/grid/Grid.module.csspackages/base-ui/src/components/image-list/ImageList.module.csspackages/base-ui/src/components/image/Image.module.csspackages/base-ui/src/components/image/Image.tsxpackages/base-ui/src/components/meter/Meter.module.csspackages/base-ui/src/components/meter/Meter.tsxpackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/radio/Radio.module.csspackages/base-ui/src/components/row-list/RowList.module.csspackages/base-ui/src/components/row-list/RowList.tsxpackages/base-ui/src/components/segmented-control/SegmentedControl.module.csspackages/base-ui/src/components/sheet/Sheet.module.csspackages/base-ui/src/components/skeleton/Skeleton.module.csspackages/base-ui/src/components/skeleton/Skeleton.tsxpackages/base-ui/src/components/slider/Slider.module.csspackages/base-ui/src/components/stack/Stack.module.csspackages/base-ui/src/components/status-bar/StatusBar.module.csspackages/base-ui/src/components/switch/Switch.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-area/TextArea.tsxpackages/base-ui/src/components/timeline/Timeline.module.csspackages/base-ui/src/components/toast/Toast.module.csspackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/tooltip/Tooltip.module.csspackages/base-ui/src/components/tree-list/TreeList.tsxpackages/editors/src/components/code-editor/CodeEditor.module.csspackages/editors/src/components/code-editor/CodeEditor.tsxpackages/editors/src/components/diff-viewer/DiffViewer.module.csspackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/src/components/object-inspector/ObjectInspector.module.csspackages/editors/src/components/object-inspector/ObjectInspector.tsxscripts/audit/index.mjsscripts/audit/report.mjsscripts/audit/scanners/accessibility.mjsscripts/audit/scanners/convention-violations.mjsscripts/audit/scanners/react-performance.mjsscripts/audit/scanners/theme-coverage.mjsscripts/audit/scanners/token-consistency.mjsscripts/audit/utils.mjs
💤 Files with no reviewable changes (1)
- packages/base-ui/src/components/drawer/Drawer.tsx
Comment on lines
+32
to
+33
| Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm add -Dw glob` | ||
|
|
There was a problem hiding this comment.
Use repo-relative commands in the plan.
These commands hardcode one developer’s local checkout path, so anyone else following the plan will fail immediately. Keep the instructions runnable from the repository root instead.
Suggested doc fix
-Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm add -Dw glob`
+Run from the repository root: `pnpm add -Dw glob`📝 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
| Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm add -Dw glob` | |
| Run from the repository root: `pnpm add -Dw glob` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 32 - 33, Replace
the hardcoded absolute path string "cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm add -Dw glob" with
a repo-relative instruction so the plan is runnable by others; update the line
in docs/superpowers/plans/2026-03-10-ui-audit.md to use a path relative to the
repository root (or a single-step package manager command that runs from the
repo root) and remove any user-specific home directory references so
contributors can execute the steps without editing the path.
Comment on lines
+459
to
+468
| // --- Inline styles in JSX (High) --- | ||
| const tsxFiles = await findFiles('packages/*/src/**/*.tsx'); | ||
|
|
||
| for (const file of tsxFiles) { | ||
| const lines = readLines(file); | ||
| for (const { line, num } of lines) { | ||
| if (INLINE_STYLE.test(line)) { | ||
| results.push(finding('High', 'Convention', 'Inline style', file, num, line, | ||
| 'style={{}} found — use CSS Modules + data attributes')); | ||
| } |
There was a problem hiding this comment.
Allow CSS-variable pass-through in the inline-style scanner.
Task 10 later prescribes style={{ '--_dynamic-value': value }} for dynamic styling, but this rule flags every style={{...}} as a High finding. That will keep compliant migrations in the report and inflate the remaining High count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 459 - 468, The
scanner currently flags any JSX style={{...}} via INLINE_STYLE; change the logic
so it skips/report-exempts style objects that only set CSS custom properties
(keys beginning with --, e.g. '--_dynamic-value') to allow the prescribed
CSS-variable pass-through. Implement a helper (e.g. isCssVarOnlyStyle(line) or
similar) used in the loop that parses the matched style object string and
returns true when every key in the object is a CSS custom property (match
/^--[A-Za-z0-9\-_]+$/ allowing quoted/unquoted keys and any expression/value),
then only call finding(...) when INLINE_STYLE.test(line) &&
!isCssVarOnlyStyle(line). Ensure the change references INLINE_STYLE,
isCssVarOnlyStyle (new helper), finding, tsxFiles, findFiles, and readLines so
the exempted styles are not reported.
Comment on lines
+1404
to
+1411
| Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit && pnpm validate` | ||
|
|
||
| Expected: | ||
| - `pnpm lint` — passes with no errors | ||
| - `pnpm format:check` — passes | ||
| - `pnpm typecheck` — passes across all packages | ||
| - `pnpm audit:ui` — passes (0 High severity findings) | ||
| - `pnpm test` — all 1225+ tests pass |
There was a problem hiding this comment.
The final typecheck expectation does not match this branch.
The PR objective explicitly says pnpm typecheck currently passes for base-ui only, with pre-existing cross-package issues in editors and ai-ui. Documenting full-monorepo typecheck success here turns the final verification step into a guaranteed false failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 1404 - 1411,
Update the final verification block so the `pnpm typecheck` expectation reflects
the branch reality: state that `pnpm typecheck` currently passes only for
`base-ui` and that there are known cross-package type issues in `editors` and
`ai-ui`, rather than claiming full-monorepo success; edit the lines mentioning
`pnpm typecheck` and the list of package checks to explicitly note the limited
scope (base-ui) and the outstanding failures for `editors` and `ai-ui`.
| position: relative; | ||
| width: 100%; | ||
| overflow: hidden; | ||
| aspect-ratio: var(--_aspect-ratio); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a fallback value for defensive CSS.
Unlike AIImageGeneration.module.css which uses var(--_image-aspect-ratio, 1), this lacks a fallback. While the TSX component always sets the variable, adding a fallback ensures consistent behavior if the CSS class is used elsewhere or the variable isn't set.
💡 Proposed fix
-.Root {
- position: relative;
- width: 100%;
- overflow: hidden;
- aspect-ratio: var(--_aspect-ratio);
-}
+.Root {
+ position: relative;
+ width: 100%;
+ overflow: hidden;
+ aspect-ratio: var(--_aspect-ratio, 16 / 9);
+}📝 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
| aspect-ratio: var(--_aspect-ratio); | |
| .Root { | |
| position: relative; | |
| width: 100%; | |
| overflow: hidden; | |
| aspect-ratio: var(--_aspect-ratio, 16 / 9); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/aspect-ratio/AspectRatio.module.css` at line
5, The aspect-ratio declaration in AspectRatio.module.css uses
var(--_aspect-ratio) without a fallback; update that property to include a
defensive fallback (e.g., var(--_aspect-ratio, 1)) so the aspect ratio remains
predictable if the CSS variable isn't set by the TSX component or when the class
is used elsewhere.
Comment on lines
97
to
105
| .Root[data-ov-elevation='1'] { | ||
| box-shadow: var(--ov-primitive-shadow-sm); | ||
| box-shadow: var(--ov-shadow-surface); /* sm elevation; --ov-shadow-surface (md) is closest semantic token */ | ||
| } | ||
| .Root[data-ov-elevation='2'] { | ||
| box-shadow: var(--ov-primitive-shadow-md); | ||
| box-shadow: var(--ov-shadow-surface); | ||
| } | ||
| .Root[data-ov-elevation='3'] { | ||
| box-shadow: var(--ov-primitive-shadow-lg); | ||
| box-shadow: var(--ov-shadow-surface); /* lg elevation; --ov-shadow-surface (md) is closest semantic token */ | ||
| } |
There was a problem hiding this comment.
Don't collapse all elevation levels to the same shadow.
Lines 98-104 make elevations 1, 2, and 3 render identically, so data-ov-elevation no longer conveys depth. Please keep distinct shadows here—even temporarily via component-local aliases or by deferring this one migration—until the semantic shadow scale exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/card/Card.module.css` around lines 97 - 105,
The three CSS rules .Root[data-ov-elevation='1'], .Root[data-ov-elevation='2'],
and .Root[data-ov-elevation='3'] currently all use the same token
(--ov-shadow-surface) so elevation no longer indicates depth; update these
selectors to apply distinct shadow tokens (or temporary component-local aliases)
for each elevation (e.g., use a small/medium/large variant or create
--card-shadow-sm / --card-shadow-md / --card-shadow-lg aliases) so
.Root[data-ov-elevation='1'], .Root[data-ov-elevation='2'], and
.Root[data-ov-elevation='3'] render different box-shadow values while you
migrate to the semantic shadow scale.
Comment on lines
+15
to
+23
| while ((match = blockRegex.exec(cssContent))) { | ||
| const mode = match[1]; | ||
| const body = match[2]; | ||
| blocks[mode] = new Set(); | ||
| const varRegex = /(--ov-[\w-]+)\s*:/g; | ||
| let varMatch; | ||
| while ((varMatch = varRegex.exec(body))) { | ||
| blocks[mode].add(varMatch[1]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re, collections, pathlib
text = pathlib.Path('packages/base-ui/src/theme/styles.css').read_text()
modes = re.findall(r'\[data-ov-theme="([^"]+)"\]', text)
print(collections.Counter(modes))
PYRepository: omniviewdev/ui
Length of output: 67
🏁 Script executed:
# First, check if the CSS file exists and view its content
ls -la packages/base-ui/src/theme/styles.css 2>&1 || echo "File not found"Repository: omniviewdev/ui
Length of output: 145
🏁 Script executed:
# Also check the blockRegex pattern in the code
cat -n scripts/audit/scanners/theme-coverage.mjs | head -30Repository: omniviewdev/ui
Length of output: 1283
🏁 Script executed:
# Search for CSS files in the packages/base-ui/src/theme directory
fd '\.css$' packages/base-ui/src/theme/ 2>&1 || echo "Directory might not exist"Repository: omniviewdev/ui
Length of output: 147
🏁 Script executed:
# Check the actual content of styles.css for theme blocks
grep -c '\[data-ov-theme=' packages/base-ui/src/theme/styles.cssRepository: omniviewdev/ui
Length of output: 59
🏁 Script executed:
# Extract all theme blocks to see the modes
grep '\[data-ov-theme=' packages/base-ui/src/theme/styles.css | head -20Repository: omniviewdev/ui
Length of output: 177
🏁 Script executed:
# Count occurrences of each theme mode
grep -o '\[data-ov-theme="[^"]*"\]' packages/base-ui/src/theme/styles.css | sort | uniq -cRepository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Let me check the actual CSS structure more carefully - check what's around the theme blocks
sed -n '1,50p' packages/base-ui/src/theme/styles.cssRepository: omniviewdev/ui
Length of output: 1558
🏁 Script executed:
# Verify the actual quote style and structure more broadly
grep -A 2 '\[data-ov-theme' packages/base-ui/src/theme/styles.css | head -30Repository: omniviewdev/ui
Length of output: 378
🏁 Script executed:
# Check if there are any theme blocks with double quotes
grep '\[data-ov-theme="' packages/base-ui/src/theme/styles.cssRepository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Double-check the exact quote style used
head -100 packages/base-ui/src/theme/styles.css | grep -E '\[data-ov-theme'Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Count single vs double quotes
echo "Single quotes:" $(grep -o "\[data-ov-theme='" packages/base-ui/src/theme/styles.css | wc -l)
echo "Double quotes:" $(grep -o '\[data-ov-theme="' packages/base-ui/src/theme/styles.css | wc -l)Repository: omniviewdev/ui
Length of output: 91
🏁 Script executed:
# Now check the exact structure to see if blocks repeat or not
python3 << 'EOF'
import re
text = open('packages/base-ui/src/theme/styles.css').read()
# The regex from the code (with double quotes)
blockRegex_code = /\[data-ov-theme="([^"]+)"\]\s*\{([^}]+)\}/g
# What it should be (with single quotes)
blockRegex_correct = r"\[data-ov-theme='([^']+)'\]\s*\{([^}]+)\}"
print("=== Using code regex (double quotes) ===")
matches_code = re.findall(r"\[data-ov-theme=\"([^\"]+)\"\]\s*\{([^}]+)\}", text)
print(f"Matches found: {len(matches_code)}")
print("\n=== Using correct regex (single quotes) ===")
matches_correct = re.findall(r"\[data-ov-theme='([^']+)'\]\s*\{([^}]+)\}", text)
print(f"Matches found: {len(matches_correct)}")
for mode, body in matches_correct:
print(f"Mode: {mode}, Variables: {len(re.findall(r'(--ov-[\w-]+)\s*:', body))}")
print("\n=== Checking for duplicate modes ===")
modes = [m[0] for m in matches_correct]
from collections import Counter
print(Counter(modes))
EOFRepository: omniviewdev/ui
Length of output: 201
🏁 Script executed:
# Also check the actual blockRegex in the code to confirm
grep "blockRegex" scripts/audit/scanners/theme-coverage.mjsRepository: omniviewdev/ui
Length of output: 174
🏁 Script executed:
# Fix the Python syntax and check both regex patterns
python3 << 'EOF'
import re
from collections import Counter
text = open('packages/base-ui/src/theme/styles.css').read()
# The regex from the code (with double quotes)
blockRegex_code = r"\[data-ov-theme=\"([^\"]+)\"\]\s*\{([^}]+)\}"
# What it should be (with single quotes)
blockRegex_correct = r"\[data-ov-theme='([^']+)'\]\s*\{([^}]+)\}"
print("=== Using code regex (double quotes) ===")
matches_code = re.findall(blockRegex_code, text)
print(f"Matches found: {len(matches_code)}")
print("\n=== Using correct regex (single quotes) ===")
matches_correct = re.findall(blockRegex_correct, text)
print(f"Matches found: {len(matches_correct)}")
for mode, body in matches_correct:
var_count = len(re.findall(r'(--ov-[\w-]+)\s*:', body))
print(f"Mode: {mode}, Variables found: {var_count}")
print("\n=== Checking for duplicate modes ===")
modes = [m[0] for m in matches_correct]
print("Mode counts:", Counter(modes))
EOFRepository: omniviewdev/ui
Length of output: 427
🏁 Script executed:
# Let me verify when this mismatch would cause the parseThemeBlocks function to fail
python3 << 'EOF'
import re
text = open('packages/base-ui/src/theme/styles.css').read()
# Simulate what the function actually does with its regex
blockRegex = r"\[data-ov-theme=\"([^\"]+)\"\]\s*\{([^}]+)\}"
blocks = {}
matches = re.finditer(blockRegex, text)
for match in matches:
mode = match.group(1)
body = match.group(2)
blocks[mode] = set()
varRegex = r"(--ov-[\w-]+)\s*:"
for var_match in re.finditer(varRegex, body):
blocks[mode].add(var_match.group(1))
print(f"Blocks found by code's regex: {len(blocks)}")
print(f"Blocks: {list(blocks.keys())}")
# Now show what SHOULD be found
blockRegex_correct = r"\[data-ov-theme='([^']+)'\]\s*\{([^}]+)\}"
blocks_correct = {}
matches = re.finditer(blockRegex_correct, text)
for match in matches:
mode = match.group(1)
body = match.group(2)
blocks_correct[mode] = set()
varRegex = r"(--ov-[\w-]+)\s*:"
for var_match in re.finditer(varRegex, body):
blocks_correct[mode].add(var_match.group(1))
print(f"\nBlocks found by corrected regex: {len(blocks_correct)}")
print(f"Blocks: {list(blocks_correct.keys())}")
for mode, vars in blocks_correct.items():
print(f" {mode}: {len(vars)} variables")
EOFRepository: omniviewdev/ui
Length of output: 292
Fix the regex pattern to use single quotes instead of double quotes.
The blockRegex uses double quotes [data-ov-theme="..."] but the CSS file uses single quotes [data-ov-theme='...']. This causes the regex to match zero blocks, returning an empty object and silently breaking the scanner. Although each theme mode appears only once (no overwriting issue), the quote mismatch is a critical bug.
Suggested fix
- const blockRegex = /\[data-ov-theme="([^"]+)"\]\s*\{([^}]+)\}/g;
+ const blockRegex = /\[data-ov-theme='([^']+)'\]\s*\{([^}]+)\}/g;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit/scanners/theme-coverage.mjs` around lines 15 - 23, The regex
for extracting theme blocks is using double quotes and thus fails to match CSS
selectors that use single quotes; update the pattern used to build blockRegex
(the code that iterates over cssContent and assigns into blocks[mode]) so it
accepts single-quoted theme attributes (e.g., change the selector part from
[data-ov-theme="..."] to [data-ov-theme='...'] in the regex), then re-run the
existing parsing loop (the while ((match = blockRegex.exec(cssContent))) that
populates blocks and uses varRegex) to correctly capture theme modes and their
CSS variables.
Comment on lines
+76
to
+100
| const varMatch = line.match(/var\((--ov-color-(?:bg|fg|border)-\w+)\)/); | ||
| if (!varMatch) continue; | ||
| const token = varMatch[1]; | ||
|
|
||
| // Determine if the component's directory name suggests it belongs to an IDE surface | ||
| const dirMatch = file.match(/components\/([^/]+)\//); | ||
| if (!dirMatch) continue; | ||
| const componentDir = dirMatch[1].toLowerCase(); | ||
|
|
||
| // Map component directories to expected IDE alias prefix | ||
| const IDE_DIR_MAP = { | ||
| 'sidebar': '--ov-color-sidebar-', | ||
| 'editor': '--ov-color-editor-', | ||
| 'terminal': '--ov-color-terminal-', | ||
| 'tab': '--ov-color-tab-', | ||
| 'statusbar': '--ov-color-statusbar-', | ||
| 'panel': '--ov-color-panel-', | ||
| 'chat': '--ov-color-chat-', | ||
| }; | ||
|
|
||
| for (const [dir, prefix] of Object.entries(IDE_DIR_MAP)) { | ||
| if (componentDir.includes(dir) && !line.includes(prefix)) { | ||
| results.push(finding('Medium', 'Token/Styling', 'Missing IDE alias', | ||
| file, num, line, | ||
| `Component in "${componentDir}" uses ${token} — consider IDE alias (${prefix}*)`)); |
There was a problem hiding this comment.
The IDE alias heuristic is both too narrow and too broad.
var\((--ov-color-(?:bg|fg|border)-\w+)\) misses hyphenated tokens like --ov-color-bg-surface-raised, while componentDir.includes('tab') will also classify names like data-table as tab surfaces. That makes this report simultaneously incomplete and noisy.
Suggested direction
- const varMatch = line.match(/var\((--ov-color-(?:bg|fg|border)-\w+)\)/);
+ const varMatch = line.match(/var\((--ov-color-(?:bg|fg|border)-[\w-]+)(?:,\s*[^)]+)?\)/);
- if (componentDir.includes(dir) && !line.includes(prefix)) {
+ const dirPattern = new RegExp(`(?:^|-)${dir}s?(?:$|-)`);
+ if (dirPattern.test(componentDir) && !token.startsWith(prefix)) {📝 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
| const varMatch = line.match(/var\((--ov-color-(?:bg|fg|border)-\w+)\)/); | |
| if (!varMatch) continue; | |
| const token = varMatch[1]; | |
| // Determine if the component's directory name suggests it belongs to an IDE surface | |
| const dirMatch = file.match(/components\/([^/]+)\//); | |
| if (!dirMatch) continue; | |
| const componentDir = dirMatch[1].toLowerCase(); | |
| // Map component directories to expected IDE alias prefix | |
| const IDE_DIR_MAP = { | |
| 'sidebar': '--ov-color-sidebar-', | |
| 'editor': '--ov-color-editor-', | |
| 'terminal': '--ov-color-terminal-', | |
| 'tab': '--ov-color-tab-', | |
| 'statusbar': '--ov-color-statusbar-', | |
| 'panel': '--ov-color-panel-', | |
| 'chat': '--ov-color-chat-', | |
| }; | |
| for (const [dir, prefix] of Object.entries(IDE_DIR_MAP)) { | |
| if (componentDir.includes(dir) && !line.includes(prefix)) { | |
| results.push(finding('Medium', 'Token/Styling', 'Missing IDE alias', | |
| file, num, line, | |
| `Component in "${componentDir}" uses ${token} — consider IDE alias (${prefix}*)`)); | |
| const varMatch = line.match(/var\((--ov-color-(?:bg|fg|border)-[\w-]+)(?:,\s*[^)]+)?\)/); | |
| if (!varMatch) continue; | |
| const token = varMatch[1]; | |
| // Determine if the component's directory name suggests it belongs to an IDE surface | |
| const dirMatch = file.match(/components\/([^/]+)\//); | |
| if (!dirMatch) continue; | |
| const componentDir = dirMatch[1].toLowerCase(); | |
| // Map component directories to expected IDE alias prefix | |
| const IDE_DIR_MAP = { | |
| 'sidebar': '--ov-color-sidebar-', | |
| 'editor': '--ov-color-editor-', | |
| 'terminal': '--ov-color-terminal-', | |
| 'tab': '--ov-color-tab-', | |
| 'statusbar': '--ov-color-statusbar-', | |
| 'panel': '--ov-color-panel-', | |
| 'chat': '--ov-color-chat-', | |
| }; | |
| for (const [dir, prefix] of Object.entries(IDE_DIR_MAP)) { | |
| const dirPattern = new RegExp(`(?:^|-)${dir}s?(?:$|-)`); | |
| if (dirPattern.test(componentDir) && !token.startsWith(prefix)) { | |
| results.push(finding('Medium', 'Token/Styling', 'Missing IDE alias', | |
| file, num, line, | |
| `Component in "${componentDir}" uses ${token} — consider IDE alias (${prefix}*)`)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit/scanners/theme-coverage.mjs` around lines 76 - 100, The current
token regex and directory check are misclassifying names: update the token
capture (varMatch) to allow hyphens in the token tail (e.g. use
/var\((--ov-color-(?:bg|fg|border)-[\w-]+)\)/) so tokens like
--ov-color-bg-surface-raised are matched, and replace the broad substring check
(componentDir.includes(dir)) with a segment-aware match (e.g. split componentDir
on /[-_]/ and check if the resulting segments include the IDE_DIR_MAP key) so
names like data-table won’t be mistaken for tab; keep using IDE_DIR_MAP and
results.push as-is.
Comment on lines
+8
to
+9
| const RAW_SPACING = /(?:margin|padding|gap)\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_RADIUS = /border-radius\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; |
There was a problem hiding this comment.
Directional spacing and radius properties are invisible to this audit.
RAW_SPACING only matches shorthand margin, padding, and gap, so padding-top, padding-inline-start, row-gap, etc. are missed. The raw padding-top: 4px added in packages/base-ui/src/components/timeline/Timeline.module.css at Line 224 and Line 229 would never be surfaced. The same gap exists for corner-specific radius properties.
Also applies to: 55-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit/scanners/token-consistency.mjs` around lines 8 - 9, The regexes
RAW_SPACING and RAW_RADIUS only match shorthand properties (margin, padding,
gap, border-radius) and therefore miss directional and corner-specific variants
(e.g., padding-top, padding-inline-start, row-gap, column-gap,
border-top-left-radius). Update RAW_SPACING and RAW_RADIUS to include
directional and corner variants (or use a pattern that matches property names
that start with or contain margin/padding/gap and border-...-radius), then
update the corresponding duplicated patterns around lines 55-65 to the same
expanded patterns so properties like padding-top: 4px and border-top-left-radius
are detected.
| const RAW_RADIUS = /border-radius\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_FONT_SIZE = /font-size\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_Z_INDEX = /z-index\s*:\s*\d+(?!.*var\(--ov)/; | ||
| const RAW_TRANSITION = /(?:transition|animation)(?:-duration|-timing-function)?\s*:(?!.*var\(--ov)/; |
There was a problem hiding this comment.
Tokenized multiline transitions will be reported as violations.
This scanner evaluates one line at a time, so a declaration split like transition: followed by tokenized values on later lines still matches RAW_TRANSITION. packages/base-ui/src/components/editor-tabs/EditorTabs.module.css at Line 178 through Line 181 is a concrete false positive.
Also applies to: 28-28, 79-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit/scanners/token-consistency.mjs` at line 12, The current
RAW_TRANSITION regex in scripts/audit/scanners/token-consistency.mjs flags
multi-line declarations as violations because the scanner tests one line at a
time; fix by making the check multiline-aware: update RAW_TRANSITION so it
matches the entire declaration span (across newlines) up to the terminating
semicolon or block boundary and only flags when that full span does not contain
var(--ov) — e.g. use the dotAll (/s) modifier or join lines until a semicolon/}
and change the negative lookahead to search that full span; refer to
RAW_TRANSITION in token-consistency.mjs and the scanner logic that applies it
when implementing this change.
Comment on lines
+4
to
+6
| import { relative } from 'path'; | ||
|
|
||
| const ROOT = new URL('../../', import.meta.url).pathname.replace(/\/$/, ''); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js ES modules, what is the recommended way to convert import.meta.urlto a filesystem path, and why isURL.pathname incorrect for Windows or percent-encoded paths?
💡 Result:
Recommended conversion is to use Node’s fileURLToPath() (and then path.dirname() if you need a directory):
import { fileURLToPath } from 'node:url';
import { dirname } from 'node:path';
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
// or: const __dirname = fileURLToPath(new URL('.', import.meta.url));URL.pathname is incorrect because it returns the URL-form path, not an OS filesystem path: on Windows it keeps a leading / and forward slashes (e.g. file:///C:/path/ → /C:/path/) and it also fails for UNC paths (e.g. file://nas/foo.txt → /foo.txt). It additionally preserves percent-encoding (e.g. file:///hello world → /hello%20world, file:///你好.txt → /%E4%BD...). fileURLToPath() fixes these by decoding percent-encoded characters and producing a valid, platform-specific absolute path. [1][2][3]
🏁 Script executed:
# Check the actual code in utils.mjs
head -10 scripts/audit/utils.mjsRepository: omniviewdev/ui
Length of output: 399
🏁 Script executed:
# Find files that import ROOT from utils.mjs
rg "from.*utils\.mjs|require.*utils" scripts/audit/ --type jsRepository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Check how ROOT is used in the file and its dependents
rg "ROOT" scripts/audit/ --type js -B 2 -A 2Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
# Read the full utils.mjs to see how ROOT is used
wc -l scripts/audit/utils.mjsRepository: omniviewdev/ui
Length of output: 84
🏁 Script executed:
# List all files in scripts/audit
ls -la scripts/audit/Repository: omniviewdev/ui
Length of output: 405
🏁 Script executed:
# Search for ROOT in any JS/mjs files in scripts/audit directory
find scripts/audit -type f \( -name "*.js" -o -name "*.mjs" \) -exec grep -l "ROOT" {} \;Repository: omniviewdev/ui
Length of output: 148
🏁 Script executed:
# Read the full utils.mjs file
cat -n scripts/audit/utils.mjsRepository: omniviewdev/ui
Length of output: 1896
🏁 Script executed:
# Read report.mjs to see how ROOT is used there
cat -n scripts/audit/report.mjsRepository: omniviewdev/ui
Length of output: 2685
Use fileURLToPath() for ROOT.
new URL(...).pathname gives you a URL path, not a real filesystem path. It leaves %20-style escapes intact and produces broken Windows drive paths, so glob() at line 17, relative() at line 49, and the output path in report.mjs line 56 can all misbehave when the repo lives under an encoded path or on Windows.
Suggested fix
-import { relative } from 'path';
+import { relative, resolve } from 'path';
+import { fileURLToPath } from 'url';
-const ROOT = new URL('../../', import.meta.url).pathname.replace(/\/$/, '');
+const ROOT = resolve(fileURLToPath(new URL('.', import.meta.url)), '../..');📝 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
| import { relative } from 'path'; | |
| const ROOT = new URL('../../', import.meta.url).pathname.replace(/\/$/, ''); | |
| import { relative, resolve } from 'path'; | |
| import { fileURLToPath } from 'url'; | |
| const ROOT = resolve(fileURLToPath(new URL('.', import.meta.url)), '../..'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/audit/utils.mjs` around lines 4 - 6, Replace the current ROOT
definition that uses new URL(...).pathname with fileURLToPath(new URL(...)) to
obtain a proper filesystem path; add an import for fileURLToPath from 'url' and
preserve the existing .replace(/\/$/, '') trimming of a trailing slash so
existing uses of ROOT (glob(), relative(), and report output) work correctly on
Windows and with percent-encoded paths.
Scanner improvements: - Use fileURLToPath for cross-platform path handling - Skip CSS-var-only inline styles (pass-through pattern) - Multi-line JSX tag buffering for accessibility scanner - Per-component memo check instead of file-level heuristic - Multi-line transition/arrow-prop detection - Single-quote support in theme block parsing - Segment-aware directory matching for IDE aliases - Directional spacing/radius property detection CSS fixes: - Add fallback values for CSS custom properties - Restore distinct elevation shadows (Card, Sheet, Toast) - Restore lg variant font-sizes (Checkbox, CodeBlock, Radio, Tooltip) - Centralize 4px spacing as component-local variables - Add box-sizing: border-box to editor containers - Switch thumb: use surface-raised instead of fg-inverse - Add TODO comments for missing --ov-space-stack-xs token TSX fixes: - DataTableHeader: add aria-sort, wrap sortable content in button - Remove 13 no-op eslint-disable-line comments - Fix React.CSSProperties → CSSProperties type reference - Update plan doc to remove absolute paths and fix typecheck expectations
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/base-ui/src/components/aspect-ratio/AspectRatio.tsx (1)
10-10:⚠️ Potential issue | 🟡 MinorGuard invalid ratios before setting
--_aspect-ratio.When
ratio={0}, a negative value, orNaNis passed, the custom property becomes defined but invalid. CSS will not fall back to the default value when a custom property is set to an invalid value—the fallback only applies if the variable is completely undefined. Add validation to ensure only positive finite values are set, falling back toundefinedfor invalid inputs.Proposed fix
export const AspectRatio = forwardRef<HTMLDivElement, AspectRatioProps>(function AspectRatio( { ratio = 16 / 9, className, style, ...props }, ref, ) { + const safeRatio = Number.isFinite(ratio) && ratio > 0 ? ratio : undefined; + return ( <div ref={ref} className={cn(styles.Root, className)} - style={{ '--_aspect-ratio': ratio, ...style } as CSSProperties} + style={{ '--_aspect-ratio': safeRatio, ...style } as CSSProperties} {...props} /> ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/aspect-ratio/AspectRatio.tsx` at line 10, The AspectRatio component currently assigns the passed ratio directly to the CSS custom property --_aspect-ratio; instead validate the ratio prop (in the AspectRatio function/component) and only set --_aspect-ratio when ratio is a positive finite number (e.g., Number.isFinite(ratio) && ratio > 0); for invalid values (0, negative, NaN, Infinity) omit the custom property (set it to undefined or don't include it in the style object) so the CSS fallback applies. Locate where style or styleVars are built (the spot using ratio / 16/9 default and setting --_aspect-ratio) and replace the direct assignment with the guarded value before merging into the component's style prop.
♻️ Duplicate comments (4)
packages/base-ui/src/components/pagination/Pagination.module.css (1)
5-6:⚠️ Potential issue | 🟡 MinorHardcoded
4pxcontradicts token usage on line 35.The comment states
--ov-space-stack-xsdoesn't exist yet, but line 35 in this same file usesvar(--ov-space-stack-xs)for padding. Either the token is available (and should be used here), or line 35 is referencing a non-existent token.Suggested fix
- /* TODO: replace 4px with var(--ov-space-stack-xs) when token is available */ - --_ov-gap: 4px; /* local alias for xs gap; no --ov-space-stack-xs semantic token yet */ + --_ov-gap: var(--ov-space-stack-xs);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/pagination/Pagination.module.css` around lines 5 - 6, The hardcoded gap (--_ov-gap: 4px) contradicts the token usage elsewhere; update the definition of the local alias (--_ov-gap) to use the semantic token with a safe fallback so it matches usages like var(--ov-space-stack-xs) used later—i.e., change the --_ov-gap declaration in Pagination.module.css to reference --ov-space-stack-xs with a 4px fallback so both the token and existing layout are supported (adjust any usages that reference --_ov-gap if needed).packages/base-ui/src/components/skeleton/Skeleton.module.css (1)
11-14:⚠️ Potential issue | 🟠 MajorText skeleton sizing is still being overridden by later selectors.
Line 13 and Line 14 hardcode the text variant back to
1em/100%, and Line 33, Line 37, and Line 41 hardcode the size-specific heights again. That still stomps the--_sk-width/--_sk-heightvalues coming fromSkeleton.tsx, sowidth,height, and the multiline 80% last-line width do not work forvariant="text".Proposed fix
.Root[data-ov-variant='text'] { - height: 1em; - width: 100%; + height: var(--_sk-height, 1em); + width: var(--_sk-width, 100%); border-radius: var(--ov-radius-sm); } @@ .Root[data-ov-variant='text'][data-ov-size='sm'] { - height: 0.75em; + height: var(--_sk-height, 0.75em); } @@ .Root[data-ov-variant='text'][data-ov-size='md'] { - height: 1em; + height: var(--_sk-height, 1em); } @@ .Root[data-ov-variant='text'][data-ov-size='lg'] { - height: 1.25em; + height: var(--_sk-height, 1.25em); }Also applies to: 32-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/skeleton/Skeleton.module.css` around lines 11 - 14, The .Root[data-ov-variant='text'] and the size-specific selectors are hardcoding height/width and overriding the CSS custom properties set by Skeleton.tsx; remove or change the hardcoded rules in the CSS (the .Root[data-ov-variant='text'] selector and the size-specific height rules around the same block) so that width and height read from --_sk-width and --_sk-height (and the multiline last-line 80% calculation) instead of forced 1em/100%; keep the selector present for variant-specific styles but replace the explicit height/width values with usage of the custom properties (so variant="text" respects --_sk-width/--_sk-height/multiline logic from Skeleton.tsx).packages/base-ui/src/components/stack/Stack.module.css (1)
20-22: 🧹 Nitpick | 🔵 TrivialClose the remaining 4px token escape hatch.
data-ov-spacing='1'still ships a literal value, so the smallest Stack spacing stays outside the semantic token system. Define the missing shared spacing token once in the token layer and consume it here instead of leaving the TODO in component CSS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/stack/Stack.module.css` around lines 20 - 22, Define the missing 4px semantic token in the design token layer (e.g., add --ov-space-stack-xs: 4px in the tokens file) and then consume that token from the Stack component CSS by replacing the hardcoded --_ov-spacing: 4px inside the .Root[data-ov-spacing='1'] rule with --_ov-space-stack-xs; update any token exports so --ov-space-stack-xs is available to packages/base-ui and remove the TODO comment.docs/superpowers/plans/2026-03-10-ui-audit.md (1)
167-170:⚠️ Potential issue | 🟡 MinorHardcoded local path in commit command.
This commit step still uses an absolute path that is specific to the author's machine. While the earlier command at line 32 was addressed in past reviews, this pattern persists in multiple commit steps throughout the document.
📝 Suggested fix
- [ ] **Step 4: Commit scaffold** ```bash -cd /Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit git add scripts/audit/ git commit -m "chore(audit): add scan script scaffold with utils and report modules"Apply similar fixes to all other commit steps (lines 296, 436, 542, 631, 694, 781, 835, 876, 912, 954, 1000, 1044, 1092, 1117, 1143, 1188, 1301, 1348, 1391, 1424, 1453). </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/superpowers/plans/2026-03-10-ui-audit.mdaround lines 167 - 170, The
commit step contains a hardcoded absolute path
"/Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit" in the git commands (the
commit/checkout step in the diff), replace it with a relative or parameterized
path to avoid machine-specific paths—e.g., run commands from repository root or
use a variable like $REPO_DIR; update every similar commit step (the git add /
git commit sequences shown and the other instances listed in the comment) so
they do not include absolute local paths and instead use relative paths or
environment variables.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/superpowers/plans/2026-03-10-ui-audit.md:
- Around line 323-337: The current parseThemeBlocks function uses a regex that
fails on nested braces; replace the regex-based body capture with a brace-aware
extraction: scan for the "[data-ov-theme="..."]" selector (use the existing
blockRegex to locate the selector and its start index), then call a
brace-counting extractor (or use a CSS parser like css-tree) to retrieve the
full block body including nested braces, and then run the existing varRegex
(--ov-[\w-]+) against that extracted body to populate blocks[mode]; update
parseThemeBlocks to advance the regex/index past the extracted block so nested
blocks are handled correctly.- Around line 1267-1279: The current logic treats any forwardRef as "wrapped"
via isForwardRef, but the rule should only consider memo-wrapped forwardRef as
satisfying the memo requirement; update the exportedComponents.set call to
remove isForwardRef from the wrapped boolean so it becomes wrapped:
isMemoWrapped || isMemoForwardRef; also ensure the isMemoForwardRef detection
covers both memo(forwardRef(...)) and React.memo(React.forwardRef(...)) patterns
by checking callee names/objects accordingly so only memo-wrapped forwardRef
components are treated as wrapped.- Around line 858-867: Several fenced code blocks (e.g., the example block
starting with thecss fence and its closing) are missing the required
blank line(s) before and/or after the fences; update each flagged block so there
is an empty line immediately before the opening fence and an empty line
immediately after the closing fence (apply to the blocks flagged around the
example and at lines reported: 860, 896, 933, 982, 1020, 1029, 1163), ensuring
the fences remaincss andrespectively and that surrounding markdown
(lists, paragraphs, details/summary) maintains proper spacing to satisfy
markdownlint.- Around line 663-672: The anchor detection in the clickable-element check uses
the regex /<(?:div|span|li|td|tr|img|a(?!\s))/ inside the if that tests for
onClick; this wrongly excludes common anchor tags like . Update
the regex used in that conditional (the one assigned inline in the if that
currently contains a(?!\s)) to either remove the negative lookahead so anchors
are matched, or explicitly omit 'a' and add a clarifying comment if anchors
should be excluded because they have native keyboard handling; ensure the change
is applied where the code builds the context and pushes findings (the block that
checks lines, context, and calls results.push with
finding('Medium','Accessibility','Missing keyboard handler', ...)).- Around line 189-195: The negative lookahead should be moved to immediately
after the colon so it only inspects the property value (not the rest of the
line); update each regex (RAW_SPACING, RAW_RADIUS, RAW_FONT_SIZE, RAW_Z_INDEX,
RAW_TRANSITION, RAW_BOX_SHADOW, RAW_OPACITY) to place the negative lookahead
right after the colon and constrain it to the value (for example use a lookahead
like (?![^;]*var(--ov)) or similar so it only scans up to the terminating
semicolon) rather than using (?!.*var(--ov) which inspects the whole line.In
@packages/ai-ui/src/components/content/AIImageGeneration.tsx:
- Line 1: The component AIImageGeneration leaves imageLoaded true after the
first image loads, so when src changes the skeleton/fade-in is skipped; fix by
resetting imageLoaded to false whenever the src prop changes (e.g., add a
useEffect in AIImageGeneration that calls setImageLoaded(false) with [src] as
dependency), ensuring the existing imageLoaded state and image loading flow (and
any onLoad handler like handleImageLoad or image ref logic) behave normally for
each new src.In
@packages/base-ui/src/components/data-table/DataTableHeader.tsx:
- Around line 60-64: The header currently only renders the sort indicator when
the local variablesortedis truthy, so unsorted but sortable columns show no
affordance; update theDataTableHeaderrender logic to always render the
with classstyles.SortIndicatorand data attribute (e.g.,
data-ov-active) but change its appearance based onsorted(muted/low-opacity
whensortedis falsy, and arrow up/down whensortedis 'asc'/'desc'), or
alternatively add asortableprop check and render a muted indicator when
sortable; adjust the conditional aroundsortedin theDataTableHeader
component accordingly to ensure a visual hint for sortable columns.In
@packages/base-ui/src/components/form-field/FormField.module.css:
- Around line 5-10: The CSS rule .Root defines the custom property
--_space-stack-xs followed immediately by declarations; add a single blank line
between the custom property block and the standard declarations (before the
display: flex; line) to satisfy the stylelint rule and project convention while
keeping the --_space-stack-xs alias and the rest of the .Root rules unchanged.In
@packages/base-ui/src/components/image-list/ImageList.module.css:
- Around line 56-57: The .Root[data-ov-gap='1'] rule sets --_ov-gap to a raw
4px; replace that literal with the semantic token var(--ov-primitive-space-1)
and update/remove the misleading comment about "no semantic token for 4px
spacing". Locate the rule for .Root[data-ov-gap='1'] in ImageList.module.css and
change the value assigned to --_ov-gap from 4px to var(--ov-primitive-space-1),
ensuring consistency with other density variants that use
--ov-primitive-space-1.In
@packages/base-ui/src/components/segmented-control/SegmentedControl.module.css:
- Line 37: Replace the horizontal padding token used for the segmented control:
in the rule that sets padding-inline (in SegmentedControl.module.css) change
var(--ov-space-stack-md) to the semantically correct
var(--ov-space-inline-control) so the horizontal spacing uses the
inline-specific design token (--ov-space-inline-control) instead of the stack
token (--ov-space-stack-md).In
@packages/base-ui/src/components/sheet/Sheet.module.css:
- Around line 9-11: The CSS root variables --_sheet-shadow-sm and
--_sheet-shadow-lg currently use hardcoded rgb(...) values while
--_sheet-shadow-md uses the semantic token --ov-shadow-surface; update
--_sheet-shadow-sm and --_sheet-shadow-lg to use equivalent semantic tokens
(e.g., --ov-shadow-surface-sm / --ov-shadow-surface-lg) if those tokens exist,
otherwise add a clear TODO comment on each line (e.g., "TODO: replace with
--ov-shadow-surface-sm when available") so the deferred sweep can replace the
hardcoded rgb values with semantic tokens; ensure you reference the variables
--_sheet-shadow-sm, --_sheet-shadow-md, and --_sheet-shadow-lg when making the
change.In
@packages/base-ui/src/components/tag-input/TagInput.module.css:
- Line 4: The CSS fallback uses a hardcoded "4px" for --_ov-gap which prevents
theming; replace the literal fallback with an existing semantic token (for
example fall back to var(--ov-space-stack-1) or var(--ov-primitive-space-1)) or
add --ov-space-stack-xs to the theme contract; update the declaration of
--_ov-gap in TagInput.module.css to use var(--ov-space-stack-xs,
var(--ov-space-stack-1, var(--ov-primitive-space-1))) instead of 4px so spacing
respects theme tokens.In
@packages/base-ui/src/components/toolbar/Toolbar.module.css:
- Line 28: Replace the hardcoded gap in Toolbar.module.css with a
component-scoped CSS variable fallback so consumers can override it; change the
gap declaration (the one currently set to "4px") to use a CSS custom property
such as --toolbar-group-gap with a fallback of 4px (update the gap rule in
Toolbar.module.css accordingly) so the default remains 4px but can be overridden
by consumers or future tokens.In
@packages/editors/src/components/diff-viewer/DiffViewer.tsx:
- Around line 117-130: The CSS variable --_diff-height set in the DiffViewer
component via the toDim(...) inline style is never used; update the
DiffViewer.module.css so the .Inner class consumes that variable (replace the
hardcoded height: 100% with height: var(--_diff-height)) so the height prop
passed into DiffViewer actually takes effect; key symbols to locate are the
toDim function and the inline style setting '--_diff-height' in DiffViewer.tsx
and the .Inner class in DiffViewer.module.css.In
@scripts/audit/report.mjs:
- Around line 49-52: The Markdown generation loop that appends item.snippet can
break when snippets contain backticks; update the snippet handling in the
for-loop that builds md (where items and item.snippet are used) to emit a fenced
code block with a language identifier (e.g.,text orjs) instead of
inline/backtick-wrapped text or alternatively sanitize/escape backticks inside
item.snippet before appending; ensure the change is applied where md is
constructed so every snippet is wrapped as a fenced block and preserves content
containing backticks.In
@scripts/audit/scanners/accessibility.mjs:
- Around line 31-32: The anchor regex is wrong: replace the occurrences of
/<(?:div|span|li|td|tr|img|a(?!\s))[\s>]/ and
/<(?:div|span|li|td|tr|img|a(?!\s))\s*$/ with patterns that match common forms (for example use a(?:\s|>) in the first pattern and a(?:\s|$)
in the second) so that the inTag logic and subsequent clickable-element checks
include anchors; update both places where these regexes are used (the
expressions that set inTag and the similar one at the later occurrence).- Around line 41-45: The current check using //?>/ treats the ">" inside arrow
functions (=>) as the end of a JSX tag; update the logic around inTag and
tagBuffer so you only treat an isolated ">" or "/>" as tag-close (not the ">" of
"=>"). Replace the simple regex with a check that ensures the ">" is not
immediately preceded by "=" (for example use a negative lookbehind like
/(?<!=>)>/ or explicitly inspect the last two characters of tagBuffer to ignore
"=>"), and keep the existing handling for "/>" so that functions like the inTag
flag and any code depending on tagBuffer, inTag, and this closing test continue
to work correctly.In
@scripts/audit/scanners/convention-violations.mjs:
- Around line 11-30: isCssVarOnlyStyle currently only matches single-line
style={{...}} and naively splits on commas; update isCssVarOnlyStyle to support
multiline style objects and optional TypeScript casts and to robustly detect
property names rather than splitting on commas. Concretely, adjust the initial
match in isCssVarOnlyStyle to use a multiline-safe pattern (e.g.
/style\s*=\s*{\s*{([\s\S]?)}\s}(?:\sas\s+[A-Za-z0-9_<>[]\s]+)?/ ) so
casts like "as any" are ignored, then replace the comma-splitting logic with a
global regex search over the captured body to extract all property keys (use the
existing CSS_VAR_KEY concept or a global variant like
/['"]?(--[A-Za-z0-9-_]+)['"]?\s:/g) and ensure every found property matches
the CSS var pattern; skip empty entries and allow trailing commas and multiline
entries. Ensure you modify the function isCssVarOnlyStyle and reuse/adjust
CSS_VAR_KEY accordingly.- Line 2: The code currently uses the POSIX-only regex //[^/]+$/ to strip
filenames when building dirSet, which breaks on Windows; update the import line
to pull dirname and join from 'node:path' (add dirname and join to the existing
import) and replace the regex-based filename stripping with
path.dirname(filePath) when populating dirSet, and use path.join(dirnameValue,
'index.ts') (or join(dirnameValue, exportName) as appropriate) instead of string
concatenation to construct the barrel export path checks; update any references
to the regex and manual string assembly so dirSet and export-path checks use
dirname() and join() for cross-platform correctness.
Outside diff comments:
In@packages/base-ui/src/components/aspect-ratio/AspectRatio.tsx:
- Line 10: The AspectRatio component currently assigns the passed ratio directly
to the CSS custom property --_aspect-ratio; instead validate the ratio prop (in
the AspectRatio function/component) and only set --_aspect-ratio when ratio is a
positive finite number (e.g., Number.isFinite(ratio) && ratio > 0); for invalid
values (0, negative, NaN, Infinity) omit the custom property (set it to
undefined or don't include it in the style object) so the CSS fallback applies.
Locate where style or styleVars are built (the spot using ratio / 16/9 default
and setting --_aspect-ratio) and replace the direct assignment with the guarded
value before merging into the component's style prop.
Duplicate comments:
In@docs/superpowers/plans/2026-03-10-ui-audit.md:
- Around line 167-170: The commit step contains a hardcoded absolute path
"/Users/joshuapare/Repos/omniviewdev/ui/feat/ui-audit" in the git commands (the
commit/checkout step in the diff), replace it with a relative or parameterized
path to avoid machine-specific paths—e.g., run commands from repository root or
use a variable like $REPO_DIR; update every similar commit step (the git add /
git commit sequences shown and the other instances listed in the comment) so
they do not include absolute local paths and instead use relative paths or
environment variables.In
@packages/base-ui/src/components/pagination/Pagination.module.css:
- Around line 5-6: The hardcoded gap (--_ov-gap: 4px) contradicts the token
usage elsewhere; update the definition of the local alias (--_ov-gap) to use the
semantic token with a safe fallback so it matches usages like
var(--ov-space-stack-xs) used later—i.e., change the --_ov-gap declaration in
Pagination.module.css to reference --ov-space-stack-xs with a 4px fallback so
both the token and existing layout are supported (adjust any usages that
reference --_ov-gap if needed).In
@packages/base-ui/src/components/skeleton/Skeleton.module.css:
- Around line 11-14: The .Root[data-ov-variant='text'] and the size-specific
selectors are hardcoding height/width and overriding the CSS custom properties
set by Skeleton.tsx; remove or change the hardcoded rules in the CSS (the
.Root[data-ov-variant='text'] selector and the size-specific height rules around
the same block) so that width and height read from --_sk-width and --_sk-height
(and the multiline last-line 80% calculation) instead of forced 1em/100%; keep
the selector present for variant-specific styles but replace the explicit
height/width values with usage of the custom properties (so variant="text"
respects --_sk-width/--_sk-height/multiline logic from Skeleton.tsx).In
@packages/base-ui/src/components/stack/Stack.module.css:
- Around line 20-22: Define the missing 4px semantic token in the design token
layer (e.g., add --ov-space-stack-xs: 4px in the tokens file) and then consume
that token from the Stack component CSS by replacing the hardcoded
--_ov-spacing: 4px inside the .Root[data-ov-spacing='1'] rule with
--_ov-space-stack-xs; update any token exports so --ov-space-stack-xs is
available to packages/base-ui and remove the TODO comment.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `5f3ec94e-bee4-47ba-9577-11253d23f876` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 148a40ce899657b8a9c58fc5b70976f5195108fc and d7bfedec8a28e3ca1dac06db303958ac81e0a6e3. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (64)</summary> * `docs/superpowers/plans/2026-03-10-ui-audit.md` * `docs/superpowers/specs/2026-03-10-ui-audit-design.md` * `docs/superpowers/specs/audit-findings.md` * `package.json` * `packages/ai-ui/src/components/artifact/AIArtifact.module.css` * `packages/ai-ui/src/components/artifact/AIArtifact.tsx` * `packages/ai-ui/src/components/chat/ChatMessageList.tsx` * `packages/ai-ui/src/components/content/AIImageGeneration.module.css` * `packages/ai-ui/src/components/content/AIImageGeneration.tsx` * `packages/base-ui/src/components/aspect-ratio/AspectRatio.module.css` * `packages/base-ui/src/components/aspect-ratio/AspectRatio.tsx` * `packages/base-ui/src/components/basic-list/BasicList.module.css` * `packages/base-ui/src/components/card/Card.module.css` * `packages/base-ui/src/components/checkbox/Checkbox.module.css` * `packages/base-ui/src/components/chip/Chip.module.css` * `packages/base-ui/src/components/code-block/CodeBlock.module.css` * `packages/base-ui/src/components/data-table/DataTable.module.css` * `packages/base-ui/src/components/data-table/DataTableContainer.tsx` * `packages/base-ui/src/components/data-table/DataTableHeader.tsx` * `packages/base-ui/src/components/drawer/Drawer.tsx` * `packages/base-ui/src/components/editor-tabs/EditorTabs.module.css` * `packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx` * `packages/base-ui/src/components/filter-bar/FilterBar.module.css` * `packages/base-ui/src/components/find-bar/FindBar.module.css` * `packages/base-ui/src/components/form-field/FormField.module.css` * `packages/base-ui/src/components/grid/Grid.module.css` * `packages/base-ui/src/components/image-list/ImageList.module.css` * `packages/base-ui/src/components/image/Image.module.css` * `packages/base-ui/src/components/image/Image.tsx` * `packages/base-ui/src/components/meter/Meter.module.css` * `packages/base-ui/src/components/meter/Meter.tsx` * `packages/base-ui/src/components/pagination/Pagination.module.css` * `packages/base-ui/src/components/radio/Radio.module.css` * `packages/base-ui/src/components/row-list/RowList.module.css` * `packages/base-ui/src/components/row-list/RowList.tsx` * `packages/base-ui/src/components/segmented-control/SegmentedControl.module.css` * `packages/base-ui/src/components/sheet/Sheet.module.css` * `packages/base-ui/src/components/skeleton/Skeleton.module.css` * `packages/base-ui/src/components/skeleton/Skeleton.tsx` * `packages/base-ui/src/components/slider/Slider.module.css` * `packages/base-ui/src/components/stack/Stack.module.css` * `packages/base-ui/src/components/status-bar/StatusBar.module.css` * `packages/base-ui/src/components/switch/Switch.module.css` * `packages/base-ui/src/components/tag-input/TagInput.module.css` * `packages/base-ui/src/components/text-area/TextArea.module.css` * `packages/base-ui/src/components/text-area/TextArea.tsx` * `packages/base-ui/src/components/timeline/Timeline.module.css` * `packages/base-ui/src/components/toast/Toast.module.css` * `packages/base-ui/src/components/toolbar/Toolbar.module.css` * `packages/base-ui/src/components/tooltip/Tooltip.module.css` * `packages/editors/src/components/code-editor/CodeEditor.module.css` * `packages/editors/src/components/code-editor/CodeEditor.tsx` * `packages/editors/src/components/diff-viewer/DiffViewer.module.css` * `packages/editors/src/components/diff-viewer/DiffViewer.tsx` * `packages/editors/src/components/object-inspector/ObjectInspector.module.css` * `packages/editors/src/components/object-inspector/ObjectInspector.tsx` * `scripts/audit/index.mjs` * `scripts/audit/report.mjs` * `scripts/audit/scanners/accessibility.mjs` * `scripts/audit/scanners/convention-violations.mjs` * `scripts/audit/scanners/react-performance.mjs` * `scripts/audit/scanners/theme-coverage.mjs` * `scripts/audit/scanners/token-consistency.mjs` * `scripts/audit/utils.mjs` </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * packages/base-ui/src/components/drawer/Drawer.tsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+189
to
+195
| const RAW_SPACING = /(?:margin|padding|gap)\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_RADIUS = /border-radius\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_FONT_SIZE = /font-size\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/; | ||
| const RAW_Z_INDEX = /z-index\s*:\s*\d+(?!.*var\(--ov)/; | ||
| const RAW_TRANSITION = /(?:transition|animation)(?:-duration|-timing-function)?\s*:(?!.*var\(--ov)/; | ||
| const RAW_BOX_SHADOW = /box-shadow\s*:\s*(?!.*var\(--ov)(?!none)/; | ||
| const RAW_OPACITY = /(?<![a-z-])opacity\s*:\s*[\d.]+(?!.*var\(--ov)/; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test the regex patterns to verify behavior
# Test RAW_Z_INDEX pattern against various inputs
echo "Testing RAW_Z_INDEX patterns:"
echo 'z-index: 10;' | grep -P 'z-index\s*:\s*\d+(?!.*var\(--ov)' && echo "Current: matched (should match)"
echo 'z-index: var(--ov-z-modal);' | grep -P 'z-index\s*:\s*\d+(?!.*var\(--ov)' && echo "Current: matched (should NOT match)" || echo "Current: not matched (correct)"
echo 'z-index: 10; /* var(--ov) */' | grep -P 'z-index\s*:\s*\d+(?!.*var\(--ov)' && echo "Current: matched (edge case)" || echo "Current: not matched"
echo ""
echo "Testing proposed fix:"
echo 'z-index: 10;' | grep -P 'z-index\s*:\s*(?!var\(--ov)\d+' && echo "Fixed: matched (should match)"
echo 'z-index: var(--ov-z-modal);' | grep -P 'z-index\s*:\s*(?!var\(--ov)\d+' && echo "Fixed: matched (should NOT match)" || echo "Fixed: not matched (correct)"Repository: omniviewdev/ui
Length of output: 279
🏁 Script executed:
#!/bin/bash
# Test RAW_SPACING and RAW_OPACITY patterns
echo "Testing RAW_SPACING:"
echo 'margin: 10px;' | grep -P '(?:margin|padding|gap)\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)' && echo "Matched (correct)"
echo 'margin: var(--ov-margin);' | grep -P '(?:margin|padding|gap)\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)' && echo "Matched (wrong)" || echo "Not matched (correct)"
echo ""
echo "Testing RAW_OPACITY:"
echo 'opacity: 0.5;' | grep -P '(?<![a-z-])opacity\s*:\s*[\d.]+(?!.*var\(--ov)' && echo "Matched (correct)"
echo 'opacity: var(--ov-opacity);' | grep -P '(?<![a-z-])opacity\s*:\s*[\d.]+(?!.*var\(--ov)' && echo "Matched (wrong)" || echo "Not matched (correct)"
echo ""
echo "Testing edge case with RAW_OPACITY:"
echo 'opacity: 0.5; /* var(--ov) */' | grep -P '(?<![a-z-])opacity\s*:\s*[\d.]+(?!.*var\(--ov)' && echo "Edge case: matched" || echo "Edge case: not matched (false negative)"Repository: omniviewdev/ui
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Find and examine the actual file
find . -name "2026-03-10-ui-audit.md" -type fRepository: omniviewdev/ui
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the file around the specified lines
sed -n '180,200p' ./docs/superpowers/plans/2026-03-10-ui-audit.mdRepository: omniviewdev/ui
Length of output: 1008
🏁 Script executed:
#!/bin/bash
# More comprehensive test of the proposed fixes
echo "Testing comprehensive cases for all three patterns:"
echo ""
echo "=== RAW_Z_INDEX ==="
echo "Case 1: z-index: 10;"
echo 'z-index: 10;' | grep -P 'z-index\s*:\s*(?!var\(--ov)\d+' && echo "Proposed: ✓ matched"
echo "Case 2: z-index: var(--ov-modal);"
echo 'z-index: var(--ov-modal);' | grep -P 'z-index\s*:\s*(?!var\(--ov)\d+' && echo "Proposed: ✗ matched (wrong)" || echo "Proposed: ✓ not matched"
echo "Case 3: z-index: 10; /* var(--ov) */"
echo 'z-index: 10; /* var(--ov) */' | grep -P 'z-index\s*:\s*(?!var\(--ov)\d+' && echo "Proposed: ✓ matched (edge case fixed)"
echo ""
echo "=== RAW_OPACITY ==="
echo "Case 1: opacity: 0.5;"
echo 'opacity: 0.5;' | grep -P 'opacity\s*:\s*(?!var\(--ov)[\d.]+' && echo "Proposed: ✓ matched"
echo "Case 2: opacity: var(--ov-opacity);"
echo 'opacity: var(--ov-opacity);' | grep -P 'opacity\s*:\s*(?!var\(--ov)[\d.]+' && echo "Proposed: ✗ matched (wrong)" || echo "Proposed: ✓ not matched"
echo "Case 3: opacity: 0.5; /* var(--ov) */"
echo 'opacity: 0.5; /* var(--ov) */' | grep -P 'opacity\s*:\s*(?!var\(--ov)[\d.]+' && echo "Proposed: ✓ matched (edge case fixed)"Repository: omniviewdev/ui
Length of output: 589
Regex patterns produce false negatives when token references appear later on the line.
The negative lookahead (?!.*var\(--ov) is positioned to check the entire remaining string after matching the property value. This causes these patterns to fail when var(--ov) appears anywhere else on the same line, including in comments:
z-index: 10; /* var(--ov) */— not detected (false negative)opacity: 0.5; /* var(--ov) */— not detected (false negative)
Move the negative lookahead immediately after the colon to check only the property value:
Fixes for all three patterns
-const RAW_SPACING = /(?:margin|padding|gap)\s*:\s*(?!.*var\(--ov).*?\d+(?:px|rem|em)/;
+const RAW_SPACING = /(?:margin|padding|gap)\s*:\s*(?!var\(--ov).*?\d+(?:px|rem|em)/;
-const RAW_Z_INDEX = /z-index\s*:\s*\d+(?!.*var\(--ov)/;
+const RAW_Z_INDEX = /z-index\s*:\s*(?!var\(--ov)\d+/;
-const RAW_OPACITY = /(?<![a-z-])opacity\s*:\s*[\d.]+(?!.*var\(--ov)/;
+const RAW_OPACITY = /(?<![a-z-])opacity\s*:\s*(?!var\(--ov)[\d.]+/;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 189 - 195, The
negative lookahead should be moved to immediately after the colon so it only
inspects the property value (not the rest of the line); update each regex
(RAW_SPACING, RAW_RADIUS, RAW_FONT_SIZE, RAW_Z_INDEX, RAW_TRANSITION,
RAW_BOX_SHADOW, RAW_OPACITY) to place the negative lookahead right after the
colon and constrain it to the value (for example use a lookahead like
(?![^;]*var\(--ov\)) or similar so it only scans up to the terminating
semicolon) rather than using (?!.*var\(--ov) which inspects the whole line.
Comment on lines
+323
to
+337
| function parseThemeBlocks(cssContent) { | ||
| const blocks = {}; | ||
| // Match [data-ov-theme="<mode>"] { ... } blocks | ||
| const blockRegex = /\[data-ov-theme="([^"]+)"\]\s*\{([^}]+)\}/g; | ||
| let match; | ||
| while ((match = blockRegex.exec(cssContent))) { | ||
| const mode = match[1]; | ||
| const body = match[2]; | ||
| blocks[mode] = new Set(); | ||
| const varRegex = /(--ov-[\w-]+)\s*:/g; | ||
| let varMatch; | ||
| while ((varMatch = varRegex.exec(body))) { | ||
| blocks[mode].add(varMatch[1]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Theme block parsing regex won't handle nested braces.
The regex \[data-ov-theme="([^"]+)"\]\s*\{([^}]+)\} uses [^}]+ to capture block content, which will fail if the theme block contains nested braces (e.g., @supports, @media, or nested selectors).
Currently this may work if styles.css has flat structure, but it's fragile for future changes.
🔧 More robust parsing suggestion
Consider using a brace-counting approach or a proper CSS parser like css-tree for more reliable block extraction:
// Alternative: count braces to find matching close
function extractBlockContent(css, startIndex) {
let depth = 0;
let start = -1;
for (let i = startIndex; i < css.length; i++) {
if (css[i] === '{') {
if (depth === 0) start = i + 1;
depth++;
} else if (css[i] === '}') {
depth--;
if (depth === 0) return css.slice(start, i);
}
}
return '';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 323 - 337, The
current parseThemeBlocks function uses a regex that fails on nested braces;
replace the regex-based body capture with a brace-aware extraction: scan for the
"[data-ov-theme="..."]" selector (use the existing blockRegex to locate the
selector and its start index), then call a brace-counting extractor (or use a
CSS parser like css-tree) to retrieve the full block body including nested
braces, and then run the existing varRegex (--ov-[\w-]+) against that extracted
body to populate blocks[mode]; update parseThemeBlocks to advance the
regex/index past the extracted block so nested blocks are handled correctly.
Comment on lines
+663
to
+672
| // Clickable non-button elements without keyboard handler (Medium) | ||
| if (/onClick/.test(line) && /<(?:div|span|li|td|tr|img|a(?!\s))/.test(line)) { | ||
| // Check surrounding lines for onKeyDown/onKeyUp | ||
| const context = lines.slice(Math.max(0, num - 3), Math.min(lines.length, num + 3)) | ||
| .map(l => l.line).join(' '); | ||
| if (!/onKeyDown|onKeyUp|role=/.test(context)) { | ||
| results.push(finding('Medium', 'Accessibility', 'Missing keyboard handler', file, num, line, | ||
| 'Clickable non-button element without onKeyDown/onKeyUp or role')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Anchor tag detection may not work as intended.
Line 664's regex /<(?:div|span|li|td|tr|img|a(?!\s))/ uses a(?!\s) (anchor not followed by space). However, valid anchor tags like <a href="..."> have a space after the tag name, so they won't match.
If the intent is to include anchors in the check, remove the negative lookahead. If anchors should be excluded (because they have native keyboard support), consider adding a comment explaining this.
🔧 Suggested fix if anchors should be included
- if (/onClick/.test(line) && /<(?:div|span|li|td|tr|img|a(?!\s))/.test(line)) {
+ if (/onClick/.test(line) && /<(?:div|span|li|td|tr|img|a)\b/.test(line)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 663 - 672, The
anchor detection in the clickable-element check uses the regex
/<(?:div|span|li|td|tr|img|a(?!\s))/ inside the if that tests for onClick; this
wrongly excludes common anchor tags like <a href="...">. Update the regex used
in that conditional (the one assigned inline in the if that currently contains
a(?!\s)) to either remove the negative lookahead so anchors are matched, or
explicitly omit 'a' and add a clarifying comment if anchors should be excluded
because they have native keyboard handling; ensure the change is applied where
the code builds the context and pushes findings (the block that checks lines,
context, and calls results.push with finding('Medium','Accessibility','Missing
keyboard handler', ...)).
Comment on lines
+858
to
+867
|
|
||
| Example: | ||
| ```css | ||
| /* Before */ | ||
| color: #6b7280; | ||
|
|
||
| /* After */ | ||
| color: var(--ov-color-fg-muted); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Markdown formatting: code blocks should have surrounding blank lines.
Multiple code blocks in the document lack blank lines before/after them (flagged by markdownlint at lines 860, 896, 933, 982, 1020, 1029, 1163). This can cause rendering issues in some markdown processors.
📝 Example fix for lines 858-867
Example:
+
```css
/* Before */
color: `#6b7280`;
/* After */
color: var(--ov-color-fg-muted);- Step 2: Run tests to ensure no regressions
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Example:
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 860-860: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 858 - 867,
Several fenced code blocks (e.g., the example block starting with the ```css
fence and its closing ```) are missing the required blank line(s) before and/or
after the fences; update each flagged block so there is an empty line
immediately before the opening fence and an empty line immediately after the
closing fence (apply to the blocks flagged around the example and at lines
reported: 860, 896, 933, 982, 1020, 1029, 1163), ensuring the fences remain
```css and ``` respectively and that surrounding markdown (lists, paragraphs,
details/summary) maintains proper spacing to satisfy markdownlint.
Comment on lines
+1267
to
+1279
| const isForwardRef = init?.type === 'CallExpression' && ( | ||
| init.callee?.name === 'forwardRef' || | ||
| (init.callee?.object?.name === 'React' && init.callee?.property?.name === 'forwardRef') | ||
| ); | ||
| // memo(forwardRef(...)) pattern | ||
| const isMemoForwardRef = isMemoWrapped && init.arguments?.[0]?.type === 'CallExpression' && ( | ||
| init.arguments[0].callee?.name === 'forwardRef' | ||
| ); | ||
|
|
||
| exportedComponents.set(node.id.name, { | ||
| node, | ||
| wrapped: isMemoWrapped || isForwardRef || isMemoForwardRef, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider whether forwardRef alone should satisfy the memo requirement.
The rule treats forwardRef components as "wrapped" (line 1278), meaning they won't trigger the warning. However, forwardRef doesn't provide memoization benefits — it only enables ref forwarding.
If the intent is to require memoization for performance, a bare forwardRef component would still re-render on parent updates. The design doc (line 54) specifies checking for "memo-wrapped forwardRef".
🔧 Suggested adjustment
- exportedComponents.set(node.id.name, {
- node,
- wrapped: isMemoWrapped || isForwardRef || isMemoForwardRef,
- });
+ exportedComponents.set(node.id.name, {
+ node,
+ // Only consider memo-wrapped (including memo(forwardRef(...))) as satisfying the requirement
+ // Pure forwardRef without memo should still trigger the warning
+ wrapped: isMemoWrapped || isMemoForwardRef,
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-10-ui-audit.md` around lines 1267 - 1279, The
current logic treats any forwardRef as "wrapped" via isForwardRef, but the rule
should only consider memo-wrapped forwardRef as satisfying the memo requirement;
update the exportedComponents.set call to remove isForwardRef from the wrapped
boolean so it becomes wrapped: isMemoWrapped || isMemoForwardRef; also ensure
the isMemoForwardRef detection covers both memo(forwardRef(...)) and
React.memo(React.forwardRef(...)) patterns by checking callee names/objects
accordingly so only memo-wrapped forwardRef components are treated as wrapped.
Scanner fixes: - Fix anchor tag detection (a(?!\s) → a\b) in accessibility scanner - Fix arrow function => treated as JSX tag close - Handle as CSSProperties cast and spreads in isCssVarOnlyStyle - Escape backticks in report snippet output Component fixes: - DiffViewer .Inner now consumes --_diff-height CSS variable - AIImageGeneration resets imageLoaded when src changes - TagInput uses var(--ov-primitive-space-1) fallback instead of 4px literal - Sheet shadow aliases get TODO comments for future tokens - Toolbar gap uses component-scoped CSS variable - SegmentedControl uses --ov-space-inline-control for padding-inline - ImageList gap uses var(--ov-primitive-space-1) with TODO - FormField empty line per Stylelint convention - DataTableHeader documents intentional unsorted column design
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
packages/base-ui/src/components/form-field/FormField.module.css (1)
75-79: 🧹 Nitpick | 🔵 TrivialCentralize the remaining 4px escape hatch too.
4pxis still hardcoded here, so this exception now lives in two places again. Reuse the same private custom property pattern for.SectionHeaderas well to keep future spacing tweaks consistent.♻️ One way to share the alias across both selectors
+.Root, +.SectionHeader { + --_space-stack-xs: 4px; /* --ov-space-stack-xs; no semantic token for 4px yet */ +} + .Root { - --_space-stack-xs: 4px; /* --ov-space-stack-xs; no semantic token for 4px yet */ - display: flex; flex-direction: column; gap: var(--_space-stack-xs); } @@ .SectionHeader { display: flex; flex-direction: column; - gap: 4px; /* --ov-space-stack-xs; no semantic token for 4px spacing; not derived from --_space-stack-xs as .Section/.SectionHeader are independent of .Root */ + gap: var(--_space-stack-xs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/form-field/FormField.module.css` around lines 75 - 79, The .SectionHeader rule still hardcodes gap: 4px; instead centralize that "escape hatch" by reusing the existing private custom property pattern (the same --_space-stack-xs alias used elsewhere) so both .Section and .SectionHeader share the value; update .SectionHeader to use the private variable (e.g., gap: var(--_space-stack-xs)) and ensure the variable is defined where the other selector expects it so spacing changes remain consistent across FormField.module.css.packages/base-ui/src/components/image-list/ImageList.module.css (1)
56-58:⚠️ Potential issue | 🟠 MajorDon’t leave a primitive-token exception in component CSS.
Line 57 still references
--ov-primitive-space-1, so this file remains outside the semantic-token cleanup the PR is aiming for. Please add thexssemantic spacing alias in the token layer and consume that here instead of shipping the TODO fallback.♻️ Proposed fix
.Root[data-ov-gap='1'] { - --_ov-gap: var(--ov-primitive-space-1, 4px); /* TODO: replace with --ov-space-stack-xs when semantic token is available */ + --_ov-gap: var(--ov-space-stack-xs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/image-list/ImageList.module.css` around lines 56 - 58, Replace the primitive token usage in the ImageList CSS by adding and using the semantic spacing alias: add the xs semantic spacing token (e.g., --ov-space-stack-xs) in the tokens layer and update the .Root[data-ov-gap='1'] rule to set --_ov-gap to that semantic token instead of --ov-primitive-space-1; specifically change the declaration that currently references --ov-primitive-space-1 to reference the new semantic token and remove the TODO fallback so the component consumes --ov-space-stack-xs via --_ov-gap.scripts/audit/scanners/convention-violations.mjs (1)
100-111:⚠️ Potential issue | 🟠 MajorUse
node:pathfor cross-platform path handling.The regex-based path manipulation on line 104 and string concatenation on line 106 only work on POSIX systems. On Windows, paths use backslashes and this code will fail.
Proposed fix
+import { dirname, join } from 'node:path'; import { findFiles, readLines, finding } from '../utils.mjs'; @@ // Get unique component directories - const dirSet = new Set(allComponentDirs.map(f => f.replace(/\/[^/]+$/, ''))); + const dirSet = new Set(allComponentDirs.map(f => dirname(f))); for (const dir of dirSet) { - const hasIndex = existsSync(`${dir}/index.ts`) || existsSync(`${dir}/index.tsx`); + const hasIndex = existsSync(join(dir, 'index.ts')) || existsSync(join(dir, 'index.tsx')); if (!hasIndex) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audit/scanners/convention-violations.mjs` around lines 100 - 111, Replace the POSIX-only path operations with Node's path utilities: import { existsSync } from 'fs' and import { dirname, join } from 'node:path', then compute directories using dirname instead of the regex (replace(/\/[^/]+$/, '')) when building dirSet and check for index files using join(dir, 'index.ts') and join(dir, 'index.tsx') rather than string concatenation; update references in the loop that use dirSet, existsSync, and the finding call to work with these path helpers to ensure cross-platform compatibility.scripts/audit/scanners/accessibility.mjs (1)
51-62:⚠️ Potential issue | 🟡 MinorThe clickable-element check may miss
<a>elements that are naturally keyboard-accessible.Anchor elements (
<a>) withhrefare already keyboard-accessible and focusable by default. Flagging them for missingonKeyDown/tabIndexcreates false positives. Consider excluding<a>from this specific check, or verifying whetherhrefis present.Proposed adjustment
// Clickable non-button elements without keyboard handler (Medium) - if (/onClick/.test(ctx) && /<(?:div|span|li|td|tr|img|a)\b/.test(ctx)) { + if (/onClick/.test(ctx) && /<(?:div|span|li|td|tr|img)\b/.test(ctx)) { const hasKeyboardHandler = /onKeyDown|onKeyUp/.test(ctx); const hasRoleButton = /role\s*=\s*["']button["']/.test(ctx); const hasTabIndex = /tabIndex|tabindex/.test(ctx);Alternatively, keep
<a>but skip ifhref=is present:+ const isAnchorWithHref = /<a\b/.test(ctx) && /href\s*=/.test(ctx); // Sufficient if: explicit keyboard handler, or role=button + tabIndex - const isSufficient = hasKeyboardHandler || (hasRoleButton && hasTabIndex); + const isSufficient = hasKeyboardHandler || (hasRoleButton && hasTabIndex) || isAnchorWithHref;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audit/scanners/accessibility.mjs` around lines 51 - 62, The check that flags clickable non-button elements in the block using ctx, hasKeyboardHandler, hasRoleButton, hasTabIndex and isSufficient should be adjusted to avoid false positives for anchor tags that are natively keyboard-accessible; update the logic that currently tests /<(?:div|span|li|td|tr|img|a)\b/ so that it either excludes 'a' or, better, verifies an href attribute (e.g. test for /<a\b[^>]*\bhref\s*=/) and treat an <a href="..."> as sufficient so it doesn't push the 'Missing keyboard handler' finding via finding(...).packages/base-ui/src/components/data-table/DataTableHeader.tsx (1)
49-73:⚠️ Potential issue | 🟠 MajorSkip the interactive wrapper for placeholder headers.
This branch still renders a focusable sort button when
header.isPlaceholderis true; it only hides the contents. In grouped-header layouts, that leaves a blank cell in the tab order that can still toggle sorting.Suggested fix
- {canSort ? ( + {canSort && !header.isPlaceholder ? ( <button type="button" className={styles.SortButton} onClick={header.column.getToggleSortingHandler()} aria-label={`Sort by ${typeof header.column.columnDef.header === 'string' ? header.column.columnDef.header : header.column.id}`} > - {header.isPlaceholder - ? null - : flexRender(header.column.columnDef.header, header.getContext())} + {flexRender(header.column.columnDef.header, header.getContext())} {sorted && ( <span className={styles.SortIndicator} data-ov-active="true"> {sorted === 'asc' ? '\u2191' : '\u2193'} </span> )} </button> ) : ( header.isPlaceholder ? null : flexRender(header.column.columnDef.header, header.getContext()) )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx` around lines 49 - 73, The placeholder headers are still rendering a focusable sort <button>, so change the conditional to skip rendering the interactive wrapper when header.isPlaceholder is true: for the canSort branch, check header.isPlaceholder first and render null instead of the <button> (use header.column.getToggleSortingHandler, styles.SortButton, and the existing sort indicator logic only when not a placeholder); the non-sortable branch can keep the existing header.isPlaceholder ? null : flexRender(...) behavior.
🤖 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/data-table/DataTableHeader.tsx`:
- Around line 50-55: The sort button's aria-label currently falls back to
header.column.id when header.column.columnDef.header is not a string, which is
inaccessible for custom renderers; update the aria-label logic in the
DataTableHeader's button (the element using
header.column.getToggleSortingHandler()) to prefer a developer-provided meta
value column.meta.sortAriaLabel when present, then fall back to the string
header (header.column.columnDef.header) and only finally to header.column.id;
ensure you reference header.column.columnDef.header,
header.column.getToggleSortingHandler(), header.column.id and
header.column.columnDef.meta.sortAriaLabel so custom headers can supply an
appropriate label for screen readers.
In `@packages/editors/src/components/diff-viewer/DiffViewer.module.css`:
- Around line 4-14: The .Root selector uses height: var(--_diff-height) without
a fallback which can allow collapse if the custom property is unset; update
.Root to include a fallback (e.g., height: var(--_diff-height, 100%)) and
simplify .Inner to use height: 100% (instead of re-reading the same variable) so
percentage heights reliably fill the parent; adjust the .Root and .Inner
declarations accordingly to keep defensive styling and consistency.
In `@scripts/audit/report.mjs`:
- Around line 22-24: The three separate filter() calls that compute highCount,
medCount, and lowCount cause multiple passes over findings; replace them with a
single-pass accumulation (e.g., use Array.prototype.reduce or a for loop) that
iterates findings once and increments counters for 'High', 'Medium', and 'Low'
to compute highCount, medCount, and lowCount in one traversal; update the
variables highCount, medCount, and lowCount accordingly.
- Around line 51-54: The code is unnecessarily escaping backticks inside fenced
code block generation: remove the replace that creates escapedSnippet and
instead use the raw item.snippet when appending to md (i.e., stop calling
item.snippet.replace(/`/g, '\\`') so the snippet is rendered with literal
backticks); update the block that sets escapedSnippet and the md concatenation
around the triple-backtick fenced block (referencing item.snippet and
escapedSnippet) to use the unescaped content.
In `@scripts/audit/scanners/accessibility.mjs`:
- Around line 19-27: The current logic unconditionally resets the JSX buffering
state when encountering lines starting with comment markers, which can
prematurely clear a tag buffer if a multiline JSX tag spans a line that begins
with '*' or '//'—so update the conditional to only reset tagBuffer and set inTag
= false when inTag is true; specifically modify the block handling lines that
trim-start with '//', '*', 'import', 'type', or 'interface' to check the inTag
boolean before clearing tagBuffer and toggling inTag, ensuring
functions/variables like inTag and tagBuffer are preserved across expected
multiline tags instead of being reset.
In `@scripts/audit/scanners/convention-violations.mjs`:
- Line 48: The duplicate filesystem traversal comes from calling
findFiles('packages/*/src/**/*.tsx') twice; keep the first result stored in
tsxFiles and remove the second call, updating the later code to reuse tsxFiles
(ensure tsxFiles is declared in a scope visible to both usages or hoist its
declaration above both usages), and delete the redundant findFiles invocation so
you only traverse once.
- Around line 28-39: Don't split the input on commas (remove body.split(','))
because commas can appear inside CSS values; instead scan the string with the
CSS_VAR_KEY regex (make it global, e.g. /(--[A-Za-z0-9\-_]+)\s*:/g) to locate
actual key tokens and validate those matches; replace the pairs/pair/trimmed
loop with an iteration over the regex matches that returns false if no valid
matches are found or any match is malformed, and true only when all found keys
conform to CSS_VAR_KEY.
- Around line 63-96: The two separate loops over cssFiles that each call
readLines(file) should be consolidated into a single pass: iterate once over
cssFiles and call readLines(file) one time, then for each { line, num } run both
the class-name check (using classMatch, name, and pushing findings via
finding/results) and the state-selector check (the regex for active|selected|...
and the data-*/:hover/:focus exclusions); ensure both checks operate on the same
line within that single loop and avoid pushing duplicate findings for the same
issue.
---
Duplicate comments:
In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx`:
- Around line 49-73: The placeholder headers are still rendering a focusable
sort <button>, so change the conditional to skip rendering the interactive
wrapper when header.isPlaceholder is true: for the canSort branch, check
header.isPlaceholder first and render null instead of the <button> (use
header.column.getToggleSortingHandler, styles.SortButton, and the existing sort
indicator logic only when not a placeholder); the non-sortable branch can keep
the existing header.isPlaceholder ? null : flexRender(...) behavior.
In `@packages/base-ui/src/components/form-field/FormField.module.css`:
- Around line 75-79: The .SectionHeader rule still hardcodes gap: 4px; instead
centralize that "escape hatch" by reusing the existing private custom property
pattern (the same --_space-stack-xs alias used elsewhere) so both .Section and
.SectionHeader share the value; update .SectionHeader to use the private
variable (e.g., gap: var(--_space-stack-xs)) and ensure the variable is defined
where the other selector expects it so spacing changes remain consistent across
FormField.module.css.
In `@packages/base-ui/src/components/image-list/ImageList.module.css`:
- Around line 56-58: Replace the primitive token usage in the ImageList CSS by
adding and using the semantic spacing alias: add the xs semantic spacing token
(e.g., --ov-space-stack-xs) in the tokens layer and update the
.Root[data-ov-gap='1'] rule to set --_ov-gap to that semantic token instead of
--ov-primitive-space-1; specifically change the declaration that currently
references --ov-primitive-space-1 to reference the new semantic token and remove
the TODO fallback so the component consumes --ov-space-stack-xs via --_ov-gap.
In `@scripts/audit/scanners/accessibility.mjs`:
- Around line 51-62: The check that flags clickable non-button elements in the
block using ctx, hasKeyboardHandler, hasRoleButton, hasTabIndex and isSufficient
should be adjusted to avoid false positives for anchor tags that are natively
keyboard-accessible; update the logic that currently tests
/<(?:div|span|li|td|tr|img|a)\b/ so that it either excludes 'a' or, better,
verifies an href attribute (e.g. test for /<a\b[^>]*\bhref\s*=/) and treat an <a
href="..."> as sufficient so it doesn't push the 'Missing keyboard handler'
finding via finding(...).
In `@scripts/audit/scanners/convention-violations.mjs`:
- Around line 100-111: Replace the POSIX-only path operations with Node's path
utilities: import { existsSync } from 'fs' and import { dirname, join } from
'node:path', then compute directories using dirname instead of the regex
(replace(/\/[^/]+$/, '')) when building dirSet and check for index files using
join(dir, 'index.ts') and join(dir, 'index.tsx') rather than string
concatenation; update references in the loop that use dirSet, existsSync, and
the finding call to work with these path helpers to ensure cross-platform
compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63916b51-8d1f-4577-aa5f-0ca4d920fa9e
📒 Files selected for processing (13)
docs/superpowers/specs/audit-findings.mdpackages/ai-ui/src/components/content/AIImageGeneration.tsxpackages/base-ui/src/components/data-table/DataTableHeader.tsxpackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/image-list/ImageList.module.csspackages/base-ui/src/components/segmented-control/SegmentedControl.module.csspackages/base-ui/src/components/sheet/Sheet.module.csspackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/editors/src/components/diff-viewer/DiffViewer.module.cssscripts/audit/report.mjsscripts/audit/scanners/accessibility.mjsscripts/audit/scanners/convention-violations.mjs
- DataTableHeader: prefer meta.sortAriaLabel, skip sort button for placeholder headers - DiffViewer: move CSS var fallback to .Root, simplify .Inner to plain 100% - report.mjs: single-pass severity counting, remove unnecessary backtick escaping - accessibility.mjs: only reset tag buffer when inTag, exclude <a href> from keyboard findings - convention-violations.mjs: deduplicate findFiles, regex-based CSS var key scanning, consolidate CSS loops, use path helpers - FormField: use local --_space-stack-xs alias in .SectionHeader - ImageList: replace primitive token with semantic --ov-space-stack-xs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/base-ui/src/components/data-table/DataTableHeader.tsx (1)
36-40:⚠️ Potential issue | 🟠 MajorAvoid overriding the button name with
column.id.This still forces screen readers to hear internal IDs like
createdAtfor non-string custom headers unless every sortable column also setsmeta.sortAriaLabel. If no explicit label is available, let the button name come from the rendered header content instead of falling back toheader.column.id.Proposed fix
- const sortAriaLabel = - (meta?.sortAriaLabel as string) ?? - (typeof header.column.columnDef.header === 'string' - ? header.column.columnDef.header - : header.column.id); + const sortAriaLabel = + typeof meta?.sortAriaLabel === 'string' + ? meta.sortAriaLabel + : typeof header.column.columnDef.header === 'string' + ? header.column.columnDef.header + : undefined; ... - aria-label={`Sort by ${sortAriaLabel}`} + aria-label={sortAriaLabel ? `Sort by ${sortAriaLabel}` : undefined}Also applies to: 56-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx` around lines 36 - 40, The current construction of sortAriaLabel in DataTableHeader.tsx uses header.column.id as a fallback which forces internal IDs (e.g., createdAt) into the button name; change the logic in the sortAriaLabel assignment (using meta?.sortAriaLabel, header.column.columnDef.header, and header.column.id) to avoid falling back to header.column.id for non-string headers and instead return undefined/null so the rendered header content is used as the button name by screen readers; update the same pattern where it appears (the other instance mentioned) so only explicit meta.sortAriaLabel or a string header are used as the aria label.scripts/audit/scanners/convention-violations.mjs (1)
12-38:⚠️ Potential issue | 🟠 MajorThe CSS-var inline-style allowlist is still too lossy for audit-grade results.
Line 21 only accepts
style={{ ... }}when the closing}}is on the same line, and Lines 34-35 count anyidentifier:token in the body. That still misclassifies approved patterns like multiline pass-through objects and ternary-valued CSS vars ('--x': cond ? a : b) as High violations, which makes the scanner over-report the very fixes this PR is trying to validate.#!/bin/bash python - <<'PY' from pathlib import Path import re for path in Path('.').glob('packages/*/src/**/*.tsx'): text = path.read_text(encoding='utf-8') for m in re.finditer(r'style\s*=\s*\{\s*\{', text): start = m.start() line = text.count('\n', 0, start) + 1 window = text[start:start+300] first_line = window.splitlines()[0] multiline = '}}' not in first_line ternary = re.search(r'\?\s*[^:\n]+:\s*', window) is not None if multiline or ternary: print(f'{path}:{line}: multiline={multiline} ternary_like_value={ternary}') PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audit/scanners/convention-violations.mjs` around lines 12 - 38, isCssVarOnlyStyle currently treats only single-line style={{...}} and treats any identifier: as a non-CSS-var key; update it to (1) stop requiring the closing "}}" to be on the same input line by expanding the match to allow multiline style objects (use a regex /style\s*=\s*\{\s*\{([\s\S]*?)\}\s*\}/m or otherwise search across lines using the full input text instead of the single line variable `line`, referencing the existing `stripped` and `styleMatch` variables), and (2) change the key-detection logic so it only flags non-CSS custom properties (replace the current ALL_KEYS /['"]?([A-Za-z\-_][\w\-]*)['"]?\s*:/g with a regex that matches keys that do NOT start with --, e.g. /['"]?(?!-)([A-Za-z\-_][\w\-]*)['"]?\s*:/g) so ternary or expression values (e.g. `'--x': cond ? a : b`) and multiline pass-through objects are allowed; keep the CSS_VAR_KEY count logic and the spread-operator check as-is (refer to CSS_VAR_KEY, ALL_KEYS, body, and the early spread test).
🤖 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/form-field/FormField.module.css`:
- Around line 75-79: The .SectionHeader rule violates stylelint's
declaration-empty-line-before because there is no empty line between the custom
property (--_space-stack-xs: 4px) and the next declarations; update the
.SectionHeader block (class name SectionHeader) to insert a single blank line
after the custom property line so the empty-line-before rule is satisfied before
the regular declarations like display, flex-direction, and gap.
In `@scripts/audit/scanners/accessibility.mjs`:
- Around line 59-64: The current sufficiency check treats role="button" +
tabIndex as enough; change logic so that a keyboard handler is required: set
isSufficient = hasKeyboardHandler (do not treat hasRoleButton && hasTabIndex as
sufficient). Additionally, when hasRoleButton && hasTabIndex &&
!hasKeyboardHandler, emit a specific violation message noting the missing
keyboard handler for Enter/Space so authors know role+tabIndex alone is
insufficient; update references to hasKeyboardHandler, hasRoleButton,
hasTabIndex and isSufficient in the scanner accordingly.
---
Duplicate comments:
In `@packages/base-ui/src/components/data-table/DataTableHeader.tsx`:
- Around line 36-40: The current construction of sortAriaLabel in
DataTableHeader.tsx uses header.column.id as a fallback which forces internal
IDs (e.g., createdAt) into the button name; change the logic in the
sortAriaLabel assignment (using meta?.sortAriaLabel,
header.column.columnDef.header, and header.column.id) to avoid falling back to
header.column.id for non-string headers and instead return undefined/null so the
rendered header content is used as the button name by screen readers; update the
same pattern where it appears (the other instance mentioned) so only explicit
meta.sortAriaLabel or a string header are used as the aria label.
In `@scripts/audit/scanners/convention-violations.mjs`:
- Around line 12-38: isCssVarOnlyStyle currently treats only single-line
style={{...}} and treats any identifier: as a non-CSS-var key; update it to (1)
stop requiring the closing "}}" to be on the same input line by expanding the
match to allow multiline style objects (use a regex
/style\s*=\s*\{\s*\{([\s\S]*?)\}\s*\}/m or otherwise search across lines using
the full input text instead of the single line variable `line`, referencing the
existing `stripped` and `styleMatch` variables), and (2) change the
key-detection logic so it only flags non-CSS custom properties (replace the
current ALL_KEYS /['"]?([A-Za-z\-_][\w\-]*)['"]?\s*:/g with a regex that matches
keys that do NOT start with --, e.g. /['"]?(?!-)([A-Za-z\-_][\w\-]*)['"]?\s*:/g)
so ternary or expression values (e.g. `'--x': cond ? a : b`) and multiline
pass-through objects are allowed; keep the CSS_VAR_KEY count logic and the
spread-operator check as-is (refer to CSS_VAR_KEY, ALL_KEYS, body, and the early
spread test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c28cd3c-1823-422a-a246-a30030b89532
📒 Files selected for processing (8)
docs/superpowers/specs/audit-findings.mdpackages/base-ui/src/components/data-table/DataTableHeader.tsxpackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/image-list/ImageList.module.csspackages/editors/src/components/diff-viewer/DiffViewer.module.cssscripts/audit/report.mjsscripts/audit/scanners/accessibility.mjsscripts/audit/scanners/convention-violations.mjs
- FormField: add blank line after custom property for stylelint compliance
- accessibility: require keyboard handler (role+tabIndex alone insufficient),
emit specific message for missing Enter/Space handler
- DataTableHeader: drop column.id fallback from sortAriaLabel, let screen
readers use button content for non-string headers
- convention-violations: support multiline style={{}} in isCssVarOnlyStyle,
detect non-CSS-var keys via negative lookahead instead of total count
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/data-table/DataTableHeader.tsx`:
- Around line 36-40: Ensure sortable custom headers always have accessible names
by enforcing a runtime check in DataTableHeader (where sortAriaLabel is
computed): if header.column.getCanSort() (or equivalent sortable flag) is true
and header.column.columnDef.header is not a string, throw or assert when
meta?.sortAriaLabel is missing; alternatively, change the typing for the column
meta to require sortAriaLabel when header is a ReactNode (non-string) and
sortable. Update the logic around sortAriaLabel (the expression using
meta.sortAriaLabel and header.column.columnDef.header) to perform this check and
fail loudly so icon-only/custom headers cannot be shipped without
meta.sortAriaLabel.
In `@scripts/audit/scanners/convention-violations.mjs`:
- Around line 49-52: The current start position calculation for finding a line's
index using fullText.indexOf(line, fullText.split('\n').slice(0, num -
1).join('\n').length) can pick the wrong occurrence when identical lines exist
and also misses the intervening newline characters; fix this in the scanner loop
that uses INLINE_STYLE by maintaining a cumulative character offset (e.g.,
cumulativeOffset) that is incremented by each processed line's length plus one
for the newline as you iterate lines, and then use fullText.indexOf(line,
cumulativeOffset) (or directly use cumulativeOffset as the lineIdx when working
with exact slices) before calling isCssVarOnlyStyle(fullText, lineIdx) so the
position always targets the correct occurrence.
- Around line 104-110: You are reading each TSX file twice because
readLines(file) is called in the inline-styles pass and again in the large-files
pass; fix by capturing and reusing the line counts during the first pass (e.g.,
populate a map like fileLineCounts[file] = lines.length inside the inline-styles
loop where readLines is already called), then in the large-files loop (iterating
tsxFiles) use the cached count from fileLineCounts[file] to decide whether to
push the "Large component file" finding, and only call readLines(file) as a
fallback if the cache key is missing.
- Line 90: The dynamic import for existsSync should be converted to a top-level
static import to match the other imports and avoid per-invocation overhead:
remove the await import('fs') line and add a top-level import of existsSync
(symbol: existsSync) alongside the other imports so callers of the function use
the statically imported existsSync; ensure any references to the dynamic import
are updated to use the top-level existsSync and run tests/lint to confirm ESM
import style is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cc65ced-c171-4d0e-b49c-1470279a910b
📒 Files selected for processing (5)
docs/superpowers/specs/audit-findings.mdpackages/base-ui/src/components/data-table/DataTableHeader.tsxpackages/base-ui/src/components/form-field/FormField.module.cssscripts/audit/scanners/accessibility.mjsscripts/audit/scanners/convention-violations.mjs
- DataTableHeader: add dev-time warning when sortable column has non-string header but no meta.sortAriaLabel - convention-violations: use cumulative offset for correct multiline style position, cache line counts to avoid double readLines, convert existsSync to static top-level import
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/base-ui/src/components/data-table/DataTableHeader.tsx`:
- Around line 36-40: The current assignment to sortAriaLabel casts
meta?.sortAriaLabel to string without runtime validation; update the logic in
DataTableHeader (the sortAriaLabel determination that references
meta?.sortAriaLabel and header.column.columnDef.header) to first check typeof
meta?.sortAriaLabel === 'string' and only then use it, otherwise fall back to
the header.column.columnDef.header (if it is a string) or undefined so you never
interpolate non-string values into the aria-label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89fc8c7a-97d0-4892-b710-5f07e731633e
📒 Files selected for processing (3)
docs/superpowers/specs/audit-findings.mdpackages/base-ui/src/components/data-table/DataTableHeader.tsxscripts/audit/scanners/convention-violations.mjs
This was referenced Mar 11, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit results (before → after)
What's included
scripts/audit/— Reusable audit scanner (run viapnpm audit:ui)docs/superpowers/specs/audit-findings.md— Full findings report with triage notes and manual review--ov-primitive-*in component CSS)What's deferred to follow-up PRs
Test plan
pnpm audit:uiruns successfully and produces findings reportpnpm typecheckpasses for base-ui (editors/ai-ui have pre-existing cross-package issues)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores / Documentation