Skip to content

Finish the desktop dialogs' button order, keys and destructive styling - #1607

Merged
realtonyyoung merged 3 commits into
mainfrom
tonyyoung/ai-3372-dialog-buttons-rest
Oct 19, 2026
Merged

realtonyyoung merged 3 commits into
mainfrom
tonyyoung/ai-3372-dialog-buttons-rest

Conversation

@realtonyyoung

@realtonyyoung realtonyyoung commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1242 — AI-3372

What & why

This finishes the dialog button rule that #1600 started on the re-auth dialog: dismiss → secondary → primary, dismiss on Esc, primary on Enter. The exception is a primary that removes, stops or approves something.

  • Launch approval. Now Deny · Allow & remember · Allow once, on the Kcap classes and canvas. Esc denies, and no button takes Enter: the window is Topmost and can appear while the user is typing elsewhere, so a keystroke must never approve a launch.
  • Destructive primaries. A new filled kcapDangerFill style is used by Remove profile (the lifecycle prompt) and Stop anyway (the force-stop dialog). Neither is on Enter any more; Remove profile was previously a blue default button and Stop anyway a hard-coded red default.
  • Feedback. Cancel/Close · Sign in/Send another · Send. Esc dismisses and Enter sends; the message box keeps its own Enter, since it accepts returns. Spacing is 8, matching the other windows.
  • Code-built windows. Force-stop and Startup failed take the canvas and text brushes and the Kcap button classes. Move to Applications was already a themed XAML window.
  • Chat cards. Action rows go from 6 to 8 spacing, so every action group uses one spacing.

Not changed:

  • First-run wizard. It is now a pane in the main window rather than a dialog, so a window-level Enter/Esc would also fire from the main window's other inputs. Its footer already follows the order.
  • Dismiss labels. They already map to one per intent: Cancel abandons, Not now defers, Close ends, Quit exits, and Deny is a decision rather than a dismiss.

This touches files that #1508 rewrites, so #1508 will need a rebase.

Where to look

  • ConsentPromptWindow.axaml: the key-binding comment states the no-Enter rule.
  • LifecyclePromptViewModel.AcceptIsDestructive, and its two-way class binding in LifecyclePromptWindow.axaml.
  • App.BuildConfirmForceStopWindow and KcapBrush. Before the theme loads, KcapBrush returns null, so the window keeps the platform default.

Verification

  • New DialogActionRowTests:
    • The launch prompt's order, with Esc on Deny and nothing on Enter.
    • Feedback's order and both keys.
    • The lifecycle prompt's Accept: destructive and off Enter for a removal, primary and on Enter for a reload.
    • The force-stop dialog's classes, with nothing on Enter.
  • Existing tests pass: SignInStepViewTests 2, FeedbackWindowSmokeTests 1, LifecyclePromptViewModelTests 17, ConsentPromptViewModelTests 29, and the startup-error window test.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Style
    • Updated dialog backgrounds, text, and button styling, including distinct styling for destructive actions.
    • Adjusted spacing between action buttons and reordered consent and feedback actions.
  • Behavior
    • Enter no longer activates destructive actions, including profile removal, stopping a participant, or force-stopping.
    • Dialog buttons now have updated default and cancel behavior; consent prompts mark Deny as the cancel action, and feedback actions vary with submission state.

Enter never presses a button that removes, stops or approves: the launch prompt is Topmost and can appear mid-typing, and a destructive primary is one keystroke from data loss.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Oct 19, 2026

Copy link
Copy Markdown

AI-3372

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 9d289a93-394c-4efb-80db-bcf2b79afde4
📥 Commits

Reviewing files that changed from the base of the PR and between c2a1280 and 3a2b070.

📒 Files selected for processing: 1
  • src/Capacitor.App/App.axaml

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Dialog action groups now use updated button styles, ordering, and keyboard bindings. Code-built windows use Kcap theme brushes. Pending-card action spacing and dialog action tests are also updated.

