fix(Dialog): Use Radix Dialog Primitives for Accessibility

[dustinhealy]

Why?

Dialog.Content has two accessibility issues:

1. Renders the title prop as a styled.h2 instead of Radix's DialogTitle primitive, causing: DialogContent requires a DialogTitle
2. No way to provide a dialog description, causing: Missing Description or aria-describedby

How?

• Changed Title from styled.h2 to styled(RadixDialog.Title)
• Added optional description prop that renders as RadixDialog.Description
• Existing tests pass

Tickets?

• ClickHouse/librechat-admin-panel#13

Contribution checklist?

• You've done enough research before writing
• You have reviewed the PR

[changeset-bot]

🦋 Changeset detected

Latest commit: 0036a0e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@clickhouse/click-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Claude finished @dustinhealy's task in 45s —— View job

---

PR Review: fix(Dialog): Use Radix DialogTitle Primitive for Accessibility

• Gathering context
• Reading changed files
• Reviewing conventions
• Posting review

---

The change is correct and minimal — swapping styled.h2 to styled(RadixDialog.Title) is exactly the right fix. Radix's DialogTitle renders as h2 by default, so the visual output is identical and the accessibility warning is suppressed when a title is provided.

🚨 Must Fix

• Changeset missing: Per the repo's conventions (CONVENTIONS.md → Changesets), bug fixes require a patch changeset before merging. The changeset bot already flagged this. Add one via this link (pre-filled by the changeset bot).

♿ Accessibility — informational (pre-existing, not introduced by this PR)

• Titleless dialogs: When title is omitted, RadixDialog.Content still has no DialogTitle, so the Radix warning persists for that usage pattern. Consider accepting an aria-label fallback or rendering a visually-hidden DialogTitle for untitled dialogs. This is out of scope for this PR but worth a follow-up issue.

Everything else looks good. The fix is clean, non-breaking, and the existing test suite covers the title rendering path. 🙌

[github-actions]

Claude finished @dustinhealy's task in 1m 19s —— View job

---

PR Review: fix(Dialog): Use Radix DialogTitle Primitive for Accessibility

The DialogTitle swap is exactly right — clean, minimal, no visual regression. The second commit adding description is a welcome a11y improvement, but has a few rough edges worth addressing.

---

🚨 Must Fix

• Changeset missing: Both commits touch the public API (new description prop) and fix a bug — a patch changeset is required before merging per CONVENTIONS.md. Add one here.

• RadixDialog.Description is unstyled (Dialog.tsx:159): Title is wrapped in styled(RadixDialog.Title) with theme typography tokens, but Description is rendered as a bare Radix primitive with no styling. It will inherit whatever the parent's default font is — inconsistent with the rest of the component. Mirror the pattern used for Title:

  const Description = styled(RadixDialog.Description)`
    font: ${({ theme }) => theme.click.dialog.typography.description.default};
    // or whatever token fits — color.text.muted etc.
  `;

  Fix this →

💡 Suggestions (non-blocking)

• Test coverage (Dialog.test.tsx): No test covers the new description prop. Per CONVENTIONS.md, new behaviors should be tested. A simple case — render with description="Some description", open the dialog, assert the text is present — would complete the coverage.

• description type (Dialog.types.ts:19): description?: string works for the common case, but ReactNode would be more composable (consistent with how children is typed) and avoids callers needing to stringify rich content.

• Storybook: The description prop isn't wired into the DialogExample args or any story. Per CONVENTIONS.md, stories should cover all component states — adding description to ModalDialog's args would cover it with no extra story needed.

---

Everything else looks great — the core fix is clean and the description addition is a genuinely useful a11y improvement. 🙌

[github-actions]

Claude finished @dustinhealy's task in 1m 26s —— View job

---

PR Review: fix(Dialog): Use Radix Dialog Primitives for Accessibility

• Gathering context
• Reading changed files
• Reviewing conventions
• Posting review

---

The DialogTitle swap is correct and the changeset is now in place. One remaining must-fix before merge — the description renders as a bare primitive despite the theme already having dedicated tokens for it.

---

🚨 Must Fix

• RadixDialog.Description unstyled (Dialog.tsx:160): The theme already defines theme.click.dialog.typography.description.default and theme.click.dialog.color.description.default specifically for this. Using a bare primitive ignores both tokens and violates CONVENTIONS.md ("ALWAYS use theme tokens"). Mirror the Title pattern:

  const Description = styled(RadixDialog.Description)`
    font: ${({ theme }) => theme.click.dialog.typography.description.default};
    color: ${({ theme }) => theme.click.dialog.color.description.default};
    padding: 0;
    margin: 0;
  `;

  Then replace the bare <RadixDialog.Description> at line 160 with <Description>.

  Fix this →

💡 Suggestions (non-blocking)

• description type (Dialog.types.ts:19): string works but ReactNode is more composable (lets callers pass <strong> or <Link> nodes) and aligns with CONVENTIONS.md's note against restricting content props. Note: if changed, also update DialogExample in stories to pass ReactNode.

• Test coverage (Dialog.test.tsx): No test for the new description prop. A simple case — render with description="Some description", open the dialog, assert the text is present — would complete coverage per CONVENTIONS.md.

