dialog: Merge button_props instead of replacing them - #3126
Merged
Merged
Conversation
`AlertDialog::confirm` set `show_cancel` on the dialog's button props, and
`Dialog::on_ok`/`on_cancel`/`on_close` wrote their callbacks into the same
value, but `button_props` then assigned the whole value over it. A caller who
wrote `.confirm().button_props(DialogButtonProps::default().ok_text("Delete"))`
lost the Cancel button, and one who set a callback before the props lost the
callback, so downstream code had to repeat `.show_cancel(true)` at every call
site and remember which order was safe.
Every field of `DialogButtonProps` is now unset until a builder sets it, and
`button_props` merges: the fields the value sets win, the rest of what the
dialog carries survives, in any call order. Unset fields fall back to the
defaults they had before — `Primary` for the OK variant, no Cancel button,
callbacks that close the dialog.
`AlertDialog` also gains `ok_text`, `ok_variant`, `cancel_text` and
`cancel_variant`, so the common dangerous confirmation no longer builds a props
bundle:
alert.title("Delete “Roadmap”?").confirm().ok_text("Delete").ok_variant(Danger)
They live on `AlertDialog` only: a plain `Dialog` renders its buttons through
`footer`, so the same builders there would compile and do nothing.
`AlertDialog` now keeps one copy of the button props, on the base dialog,
rather than a second copy it had to reconcile when building the surface.
The `DialogButtonProps` builder signatures are unchanged and `Default` still
works, so this is not a breaking change.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
huacnlee
force-pushed
the
dialog-button-props-merge
branch
from
September 18, 2026 11:29
c143220 to
5c30f12
Compare
…l host test The count of four encoded the replace bug: `AlertDialog::render` put its own button props over the base dialog's, which is where `on_close` lived, so a cancelled alert reported `on_cancel` alone. With the props merged the alert reports the same pair a cancelled `Dialog` does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
huacnlee
enabled auto-merge (squash)
September 18, 2026 16:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reported downstream (ai-desktop / pilot):
AlertDialog::confirm()only setsshow_cancelon the dialog's button props, andDialog::on_ok/on_cancel/on_closewrite their callbacks into that same value — butbutton_propsassigned the whole value over it. So this quietly lost the Cancel button:
alert .confirm() .button_props(DialogButtonProps::default().ok_text("Delete").ok_variant(Danger))and this quietly lost the callback:
Downstream had to repeat
.show_cancel(true)at every call site and leave acomment saying "button_props replaces the whole value".
Dialog::button_propshad the same behavior.
Change
Every field of
DialogButtonPropsis unset until a builder sets it(
ok_variant,cancel_variant,show_canceland the three callbacks becameOption), andbutton_propsmerges rather than assigns: the fields thevalue sets win, the rest of what the dialog already carries survives, in any
call order. Unset fields fall back at render time to the defaults they had
before —
Primaryfor the OK variant,ButtonVariant::default()for Cancel,no Cancel button, callbacks that close the dialog.
AlertDialoggainsok_text,ok_variant,cancel_textandcancel_variant, so the common dangerous confirmation no longer has to builda props bundle:
They are on
AlertDialogonly. A plainDialogrenders its buttons throughfooter, so the same builders there would compile and do nothing — exactlythe kind of API a downstream reader (or model) would mistake for a working
one.
AlertDialognow keeps one copy of the button props, on the base dialog,instead of a second copy it had to reconcile while building the surface.
A cancelled
AlertDialognow reportson_closeas well ason_cancel, thepair a cancelled
Dialogalready reported. The second copy's replace haddropped the alert's
on_closeon the way to the surface; the shell host test(
window_effects_host.rs) had encoded that with a close count of four and nowexpects five.
The
DialogButtonPropsbuilder signatures are unchanged andDefaultstillworks, so this is not a breaking change — no
Breaking Changessection needed.Tests
crates/component/src/dialog/alert_dialog.rsanddialog.rs:.confirm()followed by.button_props(…)keeps the Cancel button;.button_props(…)followed by.confirm()does too;.on_ok(…)followed by.button_props(…)still runs the callback once;button_propsvalue;Dialog::button_propscalls merge with what the dialog carries.The first, third and fifth fail against the old replace semantics (verified by
temporarily restoring them); the other two only compile with the new builders.
Docs
website/component/alert-dialog.mdand itswebsite/zh-CN/component/counterpart: the dangerous-confirmation examples now use the direct builders,
and the page states that
button_propsoverrides only the fields the valuesets.
website/component/dialog.md(both locales) gets a one-paragraph"Action Buttons" note: a
Dialogputs its own buttons infooterand has themdispatch
Confirm/Cancel,on_ok/on_canceldecide Enter/Esc, and aconfirmation with default buttons is
AlertDialog.skills/gpui-kit/references/usage.mdand the AlertDialog story follow the samestyle.
🤖 Generated with Claude Code