From a614135b9dad61f2ebce55e1f959253a15d5478e Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 10 Apr 2026 15:54:28 -0700 Subject: [PATCH] Fix sidebar draft system CI failures - Fix archive_and_activate fallback to use mw.workspace() instead of stale active_entry_workspace(), preventing a re-entrant borrow panic when the linked worktree workspace has been removed. - Fix test_sending_message_from_draft_removes_draft to properly simulate draft removal (remove draft from panel before opening a new thread, matching the AgentPanel's MessageSentOrQueued handler). - Fix test_activating_workspace_with_draft_does_not_create_extras and test_archive_thread_active_entry_management to explicitly create drafts on workspace_b via sidebar.create_new_thread(). - Fix test_sidebar_invariants property test: use panel.new_thread() for CreateDraftThread (creates a tracked draft) and relax the active_entry invariant to allow None/stale entries when the panel is uninitialized. --- crates/sidebar/src/sidebar.rs | 19 +++--- .../src/sidebar_tests.proptest-regressions | 8 +++ crates/sidebar/src/sidebar_tests.rs | 58 +++++++++++++++---- 3 files changed, 66 insertions(+), 19 deletions(-) create mode 100644 crates/sidebar/src/sidebar_tests.proptest-regressions diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 50e613c2ce9186009059a27667d04fa139b9da09..65fd49590c6de31490cc431d76a6175e9cb51831 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -596,10 +596,6 @@ impl Sidebar { cx.emit(workspace::SidebarEvent::SerializeNeeded); } - fn active_entry_workspace(&self) -> Option<&Entity> { - self.active_entry.as_ref().map(|entry| entry.workspace()) - } - fn is_active_workspace(&self, workspace: &Entity, cx: &App) -> bool { self.multi_workspace .upgrade() @@ -649,10 +645,10 @@ impl Sidebar { cx.subscribe_in( workspace, window, - |this, _workspace, event: &workspace::Event, window, cx| { + |this, _workspace, event: &workspace::Event, _window, cx| { if let workspace::Event::PanelAdded(view) = event { if let Ok(agent_panel) = view.clone().downcast::() { - this.subscribe_to_agent_panel(&agent_panel, window, cx); + this.subscribe_to_agent_panel(&agent_panel, _window, cx); } } }, @@ -892,7 +888,8 @@ impl Sidebar { workspace: active_ws.clone(), }); } - // else: conversation is mid-load (no session_id yet), keep previous active_entry + // else: conversation is mid-load or panel is + // uninitialized — keep previous active_entry. } } @@ -2986,7 +2983,7 @@ impl Sidebar { // No neighbor or its workspace isn't open — fall back to a new // draft. Use the group workspace (main project) rather than the // active entry workspace, which may be a linked worktree that is - // about to be cleaned up. + // about to be cleaned up or already removed. let fallback_workspace = thread_folder_paths .and_then(|folder_paths| { let mw = self.multi_workspace.upgrade()?; @@ -2995,7 +2992,11 @@ impl Sidebar { let group_key = thread_workspace.read(cx).project_group_key(cx); mw.workspace_for_paths(group_key.path_list(), None, cx) }) - .or_else(|| self.active_entry_workspace().cloned()); + .or_else(|| { + self.multi_workspace + .upgrade() + .map(|mw| mw.read(cx).workspace().clone()) + }); if let Some(workspace) = fallback_workspace { self.activate_workspace(&workspace, window, cx); diff --git a/crates/sidebar/src/sidebar_tests.proptest-regressions b/crates/sidebar/src/sidebar_tests.proptest-regressions new file mode 100644 index 0000000000000000000000000000000000000000..eb3a730b444c96c5e24ef4f190b24e28e4e2200e --- /dev/null +++ b/crates/sidebar/src/sidebar_tests.proptest-regressions @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc fff3369ddfaec1a3384b1e833596a49855cb34a6f7a216857abc10196753a90d # shrinks to TestSidebarInvariantsArgs = TestSidebarInvariantsArgs { __seed: 16322195281549478794, raw_operations: [12] } +cc cde5567126e238a9eaa7e03ef51d7192cd7c7ed461e30b8b514c0f0d3a3c5389 # shrinks to TestSidebarInvariantsArgs = TestSidebarInvariantsArgs { __seed: 2287347456921018254, raw_operations: [48, 33] } diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index af4d87bdfe09f6adad0f960d7683f578584a23be..0b197b5fd278bbdf19b4c30fe27e1d591ad29696 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -3521,8 +3521,16 @@ async fn test_sending_message_from_draft_removes_draft(cx: &mut TestAppContext) assert_active_draft(sidebar, &workspace, "should be on draft before sending"); }); - // Now send a message from the draft. Set up the connection to - // respond so the thread gets content. + // Simulate what happens when a draft sends its first message: + // the AgentPanel's MessageSentOrQueued handler removes the draft + // from `draft_threads`, then the sidebar rebuilds. We can't use + // the NativeAgentServer in tests, so replicate the key steps: + // remove the draft, open a real thread with a stub connection, + // and send. + let draft_id = panel.read_with(cx, |panel, _| panel.active_draft_id().unwrap()); + panel.update_in(cx, |panel, _window, _cx| { + panel.remove_draft(draft_id); + }); let draft_connection = StubAgentConnection::new(); draft_connection.set_next_prompt_updates(vec![acp::SessionUpdate::AgentMessageChunk( acp::ContentChunk::new("World".into()), @@ -5949,6 +5957,12 @@ async fn test_archive_thread_active_entry_management(cx: &mut TestAppContext) { let panel_b = add_agent_panel(&workspace_b, cx); cx.run_until_parked(); + // Explicitly create a draft on workspace_b so the sidebar tracks one. + sidebar.update_in(cx, |sidebar, window, cx| { + sidebar.create_new_thread(&workspace_b, window, cx); + }); + cx.run_until_parked(); + // --- Scenario 1: archive a thread in the non-active workspace --- // Create a thread in project-a (non-active — project-b is active). @@ -7591,6 +7605,12 @@ async fn test_activating_workspace_with_draft_does_not_create_extras(cx: &mut Te let _panel_b = add_agent_panel(&workspace_b, cx); cx.run_until_parked(); + // Explicitly create a draft on workspace_b so the sidebar tracks one. + sidebar.update_in(cx, |sidebar, window, cx| { + sidebar.create_new_thread(&workspace_b, window, cx); + }); + cx.run_until_parked(); + // Count project-b's drafts. let count_b_drafts = |cx: &mut gpui::VisualTestContext| { let entries = visible_entries_as_strings(&sidebar, cx); @@ -7865,8 +7885,9 @@ mod property_test { let panel = workspace.read_with(cx, |workspace, cx| workspace.panel::(cx)); if let Some(panel) = panel { - let connection = StubAgentConnection::new(); - open_thread_with_connection(&panel, connection, cx); + panel.update_in(cx, |panel, window, cx| { + panel.new_thread(&NewThread, window, cx); + }); cx.run_until_parked(); } workspace.update_in(cx, |workspace, window, cx| { @@ -8283,11 +8304,29 @@ mod property_test { let active_workspace = multi_workspace.read(cx).workspace(); - // 1. active_entry must always be Some after rebuild_contents. - let entry = sidebar - .active_entry - .as_ref() - .ok_or_else(|| anyhow::anyhow!("active_entry must always be Some"))?; + // 1. active_entry should be Some when the panel has content. + // It may be None when the panel is uninitialized (no drafts, + // no threads), which is fine. + // It may also temporarily point at a different workspace + // when the workspace just changed and the new panel has no + // content yet. + let panel = active_workspace.read(cx).panel::(cx).unwrap(); + let panel_has_content = panel.read(cx).active_draft_id().is_some() + || panel.read(cx).active_conversation_view().is_some(); + + let Some(entry) = sidebar.active_entry.as_ref() else { + if panel_has_content { + anyhow::bail!("active_entry is None but panel has content (draft or thread)"); + } + return Ok(()); + }; + + // If the entry workspace doesn't match the active workspace + // and the panel has no content, this is a transient state that + // will resolve when the panel gets content. + if entry.workspace().entity_id() != active_workspace.entity_id() && !panel_has_content { + return Ok(()); + } // 2. The entry's workspace must agree with the multi-workspace's // active workspace. @@ -8299,7 +8338,6 @@ mod property_test { ); // 3. The entry must match the agent panel's current state. - let panel = active_workspace.read(cx).panel::(cx).unwrap(); if panel.read(cx).active_draft_id().is_some() { anyhow::ensure!( matches!(entry, ActiveEntry::Draft { .. }),