fix(tui): structure approval details and shell previews#1765
Conversation
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: <key detail>" in the two-step confirmation footer - Remove generic description/impacts fields from ApprovalRequest
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the tool approval system in the TUI to provide more prominent and relevant details to the user. It introduces a prominent_details method to extract key parameters based on tool categories, replaces the generic impact summary with specific labels like 'Command' or 'File', and adds workspace awareness to identify the current directory. The feedback highlights an unused parameter in new_with_workspace, an inconsistency in parameter key prioritization for shell commands, and potential issues with path comparison. Additionally, several reviewers noted that new user-facing strings are hardcoded rather than being integrated into the existing localization system.
| pub fn new_with_workspace( | ||
| id: &str, | ||
| tool_name: &str, | ||
| _description: &str, |
There was a problem hiding this comment.
The _description parameter is unused because the description field was removed from the ApprovalRequest struct. It should be removed from the function signature and all call sites to keep the API clean.
| let mut details = Vec::new(); | ||
| match self.category { | ||
| ToolCategory::Shell => { | ||
| if let Some(cmd) = param_preview(&self.params, &["cmd", "command"], 120) { |
There was a problem hiding this comment.
The order of keys here (["cmd", "command"]) is inconsistent with classify_risk (line 295) and the default case (line 220), which both prioritize "command". To ensure that the displayed command matches the one used for risk analysis, the order should be unified.
| if let Some(cmd) = param_preview(&self.params, &["cmd", "command"], 120) { | |
| if let Some(cmd) = param_preview(&self.params, &["command", "cmd"], 120) { |
| let is_current = self | ||
| .workspace | ||
| .as_ref() | ||
| .is_some_and(|ws| std::path::Path::new(&dir) == std::path::Path::new(ws)); |
There was a problem hiding this comment.
Comparing paths using == on Path objects is sensitive to normalization differences, such as trailing slashes. For example, /path/to/dir and /path/to/dir/ will not be considered equal. Consider using a more robust comparison method that handles these differences, such as checking if one path is a prefix of the other with an empty remainder using strip_prefix.
| 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()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded user-facing strings and labels should be moved to the project's localization system (crate::localization) to maintain consistency and support all supported locales. Additionally, using hardcoded English strings as keys for manual matching in confirm_label is brittle and complicates adding support for new languages.
| 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(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The strings in option_approve_always are hardcoded for both English and Chinese. These should be integrated into the localization module using MessageId to follow the project's established internationalization pattern.
- 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
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<String>` 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.
`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<Line<'static>>` 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.
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).
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
tdccccc
changed the title
|
This PR was opened before the v0.8.41 rebrand and is now stale. Feel free to rebase onto current |
2 participants
What
Verification