agent_ui: Stop the sidebar creating two terminals for one new entry (#62151)
# Objective Starting a new terminal thread from the agent sidebar creates **two** terminals instead of one, in a project that has no terminals yet. With terminals already open, only one is created. ## Solution `Sidebar::create_new_terminal` calls `AgentPanel::new_terminal` and then focuses the panel. The spawn is async, so when `focus_panel` activates the panel, `base_view` is still `Uninitialized` — and `ensure_thread_initialized` reads that as an empty panel and spawns its own "initial" terminal on top of the one already in flight. Once terminals exist they are restored at panel load, so `base_view` is already `Terminal` and the second spawn never happens. That's why the bug only appears on a project with no terminals. `restore_terminal` already guarded against this by marking the spawn pending before calling `spawn_terminal`. This moves that marker into `spawn_terminal` itself, so every spawn path arms it, and drops the now-redundant assignment in `restore_terminal`. The marker is cleared exactly where it already was: when the terminal lands, when the spawn fails, and when the terminal is closed. ## Testing - Added `test_new_terminal_prevents_initial_terminal_creation`, which reproduces the sidebar's spawn-then-focus sequence. It fails without the fix (the panel ends up with two terminals) and passes with it. - `cargo test -p agent_ui --lib` — 405 passed. - `cargo test -p sidebar --lib` — 142 passed. - `./script/clippy -p agent_ui` — clean. - Tested on macOS only. The race is platform-independent, but I can't exercise the Linux/Windows terminal spawn paths. ## 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 adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [ X] Tests cover the new/changed behavior - [ X] Performance impact has been considered and is acceptable --- Release Notes: - Fixed the agent panel creating two terminals when starting a new terminal thread in a project with no existing terminals --------- Co-authored-by: Claude Opus 5 <claude-opus-5@claude-cli>
This commit is contained in:
restlessroninandClaude Opus 5 authored and GitHub committed
1 parent
30ba5d4ddb
commit
99dddba6b8
1 file changed
+103
-15
@@ -1169,6 +1169,8 @@ pub struct AgentPanel {
|
||||
retained_threads: HashMap<ThreadId, Entity<ConversationView>>,
|
||||
terminals: HashMap<TerminalId, AgentTerminal>,
|
||||
pending_terminal_spawn: Option<TerminalId>,
|
||||
#[cfg(test)]
|
||||
test_terminal_spawn_gate: Option<futures::channel::oneshot::Receiver<()>>,
|
||||
new_thread_menu_handle: PopoverMenuHandle<ContextMenu>,
|
||||
agent_panel_menu_handle: PopoverMenuHandle<ContextMenu>,
|
||||
_extension_subscription: Option<Subscription>,
|
||||
@@ -1582,6 +1584,8 @@ impl AgentPanel {
|
||||
retained_threads: HashMap::default(),
|
||||
terminals: HashMap::default(),
|
||||
pending_terminal_spawn: None,
|
||||
#[cfg(test)]
|
||||
test_terminal_spawn_gate: None,
|
||||
new_thread_menu_handle: PopoverMenuHandle::default(),
|
||||
agent_panel_menu_handle: PopoverMenuHandle::default(),
|
||||
|
||||
@@ -2073,11 +2077,14 @@ impl AgentPanel {
|
||||
window: &mut Window,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
// The spawn is async, so the panel keeps its current base view until
|
||||
// the terminal lands. Marking the spawn as pending stops
|
||||
// `ensure_thread_initialized` from treating a still-uninitialized panel
|
||||
// as empty and spawning a second, "initial" terminal on activation.
|
||||
self.pending_terminal_spawn = Some(terminal_id);
|
||||
let terminal_working_directory = working_directory.clone();
|
||||
let init_command = Self::terminal_init_command(run_init_command, cx);
|
||||
let terminal_task = self.project.update(cx, |project, cx| {
|
||||
project.create_terminal_shell(working_directory, cx)
|
||||
});
|
||||
let terminal_task = self.create_terminal_shell(working_directory, cx);
|
||||
let workspace = self.workspace.clone();
|
||||
let workspace_id = self.workspace_id;
|
||||
let project = self.project.downgrade();
|
||||
@@ -2128,6 +2135,27 @@ impl AgentPanel {
|
||||
.detach_and_log_err(cx);
|
||||
}
|
||||
|
||||
fn create_terminal_shell(
|
||||
&mut self,
|
||||
working_directory: Option<PathBuf>,
|
||||
cx: &mut Context<Self>,
|
||||
) -> Task<Result<Entity<terminal::Terminal>>> {
|
||||
// A real shell ties the spawn's timing to the host, so a test that needs
|
||||
// to observe the panel mid-spawn installs a gate and gets a display-only
|
||||
// terminal when it releases it.
|
||||
#[cfg(test)]
|
||||
if let Some(gate) = self.test_terminal_spawn_gate.take() {
|
||||
return cx.spawn(async move |this, cx| {
|
||||
gate.await.ok();
|
||||
this.update(cx, |this, cx| this.build_display_only_terminal(cx))
|
||||
});
|
||||
}
|
||||
|
||||
self.project.update(cx, |project, cx| {
|
||||
project.create_terminal_shell(working_directory, cx)
|
||||
})
|
||||
}
|
||||
|
||||
fn terminal_init_command(run_init_command: bool, cx: &App) -> Option<String> {
|
||||
run_init_command
|
||||
.then(|| AgentSettings::get_global(cx).terminal_init_command.clone())
|
||||
@@ -2452,7 +2480,6 @@ impl AgentPanel {
|
||||
return;
|
||||
}
|
||||
|
||||
self.pending_terminal_spawn = Some(metadata.terminal_id);
|
||||
let working_directory = self.terminal_restore_working_directory(&metadata, workspace, cx);
|
||||
let initial_title = Self::terminal_restore_initial_title(&metadata);
|
||||
self.spawn_terminal(
|
||||
@@ -6797,6 +6824,21 @@ impl AgentPanel {
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
fn build_display_only_terminal(&self, cx: &mut Context<Self>) -> Entity<terminal::Terminal> {
|
||||
let settings = TerminalSettings::get_global(cx).clone();
|
||||
let path_style = self.project.read(cx).path_style(cx);
|
||||
let builder = terminal::TerminalBuilder::new_display_only(
|
||||
settings.cursor_shape,
|
||||
settings.alternate_scroll,
|
||||
settings.max_scroll_history_lines,
|
||||
cx.entity_id().as_u64(),
|
||||
cx.background_executor(),
|
||||
path_style,
|
||||
);
|
||||
cx.new(|cx| builder.subscribe(cx))
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
fn insert_display_only_terminal(
|
||||
&mut self,
|
||||
@@ -6813,17 +6855,7 @@ impl AgentPanel {
|
||||
cx: &mut Context<Self>,
|
||||
) -> Result<()> {
|
||||
let init_command = Self::terminal_init_command(run_init_command, cx);
|
||||
let settings = TerminalSettings::get_global(cx).clone();
|
||||
let path_style = self.project.read(cx).path_style(cx);
|
||||
let builder = terminal::TerminalBuilder::new_display_only(
|
||||
settings.cursor_shape,
|
||||
settings.alternate_scroll,
|
||||
settings.max_scroll_history_lines,
|
||||
cx.entity_id().as_u64(),
|
||||
cx.background_executor(),
|
||||
path_style,
|
||||
);
|
||||
let terminal = cx.new(|cx| builder.subscribe(cx));
|
||||
let terminal = self.build_display_only_terminal(cx);
|
||||
let terminal_for_init_command = terminal.clone();
|
||||
let terminal_view = cx.new(|cx| {
|
||||
let mut view = TerminalView::new(
|
||||
@@ -7674,6 +7706,62 @@ mod tests {
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_new_terminal_prevents_initial_terminal_creation(cx: &mut TestAppContext) {
|
||||
let (panel, mut cx) = setup_panel(cx).await;
|
||||
|
||||
let (terminal_landed, spawn_gate) = futures::channel::oneshot::channel::<()>();
|
||||
|
||||
// `create_new_terminal` in the sidebar calls `new_terminal` and then
|
||||
// focuses the panel, which activates it while the spawn is still in
|
||||
// flight. The gate keeps the spawn in flight for as long as the test
|
||||
// wants, instead of racing a real shell.
|
||||
panel.update_in(&mut cx, |panel, window, cx| {
|
||||
panel.test_terminal_spawn_gate = Some(spawn_gate);
|
||||
panel.new_terminal(None, AgentThreadSource::AgentPanel, window, cx);
|
||||
assert!(
|
||||
panel.pending_terminal_spawn.is_some(),
|
||||
"a new terminal spawn should be marked pending until it lands"
|
||||
);
|
||||
panel.set_active(true, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
panel.read_with(&cx, |panel, cx| {
|
||||
assert!(
|
||||
panel.terminals(cx).is_empty(),
|
||||
"no terminal should land while the spawn is held"
|
||||
);
|
||||
assert!(
|
||||
panel.pending_terminal_spawn.is_some(),
|
||||
"the in-flight spawn should still be pending after activation"
|
||||
);
|
||||
});
|
||||
|
||||
terminal_landed
|
||||
.send(())
|
||||
.expect("the panel should still be waiting on the terminal spawn");
|
||||
cx.run_until_parked();
|
||||
|
||||
panel.read_with(&cx, |panel, cx| {
|
||||
let terminals = panel.terminals(cx);
|
||||
assert_eq!(
|
||||
terminals.len(),
|
||||
1,
|
||||
"activation while a new terminal is spawning should not create a second terminal"
|
||||
);
|
||||
assert_eq!(
|
||||
panel.active_terminal_id(),
|
||||
terminals.first().map(|terminal| terminal.id),
|
||||
"the terminal that was spawned should be the active one"
|
||||
);
|
||||
assert!(
|
||||
panel.pending_terminal_spawn.is_none(),
|
||||
"the pending marker should be cleared once the terminal lands"
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_restored_terminal_runs_init_command_once(cx: &mut TestAppContext) {
|
||||
let (panel, mut cx) = setup_panel(cx).await;
|
||||
|
||||
Reference in new issue
Block a user