settings_ui: Fix editable project settings not being updated when a new workspace is created (#47992)

Anthony Eid created

The bug occurred because `cx.observe_new::<Workspace>` would pass a
mutable `Window` that was also the new workspace's window. Later,
`fetch_files` would fail to read the newly created workspace from
`WorkspaceStore` because its window already had a mutable reference to
it.

The fix always passes the Settings UI's window handle to the
`fetch_files` function, and uses a `settings_window_handle.defer`
callback to call `fetch_files` after the newly created workspace's
window no longer has any mutable references to it.

Release Notes:

- settings_ui: Fixed editable project settings not being updated when a
new workspace is created

Change summary

crates/settings_ui/src/settings_ui.rs | 375 +++++++++++++++++++++++++++-
1 file changed, 362 insertions(+), 13 deletions(-)

Detailed changes

crates/settings_ui/src/settings_ui.rs 🔗

@@ -1544,19 +1544,32 @@ impl SettingsWindow {
         })
         .detach();
 
-        cx.observe_new::<Workspace>(move |_, window, cx| {
-            let workspace = cx.entity();
-            let Some(window) = window else {
-                return;
-            };
+        let handle = window.window_handle();
+        cx.observe_new::<Workspace>(move |workspace, _, cx| {
+            let project = workspace.project().clone();
+            let this_weak = this_weak.clone();
 
-            this_weak
-                .update(cx, |this, cx| {
-                    this.fetch_files(window, cx);
-                    cx.observe_release_in(&workspace, window, |this, _, window, cx| {
-                        this.fetch_files(window, cx)
-                    })
-                    .detach();
+            // We defer on the settings window (via `handle`) rather than using
+            // the workspace's window from observe_new. When window.defer() runs
+            // its callback, it calls handle.update() which temporarily removes
+            // that window from cx.windows. If we deferred on the workspace's
+            // window, then when fetch_files() tries to read ALL workspaces from
+            // the store (including the newly created one), it would fail with
+            // "window not found" because that workspace's window would be
+            // temporarily removed from cx.windows for the duration of our callback.
+            handle
+                .update(cx, move |_, window, cx| {
+                    window.defer(cx, move |window, cx| {
+                        this_weak
+                            .update(cx, |this, cx| {
+                                this.fetch_files(window, cx);
+                                cx.observe_release_in(&project, window, |this, _, window, cx| {
+                                    this.fetch_files(window, cx)
+                                })
+                                .detach();
+                            })
+                            .ok();
+                    });
                 })
                 .ok();
         })
@@ -3587,7 +3600,8 @@ impl Render for SettingsWindow {
 fn all_projects(
     window: Option<&WindowHandle<Workspace>>,
     cx: &App,
-) -> impl Iterator<Item = Entity<project::Project>> {
+) -> impl Iterator<Item = Entity<Project>> {
+    let mut seen_project_ids = std::collections::HashSet::new();
     workspace::AppState::global(cx)
         .upgrade()
         .map(|app_state| {
@@ -3600,6 +3614,7 @@ fn all_projects(
                 .chain(
                     window.and_then(|workspace| Some(workspace.read(cx).ok()?.project().clone())),
                 )
+                .filter(move |project| seen_project_ids.insert(project.entity_id()))
         })
         .into_iter()
         .flatten()
@@ -4492,6 +4507,340 @@ pub mod test {
         > Appearance & Behavior
         "
     );
+
+    #[gpui::test]
+    async fn test_settings_window_shows_worktrees_from_multiple_workspaces(
+        cx: &mut gpui::TestAppContext,
+    ) {
+        use project::Project;
+        use serde_json::json;
+
+        cx.update(|cx| {
+            register_settings(cx);
+        });
+
+        let app_state = cx.update(|cx| {
+            let app_state = AppState::test(cx);
+            AppState::set_global(Arc::downgrade(&app_state), cx);
+            app_state
+        });
+
+        let fake_fs = app_state.fs.as_fake();
+
+        fake_fs
+            .insert_tree(
+                "/workspace1",
+                json!({
+                    "worktree_a": {
+                        "file1.rs": "fn main() {}"
+                    },
+                    "worktree_b": {
+                        "file2.rs": "fn test() {}"
+                    }
+                }),
+            )
+            .await;
+
+        fake_fs
+            .insert_tree(
+                "/workspace2",
+                json!({
+                    "worktree_c": {
+                        "file3.rs": "fn foo() {}"
+                    }
+                }),
+            )
+            .await;
+
+        let project1 = cx.update(|cx| {
+            Project::local(
+                app_state.client.clone(),
+                app_state.node_runtime.clone(),
+                app_state.user_store.clone(),
+                app_state.languages.clone(),
+                app_state.fs.clone(),
+                None,
+                project::LocalProjectFlags::default(),
+                cx,
+            )
+        });
+
+        project1
+            .update(cx, |project, cx| {
+                project.find_or_create_worktree("/workspace1/worktree_a", true, cx)
+            })
+            .await
+            .expect("Failed to create worktree_a");
+        project1
+            .update(cx, |project, cx| {
+                project.find_or_create_worktree("/workspace1/worktree_b", true, cx)
+            })
+            .await
+            .expect("Failed to create worktree_b");
+
+        let project2 = cx.update(|cx| {
+            Project::local(
+                app_state.client.clone(),
+                app_state.node_runtime.clone(),
+                app_state.user_store.clone(),
+                app_state.languages.clone(),
+                app_state.fs.clone(),
+                None,
+                project::LocalProjectFlags::default(),
+                cx,
+            )
+        });
+
+        project2
+            .update(cx, |project, cx| {
+                project.find_or_create_worktree("/workspace2/worktree_c", true, cx)
+            })
+            .await
+            .expect("Failed to create worktree_c");
+
+        let (_workspace1, cx) = cx.add_window_view(|window, cx| {
+            Workspace::new(
+                Default::default(),
+                project1.clone(),
+                app_state.clone(),
+                window,
+                cx,
+            )
+        });
+
+        let _workspace1_handle = cx.window_handle().downcast::<Workspace>().unwrap();
+
+        let (_workspace2, cx) = cx.add_window_view(|window, cx| {
+            Workspace::new(
+                Default::default(),
+                project2.clone(),
+                app_state.clone(),
+                window,
+                cx,
+            )
+        });
+
+        let workspace2_handle = cx.window_handle().downcast::<Workspace>().unwrap();
+
+        cx.run_until_parked();
+
+        let (settings_window, cx) = cx
+            .add_window_view(|window, cx| SettingsWindow::new(Some(workspace2_handle), window, cx));
+
+        cx.run_until_parked();
+
+        settings_window.read_with(cx, |settings_window, _| {
+            let worktree_names: Vec<_> = settings_window
+                .worktree_root_dirs
+                .values()
+                .cloned()
+                .collect();
+
+            assert!(
+                worktree_names.iter().any(|name| name == "worktree_a"),
+                "Should contain worktree_a from workspace1, but found: {:?}",
+                worktree_names
+            );
+            assert!(
+                worktree_names.iter().any(|name| name == "worktree_b"),
+                "Should contain worktree_b from workspace1, but found: {:?}",
+                worktree_names
+            );
+            assert!(
+                worktree_names.iter().any(|name| name == "worktree_c"),
+                "Should contain worktree_c from workspace2, but found: {:?}",
+                worktree_names
+            );
+
+            assert_eq!(
+                worktree_names.len(),
+                3,
+                "Should have exactly 3 worktrees from both workspaces, but found: {:?}",
+                worktree_names
+            );
+
+            let project_files: Vec<_> = settings_window
+                .files
+                .iter()
+                .filter_map(|(f, _)| match f {
+                    SettingsUiFile::Project((worktree_id, _)) => Some(*worktree_id),
+                    _ => None,
+                })
+                .collect();
+
+            let unique_project_files: std::collections::HashSet<_> = project_files.iter().collect();
+            assert_eq!(
+                project_files.len(),
+                unique_project_files.len(),
+                "Should have no duplicate project files, but found duplicates. All files: {:?}",
+                project_files
+            );
+        });
+    }
+
+    #[gpui::test]
+    async fn test_settings_window_updates_when_new_workspace_created(
+        cx: &mut gpui::TestAppContext,
+    ) {
+        use project::Project;
+        use serde_json::json;
+
+        cx.update(|cx| {
+            register_settings(cx);
+        });
+
+        let app_state = cx.update(|cx| {
+            let app_state = AppState::test(cx);
+            AppState::set_global(Arc::downgrade(&app_state), cx);
+            app_state
+        });
+
+        let fake_fs = app_state.fs.as_fake();
+
+        fake_fs
+            .insert_tree(
+                "/workspace1",
+                json!({
+                    "worktree_a": {
+                        "file1.rs": "fn main() {}"
+                    }
+                }),
+            )
+            .await;
+
+        fake_fs
+            .insert_tree(
+                "/workspace2",
+                json!({
+                    "worktree_b": {
+                        "file2.rs": "fn test() {}"
+                    }
+                }),
+            )
+            .await;
+
+        let project1 = cx.update(|cx| {
+            Project::local(
+                app_state.client.clone(),
+                app_state.node_runtime.clone(),
+                app_state.user_store.clone(),
+                app_state.languages.clone(),
+                app_state.fs.clone(),
+                None,
+                project::LocalProjectFlags::default(),
+                cx,
+            )
+        });
+
+        project1
+            .update(cx, |project, cx| {
+                project.find_or_create_worktree("/workspace1/worktree_a", true, cx)
+            })
+            .await
+            .expect("Failed to create worktree_a");
+
+        let (_workspace1, cx) = cx.add_window_view(|window, cx| {
+            Workspace::new(
+                Default::default(),
+                project1.clone(),
+                app_state.clone(),
+                window,
+                cx,
+            )
+        });
+
+        let workspace1_handle = cx.window_handle().downcast::<Workspace>().unwrap();
+
+        cx.run_until_parked();
+
+        let (settings_window, cx) = cx
+            .add_window_view(|window, cx| SettingsWindow::new(Some(workspace1_handle), window, cx));
+
+        cx.run_until_parked();
+
+        settings_window.read_with(cx, |settings_window, _| {
+            assert_eq!(
+                settings_window.worktree_root_dirs.len(),
+                1,
+                "Should have 1 worktree initially"
+            );
+        });
+
+        let project2 = cx.update(|_, cx| {
+            Project::local(
+                app_state.client.clone(),
+                app_state.node_runtime.clone(),
+                app_state.user_store.clone(),
+                app_state.languages.clone(),
+                app_state.fs.clone(),
+                None,
+                project::LocalProjectFlags::default(),
+                cx,
+            )
+        });
+
+        project2
+            .update(&mut cx.cx, |project, cx| {
+                project.find_or_create_worktree("/workspace2/worktree_b", true, cx)
+            })
+            .await
+            .expect("Failed to create worktree_b");
+
+        let (_workspace2, cx) = cx.add_window_view(|window, cx| {
+            Workspace::new(
+                Default::default(),
+                project2.clone(),
+                app_state.clone(),
+                window,
+                cx,
+            )
+        });
+
+        cx.run_until_parked();
+
+        settings_window.read_with(cx, |settings_window, _| {
+            let worktree_names: Vec<_> = settings_window
+                .worktree_root_dirs
+                .values()
+                .cloned()
+                .collect();
+
+            assert!(
+                worktree_names.iter().any(|name| name == "worktree_a"),
+                "Should contain worktree_a, but found: {:?}",
+                worktree_names
+            );
+            assert!(
+                worktree_names.iter().any(|name| name == "worktree_b"),
+                "Should contain worktree_b from newly created workspace, but found: {:?}",
+                worktree_names
+            );
+
+            assert_eq!(
+                worktree_names.len(),
+                2,
+                "Should have 2 worktrees after new workspace created, but found: {:?}",
+                worktree_names
+            );
+
+            let project_files: Vec<_> = settings_window
+                .files
+                .iter()
+                .filter_map(|(f, _)| match f {
+                    SettingsUiFile::Project((worktree_id, _)) => Some(*worktree_id),
+                    _ => None,
+                })
+                .collect();
+
+            let unique_project_files: std::collections::HashSet<_> = project_files.iter().collect();
+            assert_eq!(
+                project_files.len(),
+                unique_project_files.len(),
+                "Should have no duplicate project files, but found duplicates. All files: {:?}",
+                project_files
+            );
+        });
+    }
 }
 
 #[cfg(test)]