Changes: Dialog action consistency

Layer / File(s) Summary
Destructive styling and lifecycle accept
src/Capacitor.App/App.axaml, src/Capacitor.App/ViewModels/LifecyclePromptViewModel.cs, src/Capacitor.App/Views/LifecyclePromptWindow.axaml, test/Capacitor.App.Tests.Unit/DialogActionRowTests.cs
A filled danger button style is added. Lifecycle profile removal uses it and does not make Accept the default action. Tests cover destructive and non-destructive lifecycle prompts.
Code-built window theming and force-stop actions
src/Capacitor.App/App.axaml.cs, test/Capacitor.App.Tests.Unit/DialogActionRowTests.cs
The startup-error and force-stop windows use Kcap theme brushes. The force-stop action uses the danger style and is not the default action. Tests check its styling and action order.
Dialog action groups and validation
src/Capacitor.App/Views/ConsentPromptWindow.axaml, src/Capacitor.App/Views/FeedbackWindow.axaml, src/Capacitor.App/Views/PendingCardTemplates.axaml, test/Capacitor.App.Tests.Unit/DialogActionRowTests.cs
Consent actions are reordered and styled, with Deny set as the cancel action. Feedback actions are reordered, with state-dependent cancel and default bindings. Pending-card action spacing changes from 6 to 8. Tests check consent and feedback action settings.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: alexeyzimarev

Merge Risk: ⚪ Minimal · up to 3a2b0

The updated dialogs preserve the expected theme and keyboard behavior. No actionable merge-blocking risk remains in the reviewed changes.

### Architecture Summary

Architecture risk: 🔵 Low · up to 3a2b0

The change affects 2 systems.

