fix(mcp): use ActionButton for install/update modal buttons (#10517)#10586
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR updates the custom Install and Update accent-button labels to choose the icon, border, and text color against accent_button_color(), which matches the default accent button contrast behavior. I did not find code-level correctness or security issues in the changed hunks.
Concerns
- This is a user-visible UI styling change, but the PR description does not include an attached screenshot or recording; the Screenshots / Videos section only says one will be attached later. Manual testing is required for changes that can be manually tested. Please include screenshots or a screen recording that show it working end to end, or justify why manual testing is not possible.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates the MCP installation and update modal custom accent-button labels to derive icon, border, and text colors from the accent button background instead of the surrounding modal surface.
Concerns
No correctness or security concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR updates the custom Install/Update accent-button labels to use text color selected against the accent button background. The enabled install path is consistent with the standard accent button foreground.
Concerns
- The update modal reuses the accent-background label color even when the Update button is disabled after all updates are deselected; because custom labels bypass the button's disabled font color, the disabled state can render with enabled/accent foreground on a disabled surface.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // The icon and label sit on the accent-button background, so colors | ||
| // must be picked for contrast against that background rather than the | ||
| // surrounding surface — see issue #10517. | ||
| let accent_label_color = appearance |
There was a problem hiding this comment.
update_button_builder.disabled() runs below; custom labels do not inherit the button's disabled font color, so deselecting all updates can render an enabled/accent-looking label on the disabled surface. Choose the label/icon/border color from the disabled button background when has_selection is false, or use a label path that lets the button apply state colors.
…elected Address oz-for-oss review on warpdotdev#10586. When the Update button is disabled (`if !has_selection { ... .disabled() }`), the button surface flips to `surface_3` per `disabled_button_styles` in `warp_core::ui::builder`, but the custom label color was still being computed against the enabled accent-button background — so the icon and text rendered with the wrong contrast on the disabled grey surface. Compute `has_selection` before constructing the label, and branch the custom label color to `disabled_text_color(surface_3())` in the disabled state. Install modal is unaffected (its accent button has no disabled state).
|
Addressed the disabled-state regression — when no rows are selected, the custom label now uses |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the custom labels for the MCP install/update modal accent buttons so their text, icon, and icon border use contrast-aware colors against the button background, including the disabled update-button state.
Concerns
- This is a user-visible styling change, but the PR description does not include attached screenshots or a screen recording demonstrating the install/update modal button contrast after the change. Manual testing is required for changes that can be manually tested. Please include screenshots or a screen recording that show it working end to end, or justify why manual testing is not possible.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Description has been updated with the install-modal screenshot inline (it was in a comment before) and a justification for not screenshotting the disabled-state Update button. /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates the custom Install and Update modal button labels to use foreground colors selected against the actual accent or disabled button surfaces. The enabled and disabled Update button paths now share the same selection state used for .disabled().
Concerns
- None identified.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
lucieleblanc
left a comment
lucieleblanc
left a comment
There was a problem hiding this comment.
Thank you for tackling this issue! Looking at this code, I think the best change would be to convert this modal to use the existing ActionButton component so that we don't need to manually define any theme + styling for it.
Also -- since this is theming and accessibility-related, please include screenshots that show multiple themes: dark, light, high-contrast, different colors, etc. That makes it easy to verify the changes will work for a broad range of themes.
| // Disable the update button if no updates are selected (matches the | ||
| // `accent_label_color` branch chosen above). |
There was a problem hiding this comment.
Please restore this comment to the shorter form; the parenthetical doesn't provide useful information.
There was a problem hiding this comment.
I think the better fix here would be to convert this modal to render an ActionButton instead of building a new button component from scratch. Ideally, we shouldn't need custom color/theming logic for this.
…review) Per code-review feedback on warpdotdev#10586, the MCP installation and update modal action rows are rewritten to use the existing `ActionButton` component with `PrimaryTheme` + `NakedTheme` instead of hand-rolling the accent button. This removes the manual contrast-color juggling that originally shipped to fix warpdotdev#10517 — `PrimaryTheme` already routes the label color through `theme.font_color(theme.accent())`, which is the same accent-aware path other primary buttons across the app already use, so the original bug is fixed by adopting the shared component. Highlights: - `InstallationModalBody` and `UpdateModalBody` now hold typed-action `ViewHandle<ActionButton>` for Cancel and the primary button, created in `new(ctx)` and rendered via `ChildView`. The previous mouse-state fields, manual icon/border construction, and custom Text labels are gone. - The Enter keybinding hint (previously a hand-built `↩` icon stitched next to the label) now uses `ActionButton::with_keybinding`. This is visual only — the hint does not register a keystroke handler — so the existing dispatch paths (`handle_editor_event` on the install modal, the Update button's own `on_click` on the update modal) cannot double-fire. - `UpdateModalBody`'s disabled-when-no-selection behavior is now driven by `update_button.set_disabled(...)` from a small helper `refresh_update_button_disabled`, called from `set_installation`, `clear`, and the `SelectOption` action handler. The disabled palette comes from `ActionButton`'s default disabled theme rather than `PrimaryTheme`. - `InstallationModalBody::new()` and `UpdateModalBody::new()` now take a `&mut ViewContext<Self>` so the buttons can be registered as typed action views; their two callers in `mcp_servers_page.rs` and `list_page.rs` are updated accordingly. `UpdateModalBody::set_installation` and `clear` likewise grow a `ctx` parameter so the disabled state stays in sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lucieleblanc — addressed both review points:
|
|
/oz-review |
lucieleblanc
left a comment
lucieleblanc
left a comment
There was a problem hiding this comment.
Excellent, thank you! Please rebase, clean up the comments, and I'll merge.
| // Cancel and Install both delegate to the existing typed-action | ||
| // handler so the modal-level routing (event emission, error | ||
| // surfacing) is unchanged. The Install button shows the Enter | ||
| // keybinding as a visual hint via `ActionButton`'s built-in | ||
| // keystroke rendering, replacing the prior hand-rolled `↩` icon. | ||
| // The hint does not register a keystroke handler — Enter is | ||
| // dispatched by the focused input editor through | ||
| // `handle_editor_event` below — so adding it here cannot cause a | ||
| // double-fire of `InstallationModalBodyAction::Install`. Theming | ||
| // (accent background, contrast-aware label color) is now driven | ||
| // entirely by `PrimaryTheme`, which fixes #10517 by deferring to | ||
| // the same accent-button color lookup the rest of the app uses. |
There was a problem hiding this comment.
Please remove this comment.
| // Buttons own their own theming via `ActionButton` — see `new()` for | ||
| // construction and the rationale for this refactor (issue #10517). |
| // Cancel and Update both delegate to the existing typed-action | ||
| // handler so the modal-level routing (event emission, error | ||
| // surfacing) is unchanged. The Update button shows the Enter | ||
| // keybinding as a visual hint via `ActionButton`'s built-in | ||
| // keystroke rendering — the hint does not register a keystroke | ||
| // handler, so `UpdateModalBodyAction::Update` is only ever | ||
| // dispatched by the button's own `on_click` below. Theming | ||
| // (accent background, contrast-aware label color) comes from | ||
| // `PrimaryTheme`; the disabled state when nothing is selected is | ||
| // driven by `ActionButton::set_disabled` (toggled below in | ||
| // `refresh_update_button_disabled`), which uses the component's | ||
| // own disabled palette. Together this fixes #10517 by deferring | ||
| // to the same accent-button + disabled lookups the rest of the | ||
| // app uses. |
| // Buttons own their own theming and disabled state via | ||
| // `ActionButton`. See `new()` for construction and the rationale for | ||
| // this refactor (#10517); see `refresh_update_button_disabled` for | ||
| // how the Update button's disabled state stays in sync with the | ||
| // selection. |
…arpdotdev#10517) Convert the MCP install and update modal action buttons to the shared ActionButton component instead of hand-rolled buttons with custom theming. This fixes the accent-button label/icon contrast (warpdotdev#10517) by deferring to the same accent-button color and disabled-state lookups the rest of the app already uses; the Update button's disabled state is driven by ActionButton::set_disabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
maxmilian
force-pushed
the
fix/10517-mcp-button-styling
branch
from
4f92a51 to
4718103
Compare
|
@lucieleblanc done — rebased onto latest One heads-up from the rebase: #10166 (by @acarl005) merged on May 11 and also closes #10517 with a minimal fix — it switches the button label/icon So #10517 is technically already fixed on |
lucieleblanc
changed the title
lucieleblanc
left a comment
lucieleblanc
left a comment
There was a problem hiding this comment.
We should be using ActionButtons anyway, so this is still good! I've changed the title of the PR to reflect the change. Thanks!
2 participants
Description
The MCP install/update modal primary buttons (
Install/Update) usedwith_custom_labelso they could include a↩icon next to the text, and hardcoded the icon and label colors totheme.active_ui_text_color(). That helper resolves tomain_text_color(surface_2())— it picks for contrast against the surrounding modal surface, not against the accent-colored button background the label actually sits on. On themes wheresurface_2andaccenthave similar luminance, the label became visually washed out (issue #10517).This PR switches the icon, icon border, and label text inside both modals' accent-button labels to:
That's the same contrast-aware color the standard accent button text label uses internally — see
warp_core::ui::builder::default_button_stylesforButtonVariant::Accent(crates/warp_core/src/ui/builder.rs:237).accent_button_color()returns the exactaccent.blend(foreground.with_opacity(...))background formuladefault_button_stylesuses for the button surface, so this matches what the button itself does for its native text labels.The Update button can also be disabled when no rows are selected. In that state the button surface flips to
surface_3perdisabled_button_styles(crates/warp_core/src/ui/builder.rs:243-245), so the custom label color branches todisabled_text_color(surface_3())to match. The Install button has no disabled state, so its label always uses the accent-button color.The cancel buttons (rendered with
ButtonVariant::Text, sitting on the modal surface) and the modal title / field labels are intentionally left alone —active_ui_text_color()is correct for them.Linked Issue
Fixes #10517.
ready-to-specorready-to-implement.Testing
./script/runBuilt
WarpOss.appfrom this branch, opened Settings → MCP Servers → clicked+on a gallery preset (Datadog) to trigger the install modal. TheInstallbutton now renders with the correct contrast-aware foreground (white text + icon on the accent-blue background) — matching the "what it should be" mock in the issue.Update modal uses identical code (separate copy in
update_modal.rs); the same fix is applied there. The disabled-state branch was not manually screenshotted because triggering the Update modal requires a cloud-synced MCP template with a newer published version than the locally-installed copy, which the OSS dev build cannot easily reproduce. The disabled label color is sourced directly fromdisabled_button_styles's standard helper (disabled_text_color(surface_3())), so it matches every other disabled accent button in the app — the regression Oz flagged was specifically that the enabled color was leaking into the disabled state, and that branch is now keyed off the samehas_selectionflag the existing.disabled()call uses, so the two states cannot diverge.Screenshots / Videos
After (this PR — install modal triggered via Datadog preset):
For comparison, the broken state ("what it is") and target state ("what it should be") are both captured in issue #10517.
Agent Mode