From cf688465c7c08184d4aaad69a8800d908c7804fa Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 28 Jul 2026 13:22:11 +0100 Subject: [PATCH] project_panel: Refactor undo and redo error messages (#60186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Improve the error messages shown when undoing or redoing project panel operations fail. Right now we're mostly relying on the error message generated by the underlying function or method that attempts to apply the inverse operation, which might not be the best UX. ## Solution * Update the style used in the `Workspace::show_notification` call, under `project_panel::undo::Inner::show_error`, in order to use markdown styling. This allows paths to stand out a little bit better, which is helpful seeing as pretty much all error messages will contain path information in the notification's body. * Update the way paths are displayed so as to show the full relative path and, if multiple worktrees are present, include the worktree name. This helps disambiguate cases where multiple worktrees might have the same path, for example, `src/lib.rs`. * Update each specific operation's error message to better convey what exactly failed. ## Testing Manually tested each of the scenarios outlined in the `Showcase` section. Please refer to the screenshots in that section to see before and after comparisons ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [ ] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Showcase ### Undo failures
1. Move conflict | Steps | Before | After | | --- | --- | --- | | 1. Move `a.txt` into `docs/` (cut/paste or drag)
2. Run `touch a.txt`
3. Undo | 1_before | 1_after |
2. Rename conflict | Steps | Before | After | | --- | --- | --- | | 1. Rename `a.txt` → `b.txt`
2. Run `touch a.txt`
3. Undo | 2_before | 2_after |
3. Source no longer exists | Steps | Before | After | | --- | --- | --- | | 1. Move `a.txt` into `docs/`
2. Run `rm docs/a.txt`
3. Undo | 3_before | 3_after |
4. Restore from emptied Trash | Steps | Before | After | | --- | --- | --- | | 1. Delete `a.txt` (moves it to Trash)
2. Empty it from the system Trash
3. Undo | 4_before | 4_after |
5. Restore collision | Steps | Before | After | | --- | --- | --- | | 1. Delete `a.txt` (to Trash)
2. Run `touch a.txt`
3. Undo | 5_before | 5_after |
6. Trash a file that's gone | Steps | Before | After | | --- | --- | --- | | 1. Create `empty.txt` in the panel
2. Run `rm empty.txt`
3. Undo | 6_before | 6_after |
### Redo failures
7. Redo a move into an occupied destination | Steps | Before | After | | --- | --- | --- | | 1. Move `a.txt` into `docs/`, then Undo (file back at root)
2. Run `touch docs/a.txt`
3. Redo | 7_before | 7_after |
8. Redo a restore after the Trash was emptied | Steps | Before | After | | --- | --- | --- | | 1. Create `empty.txt`, then Undo (it gets trashed)
2. Empty it from the system Trash
3. Redo | 8_before | 8_after |
9. Redo a re-trash of a deleted file | Steps | Before | After | | --- | --- | --- | | 1. Delete `a.txt` (to Trash), then Undo (restores it)
2. Run `rm a.txt`
3. Redo | 9_before | 9_after |
--- Release Notes: - N/A --- Cargo.lock | 1 + crates/fs/src/fs.rs | 29 +++- crates/fs/tests/integration/fs_tests.rs | 6 + crates/project_panel/Cargo.toml | 1 + crates/project_panel/src/undo.rs | 212 ++++++++++++++++++------ crates/workspace/src/notifications.rs | 2 +- 6 files changed, 196 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d4333bbd43..0b58f151b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14226,6 +14226,7 @@ dependencies = [ "itertools 0.14.0", "language", "log", + "markdown", "markdown_preview", "menu", "notifications", diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 9b3adda712..009ac99acd 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -168,9 +168,12 @@ pub trait Fs: Send + Sync { async fn is_case_sensitive(&self) -> bool; fn subscribe_to_jobs(&self) -> JobEventReceiver; - /// Restores a given `TrashedEntry`, moving it from the system's trash back - /// to the original path. - async fn restore(&self, item: TrashId) -> std::result::Result; + /// Returns the original absolute path of the item identified by `trash_id`. + fn original_path_for_trash_id(&self, trash_id: TrashId) -> Option; + + /// Restores the item identified by `trash_id`, moving it from the system's + /// trash back to its original path. + async fn restore(&self, trash_id: TrashId) -> std::result::Result; #[cfg(feature = "test-support")] fn as_fake(&self) -> Arc { @@ -1348,11 +1351,18 @@ impl Fs for RealFs { res } - async fn restore(&self, item: TrashId) -> std::result::Result { + fn original_path_for_trash_id(&self, trash_id: TrashId) -> Option { + self.trash + .lock() + .get(trash_id) + .map(|entry| entry.original_parent.join(&entry.name)) + } + + async fn restore(&self, trash_id: TrashId) -> std::result::Result { let trashed_entry = self .trash .lock() - .remove(item) + .remove(trash_id) .ok_or(TrashRestoreError::AlreadyRestored)?; let restored_item_path = trashed_entry.original_parent.join(&trashed_entry.name); @@ -3324,6 +3334,15 @@ impl Fs for FakeFs { receiver } + fn original_path_for_trash_id(&self, trash_id: TrashId) -> Option { + self.state + .lock() + .trash + .lock() + .get(trash_id) + .map(|(entry, _)| entry.original_parent.join(&entry.name)) + } + async fn restore(&self, trash_id: TrashId) -> Result { let mut state = self.state.lock(); diff --git a/crates/fs/tests/integration/fs_tests.rs b/crates/fs/tests/integration/fs_tests.rs index c644229a0d..a32c3bda84 100644 --- a/crates/fs/tests/integration/fs_tests.rs +++ b/crates/fs/tests/integration/fs_tests.rs @@ -697,7 +697,13 @@ async fn test_fake_fs_restore(executor: BackgroundExecutor) { let path = path!("/root/src/file_a.txt").as_ref(); let trashed_entry = fs.trash(path, Default::default()).await.unwrap(); + assert_eq!( + fs.original_path_for_trash_id(trashed_entry), + Some(path.to_path_buf()) + ); + fs.restore(trashed_entry).await.unwrap(); + assert_eq!(fs.original_path_for_trash_id(trashed_entry), None); assert_eq!( fs.files(), diff --git a/crates/project_panel/Cargo.toml b/crates/project_panel/Cargo.toml index 00c6637053..6f9d412579 100644 --- a/crates/project_panel/Cargo.toml +++ b/crates/project_panel/Cargo.toml @@ -44,6 +44,7 @@ client.workspace = true worktree.workspace = true workspace.workspace = true language.workspace = true +markdown.workspace = true markdown_preview.workspace = true zed_actions.workspace = true telemetry.workspace = true diff --git a/crates/project_panel/src/undo.rs b/crates/project_panel/src/undo.rs index 17b9b4b1bb..65c365c2ef 100644 --- a/crates/project_panel/src/undo.rs +++ b/crates/project_panel/src/undo.rs @@ -10,9 +10,9 @@ //! Operations Results //! ───────────────────────────────── ────────────────────────────────────── //! Create(ProjectPath) → Created(ProjectPath) -//! Trash(ProjectPath) → Trashed(TrashId) +//! Trash(ProjectPath) → Trashed(WorktreeId, TrashId) //! Rename(ProjectPath, ProjectPath) → Renamed(ProjectPath, ProjectPath) -//! Restore(TrashId) → Restored(ProjectPath) +//! Restore(WorktreeId, TrashId) → Restored(ProjectPath) //! Batch(Vec) → Batch(Vec) //! //! @@ -59,41 +59,41 @@ //! │ //! User Operation Undo v //! Execute Created(CONTRIBUTING.md) ────────> Trash(CONTRIBUTING.md) -//! Record Trashed(TrashId(1)) +//! Record Trashed(WorktreeId(1), TrashId(1)) //! History //! 0 Created(src/main.rs) -//! 1 Renamed(README.md, readme.md) ─┐ -//! 2 +++cursor+++ │(before the cursor) -//! 2 Trashed(TrashId(1)) │ -//! │ -//! User Operation Undo v +//! 1 Renamed(README.md, readme.md) ─────┐ +//! 2 +++cursor+++ │(before the cursor) +//! 2 Trashed(WorktreeId(1), TrashId(1)) │ +//! │ +//! User Operation Undo v //! Execute Renamed(README.md, readme.md) ───> Rename(readme.md, README.md) //! Record Renamed(readme.md, README.md) //! History //! 0 Created(src/main.rs) //! 1 +++cursor+++ -//! 1 Renamed(readme.md, README.md) ─┐ (at the cursor) -//! 2 Trashed(TrashId(1)) │ -//! │ -//! ┌──────────────────────────────────┴─────────────────────────────────────────┐ +//! 1 Renamed(readme.md, README.md) ─────┐ (at the cursor) +//! 2 Trashed(WorktreeId(1), TrashId(1)) │ +//! │ +//! ┌──────────────────────────────────────┴─────────────────────────────────────┐ //! Redoing will take the result at the cursor position, convert that into the //! operation that can revert that result, execute that operation and replace //! the result in the history with the new result, obtained from running the //! inverse operation, advancing the cursor position. -//! └──────────────────────────────────┬─────────────────────────────────────────┘ -//! │ -//! │ -//! User Operation Redo v +//! └─────────────────────────────────────┬──────────────────────────────────────┘ +//! │ +//! │ +//! User Operation Redo v //! Execute Renamed(readme.md, README.md) ───> Rename(README.md, readme.md) //! Record Renamed(README.md, readme.md) //! History //! 0 Created(src/main.rs) //! 1 Renamed(README.md, readme.md) //! 2 +++cursor+++ -//! 2 Trashed(TrashId(1))───────┐ (at the cursor) +//! 2 Trashed(WorktreeId(1), TrashId(1)) ─┐ (at the cursor) //! │ //! User Operation Redo v -//! Execute Trashed(TrashId(1)) ────────> Restore(TrashId(1)) +//! Execute Trashed(WorktreeId(1), TrashId(1)) ─> Restore(WorktreeId(1), TrashId(1)) //! Record Restored(ProjectPath) //! History //! 0 Created(src/main.rs) @@ -132,16 +132,21 @@ use crate::ProjectPanel; use anyhow::{Context, Result, anyhow}; -use fs::TrashId; +use fs::{TrashId, TrashRestoreError}; use futures::channel::mpsc; -use gpui::{AppContext, AsyncApp, SharedString, Task, WeakEntity}; +use gpui::{AppContext, AsyncApp, IntoElement, SharedString, Styled, Task, WeakEntity}; +use markdown::{Markdown, MarkdownElement}; +use project::Project; use project::{ProjectPath, WorktreeId}; use std::sync::atomic::{AtomicBool, Ordering}; use std::{collections::VecDeque, sync::Arc}; -use ui::App; +use ui::{App, TextSize}; +use util::{paths::PathStyle, rel_path::RelPath}; use workspace::{ Workspace, - notifications::{NotificationId, simple_message_notification::MessageNotification}, + notifications::{ + NotificationId, markdown_style, simple_message_notification::MessageNotification, + }, }; use worktree::CreatedEntry; @@ -313,12 +318,23 @@ impl UndoMessage { UndoMessage::Changed(_) => { "this is a bug in the manage_undo_and_redo task please report" } - UndoMessage::Undo => "Undo failed", - UndoMessage::Redo => "Redo failed", + UndoMessage::Undo => "Undo Failed", + UndoMessage::Redo => "Redo Failed", } } } +fn project_path_display( + project: &Project, + project_path: &ProjectPath, + path_style: PathStyle, + cx: &App, +) -> String { + project + .short_full_path_for_project_path(project_path, cx) + .unwrap_or_else(|| project_path.path.display(path_style).to_string()) +} + impl Inner { async fn manage_undo_and_redo(mut self, mut cx: AsyncApp) { loop { @@ -346,16 +362,19 @@ impl Inner { }; if let Err(e) = res { - Self::show_error(error_title, self.workspace.clone(), e.to_string(), &mut cx); + Self::show_error( + error_title, + self.workspace.clone(), + format!("{e:#}"), + &mut cx, + ); } self.can_undo.store(self.can_undo(), Ordering::Relaxed); self.can_redo.store(self.can_redo(), Ordering::Relaxed); } } -} -impl Inner { pub fn new( workspace: WeakEntity, panel: WeakEntity, @@ -401,7 +420,7 @@ impl Inner { // 0 Created(src/main.rs) // 1 Renamed(README.md, readme.md) ─┐ // 2 +++cursor+++ │(before the cursor) - // 2 Trashed(TrashId(1)) │ + // 2 Trashed(WorktreeId(1), TrashId(1)) │ // │ // User Operation Undo v // Failed execute Renamed(README.md, readme.md) ───> Rename(readme.md, README.md) @@ -409,10 +428,10 @@ impl Inner { // History // 0 Created(src/main.rs) // 1 +++cursor+++ - // 1 Trashed(TrashId(1)) --------- - // |(at the cursor) - // User Operation Redo v - // Execute Trashed(TrashId(1)) ────────> Restore(TrashId(1)) + // 1 Trashed(WorktreeId(1), TrashId(1)) --------- + // |(at the cursor) + // User Operation Redo v + // Execute Trashed(WorktreeId(1), TrashId(1)) ─> Restore(WorktreeId(1), TrashId(1)) // Record Restored(ProjectPath) // History // 0 Created(src/main.rs) @@ -497,18 +516,57 @@ impl Inner { return Err(anyhow!("Failed to obtain workspace.")); }; + let (from_name, to_name) = workspace.update(cx, |workspace, cx| { + let project = workspace.project().read(cx); + let path_style = project.path_style(cx); + + ( + project_path_display(project, from, path_style, cx), + project_path_display(project, to, path_style, cx), + ) + }); + + // Since the Project Panel's rename operation is used for both renaming + // and moving files and directories, we'll assume that, if both paths + // share the parent folder, then it was a simple rename, otherwise it + // was a move. + let operation = if from.path.parent() == to.path.parent() { + "rename" + } else { + "move" + }; + let res: Result>> = workspace.update(cx, |workspace, cx| { workspace.project().update(cx, |project, cx| { let entry_id = project .entry_for_path(from, cx) .map(|entry| entry.id) - .ok_or_else(|| anyhow!("No entry for path."))?; + .with_context(|| { + format!("Failed to {operation} `{from_name}`. It no longer exists.") + })?; Ok(project.rename_entry(entry_id, to.clone(), cx)) }) }); - res?.await + res?.await.map_err(|err| { + // It is possible for `RealFs::rename` to return an error other than + // `io::Error` when the file already exists, hence why we're also + // checking if the error contains the "already exists" string. + let already_exists = err.chain().any(|cause| { + cause + .downcast_ref::() + .is_some_and(|io_err| io_err.kind() == std::io::ErrorKind::AlreadyExists) + }) || format!("{err:#}").contains("already exists"); + + if already_exists { + anyhow!( + "Failed to {operation} `{from_name}` to `{to_name}`. A file or folder already exists there." + ) + } else { + err + } + }) } async fn trash(&self, project_path: &ProjectPath, cx: &mut AsyncApp) -> Result { @@ -516,20 +574,30 @@ impl Inner { return Err(anyhow!("Failed to obtain workspace.")); }; - workspace - .update(cx, |workspace, cx| { - workspace.project().update(cx, |project, cx| { - let entry_id = project - .entry_for_path(&project_path, cx) - .map(|entry| entry.id) - .ok_or_else(|| anyhow!("No entry for path."))?; + let name = workspace.update(cx, |workspace, cx| { + let project = workspace.project().read(cx); + let path_style = project.path_style(cx); - project - .trash_entry(entry_id, cx) - .ok_or_else(|| anyhow!("Worktree entry should exist")) - }) - })? - .await + project_path_display(project, project_path, path_style, cx) + }); + + let task = workspace.update(cx, |workspace, cx| { + workspace.project().update(cx, |project, cx| { + let entry_id = project + .entry_for_path(project_path, cx) + .map(|entry| entry.id) + .with_context(|| format!("Failed to trash `{name}`. It no longer exists."))?; + + project + .trash_entry(entry_id, cx) + .with_context(|| format!("Failed to trash `{name}`.")) + }) + })?; + + match task.await { + Ok(trash_id) => Ok(trash_id), + Err(err) => Err(err).context(format!("Failed to trash `{name}`.")), + } } async fn restore( @@ -542,6 +610,35 @@ impl Inner { return Err(anyhow!("Failed to obtain workspace.")); }; + let name = workspace + .update(cx, |workspace, cx| { + let project = workspace.project().read(cx); + let path_style = project.path_style(cx); + let original_path = project.fs().original_path_for_trash_id(trash_id)?; + let original_name = original_path + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| original_path.display().to_string()); + + let project_path = (|| { + let worktree = project.worktree_for_id(worktree_id, cx)?; + let worktree_abs_path = worktree.read(cx).abs_path(); + let relative_path = original_path + .strip_prefix(worktree_abs_path.as_ref()) + .ok()?; + let relative_path = RelPath::new(relative_path, path_style).ok()?; + Some(ProjectPath { + worktree_id, + path: relative_path.into_arc(), + }) + })(); + + Some(project_path.map_or(original_name, |project_path| { + project_path_display(project, &project_path, path_style, cx) + })) + }) + .unwrap_or_else(|| "item".to_string()); + workspace .update(cx, |workspace, cx| { workspace.project().update(cx, |project, cx| { @@ -549,22 +646,39 @@ impl Inner { }) }) .await + .map_err(|err| match err.downcast_ref::() { + Some(TrashRestoreError::Collision { .. }) => anyhow!( + "Failed to restore `{name}`. Something already exists at its original location." + ), + _ => anyhow!("Failed to restore `{name}`. It may have been permanently deleted."), + }) } - /// Displays a notification with the provided `title` and `error`. + /// Displays a notification with the provided `title` and `error`. The + /// `error` is rendered as markdown, so file names wrapped in backticks show + /// up as inline code. fn show_error( title: impl Into, workspace: WeakEntity, error: String, cx: &mut AsyncApp, ) { + let title = title.into(); workspace .update(cx, move |workspace, cx| { let notification_id = NotificationId::Named(SharedString::new_static("project_panel_undo")); workspace.show_notification(notification_id, cx, move |cx| { - cx.new(|cx| MessageNotification::new(error, cx).with_title(title)) + cx.new(move |cx| { + let markdown = cx.new(|cx| Markdown::new(error.into(), None, None, cx)); + MessageNotification::new_from_builder(cx, move |window, cx| { + MarkdownElement::new(markdown.clone(), markdown_style(window, cx)) + .text_size(TextSize::Default.rems(cx)) + .into_any_element() + }) + .with_title(title) + }) }) }) .ok(); diff --git a/crates/workspace/src/notifications.rs b/crates/workspace/src/notifications.rs index 59246eaba2..860ac2ad9f 100644 --- a/crates/workspace/src/notifications.rs +++ b/crates/workspace/src/notifications.rs @@ -428,7 +428,7 @@ fn workspace_error_notification_id() -> NotificationId { NotificationId::unique::() } -fn markdown_style(window: &Window, cx: &App) -> MarkdownStyle { +pub fn markdown_style(window: &Window, cx: &App) -> MarkdownStyle { let settings = ThemeSettings::get_global(cx); let ui_font_family = settings.ui_font.family.clone(); let ui_font_fallbacks = settings.ui_font.fallbacks.clone();