From ac3f8c04b645c27b534958efae6b4cd7db177a37 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 00:46:38 +0000 Subject: [PATCH] =?UTF-8?q?fix(tui):=20rebuild=20sidebar=20click-action=20?= =?UTF-8?q?mapping=20=E2=80=94=20lines=20and=20actions=20now=20built=20in?= =?UTF-8?q?=20one=20pass=20so=20indices=20can't=20drift;=20shell=5F*=20job?= =?UTF-8?q?s=20route=20via=20/jobs,=20task=20jobs=20via=20/task;=20finishe?= =?UTF-8?q?d=20jobs=20lose=20the=20cancel=20target;=20agents=20panel=20gat?= =?UTF-8?q?es=20role-mix=20slot=20correctly;=20fix=20test=20compile;=20add?= =?UTF-8?q?=20action-mapping=20+=20click-resolution=20tests=20(#3028)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude https://claude.ai/code/session_018zaP8vUfTAsrE38L6h6fw5 --- crates/tui/src/tui/app.rs | 4 +- crates/tui/src/tui/mouse_ui.rs | 113 ++++++++++ crates/tui/src/tui/sidebar.rs | 378 +++++++++++++++++++++++++++------ 3 files changed, 434 insertions(+), 61 deletions(-) diff --git a/crates/tui/src/tui/app.rs b/crates/tui/src/tui/app.rs index b4f068a6..09f7acbd 100644 --- a/crates/tui/src/tui/app.rs +++ b/crates/tui/src/tui/app.rs @@ -1158,7 +1158,9 @@ pub struct SidebarHoverRow { /// Whether the compact row lost information. pub is_truncated: bool, /// Slash command to execute when this row is clicked (#3028). - /// E.g. `/task show shell_abc123` or `/task cancel shell_abc123`. + /// `shell_*` job ids route through `/jobs` (e.g. `/jobs cancel + /// shell_abc123`); task-manager ids route through `/task` (e.g. + /// `/task show task_abc123`). pub click_action: Option, } diff --git a/crates/tui/src/tui/mouse_ui.rs b/crates/tui/src/tui/mouse_ui.rs index cb1d8802..0206eda3 100644 --- a/crates/tui/src/tui/mouse_ui.rs +++ b/crates/tui/src/tui/mouse_ui.rs @@ -1024,3 +1024,116 @@ pub(crate) fn selection_to_text(app: &App) -> Option { } Some(selected) } + +#[cfg(test)] +mod tests { + use super::sidebar_click_action; + use crate::config::Config; + use crate::tui::app::{App, SidebarHoverRow, SidebarHoverSection, TuiOptions}; + use crossterm::event::{KeyModifiers, MouseButton, MouseEvent, MouseEventKind}; + use ratatui::layout::Rect; + use std::path::PathBuf; + + fn create_test_app() -> App { + let options = TuiOptions { + model: "deepseek-v4-pro".to_string(), + workspace: PathBuf::from("."), + config_path: None, + config_profile: None, + allow_shell: false, + use_alt_screen: true, + use_mouse_capture: false, + use_bracketed_paste: true, + max_subagents: 1, + skills_dir: PathBuf::from("."), + memory_path: PathBuf::from("memory.md"), + notes_path: PathBuf::from("notes.txt"), + mcp_config_path: PathBuf::from("mcp.json"), + use_memory: false, + start_in_agent_mode: false, + skip_onboarding: true, + yolo: false, + resume_session_id: None, + initial_input: None, + }; + App::new(options, &Config::default()) + } + + fn hover_row(row_y: u16, action: Option<&str>) -> SidebarHoverRow { + SidebarHoverRow { + row_y, + display_text: "row".to_string(), + full_text: "row".to_string(), + detail: None, + is_truncated: false, + click_action: action.map(str::to_string), + } + } + + fn left_click(column: u16, row: u16) -> MouseEvent { + MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column, + row, + modifiers: KeyModifiers::NONE, + } + } + + #[test] + fn sidebar_click_resolves_row_actions_inside_section() { + let mut app = create_test_app(); + app.sidebar_hover.sections.push(SidebarHoverSection { + content_area: Rect::new(60, 4, 20, 6), + lines: vec![ + "header".to_string(), + "job row".to_string(), + "job detail".to_string(), + "agent row".to_string(), + ], + rows: vec![ + hover_row(4, None), + hover_row(5, Some("/jobs show shell_x")), + hover_row(6, Some("/jobs cancel shell_x")), + hover_row(7, Some("/subagents")), + ], + }); + + assert_eq!( + sidebar_click_action(&app, left_click(65, 5)).as_deref(), + Some("/jobs show shell_x"), + "job label row resolves to its show action" + ); + assert_eq!( + sidebar_click_action(&app, left_click(79, 6)).as_deref(), + Some("/jobs cancel shell_x"), + "job detail row resolves to its cancel action" + ); + assert_eq!( + sidebar_click_action(&app, left_click(60, 7)).as_deref(), + Some("/subagents"), + "agent row opens the agents view" + ); + assert_eq!( + sidebar_click_action(&app, left_click(65, 4)), + None, + "header row has no action" + ); + } + + #[test] + fn sidebar_click_outside_section_resolves_to_none() { + let mut app = create_test_app(); + app.sidebar_hover.sections.push(SidebarHoverSection { + content_area: Rect::new(60, 4, 20, 6), + lines: vec!["job row".to_string()], + rows: vec![hover_row(4, Some("/jobs show shell_x"))], + }); + + // Left of the sidebar (transcript area). + assert_eq!(sidebar_click_action(&app, left_click(10, 4)), None); + // Below the section's content area. + assert_eq!(sidebar_click_action(&app, left_click(65, 30)), None); + // Inside the section but on an empty row without metadata. + assert_eq!(sidebar_click_action(&app, left_click(65, 8)), None); + } +} diff --git a/crates/tui/src/tui/sidebar.rs b/crates/tui/src/tui/sidebar.rs index e5f9e860..f8b39249 100644 --- a/crates/tui/src/tui/sidebar.rs +++ b/crates/tui/src/tui/sidebar.rs @@ -742,56 +742,26 @@ fn render_sidebar_work(f: &mut Frame, area: Rect, app: &mut App) { render_sidebar_section(f, area, "Work", lines, full_texts, Vec::new(), app); } -/// Build click actions for each line in the Tasks panel (#3028). -/// Background task rows get `/task show `; stop-target rows get `/task cancel `. -fn task_panel_row_actions(app: &App, lines: &[Line<'static>]) -> Vec> { - let background_rows = background_task_rows(app, &[]); - let mut actions: Vec> = vec![None; lines.len()]; - // The first line is the turn label — no action. - // Subsequent lines alternate: label row then optional detail row. - // Skip the turn label (index 0) and "Live tools"/"Background commands" headers. - let mut bg_idx = 0; - for (i, _line) in lines.iter().enumerate() { - if i == 0 { - continue; // turn label - } - if bg_idx < background_rows.len() { - let task = &background_rows[bg_idx]; - // The detail row (indented) comes right after the label row. - // Label row gets show action; the stop-target [x] concept is - // handled by a separate click on the detail row. - { - let id = &task.id; - // Check if this line index corresponds to a background task label - // (every 2 lines after header rows). For now, assign show to - // the label row and cancel to the detail row. - if i % 2 == 0 && i + 1 < actions.len() { - actions[i] = Some(format!("/task show {id}")); - actions[i + 1] = Some(format!("/task cancel {id}")); - } - } - bg_idx += 1; - } - } - actions -} - -/// Build click actions for the Agents panel rows (#3028). -/// Each agent row gets `/subagents` to open the agent detail view. -fn agent_panel_row_actions(_app: &App, rows: &[SidebarAgentRow]) -> Vec> { - let mut actions: Vec> = Vec::with_capacity(rows.len().max(1)); - // First one or two lines are header (running/done count + role mix). - actions.push(None); // header line 1 - if !rows.is_empty() { - actions.push(None); // header line 2 (role mix) or first agent row - } - for row in rows { - actions.push(Some(format!("/subagents"))); // agent row - if row.status != "done" { - actions.push(None); // detail line (no action) - } - } - actions +/// Click actions for one background job row pair (#3028). +/// +/// Returns `(show, detail)` where `show` opens the job and `detail` cancels +/// it while it is still running (finished jobs make the detail row a second +/// show target instead — cancel would only error). `shell_*` ids belong to +/// the shell job manager and route through `/jobs`; everything else routes +/// through `/task`. +fn background_task_click_actions(task: &TaskPanelEntry) -> (String, String) { + let namespace = if task.id.starts_with("shell_") { + "jobs" + } else { + "task" + }; + let show = format!("/{namespace} show {}", task.id); + let detail = if matches!(task.status.as_str(), "running" | "queued") { + format!("/{namespace} cancel {}", task.id) + } else { + show.clone() + }; + (show, detail) } fn render_sidebar_tasks(f: &mut Frame, area: Rect, app: &mut App) { @@ -801,10 +771,9 @@ fn render_sidebar_tasks(f: &mut Frame, area: Rect, app: &mut App) { let content_width = area.width.saturating_sub(4) as usize; let usable_rows = area.height.saturating_sub(3) as usize; - let lines = task_panel_lines(app, content_width.max(1), usable_rows.max(1)); + let (lines, row_actions) = task_panel_rows(app, content_width.max(1), usable_rows.max(1)); let full_texts = task_panel_hover_texts(app, usable_rows.max(1)); - let row_actions = task_panel_row_actions(app, &lines); render_sidebar_section(f, area, "Tasks", lines, full_texts, row_actions, app); } @@ -816,9 +785,22 @@ struct SidebarToolRow { duration_ms: Option, } +#[cfg(test)] fn task_panel_lines(app: &App, content_width: usize, max_rows: usize) -> Vec> { + task_panel_rows(app, content_width, max_rows).0 +} + +/// Build the Tasks panel lines together with a parallel per-line click-action +/// vector (#3028). Producing both in a single pass keeps the action indices +/// aligned with the rendered lines no matter how the layout evolves. +fn task_panel_rows( + app: &App, + content_width: usize, + max_rows: usize, +) -> (Vec>, Vec>) { let theme = &app.ui_theme; let mut lines: Vec> = Vec::with_capacity(max_rows.max(4)); + let mut actions: Vec> = Vec::with_capacity(max_rows.max(4)); if let Some(turn_id) = app.runtime_turn_id.as_ref() { let status = app @@ -846,6 +828,9 @@ fn task_panel_lines(app: &App, content_width: usize, max_rows: usize) -> Vec Vec Vec Vec Vec Vec Vec { @@ -1818,7 +1811,7 @@ fn render_sidebar_subagents(f: &mut Frame, area: Rect, app: &mut App) { role_counts, }; let rows = sidebar_agent_rows(app); - let lines = subagent_panel_lines( + let (lines, row_actions) = subagent_panel_rows( &summary, &rows, content_width, @@ -1827,7 +1820,6 @@ fn render_sidebar_subagents(f: &mut Frame, area: Rect, app: &mut App) { ); let full_texts = subagent_panel_hover_texts(&summary, &rows, usable_rows.max(1)); - let row_actions = agent_panel_row_actions(app, &rows); render_sidebar_section(f, area, "Agents", lines, full_texts, row_actions, app); } @@ -1939,6 +1931,7 @@ fn subagent_status_text(status: &SubAgentStatus) -> &'static str { /// Build sub-agent sidebar lines from summary + per-agent rows. Public /// for the snapshot tests in this module. +#[cfg(test)] pub fn subagent_panel_lines( summary: &SidebarSubagentSummary, rows: &[SidebarAgentRow], @@ -1946,7 +1939,21 @@ pub fn subagent_panel_lines( max_rows: usize, theme: &palette::UiTheme, ) -> Vec> { + subagent_panel_rows(summary, rows, content_width, max_rows, theme).0 +} + +/// Build the Agents panel lines together with a parallel per-line +/// click-action vector (#3028). Agent label rows open the agents view via +/// `/subagents`; header, role-mix, detail, and RLM lines are not clickable. +fn subagent_panel_rows( + summary: &SidebarSubagentSummary, + rows: &[SidebarAgentRow], + content_width: usize, + max_rows: usize, + theme: &palette::UiTheme, +) -> (Vec>, Vec>) { let mut lines: Vec> = Vec::with_capacity(max_rows.max(4)); + let mut actions: Vec> = Vec::with_capacity(max_rows.max(4)); let fanout_total = summary.fanout_total.unwrap_or(0); if summary.cached_total == 0 @@ -1958,7 +1965,8 @@ pub fn subagent_panel_lines( "No agents", Style::default().fg(theme.text_muted), ))); - return lines; + actions.push(None); + return (lines, actions); } let (live_running, total) = if let Some(total) = summary.fanout_total { @@ -1985,6 +1993,7 @@ pub fn subagent_panel_lines( )] }; lines.push(Line::from(header)); + actions.push(None); if !summary.role_counts.is_empty() { let mix: Vec = summary @@ -1997,6 +2006,7 @@ pub fn subagent_panel_lines( truncate_line_to_width(&role_line, content_width.max(1)), Style::default().fg(theme.text_dim), ))); + actions.push(None); } for row in rows { @@ -2009,6 +2019,7 @@ pub fn subagent_panel_lines( truncate_line_to_width(&label, content_width.max(1)), Style::default().fg(color), ))); + actions.push(Some("/subagents".to_string())); // Auto-collapse finished sub-agents: hide detail lines for completed // agents so the sidebar stays compact when work is done. @@ -2045,6 +2056,7 @@ pub fn subagent_panel_lines( ), Style::default().fg(theme.text_dim), ))); + actions.push(None); } if summary.foreground_rlm_running { @@ -2055,9 +2067,11 @@ pub fn subagent_panel_lines( Style::default().fg(theme.text_dim), ), ])); + actions.push(None); } - lines + debug_assert_eq!(lines.len(), actions.len()); + (lines, actions) } fn subagent_panel_hover_texts( @@ -2413,8 +2427,8 @@ mod tests { SidebarSubagentSummary, SidebarToolRow, SidebarWorkChecklistItem, SidebarWorkStrategyStep, SidebarWorkSummary, ToolRowOrder, auto_sidebar_panels, editorial_tool_rows, normalize_activity_text, sidebar_hover_rows, sidebar_work_summary, - subagent_panel_hover_texts, subagent_panel_lines, task_panel_lines, work_panel_empty_hint, - work_panel_hover_texts, work_panel_lines, + subagent_panel_hover_texts, subagent_panel_lines, subagent_panel_rows, task_panel_lines, + task_panel_rows, work_panel_empty_hint, work_panel_hover_texts, work_panel_lines, }; use crate::config::Config; use crate::palette; @@ -3027,6 +3041,249 @@ mod tests { ); } + #[test] + fn task_panel_actions_make_single_background_job_clickable() { + let mut app = create_test_app(); + app.task_panel.push(TaskPanelEntry { + id: "shell_only".to_string(), + status: "running".to_string(), + prompt_summary: "shell: cargo build".to_string(), + duration_ms: Some(1_000), + }); + + let (lines, actions) = task_panel_rows(&app, 80, 12); + let text = lines_to_text(&lines); + assert_eq!(lines.len(), actions.len()); + + let label_idx = text + .iter() + .position(|line| line.contains("cargo build")) + .expect("background job label row"); + assert_eq!( + actions[label_idx].as_deref(), + Some("/jobs show shell_only"), + "single-job label row must be clickable: {actions:?}" + ); + assert_eq!( + actions[label_idx + 1].as_deref(), + Some("/jobs cancel shell_only"), + "single-job detail row must cancel that job: {actions:?}" + ); + } + + #[test] + fn task_panel_actions_route_each_job_to_its_own_id() { + let mut app = create_test_app(); + app.task_panel.push(TaskPanelEntry { + id: "shell_aaa".to_string(), + status: "running".to_string(), + prompt_summary: "shell: cargo test --workspace".to_string(), + duration_ms: Some(2_000), + }); + app.task_panel.push(TaskPanelEntry { + id: "task_bbb".to_string(), + status: "running".to_string(), + prompt_summary: "summarize the release notes".to_string(), + duration_ms: Some(3_000), + }); + + let (lines, actions) = task_panel_rows(&app, 96, 16); + let text = lines_to_text(&lines); + assert_eq!(lines.len(), actions.len()); + + let header_idx = text + .iter() + .position(|line| line.starts_with("Background commands")) + .expect("background header row"); + assert!(actions[header_idx].is_none(), "header is not clickable"); + + let shell_idx = text + .iter() + .position(|line| line.contains("cargo test --workspace")) + .expect("shell job label row"); + assert_eq!( + actions[shell_idx].as_deref(), + Some("/jobs show shell_aaa"), + "shell jobs route through /jobs: {actions:?}" + ); + assert_eq!( + actions[shell_idx + 1].as_deref(), + Some("/jobs cancel shell_aaa"), + "shell job detail row cancels the SAME job: {actions:?}" + ); + + let task_idx = text + .iter() + .position(|line| line.contains("task_bbb")) + .expect("task job label row"); + assert_eq!( + actions[task_idx].as_deref(), + Some("/task show task_bbb"), + "task-manager jobs route through /task: {actions:?}" + ); + assert_eq!( + actions[task_idx + 1].as_deref(), + Some("/task cancel task_bbb"), + "task job detail row cancels the SAME job: {actions:?}" + ); + + let hint_idx = text + .iter() + .position(|line| line.contains("Ctrl+K")) + .expect("cancel-all hint row"); + assert_eq!(actions[hint_idx].as_deref(), Some("/jobs cancel-all")); + } + + #[test] + fn task_panel_finished_job_detail_row_shows_instead_of_cancels() { + let mut app = create_test_app(); + app.task_panel.push(TaskPanelEntry { + id: "shell_done".to_string(), + status: "completed".to_string(), + prompt_summary: "shell: cargo fmt".to_string(), + duration_ms: Some(500), + }); + + let (lines, actions) = task_panel_rows(&app, 80, 12); + let text = lines_to_text(&lines); + + let label_idx = text + .iter() + .position(|line| line.contains("cargo fmt")) + .expect("completed job label row"); + assert_eq!(actions[label_idx].as_deref(), Some("/jobs show shell_done")); + assert_eq!( + actions[label_idx + 1].as_deref(), + Some("/jobs show shell_done"), + "finished jobs must not expose a cancel click target: {actions:?}" + ); + } + + #[test] + fn task_panel_actions_align_with_lines_when_live_tools_present() { + let mut app = create_test_app(); + app.runtime_turn_id = Some("0196f0a3-aaaa-bbbb-cccc-ddddeeee0000".to_string()); + let mut active = ActiveCell::new(); + active.push_tool( + "shell-1", + HistoryCell::Tool(ToolCell::Exec(ExecCell { + command: "sleep 600".to_string(), + status: ToolStatus::Running, + output: None, + live_output: None, + shell_task_id: None, + started_at: Some(Instant::now()), + duration_ms: None, + source: ExecSource::Assistant, + interaction: None, + output_summary: None, + })), + ); + app.active_cell = Some(active); + app.task_panel.push(TaskPanelEntry { + id: "task_q".to_string(), + status: "running".to_string(), + prompt_summary: "investigate flaky test".to_string(), + duration_ms: Some(9_000), + }); + + let (lines, actions) = task_panel_rows(&app, 96, 16); + let text = lines_to_text(&lines); + assert_eq!( + lines.len(), + actions.len(), + "actions must stay index-aligned with lines: {text:?}" + ); + + // Turn label and live-tool rows are not clickable. + assert!(actions[0].is_none(), "turn label row has no action"); + let live_idx = text + .iter() + .position(|line| line == "Live tools") + .expect("live tools header"); + assert!(actions[live_idx].is_none()); + + let task_idx = text + .iter() + .position(|line| line.contains("task_q")) + .expect("background job label row"); + assert_eq!(actions[task_idx].as_deref(), Some("/task show task_q")); + } + + #[test] + fn subagent_panel_actions_mark_agent_rows_with_role_mix_header() { + let mut role_counts = std::collections::BTreeMap::new(); + role_counts.insert("worker".to_string(), 1); + let summary = SidebarSubagentSummary { + cached_total: 1, + cached_running: 1, + role_counts, + ..SidebarSubagentSummary::default() + }; + let rows = vec![SidebarAgentRow { + id: "agent_0123456789".to_string(), + name: "investigator".to_string(), + role: "worker".to_string(), + status: "running".to_string(), + git_branch: None, + progress: Some("scanning".to_string()), + steps_taken: 2, + duration_ms: Some(1_000), + }]; + + let (lines, actions) = subagent_panel_rows(&summary, &rows, 48, 8, &palette::UI_THEME); + let text = lines_to_text(&lines); + assert_eq!(lines.len(), actions.len()); + + assert!(actions[0].is_none(), "count header has no action"); + assert!(actions[1].is_none(), "role-mix header has no action"); + let agent_idx = text + .iter() + .position(|line| line.contains("investigator")) + .expect("agent label row"); + assert_eq!(actions[agent_idx].as_deref(), Some("/subagents")); + assert!( + actions[agent_idx + 1].is_none(), + "agent detail row has no action" + ); + } + + #[test] + fn subagent_panel_actions_skip_role_mix_slot_for_progress_only_agents() { + // Progress-only agents have no cached role counts, so there is no + // role-mix line — the first agent row sits directly under the count + // header and must still resolve to /subagents (#3028 audit fix). + let summary = SidebarSubagentSummary { + progress_only_count: 1, + ..SidebarSubagentSummary::default() + }; + let rows = vec![SidebarAgentRow { + id: "agent_fedcba987654".to_string(), + name: "scout".to_string(), + role: "explorer".to_string(), + status: "running".to_string(), + git_branch: None, + progress: Some("reading".to_string()), + steps_taken: 1, + duration_ms: None, + }]; + + let (lines, actions) = subagent_panel_rows(&summary, &rows, 48, 8, &palette::UI_THEME); + let text = lines_to_text(&lines); + assert_eq!(lines.len(), actions.len()); + + assert!(actions[0].is_none(), "count header has no action"); + let agent_idx = text + .iter() + .position(|line| line.contains("scout")) + .expect("agent label row"); + assert_eq!( + agent_idx, 1, + "no role-mix line should be emitted without role counts: {text:?}" + ); + assert_eq!(actions[agent_idx].as_deref(), Some("/subagents")); + } + #[test] fn tasks_panel_collapses_repeated_low_value_recent_tools_after_failures() { let mut app = create_test_app(); @@ -3526,7 +3783,7 @@ mod tests { use ratatui::layout::Rect; let display = vec!["[~] agent imple…".to_string()]; let full = vec!["[~] agent implementation-worker-for-sidebar-detail-popover".to_string()]; - let rows = sidebar_hover_rows(Rect::new(62, 5, 16, 4), &display, &full); + let rows = sidebar_hover_rows(Rect::new(62, 5, 16, 4), &display, &full, &[]); let expected = SidebarHoverRow { row_y: 5, @@ -3534,6 +3791,7 @@ mod tests { full_text: full[0].clone(), detail: None, is_truncated: true, + click_action: None, }; assert_eq!(rows, vec![expected]); }