• Storybook (Dialog.stories.tsx): description is not wired into DialogExampleProps or ModalDialog's args. Adding it there would cover the new state without needing a separate story.

---

The core accessibility fix is exactly right. Once the Description is wrapped in a styled component with the theme tokens, this is ready to merge. 🙌

[dustinhealy]

@claude re-review

[github-actions]

Claude finished @dustinhealy's task in 1m 0s —— View job

---

PR Review: fix(Dialog): Use Radix Dialog Primitives for Accessibility

• Gathering context
• Reading changed files
• Reviewing conventions
• Posting review

---

The changeset is in place and the DialogTitle swap is correct. One must-fix remains from the previous review — the Description is still a bare unstyled primitive, and the theme already has dedicated tokens for it.

---

🚨 Must Fix

• RadixDialog.Description unstyled (Dialog.tsx:159): The theme defines theme.click.dialog.typography.description.default and theme.click.dialog.color.description.default — both confirmed present in the tokens file — but the bare <RadixDialog.Description> ignores both. Mirror the Title pattern:

  const Description = styled(RadixDialog.Description)`
    font: ${({ theme }) => theme.click.dialog.typography.description.default};
    color: ${({ theme }) => theme.click.dialog.color.description.default};
    padding: 0;
    margin: 0;
  `;

  Then replace <RadixDialog.Description> at line 159 with <Description>.

  Fix this →

💡 Suggestions (non-blocking)

• description type (Dialog.types.ts:19): string works but ReactNode is more composable — lets callers pass <strong> nodes or links without stringifying — and is consistent with how children and similar props are typed in this repo.

• Test coverage (Dialog.test.tsx): No test for the new description prop. A minimal case — render with description="Some description", open the dialog, assert the text appears — would complete coverage.

• Storybook (Dialog.stories.tsx): description isn't wired into any story args. Adding it to ModalDialog's args would cover the new state without needing a separate story.

---

Once the Description is wrapped in a styled component with the theme tokens, this is ready to merge. 🙌

[github-actions]

Claude finished @dustinhealy's task in 1m 2s —— View job

---

PR Review: fix(Dialog): Use Radix Dialog Primitives for Accessibility

• Gathering context
• Reading changed files
• Reviewing conventions
• Posting review

---

All three must-fix items from previous reviews are now resolved. This is clean and ready to merge. 🙌

✅ Previously flagged issues — all fixed

• Title → styled(RadixDialog.Title): correct, eliminates the DialogContent requires a DialogTitle warning
• Description → styled(RadixDialog.Description) with theme.click.dialog.typography.description.default + color.description.default: properly themed, no bare primitives
• Changeset added at patch level with a clear description

💡 Suggestions (non-blocking, carry-over from prior reviews)

• description type (Dialog.types.ts:19): string works fine, but ReactNode would be more composable if callers ever need to pass rich content (links, <strong>, etc.)
• Test coverage (Dialog.test.tsx): No test for the new description prop — a simple render + assert would complete coverage
• Storybook (Dialog.stories.tsx): description isn't wired into any story args yet

None of these block merge — the core fix is exactly right. Nice work, @dustinhealy!

[punkbit]

This is good, thanks @dustinhealy

Before merging to main, let's approve/merge the following #974 into yours. It adds a new story and shows before/after example in the changeset to make it clear and incentivise usage as you suggestion seem to be better, less boilerplate.

[github-actions]

Claude finished @punkbit's task in 1m 43s —— View job

---

PR Review: fix(Dialog): Use Radix Dialog Primitives for Accessibility

• Gathering context
• Reading changed files
• Reviewing conventions
• Posting review

---

All previous must-fix items are resolved — the DialogTitle swap is correct, Description uses theme tokens, and the changeset is well-written with before/after examples. This is nearly ready to merge.

⚠️ Pending blocker (from reviewer @punkbit)

PR #974 needs to be merged into this branch first, per @punkbit's review. That PR adds a new story and cleans up the changeset. Once #974 merges into main (or directly into this branch), this PR can land cleanly.

💡 Suggestions (non-blocking, carry-over)

• description type (Dialog.types.ts:19): string works, but ReactNode is more composable — lets callers pass <strong> or <Link> without stringifying — and aligns with CONVENTIONS.md's note against restricting content props. Fix this →

• Test coverage (Dialog.test.tsx): No test for the new description prop. A simple case — click trigger, assert getByText('Some description') is present — would complete coverage per CONVENTIONS.md. Fix this →

• WithDescription story (Dialog.stories.tsx:105): The story uses a raw render() rather than wiring description into DialogExampleProps + ModalDialog args. Either approach is fine — the story does demonstrate the feature clearly as-is.

---

The core fix is exactly right. Once #974 is resolved, this is ready to merge. Nice work, @dustinhealy! 🙌

[punkbit]

@dustinhealy Merged #974 test it out, if happy feel free to merge. Thank you!

[workflow-authentication-public]

📚 Storybook Preview Deployed

✅ Preview URL: https://click-i1iv0h4ab-clickhouse.vercel.app

Built from commit: fe88b53c40b7379ff589be1e69364e947fa18474