yenru0/zed
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

project_panel: Refactor undo and redo error messages (#60186)

Browse Source 
# 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

<details>
<summary>1. Move conflict</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Move `a.txt` into `docs/` (cut/paste or drag)<br>2. Run `touch
a.txt`<br>3. Undo | <img width="2736" height="1586" alt="1_before"
src="https://github.com/user-attachments/assets/d60660d1-ba85-4a08-82a3-a6e3f6d84506"
/> | <img width="2736" height="1586" alt="1_after"
src="https://github.com/user-attachments/assets/c050f287-b186-42db-8995-57102d1db4d2"
/> |

</details>

<details>
<summary>2. Rename conflict</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Rename `a.txt` → `b.txt`<br>2. Run `touch a.txt`<br>3. Undo | <img
width="2736" height="1586" alt="2_before"
src="https://github.com/user-attachments/assets/0c0eb0bc-ea01-4642-a63a-69687b2ba13e"
/> | <img width="2736" height="1586" alt="2_after"
src="https://github.com/user-attachments/assets/68dba045-60b2-462e-9f34-5bb038a4a245"
/> |

</details>

<details>
<summary>3. Source no longer exists</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Move `a.txt` into `docs/`<br>2. Run `rm docs/a.txt`<br>3. Undo |
<img width="2736" height="1586" alt="3_before"
src="https://github.com/user-attachments/assets/2f55327c-d478-44f5-ac85-5e0b10044a52"
/> | <img width="2736" height="1586" alt="3_after"
src="https://github.com/user-attachments/assets/5296b431-c2a1-408e-8920-9f2d5c1d169a"
/> |

</details>

<details>
<summary>4. Restore from emptied Trash</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Delete `a.txt` (moves it to Trash)<br>2. Empty it from the system
Trash<br>3. Undo | <img width="2736" height="1586" alt="4_before"
src="https://github.com/user-attachments/assets/41966e44-409a-4ae2-ada1-59992658f1b5"
/> | <img width="2736" height="1586" alt="4_after"
src="https://github.com/user-attachments/assets/f6df01f9-0702-4f28-80aa-eb2289077ea5"
/> |

</details>

<details>
<summary>5. Restore collision</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Delete `a.txt` (to Trash)<br>2. Run `touch a.txt`<br>3. Undo | <img
width="2736" height="1586" alt="5_before"
src="https://github.com/user-attachments/assets/f4151e85-7668-4dab-8cf4-b45ef0fdb741"
/> | <img width="2736" height="1586" alt="5_after"
src="https://github.com/user-attachments/assets/3f25aa56-1b59-4230-9a15-b39a0d280a48"
/> |

</details>

<details>
<summary>6. Trash a file that's gone</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Create `empty.txt` in the panel<br>2. Run `rm empty.txt`<br>3. Undo
| <img width="2736" height="1586" alt="6_before"
src="https://github.com/user-attachments/assets/077edeb8-3618-4b72-b49f-9d1cc3a801d5"
/> | <img width="2736" height="1586" alt="6_after"
src="https://github.com/user-attachments/assets/4b1d6689-93b1-4725-8829-2f71b4b586e9"
/> |

</details>

### Redo failures

<details>
<summary>7. Redo a move into an occupied destination</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Move `a.txt` into `docs/`, then Undo (file back at root)<br>2. Run
`touch docs/a.txt`<br>3. Redo | <img width="2736" height="1586"
alt="7_before"
src="https://github.com/user-attachments/assets/d6591d08-b0a1-4992-866f-fcc26491ba8c"
/> | <img width="2736" height="1586" alt="7_after"
src="https://github.com/user-attachments/assets/b46d5fe2-09be-4264-9834-f5965f5f1150"
/> |

</details>

<details>
<summary>8. Redo a restore after the Trash was emptied</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Create `empty.txt`, then Undo (it gets trashed)<br>2. Empty it from
the system Trash<br>3. Redo | <img width="2736" height="1586"
alt="8_before"
src="https://github.com/user-attachments/assets/9ced2696-1bdf-4b33-aaaf-abc015787505"
/> | <img width="2736" height="1586" alt="8_after"
src="https://github.com/user-attachments/assets/bfc84fc3-c97f-474d-857c-3d5137d06b83"
/> |

</details>

<details>
<summary>9. Redo a re-trash of a deleted file</summary>

| Steps | Before | After |
| --- | --- | --- |
| 1. Delete `a.txt` (to Trash), then Undo (restores it)<br>2. Run `rm
a.txt`<br>3. Redo | <img width="2736" height="1586" alt="9_before"
src="https://github.com/user-attachments/assets/3152787d-c02c-482e-a70a-600e97a44b3b"
/> | <img width="2736" height="1586" alt="9_after"
src="https://github.com/user-attachments/assets/7dd40497-0dfc-41fa-a6ac-772562d29258"
/> |

</details>

---

Release Notes:

- N/A
This commit is contained in:
Dino authored and GitHub committed 2026-07-28 12:22:11 +00:00
1 parent 6746a42cc1
commit cf688465c7
6 files changed
+196 -55
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

Cargo.lock
Generated
+1
View File
@@ -14226,6 +14226,7 @@ dependencies = [
"itertools 0.14.0",
"language",
"log",
"markdown",
"markdown_preview",
"menu",
"notifications",
crates/fs/src/fs.rs
+24 -5
View File
@@ -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<PathBuf, TrashRestoreError>;
/// Returns the original absolute path of the item identified by `trash_id`.
fn original_path_for_trash_id(&self, trash_id: TrashId) -> Option<PathBuf>;
/// 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<PathBuf, TrashRestoreError>;
#[cfg(feature = "test-support")]
fn as_fake(&self) -> Arc<FakeFs> {
@@ -1348,11 +1351,18 @@ impl Fs for RealFs {
res
}
async fn restore(&self, item: TrashId) -> std::result::Result<PathBuf, TrashRestoreError> {
fn original_path_for_trash_id(&self, trash_id: TrashId) -> Option<PathBuf> {
self.trash
.lock()
.get(trash_id)
.map(|entry| entry.original_parent.join(&entry.name))
}
async fn restore(&self, trash_id: TrashId) -> std::result::Result<PathBuf, TrashRestoreError> {
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<PathBuf> {
self.state
.lock()
.trash
.lock()
.get(trash_id)
.map(|(entry, _)| entry.original_parent.join(&entry.name))
}
async fn restore(&self, trash_id: TrashId) -> Result<PathBuf, TrashRestoreError> {
let mut state = self.state.lock();
crates/fs/tests/integration/fs_tests.rs
+6
View File
@@ -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(),
crates/project_panel/Cargo.toml
+1
View File
@@ -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
crates/project_panel/src/undo.rs
+163 -49
View File
@@ -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<Operation>) → Batch(Vec<Result>)
//!
//!
@@ -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<Workspace>,
panel: WeakEntity<ProjectPanel>,
@@ -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<Task<Result<CreatedEntry>>> = 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::<std::io::Error>()
.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<TrashId> {
@@ -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::<TrashRestoreError>() {
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<SharedString>,
workspace: WeakEntity<Workspace>,
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();
crates/workspace/src/notifications.rs
+1 -1
View File
@@ -428,7 +428,7 @@ fn workspace_error_notification_id() -> NotificationId {
NotificationId::unique::<WorkspaceErrorNotification>()
}
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();