fix(tui): structure approval details and shell previews#2269
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
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
![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 TUI approval system to precompute and cache diff previews and prominent details at construction time, avoiding filesystem reads during render frames. It also introduces a compact diff renderer to save vertical space in the popup, implements a two-step confirmation flow for destructive actions, and improves text wrapping in the pager view. The review feedback highlights three important issues: a parsing bug in split_unquoted_redirect when handling escaped quotes, a broken preview in the details pager for new files because raw content is passed directly to render_diff, and potential TUI freezes caused by blocking synchronous filesystem I/O inside the async event loop.
| fn split_unquoted_redirect(command: &str) -> Option<(&str, &str)> { | ||
| let mut quote: Option<char> = 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); | ||
| } | ||
| impacts | ||
| continue; | ||
| } | ||
| ToolCategory::FileWrite => { | ||
| let mut impacts = vec!["会写入工作区或已批准写入范围内的文件。".to_string()]; | ||
| if let Some(path) = param_preview(params, &["path", "target", "destination"], 72) { | ||
| impacts.push(format!("写入：{path}")); | ||
| } | ||
| impacts | ||
| if quote.is_none() && ch == '>' { | ||
| let before = &command[..idx]; | ||
| let after = &command[idx + ch.len_utf8()..]; | ||
| return Some((before, after)); | ||
| } | ||
| ToolCategory::Shell => { | ||
| let mut impacts = vec!["执行 shell 命令。".to_string()]; | ||
| if let Some(command) = param_preview(params, &["cmd", "command"], 96) { | ||
| impacts.push(format!("命令：{command}")); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
The split_unquoted_redirect function does not handle escaped quotes (e.g., \" or \\'). If an escaped quote appears inside a quoted string, the parser will incorrectly treat it as a closing quote. This causes subsequent quoted text to be treated as unquoted, which can lead to incorrect splitting if a > character appears later inside the quotes (e.g., printf '%s\n' "escaped \" quote > here" > file.txt).
To fix this, track backslash escapes when outside of single quotes.
fn split_unquoted_redirect(command: &str) -> Option<(&str, &str)> {
let mut quote: Option<char> = None;
let mut escaped = false;
for (idx, ch) in command.char_indices() {
if escaped {
escaped = false;
continue;
}
if ch == '\\' && quote != Some('\'') {
escaped = true;
continue;
}
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
}| 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("")); | ||
| } |
There was a problem hiding this comment.
For ApprovalDiffPreview::NewFile, preview.diff_text() returns the raw file content rather than a unified diff. Passing this raw content directly to render_diff will cause any lines that do not start with +, -, or to be completely ignored and skipped, resulting in a broken or empty preview in the details pager.
To fix this, match on the ApprovalDiffPreview variants directly (similar to how it is done in widgets/mod.rs) and synthesize a proper unified diff for the NewFile case.
if let Some(preview) = preview.as_ref() {
lines.push(Line::from(Span::styled("Changes:", label_style)));
match preview {
ApprovalDiffPreview::Diff { text, .. }
| 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, diff_width));
}
}
ApprovalDiffPreview::NewFile { path, content } => {
let diff = crate::tools::diff_format::make_unified_diff(path, "", content);
lines.extend(crate::tui::diff_render::render_diff(&diff, diff_width));
}
ApprovalDiffPreview::SkippedLargeFile { size, limit, .. } => {
lines.push(Line::from(Span::styled(
format!(" (diff preview skipped - file is {size} bytes, limit is {limit} bytes)"),
muted_style,
)));
}
ApprovalDiffPreview::NoChange { .. } => {
lines.push(Line::from(Span::styled(
" (no textual changes — content matches current file)".to_string(),
muted_style,
)));
}
}
lines.push(Line::from(""));
}| 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
Performing synchronous filesystem I/O (std::fs::metadata and std::fs::read_to_string) inside read_file_for_diff blocks the async executor thread because ApprovalRequest::new_with_workspace is called directly within the async run_event_loop in ui.rs. If the filesystem is slow (e.g., network shares, slow disks), this can cause the TUI to freeze or stutter. Consider using tokio::fs for asynchronous file reading, or offloading this synchronous construction to tokio::task::spawn_blocking.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Hmbown
left a comment
Hmbown
left a comment
There was a problem hiding this comment.
Approved for the v0.8.48 candidate queue. CI and Greptile are clean after the maintainer follow-ups, and the final diff preserves approval preview context while keeping the shell/diff preview behavior focused. Thanks @tdccccc for the original approval-detail work.
|
Independent review: Compile-check clean on Scope check (2407/508 across 9 files): all changes are TUI display/input-handling. No orthogonal refactors hiding; the engine approval decision plumbing is unchanged (params flow through verbatim). Findings beyond Greptile's two:
Two-step destructive confirm and v0.8.48: no merge conflicts with PR #2256. None of the above blocks; (2) is a small follow-up worth catching pre-cut. |
- Handle escaped quotes in split_unquoted_redirect - Render NewFile/SkippedLargeFile variants properly in detail pager - Offload diff preview I/O to spawn_blocking to avoid blocking the async executor
2 participants
What
Verification
Greptile Summary
This PR rewrites the approval modal to show structured fields (command, file path, target, working directory) instead of raw JSON, adds an inline scrollable diff panel cached at construction time, special-cases
printf-based file-write commands into a readable preview, and introduces a two-step confirmation flow for destructive approvals.approval.rs: newApprovalDiffPreview/ApprovalDetailtypes,build_diff_preview(single read per request,spawn_blockingin the modal path), shell command tokeniser,printf-write-file parser, two-stepcommit_or_stage.diff_render.rs: new compact renderer for the popup,strip_prefixinstead oftrim_start_matches,parse_hunk_headerupgraded toOption<Option<usize>>for zero-line hunks.pager.rs:wrap_text_preserving_spaceskeeps leading indent on continuation lines and prefers word boundaries over hard character breaks.ui.rs/widgets/mod.rs/views/mod.rs: wiring forOpenStyledPager, the inline diff panel, and the confirmation banner;open_details_pager_for_cellenriched with a diff preview — but that call is made synchronously (see comment).Confidence Score: 4/5
Safe to merge with one targeted fix: the detail-pager path in ui.rs reads files synchronously on the async event loop.
The approval modal path is careful to offload filesystem I/O with spawn_blocking, and the diff/pager/widget changes are well-tested. The one gap is open_details_pager_for_cell, which calls build_diff_preview directly without any blocking guard — on a large workspace file this freezes the TUI render thread for the duration of a full file read. That path is new in this PR and is the only issue that needs addressing before merge.
crates/tui/src/tui/ui.rs — the open_details_pager_for_cell function needs the same spawn_blocking treatment already applied to the approval modal path.
Important Files Changed
Comments Outside Diff (1)
crates/tui/src/tui/ui.rs, line 7748-7753 (link)build_diff_previewcallsstd::fs::metadataandstd::fs::read_to_string(up toDIFF_PREVIEW_MAX_BYTES= 512 KB) synchronously. The PR carefully offloads the exact same call for the approval modal viatokio::task::spawn_blocking, butopen_details_pager_for_cellis invoked directly from the asynchandle_view_eventspath with no blocking guard. A tool that modified a large file will freeze the TUI event loop for the duration of the read every time the user opens the detail pager.Reviews (4): Last reviewed commit: "fix(tui): address remaining Gemini revie..." | Re-trigger Greptile