Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e352040fea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'rgb-status-success-strong', | ||
| 'rgb-text-on-status', | ||
| 'rgb-surface-dialog', | ||
| 'rgb-dialog-title', |
There was a problem hiding this comment.
When a partial theme defines only the new rgb-dialog-title role and omits rgb-status-verified, this entry makes ownsMarkSurroundings true, causing both resolveTheme and the legacy applyTheme adapter to replace the bundled verified blue with rgb-status-success-strong. The dialog title is unrelated to the ToolCard surfaces against which the verified mark is measured, so merely customizing dialog typography unexpectedly changes native-tool badges and removes their deliberate distinction from the green selected-state badge.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
| 'font-theme-dialog-title font-theme-dialog-title-weight text-dialog-title text-(length:--theme-dialog-title-size) leading-(--theme-dialog-title-leading) tracking-tight', | ||
| focusOutlineVariants({ focusOutline }), | ||
| className, |
There was a problem hiding this comment.
Because the caller-provided className is merged last, existing titles such as McpOAuthDialog.tsx:67, KeyboardShortcutsDialog.tsx:381, and SkillsDialog.tsx:247 retain text-text-primary, which Tailwind Merge replaces text-dialog-title with. Consequently, themes that deliberately make rgb-dialog-title differ from body ink affect only some dialogs, while these visible dialogs silently ignore the new role; the redundant per-feature color classes need to be removed or migrated as part of introducing the shared semantic role.
AGENTS.md reference: AGENTS.md:L92-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 059b4ea: the seven titles (McpOAuthDialog, KeyboardShortcutsDialog, SkillsDialog, AssistantToolsDialog, ActionsInput, Avatar, ItemDialogHeader) drop text-text-primary, so text-dialog-title reaches them; MermaidDialog keeps its deliberate text-text-secondary toolbar title. The role defaults to text-primary, so nothing changes by default; related client tests pass and the design-rule counts were pruned.
e352040 to
f6827d3
Compare
d9beab2 to
059b4ea
Compare
059b4ea to
c40baa6
Compare
…cy Dialog Action A stylesheet that repainted --surface-inverted or its hover no longer reached primary buttons, because the stock CSS declared their fills from palette steps. They now read the inverted surface they split from. A templated dialog's legacy selection action kept a fixed h-10 and the inverted fill beside the shared cancel Button, so a theme's button height split the two. It now reads the Button's height and primary fill roles.
A templated dialog cleared its edge with border-none and set its title at a fixed text-lg, semibold, display-family heading in the body ink, so a theme could not draw Click UI's framed dialog or its 700 1.25rem/1.5 title. OGDialog now reads a stroke width (in border-light), its inline padding and header gap, and its title's size, leading, weight, family and ink from theme roles. Every default reproduces today's dialog; the title size, family and ink follow textLg, displayFontFamily and text-primary for a theme that names only those. The ClickHouse theme takes Click UI's dialog stroke, space.x, title gap, title typography and title color, the drift guard pins them, and the parity probes now read the dialog stroke and scrim.
The dialog title color was added to the surfaces the verified mark is measured against, so a theme that named only the title ink took the verified fallback. It is a text color, not a surface the mark sits on.
Seven dialogs set text-text-primary on their OGDialogTitle, which Tailwind Merge kept over the new dialog-title role, so a theme that gives dialog titles their own ink reached only some of them. The redundant classes are gone; the role defaults to the primary ink, so nothing changes by default, and the design-rule counts for those files drop by the override each removed.
c40baa6 to
bcdabad
Compare
This branch has not been deployed
1 participant
Summary
OGDialogTemplatecleared the dialog edge withborder-none, andOGDialogTitleset a fixedtext-lg, semibold, display-family heading in the inherited body ink. A theme could therefore not draw Click UI's framed dialog, its2reminline padding, or its700 1.25rem/1.5title, and the ClickHouse theme showed an unframed dialog with an 18px semibold title.Every non-bare
OGDialogContentnow reads its edge stroke width (painted inborder-light), inline padding and title-to-description gap from appearance roles (dialogStroke,dialogPaddingX,dialogHeaderGap), and its title readsdialogTitleSize,dialogTitleLeading,dialogTitleFontWeight,dialogTitleFontFamilyand adialog-titlecolor role. The template no longer clears the edge, and its legacy confirm button uses the Button's primary fill and height roles. Every default reproduces today's dialog, so the default and high-contrast themes are unchanged; a theme that names onlytextLg,displayFontFamilyortext-primarykeeps its dialog titles on them. The scrims already read the scrim roles, and the ClickHouse dark scrim keeps the black overlay canary documents.The ClickHouse theme takes Click UI's
dialog.stroke.default,dialog.space.x,dialog.title.space.gap,dialog.typography.title.defaultanddialog.color.title.default. The drift guard pins them, and its parity probes now read the dialog stroke and the scrim utilities: dialog decisions match Click UI in both modes except the documented dark scrim, and the floors rise to 13/12 color and 14 shape.It also carries two fixes for review findings on #16530, which merged before they landed: the stock
--button-primaryfills now readvar(--surface-inverted)and its hover, so a stylesheet that repaints only the inverted surface still reaches primary buttons, and the template's legacyselectionaction takes the Button's height and primary fill roles instead of a fixedh-10beside the themed cancel button. Seven dialogs that pinnedtext-text-primaryon their title drop it, so the new title ink reaches them.Closes berry-13#138
Type of change
Testing
Tested environments/configuration:
fontFamily,textLgandrgb-text-primaryAutomated tests:
packages/client:npx jest src/theme src/components src/utils(all suites pass), including a deliberately different reference theme (3px stroke, 3rem padding, a 2rem/1.2 800-weight serif title in magenta), the chained inheritance of the title family through the display role, the validators for every new role, and dialog title contrast on the ClickHouse dialog surfaceOGDialogTemplate.spec.tsx: the template keeps the themed edge, the title reads its roles, and a caller's own title size, weight and padding still winButton.spec.tsx: a templated dialog's legacy confirm action reads the Button's height and fill roles;defaults.spec.ts: split color roles are declared as the role they split from, in both modesclient: related tests for the seven dialogs whose title override was removed (181 passed); their design-rule counts were prunede2e/specs/mock/scenarios/dialog-chrome.spec.ts: default dialog unchanged, ClickHouse dialogs at 1px / 32px with a 20px/30px 700 Inter title in#1e1d1f/#f9f9f9, and a legacy type themeScreenshots / recordings
ClickHouse theme, the "Confirm Archive" dialog (a plain
OGDialogTemplate): Click UI's 1px stroke, 2rem inline padding and 700 1.25rem/1.5 title replace the unframed dialog and its 18px semibold title. The default and high-contrast themes are unchanged.Risk / compatibility
OGDialogTitlenow paints its ink (dialog-title, which defaults totext-primary) instead of inheriting it, so a title placed inside a differently colored container takes the primary ink; noOGDialogContentorOGDialogHeaderin the app sets another ink for its title to inherit. Callers that pass their own padding, title size or weight keep them through Tailwind Merge. The title keeps LibreChat'stracking-tight. An older reader ignores the new appearance keys and color, as it does any token it does not know.Checklist