Repository navigation
🩹 fix: Keep Chrome Surfaces and Destructive Confirms Legible in Quiet-Chrome Themes - #16833
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: 5ee65f9e8a
ℹ️ 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".
| destructive: | ||
| 'bg-surface-destructive text-text-on-status hover:bg-surface-destructive-hover', |
There was a problem hiding this comment.
When destructiveStyle is soft (as in the ClickHouse theme), removing the theme classes from destructive makes every unconverted caller solid. The migration misses several clear inline dialog triggers, including Nav/SettingsTabs/ApiKeys/Item.tsx:60, Nav/SettingsTabs/Data/RevokeKeys.tsx:45, Nav/SettingsTabs/Data/DeleteCache.tsx:50, and Skills/dialogs/DeleteSkill.tsx:65; these now look like dominant confirmation buttons before the confirmation dialog is opened, contrary to the new inline-versus-confirm contract. Convert the remaining inline delete/revoke triggers to destructive-soft while leaving their confirmation actions as destructive.
Useful? React with 👍 / 👎.
| @custom-variant theme-chrome-quiet { | ||
| @container style(--theme-border-chrome-alpha: 0) { |
There was a problem hiding this comment.
A custom theme can validly specify chromeBorderAlpha as .0, 0.0, or 0.00 because packages/data-provider/src/theme.ts:isOpacity accepts those forms, and applyResolvedTheme writes the string verbatim. The border calculation still treats each as numeric zero, but this unregistered-custom-property style query matches only the literal token value 0, so the header and floating-control compensations do not activate for those valid quiet-chrome themes. Normalize zero opacity values before applying them or query a separate canonical appearance flag.
AGENTS.md reference: AGENTS.md:L147-L151
Useful? React with 👍 / 👎.
5ee65f9 to
bef3482
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bef3482e48
ℹ️ 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".
| destructive: | ||
| 'bg-surface-destructive text-text-on-status hover:bg-surface-destructive-hover', |
There was a problem hiding this comment.
When 2FA is enabled, DisableTwoFactorToggle.tsx:26 still selects destructive, although that button only opens two-factor-authentication-dialog; the actual confirmation uses destructive in DisablePhase.tsx:64-67. Removing the soft-theme classes here therefore makes the ClickHouse trigger solid and as visually dominant as its confirmation. Fresh evidence beyond the earlier review thread is this conditional variant caller, which the literal variant="destructive" migration missed; use destructive-soft for its enabled arm.
AGENTS.md reference: AGENTS.md:L144-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b753f24: the enabled arm of DisableTwoFactorToggle uses destructive-soft; a grep of destructive string literals outside the variant attribute finds no other caller. Precheck and scenarios pass at this head.
…over the Quiet Surfaces
bef3482 to
b753f24
Compare
Summary
Since #16741 (chrome borders off), #16751 (soft destructive) and #16736/#16739 (ghost header buttons), the ClickHouse theme has four chrome regressions. The scroll-to-bottom chip lost its outline and became a bare chevron over the thread. The chat header bar is a fading gradient, so scrolled text bleeds between the now-ghost header buttons. The delete-chat confirm and every other dialog confirm turned into the soft tint, weaker than Cancel. The sidebar search field had a fill but no stroke next to stroked selects.
A new
theme-chrome-quiet:variant fires whenchromeBorderAlphais0. The scroll chip moves to afloatingButton variant that takes an opaque fill and a shadow under that variant, and the header bar goes opaque under it.destructiveis now always the solid filled button; the soft tint lives on a newdestructive-softvariant used by the eight inline triggers (row deletes, revoke, clear). The sidebar search takes the control stroke under the existingtheme-field-fill:variant. Default and high-contrast themes are unchanged.Not touched: the truncated "Steer" select is the real label since #16759 (
com_ui_steer), and the Inline/Prompt toggle in the agent builder is a layout change unrelated to themes. The share control tile edge no longer shows once the header bar is opaque.Type of change
Testing
Tested environments/configuration: Chromium 1440x900 and 390x844,
interface.theme: clickhouseand the default theme, light and dark, against this branch and dev (abed1a2).Automated tests:
cd packages/client && npx jest src/components/Button.spec.tsx(26 passed, new cases forfloatinganddestructive/destructive-soft)cd client && npx jest --findRelatedTestson the ten touched components (552 suites, 7637 tests passed)npx tsc --noEmitinpackages/clientandclient,npm run static-checks -- --against origin/dev(all passed)Screenshots / recordings
Before on the left, after on the right, ClickHouse theme.
Risks
destructiveButtons that were relying on the soft tint in the ClickHouse theme (the eight inline callers) were moved todestructive-soft; any new inline destructive control should use it.Checklist