-
Notifications
You must be signed in to change notification settings - Fork 4
feat(tui): display main agent TodoWrite items above input field #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| use std::time::{Duration, Instant}; | ||
|
|
||
| use crate::app::{MainAgentTodoItem, MainAgentTodoStatus}; | ||
| use crate::events::ToolEvent; | ||
| use crate::session::StoredToolCall; | ||
| use crate::views::tool_call::format_result_summary; | ||
|
|
@@ -18,6 +19,11 @@ impl EventLoop { | |
| ) { | ||
| tracing::info!("Spawning tool execution: {} ({})", tool_name, tool_call_id); | ||
|
|
||
| // Handle TodoWrite tool - update main agent todos immediately for real-time display | ||
| if tool_name == "TodoWrite" { | ||
| self.handle_main_agent_todo_write(&args); | ||
| } | ||
|
|
||
| // Get tool registry | ||
| let Some(registry) = self.tool_registry.clone() else { | ||
| tracing::warn!( | ||
|
|
@@ -641,4 +647,78 @@ impl EventLoop { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /// Handle main agent's TodoWrite tool call. | ||
| /// Parses the todos argument and updates app_state for real-time display. | ||
| fn handle_main_agent_todo_write(&mut self, args: &serde_json::Value) { | ||
| // TodoWrite format: { todos: "1. [status] content\n2. [status] content\n..." } | ||
| // OR the newer format: { todos: [{ content, status, ... }] } | ||
|
|
||
| // Try to parse as string format first (numbered list) | ||
| if let Some(todos_str) = args.get("todos").and_then(|v| v.as_str()) { | ||
| let todos: Vec<MainAgentTodoItem> = todos_str | ||
| .lines() | ||
| .filter_map(|line| { | ||
| // Parse lines like "1. [completed] First task" | ||
|
Comment on lines
+657
to
+662
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bracket parsing can drop todos The string-format parser assumes each line contains Prompt To Fix With AIThis is a comment left during a code review.
Path: src/cortex-tui/src/runner/event_loop/tools.rs
Line: 657:662
Comment:
**Bracket parsing can drop todos**
The string-format parser assumes each line contains `[` and `]` and uses `line.find(']')`/`line.find('[')` to slice status/content. If the main agent ever emits a numbered todo line without bracketed status (or with additional `]` earlier in the content), that todo will be silently skipped, potentially resulting in an empty parsed list and (due to the non-empty guard) no state update. This causes the UI to show stale items even though a new TodoWrite was issued. A more robust parse should handle missing/unknown status and still capture the content.
How can I resolve this? If you propose a fix, please make it concise. |
||
| let line = line.trim(); | ||
| if line.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| // Skip the number prefix (e.g., "1. ") | ||
| let content_start = line.find(']').map(|i| i + 1)?; | ||
| let status_start = line.find('[')?; | ||
|
|
||
| let status_str = &line[status_start + 1..content_start - 1]; | ||
| let content = line[content_start..].trim().to_string(); | ||
|
|
||
| if content.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let status = match status_str { | ||
| "in_progress" => MainAgentTodoStatus::InProgress, | ||
| "completed" => MainAgentTodoStatus::Completed, | ||
| _ => MainAgentTodoStatus::Pending, | ||
| }; | ||
|
|
||
| Some(MainAgentTodoItem { content, status }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if !todos.is_empty() { | ||
| tracing::debug!("Main agent todo list updated: {} items", todos.len()); | ||
| self.app_state.update_main_todos(todos); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Try array format (legacy or alternative) | ||
| if let Some(todos_arr) = args.get("todos").and_then(|v| v.as_array()) { | ||
| let todos: Vec<MainAgentTodoItem> = todos_arr | ||
| .iter() | ||
| .filter_map(|t| { | ||
| let content = t.get("content").and_then(|v| v.as_str())?; | ||
| let status_str = t | ||
| .get("status") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("pending"); | ||
| let status = match status_str { | ||
| "in_progress" => MainAgentTodoStatus::InProgress, | ||
| "completed" => MainAgentTodoStatus::Completed, | ||
| _ => MainAgentTodoStatus::Pending, | ||
| }; | ||
| Some(MainAgentTodoItem { | ||
| content: content.to_string(), | ||
| status, | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if !todos.is_empty() { | ||
| tracing::debug!("Main agent todo list updated: {} items", todos.len()); | ||
| self.app_state.update_main_todos(todos); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,9 @@ use crate::widgets::{HintContext, KeyHints, StatusIndicator}; | |
|
|
||
| use super::layout::LayoutManager; | ||
| use super::rendering::{ | ||
| _render_motd, generate_message_lines, generate_welcome_lines, render_message, | ||
| render_scroll_to_bottom_hint, render_scrollbar, render_subagent, render_tool_call, | ||
| _render_motd, generate_message_lines, generate_welcome_lines, render_main_agent_todos, | ||
| render_message, render_scroll_to_bottom_hint, render_scrollbar, render_subagent, | ||
| render_tool_call, | ||
| }; | ||
|
|
||
| // Re-export for convenience | ||
|
|
@@ -572,6 +573,14 @@ impl<'a> Widget for MinimalSessionView<'a> { | |
| let input_height: u16 = 3; | ||
| let hints_height: u16 = 1; | ||
|
|
||
| // Calculate main agent todos height (header + items + spacing) | ||
| let main_todos_height: u16 = if self.app_state.has_main_todos() { | ||
| // 1 for header + number of todos + 1 for spacing | ||
| (self.app_state.main_agent_todos.len() as u16) + 2 | ||
| } else { | ||
| 0 | ||
| }; | ||
|
|
||
| // Calculate welcome card heights from render_motd constants | ||
| let welcome_card_height = 11_u16; | ||
| let info_cards_height = 4_u16; | ||
|
|
@@ -584,7 +593,12 @@ impl<'a> Widget for MinimalSessionView<'a> { | |
| layout.gap(1); | ||
|
|
||
| // Calculate available height for scrollable content (before input/hints) | ||
| let bottom_reserved = status_height + input_height + autocomplete_height + hints_height + 2; // +2 for gaps | ||
| let bottom_reserved = main_todos_height | ||
| + status_height | ||
| + input_height | ||
| + autocomplete_height | ||
| + hints_height | ||
| + 2; // +2 for gaps | ||
| let available_height = area.height.saturating_sub(1 + bottom_reserved); // 1 for top margin | ||
|
|
||
| // Render scrollable content area (welcome cards + messages together) | ||
|
|
@@ -596,7 +610,17 @@ impl<'a> Widget for MinimalSessionView<'a> { | |
| let content_end_y = content_area.y + actual_content_height; | ||
| let mut next_y = content_end_y + 1; // +1 gap after content | ||
|
|
||
| // 5. Status indicator (if task running) - follows content | ||
| // 4.5. Main agent todos (if any) - above status indicator | ||
| if self.app_state.has_main_todos() { | ||
| let todo_lines = | ||
| render_main_agent_todos(&self.app_state.main_agent_todos, area.width, &self.colors); | ||
| let todo_area = Rect::new(area.x, next_y, area.width, main_todos_height); | ||
|
Comment on lines
+613
to
+617
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Todo area can overlap
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/cortex-tui/src/views/minimal_session/view.rs
Line: 613:617
Comment:
**Todo area can overlap**
`todo_area` is rendered unconditionally at `next_y` with height `main_todos_height`, but `next_y` is derived from `content_end_y + 1` where `content_end_y` comes from `actual_content_height`. If `actual_content_height` is larger than the allocated `available_height` (e.g., due to long welcome/messages rendering), `next_y` can already be past the bottom of `area`, and the todo section will render out-of-bounds/overlap subsequent regions. This is particularly easy to hit because adding `main_todos_height` reduces `available_height` but doesn’t constrain `actual_content_height` to that allocation.
How can I resolve this? If you propose a fix, please make it concise. |
||
| let paragraph = Paragraph::new(todo_lines); | ||
| paragraph.render(todo_area, buf); | ||
| next_y += main_todos_height; | ||
| } | ||
|
|
||
| // 5. Status indicator (if task running) - follows todos (or content if no todos) | ||
| if is_task_running { | ||
| let status_area = Rect::new(area.x, next_y, area.width, status_height); | ||
| let header = self.status_header(); | ||
|
|
@@ -608,7 +632,7 @@ impl<'a> Widget for MinimalSessionView<'a> { | |
| next_y += status_height; | ||
| } | ||
|
|
||
| // 6. Input area - follows status (or content if no status) | ||
| // 6. Input area - follows status (or todos/content if no status) | ||
| let input_y = next_y; | ||
| let input_area = Rect::new(area.x, input_y, area.width, input_height); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo list never clears
handle_main_agent_todo_write()only updatesapp_state.main_agent_todoswhen the parsedtodosvector is non-empty (e.g.,if !todos.is_empty() { ... }). This means a valid TodoWrite call that intentionally sends an empty list (or an empty/whitespace string) will not clear the UI, leaving stale todos displayed. Consider explicitly clearing on empty input (e.g., callclear_main_todos()whentodosparses but is empty) so the “empty state: no todo section shown” acceptance criterion is actually met.Prompt To Fix With AI