Remove easily derived sidebar state (#52790)

Mikayla Maki created

Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable

Closes #ISSUE

Release Notes:

- N/A

Change summary

crates/sidebar/src/sidebar.rs       | 58 ++++++++++++--------------
crates/sidebar/src/sidebar_tests.rs | 65 +++++++++++++-----------------
2 files changed, 56 insertions(+), 67 deletions(-)

Detailed changes

crates/sidebar/src/sidebar.rs 🔗

@@ -307,8 +307,6 @@ pub struct Sidebar {
     /// derivation, since the panel may transiently return `None` while
     /// loading. User actions may write directly for immediate feedback.
     focused_thread: Option<acp::SessionId>,
-    agent_panel_visible: bool,
-    active_thread_is_draft: bool,
     hovered_thread_index: Option<usize>,
     collapsed_groups: HashSet<PathList>,
     expanded_groups: HashMap<PathList, usize>,
@@ -406,8 +404,6 @@ impl Sidebar {
             contents: SidebarContents::default(),
             selection: None,
             focused_thread: None,
-            agent_panel_visible: false,
-            active_thread_is_draft: false,
             hovered_thread_index: None,
             collapsed_groups: HashSet::new(),
             expanded_groups: HashMap::new(),
@@ -429,6 +425,23 @@ impl Sidebar {
             .map_or(false, |mw| mw.read(cx).workspace() == workspace)
     }
 
+    fn agent_panel_visible(&self, cx: &App) -> bool {
+        self.multi_workspace.upgrade().map_or(false, |mw| {
+            let workspace = mw.read(cx).workspace();
+            AgentPanel::is_visible(&workspace, cx)
+        })
+    }
+
+    fn active_thread_is_draft(&self, cx: &App) -> bool {
+        self.multi_workspace
+            .upgrade()
+            .and_then(|mw| {
+                let workspace = mw.read(cx).workspace();
+                workspace.read(cx).panel::<AgentPanel>(cx)
+            })
+            .map_or(false, |panel| panel.read(cx).active_thread_is_draft(cx))
+    }
+
     fn subscribe_to_workspace(
         &mut self,
         workspace: &Entity<Workspace>,
@@ -486,9 +499,6 @@ impl Sidebar {
 
         if let Some(agent_panel) = workspace.read(cx).panel::<AgentPanel>(cx) {
             self.subscribe_to_agent_panel(&agent_panel, window, cx);
-            if self.is_active_workspace(workspace, cx) {
-                self.agent_panel_visible = AgentPanel::is_visible(workspace, cx);
-            }
             self.observe_draft_editor(cx);
         }
     }
@@ -544,12 +554,7 @@ impl Sidebar {
                     return;
                 }
 
-                let is_visible = AgentPanel::is_visible(&workspace, cx);
-
-                if this.agent_panel_visible != is_visible {
-                    this.agent_panel_visible = is_visible;
-                    cx.notify();
-                }
+                cx.notify();
             })
             .detach();
         }
@@ -648,18 +653,8 @@ impl Sidebar {
 
         let query = self.filter_editor.read(cx).text(cx);
 
-        // Re-derive agent_panel_visible from the active workspace so it stays
-        // correct after workspace switches.
-        self.agent_panel_visible = active_workspace
-            .as_ref()
-            .map_or(false, |ws| AgentPanel::is_visible(ws, cx));
-
-        // Derive active_thread_is_draft BEFORE focused_thread so we can
-        // use it as a guard below.
-        self.active_thread_is_draft = active_workspace
-            .as_ref()
-            .and_then(|ws| ws.read(cx).panel::<AgentPanel>(cx))
-            .map_or(false, |panel| panel.read(cx).active_thread_is_draft(cx));
+        let agent_panel_visible = self.agent_panel_visible(cx);
+        let active_thread_is_draft = self.active_thread_is_draft(cx);
 
         // Derive focused_thread from the active workspace's agent panel.
         // Only update when the panel gives us a positive signal — if the
@@ -674,7 +669,7 @@ impl Sidebar {
                     .active_conversation_view()
                     .and_then(|cv| cv.read(cx).parent_id(cx))
             });
-        if panel_focused.is_some() && !self.active_thread_is_draft {
+        if panel_focused.is_some() && !active_thread_is_draft {
             self.focused_thread = panel_focused;
         }
 
@@ -947,8 +942,8 @@ impl Sidebar {
                     entries.push(thread.into());
                 }
             } else {
-                let is_draft_for_workspace = self.agent_panel_visible
-                    && self.active_thread_is_draft
+                let is_draft_for_workspace = agent_panel_visible
+                    && active_thread_is_draft
                     && self.focused_thread.is_none()
                     && is_active;
 
@@ -2653,8 +2648,8 @@ impl Sidebar {
         let thread_workspace = thread.workspace.clone();
 
         let is_hovered = self.hovered_thread_index == Some(ix);
-        let is_selected =
-            self.agent_panel_visible && self.focused_thread.as_ref() == Some(&metadata.session_id);
+        let is_selected = self.agent_panel_visible(cx)
+            && self.focused_thread.as_ref() == Some(&metadata.session_id);
         let is_running = matches!(
             thread.status,
             AgentThreadStatus::Running | AgentThreadStatus::WaitingForConfirmation
@@ -2950,7 +2945,8 @@ impl Sidebar {
         is_selected: bool,
         cx: &mut Context<Self>,
     ) -> AnyElement {
-        let is_active = is_active_draft && self.agent_panel_visible && self.active_thread_is_draft;
+        let is_active =
+            is_active_draft && self.agent_panel_visible(cx) && self.active_thread_is_draft(cx);
 
         let label: SharedString = if is_active {
             self.active_draft_text(cx)

crates/sidebar/src/sidebar_tests.rs 🔗

@@ -2143,9 +2143,9 @@ async fn test_new_thread_button_works_after_adding_folder(cx: &mut TestAppContex
 
     // The "New Thread" button should NOT be in "active/draft" state
     // because the panel has a thread with messages.
-    sidebar.read_with(cx, |sidebar, _cx| {
+    sidebar.read_with(cx, |sidebar, cx| {
         assert!(
-            !sidebar.active_thread_is_draft,
+            !sidebar.active_thread_is_draft(cx),
             "Panel has a thread with messages, so it should not be a draft"
         );
     });
@@ -2175,9 +2175,9 @@ async fn test_new_thread_button_works_after_adding_folder(cx: &mut TestAppContex
     // The "New Thread" button must still be clickable (not stuck in
     // "active/draft" state). Verify that `active_thread_is_draft` is
     // false — the panel still has the old thread with messages.
-    sidebar.read_with(cx, |sidebar, _cx| {
+    sidebar.read_with(cx, |sidebar, cx| {
         assert!(
-            !sidebar.active_thread_is_draft,
+            !sidebar.active_thread_is_draft(cx),
             "After adding a folder the panel still has a thread with messages, \
                  so active_thread_is_draft should be false"
         );
@@ -2193,9 +2193,9 @@ async fn test_new_thread_button_works_after_adding_folder(cx: &mut TestAppContex
 
     // After creating a new thread, the panel should now be in draft
     // state (no messages on the new thread).
-    sidebar.read_with(cx, |sidebar, _cx| {
+    sidebar.read_with(cx, |sidebar, cx| {
         assert!(
-            sidebar.active_thread_is_draft,
+            sidebar.active_thread_is_draft(cx),
             "After creating a new thread the panel should be in draft state"
         );
     });
@@ -2247,13 +2247,13 @@ async fn test_cmd_n_shows_new_thread_entry(cx: &mut TestAppContext) {
         "After Cmd-N the sidebar should show a highlighted New Thread entry"
     );
 
-    sidebar.read_with(cx, |sidebar, _cx| {
+    sidebar.read_with(cx, |sidebar, cx| {
         assert!(
             sidebar.focused_thread.is_none(),
             "focused_thread should be cleared after Cmd-N"
         );
         assert!(
-            sidebar.active_thread_is_draft,
+            sidebar.active_thread_is_draft(cx),
             "the new blank thread should be a draft"
         );
     });
@@ -2383,13 +2383,13 @@ async fn test_cmd_n_shows_new_thread_entry_in_absorbed_worktree(cx: &mut TestApp
              a highlighted New Thread entry under the main repo header"
     );
 
-    sidebar.read_with(cx, |sidebar, _cx| {
+    sidebar.read_with(cx, |sidebar, cx| {
         assert!(
             sidebar.focused_thread.is_none(),
             "focused_thread should be cleared after Cmd-N"
         );
         assert!(
-            sidebar.active_thread_is_draft,
+            sidebar.active_thread_is_draft(cx),
             "the new blank thread should be a draft"
         );
     });
@@ -4881,7 +4881,8 @@ mod property_test {
             }
             Operation::ToggleAgentPanel => {
                 let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone());
-                let panel_open = sidebar.read_with(cx, |sidebar, _cx| sidebar.agent_panel_visible);
+                let panel_open =
+                    sidebar.read_with(cx, |sidebar, cx| sidebar.agent_panel_visible(cx));
                 workspace.update_in(cx, |workspace, window, cx| {
                     if panel_open {
                         workspace.close_panel::<AgentPanel>(window, cx);
@@ -5130,32 +5131,24 @@ mod property_test {
         };
 
         let workspace = multi_workspace.read(cx).workspace();
-        let panel_actually_visible = AgentPanel::is_visible(&workspace, cx);
-        let panel_active_session_id =
-            workspace
-                .read(cx)
-                .panel::<AgentPanel>(cx)
-                .and_then(|panel| {
-                    panel
-                        .read(cx)
-                        .active_conversation_view()
-                        .and_then(|cv| cv.read(cx).parent_id(cx))
-                });
 
-        // TODO: Remove this state entirely
-        anyhow::ensure!(
-            sidebar.agent_panel_visible == panel_actually_visible,
-            "sidebar.agent_panel_visible ({}) does not match AgentPanel::is_visible ({})",
-            sidebar.agent_panel_visible,
-            panel_actually_visible,
-        );
-
-        // TODO: Remove these checks, the focused_thread should _always_ be Some(item-in-the-list) after
-        // update_entries. if the activated workspace's agent panel has an active thread, this item should
-        // match the one in the list. There may be a slight delay, where a thread is loading so the agent panel
-        // returns None initially, and the focused_thread is often optimistically set to the thread the agent panel
-        // is going to be
-        if sidebar.agent_panel_visible && !sidebar.active_thread_is_draft {
+        // TODO: The focused_thread should _always_ be Some(item-in-the-list) after
+        // update_entries. If the activated workspace's agent panel has an active thread,
+        // this item should match the one in the list. There may be a slight delay where
+        // a thread is loading so the agent panel returns None initially, and the
+        // focused_thread is often optimistically set to the thread the agent panel is
+        // going to be.
+        if sidebar.agent_panel_visible(cx) && !sidebar.active_thread_is_draft(cx) {
+            let panel_active_session_id =
+                workspace
+                    .read(cx)
+                    .panel::<AgentPanel>(cx)
+                    .and_then(|panel| {
+                        panel
+                            .read(cx)
+                            .active_conversation_view()
+                            .and_then(|cv| cv.read(cx).parent_id(cx))
+                    });
             if let Some(panel_session_id) = panel_active_session_id {
                 anyhow::ensure!(
                     sidebar.focused_thread.as_ref() == Some(&panel_session_id),