From 4b7e0c1a7a68676e81c624381a4082a8edbc7360 Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Mon, 18 May 2026 16:35:53 +0800 Subject: [PATCH 1/7] fix(tui): show key tool details prominently in approval popup Replace the verbose approval popup (About, generic impacts, raw JSON params) with a focused display that highlights the most important information: Command for shell, File for writes, Target for network. - Add prominent_details() to extract key params per tool category - Pass workspace path to annotate current directory as "(current)" - Pass tool input through ApprovalRequired event instead of lookup - Show "Confirm: " in the two-step confirmation footer - Remove generic description/impacts fields from ApprovalRequest --- crates/tui/src/core/engine/turn_loop.rs | 1 + crates/tui/src/core/events.rs | 1 + crates/tui/src/runtime_threads.rs | 4 + crates/tui/src/tui/approval.rs | 390 ++++++++---------------- crates/tui/src/tui/ui.rs | 15 +- crates/tui/src/tui/widgets/mod.rs | 163 +++++----- 6 files changed, 224 insertions(+), 350 deletions(-) diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index c5b099ed2..f8f1b7de4 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1660,6 +1660,7 @@ impl Engine { .send(Event::ApprovalRequired { id: tool_id.clone(), tool_name: tool_name.clone(), + input: tool_input.clone(), description: plan.approval_description.clone(), approval_key, approval_grouping_key, diff --git a/crates/tui/src/core/events.rs b/crates/tui/src/core/events.rs index b02ba2f99..22f0172e7 100644 --- a/crates/tui/src/core/events.rs +++ b/crates/tui/src/core/events.rs @@ -225,6 +225,7 @@ pub enum Event { ApprovalRequired { id: String, tool_name: String, + input: serde_json::Value, description: String, /// Exact-argument fingerprint, used to scope *denials* (#1617). approval_key: String, diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 787142ba4..6afe78881 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -4160,6 +4160,7 @@ mod tests { approval_grouping_key: "test_key".to_string(), id: "tool_stale".to_string(), tool_name: "exec_command".to_string(), + input: json!({"command": "stale approval"}), description: "stale approval".to_string(), }) .await?; @@ -4233,6 +4234,7 @@ mod tests { approval_grouping_key: "key1".to_string(), id: "tool_external_allow".to_string(), tool_name: "exec_command".to_string(), + input: json!({"command": "external allow"}), description: "external allow".to_string(), }) .await?; @@ -4310,6 +4312,7 @@ mod tests { approval_grouping_key: "key2".to_string(), id: "tool_external_deny".to_string(), tool_name: "exec_command".to_string(), + input: json!({"command": "external deny"}), description: "external deny".to_string(), }) .await?; @@ -4496,6 +4499,7 @@ mod tests { approval_grouping_key: "key3".to_string(), id: "tool_remember".to_string(), tool_name: "exec_command".to_string(), + input: json!({"command": "remember=true"}), description: "remember=true".to_string(), }) .await?; diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index dc7e9cdcd..fdf49ac37 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -120,14 +120,10 @@ pub struct ApprovalRequest { pub id: String, /// Tool being executed pub tool_name: String, - /// Human-readable tool description from the engine - pub description: String, /// Tool category pub category: ToolCategory, /// Stakes-based routing for the takeover modal pub risk: RiskLevel, - /// Derived impact summary for the approval prompt - pub impacts: Vec, /// Tool parameters (for display) pub params: Value, /// Exact-argument fingerprint, used to scope *denials* (#1617). @@ -135,15 +131,29 @@ pub struct ApprovalRequest { /// Lossy / arity-aware fingerprint, used to scope *approvals* so an /// "approve for session" covers later flag variants (v0.8.37). pub approval_grouping_key: String, + /// Current workspace directory, used to annotate cwd as "(current)". + pub workspace: Option, } impl ApprovalRequest { + #[cfg(test)] pub fn new( id: &str, tool_name: &str, description: &str, params: &Value, approval_key: &str, + ) -> Self { + Self::new_with_workspace(id, tool_name, description, params, approval_key, None) + } + + pub fn new_with_workspace( + id: &str, + tool_name: &str, + _description: &str, + params: &Value, + approval_key: &str, + workspace: Option, ) -> Self { let category = get_tool_category(tool_name); let risk = classify_risk(tool_name, category, params); @@ -153,36 +163,68 @@ impl ApprovalRequest { Self { id: id.to_string(), tool_name: tool_name.to_string(), - description: description.to_string(), category, risk, - impacts: build_impact_summary(tool_name, category, params), params: params.clone(), approval_key: approval_key.to_string(), approval_grouping_key, + workspace, } } - /// Format parameters for display (truncated) - pub fn params_display(&self) -> String { - let truncated = truncate_params_value(&self.params, 200); - serde_json::to_string(&truncated).unwrap_or_else(|_| truncated.to_string()) - } - - pub fn description_for_locale(&self, locale: Locale) -> String { - match locale { - Locale::ZhHans => localized_description_zh_hans(self.category), - _ => self.description.clone(), - } - } - - pub fn impacts_for_locale(&self, locale: Locale) -> Vec { - match locale { - Locale::ZhHans => { - build_impact_summary_zh_hans(&self.tool_name, self.category, &self.params) + /// Extract the most important param values as (label, value) pairs for + /// prominent display in the approval widget. Returns pairs like + /// `[("Command", "npm run build"), ("Dir", "/home/user")]`. + pub fn prominent_details(&self) -> Vec<(String, String)> { + let mut details = Vec::new(); + match self.category { + ToolCategory::Shell => { + if let Some(cmd) = param_preview(&self.params, &["cmd", "command"], 120) { + details.push(("Command".into(), cmd)); + } + if let Some(dir) = param_preview(&self.params, &["workdir", "cwd"], 48) { + let is_current = self + .workspace + .as_ref() + .is_some_and(|ws| std::path::Path::new(&dir) == std::path::Path::new(ws)); + let label = if is_current { + "(current)".to_string() + } else { + dir + }; + details.push(("Dir".into(), label)); + } + } + ToolCategory::FileWrite => { + if let Some(path) = + param_preview(&self.params, &["path", "target", "destination"], 96) + { + details.push(("File".into(), path)); + } + } + ToolCategory::Safe => { + if let Some(path) = param_preview(&self.params, &["path", "ref_id", "uri"], 96) { + details.push(("Path".into(), path)); + } + } + ToolCategory::Network => { + if let Some(target) = + param_preview(&self.params, &["url", "q", "query", "location", "repo"], 96) + { + details.push(("Target".into(), target)); + } + } + _ => { + if let Some(val) = param_preview( + &self.params, + &["command", "path", "url", "q", "query", "ref_id"], + 96, + ) { + details.push(("Input".into(), val)); + } } - _ => self.impacts.clone(), } + details } } @@ -192,7 +234,10 @@ pub fn get_tool_category(name: &str) -> ToolCategory { ToolCategory::FileWrite } else if matches!(name, "web_run" | "web_search" | "fetch_url") { ToolCategory::Network - } else if name == "exec_shell" { + } else if matches!( + name, + "exec_shell" | "task_shell_start" | "exec_shell_wait" | "exec_shell_interact" + ) { ToolCategory::Shell } else if name.starts_with("list_mcp_") || name.starts_with("read_mcp_") @@ -246,7 +291,11 @@ pub fn classify_risk(tool_name: &str, category: ToolCategory, params: &Value) -> // shape so a future routing tweak (say, pure-readonly `ls` // staying benign) lands here without a second pass. ToolCategory::Shell => { - if let Some(cmd) = params.get("command").and_then(Value::as_str) { + if let Some(cmd) = params + .get("command") + .or_else(|| params.get("cmd")) + .and_then(Value::as_str) + { let _ = crate::command_safety::analyze_command(cmd); } RiskLevel::Destructive @@ -291,163 +340,6 @@ fn param_preview(params: &Value, keys: &[&str], max_len: usize) -> Option Option { - let remainder = tool_name.strip_prefix("mcp_")?; - let (server, _) = remainder.split_once('_')?; - if server.is_empty() { - None - } else { - Some(server.to_string()) - } -} - -fn build_impact_summary(tool_name: &str, category: ToolCategory, params: &Value) -> Vec { - match category { - ToolCategory::Safe => { - let mut impacts = vec!["Read-only operation.".to_string()]; - if let Some(path) = param_preview(params, &["path", "ref_id", "uri"], 72) { - impacts.push(format!("Reads: {path}")); - } - impacts - } - ToolCategory::FileWrite => { - let mut impacts = - vec!["Writes files in the workspace or an approved write scope.".to_string()]; - if let Some(path) = param_preview(params, &["path", "target", "destination"], 72) { - impacts.push(format!("Writes: {path}")); - } - impacts - } - ToolCategory::Shell => { - let mut impacts = vec!["Executes a shell command.".to_string()]; - if let Some(command) = param_preview(params, &["cmd", "command"], 96) { - impacts.push(format!("Command: {command}")); - } - if let Some(workdir) = param_preview(params, &["workdir", "cwd"], 72) { - impacts.push(format!("Working dir: {workdir}")); - } - impacts - } - ToolCategory::Network => { - let mut impacts = vec!["May reach network services or remote content.".to_string()]; - if let Some(target) = - param_preview(params, &["url", "q", "query", "location", "repo"], 96) - { - impacts.push(format!("Target: {target}")); - } - impacts - } - ToolCategory::McpRead => { - let mut impacts = - vec!["Reads from an MCP server without an obvious local write.".to_string()]; - if let Some(server) = mcp_server_hint(tool_name) { - impacts.push(format!("Server: {server}")); - } - impacts - } - ToolCategory::McpAction => { - let mut impacts = - vec!["Calls an MCP server action that may have side effects.".to_string()]; - if let Some(server) = mcp_server_hint(tool_name) { - impacts.push(format!("Server: {server}")); - } - impacts - } - ToolCategory::Unknown => { - let mut impacts = vec![ - "Tool is not classified. Review params carefully before approving.".to_string(), - ]; - if let Some(target) = param_preview( - params, - &["path", "cmd", "command", "url", "q", "query", "ref_id"], - 96, - ) { - impacts.push(format!("Primary input: {target}")); - } - impacts - } - } -} - -fn localized_description_zh_hans(category: ToolCategory) -> String { - match category { - ToolCategory::Safe => "请求执行只读操作。".to_string(), - ToolCategory::FileWrite => "请求修改文件。请确认路径和内容符合预期。".to_string(), - ToolCategory::Shell => "请求执行 shell 命令。请先检查命令和工作目录。".to_string(), - ToolCategory::Network => "请求访问网络或远程内容。请确认目标可信。".to_string(), - ToolCategory::McpRead => "请求从 MCP 服务器读取信息。".to_string(), - ToolCategory::McpAction => "请求调用 MCP 服务器操作，可能产生副作用。".to_string(), - ToolCategory::Unknown => "请求运行未分类工具。批准前请仔细检查参数。".to_string(), - } -} - -fn build_impact_summary_zh_hans( - tool_name: &str, - category: ToolCategory, - params: &Value, -) -> Vec { - match category { - ToolCategory::Safe => { - let mut impacts = vec!["只读操作。".to_string()]; - if let Some(path) = param_preview(params, &["path", "ref_id", "uri"], 72) { - impacts.push(format!("读取：{path}")); - } - impacts - } - ToolCategory::FileWrite => { - let mut impacts = vec!["会写入工作区或已批准写入范围内的文件。".to_string()]; - if let Some(path) = param_preview(params, &["path", "target", "destination"], 72) { - impacts.push(format!("写入：{path}")); - } - impacts - } - ToolCategory::Shell => { - let mut impacts = vec!["执行 shell 命令。".to_string()]; - if let Some(command) = param_preview(params, &["cmd", "command"], 96) { - impacts.push(format!("命令：{command}")); - } - if let Some(workdir) = param_preview(params, &["workdir", "cwd"], 72) { - impacts.push(format!("工作目录：{workdir}")); - } - impacts - } - ToolCategory::Network => { - let mut impacts = vec!["可能访问网络服务或远程内容。".to_string()]; - if let Some(target) = - param_preview(params, &["url", "q", "query", "location", "repo"], 96) - { - impacts.push(format!("目标：{target}")); - } - impacts - } - ToolCategory::McpRead => { - let mut impacts = vec!["从 MCP 服务器读取信息，不应产生本地写入。".to_string()]; - if let Some(server) = mcp_server_hint(tool_name) { - impacts.push(format!("服务器：{server}")); - } - impacts - } - ToolCategory::McpAction => { - let mut impacts = vec!["调用可能产生副作用的 MCP 服务器操作。".to_string()]; - if let Some(server) = mcp_server_hint(tool_name) { - impacts.push(format!("服务器：{server}")); - } - impacts - } - ToolCategory::Unknown => { - let mut impacts = vec!["工具未分类。批准前请仔细检查参数。".to_string()]; - if let Some(target) = param_preview( - params, - &["path", "cmd", "command", "url", "q", "query", "ref_id"], - 96, - ) { - impacts.push(format!("主要输入：{target}")); - } - impacts - } - } -} - /// 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. @@ -688,34 +580,6 @@ impl ModalView for ApprovalView { } } -fn truncate_params_value(value: &Value, max_len: usize) -> Value { - match value { - Value::Object(map) => { - let truncated = map - .iter() - .map(|(key, val)| (key.clone(), truncate_params_value(val, max_len))) - .collect(); - Value::Object(truncated) - } - Value::Array(items) => { - let truncated_items = items - .iter() - .map(|val| truncate_params_value(val, max_len)) - .collect(); - Value::Array(truncated_items) - } - Value::String(text) => Value::String(truncate_string_value(text, max_len)), - other => { - let rendered = other.to_string(); - if rendered.chars().count() > max_len { - Value::String(truncate_string_value(&rendered, max_len)) - } else { - other.clone() - } - } - } -} - fn truncate_string_value(value: &str, max_len: usize) -> String { if value.chars().count() <= max_len { return value.to_string(); @@ -998,6 +862,12 @@ mod tests { #[test] fn test_get_tool_category_shell_tools() { assert_eq!(get_tool_category("exec_shell"), ToolCategory::Shell); + assert_eq!(get_tool_category("task_shell_start"), ToolCategory::Shell); + assert_eq!(get_tool_category("exec_shell_wait"), ToolCategory::Shell); + assert_eq!( + get_tool_category("exec_shell_interact"), + ToolCategory::Shell + ); assert_eq!( get_tool_category("mcp_linear_save_issue"), ToolCategory::McpAction @@ -1097,61 +967,46 @@ mod tests { } #[test] - fn test_approval_request_params_display_truncates() { - let long_content = "x".repeat(300); - let params = json!({"path": "src/main.rs", "content": long_content}); + fn test_prominent_details_shell() { + let params = json!({"command": "npm run build", "cwd": "/home/user"}); let request = ApprovalRequest::new( "test-id", - "write_file", - "Write a file to disk", - ¶ms, - "test_key", - ); - - let display = request.params_display(); - assert!(display.len() < 250); - assert!(display.contains("src/main.rs")); - } - - #[test] - fn test_approval_request_params_display_short() { - let params = json!({"path": "src/main.rs"}); - let request = ApprovalRequest::new( - "test-id", - "read_file", - "Read a file from disk", + "exec_shell", + "Run a shell command", ¶ms, "test_key", ); - - let display = request.params_display(); - assert!(display.contains("src/main.rs")); + let details = request.prominent_details(); + assert_eq!(details[0].0, "Command"); + assert_eq!(details[0].1, "npm run build"); + assert_eq!(details[1].0, "Dir"); + assert_eq!(details[1].1, "/home/user"); } #[test] - fn test_approval_request_derives_impact_summary() { - let params = json!({"cmd": "cargo test", "workdir": "/tmp/project"}); - let request = ApprovalRequest::new( + fn test_prominent_details_shell_marks_current_dir() { + let params = json!({"command": "ls", "cwd": "/home/user/project"}); + let request = ApprovalRequest::new_with_workspace( "test-id", "exec_shell", "Run a shell command", ¶ms, "test_key", + Some("/home/user/project".to_string()), ); + let details = request.prominent_details(); + assert_eq!(details[1].0, "Dir"); + assert_eq!(details[1].1, "(current)"); + } - assert_eq!(request.category, ToolCategory::Shell); - assert!( - request - .impacts - .iter() - .any(|line| line.contains("Executes a shell command")) - ); - assert!( - request - .impacts - .iter() - .any(|line| line.contains("cargo test")) - ); + #[test] + fn test_prominent_details_file_write() { + let params = json!({"path": "src/main.rs", "content": "fn main() {}"}); + let request = + ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); + let details = request.prominent_details(); + assert_eq!(details[0].0, "File"); + assert_eq!(details[0].1, "src/main.rs"); } // ======================================================================== @@ -1566,7 +1421,9 @@ mod tests { joined.contains("Single key approves"), "benign hint missing:\n{joined}" ); - assert!(joined.contains("read_file")); + assert!(joined.contains("Safe")); + assert!(joined.contains("Path")); + assert!(joined.contains("src/main.rs")); } #[test] @@ -1582,7 +1439,13 @@ mod tests { joined.contains("Two keys to approve"), "destructive hint missing:\n{joined}" ); - assert!(joined.contains("write_file")); + assert!(joined.contains("File Write")); + assert!(joined.contains("File")); + assert!(joined.contains("src/main.rs")); + assert!( + joined.contains("Approve file writes this session"), + "session approval label missing:\n{joined}" + ); } #[test] @@ -1595,6 +1458,10 @@ mod tests { joined.contains("Confirm destructive action"), "confirm banner missing:\n{joined}" ); + assert!( + joined.contains("Confirm file:"), + "confirm detail missing:\n{joined}" + ); assert!( joined.contains("(staged)"), "stage marker missing:\n{joined}" @@ -1618,18 +1485,19 @@ mod tests { joined.contains("文件写入"), "missing zh category:\n{joined}" ); + assert!(joined.contains("File"), "missing file label:\n{joined}"); assert!( - joined.contains("影响："), - "missing zh impact label:\n{joined}" - ); - assert!( - joined.contains("写入：src/main.rs"), - "missing zh impact path:\n{joined}" + joined.contains("src/main.rs"), + "missing file path:\n{joined}" ); assert!( joined.contains("仅本次批准"), "missing zh approve option:\n{joined}" ); + assert!( + joined.contains("本会话自动批准文件写入"), + "missing zh session approve option:\n{joined}" + ); view.handle_key(create_key_event(KeyCode::Char('y'))); let lines = render_lines(&view, 100, 40); @@ -1638,6 +1506,10 @@ mod tests { joined.contains("确认破坏性操作"), "missing zh confirm banner:\n{joined}" ); + assert!( + joined.contains("确认文件："), + "missing zh confirm detail:\n{joined}" + ); assert!( joined.contains("(待确认)"), "missing zh staged marker:\n{joined}" diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index ef3c2a386..fc2504f39 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -1771,6 +1771,7 @@ async fn run_event_loop( EngineEvent::ApprovalRequired { id, tool_name, + input, description, approval_key, approval_grouping_key, @@ -1816,24 +1817,18 @@ async fn run_event_loop( app.status_message = Some(format!("Blocked tool '{tool_name}' (approval_mode=never)")); } else { - let tool_input = app - .pending_tool_uses - .iter() - .find(|(tool_id, _, _)| tool_id == &id) - .map(|(_, _, input)| input.clone()) - .unwrap_or_else(|| serde_json::json!({})); - if tool_name == "apply_patch" { - maybe_add_patch_preview(app, &tool_input); + maybe_add_patch_preview(app, &input); } // Create approval request and show overlay - let request = ApprovalRequest::new( + let request = ApprovalRequest::new_with_workspace( &id, &tool_name, &description, - &tool_input, + &input, &approval_key, + Some(app.workspace.display().to_string()), ); log_sensitive_event( "tool.approval.prompted", diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 226ad72a3..dee4e68da 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1049,13 +1049,13 @@ 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. -const APPROVAL_CARD_MIN_HEIGHT: u16 = 18; +const APPROVAL_CARD_MIN_HEIGHT: u16 = 16; /// Minimum card width — anything tighter makes approval copy wrap too /// aggressively on small terminals. const APPROVAL_CARD_MIN_WIDTH: u16 = 40; /// Maximum card height — taller cards stop reading like a focused /// takeover and waste vertical space on large terminals. -const APPROVAL_CARD_MAX_HEIGHT: u16 = 28; +const APPROVAL_CARD_MAX_HEIGHT: u16 = 18; /// Maximum card width — readability craters past this on wide terminals. const APPROVAL_CARD_MAX_WIDTH: u16 = 96; @@ -1098,9 +1098,10 @@ impl Renderable for ApprovalWidget<'_> { let palette_colors = approval_palette(risk); let mut lines: Vec> = Vec::with_capacity(20); - // Header: stakes badge + tool identifier. The badge is the - // first thing the eye lands on. + // Header: stakes badge + plain category. Keep the tool id out of + // the first read path so the command/path remains the focus. lines.push(Line::from("")); + let (cat_label, cat_color) = category_label_for(self.request.category, locale); lines.push(Line::from(vec![ Span::raw(" "), Span::styled( @@ -1111,20 +1112,6 @@ impl Renderable for ApprovalWidget<'_> { .add_modifier(Modifier::BOLD), ), Span::raw(" "), - Span::styled( - self.request.tool_name.clone(), - Style::default() - .fg(palette::DEEPSEEK_SKY) - .add_modifier(Modifier::BOLD), - ), - ])); - - // Category line — English remains the baseline while localized - // sessions get the same risk category in their UI language. - let (cat_label, cat_color) = category_label_for(self.request.category, locale); - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled(label_type(locale), Style::default().fg(palette::TEXT_HINT)), Span::styled( cat_label, Style::default().fg(cat_color).add_modifier(Modifier::BOLD), @@ -1132,47 +1119,43 @@ impl Renderable for ApprovalWidget<'_> { ])); lines.push(Line::from("")); - // About + impacts. Impact lines are the load-bearing content; - // they tell the user what will happen. - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled(label_about(locale), Style::default().fg(palette::TEXT_HINT)), - Span::styled( - self.request.description_for_locale(locale), - Style::default().fg(palette::TEXT_BODY), - ), - ])); - for impact in self.request.impacts_for_locale(locale).into_iter().take(4) { + // Prominent key details (command, path, target) are the default + // body. Full JSON stays behind `v`. + let details = self.request.prominent_details(); + if details.is_empty() { lines.push(Line::from(vec![ Span::raw(" "), + Span::styled(label_tool(locale), Style::default().fg(palette::TEXT_HINT)), Span::styled( - label_impact(locale), - Style::default().fg(palette::TEXT_HINT), + self.request.tool_name.clone(), + Style::default() + .fg(palette::TEXT_BODY) + .add_modifier(Modifier::BOLD), ), - Span::styled(impact, Style::default().fg(palette::TEXT_BODY)), ])); + } else { + for (label, value) in &details { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + format!("{label:<7} "), + Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD), + ), + Span::styled( + value.clone(), + Style::default() + .fg(palette::TEXT_BODY) + .add_modifier(Modifier::BOLD), + ), + ])); + } } - lines.push(Line::from("")); - let params_str = self.request.params_display(); - let params_width = card_area.width.saturating_sub(14) as usize; - let params_truncated = - crate::utils::truncate_with_ellipsis(¶ms_str, params_width.max(20), "..."); - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - label_params(locale), - Style::default().fg(palette::TEXT_HINT), - ), - Span::styled( - params_truncated, - Style::default().fg(palette::TEXT_SECONDARY), - ), - ])); - lines.push(Line::from("")); - let options = approval_options_for(risk, locale); + let options = approval_options_for(self.request, risk, locale); let pending = self.view.pending_confirm(); for (i, opt) in options.iter().enumerate() { @@ -1200,7 +1183,7 @@ impl Renderable for ApprovalWidget<'_> { .fg(palette_colors.shortcut) .add_modifier(Modifier::BOLD), ), - Span::styled(opt.label.to_string(), row_style.fg(label_color)), + Span::styled(opt.label.clone(), row_style.fg(label_color)), ]; if staged { spans.push(Span::raw(" ")); @@ -1246,6 +1229,23 @@ impl Renderable for ApprovalWidget<'_> { } _ => "Enter", }; + // Show what is being confirmed so the user never loses + // context between the selection and the commit step. + if let Some((label, value)) = details.first() { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + confirm_label(locale, label), + Style::default().fg(palette::TEXT_BODY), + ), + Span::styled( + value.clone(), + Style::default() + .fg(palette::TEXT_BODY) + .add_modifier(Modifier::BOLD), + ), + ])); + } lines.push(Line::from(vec![ Span::raw(" "), Span::styled( @@ -1419,31 +1419,24 @@ fn approval_word(locale: Locale) -> &'static str { } } -fn label_type(locale: Locale) -> &'static str { +fn label_tool(locale: Locale) -> &'static str { match locale { - Locale::ZhHans => "类型：", - _ => "Type: ", + Locale::ZhHans => "工具 ", + _ => "Tool ", } } -fn label_about(locale: Locale) -> &'static str { +fn confirm_label(locale: Locale, detail_label: &str) -> String { match locale { - Locale::ZhHans => "说明：", - _ => "About: ", - } -} - -fn label_impact(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "影响：", - _ => "Impact: ", - } -} - -fn label_params(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "参数：", - _ => "Params: ", + Locale::ZhHans => match detail_label { + "Command" => "确认命令：".to_string(), + "File" => "确认文件：".to_string(), + "Dir" => "确认目录：".to_string(), + "Target" => "确认目标：".to_string(), + "Path" => "确认路径：".to_string(), + _ => format!("确认{detail_label}："), + }, + _ => format!("Confirm {}: ", detail_label.to_ascii_lowercase()), } } @@ -1516,36 +1509,40 @@ fn two_key_value(locale: Locale) -> &'static str { struct ApprovalOptionRow { option: crate::tui::approval::ApprovalOption, - label: &'static str, + label: String, key_hint: &'static str, dangerous: bool, } -fn approval_options_for(risk: RiskLevel, locale: Locale) -> [ApprovalOptionRow; 4] { +fn approval_options_for( + request: &ApprovalRequest, + 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), + label: option_approve_once(locale).to_string(), key_hint: "1 / y", dangerous, }, ApprovalOptionRow { option: O::ApproveAlways, - label: option_approve_always(locale), + label: option_approve_always(request.category, locale), key_hint: "2 / a", dangerous, }, ApprovalOptionRow { option: O::Deny, - label: option_deny(locale), + label: option_deny(locale).to_string(), key_hint: "3 / d / n", dangerous: false, }, ApprovalOptionRow { option: O::Abort, - label: option_abort(locale), + label: option_abort(locale).to_string(), key_hint: "Esc", dangerous: false, }, @@ -1559,10 +1556,14 @@ fn option_approve_once(locale: Locale) -> &'static str { } } -fn option_approve_always(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "本会话同类自动批准", - _ => "Approve always for this kind", +fn option_approve_always(category: ToolCategory, locale: Locale) -> String { + match (locale, category) { + (Locale::ZhHans, ToolCategory::Shell) => "本会话自动批准 Shell 命令".to_string(), + (Locale::ZhHans, ToolCategory::FileWrite) => "本会话自动批准文件写入".to_string(), + (Locale::ZhHans, _) => "本会话同类自动批准".to_string(), + (_, ToolCategory::Shell) => "Approve shell commands this session".to_string(), + (_, ToolCategory::FileWrite) => "Approve file writes this session".to_string(), + _ => "Approve this kind this session".to_string(), } } From b0402be31ba7ad3bbb5f4877334166692bd61a80 Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Mon, 18 May 2026 16:51:26 +0800 Subject: [PATCH 2/7] fix: address review feedback on approval popup - Normalize key param ordering: command before cmd in prominent_details - Canonicalize paths for workspace dir comparison - Add prominent_details_for_locale with Chinese label translations - Match both English and Chinese labels in confirm_label - Update zh-hans test to match localized output --- crates/tui/src/tui/approval.rs | 36 +++++++++++++++++++++++++------ crates/tui/src/tui/widgets/mod.rs | 13 +++++------ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index fdf49ac37..a3437b2bd 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -179,14 +179,16 @@ impl ApprovalRequest { let mut details = Vec::new(); match self.category { ToolCategory::Shell => { - if let Some(cmd) = param_preview(&self.params, &["cmd", "command"], 120) { + if let Some(cmd) = param_preview(&self.params, &["command", "cmd"], 120) { details.push(("Command".into(), cmd)); } if let Some(dir) = param_preview(&self.params, &["workdir", "cwd"], 48) { - let is_current = self - .workspace - .as_ref() - .is_some_and(|ws| std::path::Path::new(&dir) == std::path::Path::new(ws)); + let is_current = self.workspace.as_ref().is_some_and(|ws| { + let a = std::path::Path::new(&dir); + let b = std::path::Path::new(ws); + a.canonicalize().unwrap_or_else(|_| a.to_path_buf()) + == b.canonicalize().unwrap_or_else(|_| b.to_path_buf()) + }); let label = if is_current { "(current)".to_string() } else { @@ -226,6 +228,28 @@ impl ApprovalRequest { } details } + + /// Like [`prominent_details`] but with localized labels. + pub fn prominent_details_for_locale(&self, locale: Locale) -> Vec<(String, String)> { + self.prominent_details() + .into_iter() + .map(|(label, value)| { + let localized = match locale { + Locale::ZhHans => match label.as_str() { + "Command" => "命令", + "Dir" => "目录", + "File" => "文件", + "Path" => "路径", + "Target" => "目标", + "Input" => "输入", + _ => &label, + }, + _ => &label, + }; + (localized.to_string(), value) + }) + .collect() + } } /// Get the category for a tool by name @@ -1485,7 +1509,7 @@ mod tests { joined.contains("文件写入"), "missing zh category:\n{joined}" ); - assert!(joined.contains("File"), "missing file label:\n{joined}"); + assert!(joined.contains("文件"), "missing zh file label:\n{joined}"); assert!( joined.contains("src/main.rs"), "missing file path:\n{joined}" diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index dee4e68da..b2b618590 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1121,7 +1121,7 @@ impl Renderable for ApprovalWidget<'_> { lines.push(Line::from("")); // Prominent key details (command, path, target) are the default // body. Full JSON stays behind `v`. - let details = self.request.prominent_details(); + let details = self.request.prominent_details_for_locale(locale); if details.is_empty() { lines.push(Line::from(vec![ Span::raw(" "), @@ -1429,11 +1429,12 @@ fn label_tool(locale: Locale) -> &'static str { fn confirm_label(locale: Locale, detail_label: &str) -> String { match locale { Locale::ZhHans => match detail_label { - "Command" => "确认命令：".to_string(), - "File" => "确认文件：".to_string(), - "Dir" => "确认目录：".to_string(), - "Target" => "确认目标：".to_string(), - "Path" => "确认路径：".to_string(), + "Command" | "命令" => "确认命令：".to_string(), + "File" | "文件" => "确认文件：".to_string(), + "Dir" | "目录" => "确认目录：".to_string(), + "Target" | "目标" => "确认目标：".to_string(), + "Path" | "路径" => "确认路径：".to_string(), + "Input" | "输入" => "确认输入：".to_string(), _ => format!("确认{detail_label}："), }, _ => format!("Confirm {}: ", detail_label.to_ascii_lowercase()), From a36d8af11035caa88075389fdf70906dc9dc0c3a Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Mon, 18 May 2026 22:37:19 +0800 Subject: [PATCH 3/7] fix(tui): cache approval diff preview and resolve workspace paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The approval popup re-read files every render frame via `std::fs::read_to_string(path)` with the raw (potentially relative) path, so a `write_file` invoked from the agent against a workspace-relative path silently produced an empty preview and the whole diff panel disappeared. `apply_patch` also only inspected the `patch` field and left popups blank when callers used the `changes` array form. Replace `Option` with a cached `ApprovalDiffPreview` enum built once at request construction: - `Diff { text, added, deleted }` — normal unified diff (now with workspace-resolved paths) - `NewFile { path, content }` — write_file against a missing file - `NoChange { path }` — explicit "content matches current file" hint instead of swallowing the panel - `MissingMatch { path, text, match_count }` — edit_file search not present; render a warning + search→replace fallback The popup uses a new `render_diff_compact` that keeps `@@` hunk headers + line numbers + colour but drops the summary / `--- +++` metadata, so a 10-row preview window shows code instead of headers. apply_patch's `changes` array now produces a multi-file diff with synthetic `diff --git` headers so the same renderer path applies. Tests cover: workspace-relative path resolution, NoChange path, NewFile path, simulated-replace edit_file, MissingMatch fallback, apply_patch changes array, and the compact renderer stripping file headers. --- crates/tui/src/tui/approval.rs | 575 +++++++++++++++++++++++++++++- crates/tui/src/tui/diff_render.rs | 151 +++++++- crates/tui/src/tui/widgets/mod.rs | 223 +++++++++++- 3 files changed, 930 insertions(+), 19 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index a3437b2bd..08a7ac26d 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -31,8 +31,9 @@ use crate::localization::Locale; use crate::sandbox::SandboxPolicy; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable}; -use crossterm::event::{KeyCode, KeyEvent}; +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseEvent, MouseEventKind}; use serde_json::Value; +use std::cell::Cell; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; @@ -113,6 +114,49 @@ pub enum RiskLevel { Destructive, } +/// Cached diff preview for file-modification tools. +/// +/// Built once at `ApprovalRequest` construction time so the modal doesn't +/// re-read the file every render frame. The variants make the "nothing to +/// show" cases explicit instead of collapsing to `None` and silently hiding +/// the preview panel (#1638). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ApprovalDiffPreview { + /// Normal unified diff against an existing file (or apply_patch content). + Diff { + text: String, + added: usize, + deleted: usize, + }, + /// `write_file` against a path that doesn't exist yet — there's no old + /// content to diff against, so we show the proposed content as additions. + NewFile { path: String, content: String }, + /// Content matches the file already — render a calm "no changes" hint + /// instead of swallowing the whole preview area. + NoChange { path: String }, + /// `edit_file` search string not present in the file — render a warning + /// plus a search→replace fallback diff so the user still sees intent. + MissingMatch { + path: String, + text: String, + match_count: usize, + }, +} + +impl ApprovalDiffPreview { + /// Plain unified-diff text suitable for the pager / detail view. + /// Returns an empty string for non-diff variants. + #[must_use] + #[allow(dead_code)] // wired up by the detail pager in the follow-up commit + pub fn diff_text(&self) -> &str { + match self { + Self::Diff { text, .. } | Self::MissingMatch { text, .. } => text, + Self::NewFile { content, .. } => content, + Self::NoChange { .. } => "", + } + } +} + /// Request for user approval of a tool execution #[derive(Debug, Clone)] pub struct ApprovalRequest { @@ -131,8 +175,12 @@ pub struct ApprovalRequest { /// Lossy / arity-aware fingerprint, used to scope *approvals* so an /// "approve for session" covers later flag variants (v0.8.37). pub approval_grouping_key: String, - /// Current workspace directory, used to annotate cwd as "(current)". + /// Current workspace directory, used to annotate cwd as "(current)" and + /// to resolve relative paths when reading old file contents for diffs. pub workspace: Option, + /// Snapshot of the diff/preview state, built once at construction so + /// renderers never re-read the filesystem mid-render. + diff_preview: Option, } impl ApprovalRequest { @@ -159,6 +207,11 @@ impl ApprovalRequest { let risk = classify_risk(tool_name, category, params); let approval_grouping_key = crate::tools::approval_cache::build_approval_grouping_key(tool_name, params).0; + // Build the diff snapshot once. Renderers read this cache instead of + // hitting the filesystem each frame; relative paths resolve against + // `workspace` so a `write_file` invoked from the agent doesn't go + // looking for the file in the TUI's CWD. + let diff_preview = build_diff_preview(tool_name, params, workspace.as_deref()); Self { id: id.to_string(), @@ -169,6 +222,7 @@ impl ApprovalRequest { approval_key: approval_key.to_string(), approval_grouping_key, workspace, + diff_preview, } } @@ -229,6 +283,14 @@ impl ApprovalRequest { details } + /// Cached diff/preview snapshot, or `None` when the tool isn't a file + /// modification. Building happens once at request construction; never + /// re-reads the filesystem. + #[must_use] + pub fn diff_preview(&self) -> Option<&ApprovalDiffPreview> { + self.diff_preview.as_ref() + } + /// Like [`prominent_details`] but with localized labels. pub fn prominent_details_for_locale(&self, locale: Locale) -> Vec<(String, String)> { self.prominent_details() @@ -332,6 +394,191 @@ pub fn classify_risk(tool_name: &str, category: ToolCategory, params: &Value) -> } } +/// Resolve a tool-supplied path against the workspace when it's relative. +/// Absolute paths are returned unchanged so `write_file` to `/etc/foo` still +/// shows the right diff. The original string flows through if there's no +/// workspace context — matching the previous behavior for tests / direct +/// constructors. +fn resolve_workspace_path(raw: &str, workspace: Option<&str>) -> std::path::PathBuf { + let path = Path::new(raw); + if path.is_absolute() { + return path.to_path_buf(); + } + match workspace { + Some(ws) => Path::new(ws).join(path), + None => path.to_path_buf(), + } +} + +/// Count `+` and `-` lines in a unified diff, ignoring the `+++/---` headers. +fn count_diff_changes(diff: &str) -> (usize, usize) { + let mut added = 0usize; + let mut deleted = 0usize; + for line in diff.lines() { + if line.starts_with("+++") || line.starts_with("---") { + continue; + } + if line.starts_with('+') { + added += 1; + } else if line.starts_with('-') { + deleted += 1; + } + } + (added, deleted) +} + +/// Build the diff snapshot for an approval request. Reads the filesystem +/// at most once per request — relative paths resolve against `workspace` +/// so previews work when the agent is rooted elsewhere from the TUI's CWD. +fn build_diff_preview( + tool_name: &str, + params: &Value, + workspace: Option<&str>, +) -> Option { + match tool_name { + "edit_file" => { + let path = params.get("path")?.as_str()?; + let search = params.get("search")?.as_str()?; + let replace = params.get("replace")?.as_str()?; + let resolved = resolve_workspace_path(path, workspace); + match std::fs::read_to_string(&resolved) { + Ok(file) => { + let count = file.matches(search).count(); + if count == 0 { + // search isn't present — render the search/replace pair + // as a fallback diff so the user still sees the intent, + // and flag it so the UI can warn. + let text = + crate::tools::diff_format::make_unified_diff(path, search, replace); + Some(ApprovalDiffPreview::MissingMatch { + path: path.to_string(), + text, + match_count: 0, + }) + } else { + // Simulate the replace and diff the full file so the + // user sees the actual change in context. + let updated = file.replacen(search, replace, 1); + let text = + crate::tools::diff_format::make_unified_diff(path, &file, &updated); + if text.is_empty() { + Some(ApprovalDiffPreview::NoChange { + path: path.to_string(), + }) + } else { + let (added, deleted) = count_diff_changes(&text); + Some(ApprovalDiffPreview::Diff { + text, + added, + deleted, + }) + } + } + } + Err(_) => { + // File missing — fall back to inputs-only diff. edit_file + // would fail at execution anyway, so MissingMatch is the + // honest framing. + let text = + crate::tools::diff_format::make_unified_diff(path, search, replace); + Some(ApprovalDiffPreview::MissingMatch { + path: path.to_string(), + text, + match_count: 0, + }) + } + } + } + "write_file" => { + let path = params.get("path")?.as_str()?; + let new_content = params.get("content")?.as_str()?; + let resolved = resolve_workspace_path(path, workspace); + match std::fs::read_to_string(&resolved) { + Ok(old_content) => { + let text = crate::tools::diff_format::make_unified_diff( + path, + &old_content, + new_content, + ); + if text.is_empty() { + Some(ApprovalDiffPreview::NoChange { + path: path.to_string(), + }) + } else { + let (added, deleted) = count_diff_changes(&text); + Some(ApprovalDiffPreview::Diff { + text, + added, + deleted, + }) + } + } + Err(_) => Some(ApprovalDiffPreview::NewFile { + path: path.to_string(), + content: new_content.to_string(), + }), + } + } + "apply_patch" => { + if let Some(patch) = params.get("patch").and_then(|v| v.as_str()) { + if patch.is_empty() { + None + } else { + let (added, deleted) = count_diff_changes(patch); + Some(ApprovalDiffPreview::Diff { + text: patch.to_string(), + added, + deleted, + }) + } + } else if let Some(changes) = params.get("changes").and_then(|v| v.as_array()) { + // `changes` is an array of `{path, content}` full-file + // replacements. Build a multi-file unified diff against the + // current contents so the approval shows the same shape as + // the `patch` path. + let mut out = String::new(); + for change in changes { + let Some(path) = change.get("path").and_then(|v| v.as_str()) else { + continue; + }; + let Some(new_content) = change.get("content").and_then(|v| v.as_str()) else { + continue; + }; + let resolved = resolve_workspace_path(path, workspace); + let old_content = std::fs::read_to_string(&resolved).unwrap_or_default(); + let fragment = crate::tools::diff_format::make_unified_diff( + path, + &old_content, + new_content, + ); + if !fragment.is_empty() { + if !out.is_empty() { + out.push('\n'); + } + // Synthesize a `diff --git` header so the multi-file + // summary in `diff_render` picks the file up. + out.push_str(&format!("diff --git a/{path} b/{path}\n")); + out.push_str(&fragment); + } + } + if out.is_empty() { + None + } else { + let (added, deleted) = count_diff_changes(&out); + Some(ApprovalDiffPreview::Diff { + text: out, + added, + deleted, + }) + } + } else { + None + } + } + _ => None, + } +} + fn param_preview(params: &Value, keys: &[&str], max_len: usize) -> Option { let Value::Object(map) = params else { return None; @@ -428,6 +675,9 @@ pub struct ApprovalView { requested_at: Instant, /// Whether the approval card is collapsed to a single-line banner. pub(crate) collapsed: bool, + diff_scroll: Cell, + diff_total_lines: Cell, + diff_visible_lines: Cell, } impl ApprovalView { @@ -445,6 +695,9 @@ impl ApprovalView { timeout: None, requested_at: Instant::now(), collapsed: false, + diff_scroll: Cell::new(0), + diff_total_lines: Cell::new(0), + diff_visible_lines: Cell::new(0), } } @@ -492,6 +745,37 @@ impl ApprovalView { self.locale } + pub(crate) fn set_diff_metrics(&self, total: usize, visible: usize) -> usize { + self.diff_total_lines.set(total); + self.diff_visible_lines.set(visible); + let max_scroll = total.saturating_sub(visible); + let scroll = self.diff_scroll.get().min(max_scroll); + self.diff_scroll.set(scroll); + scroll + } + + fn scroll_diff_up(&mut self, amount: usize) { + self.diff_scroll + .set(self.diff_scroll.get().saturating_sub(amount)); + self.pending_confirm = None; + } + + fn scroll_diff_down(&mut self, amount: usize) { + let visible = self.diff_visible_lines.get(); + let max_scroll = self.diff_total_lines.get().saturating_sub(visible); + self.diff_scroll + .set((self.diff_scroll.get() + amount).min(max_scroll)); + self.pending_confirm = None; + } + + fn diff_page_height(&self) -> usize { + self.diff_visible_lines.get().max(1) + } + + fn diff_half_page_height(&self) -> usize { + self.diff_page_height().div_ceil(2).max(1) + } + /// Try to commit (or stage) the given option respecting the /// variant's confirmation policy. Returns the action the modal /// stack should apply. @@ -524,12 +808,54 @@ impl ApprovalView { } fn emit_params_pager(&self) -> ViewAction { - let content = serde_json::to_string_pretty(&self.request.params) - .unwrap_or_else(|_| self.request.params.to_string()); - ViewAction::Emit(ViewEvent::OpenTextPager { - title: format!("Tool Params: {}", self.request.tool_name), - content, - }) + // Show a readable before/after for file tools, raw JSON otherwise. + if let Some(content) = self.build_detail_view() { + ViewAction::Emit(ViewEvent::OpenTextPager { + title: format!("Details: {}", self.request.tool_name), + content, + }) + } else { + let content = serde_json::to_string_pretty(&self.request.params) + .unwrap_or_else(|_| self.request.params.to_string()); + ViewAction::Emit(ViewEvent::OpenTextPager { + title: format!("Tool Params: {}", self.request.tool_name), + content, + }) + } + } + + /// Build a human-readable before/after view for file tools. + fn build_detail_view(&self) -> Option { + match self.request.tool_name.as_str() { + "edit_file" => { + let path = self.request.params.get("path")?.as_str()?; + let search = self.request.params.get("search")?.as_str()?; + let replace = self.request.params.get("replace")?.as_str()?; + Some(format!( + "File: {path}\n\n--- Before ---\n{search}\n\n+++ After +++\n{replace}" + )) + } + "write_file" => { + let path = self.request.params.get("path")?.as_str()?; + let new_content = self.request.params.get("content")?.as_str()?; + let old_content = std::fs::read_to_string(path).unwrap_or_default(); + if old_content.is_empty() { + Some(format!( + "File: {path} (new)\n\n--- Content ---\n{new_content}" + )) + } else { + Some(format!( + "File: {path}\n\n--- Before ---\n{old_content}\n\n+++ After +++\n{new_content}" + )) + } + } + "apply_patch" => { + let path = self.request.params.get("path")?.as_str()?; + let patch = self.request.params.get("patch")?.as_str()?; + Some(format!("File: {path}\n\n{patch}")) + } + _ => None, + } } fn is_timed_out(&self) -> bool { @@ -550,11 +876,34 @@ impl ModalView for ApprovalView { } fn handle_key(&mut self, key: KeyEvent) -> ViewAction { + let ctrl = key.modifiers.contains(KeyModifiers::CONTROL); + if ctrl { + match key.code { + KeyCode::Char('u') | KeyCode::Char('U') => { + self.scroll_diff_up(self.diff_half_page_height()); + return ViewAction::None; + } + KeyCode::Char('d') | KeyCode::Char('D') => { + self.scroll_diff_down(self.diff_half_page_height()); + return ViewAction::None; + } + _ => {} + } + } + match key.code { KeyCode::Tab => { self.collapsed = !self.collapsed; ViewAction::None } + KeyCode::PageUp => { + self.scroll_diff_up(self.diff_page_height()); + ViewAction::None + } + KeyCode::PageDown => { + self.scroll_diff_down(self.diff_page_height()); + ViewAction::None + } KeyCode::Up | KeyCode::Char('k') => { self.select_prev(); ViewAction::None @@ -591,6 +940,20 @@ impl ModalView for ApprovalView { } } + fn handle_mouse(&mut self, mouse: MouseEvent) -> ViewAction { + match mouse.kind { + MouseEventKind::ScrollUp => { + self.scroll_diff_up(3); + ViewAction::None + } + MouseEventKind::ScrollDown => { + self.scroll_diff_down(3); + ViewAction::None + } + _ => ViewAction::None, + } + } + fn render(&self, area: ratatui::layout::Rect, buf: &mut ratatui::buffer::Buffer) { let approval_widget = ApprovalWidget::new(&self.request, self); approval_widget.render(area, buf); @@ -1033,6 +1396,202 @@ mod tests { assert_eq!(details[0].1, "src/main.rs"); } + #[test] + fn test_diff_preview_edit_file() { + let params = json!({ + "path": "src/main.rs", + "search": "fn main() {\n println!(\"hello\");\n}", + "replace": "fn main() {\n println!(\"world\");\n}" + }); + let request = + ApprovalRequest::new("test-id", "edit_file", "Edit file", ¶ms, "test_key"); + let preview = request.diff_preview().expect("edit_file produces a preview"); + // Tests don't see src/main.rs, so we land in the MissingMatch fallback + // which still surfaces a search→replace diff for visual confirmation. + let diff = preview.diff_text(); + assert!(diff.contains("--- a/src/main.rs")); + assert!(diff.contains("+++ b/src/main.rs")); + assert!(diff.contains("- println!(\"hello\");")); + assert!(diff.contains("+ println!(\"world\");")); + assert!( + matches!(preview, ApprovalDiffPreview::MissingMatch { .. }), + "expected MissingMatch when file is absent, got {preview:?}" + ); + } + + #[test] + fn test_diff_preview_edit_file_existing_simulates_replace() { + // When the file exists and search matches, we should produce a full + // simulated diff against the real file content. + let path = std::env::temp_dir().join("deepseek_test_edit_file_existing.txt"); + std::fs::write(&path, "alpha\nbeta\ngamma\n").unwrap(); + let params = json!({ + "path": path.display().to_string(), + "search": "beta", + "replace": "BETA", + }); + let request = + ApprovalRequest::new("test-id", "edit_file", "Edit file", ¶ms, "test_key"); + let preview = request.diff_preview().expect("edit_file preview"); + match preview { + ApprovalDiffPreview::Diff { text, .. } => { + assert!(text.contains("-beta"), "got {text}"); + assert!(text.contains("+BETA"), "got {text}"); + } + other => panic!("expected Diff for existing edit_file, got {other:?}"), + } + let _ = std::fs::remove_file(&path); + } + + #[test] + fn test_diff_preview_write_file_existing() { + let path = std::env::temp_dir().join("deepseek_test_diff_preview.txt"); + std::fs::write(&path, "old content\n").unwrap(); + let params = json!({"path": path.display().to_string(), "content": "new content\n"}); + let request = + ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); + let preview = request + .diff_preview() + .expect("write_file on existing file should produce a preview"); + let diff = preview.diff_text(); + assert!(diff.contains("-old content"), "{diff}"); + assert!(diff.contains("+new content"), "{diff}"); + assert!( + matches!(preview, ApprovalDiffPreview::Diff { .. }), + "expected Diff variant, got {preview:?}" + ); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn test_diff_preview_write_file_unchanged_shows_no_change() { + // write_file with content identical to the file's current bytes used + // to drop the whole preview panel. We now surface a NoChange hint so + // the user knows the call is a no-op. + let path = std::env::temp_dir().join("deepseek_test_diff_no_change.txt"); + std::fs::write(&path, "same\n").unwrap(); + let params = json!({"path": path.display().to_string(), "content": "same\n"}); + let request = + ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); + let preview = request.diff_preview().expect("NoChange is still a preview"); + assert!( + matches!(preview, ApprovalDiffPreview::NoChange { .. }), + "expected NoChange, got {preview:?}" + ); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn test_diff_preview_write_file_new() { + let path = std::env::temp_dir().join("deepseek_test_diff_new_file.txt"); + let _ = std::fs::remove_file(&path); + let params = json!({"path": path.display().to_string(), "content": "brand new\n"}); + let request = + ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); + let preview = request + .diff_preview() + .expect("write_file on new file should produce a preview"); + match preview { + ApprovalDiffPreview::NewFile { content, .. } => { + assert!(content.contains("brand new"), "{content}"); + } + other => panic!("expected NewFile variant, got {other:?}"), + } + } + + #[test] + fn test_diff_preview_write_file_resolves_workspace_relative_path() { + // Regression for the bug where write_file with a workspace-relative + // path produced an empty preview because std::fs::read_to_string was + // called with the raw relative path. + let workspace = std::env::temp_dir().join("deepseek_test_ws_relative"); + std::fs::create_dir_all(&workspace).unwrap(); + let file_rel = "nested/file.txt"; + let abs = workspace.join(file_rel); + std::fs::create_dir_all(abs.parent().unwrap()).unwrap(); + std::fs::write(&abs, "before\n").unwrap(); + + let params = json!({"path": file_rel, "content": "after\n"}); + let request = ApprovalRequest::new_with_workspace( + "test-id", + "write_file", + "Write file", + ¶ms, + "test_key", + Some(workspace.display().to_string()), + ); + let preview = request.diff_preview().expect("preview built"); + match preview { + ApprovalDiffPreview::Diff { text, .. } => { + assert!(text.contains("-before"), "{text}"); + assert!(text.contains("+after"), "{text}"); + } + other => panic!("expected Diff for resolved path, got {other:?}"), + } + let _ = std::fs::remove_dir_all(&workspace); + } + + #[test] + fn test_diff_preview_apply_patch() { + let patch = "--- a/f.rs\n+++ b/f.rs\n@@ -1 +1 @@\n-old\n+new\n"; + let params = json!({"path": "f.rs", "patch": patch}); + let request = + ApprovalRequest::new("test-id", "apply_patch", "Apply patch", ¶ms, "test_key"); + let preview = request.diff_preview().expect("apply_patch preview"); + match preview { + ApprovalDiffPreview::Diff { text, .. } => assert_eq!(text, patch), + other => panic!("expected Diff variant for apply_patch, got {other:?}"), + } + } + + #[test] + fn test_diff_preview_apply_patch_changes_array() { + // apply_patch accepts a `changes` array as a full-file replacement + // alternative to `patch`. The preview must surface those changes + // instead of leaving the popup blank. + let workspace = std::env::temp_dir().join("deepseek_test_apply_patch_changes"); + std::fs::create_dir_all(&workspace).unwrap(); + let a = workspace.join("a.txt"); + std::fs::write(&a, "old\n").unwrap(); + + let params = json!({ + "changes": [ + {"path": "a.txt", "content": "new\n"}, + {"path": "b.txt", "content": "added\n"}, + ] + }); + let request = ApprovalRequest::new_with_workspace( + "test-id", + "apply_patch", + "Apply patch", + ¶ms, + "test_key", + Some(workspace.display().to_string()), + ); + let preview = request + .diff_preview() + .expect("changes array should produce a preview"); + match preview { + ApprovalDiffPreview::Diff { text, .. } => { + assert!(text.contains("diff --git a/a.txt b/a.txt"), "{text}"); + assert!(text.contains("-old"), "{text}"); + assert!(text.contains("+new"), "{text}"); + assert!(text.contains("diff --git a/b.txt b/b.txt"), "{text}"); + assert!(text.contains("+added"), "{text}"); + } + other => panic!("expected Diff for changes, got {other:?}"), + } + let _ = std::fs::remove_dir_all(&workspace); + } + + #[test] + fn test_diff_preview_none_for_other_tools() { + let params = json!({"command": "ls"}); + let request = + ApprovalRequest::new("test-id", "exec_shell", "Run shell", ¶ms, "test_key"); + assert!(request.diff_preview().is_none()); + } + // ======================================================================== // ApprovalView Tests — Benign Variant (single-key approve) // ======================================================================== diff --git a/crates/tui/src/tui/diff_render.rs b/crates/tui/src/tui/diff_render.rs index ac8cb7bca..b16d15620 100644 --- a/crates/tui/src/tui/diff_render.rs +++ b/crates/tui/src/tui/diff_render.rs @@ -39,8 +39,8 @@ pub fn render_diff(diff: &str, width: u16) -> Vec> { if raw.starts_with("@@") { if let Some((old_start, new_start)) = parse_hunk_header(raw) { - old_line = Some(old_start); - new_line = Some(new_start); + old_line = old_start; + new_line = new_start; } lines.extend(render_hunk_header(raw, width)); continue; @@ -107,6 +107,95 @@ pub fn render_diff(diff: &str, width: u16) -> Vec> { lines } +/// Render a unified diff in the **compact** layout used by the approval +/// popup: keeps `@@` hunk headers, line numbers, and `+/-` colouring, but +/// drops the multi-line summary block and the `--- a/... +++ b/...` / +/// `diff --git` headers. That trim is what frees up the ~6 rows the old +/// renderer was burning on metadata inside a 10-line preview window. +/// +/// The full renderer (`render_diff`) is still what the detail pager uses, +/// so nothing here changes what the user sees behind `v`. +pub fn render_diff_compact(diff: &str, width: u16) -> Vec> { + let mut lines = Vec::new(); + let mut old_line: Option = None; + let mut new_line: Option = None; + + for raw in diff.lines() { + if raw.starts_with("diff --git") + || raw.starts_with("index ") + || raw.starts_with("--- ") + || raw.starts_with("+++ ") + { + continue; + } + + if raw.starts_with("@@") { + if let Some((old_start, new_start)) = parse_hunk_header(raw) { + old_line = old_start; + new_line = new_start; + } + lines.extend(render_hunk_header(raw, width)); + continue; + } + + if raw.starts_with('+') { + let content = raw.trim_start_matches('+'); + lines.extend(render_diff_line( + content, + width, + old_line, + new_line, + '+', + Style::default() + .fg(palette::DIFF_ADDED) + .bg(palette::DIFF_ADDED_BG), + )); + if let Some(line) = new_line.as_mut() { + *line = line.saturating_add(1); + } + continue; + } + + if raw.starts_with('-') { + let content = raw.trim_start_matches('-'); + lines.extend(render_diff_line( + content, + width, + old_line, + new_line, + '-', + Style::default() + .fg(palette::STATUS_ERROR) + .bg(palette::DIFF_DELETED_BG), + )); + if let Some(line) = old_line.as_mut() { + *line = line.saturating_add(1); + } + continue; + } + + if raw.starts_with(' ') { + let content = raw.trim_start_matches(' '); + lines.extend(render_diff_line( + content, + width, + old_line, + new_line, + ' ', + Style::default().fg(palette::TEXT_PRIMARY), + )); + if let Some(line) = old_line.as_mut() { + *line = line.saturating_add(1); + } + if let Some(line) = new_line.as_mut() { + *line = line.saturating_add(1); + } + } + } + + lines +} + #[must_use] pub fn summarize_diff(diff: &str) -> Vec { let mut summaries = Vec::new(); @@ -254,15 +343,26 @@ fn render_diff_summary(summaries: &[DiffFileSummary], width: u16) -> Vec Option<(usize, usize)> { +fn parse_hunk_header(line: &str) -> Option<(Option, Option)> { let parts: Vec<&str> = line.split_whitespace().collect(); if parts.len() < 3 { return None; } let old = parts[1].trim_start_matches('-'); let new = parts[2].trim_start_matches('+'); - let old_start = old.split(',').next()?.parse::().ok()?; - let new_start = new.split(',').next()?.parse::().ok()?; + // 0 means "no lines" (e.g. new file) → None so the gutter stays blank. + let old_start = old + .split(',') + .next()? + .parse::() + .ok() + .filter(|&v| v > 0); + let new_start = new + .split(',') + .next()? + .parse::() + .ok() + .filter(|&v| v > 0); Some((old_start, new_start)) } @@ -478,4 +578,45 @@ diff --git a/src/a.rs b/src/a.rs assert_eq!(lines.join(""), text); } + + #[test] + fn render_diff_compact_skips_summary_and_file_headers() { + // The compact renderer is what the approval popup uses, so summary + // / `diff --git` / `--- +++` rows must be dropped to free vertical + // room for actual code. Hunk header + line-numbered code rows stay. + let diff = "\ +diff --git a/src/a.rs b/src/a.rs +--- a/src/a.rs ++++ b/src/a.rs +@@ -1,2 +1,3 @@ + line ++new +-old +"; + + let rendered = render_diff_compact(diff, 80); + let text: Vec = rendered.iter().map(line_text).collect(); + let joined = text.join("\n"); + assert!(!joined.contains("summary:"), "compact must skip summary: {joined}"); + assert!( + !joined.contains("diff --git"), + "compact must skip file headers: {joined}", + ); + assert!( + !joined.contains("--- a/src/a.rs"), + "compact must skip --- header: {joined}", + ); + assert!( + text.iter().any(|line| line.contains("@@ -1,2 +1,3 @@")), + "hunk header should remain: {text:?}", + ); + assert!( + text.iter().any(|line| line.contains(" + new")), + "added line should carry + gutter: {text:?}", + ); + assert!( + text.iter().any(|line| line.contains(" - old")), + "deleted line should carry - gutter: {text:?}", + ); + } } diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index b2b618590..9c8564e6b 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -26,7 +26,8 @@ use crate::localization::Locale; use crate::palette; use crate::tui::app::{App, AppMode, ComposerDensity, VimMode}; use crate::tui::approval::{ - ApprovalRequest, ApprovalView, ElevationOption, ElevationRequest, RiskLevel, ToolCategory, + ApprovalDiffPreview, ApprovalRequest, ApprovalView, ElevationOption, ElevationRequest, + RiskLevel, ToolCategory, }; use crate::tui::history::HistoryCell; use crate::tui::scrolling::TranscriptLineMeta; @@ -1055,7 +1056,7 @@ const APPROVAL_CARD_MIN_HEIGHT: u16 = 16; const APPROVAL_CARD_MIN_WIDTH: u16 = 40; /// Maximum card height — taller cards stop reading like a focused /// takeover and waste vertical space on large terminals. -const APPROVAL_CARD_MAX_HEIGHT: u16 = 18; +const APPROVAL_CARD_MAX_HEIGHT: u16 = 28; /// Maximum card width — readability craters past this on wide terminals. const APPROVAL_CARD_MAX_WIDTH: u16 = 96; @@ -1153,11 +1154,51 @@ impl Renderable for ApprovalWidget<'_> { } } - lines.push(Line::from("")); - let options = approval_options_for(self.request, risk, locale); let pending = self.view.pending_confirm(); + if let Some(preview) = self.request.diff_preview() { + let diff_width = card_area.width.saturating_sub(4).max(20); + let (header, body_lines) = build_diff_panel(preview, diff_width, locale); + let body_height = card_area.height.saturating_sub(4) as usize; + let footer_rows = match (risk, pending) { + (RiskLevel::Destructive, Some(_)) if !details.is_empty() => 2, + _ => 1, + }; + // Keep the action rows and confirmation copy visible while the + // diff itself scrolls inside the approval card. + let reserved_after_diff = 1 + 4 + 1 + footer_rows; + let available = body_height + .saturating_sub(lines.len()) + .saturating_sub(reserved_after_diff) + .saturating_sub(2); + let visible = available.min(body_lines.len()); + let scroll = self.view.set_diff_metrics(body_lines.len(), visible); + + lines.push(Line::from("")); + if visible > 0 { + let end = scroll.saturating_add(visible).min(body_lines.len()); + let more_above = scroll > 0; + let more_below = end < body_lines.len(); + lines.push(Line::from(Span::styled( + diff_header_with_scroll(&header, more_above, more_below, locale), + Style::default().fg(palette::TEXT_HINT), + ))); + for line in body_lines.into_iter().skip(scroll).take(visible) { + lines.push(line); + } + } else { + lines.push(Line::from(Span::styled( + format!(" {header} · {}", diff_hidden_hint(locale)), + Style::default().fg(palette::TEXT_HINT), + ))); + } + } else { + self.view.set_diff_metrics(0, 0); + } + + lines.push(Line::from("")); + for (i, opt) in options.iter().enumerate() { let is_selected = i == self.view.selected(); let staged = pending.is_some_and(|p| p == opt.option); @@ -1461,8 +1502,178 @@ fn single_key_value(_locale: Locale) -> &'static str { fn footer_controls(locale: Locale) -> &'static str { match locale { - Locale::ZhHans => " · v：完整参数 · Esc：终止", - _ => " · v: full params · Esc: abort", + Locale::ZhHans => " · v：完整 diff · Esc：终止", + _ => " · v: full details · Esc: abort", + } +} + +/// Build the rendered body + header for the approval popup diff area. +/// +/// The popup uses the compact renderer (no summary / `--- +++` lines) so a +/// 10-row preview window shows a few real lines of code instead of just +/// metadata. The header summarises the variant — `+N -M` for normal diffs, +/// a "new file" or "no change" hint for the degenerate cases — so the user +/// always knows what's behind the preview window even when it's empty. +fn build_diff_panel( + preview: &ApprovalDiffPreview, + width: u16, + locale: Locale, +) -> (String, Vec>) { + use crate::tui::diff_render::render_diff_compact; + match preview { + ApprovalDiffPreview::Diff { + text, + added, + deleted, + } => { + let lines = render_diff_compact(text, width); + let header = format!( + "{} +{} -{}", + diff_preview_word(locale), + added, + deleted + ); + (header, lines) + } + ApprovalDiffPreview::NewFile { path: _, content } => { + // Render the content as additions so the user sees the proposed + // file body with `+` gutters / colour. We synthesize a hunk so + // line numbers start at 1. + let line_count = content.lines().count().max(1); + let synthetic = format!("@@ -0,0 +1,{line_count} @@\n"); + let body = content + .lines() + .map(|l| format!("+{l}")) + .collect::>() + .join("\n"); + let combined = format!("{synthetic}{body}"); + let lines = render_diff_compact(&combined, width); + let header = format!( + "{} +{}", + new_file_word(locale), + content.lines().count() + ); + (header, lines) + } + ApprovalDiffPreview::NoChange { path: _ } => { + let msg = no_change_text(locale); + let line = Line::from(Span::styled( + msg.to_string(), + Style::default().fg(palette::TEXT_MUTED), + )); + (no_change_word(locale).to_string(), vec![line]) + } + ApprovalDiffPreview::MissingMatch { + path: _, + text, + match_count: _, + } => { + let mut lines = vec![Line::from(Span::styled( + missing_match_warning(locale).to_string(), + Style::default() + .fg(palette::STATUS_WARNING) + .add_modifier(Modifier::BOLD), + ))]; + lines.extend(render_diff_compact(text, width)); + (missing_match_header(locale).to_string(), lines) + } + } +} + +fn diff_header_with_scroll( + header: &str, + more_above: bool, + more_below: bool, + locale: Locale, +) -> String { + let mut parts: Vec = vec![format!(" {header}")]; + if more_above || more_below { + parts.push(scroll_hint(locale).to_string()); + } + if more_above { + parts.push(more_above_hint(locale).to_string()); + } + if more_below { + parts.push(more_below_hint(locale).to_string()); + } + parts.push(full_details_hint(locale).to_string()); + parts.join(" · ") +} + +fn diff_preview_word(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "差异预览", + _ => "Diff preview", + } +} + +fn new_file_word(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "新文件", + _ => "New file", + } +} + +fn no_change_word(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "无变化", + _ => "No change", + } +} + +fn no_change_text(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => " 内容与当前文件一致，无需写入。", + _ => " Content matches the current file — nothing to write.", + } +} + +fn missing_match_warning(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => " ⚠ 文件中未匹配到 search，下面是 search→replace 兜底视图。", + _ => " ⚠ Search string not found in file — fallback search→replace below.", + } +} + +fn missing_match_header(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "未匹配（兜底视图）", + _ => "No match (fallback)", + } +} + +fn scroll_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "PgUp/PgDn 滚动", + _ => "PgUp/PgDn scroll", + } +} + +fn more_above_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "↑ 上方还有", + _ => "↑ more above", + } +} + +fn more_below_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "↓ 下方还有", + _ => "↓ more below", + } +} + +fn full_details_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "v 完整 diff", + _ => "v full details", + } +} + +fn diff_hidden_hint(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "终端高度不足，按 v 查看", + _ => "preview hidden — press v", } } From 4a38cb21c4a6560dd96cfe01b8724ef391c89dd7 Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Mon, 18 May 2026 23:34:13 +0800 Subject: [PATCH 4/7] fix(tui): render tool detail pager with coloured diff lines `open_details_pager_for_cell` was concatenating the diff into a plain text body and feeding it through `PagerView::from_text`, which wraps every line as `Span::raw`. The result lost all the colour, line numbers, and hunk gutter the approval popup had been showing, so `v` and the history detail view rendered the diff as monochrome ASCII. Reuse the cached `build_diff_preview` (now `pub` from approval.rs) so the detail pager runs through the same workspace-resolved path the popup does, then build a `Vec>` directly: - Section labels (`Tool ID:` / `Tool:` / `Changes:` / `Input:` / `Output:`) get the sky-blue bold style the rest of the TUI uses for muted/label spans. - The diff section calls `diff_render::render_diff` so each line carries its `+/-` gutter colour and old/new line number prefix. - Input/Output/Spillover stay as raw spans so the pager's own `Paragraph::wrap` handles long lines. Push via `PagerView::new(title, lines)` instead of the `from_text` shim that destroys structure. --- crates/tui/src/tui/approval.rs | 3 +- crates/tui/src/tui/ui.rs | 75 +++++++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 17 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 08a7ac26d..00f088965 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -147,7 +147,6 @@ impl ApprovalDiffPreview { /// Plain unified-diff text suitable for the pager / detail view. /// Returns an empty string for non-diff variants. #[must_use] - #[allow(dead_code)] // wired up by the detail pager in the follow-up commit pub fn diff_text(&self) -> &str { match self { Self::Diff { text, .. } | Self::MissingMatch { text, .. } => text, @@ -430,7 +429,7 @@ fn count_diff_changes(diff: &str) -> (usize, usize) { /// Build the diff snapshot for an approval request. Reads the filesystem /// at most once per request — relative paths resolve against `workspace` /// so previews work when the agent is rooted elsewhere from the TUI's CWD. -fn build_diff_preview( +pub fn build_diff_preview( tool_name: &str, params: &Value, workspace: Option<&str>, diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index fc2504f39..29ba2d451 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -7420,6 +7420,8 @@ fn spillover_pager_section(app: &App, cell_index: usize) -> Option { } pub(crate) fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> bool { + use ratatui::style::Modifier; + use ratatui::text::{Line, Span}; if let Some(detail) = app.tool_detail_record_for_cell(cell_index) { let input = serde_json::to_string_pretty(&detail.input) .unwrap_or_else(|_| detail.input.to_string()); @@ -7427,6 +7429,12 @@ pub(crate) fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> b "(not available)".to_string(), std::string::ToString::to_string, ); + let workspace_str = app.workspace.to_string_lossy().to_string(); + let preview = crate::tui::approval::build_diff_preview( + &detail.tool_name, + &detail.input, + Some(workspace_str.as_str()), + ); // #500: when the tool result was spilled to disk, fold the full // file content into the pager body so the user can see what was @@ -7435,27 +7443,64 @@ pub(crate) fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> b // model received against the full payload. let spillover_section = spillover_pager_section(app, cell_index); - let content = if let Some(section) = spillover_section { - format!( - "Tool ID: {}\nTool: {}\n\nInput:\n{}\n\nOutput:\n{}\n\n{}", - detail.tool_id, detail.tool_name, input, output, section - ) - } else { - format!( - "Tool ID: {}\nTool: {}\n\nInput:\n{}\n\nOutput:\n{}", - detail.tool_id, detail.tool_name, input, output - ) - }; - let width = app .viewport .last_transcript_area .map(|area| area.width) .unwrap_or(80); - app.view_stack.push(PagerView::from_text( + // Subtract pager border + padding so wrapping in `render_diff` + // matches what the user actually sees inside the popup. + let diff_width = width.saturating_sub(4).max(20); + + let label_style = Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD); + let muted_style = Style::default().fg(palette::TEXT_MUTED); + let push_plain = |lines: &mut Vec>, text: &str| { + for raw in text.split('\n') { + lines.push(Line::from(Span::raw(raw.to_string()))); + } + }; + + let mut lines: Vec> = Vec::new(); + lines.push(Line::from(vec![ + Span::styled("Tool ID: ", muted_style), + Span::raw(detail.tool_id.clone()), + ])); + lines.push(Line::from(vec![ + Span::styled("Tool: ", muted_style), + Span::raw(detail.tool_name.clone()), + ])); + lines.push(Line::from("")); + + if let Some(preview) = preview.as_ref() { + lines.push(Line::from(Span::styled("Changes:", label_style))); + let diff_text = preview.diff_text(); + if diff_text.is_empty() { + lines.push(Line::from(Span::styled( + " (no textual changes — content matches current file)".to_string(), + muted_style, + ))); + } else { + lines.extend(crate::tui::diff_render::render_diff(diff_text, diff_width)); + } + lines.push(Line::from("")); + } + + lines.push(Line::from(Span::styled("Input:", label_style))); + push_plain(&mut lines, &input); + lines.push(Line::from("")); + lines.push(Line::from(Span::styled("Output:", label_style))); + push_plain(&mut lines, &output); + + if let Some(section) = spillover_section { + lines.push(Line::from("")); + push_plain(&mut lines, §ion); + } + + app.view_stack.push(PagerView::new( format!("Tool: {}", detail.tool_name), - &content, - width.saturating_sub(2), + lines, )); return true; } From 9a7be5e37ec8bd02591c37fcaff730985935d224 Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Tue, 19 May 2026 00:14:34 +0800 Subject: [PATCH 5/7] fix(tui): align approval diff body and stop truncating shell commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier approval-popup diff cache landed but the rendered diff body started at column 0 while every other row in the popup uses a two-space margin, so the hunk header and code rows visually broke out of the card. Shell commands were also hard-clipped at 120 characters with a trailing ellipsis, hiding the dangerous tail (the part that usually contains `> redirect` / `rm -rf` / piped side effects) — exactly the part the user needs to read before approving. - Indent every diff body line (Diff / NewFile / MissingMatch variants) by two spaces and shrink the rendering width to match so the renderer's wrap decisions agree with what fits. - Drop the synthetic `@@ -0,0 +1,N @@` hunk header for NewFile; the panel header already says "New file +N" and the hunk row just wasted one of the few preview rows. - Hand-build the NewFile body lines so they carry the same line-number gutter and addition colour the diff path uses, without routing through render_diff_compact's hunk-header path. - New `param_text` helper returns shell `command` / `cmd` values verbatim. The popup body uses `Paragraph::wrap`, so long lines fold naturally instead of needing in-band ellipsis truncation. - Bump path / target / input previews from 96 to 200 chars so long file paths and URLs survive without `…` in the popup. - Replace the local `count_diff_changes` loop with a call into `diff_render::summarize_diff` so the popup header's `+N -M` totals agree with the detail pager's summary (with a single- file fallback for fragments that lack a `diff --git` header). --- crates/tui/src/tui/approval.rs | 87 +++++++++++++++++++++++++------ crates/tui/src/tui/widgets/mod.rs | 61 +++++++++++++++++----- 2 files changed, 118 insertions(+), 30 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index 00f088965..b235866b5 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -232,10 +232,14 @@ impl ApprovalRequest { let mut details = Vec::new(); match self.category { ToolCategory::Shell => { - if let Some(cmd) = param_preview(&self.params, &["command", "cmd"], 120) { + // Shell commands stay verbatim — the popup body uses + // `Paragraph::wrap`, so it folds long lines on its own and + // an in-band `...` truncation just hides the tail of the + // command the user is being asked to approve. + if let Some(cmd) = param_text(&self.params, &["command", "cmd"]) { details.push(("Command".into(), cmd)); } - if let Some(dir) = param_preview(&self.params, &["workdir", "cwd"], 48) { + if let Some(dir) = param_preview(&self.params, &["workdir", "cwd"], 96) { let is_current = self.workspace.as_ref().is_some_and(|ws| { let a = std::path::Path::new(&dir); let b = std::path::Path::new(ws); @@ -252,19 +256,19 @@ impl ApprovalRequest { } ToolCategory::FileWrite => { if let Some(path) = - param_preview(&self.params, &["path", "target", "destination"], 96) + param_preview(&self.params, &["path", "target", "destination"], 200) { details.push(("File".into(), path)); } } ToolCategory::Safe => { - if let Some(path) = param_preview(&self.params, &["path", "ref_id", "uri"], 96) { + if let Some(path) = param_preview(&self.params, &["path", "ref_id", "uri"], 200) { details.push(("Path".into(), path)); } } ToolCategory::Network => { if let Some(target) = - param_preview(&self.params, &["url", "q", "query", "location", "repo"], 96) + param_preview(&self.params, &["url", "q", "query", "location", "repo"], 200) { details.push(("Target".into(), target)); } @@ -273,7 +277,7 @@ impl ApprovalRequest { if let Some(val) = param_preview( &self.params, &["command", "path", "url", "q", "query", "ref_id"], - 96, + 200, ) { details.push(("Input".into(), val)); } @@ -393,6 +397,22 @@ pub fn classify_risk(tool_name: &str, category: ToolCategory, params: &Value) -> } } +/// Like [`param_preview`] but never truncates the string value. Used for +/// shell commands so the popup shows what's actually being run instead of +/// `...`-eliding the dangerous tail. The popup body uses `Paragraph::wrap` +/// so long values fold across multiple visual lines on their own. +fn param_text(params: &Value, keys: &[&str]) -> Option { + let Value::Object(map) = params else { + return None; + }; + for key in keys { + if let Some(Value::String(text)) = map.get(*key) { + return Some(text.clone()); + } + } + None +} + /// Resolve a tool-supplied path against the workspace when it's relative. /// Absolute paths are returned unchanged so `write_file` to `/etc/foo` still /// shows the right diff. The original string flows through if there's no @@ -409,20 +429,34 @@ fn resolve_workspace_path(raw: &str, workspace: Option<&str>) -> std::path::Path } } -/// Count `+` and `-` lines in a unified diff, ignoring the `+++/---` headers. +/// Count `+` and `-` lines in a unified diff. Delegates to the shared +/// `summarize_diff` so the popup header reads the same `+N -M` totals +/// the detail pager shows in its summary section — keeps the two views +/// agreeing on what "changed" means even for tricky inputs (no-newline +/// markers, multi-file patches, etc.). fn count_diff_changes(diff: &str) -> (usize, usize) { - let mut added = 0usize; - let mut deleted = 0usize; - for line in diff.lines() { - if line.starts_with("+++") || line.starts_with("---") { - continue; - } - if line.starts_with('+') { - added += 1; - } else if line.starts_with('-') { - deleted += 1; + let summaries = crate::tui::diff_render::summarize_diff(diff); + if summaries.is_empty() { + // summarize_diff only collects files that have a `diff --git` or + // `+++` header. For single-file fragments produced by + // `make_unified_diff` we fall back to a plain line scan so the + // header still reflects the change. + let mut added = 0usize; + let mut deleted = 0usize; + for line in diff.lines() { + if line.starts_with("+++") || line.starts_with("---") { + continue; + } + if line.starts_with('+') { + added += 1; + } else if line.starts_with('-') { + deleted += 1; + } } + return (added, deleted); } + let added = summaries.iter().map(|s| s.added).sum(); + let deleted = summaries.iter().map(|s| s.deleted).sum(); (added, deleted) } @@ -1369,6 +1403,25 @@ mod tests { assert_eq!(details[1].1, "/home/user"); } + #[test] + fn test_prominent_details_shell_does_not_truncate_long_command() { + // Regression: shell commands used to be hard-clipped at 120 chars + // with a trailing `…`, hiding the dangerous tail of long pipelines + // (the part where `rm -rf` or `>` redirects usually live). The + // popup body wraps long lines via `Paragraph::wrap`, so we now + // pass the command through verbatim. + let cmd = format!("printf '{}\n' > /tmp/x && cat /tmp/x", "x".repeat(300)); + let params = json!({"command": cmd, "cwd": "/home/user"}); + let request = + ApprovalRequest::new("test-id", "exec_shell", "Run shell", ¶ms, "test_key"); + let details = request.prominent_details(); + assert_eq!(details[0].0, "Command"); + assert_eq!( + details[0].1, cmd, + "shell command must be returned verbatim, no `…` truncation", + ); + } + #[test] fn test_prominent_details_shell_marks_current_dir() { let params = json!({"command": "ls", "cwd": "/home/user/project"}); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 9c8564e6b..1b13ce112 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1514,19 +1514,40 @@ fn footer_controls(locale: Locale) -> &'static str { /// metadata. The header summarises the variant — `+N -M` for normal diffs, /// a "new file" or "no change" hint for the degenerate cases — so the user /// always knows what's behind the preview window even when it's empty. +/// Build the rendered body + header for the approval popup diff area. +/// +/// The popup uses the compact renderer (no summary / `--- +++` lines) so a +/// 10-row preview window shows a few real lines of code instead of just +/// metadata. The header summarises the variant — `+N -M` for normal diffs, +/// a "new file" or "no change" hint for the degenerate cases — so the user +/// always knows what's behind the preview window even when it's empty. +/// +/// Body lines are indented by two spaces to line up with the rest of the +/// popup body (the popup margin) so the hunk header and code rows visually +/// align with the `File` / option rows above and below. fn build_diff_panel( preview: &ApprovalDiffPreview, width: u16, locale: Locale, ) -> (String, Vec>) { use crate::tui::diff_render::render_diff_compact; + // Reserve 2 columns for the indent we prepend below so the renderer's + // wrapping decisions agree with the visible width. + let body_width = width.saturating_sub(2).max(20); + let indent = || Span::raw(" "); + let indent_lines = |mut lines: Vec>| -> Vec> { + for line in &mut lines { + line.spans.insert(0, indent()); + } + lines + }; match preview { ApprovalDiffPreview::Diff { text, added, deleted, } => { - let lines = render_diff_compact(text, width); + let lines = indent_lines(render_diff_compact(text, body_width)); let header = format!( "{} +{} -{}", diff_preview_word(locale), @@ -1536,24 +1557,38 @@ fn build_diff_panel( (header, lines) } ApprovalDiffPreview::NewFile { path: _, content } => { - // Render the content as additions so the user sees the proposed - // file body with `+` gutters / colour. We synthesize a hunk so - // line numbers start at 1. - let line_count = content.lines().count().max(1); - let synthetic = format!("@@ -0,0 +1,{line_count} @@\n"); + // Render the proposed content directly as added lines so the + // user sees the body with the standard `+` gutter + new-side + // line numbers. No synthetic hunk header — the panel header + // already says "New file +N", and the hunk row would just + // duplicate metadata while costing one of the few rows we have. let body = content .lines() - .map(|l| format!("+{l}")) - .collect::>() - .join("\n"); - let combined = format!("{synthetic}{body}"); - let lines = render_diff_compact(&combined, width); + .enumerate() + .map(|(idx, line)| { + // Reuse the diff renderer by feeding a synthetic line + // wearing the `+` marker, but the renderer doesn't see + // a hunk header so we build the styled line by hand + // to keep colour + line numbers consistent. + let prefix = format!("{:>4} {:>4} + ", "", idx + 1); + Line::from(vec![ + Span::raw(" "), + Span::styled(prefix, Style::default().fg(palette::TEXT_MUTED)), + Span::styled( + line.to_string(), + Style::default() + .fg(palette::DIFF_ADDED) + .bg(palette::DIFF_ADDED_BG), + ), + ]) + }) + .collect::>(); let header = format!( "{} +{}", new_file_word(locale), content.lines().count() ); - (header, lines) + (header, body) } ApprovalDiffPreview::NoChange { path: _ } => { let msg = no_change_text(locale); @@ -1574,7 +1609,7 @@ fn build_diff_panel( .fg(palette::STATUS_WARNING) .add_modifier(Modifier::BOLD), ))]; - lines.extend(render_diff_compact(text, width)); + lines.extend(indent_lines(render_diff_compact(text, body_width))); (missing_match_header(locale).to_string(), lines) } } From 37635593a659a3a9b72d7ecf8ad74bc2ec0bbe15 Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Tue, 19 May 2026 18:15:01 +0800 Subject: [PATCH 6/7] fix(tui): structure approval details and shell previews Render approval details with structured fields instead of raw JSON so multiline edit, patch, and change payloads stay readable in the details view. Improve shell command formatting, special-case printf-based file writes into a clearer preview, and keep diff/pager indentation intact.\n\nAlso keep diff scrolling on Up/Down while j/k continues to move selection. Add regression coverage for the approval details, shell previews, diff indentation, and pager wrapping behavior.\n\nVerification:\n- cargo fmt --all\n- cargo fmt --check\n- cargo test -p deepseek-tui approval::tests -- --nocapture\n- cargo test -p deepseek-tui widgets::tests::approval_shell_command -- --nocapture\n- cargo test -p deepseek-tui diff_render::tests -- --nocapture\n- cargo test -p deepseek-tui pager::tests -- --nocapture\n- cargo build --- crates/tui/src/tui/approval.rs | 298 ++++++++++++++++++++++---- crates/tui/src/tui/diff_render.rs | 91 ++++---- crates/tui/src/tui/pager.rs | 72 ++----- crates/tui/src/tui/ui.rs | 13 +- crates/tui/src/tui/views/mod.rs | 6 +- crates/tui/src/tui/widgets/mod.rs | 338 ++++++++++++++++++++++++++---- 6 files changed, 637 insertions(+), 181 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index b235866b5..a29326ca6 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -28,10 +28,13 @@ //! module always assumes the user is being asked. use crate::localization::Locale; +use crate::palette; use crate::sandbox::SandboxPolicy; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable}; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseEvent, MouseEventKind}; +use ratatui::style::{Modifier, Style}; +use ratatui::text::{Line, Span}; use serde_json::Value; use std::cell::Cell; use std::path::{Path, PathBuf}; @@ -267,9 +270,11 @@ impl ApprovalRequest { } } ToolCategory::Network => { - if let Some(target) = - param_preview(&self.params, &["url", "q", "query", "location", "repo"], 200) - { + if let Some(target) = param_preview( + &self.params, + &["url", "q", "query", "location", "repo"], + 200, + ) { details.push(("Target".into(), target)); } } @@ -512,8 +517,7 @@ pub fn build_diff_preview( // File missing — fall back to inputs-only diff. edit_file // would fail at execution anyway, so MissingMatch is the // honest framing. - let text = - crate::tools::diff_format::make_unified_diff(path, search, replace); + let text = crate::tools::diff_format::make_unified_diff(path, search, replace); Some(ApprovalDiffPreview::MissingMatch { path: path.to_string(), text, @@ -841,11 +845,10 @@ impl ApprovalView { } fn emit_params_pager(&self) -> ViewAction { - // Show a readable before/after for file tools, raw JSON otherwise. - if let Some(content) = self.build_detail_view() { - ViewAction::Emit(ViewEvent::OpenTextPager { + if let Some(lines) = self.build_detail_lines() { + ViewAction::Emit(ViewEvent::OpenStyledPager { title: format!("Details: {}", self.request.tool_name), - content, + lines, }) } else { let content = serde_json::to_string_pretty(&self.request.params) @@ -857,35 +860,85 @@ impl ApprovalView { } } - /// Build a human-readable before/after view for file tools. - fn build_detail_view(&self) -> Option { + /// Build a styled details view for file tools. + fn build_detail_lines(&self) -> Option>> { + let label_style = Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD); + let muted_style = Style::default().fg(palette::TEXT_MUTED); + let mut lines: Vec> = Vec::new(); + match self.request.tool_name.as_str() { "edit_file" => { let path = self.request.params.get("path")?.as_str()?; - let search = self.request.params.get("search")?.as_str()?; - let replace = self.request.params.get("replace")?.as_str()?; - Some(format!( - "File: {path}\n\n--- Before ---\n{search}\n\n+++ After +++\n{replace}" - )) + lines.push(Line::from(vec![ + Span::styled("File: ", muted_style), + Span::raw(path.to_string()), + ])); + push_approval_changes(&mut lines, self.request.diff_preview(), 120); + if let Some(search) = self.request.params.get("search").and_then(|v| v.as_str()) { + push_multiline_field(&mut lines, "Search", search, label_style); + } + if let Some(replace) = self.request.params.get("replace").and_then(|v| v.as_str()) { + push_multiline_field(&mut lines, "Replace", replace, label_style); + } + if let Some(fuzz) = self.request.params.get("fuzz") { + push_scalar_field(&mut lines, "Fuzz", &fuzz.to_string(), label_style); + } + Some(lines) } "write_file" => { let path = self.request.params.get("path")?.as_str()?; - let new_content = self.request.params.get("content")?.as_str()?; - let old_content = std::fs::read_to_string(path).unwrap_or_default(); - if old_content.is_empty() { - Some(format!( - "File: {path} (new)\n\n--- Content ---\n{new_content}" - )) - } else { - Some(format!( - "File: {path}\n\n--- Before ---\n{old_content}\n\n+++ After +++\n{new_content}" - )) + lines.push(Line::from(vec![ + Span::styled("File: ", muted_style), + Span::raw(path.to_string()), + ])); + push_approval_changes(&mut lines, self.request.diff_preview(), 120); + if let Some(content) = self.request.params.get("content").and_then(|v| v.as_str()) { + push_multiline_field(&mut lines, "Content", content, label_style); + } + if let Some(mode) = self.request.params.get("mode").and_then(|v| v.as_str()) { + push_scalar_field(&mut lines, "Mode", mode, label_style); } + Some(lines) } "apply_patch" => { - let path = self.request.params.get("path")?.as_str()?; - let patch = self.request.params.get("patch")?.as_str()?; - Some(format!("File: {path}\n\n{patch}")) + if let Some(path) = self.request.params.get("path").and_then(|v| v.as_str()) { + lines.push(Line::from(vec![ + Span::styled("File: ", muted_style), + Span::raw(path.to_string()), + ])); + } + push_approval_changes(&mut lines, self.request.diff_preview(), 120); + if let Some(patch) = self.request.params.get("patch").and_then(|v| v.as_str()) { + push_multiline_field(&mut lines, "Patch", patch, label_style); + } + if let Some(changes) = self + .request + .params + .get("changes") + .and_then(|v| v.as_array()) + { + lines.push(Line::from(Span::styled("Changes:", label_style))); + for (idx, change) in changes.iter().enumerate() { + let Some(change_path) = change.get("path").and_then(|v| v.as_str()) else { + continue; + }; + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + format!("{}.", idx + 1), + Style::default().fg(palette::TEXT_MUTED), + ), + Span::raw(" "), + Span::styled(change_path.to_string(), muted_style), + ])); + if let Some(content) = change.get("content").and_then(|v| v.as_str()) { + push_multiline_body(&mut lines, content, 4); + } + } + } + Some(lines) } _ => None, } @@ -937,11 +990,19 @@ impl ModalView for ApprovalView { self.scroll_diff_down(self.diff_page_height()); ViewAction::None } - KeyCode::Up | KeyCode::Char('k') => { + KeyCode::Up => { + self.scroll_diff_up(1); + ViewAction::None + } + KeyCode::Down => { + self.scroll_diff_down(1); + ViewAction::None + } + KeyCode::Char('k') => { self.select_prev(); ViewAction::None } - KeyCode::Down | KeyCode::Char('j') => { + KeyCode::Char('j') => { self.select_next(); ViewAction::None } @@ -1000,6 +1061,72 @@ impl ModalView for ApprovalView { } } +fn push_approval_changes( + lines: &mut Vec>, + preview: Option<&ApprovalDiffPreview>, + width: u16, +) { + let label_style = Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD); + let muted_style = Style::default().fg(palette::TEXT_MUTED); + + lines.push(Line::from("")); + lines.push(Line::from(Span::styled("Changes:", label_style))); + match preview { + Some(ApprovalDiffPreview::Diff { text, .. }) + | Some(ApprovalDiffPreview::MissingMatch { text, .. }) => { + if text.is_empty() { + lines.push(Line::from(Span::styled( + " (no textual changes - content matches current file)".to_string(), + muted_style, + ))); + } else { + lines.extend(crate::tui::diff_render::render_diff(text, width)); + } + } + Some(ApprovalDiffPreview::NewFile { path, content }) => { + let diff = crate::tools::diff_format::make_unified_diff(path, "", content); + lines.extend(crate::tui::diff_render::render_diff(&diff, width)); + } + Some(ApprovalDiffPreview::NoChange { .. }) | None => { + lines.push(Line::from(Span::styled( + " (no textual changes - content matches current file)".to_string(), + muted_style, + ))); + } + } + lines.push(Line::from("")); +} + +fn push_scalar_field(lines: &mut Vec>, label: &str, value: &str, label_style: Style) { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled(format!("{label}: "), label_style), + Span::raw(value.to_string()), + ])); +} + +fn push_multiline_field( + lines: &mut Vec>, + label: &str, + value: &str, + label_style: Style, +) { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled(format!("{label}:"), label_style), + ])); + push_multiline_body(lines, value, 4); +} + +fn push_multiline_body(lines: &mut Vec>, value: &str, indent: usize) { + let indent = " ".repeat(indent); + for raw in value.split('\n') { + lines.push(Line::from(Span::raw(format!("{indent}{raw}")))); + } +} + fn truncate_string_value(value: &str, max_len: usize) -> String { if value.chars().count() <= max_len { return value.to_string(); @@ -1457,7 +1584,9 @@ mod tests { }); let request = ApprovalRequest::new("test-id", "edit_file", "Edit file", ¶ms, "test_key"); - let preview = request.diff_preview().expect("edit_file produces a preview"); + let preview = request + .diff_preview() + .expect("edit_file produces a preview"); // Tests don't see src/main.rs, so we land in the MissingMatch fallback // which still surfaces a search→replace diff for visual confirmation. let diff = preview.diff_text(); @@ -1495,6 +1624,35 @@ mod tests { let _ = std::fs::remove_file(&path); } + #[test] + fn test_build_detail_lines_edit_file_structures_multiline_params() { + let params = json!({ + "path": "test_db.js", + "search": "async transaction(callback) {\n const conn = await this.pool.getConnection();\n}", + "replace": "async transaction(callback, timeout = 30000) {\n const conn = await this.pool.getConnection();\n}", + }); + let request = + ApprovalRequest::new("test-id", "edit_file", "Edit file", ¶ms, "test_key"); + let view = ApprovalView::new(request); + let lines = view.build_detail_lines().expect("file tools have details"); + let rendered = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n"); + assert!(rendered.contains("File: test_db.js"), "{rendered}"); + assert!(rendered.contains("Changes:"), "{rendered}"); + assert!(rendered.contains("Search:"), "{rendered}"); + assert!(rendered.contains("Replace:"), "{rendered}"); + assert!(!rendered.contains("\"search\""), "{rendered}"); + assert!(!rendered.contains("\"replace\""), "{rendered}"); + } + #[test] fn test_diff_preview_write_file_existing() { let path = std::env::temp_dir().join("deepseek_test_diff_preview.txt"); @@ -1636,6 +1794,32 @@ mod tests { let _ = std::fs::remove_dir_all(&workspace); } + #[test] + fn test_build_detail_lines_apply_patch_changes_without_top_level_path() { + let params = json!({ + "changes": [ + {"path": "a.txt", "content": "new\n"}, + ] + }); + let request = + ApprovalRequest::new("test-id", "apply_patch", "Apply patch", ¶ms, "test_key"); + let view = ApprovalView::new(request); + let lines = view.build_detail_lines().expect("apply_patch has details"); + let rendered = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n"); + assert!(rendered.contains("Changes:"), "{rendered}"); + assert!(rendered.contains("a.txt"), "{rendered}"); + assert!(!rendered.contains("\"changes\""), "{rendered}"); + } + #[test] fn test_diff_preview_none_for_other_tools() { let params = json!({"command": "ls"}); @@ -1825,13 +2009,13 @@ mod tests { assert_eq!(view.selected, 0); // clamped at 0 view.handle_key(create_key_event(KeyCode::Down)); - assert_eq!(view.selected, 1); + assert_eq!(view.selected, 0); view.handle_key(create_key_event(KeyCode::Char('j'))); - assert_eq!(view.selected, 2); + assert_eq!(view.selected, 1); view.handle_key(create_key_event(KeyCode::Char('k'))); - assert_eq!(view.selected, 1); + assert_eq!(view.selected, 0); } #[test] @@ -1840,17 +2024,41 @@ mod tests { let action = view.handle_key(create_key_event(KeyCode::Char('v'))); assert!(matches!( action, - ViewAction::Emit(ViewEvent::OpenTextPager { .. }) + ViewAction::Emit(ViewEvent::OpenTextPager { .. } | ViewEvent::OpenStyledPager { .. }) )); let mut view = ApprovalView::new(benign_request()); let action = view.handle_key(create_key_event(KeyCode::Char('V'))); assert!(matches!( action, - ViewAction::Emit(ViewEvent::OpenTextPager { .. }) + ViewAction::Emit(ViewEvent::OpenTextPager { .. } | ViewEvent::OpenStyledPager { .. }) )); } + #[test] + fn test_approval_view_view_params_uses_styled_pager_for_files() { + let mut view = ApprovalView::new(destructive_request()); + let action = view.handle_key(create_key_event(KeyCode::Char('v'))); + match action { + ViewAction::Emit(ViewEvent::OpenStyledPager { title, lines }) => { + assert!(title.contains("Details")); + let rendered = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n"); + assert!(rendered.contains("Changes:")); + assert!(rendered.contains("@@")); + } + other => panic!("expected styled pager, got {other:?}"), + } + } + #[test] fn test_approval_view_current_decision_mapping() { let mut view = ApprovalView::new(benign_request()); @@ -2103,6 +2311,22 @@ mod tests { ); } + #[test] + fn up_down_scroll_diff_without_changing_selection() { + let mut view = ApprovalView::new(destructive_request()); + let before = view.selected(); + assert!(matches!( + view.handle_key(create_key_event(KeyCode::Up)), + ViewAction::None + )); + assert_eq!(view.selected(), before); + assert!(matches!( + view.handle_key(create_key_event(KeyCode::Down)), + ViewAction::None + )); + assert_eq!(view.selected(), before); + } + #[test] fn render_destructive_zh_hans_localizes_security_copy() { let mut view = ApprovalView::new_for_locale(destructive_request(), Locale::ZhHans); diff --git a/crates/tui/src/tui/diff_render.rs b/crates/tui/src/tui/diff_render.rs index b16d15620..c0c982bda 100644 --- a/crates/tui/src/tui/diff_render.rs +++ b/crates/tui/src/tui/diff_render.rs @@ -47,7 +47,7 @@ pub fn render_diff(diff: &str, width: u16) -> Vec> { } if raw.starts_with('+') && !raw.starts_with("+++") { - let content = raw.trim_start_matches('+'); + let content = raw.strip_prefix('+').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -65,7 +65,7 @@ pub fn render_diff(diff: &str, width: u16) -> Vec> { } if raw.starts_with('-') && !raw.starts_with("---") { - let content = raw.trim_start_matches('-'); + let content = raw.strip_prefix('-').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -83,7 +83,7 @@ pub fn render_diff(diff: &str, width: u16) -> Vec> { } if raw.starts_with(' ') { - let content = raw.trim_start_matches(' '); + let content = raw.strip_prefix(' ').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -139,7 +139,7 @@ pub fn render_diff_compact(diff: &str, width: u16) -> Vec> { } if raw.starts_with('+') { - let content = raw.trim_start_matches('+'); + let content = raw.strip_prefix('+').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -157,7 +157,7 @@ pub fn render_diff_compact(diff: &str, width: u16) -> Vec> { } if raw.starts_with('-') { - let content = raw.trim_start_matches('-'); + let content = raw.strip_prefix('-').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -175,7 +175,7 @@ pub fn render_diff_compact(diff: &str, width: u16) -> Vec> { } if raw.starts_with(' ') { - let content = raw.trim_start_matches(' '); + let content = raw.strip_prefix(' ').unwrap_or(raw); lines.extend(render_diff_line( content, width, @@ -445,33 +445,14 @@ fn wrap_text(text: &str, width: usize) -> Vec { let mut current = String::new(); let mut current_width = 0; - for word in text.split_whitespace() { - let word_width = word.width(); - if word_width > width { - if !current.is_empty() { - lines.push(std::mem::take(&mut current)); - current_width = 0; - } - push_word_breaking_chars(word, width, &mut current, &mut current_width, &mut lines); - continue; - } - let additional = if current.is_empty() { - word_width - } else { - word_width + 1 - }; - if current_width + additional > width && !current.is_empty() { - lines.push(current); - current = word.to_string(); - current_width = word_width; - } else { - if !current.is_empty() { - current.push(' '); - current_width += 1; - } - current.push_str(word); - current_width += word_width; + for ch in text.chars() { + let char_width = ch.width().unwrap_or(1); + if current_width + char_width > width && current_width > 0 { + lines.push(std::mem::take(&mut current)); + current_width = 0; } + current.push(ch); + current_width += char_width; } if current.is_empty() { @@ -483,24 +464,6 @@ fn wrap_text(text: &str, width: usize) -> Vec { lines } -fn push_word_breaking_chars( - word: &str, - width: usize, - current: &mut String, - current_width: &mut usize, - lines: &mut Vec, -) { - for ch in word.chars() { - let char_width = ch.width().unwrap_or(1); - if *current_width + char_width > width && *current_width > 0 { - lines.push(std::mem::take(current)); - *current_width = 0; - } - current.push(ch); - *current_width += char_width; - } -} - #[cfg(test)] mod tests { use super::*; @@ -556,7 +519,8 @@ diff --git a/src/a.rs b/src/a.rs let rendered = render_diff(diff, 80); let text = rendered.iter().map(line_text).collect::>(); assert!(text[0].contains("summary: 1 file, +1 -1, 1 hunk")); - assert!(text.iter().any(|line| line.contains("src/a.rs +1 -1"))); + assert!(text.iter().any(|line| line.contains("src/a.rs"))); + assert!(text.iter().any(|line| line.contains("+1 -1"))); assert!( text.iter().any(|line| line.contains(" + new")), "added line should carry + gutter: {text:?}" @@ -597,7 +561,10 @@ diff --git a/src/a.rs b/src/a.rs let rendered = render_diff_compact(diff, 80); let text: Vec = rendered.iter().map(line_text).collect(); let joined = text.join("\n"); - assert!(!joined.contains("summary:"), "compact must skip summary: {joined}"); + assert!( + !joined.contains("summary:"), + "compact must skip summary: {joined}" + ); assert!( !joined.contains("diff --git"), "compact must skip file headers: {joined}", @@ -619,4 +586,24 @@ diff --git a/src/a.rs b/src/a.rs "deleted line should carry - gutter: {text:?}", ); } + + #[test] + fn render_diff_preserves_indented_context_lines() { + let diff = "\ +diff --git a/src/a.rs b/src/a.rs +--- a/src/a.rs ++++ b/src/a.rs +@@ -1,2 +1,2 @@ + fn main() { + println!(\"hello\"); +"; + let rendered = render_diff_compact(diff, 80); + let text = rendered + .iter() + .map(line_text) + .collect::>() + .join("\n"); + assert!(text.contains(" fn main() {"), "{text}"); + assert!(text.contains(" println!"), "{text}"); + } } diff --git a/crates/tui/src/tui/pager.rs b/crates/tui/src/tui/pager.rs index a67339c75..c1cc5680b 100644 --- a/crates/tui/src/tui/pager.rs +++ b/crates/tui/src/tui/pager.rs @@ -23,7 +23,7 @@ use ratatui::{ text::{Line, Span}, widgets::{Block, Borders, Clear, Padding, Paragraph, Widget, Wrap}, }; -use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; +use unicode_width::UnicodeWidthChar; use crate::palette; use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent}; @@ -70,7 +70,7 @@ impl PagerView { pub fn from_text(title: impl Into, text: &str, width: u16) -> Self { let mut lines = Vec::new(); for raw in text.lines() { - for wrapped in wrap_text(raw, width.max(1) as usize) { + for wrapped in wrap_text_preserving_spaces(raw, width.max(1) as usize) { lines.push(Line::from(Span::raw(wrapped))); } if raw.is_empty() { @@ -487,7 +487,7 @@ fn line_to_string(line: &Line<'static>) -> String { .collect::() } -fn wrap_text(text: &str, width: usize) -> Vec { +fn wrap_text_preserving_spaces(text: &str, width: usize) -> Vec { if width == 0 { return vec![text.to_string()]; } @@ -495,33 +495,14 @@ fn wrap_text(text: &str, width: usize) -> Vec { let mut current = String::new(); let mut current_width = 0usize; - for word in text.split_whitespace() { - let word_width = word.width(); - if word_width > width { - if !current.is_empty() { - lines.push(std::mem::take(&mut current)); - current_width = 0; - } - push_word_breaking_chars(word, width, &mut current, &mut current_width, &mut lines); - continue; - } - let additional = if current.is_empty() { - word_width - } else { - word_width + 1 - }; - if current_width + additional > width && !current.is_empty() { - lines.push(current); - current = word.to_string(); - current_width = word_width; - } else { - if !current.is_empty() { - current.push(' '); - current_width += 1; - } - current.push_str(word); - current_width += word_width; + for ch in text.chars() { + let char_width = ch.width().unwrap_or(1); + if current_width + char_width > width && current_width > 0 { + lines.push(std::mem::take(&mut current)); + current_width = 0; } + current.push(ch); + current_width += char_width; } if current.is_empty() { @@ -533,24 +514,6 @@ fn wrap_text(text: &str, width: usize) -> Vec { lines } -fn push_word_breaking_chars( - word: &str, - width: usize, - current: &mut String, - current_width: &mut usize, - lines: &mut Vec, -) { - for ch in word.chars() { - let char_width = ch.width().unwrap_or(1); - if *current_width + char_width > width && *current_width > 0 { - lines.push(std::mem::take(current)); - *current_width = 0; - } - current.push(ch); - *current_width += char_width; - } -} - #[cfg(test)] mod tests { use super::*; @@ -814,6 +777,13 @@ mod tests { assert_eq!(p.search_input, "c"); } + #[test] + fn from_text_preserves_leading_indent() { + let p = PagerView::from_text("T", " indented\n more", 40); + assert!(p.body_text().contains(" indented")); + assert!(p.body_text().contains(" more")); + } + #[test] fn footer_hint_is_rendered_in_buffer() { let p = make_pager(5); @@ -1006,10 +976,14 @@ mod tests { #[test] fn wrap_text_breaks_overlong_cjk_runs() { let text = "这是一个非常长的中文字符串".repeat(10); - let lines = wrap_text(&text, 16); + let lines = wrap_text_preserving_spaces(&text, 16); for line in &lines { - assert!(line.width() <= 16, "line {line:?} exceeds width 16"); + let width = line + .chars() + .map(|ch| ch.width().unwrap_or(1)) + .sum::(); + assert!(width <= 16, "line {line:?} exceeds width 16"); } assert_eq!(lines.join(""), text); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 29ba2d451..5bef8352b 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -4363,6 +4363,10 @@ fn open_text_pager(app: &mut App, title: String, content: String) { )); } +fn open_styled_pager(app: &mut App, title: String, lines: Vec>) { + app.view_stack.push(PagerView::new(title, lines)); +} + pub(crate) fn open_context_inspector(app: &mut App) { let width = app .viewport @@ -5752,6 +5756,9 @@ async fn handle_view_events( ViewEvent::OpenTextPager { title, content } => { open_text_pager(app, title, content); } + ViewEvent::OpenStyledPager { title, lines } => { + open_styled_pager(app, title, lines); + } ViewEvent::CopyToClipboard { text, label } => { if text.is_empty() { app.status_message = Some(format!("{label} is empty")); @@ -7498,10 +7505,8 @@ pub(crate) fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> b push_plain(&mut lines, §ion); } - app.view_stack.push(PagerView::new( - format!("Tool: {}", detail.tool_name), - lines, - )); + app.view_stack + .push(PagerView::new(format!("Tool: {}", detail.tool_name), lines)); return true; } diff --git a/crates/tui/src/tui/views/mod.rs b/crates/tui/src/tui/views/mod.rs index 7c3bd73d6..76a48e2fa 100644 --- a/crates/tui/src/tui/views/mod.rs +++ b/crates/tui/src/tui/views/mod.rs @@ -1,5 +1,5 @@ use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind}; -use ratatui::{buffer::Buffer, layout::Rect}; +use ratatui::{buffer::Buffer, layout::Rect, text::Line}; use std::cell::{Cell, RefCell}; use std::fmt; @@ -87,6 +87,10 @@ pub enum ViewEvent { title: String, content: String, }, + OpenStyledPager { + title: String, + lines: Vec>, + }, ApprovalDecision { tool_id: String, tool_name: String, diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 1b13ce112..e21a168d7 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -1136,21 +1136,13 @@ impl Renderable for ApprovalWidget<'_> { ])); } else { for (label, value) in &details { - lines.push(Line::from(vec![ - Span::raw(" "), - Span::styled( - format!("{label:<7} "), - Style::default() - .fg(palette::DEEPSEEK_SKY) - .add_modifier(Modifier::BOLD), - ), - Span::styled( - value.clone(), - Style::default() - .fg(palette::TEXT_BODY) - .add_modifier(Modifier::BOLD), - ), - ])); + if self.request.category == ToolCategory::Shell + && matches!(label.as_str(), "Command" | "命令") + { + push_shell_command_lines(&mut lines, label, value); + } else { + push_detail_line(&mut lines, label, value); + } } } @@ -1548,12 +1540,7 @@ fn build_diff_panel( deleted, } => { let lines = indent_lines(render_diff_compact(text, body_width)); - let header = format!( - "{} +{} -{}", - diff_preview_word(locale), - added, - deleted - ); + let header = format!("{} +{} -{}", diff_preview_word(locale), added, deleted); (header, lines) } ApprovalDiffPreview::NewFile { path: _, content } => { @@ -1583,11 +1570,7 @@ fn build_diff_panel( ]) }) .collect::>(); - let header = format!( - "{} +{}", - new_file_word(locale), - content.lines().count() - ); + let header = format!("{} +{}", new_file_word(locale), content.lines().count()); (header, body) } ApprovalDiffPreview::NoChange { path: _ } => { @@ -1622,9 +1605,6 @@ fn diff_header_with_scroll( locale: Locale, ) -> String { let mut parts: Vec = vec![format!(" {header}")]; - if more_above || more_below { - parts.push(scroll_hint(locale).to_string()); - } if more_above { parts.push(more_above_hint(locale).to_string()); } @@ -1677,13 +1657,6 @@ fn missing_match_header(locale: Locale) -> &'static str { } } -fn scroll_hint(locale: Locale) -> &'static str { - match locale { - Locale::ZhHans => "PgUp/PgDn 滚动", - _ => "PgUp/PgDn scroll", - } -} - fn more_above_hint(locale: Locale) -> &'static str { match locale { Locale::ZhHans => "↑ 上方还有", @@ -1700,9 +1673,208 @@ fn more_below_hint(locale: Locale) -> &'static str { fn full_details_hint(locale: Locale) -> &'static str { match locale { - Locale::ZhHans => "v 完整 diff", - _ => "v full details", + Locale::ZhHans => "v 详情", + _ => "v details", + } +} + +fn push_detail_line(lines: &mut Vec>, label: &str, value: &str) { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + format!("{label:<7} "), + Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD), + ), + Span::styled( + value.to_string(), + Style::default() + .fg(palette::TEXT_BODY) + .add_modifier(Modifier::BOLD), + ), + ])); +} + +fn push_shell_command_lines(lines: &mut Vec>, label: &str, command: &str) { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + format!("{label:<7} "), + Style::default() + .fg(palette::DEEPSEEK_SKY) + .add_modifier(Modifier::BOLD), + ), + ])); + + for line in format_shell_command_for_approval(command) { + lines.push(Line::from(vec![ + Span::raw(" "), + Span::styled( + line, + Style::default() + .fg(palette::TEXT_BODY) + .add_modifier(Modifier::BOLD), + ), + ])); + } +} + +fn format_shell_command_for_approval(command: &str) -> Vec { + if let Some(write) = parse_printf_write_file_command(command) { + return format_printf_write_file_preview(write); + } + + let mut out = Vec::new(); + for raw in command.split('\n') { + let mut current = String::new(); + let mut chars = raw.chars().peekable(); + let mut quote: Option = None; + while let Some(ch) = chars.next() { + if matches!(ch, '"' | '\'') { + if quote == Some(ch) { + quote = None; + } else if quote.is_none() { + quote = Some(ch); + } + current.push(ch); + continue; + } + + if quote.is_none() && ch == '&' && chars.peek() == Some(&'&') { + chars.next(); + push_shell_clause(&mut out, &mut current, Some("&&")); + continue; + } + + if quote.is_none() && ch == '|' { + push_shell_clause(&mut out, &mut current, Some("|")); + continue; + } + + if quote.is_none() && ch == ';' { + current.push(';'); + push_shell_clause(&mut out, &mut current, None); + continue; + } + + if quote.is_none() && matches!(ch, '>' | '<') { + current.push(ch); + while chars + .peek() + .is_some_and(|next| matches!(next, '>' | '<' | '&')) + { + current.push(chars.next().expect("peeked char exists")); + } + while chars.peek().is_some_and(|next| next.is_whitespace()) { + current.push(chars.next().expect("peeked char exists")); + } + while chars + .peek() + .is_some_and(|next| !next.is_whitespace() && !matches!(next, '&' | '|' | ';')) + { + current.push(chars.next().expect("peeked char exists")); + } + continue; + } + + current.push(ch); + } + push_shell_clause(&mut out, &mut current, None); + } + + if out.is_empty() { + vec![String::new()] + } else { + out + } +} + +struct PrintfWriteFilePreview { + target: String, + lines: Vec, +} + +fn parse_printf_write_file_command(command: &str) -> Option { + let (before_redirect, after_redirect) = split_unquoted_redirect(command)?; + let before_redirect = before_redirect.trim(); + if !before_redirect.starts_with("printf") { + return None; + } + let tokens = shlex::split(before_redirect)?; + if tokens.len() < 3 || tokens.first()?.as_str() != "printf" { + return None; + } + let fmt = tokens.get(1)?.as_str(); + if fmt != "%s\n" && fmt != "%s\\n" { + return None; + } + let target_tokens = shlex::split(after_redirect.trim_start_matches(['>', '&']).trim_start())?; + if target_tokens.len() != 1 { + return None; } + let target = target_tokens.into_iter().next()?; + let lines = tokens.into_iter().skip(2).collect::>(); + if lines.is_empty() { + return None; + } + Some(PrintfWriteFilePreview { target, lines }) +} + +fn format_printf_write_file_preview(write: PrintfWriteFilePreview) -> Vec { + let mut out = Vec::new(); + out.push("Write file via printf".to_string()); + out.push(format!("Target: {}", write.target)); + out.push(format!("Lines: {}", write.lines.len())); + out.push(String::new()); + out.push("Content:".to_string()); + let total = write.lines.len(); + for (idx, line) in write.lines.into_iter().enumerate().take(12) { + out.push(format!("{:>4} {}", idx + 1, line)); + } + if total > 12 { + out.push(format!(" ... (+{} more lines)", total - 12)); + } + out +} + +fn split_unquoted_redirect(command: &str) -> Option<(&str, &str)> { + let mut quote: Option = None; + for (idx, ch) in command.char_indices() { + if matches!(ch, '"' | '\'') { + if quote == Some(ch) { + quote = None; + } else if quote.is_none() { + quote = Some(ch); + } + continue; + } + if quote.is_none() && ch == '>' { + let before = &command[..idx]; + let after = &command[idx + ch.len_utf8()..]; + return Some((before, after)); + } + } + None +} + +fn push_shell_clause(out: &mut Vec, current: &mut String, connector: Option<&str>) { + let trimmed = current.trim(); + if !trimmed.is_empty() { + let mut line = trimmed.to_string(); + if let Some(connector) = connector { + line.push(' '); + line.push_str(connector); + } + out.push(line); + } else if let Some(connector) = connector + && let Some(prev) = out.last_mut() + && !prev.ends_with(connector) + { + prev.push(' '); + prev.push_str(connector); + } + current.clear(); } fn diff_hidden_hint(locale: Locale) -> &'static str { @@ -3597,6 +3769,96 @@ mod tests { } } + #[test] + fn approval_shell_command_splits_long_command_into_multiple_lines() { + let request = crate::tui::approval::ApprovalRequest::new( + "approval-1", + "exec_shell", + "Run shell command", + &serde_json::json!({ + "command": "printf 'a b c' && cat /tmp/x | sed -n '1,5p' > out.txt", + }), + "exec_shell:printf", + ); + let view = crate::tui::approval::ApprovalView::new(request.clone()); + let widget = ApprovalWidget::new(&request, &view); + let area = Rect::new(0, 0, 120, 24); + let mut buf = Buffer::empty(area); + widget.render(area, &mut buf); + + let mut body = String::new(); + for y in 0..area.height { + for x in 0..area.width { + body.push_str(buf[(x, y)].symbol()); + } + body.push('\n'); + } + assert!(body.contains("Command")); + assert!(body.contains("printf 'a b c'")); + assert!(body.contains("cat /tmp/x")); + assert!(body.contains("sed -n '1,5p'")); + } + + #[test] + fn approval_shell_command_detects_printf_write_file_preview() { + let request = crate::tui::approval::ApprovalRequest::new( + "approval-1", + "exec_shell", + "Run shell command", + &serde_json::json!({ + "command": "printf '%s\\n' \"line one\" \"line two\" > /tmp/out.txt", + }), + "exec_shell:printf", + ); + let view = crate::tui::approval::ApprovalView::new(request.clone()); + let widget = ApprovalWidget::new(&request, &view); + let area = Rect::new(0, 0, 120, 24); + let mut buf = Buffer::empty(area); + widget.render(area, &mut buf); + + let mut body = String::new(); + for y in 0..area.height { + for x in 0..area.width { + body.push_str(buf[(x, y)].symbol()); + } + body.push('\n'); + } + assert!(body.contains("Write file via printf"), "{body}"); + assert!(body.contains("Target: /tmp/out.txt"), "{body}"); + assert!(body.contains("Lines: 2"), "{body}"); + assert!(body.contains("1 line one"), "{body}"); + assert!(body.contains("2 line two"), "{body}"); + } + + #[test] + fn approval_shell_command_keeps_compound_printf_command_visible() { + let request = crate::tui::approval::ApprovalRequest::new( + "approval-1", + "exec_shell", + "Run shell command", + &serde_json::json!({ + "command": "printf '%s\\n' \"line one\" > /tmp/out.txt && rm /tmp/other.txt", + }), + "exec_shell:printf", + ); + let view = crate::tui::approval::ApprovalView::new(request.clone()); + let widget = ApprovalWidget::new(&request, &view); + let area = Rect::new(0, 0, 120, 24); + let mut buf = Buffer::empty(area); + widget.render(area, &mut buf); + + let mut body = String::new(); + for y in 0..area.height { + for x in 0..area.width { + body.push_str(buf[(x, y)].symbol()); + } + body.push('\n'); + } + assert!(!body.contains("Write file via printf"), "{body}"); + assert!(body.contains("printf '%s"), "{body}"); + assert!(body.contains("rm /tmp/other.txt"), "{body}"); + } + /// 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 From 522d89ddf5d8bc4ac203005a41ada002d0808d0b Mon Sep 17 00:00:00 2001 From: tdccccc <290923272@qq.com> Date: Sun, 24 May 2026 18:47:37 +0800 Subject: [PATCH 7/7] fix(tui): cache approval preview rendering Precompute approval popup details and parsed shell command lines when the approval request is created so the render loop no longer canonicalizes paths or reparses shell commands every frame.\n\nCache rendered diff preview panels by width and locale in ApprovalView, and add a size guard that skips inline diff generation for very large existing files instead of reading and diffing them synchronously in the TUI event path.\n\nUpdate pager text wrapping to prefer whitespace boundaries for natural language while preserving indentation and still character-wrapping long CJK or unbroken text.\n\nVerification:\n- cargo fmt --all\n- cargo fmt --all --check\n- git diff --check\n- cargo check -p codewhale-tui\n- cargo test -p codewhale-tui approval::tests -- --nocapture\n- cargo test -p codewhale-tui widgets::tests::approval_shell_command -- --nocapture\n- cargo test -p codewhale-tui pager::tests -- --nocapture\n- cargo build --- crates/tui/src/tui/approval.rs | 527 +++++++++++++++++++++++------- crates/tui/src/tui/pager.rs | 72 +++- crates/tui/src/tui/widgets/mod.rs | 219 +++---------- 3 files changed, 534 insertions(+), 284 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index a29326ca6..f2f98b2a6 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -36,10 +36,12 @@ use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseEvent, MouseEventKi use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; use serde_json::Value; -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; +const DIFF_PREVIEW_MAX_BYTES: u64 = 512 * 1024; + /// Determines when tool executions require user approval #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum ApprovalMode { @@ -144,6 +146,9 @@ pub enum ApprovalDiffPreview { text: String, match_count: usize, }, + /// Existing file is large enough that building a full in-memory diff would + /// risk a visible pause in the TUI event loop. + SkippedLargeFile { path: String, size: u64, limit: u64 }, } impl ApprovalDiffPreview { @@ -154,11 +159,31 @@ impl ApprovalDiffPreview { match self { Self::Diff { text, .. } | Self::MissingMatch { text, .. } => text, Self::NewFile { content, .. } => content, - Self::NoChange { .. } => "", + Self::NoChange { .. } | Self::SkippedLargeFile { .. } => "", } } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ApprovalDetail { + pub label: String, + pub value: String, + pub shell_lines: Option>, +} + +#[derive(Debug, Clone)] +pub struct RenderedDiffPanel { + pub header: String, + pub lines: Vec>, +} + +#[derive(Debug, Clone)] +struct DiffPanelCache { + width: u16, + locale: Locale, + panel: RenderedDiffPanel, +} + /// Request for user approval of a tool execution #[derive(Debug, Clone)] pub struct ApprovalRequest { @@ -177,12 +202,11 @@ pub struct ApprovalRequest { /// Lossy / arity-aware fingerprint, used to scope *approvals* so an /// "approve for session" covers later flag variants (v0.8.37). pub approval_grouping_key: String, - /// Current workspace directory, used to annotate cwd as "(current)" and - /// to resolve relative paths when reading old file contents for diffs. - pub workspace: Option, /// Snapshot of the diff/preview state, built once at construction so /// renderers never re-read the filesystem mid-render. diff_preview: Option, + /// Precomputed popup details, including parsed shell command lines. + prominent_details: Vec, } impl ApprovalRequest { @@ -209,10 +233,9 @@ impl ApprovalRequest { let risk = classify_risk(tool_name, category, params); let approval_grouping_key = crate::tools::approval_cache::build_approval_grouping_key(tool_name, params).0; - // Build the diff snapshot once. Renderers read this cache instead of - // hitting the filesystem each frame; relative paths resolve against - // `workspace` so a `write_file` invoked from the agent doesn't go - // looking for the file in the TUI's CWD. + // Build snapshots once. Renderers read these caches instead of doing + // path canonicalization, shell parsing, or filesystem reads per frame. + let prominent_details = build_prominent_details(category, params, workspace.as_deref()); let diff_preview = build_diff_preview(tool_name, params, workspace.as_deref()); Self { @@ -223,72 +246,22 @@ impl ApprovalRequest { params: params.clone(), approval_key: approval_key.to_string(), approval_grouping_key, - workspace, diff_preview, + prominent_details, } } - /// Extract the most important param values as (label, value) pairs for - /// prominent display in the approval widget. Returns pairs like - /// `[("Command", "npm run build"), ("Dir", "/home/user")]`. - pub fn prominent_details(&self) -> Vec<(String, String)> { - let mut details = Vec::new(); - match self.category { - ToolCategory::Shell => { - // Shell commands stay verbatim — the popup body uses - // `Paragraph::wrap`, so it folds long lines on its own and - // an in-band `...` truncation just hides the tail of the - // command the user is being asked to approve. - if let Some(cmd) = param_text(&self.params, &["command", "cmd"]) { - details.push(("Command".into(), cmd)); - } - if let Some(dir) = param_preview(&self.params, &["workdir", "cwd"], 96) { - let is_current = self.workspace.as_ref().is_some_and(|ws| { - let a = std::path::Path::new(&dir); - let b = std::path::Path::new(ws); - a.canonicalize().unwrap_or_else(|_| a.to_path_buf()) - == b.canonicalize().unwrap_or_else(|_| b.to_path_buf()) - }); - let label = if is_current { - "(current)".to_string() - } else { - dir - }; - details.push(("Dir".into(), label)); - } - } - ToolCategory::FileWrite => { - if let Some(path) = - param_preview(&self.params, &["path", "target", "destination"], 200) - { - details.push(("File".into(), path)); - } - } - ToolCategory::Safe => { - if let Some(path) = param_preview(&self.params, &["path", "ref_id", "uri"], 200) { - details.push(("Path".into(), path)); - } - } - ToolCategory::Network => { - if let Some(target) = param_preview( - &self.params, - &["url", "q", "query", "location", "repo"], - 200, - ) { - details.push(("Target".into(), target)); - } - } - _ => { - if let Some(val) = param_preview( - &self.params, - &["command", "path", "url", "q", "query", "ref_id"], - 200, - ) { - details.push(("Input".into(), val)); - } - } - } - details + /// Extract the most important param values for the approval widget. + #[must_use] + pub fn prominent_detail_items(&self, locale: Locale) -> Vec { + self.prominent_details + .iter() + .map(|detail| ApprovalDetail { + label: localize_detail_label(&detail.label, locale).to_string(), + value: detail.value.clone(), + shell_lines: detail.shell_lines.clone(), + }) + .collect() } /// Cached diff/preview snapshot, or `None` when the tool isn't a file @@ -298,27 +271,20 @@ impl ApprovalRequest { pub fn diff_preview(&self) -> Option<&ApprovalDiffPreview> { self.diff_preview.as_ref() } +} - /// Like [`prominent_details`] but with localized labels. - pub fn prominent_details_for_locale(&self, locale: Locale) -> Vec<(String, String)> { - self.prominent_details() - .into_iter() - .map(|(label, value)| { - let localized = match locale { - Locale::ZhHans => match label.as_str() { - "Command" => "命令", - "Dir" => "目录", - "File" => "文件", - "Path" => "路径", - "Target" => "目标", - "Input" => "输入", - _ => &label, - }, - _ => &label, - }; - (localized.to_string(), value) - }) - .collect() +fn localize_detail_label(label: &str, locale: Locale) -> &str { + match locale { + Locale::ZhHans => match label { + "Command" => "命令", + "Dir" => "目录", + "File" => "文件", + "Path" => "路径", + "Target" => "目标", + "Input" => "输入", + _ => label, + }, + _ => label, } } @@ -418,6 +384,248 @@ fn param_text(params: &Value, keys: &[&str]) -> Option { None } +fn build_prominent_details( + category: ToolCategory, + params: &Value, + workspace: Option<&str>, +) -> Vec { + let mut details = Vec::new(); + match category { + ToolCategory::Shell => { + if let Some(cmd) = param_text(params, &["command", "cmd"]) { + let shell_lines = format_shell_command_for_approval(&cmd); + details.push(ApprovalDetail { + label: "Command".into(), + value: cmd, + shell_lines: Some(shell_lines), + }); + } + if let Some(raw_dir) = param_text(params, &["workdir", "cwd"]) + .or_else(|| param_preview(params, &["workdir", "cwd"], 96)) + { + let is_current = workspace.is_some_and(|ws| paths_equivalent(&raw_dir, ws)); + let value = if is_current { + "(current)".to_string() + } else { + truncate_string_value(&raw_dir, 96) + }; + details.push(ApprovalDetail { + label: "Dir".into(), + value, + shell_lines: None, + }); + } + } + ToolCategory::FileWrite => { + if let Some(path) = param_preview(params, &["path", "target", "destination"], 200) { + details.push(ApprovalDetail { + label: "File".into(), + value: path, + shell_lines: None, + }); + } + } + ToolCategory::Safe => { + if let Some(path) = param_preview(params, &["path", "ref_id", "uri"], 200) { + details.push(ApprovalDetail { + label: "Path".into(), + value: path, + shell_lines: None, + }); + } + } + ToolCategory::Network => { + if let Some(target) = + param_preview(params, &["url", "q", "query", "location", "repo"], 200) + { + details.push(ApprovalDetail { + label: "Target".into(), + value: target, + shell_lines: None, + }); + } + } + _ => { + if let Some(val) = param_preview( + params, + &["command", "path", "url", "q", "query", "ref_id"], + 200, + ) { + details.push(ApprovalDetail { + label: "Input".into(), + value: val, + shell_lines: None, + }); + } + } + } + details +} + +fn paths_equivalent(left: &str, right: &str) -> bool { + let left = Path::new(left); + let right = Path::new(right); + left.canonicalize().unwrap_or_else(|_| left.to_path_buf()) + == right.canonicalize().unwrap_or_else(|_| right.to_path_buf()) +} + +pub(crate) fn format_shell_command_for_approval(command: &str) -> Vec { + if let Some(write) = parse_printf_write_file_command(command) { + return format_printf_write_file_preview(write); + } + + let mut out = Vec::new(); + for raw in command.split('\n') { + let mut current = String::new(); + let mut chars = raw.chars().peekable(); + let mut quote: Option = None; + while let Some(ch) = chars.next() { + if matches!(ch, '"' | '\'') { + if quote == Some(ch) { + quote = None; + } else if quote.is_none() { + quote = Some(ch); + } + current.push(ch); + continue; + } + + if quote.is_none() && ch == '&' && chars.peek() == Some(&'&') { + chars.next(); + push_shell_clause(&mut out, &mut current, Some("&&")); + continue; + } + + if quote.is_none() && ch == '|' { + push_shell_clause(&mut out, &mut current, Some("|")); + continue; + } + + if quote.is_none() && ch == ';' { + current.push(';'); + push_shell_clause(&mut out, &mut current, None); + continue; + } + + if quote.is_none() && matches!(ch, '>' | '<') { + current.push(ch); + while chars + .peek() + .is_some_and(|next| matches!(next, '>' | '<' | '&')) + { + current.push(chars.next().expect("peeked char exists")); + } + while chars.peek().is_some_and(|next| next.is_whitespace()) { + current.push(chars.next().expect("peeked char exists")); + } + while chars + .peek() + .is_some_and(|next| !next.is_whitespace() && !matches!(next, '&' | '|' | ';')) + { + current.push(chars.next().expect("peeked char exists")); + } + continue; + } + + current.push(ch); + } + push_shell_clause(&mut out, &mut current, None); + } + + if out.is_empty() { + vec![String::new()] + } else { + out + } +} + +struct PrintfWriteFilePreview { + target: String, + lines: Vec, +} + +fn parse_printf_write_file_command(command: &str) -> Option { + let (before_redirect, after_redirect) = split_unquoted_redirect(command)?; + let before_redirect = before_redirect.trim(); + if !before_redirect.starts_with("printf") { + return None; + } + let tokens = shlex::split(before_redirect)?; + if tokens.len() < 3 || tokens.first()?.as_str() != "printf" { + return None; + } + let fmt = tokens.get(1)?.as_str(); + if fmt != "%s\n" && fmt != "%s\\n" { + return None; + } + let target_tokens = shlex::split(after_redirect.trim_start_matches(['>', '&']).trim_start())?; + if target_tokens.len() != 1 { + return None; + } + let target = target_tokens.into_iter().next()?; + let lines = tokens.into_iter().skip(2).collect::>(); + if lines.is_empty() { + return None; + } + Some(PrintfWriteFilePreview { target, lines }) +} + +fn format_printf_write_file_preview(write: PrintfWriteFilePreview) -> Vec { + let mut out = Vec::new(); + out.push("Write file via printf".to_string()); + out.push(format!("Target: {}", write.target)); + out.push(format!("Lines: {}", write.lines.len())); + out.push(String::new()); + out.push("Content:".to_string()); + let total = write.lines.len(); + for (idx, line) in write.lines.into_iter().enumerate().take(12) { + out.push(format!("{:>4} {}", idx + 1, line)); + } + if total > 12 { + out.push(format!(" ... (+{} more lines)", total - 12)); + } + out +} + +fn split_unquoted_redirect(command: &str) -> Option<(&str, &str)> { + let mut quote: Option = None; + for (idx, ch) in command.char_indices() { + if matches!(ch, '"' | '\'') { + if quote == Some(ch) { + quote = None; + } else if quote.is_none() { + quote = Some(ch); + } + continue; + } + if quote.is_none() && ch == '>' { + let before = &command[..idx]; + let after = &command[idx + ch.len_utf8()..]; + return Some((before, after)); + } + } + None +} + +fn push_shell_clause(out: &mut Vec, current: &mut String, connector: Option<&str>) { + let trimmed = current.trim(); + if !trimmed.is_empty() { + let mut line = trimmed.to_string(); + if let Some(connector) = connector { + line.push(' '); + line.push_str(connector); + } + out.push(line); + } else if let Some(connector) = connector + && let Some(prev) = out.last_mut() + && !prev.ends_with(connector) + { + prev.push(' '); + prev.push_str(connector); + } + current.clear(); +} + /// Resolve a tool-supplied path against the workspace when it's relative. /// Absolute paths are returned unchanged so `write_file` to `/etc/foo` still /// shows the right diff. The original string flows through if there's no @@ -465,6 +673,30 @@ fn count_diff_changes(diff: &str) -> (usize, usize) { (added, deleted) } +enum PreviewFileRead { + Content(String), + Skipped(ApprovalDiffPreview), + Unreadable, +} + +fn read_file_for_diff(resolved: &Path, display_path: &str) -> PreviewFileRead { + if let Ok(metadata) = std::fs::metadata(resolved) + && metadata.is_file() + && metadata.len() > DIFF_PREVIEW_MAX_BYTES + { + return PreviewFileRead::Skipped(ApprovalDiffPreview::SkippedLargeFile { + path: display_path.to_string(), + size: metadata.len(), + limit: DIFF_PREVIEW_MAX_BYTES, + }); + } + + match std::fs::read_to_string(resolved) { + Ok(content) => PreviewFileRead::Content(content), + Err(_) => PreviewFileRead::Unreadable, + } +} + /// Build the diff snapshot for an approval request. Reads the filesystem /// at most once per request — relative paths resolve against `workspace` /// so previews work when the agent is rooted elsewhere from the TUI's CWD. @@ -479,8 +711,8 @@ pub fn build_diff_preview( let search = params.get("search")?.as_str()?; let replace = params.get("replace")?.as_str()?; let resolved = resolve_workspace_path(path, workspace); - match std::fs::read_to_string(&resolved) { - Ok(file) => { + match read_file_for_diff(&resolved, path) { + PreviewFileRead::Content(file) => { let count = file.matches(search).count(); if count == 0 { // search isn't present — render the search/replace pair @@ -513,7 +745,8 @@ pub fn build_diff_preview( } } } - Err(_) => { + PreviewFileRead::Skipped(preview) => Some(preview), + PreviewFileRead::Unreadable => { // File missing — fall back to inputs-only diff. edit_file // would fail at execution anyway, so MissingMatch is the // honest framing. @@ -530,8 +763,8 @@ pub fn build_diff_preview( let path = params.get("path")?.as_str()?; let new_content = params.get("content")?.as_str()?; let resolved = resolve_workspace_path(path, workspace); - match std::fs::read_to_string(&resolved) { - Ok(old_content) => { + match read_file_for_diff(&resolved, path) { + PreviewFileRead::Content(old_content) => { let text = crate::tools::diff_format::make_unified_diff( path, &old_content, @@ -550,7 +783,8 @@ pub fn build_diff_preview( }) } } - Err(_) => Some(ApprovalDiffPreview::NewFile { + PreviewFileRead::Skipped(preview) => Some(preview), + PreviewFileRead::Unreadable => Some(ApprovalDiffPreview::NewFile { path: path.to_string(), content: new_content.to_string(), }), @@ -582,7 +816,11 @@ pub fn build_diff_preview( continue; }; let resolved = resolve_workspace_path(path, workspace); - let old_content = std::fs::read_to_string(&resolved).unwrap_or_default(); + let old_content = match read_file_for_diff(&resolved, path) { + PreviewFileRead::Content(content) => content, + PreviewFileRead::Skipped(preview) => return Some(preview), + PreviewFileRead::Unreadable => String::new(), + }; let fragment = crate::tools::diff_format::make_unified_diff( path, &old_content, @@ -715,6 +953,7 @@ pub struct ApprovalView { diff_scroll: Cell, diff_total_lines: Cell, diff_visible_lines: Cell, + diff_panel_cache: RefCell>, } impl ApprovalView { @@ -735,6 +974,7 @@ impl ApprovalView { diff_scroll: Cell::new(0), diff_total_lines: Cell::new(0), diff_visible_lines: Cell::new(0), + diff_panel_cache: RefCell::new(None), } } @@ -791,6 +1031,33 @@ impl ApprovalView { scroll } + pub(crate) fn cached_diff_panel( + &self, + width: u16, + locale: Locale, + ) -> Option { + let cache = self.diff_panel_cache.borrow(); + let cache = cache.as_ref()?; + if cache.width == width && cache.locale == locale { + Some(cache.panel.clone()) + } else { + None + } + } + + pub(crate) fn set_cached_diff_panel( + &self, + width: u16, + locale: Locale, + panel: RenderedDiffPanel, + ) { + *self.diff_panel_cache.borrow_mut() = Some(DiffPanelCache { + width, + locale, + panel, + }); + } + fn scroll_diff_up(&mut self, amount: usize) { self.diff_scroll .set(self.diff_scroll.get().saturating_sub(amount)); @@ -1089,6 +1356,12 @@ fn push_approval_changes( let diff = crate::tools::diff_format::make_unified_diff(path, "", content); lines.extend(crate::tui::diff_render::render_diff(&diff, width)); } + Some(ApprovalDiffPreview::SkippedLargeFile { size, limit, .. }) => { + lines.push(Line::from(Span::styled( + format!(" (diff preview skipped - file is {size} bytes, limit is {limit} bytes)"), + muted_style, + ))); + } Some(ApprovalDiffPreview::NoChange { .. }) | None => { lines.push(Line::from(Span::styled( " (no textual changes - content matches current file)".to_string(), @@ -1523,11 +1796,15 @@ mod tests { ¶ms, "test_key", ); - let details = request.prominent_details(); - assert_eq!(details[0].0, "Command"); - assert_eq!(details[0].1, "npm run build"); - assert_eq!(details[1].0, "Dir"); - assert_eq!(details[1].1, "/home/user"); + let details = request.prominent_detail_items(Locale::En); + assert_eq!(details[0].label, "Command"); + assert_eq!(details[0].value, "npm run build"); + assert_eq!( + details[0].shell_lines.as_deref(), + Some(&["npm run build".to_string()][..]) + ); + assert_eq!(details[1].label, "Dir"); + assert_eq!(details[1].value, "/home/user"); } #[test] @@ -1541,12 +1818,17 @@ mod tests { let params = json!({"command": cmd, "cwd": "/home/user"}); let request = ApprovalRequest::new("test-id", "exec_shell", "Run shell", ¶ms, "test_key"); - let details = request.prominent_details(); - assert_eq!(details[0].0, "Command"); + let details = request.prominent_detail_items(Locale::En); + assert_eq!(details[0].label, "Command"); assert_eq!( - details[0].1, cmd, + details[0].value, cmd, "shell command must be returned verbatim, no `…` truncation", ); + let shell_lines = details[0] + .shell_lines + .as_ref() + .expect("shell command lines should be cached"); + assert!(shell_lines.iter().any(|line| line.contains("cat /tmp/x"))); } #[test] @@ -1560,9 +1842,9 @@ mod tests { "test_key", Some("/home/user/project".to_string()), ); - let details = request.prominent_details(); - assert_eq!(details[1].0, "Dir"); - assert_eq!(details[1].1, "(current)"); + let details = request.prominent_detail_items(Locale::En); + assert_eq!(details[1].label, "Dir"); + assert_eq!(details[1].value, "(current)"); } #[test] @@ -1570,9 +1852,9 @@ mod tests { let params = json!({"path": "src/main.rs", "content": "fn main() {}"}); let request = ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); - let details = request.prominent_details(); - assert_eq!(details[0].0, "File"); - assert_eq!(details[0].1, "src/main.rs"); + let details = request.prominent_detail_items(Locale::En); + assert_eq!(details[0].label, "File"); + assert_eq!(details[0].value, "src/main.rs"); } #[test] @@ -1673,6 +1955,27 @@ mod tests { let _ = std::fs::remove_file(&path); } + #[test] + fn test_diff_preview_write_file_large_existing_skips_inline_diff() { + let path = std::env::temp_dir().join("deepseek_test_diff_large_file.txt"); + std::fs::write(&path, "x".repeat((DIFF_PREVIEW_MAX_BYTES + 1) as usize)).unwrap(); + let params = json!({"path": path.display().to_string(), "content": "new content +"}); + let request = + ApprovalRequest::new("test-id", "write_file", "Write file", ¶ms, "test_key"); + let preview = request + .diff_preview() + .expect("large existing file should still produce a preview state"); + match preview { + ApprovalDiffPreview::SkippedLargeFile { size, limit, .. } => { + assert!(*size > *limit); + assert_eq!(*limit, DIFF_PREVIEW_MAX_BYTES); + } + other => panic!("expected SkippedLargeFile, got {other:?}"), + } + let _ = std::fs::remove_file(&path); + } + #[test] fn test_diff_preview_write_file_unchanged_shows_no_change() { // write_file with content identical to the file's current bytes used diff --git a/crates/tui/src/tui/pager.rs b/crates/tui/src/tui/pager.rs index c1cc5680b..dd434bf69 100644 --- a/crates/tui/src/tui/pager.rs +++ b/crates/tui/src/tui/pager.rs @@ -491,6 +491,21 @@ fn wrap_text_preserving_spaces(text: &str, width: usize) -> Vec { if width == 0 { return vec![text.to_string()]; } + if text.is_empty() { + return vec![String::new()]; + } + + let leading_indent = text + .chars() + .take_while(|ch| ch.is_whitespace()) + .collect::(); + let indent_width = display_width(&leading_indent); + let continuation_indent = if indent_width < width { + leading_indent + } else { + String::new() + }; + let mut lines = Vec::new(); let mut current = String::new(); let mut current_width = 0usize; @@ -498,8 +513,28 @@ fn wrap_text_preserving_spaces(text: &str, width: usize) -> Vec { for ch in text.chars() { let char_width = ch.width().unwrap_or(1); if current_width + char_width > width && current_width > 0 { - lines.push(std::mem::take(&mut current)); - current_width = 0; + if let Some((break_idx, break_len)) = last_word_break(¤t) { + let line = current[..break_idx].trim_end().to_string(); + let mut rest = current[break_idx + break_len..].trim_start().to_string(); + lines.push(line); + current.clear(); + current_width = 0; + + let rest_width = display_width(&rest); + if !rest.is_empty() + && !continuation_indent.is_empty() + && display_width(&continuation_indent) + rest_width <= width + { + current.push_str(&continuation_indent); + current_width += display_width(&continuation_indent); + } + current.push_str(&rest); + current_width += rest_width; + rest.clear(); + } else { + lines.push(std::mem::take(&mut current)); + current_width = 0; + } } current.push(ch); current_width += char_width; @@ -514,6 +549,25 @@ fn wrap_text_preserving_spaces(text: &str, width: usize) -> Vec { lines } +fn last_word_break(text: &str) -> Option<(usize, usize)> { + let mut seen_non_ws = false; + let mut last = None; + for (idx, ch) in text.char_indices() { + if ch.is_whitespace() { + if seen_non_ws { + last = Some((idx, ch.len_utf8())); + } + } else { + seen_non_ws = true; + } + } + last +} + +fn display_width(text: &str) -> usize { + text.chars().map(|ch| ch.width().unwrap_or(1)).sum() +} + #[cfg(test)] mod tests { use super::*; @@ -973,6 +1027,20 @@ mod tests { assert_eq!(p.scroll, bottom); } + #[test] + fn wrap_text_prefers_word_boundaries_for_english() { + let lines = wrap_text_preserving_spaces("alpha beta gamma delta", 12); + + assert_eq!(lines, vec!["alpha beta", "gamma delta"]); + } + + #[test] + fn wrap_text_keeps_continuation_indent() { + let lines = wrap_text_preserving_spaces(" alpha beta gamma", 14); + + assert_eq!(lines, vec![" alpha", " beta gamma"]); + } + #[test] fn wrap_text_breaks_overlong_cjk_runs() { let text = "这是一个非常长的中文字符串".repeat(10); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index e21a168d7..95db1e0a2 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -27,7 +27,7 @@ use crate::palette; use crate::tui::app::{App, AppMode, ComposerDensity, VimMode}; use crate::tui::approval::{ ApprovalDiffPreview, ApprovalRequest, ApprovalView, ElevationOption, ElevationRequest, - RiskLevel, ToolCategory, + RenderedDiffPanel, RiskLevel, ToolCategory, }; use crate::tui::history::HistoryCell; use crate::tui::scrolling::TranscriptLineMeta; @@ -1122,7 +1122,7 @@ impl Renderable for ApprovalWidget<'_> { lines.push(Line::from("")); // Prominent key details (command, path, target) are the default // body. Full JSON stays behind `v`. - let details = self.request.prominent_details_for_locale(locale); + let details = self.request.prominent_detail_items(locale); if details.is_empty() { lines.push(Line::from(vec![ Span::raw(" "), @@ -1135,13 +1135,14 @@ impl Renderable for ApprovalWidget<'_> { ), ])); } else { - for (label, value) in &details { + for detail in &details { if self.request.category == ToolCategory::Shell - && matches!(label.as_str(), "Command" | "命令") + && matches!(detail.label.as_str(), "Command" | "命令") + && let Some(shell_lines) = detail.shell_lines.as_deref() { - push_shell_command_lines(&mut lines, label, value); + push_shell_command_lines(&mut lines, &detail.label, shell_lines); } else { - push_detail_line(&mut lines, label, value); + push_detail_line(&mut lines, &detail.label, &detail.value); } } } @@ -1151,7 +1152,19 @@ impl Renderable for ApprovalWidget<'_> { if let Some(preview) = self.request.diff_preview() { let diff_width = card_area.width.saturating_sub(4).max(20); - let (header, body_lines) = build_diff_panel(preview, diff_width, locale); + let RenderedDiffPanel { + header, + lines: body_lines, + } = self + .view + .cached_diff_panel(diff_width, locale) + .unwrap_or_else(|| { + let (header, lines) = build_diff_panel(preview, diff_width, locale); + let panel = RenderedDiffPanel { header, lines }; + self.view + .set_cached_diff_panel(diff_width, locale, panel.clone()); + panel + }); let body_height = card_area.height.saturating_sub(4) as usize; let footer_rows = match (risk, pending) { (RiskLevel::Destructive, Some(_)) if !details.is_empty() => 2, @@ -1264,15 +1277,15 @@ impl Renderable for ApprovalWidget<'_> { }; // Show what is being confirmed so the user never loses // context between the selection and the commit step. - if let Some((label, value)) = details.first() { + if let Some(detail) = details.first() { lines.push(Line::from(vec![ Span::raw(" "), Span::styled( - confirm_label(locale, label), + confirm_label(locale, &detail.label), Style::default().fg(palette::TEXT_BODY), ), Span::styled( - value.clone(), + detail.value.clone(), Style::default() .fg(palette::TEXT_BODY) .add_modifier(Modifier::BOLD), @@ -1581,6 +1594,13 @@ fn build_diff_panel( )); (no_change_word(locale).to_string(), vec![line]) } + ApprovalDiffPreview::SkippedLargeFile { size, limit, .. } => { + let line = Line::from(Span::styled( + skipped_large_file_text(*size, *limit, locale), + Style::default().fg(palette::TEXT_MUTED), + )); + (preview_skipped_word(locale).to_string(), vec![line]) + } ApprovalDiffPreview::MissingMatch { path: _, text, @@ -1636,6 +1656,22 @@ fn no_change_word(locale: Locale) -> &'static str { } } +fn preview_skipped_word(locale: Locale) -> &'static str { + match locale { + Locale::ZhHans => "预览已跳过", + _ => "Preview skipped", + } +} + +fn skipped_large_file_text(size: u64, limit: u64, locale: Locale) -> String { + match locale { + Locale::ZhHans => format!("文件过大，已跳过差异预览（{size} bytes，限制 {limit} bytes）"), + _ => format!( + "file is too large for an inline diff preview ({size} bytes, limit {limit} bytes)" + ), + } +} + fn no_change_text(locale: Locale) -> &'static str { match locale { Locale::ZhHans => " 内容与当前文件一致，无需写入。", @@ -1696,7 +1732,7 @@ fn push_detail_line(lines: &mut Vec>, label: &str, value: &str) { ])); } -fn push_shell_command_lines(lines: &mut Vec>, label: &str, command: &str) { +fn push_shell_command_lines(lines: &mut Vec>, label: &str, command_lines: &[String]) { lines.push(Line::from(vec![ Span::raw(" "), Span::styled( @@ -1707,11 +1743,11 @@ fn push_shell_command_lines(lines: &mut Vec>, label: &str, command ), ])); - for line in format_shell_command_for_approval(command) { + for line in command_lines { lines.push(Line::from(vec![ Span::raw(" "), Span::styled( - line, + line.clone(), Style::default() .fg(palette::TEXT_BODY) .add_modifier(Modifier::BOLD), @@ -1720,163 +1756,6 @@ fn push_shell_command_lines(lines: &mut Vec>, label: &str, command } } -fn format_shell_command_for_approval(command: &str) -> Vec { - if let Some(write) = parse_printf_write_file_command(command) { - return format_printf_write_file_preview(write); - } - - let mut out = Vec::new(); - for raw in command.split('\n') { - let mut current = String::new(); - let mut chars = raw.chars().peekable(); - let mut quote: Option = None; - while let Some(ch) = chars.next() { - if matches!(ch, '"' | '\'') { - if quote == Some(ch) { - quote = None; - } else if quote.is_none() { - quote = Some(ch); - } - current.push(ch); - continue; - } - - if quote.is_none() && ch == '&' && chars.peek() == Some(&'&') { - chars.next(); - push_shell_clause(&mut out, &mut current, Some("&&")); - continue; - } - - if quote.is_none() && ch == '|' { - push_shell_clause(&mut out, &mut current, Some("|")); - continue; - } - - if quote.is_none() && ch == ';' { - current.push(';'); - push_shell_clause(&mut out, &mut current, None); - continue; - } - - if quote.is_none() && matches!(ch, '>' | '<') { - current.push(ch); - while chars - .peek() - .is_some_and(|next| matches!(next, '>' | '<' | '&')) - { - current.push(chars.next().expect("peeked char exists")); - } - while chars.peek().is_some_and(|next| next.is_whitespace()) { - current.push(chars.next().expect("peeked char exists")); - } - while chars - .peek() - .is_some_and(|next| !next.is_whitespace() && !matches!(next, '&' | '|' | ';')) - { - current.push(chars.next().expect("peeked char exists")); - } - continue; - } - - current.push(ch); - } - push_shell_clause(&mut out, &mut current, None); - } - - if out.is_empty() { - vec![String::new()] - } else { - out - } -} - -struct PrintfWriteFilePreview { - target: String, - lines: Vec, -} - -fn parse_printf_write_file_command(command: &str) -> Option { - let (before_redirect, after_redirect) = split_unquoted_redirect(command)?; - let before_redirect = before_redirect.trim(); - if !before_redirect.starts_with("printf") { - return None; - } - let tokens = shlex::split(before_redirect)?; - if tokens.len() < 3 || tokens.first()?.as_str() != "printf" { - return None; - } - let fmt = tokens.get(1)?.as_str(); - if fmt != "%s\n" && fmt != "%s\\n" { - return None; - } - let target_tokens = shlex::split(after_redirect.trim_start_matches(['>', '&']).trim_start())?; - if target_tokens.len() != 1 { - return None; - } - let target = target_tokens.into_iter().next()?; - let lines = tokens.into_iter().skip(2).collect::>(); - if lines.is_empty() { - return None; - } - Some(PrintfWriteFilePreview { target, lines }) -} - -fn format_printf_write_file_preview(write: PrintfWriteFilePreview) -> Vec { - let mut out = Vec::new(); - out.push("Write file via printf".to_string()); - out.push(format!("Target: {}", write.target)); - out.push(format!("Lines: {}", write.lines.len())); - out.push(String::new()); - out.push("Content:".to_string()); - let total = write.lines.len(); - for (idx, line) in write.lines.into_iter().enumerate().take(12) { - out.push(format!("{:>4} {}", idx + 1, line)); - } - if total > 12 { - out.push(format!(" ... (+{} more lines)", total - 12)); - } - out -} - -fn split_unquoted_redirect(command: &str) -> Option<(&str, &str)> { - let mut quote: Option = None; - for (idx, ch) in command.char_indices() { - if matches!(ch, '"' | '\'') { - if quote == Some(ch) { - quote = None; - } else if quote.is_none() { - quote = Some(ch); - } - continue; - } - if quote.is_none() && ch == '>' { - let before = &command[..idx]; - let after = &command[idx + ch.len_utf8()..]; - return Some((before, after)); - } - } - None -} - -fn push_shell_clause(out: &mut Vec, current: &mut String, connector: Option<&str>) { - let trimmed = current.trim(); - if !trimmed.is_empty() { - let mut line = trimmed.to_string(); - if let Some(connector) = connector { - line.push(' '); - line.push_str(connector); - } - out.push(line); - } else if let Some(connector) = connector - && let Some(prev) = out.last_mut() - && !prev.ends_with(connector) - { - prev.push(' '); - prev.push_str(connector); - } - current.clear(); -} - fn diff_hidden_hint(locale: Locale) -> &'static str { match locale { Locale::ZhHans => "终端高度不足，按 v 查看",