fix: simplify approval confirmation flow#2143
Merged
Merged
Conversation
Remove staged two-step approval for destructive actions. Enter now commits the selected option, while y/a/d still act as direct shortcuts. Keep destructive warnings visible and improve selected option contrast in the approval modal. Add test coverage for the one-step flow and highlight styling.
Contributor
There was a problem hiding this comment.
Code Review
This pull request simplifies the TUI approval flow by removing the two-key staged confirmation mechanism for destructive actions. Instead, it implements a direct one-step approval flow where users can commit the highlighted option with Enter or use direct shortcuts (y/a/d), while maintaining prominent warning copy and styling for destructive actions. The associated tests and widgets have been updated to align with this streamlined interaction model, and a new test has been added to verify the contrasting highlight style of selected destructive options. I have no feedback to provide as there are no review comments.
Treat approval abort separately from deny. Deny rejects only the current tool call; abort now cancels the active turn and marks in-flight UI state as interrupted.
Contributor
Author
|
hi @Hmbown please help to take a look again if you have time. thanks. |
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.
Summary
Fixes #1257.
Destructive approval prompts required two matching approval keypresses before a
tool could run. This made common approval flows slower and confusing because the
first keypress only staged the decision.
This PR removes the staged confirmation state. Approval prompts now behave
consistently:
Entercommits the currently selected optiony/1approves oncea/2approves for the sessiond/n/3denies the current toolEscaborts the current turnTabstill collapses or expands the approval cardvstill opens full tool parametersThis also fixes a pre-existing mismatch where
Escemitted an abort decision,but the downstream handler treated it the same as deny.
Escnow cancels theactive turn and marks local in-flight output as interrupted.
The destructive risk badge, impact summary, and warning styling remain visible.
The selected approval row now uses a stronger blue/white highlight for better
contrast.
Reimplements the useful parts of #1459 against current
main.FROM:

TO:
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR simplifies the approval confirmation flow by removing the two-step "stage then confirm" requirement for destructive operations. All risk levels now share the same single-keypress commit behaviour, while the destructive badge, impact summary, and warning styling remain.
Escis also fixed to properly cancel the active turn (viaengine_handle.cancel()) rather than being treated identically to a deny.approval.rs: Removespending_confirmstate andcommit_or_stage/requires_confirmlogic; replaces with a simplercommit_optionthat immediately emits a decision. All downstream tests are updated to match the one-step behaviour.ui.rs: ExtractsApprovalDecisionEvent+apply_approval_decisionfor cleanliness; separatesAbortfromDenysoEsccallsengine_handle.cancel()(consistent with Ctrl+C / shell-cancel paths). The engine'sawait_tool_approvalusestokio::select!on the cancel token so the pending call is safely unblocked.widgets/mod.rs: Unifies the footer hint across both risk levels, removes staged-confirmation rendering, and switches the selection highlight fromSELECTION_BGtoDEEPSEEK_BLUEfor stronger contrast.Confidence Score: 5/5
Safe to merge — the simplification is well-scoped and the abort path is consistent with every other turn-cancellation site in the codebase.
The removal of the staged-confirmation state is clean and fully tested. The most load-bearing change — Esc now calling engine_handle.cancel() instead of deny_tool_call() — is sound because the engine's await_tool_approval uses tokio::select! that races the cancel token against the approval channel, so the pending tool call is always unblocked. This pattern already exists in the Ctrl+C and shell-cancel paths. No unresolved tool calls, no dropped state, and the session-denied cache logic is unchanged for the Deny arm.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant ApprovalView participant UI as UI (apply_approval_decision) participant EngineHandle Note over User,EngineHandle: Approve / Deny path (all risk levels) User->>ApprovalView: y / a / d / Enter / 1 / 2 / 3 ApprovalView->>ApprovalView: commit_option(option) ApprovalView-->>UI: ViewEvent::ApprovalDecision alt Approved or ApprovedForSession UI->>EngineHandle: approve_tool_call(tool_id) else Denied UI->>UI: cache approval_key in session_denied UI->>EngineHandle: deny_tool_call(tool_id) end EngineHandle-->>EngineHandle: unblocks await_tool_approval (rx_approval) Note over User,EngineHandle: Abort path (Esc cancels the active turn) User->>ApprovalView: Esc ApprovalView-->>UI: ViewEvent::ApprovalDecision Abort UI->>EngineHandle: cancel() sets CancellationToken EngineHandle-->>EngineHandle: await_tool_approval select! fires on cancel_token.cancelled() UI->>UI: mark_active_turn_cancelled_locally(app)Reviews (2): Last reviewed commit: "fix: make approval abort cancel the turn" | Re-trigger Greptile