diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index dc7e9cdc..92e3208e 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -16,10 +16,9 @@ //! `2` / `a` approves for the session. //! - **Destructive** (`RiskLevel::Destructive`) — file writes, shell, //! patches, MCP actions, unclassified tools, and any "fetch arbitrary -//! content" surface. The first approve press *stages* a decision and -//! the second matching press commits — muscle-memory `Enter` cannot -//! accidentally land on an approval. Any non-approve key clears the -//! staging and keeps the user in selection mode. +//! content" surface. The takeover keeps the destructive badge and +//! impact summary visible, then lets `Enter` commit the highlighted +//! option or `y` / `a` / `d` commit directly. //! //! The decision events emitted upstream are unchanged //! (`ViewEvent::ApprovalDecision`), so `ui.rs` and the engine handle @@ -102,8 +101,8 @@ pub enum ToolCategory { /// Stakes-based variant for the takeover modal. /// /// `RiskLevel::Benign` lets a single keystroke commit the approval. -/// `RiskLevel::Destructive` requires an explicit second confirmation -/// keypress so muscle-memory `Enter` never lands on an irreversible op. +/// `RiskLevel::Destructive` keeps stronger warning copy and styling +/// around approvals that can touch files, shell, or remote state. /// /// Routing rules live in [`classify_risk`] — when in doubt, route to /// `Destructive`. @@ -228,13 +227,12 @@ pub fn get_tool_category(name: &str) -> ToolCategory { /// The bias is conservative: a category we don't recognise routes to /// `Destructive`, and any shell command that `command_safety` flags as /// `Dangerous` is forced to `Destructive` even when the rest of the -/// request looks calm. The split lets the modal swap muscle-memory -/// approval for an explicit two-key confirmation on anything that can -/// touch state outside this turn. +/// request looks calm. The split lets the modal render stronger warning +/// copy on anything that can touch state outside this turn. #[must_use] pub fn classify_risk(tool_name: &str, category: ToolCategory, params: &Value) -> RiskLevel { match category { - // Read paths and discovery — never staged. + // Read paths and discovery. ToolCategory::Safe | ToolCategory::McpRead => RiskLevel::Benign, // Query-only network is benign; opening a URL pulls arbitrary // remote content, so it stays destructive. @@ -448,9 +446,7 @@ fn build_impact_summary_zh_hans( } } -/// Indices into the option list shared by both variants. Visible to -/// the widget module so it can render the staged-confirmation banner -/// without re-deriving the variant from the request. +/// Indices into the option list shared by both variants. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ApprovalOption { ApproveOnce, @@ -486,16 +482,6 @@ impl ApprovalOption { ApprovalOption::Abort => ReviewDecision::Abort, } } - - /// Whether this option needs an explicit second-key confirmation in - /// the destructive variant. Deny/Abort are never staged. - fn requires_confirm(self, risk: RiskLevel) -> bool { - matches!(risk, RiskLevel::Destructive) - && matches!( - self, - ApprovalOption::ApproveOnce | ApprovalOption::ApproveAlways - ) - } } /// Approval overlay state managed by the modal view stack @@ -504,10 +490,6 @@ pub struct ApprovalView { request: ApprovalRequest, selected: usize, locale: Locale, - /// When `Some`, the destructive variant has staged this approval and - /// is waiting for the user to press the same key (or `Enter`) again. - /// Any other key clears the staging. - pending_confirm: Option, timeout: Option, requested_at: Instant, /// Whether the approval card is collapsed to a single-line banner. @@ -525,7 +507,6 @@ impl ApprovalView { request, selected: 0, locale, - pending_confirm: None, timeout: None, requested_at: Instant::now(), collapsed: false, @@ -534,22 +515,17 @@ impl ApprovalView { fn select_prev(&mut self) { self.selected = self.selected.saturating_sub(1); - // Moving the selection abandons any staged confirmation; the - // user is reconsidering. - self.pending_confirm = None; } fn select_next(&mut self) { self.selected = (self.selected + 1).min(ApprovalOption::ORDER.len() - 1); - self.pending_confirm = None; } fn current_option(&self) -> ApprovalOption { ApprovalOption::from_index(self.selected) } - /// Test-only accessor — the widget reads decisions through - /// `commit_or_stage` instead of polling. + /// Test-only accessor for the selected option's decision. #[cfg(test)] fn current_decision(&self) -> ReviewDecision { self.current_option().decision() @@ -566,33 +542,13 @@ impl ApprovalView { self.request.risk } - /// The staged option, if any. `None` in the benign variant or when - /// no approve key has been pressed yet. - pub(crate) fn pending_confirm(&self) -> Option { - self.pending_confirm - } - pub(crate) fn locale(&self) -> Locale { self.locale } - /// Try to commit (or stage) the given option respecting the - /// variant's confirmation policy. Returns the action the modal - /// stack should apply. - fn commit_or_stage(&mut self, option: ApprovalOption) -> ViewAction { - if option.requires_confirm(self.request.risk) { - // Two-step destructive flow: first press stages, second - // press of the same option commits. - if self.pending_confirm == Some(option) { - self.pending_confirm = None; - return self.emit_decision(option.decision(), false); - } - self.pending_confirm = Some(option); - self.selected = option.index(); - return ViewAction::None; - } - // Benign variant or non-approve options commit immediately. - self.pending_confirm = None; + /// Commit the given option and close the approval modal. + fn commit_option(&mut self, option: ApprovalOption) -> ViewAction { + self.selected = option.index(); self.emit_decision(option.decision(), false) } @@ -647,31 +603,23 @@ impl ModalView for ApprovalView { self.select_next(); ViewAction::None } - KeyCode::Enter => self.commit_or_stage(self.current_option()), + KeyCode::Enter => self.commit_option(self.current_option()), // Direct shortcuts; '1' / '2' map to the first two options - // so a numeric pad still works for benign approve flows. + // so a numeric pad still works for approve flows. KeyCode::Char('y') | KeyCode::Char('Y') | KeyCode::Char('1') => { - self.commit_or_stage(ApprovalOption::ApproveOnce) + self.commit_option(ApprovalOption::ApproveOnce) } KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => { - self.commit_or_stage(ApprovalOption::ApproveAlways) + self.commit_option(ApprovalOption::ApproveAlways) } KeyCode::Char('n') | KeyCode::Char('N') | KeyCode::Char('d') | KeyCode::Char('D') - | KeyCode::Char('3') => self.commit_or_stage(ApprovalOption::Deny), - KeyCode::Char('v') | KeyCode::Char('V') => { - self.pending_confirm = None; - self.emit_params_pager() - } + | KeyCode::Char('3') => self.commit_option(ApprovalOption::Deny), + KeyCode::Char('v') | KeyCode::Char('V') => self.emit_params_pager(), KeyCode::Esc => self.emit_decision(ReviewDecision::Abort, false), - _ => { - // Any unrecognised key cancels a staged confirmation — - // the user is no longer aiming at "approve". - self.pending_confirm = None; - ViewAction::None - } + _ => ViewAction::None, } } @@ -1030,13 +978,13 @@ mod tests { #[test] fn risk_query_only_network_is_benign_but_fetch_is_destructive() { - // web_search is read-only enough to skip the two-key dance. + // web_search is read-only enough to use the benign variant. let cat = ToolCategory::Network; assert_eq!( classify_risk("web_search", cat, &json!({"q": "rust"})), RiskLevel::Benign ); - // fetch_url pulls arbitrary remote content; never staged. + // fetch_url pulls arbitrary remote content, so it stays destructive. assert_eq!( classify_risk("fetch_url", cat, &json!({"url": "https://example.com"})), RiskLevel::Destructive @@ -1163,7 +1111,6 @@ mod tests { let view = ApprovalView::new(benign_request()); assert_eq!(view.selected, 0); assert!(view.timeout.is_none()); - assert_eq!(view.pending_confirm(), None); assert_eq!(view.risk(), RiskLevel::Benign); } @@ -1376,7 +1323,7 @@ mod tests { } // ======================================================================== - // ApprovalView Tests — Destructive Variant (two-key confirm) + // ApprovalView Tests — Destructive Variant (one-step approve with warning) // ======================================================================== #[test] @@ -1386,16 +1333,10 @@ mod tests { } #[test] - fn destructive_y_first_press_stages_then_second_commits() { + fn destructive_y_first_press_approves_once() { for code in [KeyCode::Char('y'), KeyCode::Char('Y')] { let mut view = ApprovalView::new(destructive_request()); - // First press stages — no decision emitted yet. - let action = view.handle_key(create_key_event(code)); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - - // Second press of the same key commits. let action = view.handle_key(create_key_event(code)); assert!( matches!( @@ -1411,15 +1352,10 @@ mod tests { } #[test] - fn destructive_enter_first_press_stages_then_second_commits() { + fn destructive_enter_approves_selected_option() { let mut view = ApprovalView::new(destructive_request()); - // Selection starts at ApproveOnce — Enter stages. - let action = view.handle_key(create_key_event(KeyCode::Enter)); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - - // Second Enter on the same selection commits. + // Selection starts at ApproveOnce — Enter commits the selected option. let action = view.handle_key(create_key_event(KeyCode::Enter)); assert!(matches!( action, @@ -1431,39 +1367,33 @@ mod tests { } #[test] - fn destructive_navigation_clears_staged_confirmation() { + fn destructive_navigation_then_enter_commits_highlighted_option() { let mut view = ApprovalView::new(destructive_request()); - view.handle_key(create_key_event(KeyCode::Char('y'))); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - - // Moving the selection abandons the staging. view.handle_key(create_key_event(KeyCode::Down)); - assert_eq!(view.pending_confirm(), None); + let action = view.handle_key(create_key_event(KeyCode::Enter)); + assert!(matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::ApprovedForSession, + .. + }) + )); } #[test] - fn destructive_unrelated_key_clears_staged_confirmation() { + fn destructive_unrelated_key_keeps_modal_open() { let mut view = ApprovalView::new(destructive_request()); - view.handle_key(create_key_event(KeyCode::Char('y'))); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - - // A key with no mapped action clears the staging. let action = view.handle_key(create_key_event(KeyCode::Char('q'))); assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), None); } #[test] - fn destructive_a_first_press_stages_then_second_commits_session() { + fn destructive_a_first_press_approves_for_session() { for code in [KeyCode::Char('a'), KeyCode::Char('A')] { let mut view = ApprovalView::new(destructive_request()); - let action = view.handle_key(create_key_event(code)); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways)); - let action = view.handle_key(create_key_event(code)); assert!( matches!( @@ -1479,23 +1409,8 @@ mod tests { } #[test] - fn destructive_y_then_a_does_not_commit_either() { - // Pressing 'y' then 'a' must NOT commit ApproveAlways — the - // second key is a different option, so it re-stages instead. - let mut view = ApprovalView::new(destructive_request()); - - let action = view.handle_key(create_key_event(KeyCode::Char('y'))); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - - let action = view.handle_key(create_key_event(KeyCode::Char('a'))); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways)); - } - - #[test] - fn destructive_deny_does_not_require_confirmation() { - // Deny / Abort skip the two-key dance — the user is bailing. + fn destructive_deny_commits_immediately() { + // Deny commits immediately — the user is rejecting the tool. for code in [ KeyCode::Char('n'), KeyCode::Char('N'), @@ -1520,9 +1435,6 @@ mod tests { #[test] fn destructive_esc_aborts_immediately() { let mut view = ApprovalView::new(destructive_request()); - // Stage something first. - view.handle_key(create_key_event(KeyCode::Char('y'))); - // Esc still aborts in one press. let action = view.handle_key(create_key_event(KeyCode::Esc)); assert!(matches!( action, @@ -1557,20 +1469,21 @@ mod tests { } #[test] - fn render_benign_includes_review_badge_and_one_step_hint() { + fn render_benign_includes_review_badge_and_selection_hint() { let view = ApprovalView::new(benign_request()); let lines = render_lines(&view, 100, 40); let joined = lines.join("\n"); assert!(joined.contains("REVIEW"), "missing REVIEW badge:\n{joined}"); + assert!(joined.contains("Choose"), "benign hint missing:\n{joined}"); assert!( - joined.contains("Single key approves"), - "benign hint missing:\n{joined}" + joined.contains("Enter selected option"), + "benign selection hint missing:\n{joined}" ); assert!(joined.contains("read_file")); } #[test] - fn render_destructive_shows_warning_badge_and_two_step_hint() { + fn render_destructive_shows_warning_badge_and_one_step_hint() { let view = ApprovalView::new(destructive_request()); let lines = render_lines(&view, 100, 40); let joined = lines.join("\n"); @@ -1579,31 +1492,15 @@ mod tests { "missing DESTRUCTIVE badge:\n{joined}" ); assert!( - joined.contains("Two keys to approve"), + joined.contains("Enter selected option"), "destructive hint missing:\n{joined}" ); assert!(joined.contains("write_file")); } - #[test] - fn render_destructive_after_stage_shows_confirm_banner() { - let mut view = ApprovalView::new(destructive_request()); - view.handle_key(create_key_event(KeyCode::Char('y'))); - let lines = render_lines(&view, 100, 40); - let joined = lines.join("\n"); - assert!( - joined.contains("Confirm destructive action"), - "confirm banner missing:\n{joined}" - ); - assert!( - joined.contains("(staged)"), - "stage marker missing:\n{joined}" - ); - } - #[test] fn render_destructive_zh_hans_localizes_security_copy() { - let mut view = ApprovalView::new_for_locale(destructive_request(), Locale::ZhHans); + let view = ApprovalView::new_for_locale(destructive_request(), Locale::ZhHans); let lines = render_lines(&view, 100, 40); let joined = compact_rendered_text(&lines); assert!( @@ -1611,8 +1508,12 @@ mod tests { "missing zh risk badge:\n{joined}" ); assert!( - joined.contains("两次按键确认"), - "missing zh two-step hint:\n{joined}" + joined.contains("选择："), + "missing zh selection prefix:\n{joined}" + ); + assert!( + joined.contains("Enter执行选中项，或直接按y/a/d"), + "missing zh one-step hint:\n{joined}" ); assert!( joined.contains("文件写入"), @@ -1630,22 +1531,6 @@ mod tests { joined.contains("仅本次批准"), "missing zh approve option:\n{joined}" ); - - view.handle_key(create_key_event(KeyCode::Char('y'))); - let lines = render_lines(&view, 100, 40); - let joined = compact_rendered_text(&lines); - assert!( - joined.contains("确认破坏性操作"), - "missing zh confirm banner:\n{joined}" - ); - assert!( - joined.contains("(待确认)"), - "missing zh staged marker:\n{joined}" - ); - assert!( - joined.contains("Enter或y"), - "missing zh confirm key:\n{joined}" - ); } #[test] diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index f1ebabfe..dffa442c 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -6036,31 +6036,19 @@ async fn handle_view_events( approval_key, approval_grouping_key, } => { - if decision == ReviewDecision::ApprovedForSession { - // Store the tool name (backward compat) and the lossy - // grouping key so later flag variants of the same - // command family are also auto-approved (v0.8.37). - app.approval_session_approved.insert(tool_name.clone()); - app.approval_session_approved - .insert(approval_grouping_key.clone()); - } - - match decision { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { - let _ = engine_handle.approve_tool_call(tool_id).await; - } - ReviewDecision::Denied | ReviewDecision::Abort => { - // Cache the denial so the model retry-loop doesn't - // re-prompt for the exact same approval_key (#360). - // Only the key (per-call unique) is stored — NOT - // the tool_name, which would block all future - // invocations of the same tool type (#1377). - if !timed_out { - app.approval_session_denied.insert(approval_key); - } - let _ = engine_handle.deny_tool_call(tool_id).await; - } - } + apply_approval_decision( + app, + engine_handle, + ApprovalDecisionEvent { + tool_id, + tool_name, + decision, + timed_out, + approval_key, + approval_grouping_key, + }, + ) + .await; if timed_out { app.add_message(HistoryCell::System { @@ -6333,6 +6321,52 @@ async fn handle_view_events( Ok(false) } +struct ApprovalDecisionEvent { + tool_id: String, + tool_name: String, + decision: ReviewDecision, + timed_out: bool, + approval_key: String, + approval_grouping_key: String, +} + +async fn apply_approval_decision( + app: &mut App, + engine_handle: &mut EngineHandle, + event: ApprovalDecisionEvent, +) { + if event.decision == ReviewDecision::ApprovedForSession { + // Store the tool name (backward compat) and the lossy grouping key so + // later flag variants of the same command family are also auto-approved + // (v0.8.37). + app.approval_session_approved + .insert(event.tool_name.clone()); + app.approval_session_approved + .insert(event.approval_grouping_key.clone()); + } + + match event.decision { + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + let _ = engine_handle.approve_tool_call(event.tool_id).await; + } + ReviewDecision::Denied => { + // Cache the denial so the model retry-loop doesn't re-prompt for + // the exact same approval_key (#360). Only the key (per-call + // unique) is stored — NOT the tool_name, which would block all + // future invocations of the same tool type (#1377). + if !event.timed_out { + app.approval_session_denied.insert(event.approval_key); + } + let _ = engine_handle.deny_tool_call(event.tool_id).await; + } + ReviewDecision::Abort => { + engine_handle.cancel(); + mark_active_turn_cancelled_locally(app); + app.status_message = Some("Request cancelled".to_string()); + } + } +} + fn mark_active_turn_cancelled_locally(app: &mut App) { app.is_loading = false; app.dispatch_started_at = None; diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index a8179769..05ff8a95 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1015,13 +1015,10 @@ impl Renderable for ComposerWidget<'_> { /// Codex-style full-screen approval takeover (#129). /// -/// The widget reads its mutable state (selected option, staged -/// confirmation) directly from the [`ApprovalView`] so the destructive -/// variant can render its "Press Y again to confirm" banner without -/// touching internal fields. Rendering reflows to fill most of the -/// transcript area instead of a centered popup; on small terminals it -/// falls back to a 65×22 card so existing snapshot tests still see a -/// coherent layout. +/// The widget reads its selected option and locale directly from the +/// [`ApprovalView`]. Rendering reflows to fill most of the transcript +/// area instead of a centered popup; on small terminals it falls back to +/// a 65×22 card so existing snapshot tests still see a coherent layout. pub struct ApprovalWidget<'a> { request: &'a ApprovalRequest, view: &'a ApprovalView, @@ -1038,8 +1035,8 @@ impl<'a> ApprovalWidget<'a> { /// terminal can hold. const APPROVAL_CARD_HORIZONTAL_PAD: u16 = 6; const APPROVAL_CARD_VERTICAL_PAD: u16 = 2; -/// Minimum card height — anything tighter and the destructive variant's -/// confirmation banner overlaps the option list. +/// Minimum card height — anything tighter and the approval controls +/// overlap the option list. const APPROVAL_CARD_MIN_HEIGHT: u16 = 18; /// Minimum card width — anything tighter makes approval copy wrap too /// aggressively on small terminals. @@ -1164,120 +1161,49 @@ impl Renderable for ApprovalWidget<'_> { lines.push(Line::from("")); let options = approval_options_for(risk, locale); - let pending = self.view.pending_confirm(); for (i, opt) in options.iter().enumerate() { let is_selected = i == self.view.selected(); - let staged = pending.is_some_and(|p| p == opt.option); let label_color = if opt.dangerous { palette_colors.accent } else { palette::TEXT_BODY }; - let row_style = if is_selected { - Style::default() - .fg(palette::SELECTION_TEXT) - .bg(palette::SELECTION_BG) - } else { - Style::default() - }; + let option_style = approval_option_style(is_selected, label_color); + let shortcut_style = approval_option_style(is_selected, palette_colors.shortcut); - let mut spans = vec![ + let spans = vec![ Span::raw(" "), Span::styled( format!("[{}] ", opt.key_hint), - Style::default() - .fg(palette_colors.shortcut) - .add_modifier(Modifier::BOLD), + shortcut_style.add_modifier(Modifier::BOLD), ), - Span::styled(opt.label.to_string(), row_style.fg(label_color)), + Span::styled(opt.label.to_string(), option_style), ]; - if staged { - spans.push(Span::raw(" ")); - spans.push(Span::styled( - staged_marker(locale), - Style::default() - .fg(palette_colors.accent) - .add_modifier(Modifier::BOLD), - )); - } lines.push(Line::from(spans)); } - // Variant-specific footer: benign nudges single-key approve; - // destructive shows either the standing prompt or the - // confirmation banner when an approve key has been staged. + // Footer: Enter commits the highlighted row; y/a/d remain direct + // shortcuts for users who do not want to move the selection. lines.push(Line::from("")); - match (risk, pending) { - (RiskLevel::Benign, _) => { - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - single_key_prefix(locale), - Style::default().fg(palette::TEXT_HINT), - ), - Span::styled( - single_key_value(locale), - Style::default() - .fg(palette_colors.accent) - .add_modifier(Modifier::BOLD), - ), - Span::styled( - footer_controls(locale), - Style::default().fg(palette::TEXT_HINT), - ), - ])); - } - (RiskLevel::Destructive, Some(opt)) => { - let again_key = match opt { - crate::tui::approval::ApprovalOption::ApproveOnce => confirm_key_once(locale), - crate::tui::approval::ApprovalOption::ApproveAlways => { - confirm_key_always(locale) - } - _ => "Enter", - }; - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - destructive_confirm_prefix(locale), - Style::default() - .fg(palette_colors.accent) - .add_modifier(Modifier::BOLD), - ), - Span::styled( - again_key.to_string(), - Style::default() - .fg(palette::DEEPSEEK_INK) - .bg(palette_colors.accent) - .add_modifier(Modifier::BOLD), - ), - Span::styled( - destructive_confirm_suffix(locale), - Style::default().fg(palette::TEXT_HINT), - ), - ])); - } - (RiskLevel::Destructive, None) => { - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - two_key_prefix(locale), - Style::default().fg(palette::TEXT_HINT), - ), - Span::styled( - two_key_value(locale), - Style::default() - .fg(palette_colors.accent) - .add_modifier(Modifier::BOLD), - ), - Span::styled( - footer_controls(locale), - Style::default().fg(palette::TEXT_HINT), - ), - ])); - } - } + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + selection_hint_prefix(locale), + Style::default().fg(palette::TEXT_HINT), + ), + Span::styled( + selection_hint_value(locale), + Style::default() + .fg(palette_colors.accent) + .add_modifier(Modifier::BOLD), + ), + Span::styled( + footer_controls(locale), + Style::default().fg(palette::TEXT_HINT), + ), + ])); let title = format!( " {} {} — {} ", @@ -1375,6 +1301,21 @@ fn approval_palette(risk: RiskLevel) -> ApprovalColors { } } +fn approval_selected_style() -> Style { + Style::default() + .fg(palette::SELECTION_TEXT) + .bg(palette::DEEPSEEK_BLUE) + .add_modifier(Modifier::BOLD) +} + +fn approval_option_style(is_selected: bool, color: Color) -> Style { + if is_selected { + approval_selected_style() + } else { + Style::default().fg(color) + } +} + fn risk_badge_text(risk: RiskLevel, locale: Locale) -> &'static str { match (locale, risk) { (Locale::ZhHans, RiskLevel::Benign) => "审查", @@ -1438,24 +1379,6 @@ fn label_params(locale: Locale) -> &'static str { } } -fn staged_marker(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "(待确认)", - _ => "(staged)", - } -} - -fn single_key_prefix(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "单键批准：", - _ => "Single key approves: ", - } -} - -fn single_key_value(_locale: Locale) -> &'static str { - "Enter / 1 / y" -} - fn footer_controls(locale: Locale) -> &'static str { match locale { Locale::ZhHans => " · v：完整参数 · Esc：终止", @@ -1463,79 +1386,45 @@ fn footer_controls(locale: Locale) -> &'static str { } } -fn destructive_confirm_prefix(locale: Locale) -> &'static str { +fn selection_hint_prefix(locale: Locale) -> &'static str { match locale { - Locale::ZhHans => "确认破坏性操作：再次按 ", - _ => "Confirm destructive action — press ", + Locale::ZhHans => "选择：", + _ => "Choose: ", } } -fn destructive_confirm_suffix(locale: Locale) -> &'static str { +fn selection_hint_value(locale: Locale) -> &'static str { match locale { - Locale::ZhHans => " 执行；按其他键取消。", - _ => " again to commit, anything else cancels.", - } -} - -fn confirm_key_once(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "Enter 或 y", - _ => "Enter or y", - } -} - -fn confirm_key_always(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "Enter 或 a", - _ => "Enter or a", - } -} - -fn two_key_prefix(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "两次按键确认：", - _ => "Two keys to approve: ", - } -} - -fn two_key_value(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "先按 y/a，再按一次 y/a", - _ => "y/a then y/a again", + Locale::ZhHans => "Enter 执行选中项，或直接按 y/a/d", + _ => "Enter selected option, or press y/a/d directly", } } struct ApprovalOptionRow { - option: crate::tui::approval::ApprovalOption, label: &'static str, key_hint: &'static str, dangerous: bool, } fn approval_options_for(risk: RiskLevel, locale: Locale) -> [ApprovalOptionRow; 4] { - use crate::tui::approval::ApprovalOption as O; let dangerous = matches!(risk, RiskLevel::Destructive); [ ApprovalOptionRow { - option: O::ApproveOnce, label: option_approve_once(locale), key_hint: "1 / y", dangerous, }, ApprovalOptionRow { - option: O::ApproveAlways, label: option_approve_always(locale), key_hint: "2 / a", dangerous, }, ApprovalOptionRow { - option: O::Deny, label: option_deny(locale), key_hint: "3 / d / n", dangerous: false, }, ApprovalOptionRow { - option: O::Abort, label: option_abort(locale), key_hint: "Esc", dangerous: false, @@ -3479,6 +3368,43 @@ mod tests { } } + #[test] + fn approval_selected_destructive_option_uses_contrasting_highlight() { + let request = crate::tui::approval::ApprovalRequest::new( + "approval-1", + "exec_shell", + "Run git commit", + &serde_json::json!({ "command": "git commit -m fix" }), + "exec_shell:git commit", + ); + let view = crate::tui::approval::ApprovalView::new(request.clone()); + let widget = ApprovalWidget::new(&request, &view); + let area = Rect::new(0, 0, 100, 30); + let mut buf = Buffer::empty(area); + + widget.render(area, &mut buf); + + let selected_row = (area.y..area.y.saturating_add(area.height)) + .find(|&y| { + (area.x..area.x.saturating_add(area.width)) + .any(|x| buf[(x, y)].bg == palette::DEEPSEEK_BLUE) + }) + .expect("selected approval row should use blue background"); + let highlighted_cells = (area.x..area.x.saturating_add(area.width)) + .filter(|&x| { + let cell = &buf[(x, selected_row)]; + !cell.symbol().trim().is_empty() + && cell.bg == palette::DEEPSEEK_BLUE + && cell.fg == palette::SELECTION_TEXT + }) + .count(); + + assert!( + highlighted_cells >= 4, + "selected destructive option should render visible blue/white text" + ); + } + /// Regression for issue #65: after `App::handle_resize`, the chat widget /// must produce a clean render at the new width — no stale wrapping, /// no panic, no content exceeding the requested width. Cycling through