feat: redesign ui - #48

packages/ui/src/components/alert.test.tsx-7-18 (1)

7-18: ⚠️ Potential issue | 🟡 Minor

Test intent says class forwarding, but class forwarding is not asserted.

Line 7 says this test verifies class forwarding, but the render call does not pass className, and no class assertion exists. Please either rename the test or assert class forwarding explicitly.

Suggested patch

-  it("renders role=alert by default and forwards class + data-variant", () => {
+  it("renders role=alert by default and forwards class + data-variant", () => {
     render(
-      <Alert data-testid="alert" variant="warning">
+      <Alert data-testid="alert" variant="warning" className="custom-alert">
         <AlertTitle>Heads up</AlertTitle>
         <AlertDescription>Restart required</AlertDescription>
       </Alert>
     );
     const alert = screen.getByTestId("alert");
     expect(alert).toHaveAttribute("role", "alert");
     expect(alert).toHaveAttribute("data-variant", "warning");
     expect(alert).toHaveAttribute("data-slot", "alert");
+    expect(alert).toHaveClass("custom-alert");
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/alert.test.tsx` around lines 7 - 18, The test
named "renders role=alert by default and forwards class + data-variant"
currently does not exercise class forwarding; update the test for the Alert
component by passing a className (e.g., className="foo") into the rendered
<Alert ...> and add an assertion that the rendered element (queried via
getByTestId("alert")) includes that class (e.g., toHaveClass("foo");
alternatively, if you prefer to keep the current assertions, rename the test to
remove "forwards class" from the title so it accurately reflects that it only
verifies role, data-variant, and data-slot for Alert with AlertTitle and
AlertDescription).

packages/ui/src/components/kind-chip.tsx-17-27 (1)

17-27: ⚠️ Potential issue | 🟡 Minor

Prevent overriding internal data-slot / data-kind markers.

Because {...props} is applied last, callers can override internal markers (e.g., data-slot, data-kind). If these are intended to be stable hooks, make internal attributes win.

Suggested fix

     <span
-      data-slot="kind-chip"
-      data-kind={kind}
+      {...props}
+      data-slot="kind-chip"
+      data-kind={kind}
       className={cn(
         "inline-flex items-center rounded-[var(--radius-chip)] px-1.5 py-0.5",
         "font-mono text-[10.5px] font-medium leading-[14px] lowercase whitespace-nowrap",
         "bg-[color:var(--color-accent-tint)] text-[color:var(--color-accent)]",
         className
       )}
-      {...props}
     >

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/kind-chip.tsx` around lines 17 - 27, The span in
kind-chip.tsx exposes internal markers data-slot and data-kind to be overridden
because {...props} is spread last; change the spread order or filter props so
internal attributes win: either spread {...props} before the explicit attributes
(data-slot="kind-chip", data-kind={kind}, className={cn(...)}), or destructure
and remove data-slot/data-kind from incoming props (e.g., const { "data-slot":
_, "data-kind": __, ...rest } = props) and then spread rest; update the span to
use the chosen approach so data-slot and data-kind (and the className computed
by cn) cannot be overwritten by callers.

packages/ui/src/components/chat-message-bubble.test.tsx-130-139 (1)

130-139: ⚠️ Potential issue | 🟡 Minor

Add align-override cases for non-user roles (or explicitly lock the API down)

Line 130 currently validates align override only for role="user". Since align is part of the public props, add coverage for agent/tool/diff (and clarify system behavior) so API intent is explicit.

✅ Suggested test expansion

   it("Should honour an explicit align override", () => {
@@
   });
+
+  it.each(["agent", "tool", "diff"] as const)(
+    "Should honour align override for role='%s'",
+    role => {
+      const { container } = render(
+        <ChatMessageBubble role={role} align="right">
+          override
+        </ChatMessageBubble>
+      );
+      const root = container.querySelector<HTMLElement>('[data-slot="chat-message"]');
+      expect(root?.getAttribute("data-align")).toBe("right");
+    }
+  );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/chat-message-bubble.test.tsx` around lines 130 -
139, The test only checks ChatMessageBubble align override for role="user"; add
additional test cases in chat-message-bubble.test.tsx that call
render(<ChatMessageBubble role="agent" align="left">...</ChatMessageBubble>),
role="tool", and role="diff" (and a note/test for role="system" if behavior
differs) and assert the same expectations on the slot element selected via
'[data-slot="chat-message"]' (getAttribute("data-align") === "left" and
className contains the expected alignment class like "justify-start" or the
appropriate class for your implementation) so the public align prop is
explicitly validated across roles, or alternatively adjust ChatMessageBubble
type/prop docs and tests to lock align usage to user-only if intended. Ensure
you reference the ChatMessageBubble component and the test file when adding
these assertions.

packages/ui/src/components/mono-badge.tsx-48-53 (1)

48-53: ⚠️ Potential issue | 🟡 Minor

Prevent overriding internal data attributes via trailing prop spread.

Placing {...props} last allows consumers to override data-slot/data-tone, which can desync the component’s internal contract and selectors.

Suggested fix

 function MonoBadge({ tone, uppercase, className, ...props }: MonoBadgeProps) {
   return (
     <span
+      {...props}
       data-slot="mono-badge"
       data-tone={tone ?? "default"}
       className={cn(monoBadgeVariants({ tone, uppercase }), className)}
-      {...props}
     />
   );
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/mono-badge.tsx` around lines 48 - 53, The span
currently spreads {...props} last which lets callers override internal data
attributes; change this to filter out data-slot and data-tone from the incoming
props before spreading so the component-controlled attributes win. Locate the
JSX span in mono-badge.tsx and replace the trailing {...props} usage with a
filtered rest (e.g., extract 'data-slot' and 'data-tone' from the incoming props
into throwaway variables and spread the remaining rest), keeping
data-slot="mono-badge" and data-tone={tone ?? "default"} as the explicit
attributes and still using cn(monoBadgeVariants({ tone, uppercase }), className)
for className.

packages/ui/src/components/code-block.tsx-35-51 (1)

35-51: ⚠️ Potential issue | 🟡 Minor

Reset copy feedback duration on repeated copy clicks.

On Line 47, clicking copy again while copied is already true does not retrigger the timeout in Lines 37-41, so the success state can clear too early relative to the latest click.

Proposed fix

 function CodeBlock({
@@
 }: CodeBlockProps) {
   const [copied, setCopied] = React.useState(false);
+  const copyResetTimerRef = React.useRef<ReturnType<typeof setTimeout> | null>(null);
 
   React.useEffect(() => {
-    if (!copied) return;
-    const timer = setTimeout(() => setCopied(false), COPY_FEEDBACK_MS);
-    return () => clearTimeout(timer);
-  }, [copied]);
+    return () => {
+      if (copyResetTimerRef.current) {
+        clearTimeout(copyResetTimerRef.current);
+      }
+    };
+  }, []);
@@
   const handleCopy = React.useCallback(async () => {
     if (typeof navigator === "undefined" || !navigator.clipboard?.writeText) return;
     try {
       await navigator.clipboard.writeText(code);
       setCopied(true);
+      if (copyResetTimerRef.current) {
+        clearTimeout(copyResetTimerRef.current);
+      }
+      copyResetTimerRef.current = setTimeout(() => {
+        setCopied(false);
+        copyResetTimerRef.current = null;
+      }, COPY_FEEDBACK_MS);
     } catch {
       // Silently ignore — some browsers block clipboard access in insecure contexts.
     }
   }, [code]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/code-block.tsx` around lines 35 - 51, The copy
success timeout doesn't reset if the user clicks copy again while copied is
already true, so modify handleCopy (and related logic around copied and
COPY_FEEDBACK_MS) to restart the feedback timeout on every successful copy: when
navigator.clipboard.writeText(code) resolves, ensure any existing timer is
cleared and a new timer is started (or alternatively bump a version/timestamp
state used in the useEffect dependency so the effect re-runs) so the success
state persists for COPY_FEEDBACK_MS after the most recent click; keep references
to the copied state, COPY_FEEDBACK_MS, the existing useEffect, and handleCopy
when implementing this fix.

packages/ui/src/components/avatar.test.tsx-26-35 (1)

26-35: ⚠️ Potential issue | 🟡 Minor

Image-slot test is asserting the wrong element.

The case says it verifies image rendering, but it checks avatar-fallback. This can miss real regressions in image-slot rendering.

✅ Suggested test fix

   it("Should render the image slot when provided", () => {
     const { container } = render(
       <Avatar>
         <AvatarImage src="https://example.test/me.png" alt="me" />
         <AvatarFallback>ME</AvatarFallback>
       </Avatar>
     );
-    const fallbackSlot = container.querySelector('[data-slot="avatar-fallback"]');
-    expect(fallbackSlot).not.toBeNull();
+    const imageSlot = container.querySelector('[data-slot="avatar-image"]');
+    expect(imageSlot).not.toBeNull();
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/avatar.test.tsx` around lines 26 - 35, The test
"Should render the image slot when provided" is asserting the wrong element;
update the assertion to check for the image slot instead of the fallback. In the
test that renders <Avatar> with <AvatarImage src=...> and <AvatarFallback>,
locate the element with the image slot (e.g.,
querySelector('[data-slot="avatar-image"]') or check for an <img> with src/alt)
and assert it is present; optionally also assert that the fallback slot
('avatar-fallback') is not used when the image is provided. This fixes the test
for Avatar/AvatarImage rendering.

packages/ui/src/components/popover.test.tsx-54-65 (1)

54-65: ⚠️ Potential issue | 🟡 Minor

Ensure console.error is always restored in the throw-path test.

Current mutation on Lines 55–64 can leak if the expectation fails before restoration. Wrap mutation in try/finally.

Suggested patch

   it("Should throw when PopoverContent is rendered outside <Popover>", () => {
     const originalError = console.error;
-    console.error = () => {};
-    expect(() =>
-      render(
-        <PopoverContent>
-          <PopoverTitle>orphan</PopoverTitle>
-        </PopoverContent>
-      )
-    ).toThrow(/Popover\.\* components must be used inside <Popover>/);
-    console.error = originalError;
+    try {
+      console.error = () => {};
+      expect(() =>
+        render(
+          <PopoverContent>
+            <PopoverTitle>orphan</PopoverTitle>
+          </PopoverContent>
+        )
+      ).toThrow(/Popover\.\* components must be used inside <Popover>/);
+    } finally {
+      console.error = originalError;
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/popover.test.tsx` around lines 54 - 65, The test
that silences console.error while asserting that PopoverContent (with
PopoverTitle) throws when rendered outside <Popover> should ensure console.error
is always restored on failure; wrap the mutation of console.error and the
render/assertion in a try/finally block so originalError is restored in the
finally clause, locating the change around the it(...) test that references
PopoverContent, PopoverTitle and console.error.

packages/ui/src/components/section.tsx-23-44 (1)

23-44: ⚠️ Potential issue | 🟡 Minor

Header visibility check treats null/false as present content.

Line 23 can render an empty header when consumers pass label={condition && ...} (false) or null. That creates unintended chrome (border + spacing).

Suggested patch

 function Section({ label, right, className, children, ...props }: SectionProps) {
+  const hasLabel = label !== null && label !== undefined && label !== false;
+  const hasRight = right !== null && right !== undefined && right !== false;
   return (
@@
-      {label !== undefined || right !== undefined ? (
+      {hasLabel || hasRight ? (
         <header
@@
-          {label !== undefined ? (
+          {hasLabel ? (
             <h2
@@
-          {right !== undefined ? (
+          {hasRight ? (
             <div data-slot="section-right" className="flex items-center gap-2">
               {right}
             </div>
           ) : null}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/section.tsx` around lines 23 - 44, The header
rendering currently checks `label !== undefined` and `right !== undefined`,
which treats null/false as present and can render an empty header; update the
visibility checks to treat null and boolean false as absent by using
nullish/boolean-aware checks (e.g., test `label != null && label !== false` and
`right != null && right !== false`) for the outer header condition and the inner
branches; adjust the conditions around the header element, the `h2`
(`section-label`) branch and the `div` (`section-right`) branch so the header
(data-slot="section-head") is only rendered when there is real content for
`label` or `right`.

packages/ui/src/components/metric.test.tsx-35-37 (1)

35-37: ⚠️ Potential issue | 🟡 Minor

Fix selector mismatch in the tone test block.

On Line 35, root queries metric-value instead of the root metric slot, so the assertion is not validating what the variable name implies.

✅ Suggested fix

-    const root = container.querySelector<HTMLElement>('[data-slot="metric-value"]');
+    const root = container.querySelector<HTMLElement>('[data-slot="metric"]');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/metric.test.tsx` around lines 35 - 37, The test's
selector is wrong: the variable root is querying '[data-slot="metric-value"]'
but should select the root slot '[data-slot="metric"]'; update the query for the
root variable in metric.test.tsx to use '[data-slot="metric"]' (and then use
that root variable for assertions about the root element's attributes, e.g. the
"data-tone" check) so the test name and assertions match the element being
inspected.

packages/ui/scripts/serve-storybook.ts-9-13 (1)

9-13: ⚠️ Potential issue | 🟡 Minor

Also verify the bundle root is a directory.

Lines 9-13 only check existence. If root points to a file, the server starts but cannot serve expected paths.

Proposed fix

-if (!existsSync(root)) {
+if (!existsSync(root) || !statSync(root).isDirectory()) {
   console.error(`[serve-storybook] missing Storybook bundle at ${root}`);
   console.error(`[serve-storybook] run 'bun run build:visual' before serving`);
   process.exit(1);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/scripts/serve-storybook.ts` around lines 9 - 13, The current
check only uses existsSync(root) which allows a file path to pass; update the
validation in serve-storybook.ts to also ensure root is a directory by using
fs.statSync or fs.lstatSync on root and calling isDirectory(); if the path
exists but is not a directory, log a clear error (e.g., "[serve-storybook]
storybook bundle root is not a directory: {root}") and call process.exit(1) —
keep the existing messages but add this directory check around the same block
that references existsSync(root).

packages/ui/src/components/direction.test.tsx (1)

12-19: Test name does not match behavior under test.

Line 12 says “default,” but Line 14 explicitly sets direction="ltr". Rename this test (or remove the prop) to reflect intent clearly.

Suggested wording fix

-  it("Should default descendants to ltr when wrapped in a DirectionProvider", () => {
+  it("Should forward ltr to descendants", () => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/direction.test.tsx` around lines 12 - 19, The test
title claims descendants "default" to ltr but the test explicitly passes a prop;
update the test to reflect its actual behavior by either removing
direction="ltr" from the render call to assert the default (using
<DirectionProvider><DirReader/></DirectionProvider>) or rename the it(...)
description to something like "Should provide explicit ltr direction to
descendants when wrapped in a DirectionProvider"; make the change in the test
that references DirectionProvider and DirReader so the name and implementation
match.

packages/ui/src/components/dropdown-menu.test.tsx (1)

56-107: Tighten mock assertions to catch duplicate/unexpected event emissions.

toHaveBeenCalledWith(...) can still pass if callbacks fire multiple times unexpectedly. Consider asserting call counts alongside payload checks.

Suggested assertion tightening

-    expect(handleClick).toHaveBeenCalled();
+    expect(handleClick).toHaveBeenCalledTimes(1);

-    expect(handleChange).toHaveBeenCalledWith(true, expect.anything());
+    expect(handleChange).toHaveBeenCalledTimes(1);
+    expect(handleChange).toHaveBeenCalledWith(true, expect.anything());

-    expect(handleChange).toHaveBeenCalledWith("warning", expect.anything());
+    expect(handleChange).toHaveBeenCalledTimes(1);
+    expect(handleChange).toHaveBeenCalledWith("warning", expect.anything());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/dropdown-menu.test.tsx` around lines 56 - 107, The
tests currently only assert payloads which can hide duplicate emissions; for
each mock callback (handleClick, the checkbox handleChange, and the radio
handleChange) add a call-count assertion such as
expect(mock).toHaveBeenCalledTimes(1) in addition to the existing
expect(...).toHaveBeenCalledWith(...) to ensure the handler fired exactly once
when interacting with DropdownMenuItem, DropdownMenuCheckboxItem, and
DropdownMenuRadioItem respectively; if reusing the same variable name for
multiple tests, ensure each test uses its own fresh vi.fn() so the call-count
assertion is meaningful.

packages/ui/src/components/kind-chip.test.tsx (1)

7-12: Test title is misleading vs asserted behavior.

Line 7 says “render the kind lowercase”, but Line 11 asserts "Greet" (original casing). Consider renaming the test to reflect that lowercase is CSS-driven (className) rather than text normalization.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/kind-chip.test.tsx` around lines 7 - 12, The test
title is inaccurate: it claims "lowercase" but the assertion expects the
original casing ("Greet"); update the test for KindChip to either (A) rename the
it(...) description to state that the component preserves original text casing
and applies mono font and accent tint tokens (e.g., "renders the kind with
original casing in mono with the accent tint tokens"), or (B) if the intention
is to assert lowercase text content, change the assertion to expect the
lowercased string and ensure the component transforms text accordingly; locate
the test referencing KindChip and the container query '[data-slot="kind-chip"]'
to make the edit.

packages/ui/src/components/pills.tsx (1)

131-132: Avoid redundant onChange calls for already-active items.

Even when value is unchanged, Line 131 still emits onChange, which can trigger unnecessary parent work/side effects.

Small guard to keep changes idempotent

-            onClick={() => onChange(item.value)}
+            onClick={() => {
+              if (!item.disabled && item.value !== value) onChange(item.value);
+            }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/pills.tsx` around lines 131 - 132, The onClick
handler currently always calls onChange(item.value) even when the item is
already active; change it to guard so it only invokes onChange when the value
would actually change. Modify the onClick in the pills component to something
like: if not isActive (and onChange is defined) then call onChange(item.value),
i.e. use a conditional/no-op when isActive to avoid redundant emissions while
keeping the pillToggleVariants({ active: isActive, size }) logic unchanged.

packages/ui/src/components/collapsible.tsx (1)

15-15: Export order alphabetized.

The export order was changed to alphabetical (CollapsibleContent before CollapsibleTrigger). While this doesn't affect functionality, consider whether alphabetical ordering is the preferred convention for exports across the component library.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/collapsible.tsx` at line 15, Export order changed
to alphabetical; revert or standardize exports to match the repository
convention (either grouped by primary component then helpers or alphabetized).
Update the export statement so the symbols appear in the agreed order
(Collapsible, CollapsibleTrigger, CollapsibleContent if keeping primary-first
grouping, or CollapsibleContent, CollapsibleTrigger, Collapsible if using
alphabetical) and ensure other component files follow the same pattern for
consistency with exports like Collapsible, CollapsibleContent,
CollapsibleTrigger.

packages/ui/src/components/collapsible.test.tsx (1)

45-56: Strengthen keepMounted assertion with closed-state visibility check.

Right now this test only proves DOM persistence. Add a “closed but not visible” expectation to catch regressions where content stays visually open.

Suggested test improvement

   await user.click(screen.getByRole("button", { name: "Toggle" }));
-  expect(screen.getByText("Body")).toBeInTheDocument();
+  const body = screen.getByText("Body");
+  expect(body).toBeInTheDocument();
+  expect(body).not.toBeVisible();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/collapsible.test.tsx` around lines 45 - 56, Update
the test for Collapsible/CollapsibleContent to not only assert the content
remains in the DOM when closed with keepMounted, but also assert it is not
visible after toggling: render using Collapsible, CollapsibleTrigger and
CollapsibleContent(keepMounted), click the trigger (the button with name
"Toggle") and replace or add the final assertion to check that the element
returned by screen.getByText("Body") exists but is not visible (use the
appropriate matcher such as toBeVisible or its negation) so the test fails if
the content remains visually open.

packages/ui/src/components/dialog.tsx (1)

30-63: Consider extracting shared “overlay root + motion context” wrapper logic.

The controlled/uncontrolled open handling + actionsRef + provider setup is very similar to the new Sheet (and likely Popover) implementation. A shared helper/hook would reduce drift and make future fixes safer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/dialog.tsx` around lines 30 - 63, Extract the
shared "overlay root + motion context" logic used in Dialog (actionsRef,
controlled/uncontrolled open handling, handleOpenChange, and
DialogMotionContext.Provider) into a reusable helper (either a hook like
useOverlayRootMotionContext or a wrapper component like OverlayRoot) so Sheet
and Popover can reuse it; move the state and handlers (actionsRef,
uncontrolledOpen/setUncontrolledOpen, isControlled, open resolution, and
handleOpenChange) and the memoized value into that helper and have Dialog call
it and then render DialogPrimitive.Root with the returned props plus the shared
provider value instead of duplicating the logic from Dialog (refer to Dialog,
handleOpenChange, actionsRef, DialogMotionContext, and DialogPrimitive.Root to
locate the code to extract).

packages/ui/src/components/sidebar.test.tsx (2)

163-171: Tighten this test to validate an actual breakpoint transition.

The current setup starts in narrow mode immediately. Consider rendering wide first, then using the mock’s fire(...) to simulate dropping below the breakpoint and asserting the state change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/sidebar.test.tsx` around lines 163 - 171, The test
currently renders Sidebar already in narrow mode; instead, render with a wide
viewport then simulate the media-query change to narrow and assert the
transition: use installMatchMedia to set initial query results that represent a
wide viewport, render <Sidebar nav={<span>nav</span>} collapsed={false}
collapseBreakpoint={800} />, verify the sidebar (query "[data-slot=sidebar]") is
initially not collapsed, then call the mock's fire(...) to simulate the viewport
dropping below the breakpoint and finally assert the sidebar has
data-state="collapsed" and data-narrow="true"; reference installMatchMedia, its
fire(...) helper, and the Sidebar component/collapseBreakpoint prop when making
these changes.

---

91-99: Re-querying nodes after rerender is a best practice but the stated concern is not applicable here.

The rail and panel elements are never conditionally rendered or keyed in a way that causes remounting when the collapsed prop changes. Both elements persist in the DOM across rerenders—only their properties (width, aria-hidden) change. References captured before rerender remain valid.

That said, re-querying after rerender follows React Testing Library best practices of testing current DOM state rather than relying on captured references, which improves test resilience to implementation changes. However, this is a stylistic improvement, not a correctness issue.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/sidebar.test.tsx` around lines 91 - 99, Update the
test in packages/ui/src/components/sidebar.test.tsx to re-query the DOM after
calling rerender instead of reusing the earlier captured reference: after
calling rerender(<Sidebar ... collapsed={true} />) call
container.querySelector<HTMLElement>("[data-slot=sidebar-rail]") again and
assert on that new rail reference (and do the same for the panel if applicable)
so the test uses current DOM nodes for the Sidebar component checks.

packages/ui/src/components/command.tsx (1)

57-62: Hide decorative search icon from assistive technologies.

Line 61 renders a visual-only icon; add aria-hidden so it isn’t announced redundantly.

Suggested patch

-        <SearchIcon className="size-4 shrink-0 opacity-50" />
+        <SearchIcon aria-hidden="true" className="size-4 shrink-0 opacity-50" />

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/command.tsx` around lines 57 - 62, The SearchIcon
rendered inside the command input group is decorative and should be hidden from
screen readers; update the JSX where SearchIcon is used (the SearchIcon element
next to CommandPrimitive.Input) to include aria-hidden="true" so it isn't
announced redundantly by assistive technologies.

packages/ui/src/components/connection-indicator.tsx (1)

35-40: Consider status semantics for live connection changes.

For dynamic status updates, adding role="status" and aria-live="polite" on the container improves screen-reader announcements.

Suggested patch

     <div
       data-slot="connection-indicator"
       data-status={status}
+      role="status"
+      aria-live="polite"
       className={cn("inline-flex items-center gap-2", className)}
       {...props}
     >

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/connection-indicator.tsx` around lines 35 - 40,
The connection-indicator container (the div with
data-slot="connection-indicator" and data-status={status} in the
ConnectionIndicator component) should expose live-region semantics so screen
readers announce dynamic status changes; add role="status" and
aria-live="polite" attributes to that container (alongside the existing
data-slot, data-status, className and spread {...props}) to enable polite
announcements of status updates.

packages/ui/src/components/page-header.tsx (1)

66-71: Optionally avoid rendering an empty meta container.

On Line 66-Line 71, page-header-meta is rendered even when meta is not provided. Conditionally rendering this block can keep DOM output leaner and avoid layout/debug noise.

♻️ Proposed refactor

-      <div
-        data-slot="page-header-meta"
-        className="ml-auto flex shrink-0 items-center gap-2 text-[13px] text-[color:var(--color-text-secondary)]"
-      >
-        {meta}
-      </div>
+      {meta ? (
+        <div
+          data-slot="page-header-meta"
+          className="ml-auto flex shrink-0 items-center gap-2 text-[13px] text-[color:var(--color-text-secondary)]"
+        >
+          {meta}
+        </div>
+      ) : null}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/page-header.tsx` around lines 66 - 71, The
page-header currently always renders the meta container
(data-slot="page-header-meta") even when the prop/variable meta is empty; update
the PageHeader component to conditionally render that div only when meta is
non-empty/truthy (check the meta prop/variable) so the DOM omits the container
when there's no meta content, preserving the same className and attributes when
rendered.

packages/ui/src/components/sonner.test.tsx (1)

9-10: Replace document.querySelector with semantic RTL queries.

screen.getByRole aligns with React Testing Library best practices by making tests more robust against implementation changes and reflecting how users (including those using assistive technology) interact with the app.

Suggested refactor

-import { act, render, waitFor } from "@testing-library/react";
+import { act, render, screen, waitFor } from "@testing-library/react";
@@
-    const region = document.querySelector("section[aria-label^='Notifications']");
-    expect(region).not.toBeNull();
+    expect(
+      screen.getByRole("region", { name: /notifications/i }),
+    ).toBeTruthy();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/sonner.test.tsx` around lines 9 - 10, The test
currently uses document.querySelector to find the notifications region (the
region variable); replace that with a semantic RTL query such as
screen.getByRole('region', { name: /^Notifications/i }) (or getByLabelText /
getByAccessibleName if you prefer) and assert with RTL matchers (e.g.
toBeInTheDocument) instead of checking for not.toBeNull; update imports to use
screen and the RTL matchers if needed and remove the document.querySelector
usage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/ui/src/components/dialog.test.tsx (1)

93-104: ⚠️ Potential issue | 🟠 Major

Ensure console.error is always restored.

Line 95 and Line 103 restore logic is not exception-safe. If the assertion path fails early, the mock leaks into other tests.

🛠️ Suggested fix

   it("Should throw when DialogContent is rendered outside <Dialog>", () => {
     const originalError = console.error;
-    console.error = () => {};
-    expect(() =>
-      render(
-        <DialogContent>
-          <DialogTitle>orphan</DialogTitle>
-        </DialogContent>
-      )
-    ).toThrow(/Dialog\.\* components must be used inside <Dialog>/);
-    console.error = originalError;
+    try {
+      console.error = () => {};
+      expect(() =>
+        render(
+          <DialogContent>
+            <DialogTitle>orphan</DialogTitle>
+          </DialogContent>
+        )
+      ).toThrow(/Dialog\.\* components must be used inside <Dialog>/);
+    } finally {
+      console.error = originalError;
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/dialog.test.tsx` around lines 93 - 104, The test
"Should throw when DialogContent is rendered outside <Dialog>" currently
overrides console.error by assigning to a local originalError and resetting it
after the expect, which is not exception-safe; wrap the override and test
assertion in a try/finally so console.error is restored in finally (use
originalError in finally) to guarantee restoration even if render or expect
throws; update the test that references console.error and the render of
<DialogContent> / <DialogTitle> accordingly.

packages/ui/src/components/dialog.tsx (1)

175-177: Use DialogClose wrapper here for slot consistency.

This path currently uses DialogPrimitive.Close directly, which skips your local wrapper conventions (data-slot="dialog-close"). Prefer reusing DialogClose for consistent markup and selectors.

♻️ Suggested refactor

-      {showCloseButton ? (
-        <DialogPrimitive.Close render={<Button variant="outline" />}>Close</DialogPrimitive.Close>
-      ) : null}
+      {showCloseButton ? (
+        <DialogClose render={<Button variant="outline" />}>Close</DialogClose>
+      ) : null}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/dialog.tsx` around lines 175 - 177, The close
button currently uses DialogPrimitive.Close directly which bypasses our local
wrapper conventions; replace the direct usage of DialogPrimitive.Close with the
local DialogClose wrapper so the element includes the expected data-slot and
wrapper behavior — locate the JSX branch checking showCloseButton and swap
DialogPrimitive.Close to DialogClose while keeping the inner Button
(variant="outline") and text "Close" unchanged to preserve functionality and
styling.

packages/ui/src/components/pills.tsx (1)

127-129: Add a defensive disabled guard in the click handler.

Right now Line 129 relies on native disabled-button behavior to suppress onChange. Adding an explicit guard makes intent resilient to future refactors.

♻️ Suggested hardening

-            onClick={() => onChange(item.value)}
+            onClick={() => {
+              if (item.disabled) return;
+              onChange(item.value);
+            }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/components/pills.tsx` around lines 127 - 129, The click
handler for the pill relies on native disabled behavior and should defensively
guard against calling onChange when the item is disabled; update the onClick
arrow handler in the pills component so it first checks item.disabled (and
optionally that onChange is defined) and returns early if disabled, otherwise
calls onChange(item.value) — locate the onClick prop on the element that
currently reads onClick={() => onChange(item.value)} and add the guard there.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes