Address review feedback on worktree drag-and-drop

- `move_worktrees` now validates every source up front and errors on
  missing IDs, matching the original `move_worktree` contract instead
  of silently dropping unknown sources.
- A self-drop of a multi-root selection (dropping the group onto one
  of its own members) is now a no-op rather than reordering the
  remaining roots around it.
- The reorder path now calls `send_project_updates` so collaborators
  see the new worktree order.
- `disjoint_entries` skips the worktree root when building `dir_paths`,
  so a marked root no longer silently filters out its own selected
  descendants before the move/copy branches see them.
- The drag-over highlight only restricts itself to worktree-root
  targets when the entire drag is roots-only; mixed drags continue to
  highlight directory targets so the file portion still gets feedback.
- Adds regression tests: invalid-source error, multi-root self-drop
  no-op, and a marked-root + nested-file drag that exercises both the
  reorder and per-entry move paths.
This commit is contained in:
eth0net 2026-05-05 15:57:47 +01:00
parent 214d929281
commit a8d9c97dc4
No known key found for this signature in database
4 changed files with 203 additions and 10 deletions

View file

@ -1057,6 +1057,12 @@ impl WorktreeStore {
if sources.is_empty() {
return Ok(());
}
// Self-drop of any selection member is a no-op: the user dropping a
// multi-selection onto one of its own roots has no well-defined
// intent.
if sources.contains(&destination) {
return Ok(());
}
let destination_index = self
.worktrees
@ -1067,13 +1073,23 @@ impl WorktreeStore {
})
.with_context(|| format!("Missing worktree for id {destination}"))?;
for &source in sources {
if !self
.worktrees
.iter()
.any(|wt| wt.upgrade().is_some_and(|wt| wt.read(cx).id() == source))
{
anyhow::bail!("Missing worktree for id {source}");
}
}
let source_indices: Vec<usize> = self
.worktrees
.iter()
.enumerate()
.filter_map(|(i, wt)| {
let id = wt.upgrade()?.read(cx).id();
(sources.contains(&id) && id != destination).then_some(i)
sources.contains(&id).then_some(i)
})
.collect();
@ -1114,6 +1130,7 @@ impl WorktreeStore {
cx.emit(WorktreeStoreEvent::WorktreeOrderChanged);
cx.notify();
self.send_project_updates(cx);
Ok(())
}

View file

