fix(tui): structure approval details and shell previews#1991
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
- 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
![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 significantly improves the TUI approval workflow by introducing a scrollable diff preview and prominent tool details (such as commands, directories, and file paths) directly within the approval modal. It adds a compact diff renderer to maximize space and updates the pager to preserve indentation for code and diffs. My feedback focuses on performance optimizations within the TUI render loop: specifically, addressing blocking I/O operations and redundant computations—such as path canonicalization, shell command parsing, and diff rendering—that are currently executed every frame. Additionally, I recommend refining the text-wrapping logic to balance indentation preservation with the readability of natural language text.
| let request = ApprovalRequest::new_with_workspace( | ||
| &id, | ||
| &tool_name, | ||
| &description, | ||
| &tool_input, | ||
| &input, | ||
| &approval_key, | ||
| Some(app.workspace.display().to_string()), | ||
| ); |
There was a problem hiding this comment.
The call to ApprovalRequest::new_with_workspace triggers build_diff_preview, which performs synchronous filesystem operations (std::fs::read_to_string). Since this is called within the main TUI event loop (run_event_loop), it will block the UI thread. If the target file is large or located on a slow filesystem, the TUI will freeze until the read is complete. Consider performing these I/O operations asynchronously or using spawn_blocking before constructing the request.
| 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()) | ||
| }); |
There was a problem hiding this comment.
The canonicalize() calls here are problematic for two reasons: 1) They are blocking system calls executed every frame within the render loop (via prominent_details_for_locale in ApprovalWidget::render), and 2) b.canonicalize() is re-evaluating the constant workspace path repeatedly. This will cause significant UI lag, especially on slow filesystems. You should canonicalize the workspace path once during ApprovalRequest construction and store it, and ideally pre-calculate the is_current flag as well.
| fn wrap_text_preserving_spaces(text: &str, width: usize) -> Vec<String> { | ||
| if width == 0 { | ||
| return vec![text.to_string()]; | ||
| } | ||
| let mut lines = Vec::new(); | ||
| 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() { |
There was a problem hiding this comment.
The new wrap_text_preserving_spaces implementation performs a strict character-based wrap, which will break words in the middle when they exceed the remaining line width. While this preserves indentation (which is important for diffs), it significantly degrades readability for natural language text displayed in the pager. Consider an implementation that attempts to break at whitespace first, falling back to character-breaking only when a single word exceeds the total width.
| if self.request.category == ToolCategory::Shell | ||
| && matches!(label.as_str(), "Command" | "命令") | ||
| { | ||
| push_shell_command_lines(&mut lines, label, value); |
There was a problem hiding this comment.
push_shell_command_lines calls format_shell_command_for_approval, which performs complex string parsing to split shell commands into readable clauses. Executing this logic every frame in the render loop is inefficient. Consider pre-parsing the command and caching the resulting lines in the ApprovalRequest during construction.
| lines.push(Line::from("")); | ||
| 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); |
There was a problem hiding this comment.
build_diff_panel is called every frame during rendering and it re-renders the entire diff text into a Vec<Line> using render_diff_compact. For large diffs, this is computationally expensive and redundant if the diff content and width haven't changed. Consider caching the rendered lines in ApprovalView or ApprovalRequest and only re-rendering when the available width changes.
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
1 participant
What\n- Replaces #1765 after rebasing onto the current rebranded main branch\n- Render approval details with structured fields instead of raw JSON\n- Improve shell command formatting and special-case printf-based file writes into a readable preview\n- Preserve diff/pager indentation and keep Up/Down for scrolling\n- Add regression coverage for approval details, shell previews, diff indentation, and pager wrapping\n\n## Verification\n- cargo fmt --all --check\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 diff_render::tests -- --nocapture\n- cargo test -p codewhale-tui pager::tests -- --nocapture\n- cargo build