Solid structural instincts undercut by accessibility afterthoughts: clickable divs, nested buttons, and icon-only controls with no name keep locking out keyboard and screen-reader users. Credit where due, the disabled-state and Listbox patterns show real design-system thinking, but the config nav's core interaction is currently unusable without a mouse.

Files Rams reviewed

gui/src/pages/config/components/ConfigHeader.tsx

gui/src/pages/config/components/ConfigItemSelect.tsx

gui/src/pages/config/components/ConfigRow.tsx

gui/src/pages/config/components/ModelRoleRow.tsx

gui/src/pages/config/components/ModelRoleSelector.tsx

gui/src/pages/config/components/ToolPoliciesGroup.tsx

gui/src/pages/config/components/ToolPolicyItem.tsx

gui/src/pages/config/components/UserSetting.tsx

docs-site/app/components/NotFoundPage.tsx

gui/src/components/Layout.tsx

gui/src/components/console/Layout.tsx

gui/src/pages/config/components/ConfigSection.tsx

gui/src/pages/config/components/ConfigSubsection.tsx

docs-site/app/[[...slug]]/page.tsx

docs-site/app/layout.tsx

gui/src/pages/config/index.tsx

gui/src/pages/gui/ToolCallDiv/index.tsx

docs-site/app/components/ClientRedirect.tsx

gui/src/components/AssistantAndOrgListbox/index.tsx

gui/src/components/History/index.tsx

gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/index.tsx

gui/src/components/StyledMarkdownPreview/index.tsx

gui/src/components/dialogs/index.tsx

gui/src/pages/config/configTabs.tsx

gui/src/pages/config/features/indexing/IndexingProgress.tsx

gui/src/pages/config/features/indexing/IndexingProgressErrorText.tsx

gui/src/pages/config/features/indexing/IndexingProgressIndicator.tsx

gui/src/pages/config/features/indexing/IndexingProgressSubtext.tsx

gui/src/pages/config/features/keyboard/KeyboardShortcuts.tsx

gui/src/pages/config/sections/ConfigsSection.tsx

89/100

Accessibility

3 critical1 serious

Accessibility·gui/src/pages/config/components/ConfigItemSelect.tsx:33Critical

Tab items are unclickable divs, locking out keyboard users entirely

The desktop tab row renders each item as a `<div onClick={...}>` with `cursor-pointer` styling instead of a `<button>`. There is no `tabIndex`, `role`, or keyboard handler.

Why it matters

Keyboard-only and screen-reader users cannot reach or activate this control at all since a div has no default focus stop or Enter/Space handling. This is the primary navigation for config sections, so it fails WCAG 2.1.1 for the whole feature.

Fix

Use a native `<button>` for any element that triggers a state change.

<div
  style={{ fontSize: fontSize(-2) }}
  key={item.id}
  className={`hover:bg-vsc-input-background flex cursor-pointer items-center justify-center gap-1.5 rounded-md px-2 py-2 ${
    activeId === item.id ? "" : "text-gray-400"
  }`}
  onClick={() => onSelect(item.id)}
>
  {item.icon}
  {item.label}
</div>

<button
  type="button"
  style={{ fontSize: fontSize(-2) }}
  key={item.id}
  className={`hover:bg-vsc-input-background flex cursor-pointer items-center justify-center gap-1.5 rounded-md px-2 py-2 ${
    activeId === item.id ? "" : "text-gray-400"
  }`}
  onClick={() => onSelect(item.id)}
>
  {item.icon}
  {item.label}
</button>

Accessibility·gui/src/pages/config/components/ConfigRow.tsx:59Critical

Interactive children get nested inside a native button element

When `onClick` is set, the entire row (title, description, `children`, and `Icon`) renders inside `<Button variant="ghost">`. The `handleClick` guard on lines 29-43 explicitly checks for `button, input, textarea, select, [role="button"], [role="switch"]` among `children`, meaning toggles or action buttons are expected to be passed in as children of this Button.

Why it matters

Nesting a `<button>` inside another `<button>` is invalid HTML. Browsers auto-close the outer button when they hit the inner one, so the actual DOM doesn't match the JSX: focus order, hit-testing, and screen reader announcement of the row all become unpredictable. Keyboard users may not be able to reach the nested switch at all.

Fix

Never nest interactive elements inside a button; move interactive children outside the clickable button boundary.

return (
  <Button
    variant="ghost"
    className={cn(
      baseClasses,
      interactiveClasses,
      disabledClasses,
      "!my-0 text-left",
      className,
    )}
    onClick={handleClick}
    disabled={disabled}
    data-config-row
  >
    <div className="flex flex-col">
      <span className="text-sm font-medium">{title}</span>
      <p className="mt-0.5 text-xs text-gray-500">{description}</p>
    </div>
    <div className="flex items-center gap-4">
      {children}
      {Icon && <Icon className="h-5 w-5 flex-shrink-0 text-gray-400" />}
    </div>
  </Button>
);

return (
  <div
    className={cn(baseClasses, interactiveClasses, disabledClasses, className)}
    data-config-row
  >
    <Button
      variant="ghost"
      className="!my-0 flex-1 text-left"
      onClick={onClick}
      disabled={disabled}
    >
      <div className="flex flex-col">
        <span className="text-sm font-medium">{title}</span>
        <p className="mt-0.5 text-xs text-gray-500">{description}</p>
      </div>
    </Button>
    <div className="flex items-center gap-4">
      {children}
      {Icon && <Icon aria-hidden="true" className="h-5 w-5 flex-shrink-0 text-gray-400" />}
    </div>
  </div>
);

Accessibility·gui/src/pages/config/components/ModelRoleRow.tsx:57Critical

Configure button has no accessible name for screen readers

