Repository navigation
🪶 feat: Quiet Prompt Editor Boxes, Table Columns and Destructive Buttons - #16751
Conversation
e1e4618 to
fd045f8
Compare
fd045f8 to
7c38830
Compare
7c38830 to
2a39a54
Compare
2a39a54 to
416fc3a
Compare
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. |
416fc3a to
50388e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416fc3ac15
ℹ️ 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".
| html:not(.high-contrast) .markdown th { | ||
| border-left-color: rgb(from theme(colors.gray.200) r g b / var(--theme-border-inset-alpha, 1)); |
There was a problem hiding this comment.
Use the semantic border color for header rules
In every non-high-contrast multi-column markdown table, this higher-specificity rule overrides the later border-medium declaration with the fixed compatibility gray.200. In the stock dark theme that changes most header separators from #393939 to #e3e3e3, producing conspicuously bright lines, while custom themes cannot apply their configured border color. Apply the inset alpha to the semantic --border-medium role instead.
AGENTS.md reference: AGENTS.md:L142-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e759c3f: the column rule is border-medium at the inset share, no fixed gray remains.
| html:not(.high-contrast) .markdown th:last-child, | ||
| html:not(.high-contrast) .markdown td, | ||
| html:not(.high-contrast) .markdown td:last-child { |
There was a problem hiding this comment.
Preserve the table's left outer edge
When insetBorderAlpha is 0, as in ClickHouse, these selectors make the left border transparent on every body cell, including the first cell in each row; the preceding header selector does the same to the first header cell. Because those first-cell borders form the table's outer frame, the resulting table is open on the left rather than merely losing its internal column rules. Restrict the inset treatment to cells after the first, or explicitly restore the first-child edge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e759c3f: only cells after the first take the inset share, so the first cell keeps the outer left edge; the prompt-surfaces scenarios now assert it.
e759c3f to
327c8bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 327c8bb2cb
ℹ️ 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".
| {allowExecution === true && toolCalls && toolCalls.length > 0 && ( | ||
| <> | ||
| <div className="border-border-light bg-surface-code border-t p-4 text-xs"> | ||
| <div className="border-border-inset bg-surface-code border-t p-4 text-xs"> |
There was a problem hiding this comment.
Route the multi-result divider through the inset role
When code execution has more than one result, this newly inset output section is immediately followed by ResultSwitcher, whose root still uses border-border-light for its top border. Consequently, a theme such as ClickHouse with insetBorderAlpha: '0' still draws an internal rule at the bottom of the output, so the code block is only quiet for single-result executions. Change that divider to the inset border role as well and cover the multi-result state.
AGENTS.md reference: AGENTS.md:L142-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 877eb18: the result switcher divider draws border-border-inset, so a multi-result code block is quiet end to end.
| html:not(.high-contrast) .markdown :is(th, td):not(:first-child) { | ||
| border-left-color: rgb( | ||
| var(--border-medium) / calc(var(--border-medium-alpha, 1) * var(--theme-border-inset-alpha, 1)) | ||
| ); |
There was a problem hiding this comment.
Preserve the outer edge in RTL tables
For an RTL locale, the first DOM cell is laid out on the visual right, so this selector preserves that cell's left border as an internal divider while making the last cell's left border—the visual outer-left edge—transparent when insetBorderAlpha is 0. The fresh evidence beyond the earlier LTR edge fix is that client/src/locales/i18n.ts:285 sets document.documentElement.dir from the active locale, so this path is exercised by supported RTL languages; make the inset-cell selection direction-aware and test an RTL table.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 877eb18: the inset share is direction aware (not-first-child in LTR, not-last-child under html[dir=rtl]), and prompt-surfaces now asserts the outer edge and the column rule in RTL.
327c8bb to
877eb18
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877eb18d1c
ℹ️ 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".
| html:not(.high-contrast, [dir='rtl']) .markdown :is(th, td):not(:first-child), | ||
| html[dir='rtl']:not(.high-contrast) .markdown :is(th, td):not(:last-child) { |
There was a problem hiding this comment.
Use each table's resolved direction for border selection
When an LTR locale displays a message beginning with RTL text, Content/Container.tsx:6-10 resolves its dir="auto" container to RTL while <html> remains LTR. This selector consequently takes the LTR branch: the first DOM cell is visually rightmost, so its internal left rule stays opaque while the visual-left frame on the last cell becomes transparent when insetBorderAlpha is 0. Fresh evidence beyond the resolved RTL thread is the per-message dir="auto"; the added test only changes document.documentElement.dir and cannot exercise this supported mixed-direction path. Select cells using their resolved :dir(rtl) direction instead.
AGENTS.md reference: AGENTS.md:L144-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e1a5a64. Table edges are now drawn per cell in logical start/end terms, so a dir=auto RTL message under an LTR page is correct; the scenario probes a dir=rtl table under an LTR html.
| html[dir='rtl']:not(.high-contrast) .markdown :is(th, td):not(:last-child) { | ||
| border-left-color: rgb( | ||
| var(--border-medium) / calc(var(--border-medium-alpha, 1) * var(--theme-border-inset-alpha, 1)) | ||
| ); |
There was a problem hiding this comment.
Clear the remaining RTL right-side divider
With an RTL table and insetBorderAlpha: 0, the DOM-last cell is visually leftmost, so this rule correctly preserves its left outer edge, but the existing th/td:last-child rules at lines 1422-1434 still paint that same cell's physical right edge at full border-medium; that edge is the internal column divider. Fresh evidence beyond the earlier RTL fix is that the new scenario asserts only borderLeftColor, so it cannot detect this remaining opaque borderRightColor. Extend the RTL override to suppress that internal right edge while preserving the actual outer frame.
AGENTS.md reference: AGENTS.md:L144-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e1a5a64. Every side edge takes the inset share and only the first cell's start and last cell's end edge are restored whole, so the RTL last cell's inner right edge is cleared; the scenario now asserts it.
| <div className="border-border-inset-medium overflow-hidden rounded-xl border"> | ||
| <header className="border-border-inset-medium flex items-center justify-between border-b p-3"> |
There was a problem hiding this comment.
Scope the inset variable card styling to editor views
When the ClickHouse theme opens a prompt preview or read-only detail containing variables, this shared component is also rendered by PromptDetails.tsx:42, so the new inset classes make its outer frame and header separator transparent there too. The adjacent PromptTextCard and command display still use opaque border-medium frames, leaving only the variables section visually unbounded even though the stated change targets the editor's stacked boxes. Scope this appearance to editor call sites, or intentionally migrate and test the complete details layout as well.
AGENTS.md reference: AGENTS.md:L144-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e1a5a64. PromptVariables takes an inset prop that only the two editor forms pass; the details view keeps the opaque border-medium frame like its sibling cards.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc894ee31
ℹ️ 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".
| 'bg-button-primary text-text-inverted hover:bg-button-primary-hover hover:active:bg-surface-inverted-pressed', | ||
| destructive: | ||
| 'bg-surface-destructive text-text-on-status hover:bg-surface-destructive-hover', | ||
| 'bg-surface-destructive text-text-on-status hover:bg-surface-destructive-hover theme-destructive-soft:bg-surface-destructive/10 theme-destructive-soft:text-text-destructive theme-destructive-soft:hover:bg-surface-destructive/20 theme-destructive-soft:hover:active:bg-surface-destructive/30', |
There was a problem hiding this comment.
Preserve contrast in soft destructive states
In the ClickHouse light theme, labeled destructive buttons use text-destructive (#c10000) over a white surface. Compositing surface-destructive at 20% on hover lowers the contrast to about 4.38:1, and the 30% pressed state lowers it to about 3.54:1, both below the 4.5:1 requirement for these small button labels. Use state colors that keep the destructive text readable, and cover the hover and pressed states rather than testing only the resting tint.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 303b632: hover and pressed tints are now 14% and 17%, which keep the c10000 ink at 4.92:1 and 4.66:1 over white (was 4.38 and 3.54). Button.spec asserts both classes.
| html:not(.high-contrast) .markdown :is(th, td):dir(rtl):last-child { | ||
| border-right-color: rgb( | ||
| var(--border-medium) / calc(var(--border-medium-alpha, 1) * var(--theme-border-inset-alpha, 1)) | ||
| ); |
There was a problem hiding this comment.
Preserve the right edge of one-column RTL tables
With an RTL markdown table containing only one column and a theme such as ClickHouse where insetBorderAlpha is 0, the sole cell is also :last-child, so this rule makes its physical right border transparent even though that border is part of the outer frame rather than an internal divider. Fresh evidence beyond the earlier RTL-divider thread is the one-column case: the added scenarios always create two cells, so they do not exercise this selector when :first-child and :last-child are the same element. Exclude single-child rows from this override or restore the right edge for that case.
AGENTS.md reference: AGENTS.md:L144-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproducible on the current head: that selector was replaced in e1a5a64. Cells now set border-inline-start-color on :first-child and border-inline-end-color on :last-child, both at full share, so a single cell matches both and keeps both edges in LTR and RTL; no physical right-edge override remains.
Summary
The prompt editor, table and code-block screens in the ClickHouse theme still show borders Click UI does not draw: four same-weight boxes stack down the prompt editor, a markdown table is a full cell grid, a code block is outlined as well as filled, and the delete action is a solid red square. This follows the chrome and inset border roles with the remaining surfaces, and every change leaves the bundled themes as they were.
border-inset-mediumisborder-mediumat the inset share, so the description, command and variables boxes drop their edge in ClickHouse and keep it elsewhere. The markdown column rules and the code block's outer edge and header rule follow the same share. A newdestructiveStyleappearance role (fillby default,softin ClickHouse) lets thedestructiveButton paint a 10% tint of the destructive surface under the destructive ink, as Click UI's danger button does, through atheme-destructive-soft:variant. The filter toggle ("My agents") takes the control border so it matches the sort select beside it, which is the same colour in the bundled themes.Type of change
Testing
Tested environments/configuration:
interface.theme: clickhouseand the default theme, light and darkAutomated tests:
cd packages/client && npx jest src/theme src/components; new Button, registry, tokens and Click UI decision specse2e/specs/mock/scenarios/prompt-surfaces.spec.ts: default light/dark unchanged, ClickHouse light/dark, a theme that names the destructive style and the inset sharenpm run static-checks -- --against origin/berry-13/ch-theme-borderspassesScreenshots / recordings
No screenshots yet; the changes are covered by computed-style scenarios in the harness.
Risk / compatibility
Stacked on the chrome and inset border PR. The
destructiveButton is tinted for every use in ClickHouse, including confirmation dialogs, not only the prompt editor. The filter input beside the prompts list already takes the control border in the previous PR; it keeps a transparent fill because its floating label notches the field.Checklist
packages/client/src/theme/README.md)