@ -8946,6 +8946,40 @@ async fn test_reordering_worktrees(cx: &mut gpui::TestAppContext) {
});
}
#[gpui::test]
async fn test_move_worktree_with_invalid_source_errors(cx: &mut gpui::TestAppContext) {
use worktree::WorktreeId;
init_test(cx);
let fs = FakeFs::new(cx.executor());
fs.insert_tree(
"/dir",
json!({
"a.rs": "",
"b.rs": "",
}),
)
.await;
let project =
Project::test(fs, ["/dir/a.rs".as_ref(), "/dir/b.rs".as_ref()], cx).await;
project.update(cx, |project, cx| {
let valid_id = project.visible_worktrees(cx).next().unwrap().read(cx).id();
let invalid_id = WorktreeId::from_usize(99_999);
assert!(
project.move_worktree(invalid_id, valid_id, cx).is_err(),
"moving an unknown source worktree should error"
);
assert!(
project.move_worktree(valid_id, invalid_id, cx).is_err(),
"moving onto an unknown destination should error"
);
});
}
#[gpui::test]
async fn test_unstaged_diff_for_buffer(cx: &mut gpui::TestAppContext) {
init_test(cx);

View file

@ -3718,11 +3718,14 @@ impl ProjectPanel {
for (worktree_id, worktree_entries) in entries_by_worktree {
if let Some(worktree) = project.worktree_for_id(worktree_id, cx) {
let worktree = worktree.read(cx);
// Skip the worktree root: its empty path would consume every
// other selected entry in the same worktree as "nested inside
// a selected directory" and silently drop them.
let dir_paths = worktree_entries
.iter()
.filter_map(|entry| {
worktree.entry_for_id(entry.entry_id).and_then(|entry| {
if entry.is_dir() {
if entry.is_dir() && !entry.path.is_empty() {
Some(entry.path.as_ref())
} else {
None
@ -5223,14 +5226,15 @@ impl ProjectPanel {
drag_state: &DraggedSelection,
cx: &Context<Self>,
) -> Option<ProjectEntryId> {
// Worktree-root drags are only meaningful when dropped on another
// worktree's root, so suppress highlights elsewhere — including over
// the source worktree itself, where the drop would be a no-op.
if self
.project
.read(cx)
.entry_is_worktree_root(drag_state.active_selection.entry_id, cx)
{
// Pure worktree-root drags are only meaningful when dropped on
// another worktree's root; suppress highlights elsewhere. Mixed drags
// (e.g. a root with a marked file) fall through so the file portion
// can still receive feedback on directory targets.
let project = self.project.read(cx);
let drag_is_root_only = drag_state
.items()
.all(|entry| project.entry_is_worktree_root(entry.entry_id, cx));
if drag_is_root_only {
let root_id = target_worktree.root_entry()?.id;
if target_entry.id == root_id
&& target_worktree.id() != drag_state.active_selection.worktree_id

View file

@ -4416,6 +4416,144 @@ async fn test_drag_worktree_root_onto_nested_entry_is_rejected(
);
}
// Dropping a multi-root selection onto one of its own members has no
// well-defined intent, so it should leave the order untouched rather than
// reorder the remaining roots around the destination.
#[gpui::test]
async fn test_drag_multiple_marked_worktree_roots_onto_self_is_no_op(
cx: &mut gpui::TestAppContext,
) {
init_test(cx);
let fs = FakeFs::new(cx.executor());
fs.insert_tree("/root1", json!({ "a.txt": "" })).await;
fs.insert_tree("/root2", json!({ "b.txt": "" })).await;
fs.insert_tree("/root3", json!({ "c.txt": "" })).await;
let project = Project::test(
fs.clone(),
["/root1".as_ref(), "/root2".as_ref(), "/root3".as_ref()],
cx,
)
.await;
let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
let workspace = window
.read_with(cx, |mw, _| mw.workspace().clone())
.unwrap();
let cx = &mut VisualTestContext::from_window(window.into(), cx);
let panel = workspace.update_in(cx, ProjectPanel::new);
cx.run_until_parked();
panel.update_in(cx, |panel, window, cx| {
let project = panel.project.read(cx);
let worktrees = project.visible_worktrees(cx).collect::<Vec<_>>();
let r1 = worktrees[0].read(cx);
let r2 = worktrees[1].read(cx);
let r1_entry = SelectedEntry {
worktree_id: r1.id(),
entry_id: r1.root_entry().unwrap().id,
};
let r2_entry = SelectedEntry {
worktree_id: r2.id(),
entry_id: r2.root_entry().unwrap().id,
};
let drag = DraggedSelection {
active_selection: r1_entry,
marked_selections: Arc::new([r1_entry, r2_entry]),
};
// Drop the [r1, r2] selection onto r2, one of its own members.
panel.drag_onto(&drag, r2_entry.entry_id, false, window, cx);
});
cx.run_until_parked();
assert_eq!(
visible_entries_as_strings(&panel, 0..20, cx),
&[
"v root1",
" a.txt",
"v root2",
" b.txt",
"v root3",
" c.txt",
],
"self-drop of a multi-root selection should leave the order untouched"
);
}
// When a worktree root and one of its descendants are both marked, the
// descendant must survive `disjoint_entries`. The root's empty path would
// otherwise classify every other entry in the same worktree as nested
// inside a "selected directory" and silently drop it before the move/copy
// branches see it.
#[gpui::test]
async fn test_drag_marked_root_with_nested_file_keeps_both(
cx: &mut gpui::TestAppContext,
) {
init_test(cx);
let fs = FakeFs::new(cx.executor());
fs.insert_tree("/root1", json!({ "sub": { "a.txt": "" } }))
.await;
fs.insert_tree("/root2", json!({ "b.txt": "" })).await;
fs.insert_tree("/root3", json!({ "c.txt": "" })).await;
let project = Project::test(
fs.clone(),
["/root1".as_ref(), "/root2".as_ref(), "/root3".as_ref()],
cx,
)
.await;
let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
let workspace = window
.read_with(cx, |mw, _| mw.workspace().clone())
.unwrap();
let cx = &mut VisualTestContext::from_window(window.into(), cx);
let panel = workspace.update_in(cx, ProjectPanel::new);
cx.run_until_parked();
let (r1_id, r2_id, r3_id) = panel.update_in(cx, |panel, _, cx| {
let worktrees = panel
.project
.read(cx)
.visible_worktrees(cx)
.collect::<Vec<_>>();
(
worktrees[0].read(cx).id(),
worktrees[1].read(cx).id(),
worktrees[2].read(cx).id(),
)
});
select_path_with_mark(&panel, "root1", cx);
select_path_with_mark(&panel, "root1/sub/a.txt", cx);
drag_selection_to(&panel, "root3", false, cx);
// root1 reorders past root3 (active is the file in root1, so direction
// follows root1's position via the source-side fallback in the active
// filter).
let order = panel.update_in(cx, |panel, _, cx| {
panel
.project
.read(cx)
.visible_worktrees(cx)
.map(|wt| wt.read(cx).id())
.collect::<Vec<_>>()
});
assert_eq!(order, vec![r2_id, r3_id, r1_id]);
// The marked file moves to root3 instead of being silently dropped.
assert!(
find_project_entry(&panel, "root3/a.txt", cx).is_some(),
"a.txt should land in root3"
);
assert_eq!(
find_project_entry(&panel, "root1/sub/a.txt", cx),
None,
"a.txt should be removed from root1/sub"
);
}
// Mixed selection: marked worktree root + non-root entry, with the non-root
// as the active drag. The root reorders, the file moves, both in one gesture.
//