mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Keep Command Center selection adjacent after task removal (#50505)
## Why Archiving or deleting the selected task could make Command Center selection jump to the current task instead of a neighboring task in the displayed list. ## What changed Choose the next surviving task in displayed order before removing tasks, falling back to the previous task when needed. Apply this selection behavior to archive, delete, and hide actions, respecting filtering and grouping. Preserve the pending selection across view rebuilds, batched removals, and duplicate server notifications. Allow selection to fall back when a rename target disappears while retaining the existing notice about its unsubmitted title. ## Testing Add regression tests and snapshots for archive/delete selection across project, status, and model grouping, with and without filtering. Cover removal of the attached task, external removal notifications, removal of a pending successor, repeated repaints, and an empty task list. Update the disappeared-rename-target snapshot to verify that the remaining selection is preserved. GitOrigin-RevId: ca93d664fadbc602993fbdae316c7a375b449345
This commit is contained in:
committed by
copyberry
parent
d42aecc56b
commit
47379efd52
@@ -12,6 +12,9 @@ mod errors;
|
||||
#[path = "agents_overview_loading.rs"]
|
||||
mod loading;
|
||||
|
||||
#[path = "agents_overview_selection.rs"]
|
||||
mod selection;
|
||||
|
||||
use super::agents_overview_view::AgentsOverviewGroup;
|
||||
use super::agents_overview_view::AgentsOverviewRow;
|
||||
use super::agents_overview_view::AgentsOverviewView;
|
||||
@@ -55,6 +58,8 @@ pub(super) struct AgentsOverviewState {
|
||||
pub(super) refresh_notifications: HashMap<ThreadId, Vec<ServerNotification>>,
|
||||
pub(super) rendered_full_screen: bool,
|
||||
pub(super) visible_thread_ids: Vec<ThreadId>,
|
||||
/// One-shot successor selected before a removal invalidates the displayed rows.
|
||||
pub(super) selection_after_removal: Option<ThreadId>,
|
||||
pub(super) view_state:
|
||||
Arc<std::sync::Mutex<super::agents_overview_view::AgentsOverviewViewState>>,
|
||||
/// Explicit permission-profile choices for new-session carryover, retained across navigation.
|
||||
@@ -133,7 +138,8 @@ impl App {
|
||||
.flatten()
|
||||
.cloned()
|
||||
.collect();
|
||||
let view = self.agents_overview_view(threads, /*selected_thread_id*/ None);
|
||||
let selected_thread_id = self.agents_overview.selection_after_removal.take();
|
||||
let view = self.agents_overview_view(threads, selected_thread_id);
|
||||
self.agents_overview.visible_thread_ids = view.thread_ids();
|
||||
self.chat_widget.show_bottom_pane_view(Box::new(view));
|
||||
if self.reconnect.offline {
|
||||
@@ -271,6 +277,11 @@ impl App {
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.rename_target
|
||||
.filter(|id| {
|
||||
self.agents_overview.threads.contains_key(id)
|
||||
&& !self.agents_overview.hidden_threads.contains(id)
|
||||
})
|
||||
.or(self.agents_overview.selection_after_removal.take())
|
||||
.or_else(|| {
|
||||
self.agents_overview
|
||||
.visible_thread_ids
|
||||
@@ -286,10 +297,10 @@ impl App {
|
||||
.collect();
|
||||
let view = self.agents_overview_view(threads, selected_thread_id);
|
||||
self.agents_overview.visible_thread_ids = view.thread_ids();
|
||||
if selected_thread_id
|
||||
.is_some_and(|thread_id| !self.agents_overview.visible_thread_ids.contains(&thread_id))
|
||||
&& let Ok(mut state) = self.agents_overview.view_state.lock()
|
||||
&& state.rename_target.is_some()
|
||||
if let Ok(mut state) = self.agents_overview.view_state.lock()
|
||||
&& state
|
||||
.rename_target
|
||||
.is_some_and(|id| !self.agents_overview.visible_thread_ids.contains(&id))
|
||||
{
|
||||
self.chat_widget.add_info_message(
|
||||
format!(
|
||||
|
||||
@@ -268,6 +268,7 @@ impl App {
|
||||
{
|
||||
removed.insert(primary);
|
||||
}
|
||||
self.prepare_agents_overview_removal(&removed);
|
||||
for removed_id in removed {
|
||||
self.remove_agents_overview_thread(removed_id);
|
||||
self.agents_overview.activity.remove(&removed_id);
|
||||
|
||||
@@ -109,6 +109,254 @@ async fn lifecycle_shortcuts_target_filtered_task_in_any_state() {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn archiving_selects_the_next_displayed_task() -> Result<()> {
|
||||
for action in [AgentsOverviewAction::Archive, AgentsOverviewAction::Delete] {
|
||||
let mut selections = Vec::new();
|
||||
for grouping in [
|
||||
AgentsOverviewGrouping::Project,
|
||||
AgentsOverviewGrouping::Status,
|
||||
AgentsOverviewGrouping::Model,
|
||||
] {
|
||||
for filtered in [false, true] {
|
||||
let (mut app, mut rx, _op_rx) =
|
||||
crate::app::tests::make_test_app_with_channels().await;
|
||||
let mut app_server =
|
||||
crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
tui.pause_events();
|
||||
app.app_server_target = AppServerTarget::LocalDaemon {
|
||||
allow_embedded_fallback: true,
|
||||
endpoint: crate::RemoteAppServerEndpoint::UnixSocket {
|
||||
socket_path: test_path_buf("/tmp/unused.sock").abs(),
|
||||
},
|
||||
};
|
||||
let mut keymap = TuiKeymap::default();
|
||||
match action {
|
||||
AgentsOverviewAction::Archive => {
|
||||
keymap.agents.archive =
|
||||
Some(KeybindingsSpec::One(KeybindingSpec("f5".into())))
|
||||
}
|
||||
AgentsOverviewAction::Delete => {
|
||||
keymap.agents.delete =
|
||||
Some(KeybindingsSpec::One(KeybindingSpec("f5".into())))
|
||||
}
|
||||
}
|
||||
app.keymap = RuntimeKeymap::from_config(&keymap).unwrap();
|
||||
let mut threads = Vec::new();
|
||||
let mut ids = Vec::new();
|
||||
for index in 1..=4 {
|
||||
let title = format!("{} {index}", if index == 2 { "Other" } else { "Task" });
|
||||
let id = ThreadId::from_string(
|
||||
&app_test_support::create_fake_rollout(
|
||||
&app.config.codex_home,
|
||||
&format!("2025-01-0{index}T12-00-00"),
|
||||
&format!("2025-01-0{index}T12:00:00Z"),
|
||||
&title,
|
||||
Some(&app.config.model_provider_id),
|
||||
/*git_info*/ None,
|
||||
)
|
||||
.expect("materialize task"),
|
||||
)?;
|
||||
let mut thread = overview_thread(
|
||||
id,
|
||||
/*parent_thread_id*/ None,
|
||||
&title,
|
||||
ThreadStatus::Idle,
|
||||
);
|
||||
thread.cwd = test_path_buf(&format!("/tmp/project-{index}")).abs();
|
||||
thread.model = Some(format!("model-{index}"));
|
||||
thread.updated_at = index as i64;
|
||||
app.agents_overview.threads.insert(id, Some(thread.clone()));
|
||||
threads.push(thread);
|
||||
ids.push(id);
|
||||
}
|
||||
// Task 1 exercises rebuilding the dashboard after archiving the attached task.
|
||||
let resumed = app_server
|
||||
.resume_thread(
|
||||
&app.local_settings,
|
||||
app.config.clone(),
|
||||
ids[0],
|
||||
crate::app_server_session::ResumeModelSettings::PreserveExistingThread,
|
||||
)
|
||||
.await?;
|
||||
app.enqueue_primary_thread_session(resumed.session, resumed.turns)
|
||||
.await?;
|
||||
app.agents_overview.view_state.lock().unwrap().grouping = grouping;
|
||||
let mut view = app.agents_overview_view(threads, Some(ids[2]));
|
||||
if filtered {
|
||||
view.handle_key_event(KeyCode::Char('/').into());
|
||||
view.handle_paste("Task".into());
|
||||
view.handle_key_event(KeyCode::Down.into());
|
||||
}
|
||||
let expected = match (grouping, filtered) {
|
||||
(AgentsOverviewGrouping::Status, false) => vec![3, 2, 1, 4],
|
||||
(AgentsOverviewGrouping::Status, true) => vec![3, 1, 4],
|
||||
(AgentsOverviewGrouping::Project | AgentsOverviewGrouping::Model, false) => {
|
||||
vec![3, 4, 2, 1]
|
||||
}
|
||||
(AgentsOverviewGrouping::Project | AgentsOverviewGrouping::Model, true) => {
|
||||
vec![3, 4, 1]
|
||||
}
|
||||
};
|
||||
assert_eq!(
|
||||
view.selection_after_removal(&HashSet::from([ids[2], ids[expected[1] - 1]])),
|
||||
Some(ids[expected[2] - 1])
|
||||
);
|
||||
app.agents_overview.visible_thread_ids = view.thread_ids();
|
||||
app.chat_widget.show_bottom_pane_view(Box::new(view));
|
||||
for index in expected {
|
||||
let selected = app
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.agents_overview.visible_thread_ids.get(selected),
|
||||
Some(&ids[index - 1])
|
||||
);
|
||||
let rendered = render_bottom_popup(&app.chat_widget, /*width*/ 100);
|
||||
let selected_row = rendered
|
||||
.lines()
|
||||
.find(|line| line.trim_start().starts_with('›'))
|
||||
.expect("selected task");
|
||||
let selected_row = selected_row.split('│').next().unwrap().trim();
|
||||
if !filtered && grouping == AgentsOverviewGrouping::Status {
|
||||
selections.push(selected_row.to_owned());
|
||||
}
|
||||
if !filtered {
|
||||
app.chat_widget.handle_key_event(KeyCode::Char('r').into());
|
||||
}
|
||||
app.chat_widget.handle_key_event(KeyCode::F(5).into());
|
||||
let confirmation = std::iter::from_fn(|| rx.try_recv().ok())
|
||||
.find(|event| matches!(event, AppEvent::ConfirmAgentsOverviewAction { .. }))
|
||||
.expect("archive shortcut while editing");
|
||||
Box::pin(app.handle_event(&mut tui, &mut app_server, confirmation)).await?;
|
||||
app.chat_widget.handle_key_event(KeyCode::Char('2').into());
|
||||
if action == AgentsOverviewAction::Delete {
|
||||
app.chat_widget.handle_key_event(KeyCode::Enter.into());
|
||||
}
|
||||
let confirmed = std::iter::from_fn(|| rx.try_recv().ok())
|
||||
.find(|event| matches!(event, AppEvent::RunAgentsOverviewAction { .. }))
|
||||
.expect("confirmed archive");
|
||||
Box::pin(app.handle_event(&mut tui, &mut app_server, confirmed)).await?;
|
||||
// Later repaints retain the selected successor.
|
||||
app.repaint_agents_overview();
|
||||
}
|
||||
let selected = app
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.unwrap();
|
||||
assert_eq!(app.agents_overview.visible_thread_ids.get(selected), None);
|
||||
assert_eq!(app.agents_overview.selection_after_removal, None);
|
||||
app_server.shutdown().await?;
|
||||
}
|
||||
}
|
||||
let snapshot = match action {
|
||||
AgentsOverviewAction::Archive => "archiving_selects_the_next_displayed_task",
|
||||
AgentsOverviewAction::Delete => "deleting_selects_the_next_displayed_task",
|
||||
};
|
||||
insta::assert_snapshot!(snapshot, selections.join("\n"));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn external_removals_preserve_adjacent_selection() {
|
||||
for grouping in [
|
||||
AgentsOverviewGrouping::Project,
|
||||
AgentsOverviewGrouping::Status,
|
||||
AgentsOverviewGrouping::Model,
|
||||
] {
|
||||
for filtered in [false, true] {
|
||||
let mut app = make_test_app().await;
|
||||
let ids = [1, 2, 3, 4].map(ThreadId::from_u128);
|
||||
let threads = ids
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(index, id)| {
|
||||
let mut thread = overview_thread(
|
||||
*id,
|
||||
/*parent_thread_id*/ None,
|
||||
if index == 1 { "Other" } else { "Task" },
|
||||
ThreadStatus::Idle,
|
||||
);
|
||||
thread.cwd = test_path_buf(&format!("/tmp/project-{index}")).abs();
|
||||
thread.model = Some(format!("model-{index}"));
|
||||
thread.updated_at = index as i64;
|
||||
thread
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
app.agents_overview.threads = ids
|
||||
.into_iter()
|
||||
.zip(threads.iter().cloned().map(Some))
|
||||
.collect();
|
||||
app.agents_overview.view_state.lock().unwrap().grouping = grouping;
|
||||
let mut view = app.agents_overview_view(threads, Some(ids[2]));
|
||||
if filtered {
|
||||
view.handle_key_event(KeyCode::Char('/').into());
|
||||
view.handle_paste("Task".into());
|
||||
view.handle_key_event(KeyCode::Down.into());
|
||||
}
|
||||
app.agents_overview.visible_thread_ids = view.thread_ids();
|
||||
app.chat_widget.show_bottom_pane_view(Box::new(view));
|
||||
// Removing another task must retain selection even when its index changes.
|
||||
let other = ServerNotification::ThreadDeleted(ThreadDeletedNotification {
|
||||
thread_id: ids[1].to_string(),
|
||||
});
|
||||
app.track_agents_overview_notification(&other);
|
||||
app.repaint_agents_overview();
|
||||
let selected = app
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.agents_overview.visible_thread_ids.get(selected),
|
||||
Some(&ids[2])
|
||||
);
|
||||
let expected = match grouping {
|
||||
AgentsOverviewGrouping::Status => [ids[0], ids[3]],
|
||||
AgentsOverviewGrouping::Project | AgentsOverviewGrouping::Model => [ids[3], ids[0]],
|
||||
};
|
||||
let archive = ServerNotification::ThreadArchived(ThreadArchivedNotification {
|
||||
thread_id: ids[2].to_string(),
|
||||
});
|
||||
app.track_agents_overview_notification(&archive);
|
||||
app.track_agents_overview_notification(&archive);
|
||||
assert_eq!(
|
||||
app.agents_overview.selection_after_removal,
|
||||
Some(expected[0])
|
||||
);
|
||||
// A notification burst can remove the pending successor before repaint.
|
||||
let delete = ServerNotification::ThreadDeleted(ThreadDeletedNotification {
|
||||
thread_id: expected[0].to_string(),
|
||||
});
|
||||
app.track_agents_overview_notification(&delete);
|
||||
app.track_agents_overview_notification(&archive);
|
||||
app.repaint_agents_overview();
|
||||
app.repaint_agents_overview();
|
||||
let selected = app
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.agents_overview.visible_thread_ids.get(selected),
|
||||
Some(&expected[1])
|
||||
);
|
||||
app.track_agents_overview_notification(&ServerNotification::ThreadDeleted(
|
||||
ThreadDeletedNotification {
|
||||
thread_id: expected[1].to_string(),
|
||||
},
|
||||
));
|
||||
app.repaint_agents_overview();
|
||||
let selected = app
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.unwrap();
|
||||
assert_eq!(app.agents_overview.visible_thread_ids.get(selected), None);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn hiding_tasks_keeps_selection_adjacent_in_display_order() -> Result<()> {
|
||||
let (mut app, mut rx, _op_rx) = crate::app::tests::make_test_app_with_channels().await;
|
||||
|
||||
@@ -0,0 +1,32 @@
|
||||
//! Preserves Command Center selection across local and server-driven removals.
|
||||
//! Choose against the displayed order before mutation; pending successors survive
|
||||
//! batched removals and duplicate completion notifications until the next repaint.
|
||||
|
||||
use super::AGENTS_OVERVIEW_VIEW_ID;
|
||||
use super::App;
|
||||
use codex_protocol::ThreadId;
|
||||
use std::collections::HashSet;
|
||||
|
||||
impl App {
|
||||
pub(in crate::app) fn prepare_agents_overview_removal(&mut self, removed: &HashSet<ThreadId>) {
|
||||
let selected = self.agents_overview.selection_after_removal.or_else(|| {
|
||||
let index = self
|
||||
.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)?;
|
||||
self.agents_overview.visible_thread_ids.get(index).copied()
|
||||
});
|
||||
let Some(selected) = selected.filter(|id| removed.contains(id)) else {
|
||||
return;
|
||||
};
|
||||
let threads = self
|
||||
.agents_overview
|
||||
.threads
|
||||
.values()
|
||||
.flatten()
|
||||
.cloned()
|
||||
.collect();
|
||||
self.agents_overview.selection_after_removal = self
|
||||
.agents_overview_view(threads, Some(selected))
|
||||
.selection_after_removal(removed);
|
||||
}
|
||||
}
|
||||
@@ -55,6 +55,7 @@ fn detail_thread_ids<'a>(threads: impl Iterator<Item = &'a Thread>) -> Vec<Threa
|
||||
|
||||
impl App {
|
||||
pub(super) fn remove_agents_overview_thread(&mut self, thread_id: ThreadId) {
|
||||
self.prepare_agents_overview_removal(&std::collections::HashSet::from([thread_id]));
|
||||
self.agents_overview.removed_threads.insert(thread_id);
|
||||
if let Some(Some(thread)) = self.agents_overview.threads.remove(&thread_id)
|
||||
&& !thread.ephemeral
|
||||
|
||||
@@ -204,8 +204,10 @@ impl AgentsOverviewView {
|
||||
.lock()
|
||||
.unwrap_or_else(PoisonError::into_inner)
|
||||
.rename_target
|
||||
.or(selected_thread_id)
|
||||
.and_then(|thread_id| rows.iter().position(|row| row.thread_id == thread_id))
|
||||
.or_else(|| {
|
||||
selected_thread_id.and_then(|id| rows.iter().position(|row| row.thread_id == id))
|
||||
})
|
||||
.or_else(|| rows.iter().position(|row| row.is_current))
|
||||
.unwrap_or(0);
|
||||
let project_groups = rows
|
||||
@@ -268,6 +270,19 @@ impl AgentsOverviewView {
|
||||
self.rows.iter().map(|row| row.thread_id).collect()
|
||||
}
|
||||
|
||||
pub(super) fn selection_after_removal(
|
||||
&self,
|
||||
removed: &std::collections::HashSet<ThreadId>,
|
||||
) -> Option<ThreadId> {
|
||||
let visible = self.visible_indices();
|
||||
let position = visible.iter().position(|index| *index == self.selected)?;
|
||||
visible[position..]
|
||||
.iter()
|
||||
.chain(visible[..position].iter().rev())
|
||||
.map(|index| self.rows[*index].thread_id)
|
||||
.find(|id| !removed.contains(id))
|
||||
}
|
||||
|
||||
fn title_style(&self, thread_id: ThreadId) -> Style {
|
||||
if self.use_theme_colors {
|
||||
Style::default().fg(crate::thread_color::thread_color(thread_id))
|
||||
@@ -701,9 +716,6 @@ impl BottomPaneView for AgentsOverviewView {
|
||||
if self.agents_keymap.hide.is_pressed(key) {
|
||||
if let Some(row) = self.selected_row() {
|
||||
let thread_id = row.thread_id;
|
||||
// Keep an adjacent task selected when hiding rebuilds the view.
|
||||
let forward = self.visible_indices().last() != Some(&self.selected);
|
||||
self.move_selection(forward);
|
||||
self.app_event_tx
|
||||
.send(AppEvent::HideAgentsOverviewThread { thread_id });
|
||||
}
|
||||
|
||||
@@ -2902,6 +2902,7 @@ impl App {
|
||||
}
|
||||
}
|
||||
AppEvent::HideAgentsOverviewThread { thread_id } => {
|
||||
self.prepare_agents_overview_removal(&HashSet::from([thread_id]));
|
||||
self.agents_overview.hidden_threads.insert(thread_id);
|
||||
self.repaint_agents_overview();
|
||||
}
|
||||
|
||||
+9
@@ -0,0 +1,9 @@
|
||||
---
|
||||
source: tui/src/app/agents_overview_actions_tests.rs
|
||||
assertion_line: 301
|
||||
expression: "selections.join(\"\\n\")"
|
||||
---
|
||||
› ○ Task 3
|
||||
› ○ Other 2
|
||||
› ○ Task 1
|
||||
› ○ Task 4
|
||||
+9
@@ -0,0 +1,9 @@
|
||||
---
|
||||
source: tui/src/app/agents_overview_actions_tests.rs
|
||||
assertion_line: 308
|
||||
expression: "selections.join(\"\\n\")"
|
||||
---
|
||||
› ○ Task 3
|
||||
› ○ Other 2
|
||||
› ○ Task 1
|
||||
› ○ Task 4
|
||||
+3
-3
@@ -7,15 +7,15 @@ expression: "render_bottom_popup(&app.chat_widget, 100)"
|
||||
────────────────────────────────────────────────────────────────────────────────────────────────
|
||||
Tasks │ Task details
|
||||
/tmp/project 2 │
|
||||
› ● New task │ New task
|
||||
○ Selected task │ ● Working
|
||||
● New task │ Selected task
|
||||
› ○ Selected task │ ○ Ready
|
||||
│
|
||||
│ Project
|
||||
│ /tmp/project
|
||||
│ Model: Unknown
|
||||
│
|
||||
│ Prompt
|
||||
│ New task
|
||||
│ Selected task
|
||||
│
|
||||
│
|
||||
│
|
||||
|
||||
Reference in New Issue
Block a user