feat(rust tui): add delete uri funciton with confirmation and refresh behaviour#696
feat(rust tui): add delete uri funciton with confirmation and refresh behaviour#696xiaobin83 wants to merge 4 commits intovolcengine:mainfrom
Conversation
…h behavior This commit adds two key TUI features: 1. Delete functionality (d key): - Press 'd' to delete the selected file or directory - A confirmation prompt appears: "Delete [file/directory]? (y/n): [URI]" - Press 'y' to confirm deletion or 'n' to cancel - After deletion, the tree refreshes and cursor moves to the next or previous sibling 2. Refresh behavior (r key): - Press 'r' to refresh the entire tree - Loads the root node "viking://" - Restores the cursor to the originally selected node - Maintains user context after refresh
|
|
qin-ctx
left a comment
qin-ctx
left a comment
There was a problem hiding this comment.
Overall a well-structured PR that adds useful TUI features. One blocking bug found: the delete confirmation prompt can be prematurely cleared by the auto-clear timer from a previous status message. 4 additional non-blocking suggestions included.
Also note: no tests were added for the new functionality (delete confirmation state machine, status message auto-clear, tree refresh with state restoration). Consider adding unit tests for at least the timer logic and confirmation flow.
crates/ov_cli/src/tui/event.rs
Outdated
|
|
||
| // Set delete confirmation state | ||
| app.delete_confirmation = Some((selected_uri.clone(), is_dir)); | ||
| app.status_message = format!("Delete {}? (y/n): {}", |
There was a problem hiding this comment.
[Bug] (blocking) The confirmation message is set by directly assigning status_message without resetting status_message_time. If a previous timed message (e.g., "Tree refreshed" from pressing r) set the timer via set_status_message, update_status_message() in the main loop will clear this confirmation prompt when the old 3-second timer expires — even though delete_confirmation is still Some.
This creates a UX inconsistency: the confirmation prompt disappears but the app still silently waits for y/n, with no visible indication to the user.
Suggested fix:
app.delete_confirmation = Some((selected_uri.clone(), is_dir));
app.status_message = format!("Delete {}? (y/n): {}",
if is_dir { "directory" } else { "file" },
selected_uri
);
app.status_message_time = None; // Prevent auto-clear during confirmationThere was a problem hiding this comment.
- refactored the deletion_confirmation to a generic confirmation system.
- when confirmation pops up, no new status message can be set.
- added error message for error happens (for example, when deleting protected uri)
crates/ov_cli/src/tui/app.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub async fn reload_entire_tree(&mut self) { |
There was a problem hiding this comment.
[Design] (non-blocking) reload_entire_tree() and delete_uri() share ~40 lines of nearly identical logic:
- Collecting expanded nodes from
self.tree.visible - Reloading root and restoring expanded state via
expand_node_by_uriloop - Walking parent paths to ensure target URI visibility
- Finding cursor position with parent-fallback closure
Consider extracting a shared helper (e.g., refresh_tree_and_restore(&mut self, target_uri: &str)) to reduce duplication and make future maintenance easier.
There was a problem hiding this comment.
extracted common code into small methods
| app.tree.toggle_expand(&client).await; | ||
| app.load_content_for_selected().await; | ||
| } | ||
| KeyCode::Char('d') => { |
There was a problem hiding this comment.
[Suggestion] (non-blocking) There's no guard against deleting the root node (/) or scope-level URIs (viking://resources, viking://agent, etc.). If the server's rm endpoint doesn't reject these, it could lead to unintended data loss.
Consider adding a check before entering confirmation:
if selected_uri == "/" || Self::ROOT_SCOPES.iter().any(|s| selected_uri == format!("viking://{}", s)) {
app.set_status_message("Cannot delete root or scope directories".to_string());
return;
}There was a problem hiding this comment.
added a tree.allow_deletion method to ensure the those nodes cannot be delete.
crates/ov_cli/src/tui/ui.rs
Outdated
| .style(Style::default().bg(Color::DarkGray).fg(Color::White)); | ||
| frame.render_widget(bar, area); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
[Suggestion] (non-blocking) Missing trailing newline at end of file. Most editors, linters, and POSIX tools expect files to end with a newline character.
Changes: - Replace delete_confirmation with generic confirmation system using callbacks - Add error message handling with dedicated display - Add status message locking during confirmations - Add delete_selected_uri method for cleaner deletion flow - Add deletion protection for root and scope directories - Update UI to show error messages and confirmation prompts - Rename update_status_message to update_messages - Add allow_deletion method to TreeState - Update key handling to prioritize error messages
3 participants
Description
This commit adds two TUI features:
Delete functionality (d key):
Refresh behavior (r key):
Type of Change
Changes Made
Testing
Checklist
Screenshots