The icon-only `<Button>` wrapping `Cog6ToothIcon` relies on `<ToolTip content="Configure">` for its label. Tooltip content is not read as an accessible name by screen readers unless it's wired through `aria-label` or `aria-describedby` on the button itself.

Why it matters

Screen reader users hear an unlabeled button with no indication it opens model configuration, so they can't act on it without guessing.

Fix

Add an explicit aria-label to the icon-only button and mark its icon decorative.

<Button
  variant="ghost"
  size="sm"
  onClick={() => onConfigure(selectedModel)}
  className="text-description hover:enabled:text-foreground my-0 h-6 w-6 p-0"
>
  <Cog6ToothIcon className="h-4 w-4 flex-shrink-0" />
</Button>

<Button
  variant="ghost"
  size="sm"
  aria-label="Configure"
  onClick={() => onConfigure(selectedModel)}
  className="text-description hover:enabled:text-foreground my-0 h-6 w-6 p-0"
>
  <Cog6ToothIcon aria-hidden="true" className="h-4 w-4 flex-shrink-0" />
</Button>

Accessibility·gui/src/pages/config/components/ConfigHeader.tsx:25Serious

Heading level is decided by visual size variant, not document structure

`HeadingTag` is set to `h3` when `variant="sm"` and `h2` for the default variant: `const HeadingTag = isSmall ? "h3" : "h2";`. The semantic heading level is derived from a styling choice (`isSmall`), not from where the header sits in the page outline.

Why it matters

A future call site that reaches for `variant="sm"` purely to make the header visually smaller silently changes the document's heading hierarchy, which can skip or duplicate levels across the page and breaks screen reader navigation by heading.

Fix

Decouple heading level from visual variant by accepting an explicit `as` or `level` prop with a sensible default, so size and semantics can vary independently.

const isSmall = variant === "sm";
const marginBottom = isSmall ? "mb-4" : "mb-6";
const titleSize = isSmall ? "text-sm font-semibold" : "text-xl font-semibold";
const HeadingTag = isSmall ? "h3" : "h2";

const isSmall = variant === "sm";
const marginBottom = isSmall ? "mb-4" : "mb-6";
const titleSize = isSmall ? "text-sm font-semibold" : "text-xl font-semibold";
const HeadingTag = headingLevel ?? (isSmall ? "h3" : "h2");

98/100

Components

1 serious

Components·gui/src/pages/config/components/ModelRoleSelector.tsx:118Serious

Dropdown panel radius hardcoded via inline style, not a token

`ListboxOptions` sets `style={{ borderRadius: defaultBorderRadius }}` instead of a rounded-* class.

Why it matters

An inline style on a shared radius value can't be themed or overridden per-surface the way a Tailwind class can, and it silently diverges the moment someone updates the radius scale elsewhere in the config.

Fix

Use the design system's rounded-* utility instead of an inline style pulling from a JS constant.

<ListboxOptions
  style={{ borderRadius: defaultBorderRadius }}
  className="min-w-40"
>

<ListboxOptions
  className="min-w-40 rounded"
>

Want this on your repo?

Rams reviews your next PR automatically and posts inline fix suggestions.

Review my public repo for free

98/100

UX

1 serious

UX·gui/src/pages/config/components/ToolPoliciesGroup.tsx:90Serious

Toggle switch click also collapses/expands the row underneath it

The `ToggleSwitch` sits inside a wrapping `<div>` that is itself nested inside the outer row's `onClick={() => setIsExpanded(!isExpanded)}`. Clicking the toggle does not stop propagation, so `onToggle` fires and the click bubbles up to trigger `setIsExpanded` at the same time.

Why it matters

A user flipping the group on/off also randomly expands or collapses the tool list in the same click, so the two controls fight each other and the interaction feels broken rather than intentional.

Fix

Stop click propagation on nested interactive controls so they don't trigger the parent's handler.

<div>
  <ToggleSwitch
    isToggled={isGroupEnabled}
    onToggle={() => dispatch(toggleToolGroupSetting(groupName))}
    text=""
    size={10}
    disabled={allToolsOff}
  />
</div>

<div onClick={(e) => e.stopPropagation()}>
  <ToggleSwitch
    isToggled={isGroupEnabled}
    onToggle={() => dispatch(toggleToolGroupSetting(groupName))}
    text=""
    size={10}
    disabled={allToolsOff}
  />
</div>

Typography

No issues found

Color

No issues found

Spacing

No issues found

Motion

No issues found

Craft

No issues found

Working well

The tooltip content on the toggle correctly branches on `allToolsOff`, `isGroupEnabled` to explain exactly what the toggle will do next ("Disable all tools in {groupName} group" vs "Enable all tools...") rather than a generic label. Naming the consequence in the tooltip is the right call for a control whose effect isn't obvious from a bare switch.
Using `cn()` to merge the computed margin/layout classes with the incoming `className` prop (`cn(\`${marginBottom} flex items-center justify-between\`, className)`) follows the conditional-class convention correctly instead of a template-literal ternary chain, so consumers can safely extend the header without fighting specificity.
The `handleClick` guard (checking `target.closest('button, input, ... [role="switch"]')` against `data-config-row`) is a thoughtful mitigation against double-firing the row's onClick when a nested control is clicked. It shows real awareness of the composition risk, even though the underlying nesting itself needs fixing.
The responsive split between a tab row for large screens and a popover select for small screens is the right call: tabs need horizontal room to stay scannable, and collapsing to a single popover trigger on mobile keeps the control usable at narrow widths instead of wrapping or truncating a tab strip.

Score your own repo with Rams.

Free on public repos. Rams reviews every pull request for accessibility, color, type, spacing, components, motion, UX, and craft, and posts inline fixes.

Review my public repo for free