From e39d5c99085792434feee223330e10fa2ec956a2 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Mon, 30 Mar 2026 18:46:08 -0700 Subject: [PATCH] Remove easily derived sidebar state (#52790) 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 --- crates/sidebar/src/sidebar.rs | 58 ++++++++++++------------- crates/sidebar/src/sidebar_tests.rs | 65 +++++++++++++---------------- 2 files changed, 56 insertions(+), 67 deletions(-) diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index d80e1f19c709ed5c865779752da46087398212df..df0d43ea03eb8e411050340eb81d1c1002585266 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/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, - agent_panel_visible: bool, - active_thread_is_draft: bool, hovered_thread_index: Option, collapsed_groups: HashSet, expanded_groups: HashMap, @@ -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::(cx) + }) + .map_or(false, |panel| panel.read(cx).active_thread_is_draft(cx)) + } + fn subscribe_to_workspace( &mut self, workspace: &Entity, @@ -486,9 +499,6 @@ impl Sidebar { if let Some(agent_panel) = workspace.read(cx).panel::(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::(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, ) -> 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) diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index 8efea2970aab9529443107b71c395d16d484c8cc..6076a056f8b0340afec88eac79b1678af27f06c2 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/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::(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::(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::(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),