Changed systems: src, test

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 7 changed files map to changed impact.
  • observed — test (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/Capacitor.App/App.axaml.cs: Added KcapBrush, which returns the current theme resource as an IBrush when available, or null otherwise.
  • observed — Modified behavior in src/Capacitor.App/App.axaml.cs: The startup error window now gets its background and foreground from the Kcap theme brushes.
  • observed — Modified behavior in src/Capacitor.App/App.axaml.cs: Updated the force-stop dialog comment to state that Enter does not trigger “Stop anyway.” The buttons now use theme classes, and the stop button no longer uses IsDefault or hard-coded red and white brushes; the window also uses the Kcap theme background and foreground.
  • observed — Modified behavior in src/Capacitor.App/ViewModels/LifecyclePromptViewModel.cs: Added the AcceptIsDestructive property and documentation identifying profile removal as destructive and stating that Enter does not press its Accept action.
Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Linked Issues check Warning Direct issue [#1242] requires Enter and Esc behavior for the consent prompt and key behavior for the wizard. The current evidence confirms Esc on Deny but deliberately leaves Enter unbound for `Consen… Bind Enter and Esc for the consent prompt as required by [#1242]. Add scoped wizard-pane handling for Enter and Esc, including the import step, without affecting unrelated main-window inputs. Add or update automated tests for these behavior…
Docstring Coverage Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely summarizes the main changes: dialog button order, keyboard behavior, and destructive styling.
Out of Scope Changes check Passed The reported changes support [#1242]. They implement dialog ordering and key behavior, destructive styling, Kcap theming, lifecycle behavior, action spacing, and focused automated tests. No unrelated …
Full details: Out of Scope Changes check

Explanation

The reported changes support [#1242]. They implement dialog ordering and key behavior, destructive styling, Kcap theming, lifecycle behavior, action spacing, and focused automated tests. No unrelated change is demonstrated by the available PR evidence.

Full details: Linked Issues check

Explanation

Direct issue [#1242] requires Enter and Esc behavior for the consent prompt and key behavior for the wizard. The current evidence confirms Esc on Deny but deliberately leaves Enter unbound for ConsentPromptWindow. The PR also excludes wizard key handling because the wizard is a pane. The safety rationale does not satisfy these issue requirements. The reported feedback, lifecycle, force-stop, theming, danger styling, and spacing changes support the remaining objectives.

Resolution

Bind Enter and Esc for the consent prompt as required by [#1242]. Add scoped wizard-pane handling for Enter and Esc, including the import step, without affecting unrelated main-window inputs. Add or update automated tests for these behaviors.

Full details: Docstring Coverage

Explanation

Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Standardize desktop dialog actions and prevent unsafe Enter shortcuts

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Put dismiss actions first and primary actions last across desktop dialogs.
• Reserve Enter for safe actions; use Esc to dismiss and filled danger styling for destructive
 actions.
• Apply consistent dialog theming and spacing, with tests for button order, styling, and key
 bindings.
Diagram

graph TD
  A["Dialog actions"] --> B["Dismiss first"] --> C["Primary last"] --> D{"Action type"}
  D -->|Safe| E["Enter activates"]
  D -->|Approval| F["No Enter"]
  D -->|Destructive| G["Danger fill"] --> F
  B --> H["Esc dismisses"]
Loading
High-Level Assessment

Keep the shared danger style and per-dialog key bindings. They express the safety rule without introducing a dialog framework for a small set of existing windows.

Files changed (8) +152 / -27

Enhancement (4) +43 / -7
App.axamlAdd a shared filled destructive button style +27/-0

Add a shared filled destructive button style

• Adds kcapDangerFill with danger-colored resting, hover, pressed, and disabled states for destructive primary actions.

src/Capacitor.App/App.axaml

LifecyclePromptViewModel.csIdentify profile removal as a destructive acceptance +6/-0

Identify profile removal as a destructive acceptance

• Exposes AcceptIsDestructive for remove-profile prompts so the view can select the danger style and disable Enter activation.

src/Capacitor.App/ViewModels/LifecyclePromptViewModel.cs

FeedbackWindow.axamlPut feedback dismissal first and bind dialog keys +7/-4

Put feedback dismissal first and bind dialog keys

• Moves Cancel or Close ahead of secondary actions, binds Esc to the visible dismissal and Enter to Send before submission, and changes action spacing to eight.

src/Capacitor.App/Views/FeedbackWindow.axaml

PendingCardTemplates.axamlStandardize chat-card action spacing +3/-3

Standardize chat-card action spacing

• Changes three action rows from six to eight pixels of button spacing.

src/Capacitor.App/Views/PendingCardTemplates.axaml

Bug fix (3) +25 / -20
App.axaml.csTheme code-built dialogs and remove force-stop's Enter shortcut +13/-13

Theme code-built dialogs and remove force-stop's Enter shortcut

• Looks up app brushes with a fallback when the theme is unavailable, then applies them to the startup-error and force-stop windows. Force-stop uses shared button classes, keeps Esc on Cancel, and no longer makes Stop anyway the Enter default.

src/Capacitor.App/App.axaml.cs

ConsentPromptWindow.axamlReorder launch approval actions without an Enter default +9/-6

Reorder launch approval actions without an Enter default

• Orders Deny, Allow & remember, then Allow once; Esc denies while Enter approves nothing. Adds app brushes and shared button classes.

src/Capacitor.App/Views/ConsentPromptWindow.axaml

LifecyclePromptWindow.axamlStyle removal as destructive and keep it off Enter +3/-1

Style removal as destructive and keep it off Enter

• Binds the Accept button's danger or primary class and default-button status to AcceptIsDestructive. Non-destructive lifecycle prompts retain Enter activation.

src/Capacitor.App/Views/LifecyclePromptWindow.axaml

Tests (1) +84 / -0
DialogActionRowTests.csTest dialog action order, classes, and default keys +84/-0

Test dialog action order, classes, and default keys

• Adds Avalonia tests for launch approval, feedback, lifecycle removal versus reload, and force-stop. Assertions cover action ordering, Esc and Enter assignments, and destructive styling.

test/Capacitor.App.Tests.Unit/DialogActionRowTests.cs

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@qodo-code-review

qodo-code-review Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Destructive buttons turn gray on hover ✓ Resolved
Description
The kcapDangerFill hover and pressed style replaces its red background with the neutral
KcapMutedBrush. When users hover over or press Remove profile or Stop anyway, the prominent danger
fill disappears even though the border remains red.
Code

src/Capacitor.App/App.axaml[542]

+            <Setter Property="Background" Value="{StaticResource KcapMutedBrush}" />
Relevance

●●● Strong

Destructive styling explicitly intends danger emphasis; neutral hover fill contradicts that intent
and is a deterministic visual correctness issue.

PR-#1030

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The palette defines KcapDangerBrush as red and KcapMutedBrush as neutral gray. The new style
uses red at rest but switches its fill to gray for both interactive states; the lifecycle removal
and force-stop buttons both use this class.

src/Capacitor.App/App.axaml[21-33]
src/Capacitor.App/App.axaml[526-545]
src/Capacitor.App/Views/LifecyclePromptWindow.axaml[73-75]
src/Capacitor.App/App.axaml.cs[1848-1850]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The destructive button changes from a red fill to a neutral gray fill on hover and press, weakening its warning appearance.
## Fix Focus Areas
- src/Capacitor.App/App.axaml[540-545]
## Recommended Fix
Use a danger-colored background for the hover and pressed states, preserving readable text contrast and a matching border.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
✅ Compliance rules (platform): 59 rules
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Multiple dialog behaviors and keyboard paths carry real UI regression risk.
✅ Fix-loop round: fix loop

Grey Divider

Tip of the day
💡 Did you know, you can import rules and skills only from the repositories your team follows

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 3a2b070

Results up to commit c2a1280 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Destructive buttons turn gray on hover ✓ Resolved
Description
The kcapDangerFill hover and pressed style replaces its red background with the neutral
KcapMutedBrush. When users hover over or press Remove profile or Stop anyway, the prominent danger
fill disappears even though the border remains red.
Code

src/Capacitor.App/App.axaml[542]

+            <Setter Property="Background" Value="{StaticResource KcapMutedBrush}" />
Relevance

●●● Strong

Destructive styling explicitly intends danger emphasis; neutral hover fill contradicts that intent
and is a deterministic visual correctness issue.

PR-#1030

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The palette defines KcapDangerBrush as red and KcapMutedBrush as neutral gray. The new style
uses red at rest but switches its fill to gray for both interactive states; the lifecycle removal
and force-stop buttons both use this class.

src/Capacitor.App/App.axaml[21-33]
src/Capacitor.App/App.axaml[526-545]
src/Capacitor.App/Views/LifecyclePromptWindow.axaml[73-75]
src/Capacitor.App/App.axaml.cs[1848-1850]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The destructive button changes from a red fill to a neutral gray fill on hover and press, weakening its warning appearance.
## Fix Focus Areas
- src/Capacitor.App/App.axaml[540-545]
## Recommended Fix
Use a danger-colored background for the hover and pressed states, preserving readable text contrast and a matching border.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

coderabbitai[bot]
coderabbitai Bot previously approved these changes Oct 19, 2026
@realtonyyoung

Copy link
Copy Markdown
Collaborator Author

/agentic_review

Comment thread src/Capacitor.App/App.axaml Outdated
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit c2a1280

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@realtonyyoung

Copy link
Copy Markdown
Collaborator Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 3a2b070

@realtonyyoung
realtonyyoung merged commit dc17020 into main Oct 19, 2026
9 checks passed
@realtonyyoung
realtonyyoung deleted the tonyyoung/ai-3372-dialog-buttons-rest branch October 19, 2026 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make desktop app dialog button groups consistent

1 participant