From 8f79f2377cdd0be78683900e399c5ae8663d6d50 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 13:11:30 -0700 Subject: [PATCH 01/20] Refine TUI hook status rendering --- codex-rs/tui/src/chatwidget.rs | 150 +++++++--- ...cked_failed_feedback_history_snapshot.snap | 9 + ...running_then_quiet_completed_snapshot.snap | 21 ++ ..._hook_runs_while_exec_active_snapshot.snap | 25 ++ ...__overlapping_hook_live_cell_snapshot.snap | 43 +++ ..._tool_use_hook_events_render_snapshot.snap | 6 +- ..._tool_use_hook_events_render_snapshot.snap | 6 +- ...ion_start_hook_events_render_snapshot.snap | 6 +- ...er_hook_notifications_render_snapshot.snap | 7 +- codex-rs/tui/src/chatwidget/tests/helpers.rs | 37 +++ .../src/chatwidget/tests/status_and_layout.rs | 274 ++++++++++++++++++ codex-rs/tui/src/history_cell.rs | 268 +++++++++++++++++ 12 files changed, 799 insertions(+), 53 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_blocked_failed_feedback_history_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_live_running_then_quiet_completed_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_runs_while_exec_active_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__overlapping_hook_live_cell_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c110660549be..c6ff56b89f77 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -330,6 +330,7 @@ use crate::history_cell; #[cfg(test)] use crate::history_cell::AgentMessageCell; use crate::history_cell::HistoryCell; +use crate::history_cell::HookCell; use crate::history_cell::McpToolCallCell; use crate::history_cell::PlainHistoryCell; use crate::history_cell::WebSearchCell; @@ -832,6 +833,8 @@ pub(crate) struct ChatWidget { // Guardian review keeps its own pending set so it can derive a single // footer summary from one or more in-flight review events. pending_guardian_review_status: PendingGuardianReviewStatus, + // Active hook runs render in a dedicated live cell so they can run alongside tools. + active_hook_cell: Option, // Semantic status used for terminal-title status rendering. terminal_title_status_kind: TerminalTitleStatusKind, // Previous status header to restore after a transient stream retry. @@ -2293,6 +2296,7 @@ impl ChatWidget { self.quit_shortcut_key = None; self.update_task_running_state(); self.retry_status_header = None; + self.active_hook_cell = None; self.pending_status_indicator_restore = false; self.bottom_pane .set_interrupt_hint_visible(/*visible*/ true); @@ -3946,36 +3950,92 @@ impl ChatWidget { } fn on_hook_started(&mut self, event: codex_protocol::protocol::HookStartedEvent) { - let label = hook_event_label(event.run.event_name); - let mut message = format!("Running {label} hook"); - if let Some(status_message) = event.run.status_message - && !status_message.is_empty() - { - message.push_str(": "); - message.push_str(&status_message); + self.flush_answer_stream_with_separator(); + match self.active_hook_cell.as_mut() { + Some(cell) => { + cell.start_run(event.run); + self.bump_active_cell_revision(); + } + None => { + self.active_hook_cell = Some(history_cell::new_active_hook_cell( + event.run, + self.config.animations, + )); + self.bump_active_cell_revision(); + } } - self.add_to_history(history_cell::new_info_event(message, /*hint*/ None)); self.request_redraw(); } fn on_hook_completed(&mut self, event: codex_protocol::protocol::HookCompletedEvent) { - let status = format!("{:?}", event.run.status).to_lowercase(); - let header = format!("{} hook ({status})", hook_event_label(event.run.event_name)); - let mut lines: Vec> = vec![header.into()]; - for entry in event.run.entries { - let prefix = match entry.kind { - codex_protocol::protocol::HookOutputEntryKind::Warning => "warning: ", - codex_protocol::protocol::HookOutputEntryKind::Stop => "stop: ", - codex_protocol::protocol::HookOutputEntryKind::Feedback => "feedback: ", - codex_protocol::protocol::HookOutputEntryKind::Context => "hook context: ", - codex_protocol::protocol::HookOutputEntryKind::Error => "error: ", - }; - lines.push(format!(" {prefix}{}", entry.text).into()); + let completed = event.run; + let completed_existing_run = self + .active_hook_cell + .as_mut() + .map(|cell| cell.complete_run(completed.clone())) + .unwrap_or(false); + if completed_existing_run { + self.bump_active_cell_revision(); + } else { + match self.active_hook_cell.as_mut() { + Some(cell) => { + cell.add_completed_run(completed); + self.bump_active_cell_revision(); + } + None => { + let cell = + history_cell::new_completed_hook_cell(completed, self.config.animations); + if !cell.is_empty() { + self.active_hook_cell = Some(cell); + self.bump_active_cell_revision(); + } + } + } } - self.add_to_history(PlainHistoryCell::new(lines)); + self.finish_active_hook_cell_if_idle(); self.request_redraw(); } + fn finish_active_hook_cell_if_idle(&mut self) { + let Some(cell) = self.active_hook_cell.as_ref() else { + return; + }; + if cell.is_empty() { + self.active_hook_cell = None; + self.bump_active_cell_revision(); + return; + } + if cell.should_flush() + && let Some(cell) = self.active_hook_cell.take() + { + self.needs_final_message_separator = true; + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(cell))); + } + } + + fn prune_expired_quiet_hook_runs(&mut self) { + let Some(cell) = self.active_hook_cell.as_mut() else { + return; + }; + if cell.prune_expired_quiet_runs(Instant::now()) { + self.bump_active_cell_revision(); + } + self.finish_active_hook_cell_if_idle(); + } + + fn schedule_quiet_hook_removal_if_needed(&self) { + let Some(deadline) = self + .active_hook_cell + .as_ref() + .and_then(HookCell::next_quiet_removal_deadline) + else { + return; + }; + let delay = deadline.saturating_duration_since(Instant::now()); + self.frame_requester.schedule_frame_in(delay); + } + #[cfg(test)] fn on_undo_started(&mut self, event: UndoStartedEvent) { self.bottom_pane.ensure_status_indicator(); @@ -4023,6 +4083,8 @@ impl ChatWidget { } pub(crate) fn pre_draw_tick(&mut self) { + self.prune_expired_quiet_hook_runs(); + self.schedule_quiet_hook_removal_if_needed(); self.bottom_pane.pre_draw_tick(); if self.should_animate_terminal_title_spinner() { self.refresh_terminal_title(); @@ -4670,6 +4732,7 @@ impl ChatWidget { full_reasoning_buffer: String::new(), current_status: StatusIndicatorState::working(), pending_guardian_review_status: PendingGuardianReviewStatus::default(), + active_hook_cell: None, terminal_title_status_kind: TerminalTitleStatusKind::Working, retry_status_header: None, pending_status_indicator_restore: false, @@ -10810,11 +10873,21 @@ impl ChatWidget { /// providing an appropriate animation tick), the overlay will keep showing a stale tail while /// the main viewport updates. pub(crate) fn active_cell_transcript_key(&self) -> Option { - let cell = self.active_cell.as_ref()?; + let cell = self.active_cell.as_ref(); + let hook_cell = self.active_hook_cell.as_ref(); + if cell.is_none() && hook_cell.is_none() { + return None; + } Some(ActiveCellTranscriptKey { revision: self.active_cell_revision, - is_stream_continuation: cell.is_stream_continuation(), - animation_tick: cell.transcript_animation_tick(), + is_stream_continuation: cell + .map(|cell| cell.is_stream_continuation()) + .unwrap_or(false), + animation_tick: cell + .and_then(|cell| cell.transcript_animation_tick()) + .or_else(|| { + hook_cell.and_then(super::history_cell::HistoryCell::transcript_animation_tick) + }), }) } @@ -10825,8 +10898,16 @@ impl ChatWidget { /// should pass the same width the overlay uses; using a different width will cause wrapping /// mismatches between the main viewport and the transcript overlay. pub(crate) fn active_cell_transcript_lines(&self, width: u16) -> Option>> { - let cell = self.active_cell.as_ref()?; - let lines = cell.transcript_lines(width); + let mut lines = Vec::new(); + if let Some(cell) = self.active_cell.as_ref() { + lines.extend(cell.transcript_lines(width)); + } + if let Some(hook_cell) = self.active_hook_cell.as_ref() { + if !lines.is_empty() { + lines.push("".into()); + } + lines.extend(hook_cell.transcript_lines(width)); + } (!lines.is_empty()).then_some(lines) } @@ -10852,8 +10933,15 @@ impl ChatWidget { )), None => RenderableItem::Owned(Box::new(())), }; + let active_hook_cell_renderable = match &self.active_hook_cell { + Some(cell) => RenderableItem::Borrowed(cell).inset(Insets::tlbr( + /*top*/ 1, /*left*/ 0, /*bottom*/ 0, /*right*/ 0, + )), + None => RenderableItem::Owned(Box::new(())), + }; let mut flex = FlexRenderable::new(); flex.push(/*flex*/ 1, active_cell_renderable); + flex.push(/*flex*/ 0, active_hook_cell_renderable); flex.push( /*flex*/ 0, RenderableItem::Borrowed(&self.bottom_pane).inset(Insets::tlbr( @@ -11082,16 +11170,6 @@ fn extract_first_bold(s: &str) -> Option { None } -fn hook_event_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { - match event_name { - codex_protocol::protocol::HookEventName::PreToolUse => "PreToolUse", - codex_protocol::protocol::HookEventName::PostToolUse => "PostToolUse", - codex_protocol::protocol::HookEventName::SessionStart => "SessionStart", - codex_protocol::protocol::HookEventName::UserPromptSubmit => "UserPromptSubmit", - codex_protocol::protocol::HookEventName::Stop => "Stop", - } -} - #[cfg(test)] pub(crate) fn show_review_commit_picker_with_entries( chat: &mut ChatWidget, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_blocked_failed_feedback_history_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_blocked_failed_feedback_history_snapshot.snap new file mode 100644 index 000000000000..e0d2e7e2252a --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_blocked_failed_feedback_history_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: rendered +--- +• PreToolUse hook (blocked) + feedback: run tests before touching the fixture + +• PostToolUse hook (failed) + error: hook exited with code 7 diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_live_running_then_quiet_completed_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_live_running_then_quiet_completed_snapshot.snap new file mode 100644 index 000000000000..290d093ed870 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_live_running_then_quiet_completed_snapshot.snap @@ -0,0 +1,21 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"{running_snapshot}\\n\\n{completed_lingering_snapshot}\\n\\n{completed_snapshot}\")" +--- +running +live hooks: +• Running PostToolUse hook +history: + + +completed lingering +live hooks: +• Running PostToolUse hook +history: + + +completed after linger +live hooks: + +history: + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_runs_while_exec_active_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_runs_while_exec_active_snapshot.snap new file mode 100644 index 000000000000..2f28b4ad9a0f --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_runs_while_exec_active_snapshot.snap @@ -0,0 +1,25 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"exec running:\\n{exec_running}\\nexec and hook running:\\n{exec_and_hook_running}\\nhistory after exec:\\n{history_after_exec}\\nhook running after exec:\\n{hook_running_after_exec}\\nquiet hook completed lingering:\\n{quiet_hook_completed_lingering}\\nquiet hook completed:\\n{quiet_hook_completed}\")" +--- +exec running: +• Running echo done + +exec and hook running: +active exec: +• Running echo done +active hooks: +• Running PostToolUse hook: checking output policy + +history after exec: +• Ran echo done + └ done + +hook running after exec: +• Running PostToolUse hook: checking output policy + +quiet hook completed lingering: +• Running PostToolUse hook: checking output policy + +quiet hook completed: + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__overlapping_hook_live_cell_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__overlapping_hook_live_cell_snapshot.snap new file mode 100644 index 000000000000..f71909dddf39 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__overlapping_hook_live_cell_snapshot.snap @@ -0,0 +1,43 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"{first_running_snapshot}\\n\\n{second_running_snapshot}\\n\\n{older_completed_snapshot}\\n\\n{older_completed_expired_snapshot}\\n\\n{all_completed_lingering_snapshot}\\n\\n{all_completed_snapshot}\")" +--- +pre running +live hooks: +• Running PreToolUse hook: checking command policy +history: + + +post running +live hooks: +• Running PreToolUse hook: checking command policy + +• Running PostToolUse hook: checking output policy +history: + + +pre completed lingering +live hooks: +• Running PreToolUse hook: checking command policy + +• Running PostToolUse hook: checking output policy +history: + + +pre completed after linger +live hooks: +• Running PostToolUse hook: checking output policy +history: + + +all completed lingering +live hooks: +• Running PostToolUse hook: checking output policy +history: + + +all completed +live hooks: + +history: + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__post_tool_use_hook_events_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__post_tool_use_hook_events_render_snapshot.snap index 722aa89e47c8..b00225f1cd5a 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__post_tool_use_hook_events_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__post_tool_use_hook_events_render_snapshot.snap @@ -1,9 +1,7 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/helpers.rs expression: combined --- -• Running PostToolUse hook: warming the shell - -PostToolUse hook (completed) +• PostToolUse hook (completed) warning: Heads up from the hook hook context: Remember the startup checklist. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap index 11fa8ab8e349..311be2edf5ad 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap @@ -1,9 +1,7 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/helpers.rs expression: combined --- -• Running PreToolUse hook: warming the shell - -PreToolUse hook (completed) +• PreToolUse hook (completed) warning: Heads up from the hook hook context: Remember the startup checklist. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap index 30dfa78e5b03..3d08e82937a9 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap @@ -1,9 +1,7 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/helpers.rs expression: combined --- -• Running SessionStart hook: warming the shell - -SessionStart hook (completed) +• SessionStart hook (completed) warning: Heads up from the hook hook context: Remember the startup checklist. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__user_prompt_submit_app_server_hook_notifications_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__user_prompt_submit_app_server_hook_notifications_render_snapshot.snap index c7e23942304d..43b91941416f 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__user_prompt_submit_app_server_hook_notifications_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__user_prompt_submit_app_server_hook_notifications_render_snapshot.snap @@ -1,10 +1,7 @@ --- -source: tui/src/chatwidget/tests.rs -assertion_line: 12789 +source: tui/src/chatwidget/tests/status_and_layout.rs expression: combined --- -• Running UserPromptSubmit hook: checking go-workflow input policy - -UserPromptSubmit hook (stopped) +• UserPromptSubmit hook (stopped) warning: go-workflow must start from PlanMode stop: prompt blocked diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index d5b30bc042b5..b48fbe495656 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -238,6 +238,7 @@ pub(super) async fn make_chatwidget_manual( reasoning_buffer: String::new(), full_reasoning_buffer: String::new(), current_status: StatusIndicatorState::working(), + active_hook_cell: None, retry_status_header: None, pending_status_indicator_restore: false, suppress_queue_autosend: false, @@ -664,6 +665,21 @@ pub(super) fn active_blob(chat: &ChatWidget) -> String { lines_to_single_string(&lines) } +pub(super) fn active_hook_blob(chat: &ChatWidget) -> String { + let Some(cell) = chat.active_hook_cell.as_ref() else { + return "\n".to_string(); + }; + let lines = cell.display_lines(/*width*/ 80); + lines_to_single_string(&lines) +} + +pub(super) fn expire_quiet_hook_linger(chat: &mut ChatWidget) { + if let Some(cell) = chat.active_hook_cell.as_mut() { + cell.expire_quiet_runs_now_for_test(); + } + chat.pre_draw_tick(); +} + pub(super) fn get_available_model(chat: &ChatWidget, model: &str) -> ModelPreset { let models = chat .model_catalog @@ -977,6 +993,17 @@ pub(super) async fn assert_hook_events_snapshot( }, }), }); + assert!( + drain_insert_history(&mut rx).is_empty(), + "hook start should update the live hook cell instead of writing history" + ); + assert!( + active_hook_blob(&chat).contains(&format!( + "Running {} hook: {status_message}", + hook_event_label(event_name) + )), + "hook start should render in the live hook cell" + ); chat.handle_codex_event(Event { id: "hook-1".into(), @@ -1016,3 +1043,13 @@ pub(super) async fn assert_hook_events_snapshot( .collect::(); assert_chatwidget_snapshot!(snapshot_name, combined); } + +fn hook_event_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { + match event_name { + codex_protocol::protocol::HookEventName::PreToolUse => "PreToolUse", + codex_protocol::protocol::HookEventName::PostToolUse => "PostToolUse", + codex_protocol::protocol::HookEventName::SessionStart => "SessionStart", + codex_protocol::protocol::HookEventName::UserPromptSubmit => "UserPromptSubmit", + codex_protocol::protocol::HookEventName::Stop => "Stop", + } +} diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index b04ce74a1466..092031fc433a 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1389,6 +1389,7 @@ async fn user_prompt_submit_app_server_hook_notifications_render_snapshot() { "user_prompt_submit_app_server_hook_notifications_render_snapshot", combined ); + assert!(!chat.bottom_pane.status_indicator_visible()); } #[tokio::test] @@ -1413,6 +1414,208 @@ async fn post_tool_use_hook_events_render_snapshot() { .await; } +#[tokio::test] +async fn completed_hook_with_no_entries_stays_out_of_history() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(Event { + id: "hook-1".into(), + msg: EventMsg::HookStarted(codex_protocol::protocol::HookStartedEvent { + turn_id: None, + run: codex_protocol::protocol::HookRunSummary { + id: "post-tool-use:0:/tmp/hooks.json".to_string(), + event_name: codex_protocol::protocol::HookEventName::PostToolUse, + handler_type: codex_protocol::protocol::HookHandlerType::Command, + execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, + scope: codex_protocol::protocol::HookScope::Turn, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + status: codex_protocol::protocol::HookRunStatus::Running, + status_message: None, + started_at: 1, + completed_at: None, + duration_ms: None, + entries: Vec::new(), + }, + }), + }); + assert!(drain_insert_history(&mut rx).is_empty()); + let running_snapshot = hook_live_and_history_snapshot(&chat, "running", ""); + + chat.handle_codex_event(Event { + id: "hook-1".into(), + msg: EventMsg::HookCompleted(codex_protocol::protocol::HookCompletedEvent { + turn_id: None, + run: codex_protocol::protocol::HookRunSummary { + id: "post-tool-use:0:/tmp/hooks.json".to_string(), + event_name: codex_protocol::protocol::HookEventName::PostToolUse, + handler_type: codex_protocol::protocol::HookHandlerType::Command, + execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, + scope: codex_protocol::protocol::HookScope::Turn, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + status: codex_protocol::protocol::HookRunStatus::Completed, + status_message: None, + started_at: 1, + completed_at: Some(2), + duration_ms: Some(1), + entries: Vec::new(), + }, + }), + }); + + assert!(drain_insert_history(&mut rx).is_empty()); + let completed_lingering_snapshot = + hook_live_and_history_snapshot(&chat, "completed lingering", ""); + expire_quiet_hook_linger(&mut chat); + let completed_snapshot = hook_live_and_history_snapshot(&chat, "completed after linger", ""); + assert_chatwidget_snapshot!( + "hook_live_running_then_quiet_completed_snapshot", + format!("{running_snapshot}\n\n{completed_lingering_snapshot}\n\n{completed_snapshot}") + ); +} + +#[tokio::test] +async fn blocked_and_failed_hooks_render_feedback_and_errors() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(hook_completed_event( + "pre-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PreToolUse, + codex_protocol::protocol::HookRunStatus::Blocked, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Feedback, + text: "run tests before touching the fixture".to_string(), + }], + )); + chat.handle_codex_event(hook_completed_event( + "post-tool-use:1:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + codex_protocol::protocol::HookRunStatus::Failed, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Error, + text: "hook exited with code 7".to_string(), + }], + )); + + let rendered = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_chatwidget_snapshot!("hook_blocked_failed_feedback_history_snapshot", rendered); + assert!( + rendered.contains( + "PreToolUse hook (blocked)\n feedback: run tests before touching the fixture" + ), + "expected blocked hook feedback: {rendered:?}" + ); + assert!( + rendered.contains("PostToolUse hook (failed)\n error: hook exited with code 7"), + "expected failed hook error: {rendered:?}" + ); +} + +#[tokio::test] +async fn overlapping_hook_live_cell_tracks_parallel_quiet_hooks() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.set_status_header("Thinking".to_string()); + chat.bottom_pane.ensure_status_indicator(); + + chat.handle_codex_event(hook_started_event( + "pre-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PreToolUse, + Some("checking command policy"), + )); + assert_eq!(chat.current_status.header, "Thinking"); + let first_running_snapshot = hook_live_and_history_snapshot(&chat, "pre running", ""); + + chat.handle_codex_event(hook_started_event( + "post-tool-use:1:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + Some("checking output policy"), + )); + assert_eq!(chat.current_status.header, "Thinking"); + let second_running_snapshot = hook_live_and_history_snapshot(&chat, "post running", ""); + + chat.handle_codex_event(hook_completed_event( + "pre-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PreToolUse, + codex_protocol::protocol::HookRunStatus::Completed, + Vec::new(), + )); + assert_eq!(chat.current_status.header, "Thinking"); + let older_completed_snapshot = + hook_live_and_history_snapshot(&chat, "pre completed lingering", ""); + expire_quiet_hook_linger(&mut chat); + let older_completed_expired_snapshot = + hook_live_and_history_snapshot(&chat, "pre completed after linger", ""); + + chat.handle_codex_event(hook_completed_event( + "post-tool-use:1:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + codex_protocol::protocol::HookRunStatus::Completed, + Vec::new(), + )); + assert_eq!(chat.current_status.header, "Thinking"); + assert!(chat.bottom_pane.status_indicator_visible()); + assert!(drain_insert_history(&mut rx).is_empty()); + let all_completed_lingering_snapshot = + hook_live_and_history_snapshot(&chat, "all completed lingering", ""); + expire_quiet_hook_linger(&mut chat); + let all_completed_snapshot = hook_live_and_history_snapshot(&chat, "all completed", ""); + assert_chatwidget_snapshot!( + "overlapping_hook_live_cell_snapshot", + format!( + "{first_running_snapshot}\n\n{second_running_snapshot}\n\n{older_completed_snapshot}\n\n{older_completed_expired_snapshot}\n\n{all_completed_lingering_snapshot}\n\n{all_completed_snapshot}" + ) + ); +} + +#[tokio::test] +async fn running_hook_does_not_displace_active_exec_cell() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + let begin = begin_exec(&mut chat, "call-1", "echo done"); + let exec_running = active_blob(&chat); + + chat.handle_codex_event(hook_started_event( + "post-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + Some("checking output policy"), + )); + let exec_and_hook_running = format!( + "active exec:\n{}active hooks:\n{}", + active_blob(&chat), + active_hook_blob(&chat) + ); + + end_exec(&mut chat, begin, "done", "", /*exit_code*/ 0); + let history_after_exec = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + let hook_running_after_exec = active_hook_blob(&chat); + + chat.handle_codex_event(hook_completed_event( + "post-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + codex_protocol::protocol::HookRunStatus::Completed, + Vec::new(), + )); + assert!(drain_insert_history(&mut rx).is_empty()); + let quiet_hook_completed_lingering = active_hook_blob(&chat); + expire_quiet_hook_linger(&mut chat); + let quiet_hook_completed = active_hook_blob(&chat); + + assert_chatwidget_snapshot!( + "hook_runs_while_exec_active_snapshot", + format!( + "exec running:\n{exec_running}\nexec and hook running:\n{exec_and_hook_running}\nhistory after exec:\n{history_after_exec}\nhook running after exec:\n{hook_running_after_exec}\nquiet hook completed lingering:\n{quiet_hook_completed_lingering}\nquiet hook completed:\n{quiet_hook_completed}" + ) + ); +} + #[tokio::test] async fn session_start_hook_events_render_snapshot() { assert_hook_events_snapshot( @@ -1424,6 +1627,77 @@ async fn session_start_hook_events_render_snapshot() { .await; } +fn hook_started_event( + id: &str, + event_name: codex_protocol::protocol::HookEventName, + status_message: Option<&str>, +) -> Event { + Event { + id: id.to_string(), + msg: EventMsg::HookStarted(codex_protocol::protocol::HookStartedEvent { + turn_id: None, + run: hook_run_summary( + id, + event_name, + codex_protocol::protocol::HookRunStatus::Running, + status_message, + Vec::new(), + ), + }), + } +} + +fn hook_completed_event( + id: &str, + event_name: codex_protocol::protocol::HookEventName, + status: codex_protocol::protocol::HookRunStatus, + entries: Vec, +) -> Event { + Event { + id: id.to_string(), + msg: EventMsg::HookCompleted(codex_protocol::protocol::HookCompletedEvent { + turn_id: None, + run: hook_run_summary(id, event_name, status, None, entries), + }), + } +} + +fn hook_run_summary( + id: &str, + event_name: codex_protocol::protocol::HookEventName, + status: codex_protocol::protocol::HookRunStatus, + status_message: Option<&str>, + entries: Vec, +) -> codex_protocol::protocol::HookRunSummary { + codex_protocol::protocol::HookRunSummary { + id: id.to_string(), + event_name, + handler_type: codex_protocol::protocol::HookHandlerType::Command, + execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, + scope: codex_protocol::protocol::HookScope::Turn, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + status, + status_message: status_message.map(str::to_string), + started_at: 1, + completed_at: (status != codex_protocol::protocol::HookRunStatus::Running).then_some(2), + duration_ms: (status != codex_protocol::protocol::HookRunStatus::Running).then_some(1), + entries, + } +} + +fn hook_live_and_history_snapshot(chat: &ChatWidget, phase: &str, history: &str) -> String { + let history = if history.is_empty() { + "" + } else { + history + }; + format!( + "{phase}\nlive hooks:\n{}history:\n{history}", + active_hook_blob(chat), + ) +} + // Combined visual snapshot using vt100 for history + direct buffer overlay for UI. // This renders the final visual as seen in a terminal: history above, then a blank line, // then the exec block, another blank line, the status line, a blank line, and the composer. diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 67c7e9f98b57..486e8fc8b2ec 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -64,6 +64,11 @@ use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::FileChange; +use codex_protocol::protocol::HookEventName; +use codex_protocol::protocol::HookOutputEntry; +use codex_protocol::protocol::HookOutputEntryKind; +use codex_protocol::protocol::HookRunStatus; +use codex_protocol::protocol::HookRunSummary; use codex_protocol::protocol::McpAuthStatus; use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::SessionConfiguredEvent; @@ -1754,6 +1759,269 @@ pub(crate) fn new_warning_event(message: String) -> PrefixedWrappedHistoryCell { PrefixedWrappedHistoryCell::new(message.yellow(), "⚠ ".yellow(), " ") } +#[derive(Debug)] +pub(crate) struct HookCell { + runs: Vec, + animations_enabled: bool, +} + +const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(300); + +#[derive(Debug)] +struct HookRunCell { + id: String, + event_name: HookEventName, + status_message: Option, + start_time: Option, + completed: Option, + quiet_removal_deadline: Option, +} + +#[derive(Debug)] +struct CompletedHookRun { + status: HookRunStatus, + entries: Vec, +} + +impl HookCell { + pub(crate) fn new(run: HookRunSummary, animations_enabled: bool) -> Self { + let mut cell = Self { + runs: Vec::new(), + animations_enabled, + }; + cell.start_run(run); + cell + } + + pub(crate) fn is_empty(&self) -> bool { + self.runs.is_empty() + } + + pub(crate) fn is_active(&self) -> bool { + self.runs.iter().any(|run| run.completed.is_none()) + } + + pub(crate) fn should_flush(&self) -> bool { + !self.is_active() && !self.is_empty() + } + + pub(crate) fn start_run(&mut self, run: HookRunSummary) { + if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { + existing.event_name = run.event_name; + existing.status_message = run.status_message; + existing.start_time = Some(Instant::now()); + existing.completed = None; + existing.quiet_removal_deadline = None; + return; + } + self.runs.push(HookRunCell { + id: run.id, + event_name: run.event_name, + status_message: run.status_message, + start_time: Some(Instant::now()), + completed: None, + quiet_removal_deadline: None, + }); + } + + /// Completes a run and returns whether the run was already present in this cell. + pub(crate) fn complete_run(&mut self, run: HookRunSummary) -> bool { + let Some(index) = self.runs.iter().position(|existing| existing.id == run.id) else { + return false; + }; + if hook_run_is_quiet_success(&run) { + if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_complete() { + self.runs[index].quiet_removal_deadline = Some(deadline); + } else { + self.runs.remove(index); + } + return true; + } + let existing = &mut self.runs[index]; + existing.event_name = run.event_name; + existing.status_message = run.status_message; + existing.start_time = None; + existing.completed = Some(CompletedHookRun { + status: run.status, + entries: run.entries, + }); + existing.quiet_removal_deadline = None; + true + } + + pub(crate) fn add_completed_run(&mut self, run: HookRunSummary) { + if hook_run_is_quiet_success(&run) { + return; + } + self.runs.push(HookRunCell { + id: run.id, + event_name: run.event_name, + status_message: run.status_message, + start_time: None, + completed: Some(CompletedHookRun { + status: run.status, + entries: run.entries, + }), + quiet_removal_deadline: None, + }); + } + + pub(crate) fn prune_expired_quiet_runs(&mut self, now: Instant) -> bool { + let old_len = self.runs.len(); + self.runs.retain(|run| { + run.quiet_removal_deadline + .map(|deadline| now < deadline) + .unwrap_or(true) + }); + self.runs.len() != old_len + } + + pub(crate) fn next_quiet_removal_deadline(&self) -> Option { + self.runs + .iter() + .filter_map(|run| run.quiet_removal_deadline) + .min() + } + + #[cfg(test)] + pub(crate) fn expire_quiet_runs_now_for_test(&mut self) { + for run in &mut self.runs { + if run.quiet_removal_deadline.is_some() { + run.quiet_removal_deadline = Some(Instant::now()); + } + } + } + + fn display_lines_inner(&self) -> Vec> { + let mut lines = Vec::new(); + for run in &self.runs { + if !lines.is_empty() { + lines.push("".into()); + } + run.push_display_lines(&mut lines, self.animations_enabled); + } + lines + } +} + +impl HistoryCell for HookCell { + fn display_lines(&self, _width: u16) -> Vec> { + self.display_lines_inner() + } + + fn transcript_lines(&self, width: u16) -> Vec> { + self.display_lines(width) + } + + fn transcript_animation_tick(&self) -> Option { + let elapsed = self + .runs + .iter() + .find_map(|run| run.completed.is_none().then_some(run.start_time).flatten())? + .elapsed(); + Some(elapsed.as_millis() as u64 / 600) + } +} + +impl Renderable for HookCell { + fn render(&self, area: Rect, buf: &mut Buffer) { + let lines = self.display_lines(area.width); + let paragraph = Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }); + paragraph.render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + HistoryCell::desired_height(self, width) + } +} + +impl HookRunCell { + fn quiet_removal_deadline_after_complete(&self) -> Option { + let start_time = self.start_time?; + let minimum_deadline = start_time + QUIET_HOOK_MIN_VISIBLE; + (Instant::now() < minimum_deadline).then_some(minimum_deadline) + } + + fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { + let label = hook_event_label(self.event_name); + match &self.completed { + Some(completed) => { + let status = format!("{:?}", completed.status).to_lowercase(); + let bullet = match completed.status { + HookRunStatus::Completed => "•".green().bold(), + HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => { + "•".red().bold() + } + HookRunStatus::Running => "•".into(), + }; + lines.push( + vec![ + bullet, + " ".into(), + format!("{label} hook ({status})").into(), + ] + .into(), + ); + for entry in &completed.entries { + lines + .push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); + } + } + None => { + let mut header = vec![ + spinner(self.start_time, animations_enabled), + " ".into(), + format!("Running {label} hook").bold(), + ]; + if let Some(status_message) = &self.status_message + && !status_message.is_empty() + { + header.push(": ".into()); + header.push(status_message.clone().dim()); + } + lines.push(header.into()); + } + } + } +} + +pub(crate) fn new_active_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { + HookCell::new(run, animations_enabled) +} + +pub(crate) fn new_completed_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { + let mut cell = HookCell { + runs: Vec::new(), + animations_enabled, + }; + cell.add_completed_run(run); + cell +} + +fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { + run.status == HookRunStatus::Completed && run.entries.is_empty() +} + +fn hook_output_prefix(kind: HookOutputEntryKind) -> &'static str { + match kind { + HookOutputEntryKind::Warning => "warning: ", + HookOutputEntryKind::Stop => "stop: ", + HookOutputEntryKind::Feedback => "feedback: ", + HookOutputEntryKind::Context => "hook context: ", + HookOutputEntryKind::Error => "error: ", + } +} + +fn hook_event_label(event_name: HookEventName) -> &'static str { + match event_name { + HookEventName::PreToolUse => "PreToolUse", + HookEventName::PostToolUse => "PostToolUse", + HookEventName::SessionStart => "SessionStart", + HookEventName::UserPromptSubmit => "UserPromptSubmit", + HookEventName::Stop => "Stop", + } +} + #[derive(Debug)] pub(crate) struct DeprecationNoticeCell { summary: String, From 0089d6640c4afc4136c31fd4701b25710eb398b2 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 13:51:08 -0700 Subject: [PATCH 02/20] Delay revealing running hook rows --- codex-rs/tui/src/chatwidget.rs | 9 +-- ...pleted_without_running_flash_snapshot.snap | 9 +++ codex-rs/tui/src/chatwidget/tests/helpers.rs | 8 +++ .../src/chatwidget/tests/status_and_layout.rs | 35 +++++++++++ codex-rs/tui/src/history_cell.rs | 59 ++++++++++++++++--- 5 files changed, 109 insertions(+), 11 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_completed_before_reveal_renders_completed_without_running_flash_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c6ff56b89f77..a7317e2ca799 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -4018,17 +4018,18 @@ impl ChatWidget { let Some(cell) = self.active_hook_cell.as_mut() else { return; }; - if cell.prune_expired_quiet_runs(Instant::now()) { + let now = Instant::now(); + if cell.update_due_running_visibility(now) || cell.prune_expired_quiet_runs(now) { self.bump_active_cell_revision(); } self.finish_active_hook_cell_if_idle(); } - fn schedule_quiet_hook_removal_if_needed(&self) { + fn schedule_hook_timer_if_needed(&self) { let Some(deadline) = self .active_hook_cell .as_ref() - .and_then(HookCell::next_quiet_removal_deadline) + .and_then(HookCell::next_timer_deadline) else { return; }; @@ -4084,7 +4085,7 @@ impl ChatWidget { pub(crate) fn pre_draw_tick(&mut self) { self.prune_expired_quiet_hook_runs(); - self.schedule_quiet_hook_removal_if_needed(); + self.schedule_hook_timer_if_needed(); self.bottom_pane.pre_draw_tick(); if self.should_animate_terminal_title_spinner() { self.refresh_terminal_title(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_completed_before_reveal_renders_completed_without_running_flash_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_completed_before_reveal_renders_completed_without_running_flash_snapshot.snap new file mode 100644 index 000000000000..b38d4e260af7 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_completed_before_reveal_renders_completed_without_running_flash_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"started hidden:\\n{started_hidden_snapshot}\\nhistory:\\n{history}\")" +--- +started hidden: + +history: +• SessionStart hook (completed) + hook context: session context diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index b48fbe495656..94c267946e94 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -680,6 +680,13 @@ pub(super) fn expire_quiet_hook_linger(chat: &mut ChatWidget) { chat.pre_draw_tick(); } +pub(super) fn reveal_running_hooks(chat: &mut ChatWidget) { + if let Some(cell) = chat.active_hook_cell.as_mut() { + cell.reveal_running_runs_now_for_test(); + } + chat.pre_draw_tick(); +} + pub(super) fn get_available_model(chat: &ChatWidget, model: &str) -> ModelPreset { let models = chat .model_catalog @@ -997,6 +1004,7 @@ pub(super) async fn assert_hook_events_snapshot( drain_insert_history(&mut rx).is_empty(), "hook start should update the live hook cell instead of writing history" ); + reveal_running_hooks(&mut chat); assert!( active_hook_blob(&chat).contains(&format!( "Running {} hook: {status_message}", diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 092031fc433a..e0124f556646 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1440,6 +1440,7 @@ async fn completed_hook_with_no_entries_stays_out_of_history() { }), }); assert!(drain_insert_history(&mut rx).is_empty()); + reveal_running_hooks(&mut chat); let running_snapshot = hook_live_and_history_snapshot(&chat, "running", ""); chat.handle_codex_event(Event { @@ -1528,6 +1529,7 @@ async fn overlapping_hook_live_cell_tracks_parallel_quiet_hooks() { Some("checking command policy"), )); assert_eq!(chat.current_status.header, "Thinking"); + reveal_running_hooks(&mut chat); let first_running_snapshot = hook_live_and_history_snapshot(&chat, "pre running", ""); chat.handle_codex_event(hook_started_event( @@ -1536,6 +1538,7 @@ async fn overlapping_hook_live_cell_tracks_parallel_quiet_hooks() { Some("checking output policy"), )); assert_eq!(chat.current_status.header, "Thinking"); + reveal_running_hooks(&mut chat); let second_running_snapshot = hook_live_and_history_snapshot(&chat, "post running", ""); chat.handle_codex_event(hook_completed_event( @@ -1584,6 +1587,7 @@ async fn running_hook_does_not_displace_active_exec_cell() { codex_protocol::protocol::HookEventName::PostToolUse, Some("checking output policy"), )); + reveal_running_hooks(&mut chat); let exec_and_hook_running = format!( "active exec:\n{}active hooks:\n{}", active_blob(&chat), @@ -1616,6 +1620,37 @@ async fn running_hook_does_not_displace_active_exec_cell() { ); } +#[tokio::test] +async fn hook_completed_before_reveal_renders_completed_without_running_flash() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(hook_started_event( + "session-start:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::SessionStart, + Some("warming the shell"), + )); + let started_hidden_snapshot = active_hook_blob(&chat); + + chat.handle_codex_event(hook_completed_event( + "session-start:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::SessionStart, + codex_protocol::protocol::HookRunStatus::Completed, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Context, + text: "session context".to_string(), + }], + )); + + let history = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_chatwidget_snapshot!( + "hook_completed_before_reveal_renders_completed_without_running_flash_snapshot", + format!("started hidden:\n{started_hidden_snapshot}\nhistory:\n{history}") + ); +} + #[tokio::test] async fn session_start_hook_events_render_snapshot() { assert_hook_events_snapshot( diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 486e8fc8b2ec..d3ce4c33723d 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1765,7 +1765,8 @@ pub(crate) struct HookCell { animations_enabled: bool, } -const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(300); +const HOOK_RUN_REVEAL_DELAY: Duration = Duration::from_millis(300); +const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(1000); #[derive(Debug)] struct HookRunCell { @@ -1773,6 +1774,8 @@ struct HookRunCell { event_name: HookEventName, status_message: Option, start_time: Option, + reveal_deadline: Instant, + running_visible: bool, completed: Option, quiet_removal_deadline: Option, } @@ -1809,16 +1812,22 @@ impl HookCell { if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { existing.event_name = run.event_name; existing.status_message = run.status_message; - existing.start_time = Some(Instant::now()); + let now = Instant::now(); + existing.start_time = Some(now); + existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; + existing.running_visible = false; existing.completed = None; existing.quiet_removal_deadline = None; return; } + let now = Instant::now(); self.runs.push(HookRunCell { id: run.id, event_name: run.event_name, status_message: run.status_message, - start_time: Some(Instant::now()), + start_time: Some(now), + reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, + running_visible: false, completed: None, quiet_removal_deadline: None, }); @@ -1858,6 +1867,8 @@ impl HookCell { event_name: run.event_name, status_message: run.status_message, start_time: None, + reveal_deadline: Instant::now(), + running_visible: false, completed: Some(CompletedHookRun { status: run.status, entries: run.entries, @@ -1876,10 +1887,25 @@ impl HookCell { self.runs.len() != old_len } - pub(crate) fn next_quiet_removal_deadline(&self) -> Option { + pub(crate) fn update_due_running_visibility(&mut self, now: Instant) -> bool { + let mut changed = false; + for run in &mut self.runs { + if run.completed.is_none() && !run.running_visible && now >= run.reveal_deadline { + run.running_visible = true; + changed = true; + } + } + changed + } + + pub(crate) fn next_timer_deadline(&self) -> Option { self.runs .iter() - .filter_map(|run| run.quiet_removal_deadline) + .filter_map(|run| { + run.quiet_removal_deadline.or_else(|| { + (run.completed.is_none() && !run.running_visible).then_some(run.reveal_deadline) + }) + }) .min() } @@ -1892,9 +1918,22 @@ impl HookCell { } } + #[cfg(test)] + pub(crate) fn reveal_running_runs_now_for_test(&mut self) { + let now = Instant::now(); + for run in &mut self.runs { + if run.completed.is_none() { + run.reveal_deadline = now; + } + } + } + fn display_lines_inner(&self) -> Vec> { let mut lines = Vec::new(); for run in &self.runs { + if !run.should_render() { + continue; + } if !lines.is_empty() { lines.push("".into()); } @@ -1937,11 +1976,17 @@ impl Renderable for HookCell { impl HookRunCell { fn quiet_removal_deadline_after_complete(&self) -> Option { - let start_time = self.start_time?; - let minimum_deadline = start_time + QUIET_HOOK_MIN_VISIBLE; + if !self.running_visible { + return None; + } + let minimum_deadline = self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE; (Instant::now() < minimum_deadline).then_some(minimum_deadline) } + fn should_render(&self) -> bool { + self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() + } + fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { let label = hook_event_label(self.event_name); match &self.completed { From 54ea2bfe2da8811e5a1223fd8b57067dec37883b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 14:05:12 -0700 Subject: [PATCH 03/20] Scope tool-use hook run ids to tool calls --- codex-rs/hooks/src/events/post_tool_use.rs | 59 ++++++++++++++++++++-- codex-rs/hooks/src/events/pre_tool_use.rs | 57 +++++++++++++++++++-- 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 3af9bef5e562..96353a36001b 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -59,7 +59,7 @@ pub(crate) fn preview( Some(&request.tool_name), ) .into_iter() - .map(|handler| dispatcher::running_summary(&handler)) + .map(|handler| hook_run_for_tool_use(dispatcher::running_summary(&handler), request)) .collect() } @@ -113,7 +113,7 @@ pub(crate) async fn run( matched, input_json, request.cwd.as_path(), - Some(request.turn_id), + Some(request.turn_id.clone()), parse_completed, ) .await; @@ -135,7 +135,10 @@ pub(crate) async fn run( ); PostToolUseOutcome { - hook_events: results.into_iter().map(|result| result.completed).collect(), + hook_events: results + .into_iter() + .map(|result| hook_completed_for_tool_use(result.completed, &request)) + .collect(), should_stop, stop_reason, additional_contexts, @@ -143,6 +146,19 @@ pub(crate) async fn run( } } +fn hook_completed_for_tool_use( + mut event: HookCompletedEvent, + request: &PostToolUseRequest, +) -> HookCompletedEvent { + event.run = hook_run_for_tool_use(event.run, request); + event +} + +fn hook_run_for_tool_use(mut run: HookRunSummary, request: &PostToolUseRequest) -> HookRunSummary { + run.id = format!("{}:{}", run.id, request.tool_use_id); + run +} + fn parse_completed( handler: &ConfiguredHandler, run_result: CommandRunResult, @@ -295,14 +311,18 @@ fn serialization_failure_outcome(hook_events: Vec) -> PostTo mod tests { use std::path::PathBuf; + use codex_protocol::ThreadId; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; use pretty_assertions::assert_eq; + use serde_json::json; use super::PostToolUseHandlerData; + use super::hook_completed_for_tool_use; use super::parse_completed; + use super::preview; use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; @@ -463,6 +483,24 @@ mod tests { assert_eq!(parsed.completed.run.entries, Vec::::new()); } + #[test] + fn preview_and_completed_run_ids_include_tool_use_id() { + let request = request_for_tool_use("tool-call-456"); + let runs = preview(&[handler()], &request); + + assert_eq!(runs.len(), 1); + assert_eq!(runs[0].id, "post-tool-use:0:/tmp/hooks.json:tool-call-456"); + + let parsed = parse_completed( + &handler(), + run_result(Some(0), "", ""), + Some("turn-1".to_string()), + ); + let completed = hook_completed_for_tool_use(parsed.completed, &request); + + assert_eq!(completed.run.id, runs[0].id); + } + fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PostToolUse, @@ -486,4 +524,19 @@ mod tests { error: None, } } + + fn request_for_tool_use(tool_use_id: &str) -> super::PostToolUseRequest { + super::PostToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: tool_use_id.to_string(), + command: "echo hello".to_string(), + tool_response: json!({"ok": true}), + } + } } diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 8366bb632cb0..f6cc31187726 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -52,7 +52,7 @@ pub(crate) fn preview( Some(&request.tool_name), ) .into_iter() - .map(|handler| dispatcher::running_summary(&handler)) + .map(|handler| hook_run_for_tool_use(dispatcher::running_summary(&handler), request)) .collect() } @@ -103,7 +103,7 @@ pub(crate) async fn run( matched, input_json, request.cwd.as_path(), - Some(request.turn_id), + Some(request.turn_id.clone()), parse_completed, ) .await; @@ -114,12 +114,28 @@ pub(crate) async fn run( .find_map(|result| result.data.block_reason.clone()); PreToolUseOutcome { - hook_events: results.into_iter().map(|result| result.completed).collect(), + hook_events: results + .into_iter() + .map(|result| hook_completed_for_tool_use(result.completed, &request)) + .collect(), should_block, block_reason, } } +fn hook_completed_for_tool_use( + mut event: HookCompletedEvent, + request: &PreToolUseRequest, +) -> HookCompletedEvent { + event.run = hook_run_for_tool_use(event.run, request); + event +} + +fn hook_run_for_tool_use(mut run: HookRunSummary, request: &PreToolUseRequest) -> HookRunSummary { + run.id = format!("{}:{}", run.id, request.tool_use_id); + run +} + fn parse_completed( handler: &ConfiguredHandler, run_result: CommandRunResult, @@ -232,6 +248,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo mod tests { use std::path::PathBuf; + use codex_protocol::ThreadId; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; @@ -239,7 +256,9 @@ mod tests { use pretty_assertions::assert_eq; use super::PreToolUseHandlerData; + use super::hook_completed_for_tool_use; use super::parse_completed; + use super::preview; use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; @@ -453,6 +472,24 @@ mod tests { ); } + #[test] + fn preview_and_completed_run_ids_include_tool_use_id() { + let request = request_for_tool_use("tool-call-123"); + let runs = preview(&[handler()], &request); + + assert_eq!(runs.len(), 1); + assert_eq!(runs[0].id, "pre-tool-use:0:/tmp/hooks.json:tool-call-123"); + + let parsed = parse_completed( + &handler(), + run_result(Some(0), "", ""), + Some("turn-1".to_string()), + ); + let completed = hook_completed_for_tool_use(parsed.completed, &request); + + assert_eq!(completed.run.id, runs[0].id); + } + fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PreToolUse, @@ -476,4 +513,18 @@ mod tests { error: None, } } + + fn request_for_tool_use(tool_use_id: &str) -> super::PreToolUseRequest { + super::PreToolUseRequest { + session_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + cwd: PathBuf::from("/tmp"), + transcript_path: None, + model: "gpt-test".to_string(), + permission_mode: "default".to_string(), + tool_name: "Bash".to_string(), + tool_use_id: tool_use_id.to_string(), + command: "echo hello".to_string(), + } + } } From afdc1266a0aa1e0d761d49821cb13b8aeb371e8d Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 14:07:44 -0700 Subject: [PATCH 04/20] Animate running hook status rows --- codex-rs/tui/src/chatwidget.rs | 10 ++++++++++ codex-rs/tui/src/history_cell.rs | 19 ++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index a7317e2ca799..b5c77ad3f35c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -4026,6 +4026,16 @@ impl ChatWidget { } fn schedule_hook_timer_if_needed(&self) { + if self.config.animations + && self + .active_hook_cell + .as_ref() + .is_some_and(HookCell::has_visible_running_run) + { + self.frame_requester + .schedule_frame_in(Duration::from_millis(50)); + } + let Some(deadline) = self .active_hook_cell .as_ref() diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index d3ce4c33723d..3101bd1db4a0 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -25,6 +25,7 @@ use crate::render::line_utils::line_to_static; use crate::render::line_utils::prefix_lines; use crate::render::line_utils::push_owned_lines; use crate::render::renderable::Renderable; +use crate::shimmer::shimmer_spans; use crate::style::proposed_plan_style; use crate::style::user_message_style; #[cfg(test)] @@ -1808,6 +1809,12 @@ impl HookCell { !self.is_active() && !self.is_empty() } + pub(crate) fn has_visible_running_run(&self) -> bool { + self.runs + .iter() + .any(|run| run.completed.is_none() && run.running_visible) + } + pub(crate) fn start_run(&mut self, run: HookRunSummary) { if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { existing.event_name = run.event_name; @@ -2013,11 +2020,13 @@ impl HookRunCell { } } None => { - let mut header = vec![ - spinner(self.start_time, animations_enabled), - " ".into(), - format!("Running {label} hook").bold(), - ]; + let header_text = format!("Running {label} hook"); + let mut header = vec![spinner(self.start_time, animations_enabled), " ".into()]; + if animations_enabled { + header.extend(shimmer_spans(&header_text)); + } else { + header.push(header_text.bold()); + } if let Some(status_message) = &self.status_message && !status_message.is_empty() { From a2e31864db475ca8d4f98a8c52d1f8a9073f9a18 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 14:57:52 -0700 Subject: [PATCH 05/20] Keep completed hooks running for visible floor --- codex-rs/tui/src/chatwidget.rs | 2 +- ...ps_running_visible_for_floor_snapshot.snap | 22 +++ codex-rs/tui/src/chatwidget/tests/helpers.rs | 8 + .../src/chatwidget/tests/status_and_layout.rs | 44 ++++++ codex-rs/tui/src/history_cell.rs | 142 ++++++++++++------ 5 files changed, 173 insertions(+), 45 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index b5c77ad3f35c..ff00b8c7dd2a 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -4019,7 +4019,7 @@ impl ChatWidget { return; }; let now = Instant::now(); - if cell.update_due_running_visibility(now) || cell.prune_expired_quiet_runs(now) { + if cell.update_due_visibility(now) || cell.prune_expired_quiet_runs(now) { self.bump_active_cell_revision(); } self.finish_active_hook_cell_if_idle(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap new file mode 100644 index 000000000000..4eed0417cd1d --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap @@ -0,0 +1,22 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"{running_snapshot}\\n\\n{completed_deferred_snapshot}\\n\\n{completed_revealed_snapshot}\")" +--- +running +live hooks: +• Running PreToolUse hook: checking command +history: + + +completed deferred +live hooks: +• Running PreToolUse hook +history: + + +completed revealed +live hooks: + +history: +• PreToolUse hook (blocked) + feedback: command blocked by policy diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index 94c267946e94..39a3f9cb3d71 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -687,6 +687,13 @@ pub(super) fn reveal_running_hooks(chat: &mut ChatWidget) { chat.pre_draw_tick(); } +pub(super) fn reveal_completed_hooks(chat: &mut ChatWidget) { + if let Some(cell) = chat.active_hook_cell.as_mut() { + cell.reveal_completed_runs_now_for_test(); + } + chat.pre_draw_tick(); +} + pub(super) fn get_available_model(chat: &ChatWidget, model: &str) -> ModelPreset { let models = chat .model_catalog @@ -1044,6 +1051,7 @@ pub(super) async fn assert_hook_events_snapshot( }), }); + reveal_completed_hooks(&mut chat); let cells = drain_insert_history(&mut rx); let combined = cells .iter() diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index e0124f556646..b6a57ee0bb32 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1516,6 +1516,50 @@ async fn blocked_and_failed_hooks_render_feedback_and_errors() { ); } +#[tokio::test] +async fn completed_hook_with_output_keeps_running_visible_for_floor() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(hook_started_event( + "pre-tool-use:0:/tmp/hooks.json:tool-call-1", + codex_protocol::protocol::HookEventName::PreToolUse, + Some("checking command"), + )); + reveal_running_hooks(&mut chat); + let running_snapshot = hook_live_and_history_snapshot(&chat, "running", ""); + + chat.handle_codex_event(hook_completed_event( + "pre-tool-use:0:/tmp/hooks.json:tool-call-1", + codex_protocol::protocol::HookEventName::PreToolUse, + codex_protocol::protocol::HookRunStatus::Blocked, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Feedback, + text: "command blocked by policy".to_string(), + }], + )); + let completed_deferred_snapshot = + hook_live_and_history_snapshot(&chat, "completed deferred", ""); + assert!( + drain_insert_history(&mut rx).is_empty(), + "completed hook should stay live until the running row has met its visible floor" + ); + + reveal_completed_hooks(&mut chat); + let history = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + let completed_revealed_snapshot = + hook_live_and_history_snapshot(&chat, "completed revealed", &history); + + assert_chatwidget_snapshot!( + "completed_hook_with_output_keeps_running_visible_for_floor_snapshot", + format!( + "{running_snapshot}\n\n{completed_deferred_snapshot}\n\n{completed_revealed_snapshot}" + ) + ); +} + #[tokio::test] async fn overlapping_hook_live_cell_tracks_parallel_quiet_hooks() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 3101bd1db4a0..b5c6d4f24929 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1778,6 +1778,7 @@ struct HookRunCell { reveal_deadline: Instant, running_visible: bool, completed: Option, + completed_reveal_deadline: Option, quiet_removal_deadline: Option, } @@ -1802,7 +1803,7 @@ impl HookCell { } pub(crate) fn is_active(&self) -> bool { - self.runs.iter().any(|run| run.completed.is_none()) + self.runs.iter().any(HookRunCell::is_active) } pub(crate) fn should_flush(&self) -> bool { @@ -1824,6 +1825,7 @@ impl HookCell { existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; existing.running_visible = false; existing.completed = None; + existing.completed_reveal_deadline = None; existing.quiet_removal_deadline = None; return; } @@ -1836,6 +1838,7 @@ impl HookCell { reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, running_visible: false, completed: None, + completed_reveal_deadline: None, quiet_removal_deadline: None, }); } @@ -1861,6 +1864,7 @@ impl HookCell { status: run.status, entries: run.entries, }); + existing.completed_reveal_deadline = existing.completed_reveal_deadline_after_complete(); existing.quiet_removal_deadline = None; true } @@ -1880,6 +1884,7 @@ impl HookCell { status: run.status, entries: run.entries, }), + completed_reveal_deadline: None, quiet_removal_deadline: None, }); } @@ -1894,13 +1899,17 @@ impl HookCell { self.runs.len() != old_len } - pub(crate) fn update_due_running_visibility(&mut self, now: Instant) -> bool { + pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { let mut changed = false; for run in &mut self.runs { - if run.completed.is_none() && !run.running_visible && now >= run.reveal_deadline { + if run.should_reveal_running(now) { run.running_visible = true; changed = true; } + if run.should_reveal_completed(now) { + run.completed_reveal_deadline = None; + changed = true; + } } changed } @@ -1909,9 +1918,12 @@ impl HookCell { self.runs .iter() .filter_map(|run| { - run.quiet_removal_deadline.or_else(|| { - (run.completed.is_none() && !run.running_visible).then_some(run.reveal_deadline) - }) + run.quiet_removal_deadline + .or(run.completed_reveal_deadline) + .or_else(|| { + run.should_wait_to_reveal_running() + .then_some(run.reveal_deadline) + }) }) .min() } @@ -1925,11 +1937,20 @@ impl HookCell { } } + #[cfg(test)] + pub(crate) fn reveal_completed_runs_now_for_test(&mut self) { + for run in &mut self.runs { + if run.completed_reveal_deadline.is_some() { + run.completed_reveal_deadline = Some(Instant::now()); + } + } + } + #[cfg(test)] pub(crate) fn reveal_running_runs_now_for_test(&mut self) { let now = Instant::now(); for run in &mut self.runs { - if run.completed.is_none() { + if run.should_wait_to_reveal_running() { run.reveal_deadline = now; } } @@ -1963,7 +1984,7 @@ impl HistoryCell for HookCell { let elapsed = self .runs .iter() - .find_map(|run| run.completed.is_none().then_some(run.start_time).flatten())? + .find_map(|run| run.is_active().then_some(run.start_time).flatten())? .elapsed(); Some(elapsed.as_millis() as u64 / 600) } @@ -1990,50 +2011,83 @@ impl HookRunCell { (Instant::now() < minimum_deadline).then_some(minimum_deadline) } + fn completed_reveal_deadline_after_complete(&self) -> Option { + if !self.running_visible { + return None; + } + let minimum_deadline = self.minimum_running_deadline(); + (Instant::now() < minimum_deadline).then_some(minimum_deadline) + } + + fn minimum_running_deadline(&self) -> Instant { + self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE + } + + fn is_active(&self) -> bool { + self.completed.is_none() || self.completed_reveal_deadline.is_some() + } + + fn should_wait_to_reveal_running(&self) -> bool { + self.completed.is_none() && !self.running_visible + } + + fn should_reveal_running(&self, now: Instant) -> bool { + self.should_wait_to_reveal_running() && now >= self.reveal_deadline + } + + fn should_reveal_completed(&self, now: Instant) -> bool { + self.completed_reveal_deadline + .map(|deadline| now >= deadline) + .unwrap_or(false) + } + + fn should_display_running(&self) -> bool { + self.completed.is_none() || self.completed_reveal_deadline.is_some() + } + fn should_render(&self) -> bool { self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() } fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { let label = hook_event_label(self.event_name); - match &self.completed { - Some(completed) => { - let status = format!("{:?}", completed.status).to_lowercase(); - let bullet = match completed.status { - HookRunStatus::Completed => "•".green().bold(), - HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => { - "•".red().bold() - } - HookRunStatus::Running => "•".into(), - }; - lines.push( - vec![ - bullet, - " ".into(), - format!("{label} hook ({status})").into(), - ] - .into(), - ); - for entry in &completed.entries { - lines - .push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); - } + if self.should_display_running() { + let header_text = format!("Running {label} hook"); + let mut header = vec![spinner(self.start_time, animations_enabled), " ".into()]; + if animations_enabled { + header.extend(shimmer_spans(&header_text)); + } else { + header.push(header_text.bold()); } - None => { - let header_text = format!("Running {label} hook"); - let mut header = vec![spinner(self.start_time, animations_enabled), " ".into()]; - if animations_enabled { - header.extend(shimmer_spans(&header_text)); - } else { - header.push(header_text.bold()); - } - if let Some(status_message) = &self.status_message - && !status_message.is_empty() - { - header.push(": ".into()); - header.push(status_message.clone().dim()); + if let Some(status_message) = &self.status_message + && !status_message.is_empty() + { + header.push(": ".into()); + header.push(status_message.clone().dim()); + } + lines.push(header.into()); + return; + } + + if let Some(completed) = &self.completed { + let status = format!("{:?}", completed.status).to_lowercase(); + let bullet = match completed.status { + HookRunStatus::Completed => "•".green().bold(), + HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => { + "•".red().bold() } - lines.push(header.into()); + HookRunStatus::Running => "•".into(), + }; + lines.push( + vec![ + bullet, + " ".into(), + format!("{label} hook ({status})").into(), + ] + .into(), + ); + for entry in &completed.entries { + lines.push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); } } } From cfabf4483a8a97ecf6e0a5e15e39e6a656a291a4 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 15:07:45 -0700 Subject: [PATCH 06/20] Collapse identical running hook rows --- ...ning_hooks_collapse_to_count_snapshot.snap | 9 ++ .../src/chatwidget/tests/status_and_layout.rs | 19 ++++ codex-rs/tui/src/history_cell.rs | 96 ++++++++++++++++++- 3 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__identical_parallel_running_hooks_collapse_to_count_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__identical_parallel_running_hooks_collapse_to_count_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__identical_parallel_running_hooks_collapse_to_count_snapshot.snap new file mode 100644 index 000000000000..353d7227944c --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__identical_parallel_running_hooks_collapse_to_count_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "hook_live_and_history_snapshot(&chat, \"running\", \"\")" +--- +running +live hooks: +• Running 3 PreToolUse hooks: checking command policy +history: + diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index b6a57ee0bb32..46fe13166ddb 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1560,6 +1560,25 @@ async fn completed_hook_with_output_keeps_running_visible_for_floor() { ); } +#[tokio::test] +async fn identical_parallel_running_hooks_collapse_to_count() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + for tool_call_id in ["tool-call-1", "tool-call-2", "tool-call-3"] { + chat.handle_codex_event(hook_started_event( + &format!("pre-tool-use:0:/tmp/hooks.json:{tool_call_id}"), + codex_protocol::protocol::HookEventName::PreToolUse, + Some("checking command policy"), + )); + } + reveal_running_hooks(&mut chat); + + assert_chatwidget_snapshot!( + "identical_parallel_running_hooks_collapse_to_count_snapshot", + hook_live_and_history_snapshot(&chat, "running", "") + ); +} + #[tokio::test] async fn overlapping_hook_live_cell_tracks_parallel_quiet_hooks() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index b5c6d4f24929..4b074ff94209 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1788,6 +1788,18 @@ struct CompletedHookRun { entries: Vec, } +#[derive(Debug, PartialEq, Eq)] +struct RunningHookGroupKey { + event_name: HookEventName, + status_message: Option, +} + +struct RunningHookGroup { + key: RunningHookGroupKey, + start_time: Option, + count: usize, +} + impl HookCell { pub(crate) fn new(run: HookRunSummary, animations_enabled: bool) -> Self { let mut cell = Self { @@ -1958,15 +1970,36 @@ impl HookCell { fn display_lines_inner(&self) -> Vec> { let mut lines = Vec::new(); + let mut running_group: Option = None; for run in &self.runs { if !run.should_render() { continue; } - if !lines.is_empty() { - lines.push("".into()); + if let Some(key) = run.running_group_key() { + match running_group.as_mut() { + Some(group) if group.key == key => { + group.count += 1; + group.start_time = earliest_instant(group.start_time, run.start_time); + } + Some(group) => { + push_running_hook_group(&mut lines, group, self.animations_enabled); + running_group = Some(RunningHookGroup::new(key, run.start_time)); + } + None => { + running_group = Some(RunningHookGroup::new(key, run.start_time)); + } + } + continue; } + if let Some(group) = running_group.take() { + push_running_hook_group(&mut lines, &group, self.animations_enabled); + } + push_hook_line_separator(&mut lines); run.push_display_lines(&mut lines, self.animations_enabled); } + if let Some(group) = running_group { + push_running_hook_group(&mut lines, &group, self.animations_enabled); + } lines } } @@ -2045,6 +2078,13 @@ impl HookRunCell { self.completed.is_none() || self.completed_reveal_deadline.is_some() } + fn running_group_key(&self) -> Option { + self.should_display_running().then(|| RunningHookGroupKey { + event_name: self.event_name, + status_message: self.status_message.clone(), + }) + } + fn should_render(&self) -> bool { self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() } @@ -2093,6 +2133,58 @@ impl HookRunCell { } } +impl RunningHookGroup { + fn new(key: RunningHookGroupKey, start_time: Option) -> Self { + Self { + key, + start_time, + count: 1, + } + } +} + +fn push_running_hook_group( + lines: &mut Vec>, + group: &RunningHookGroup, + animations_enabled: bool, +) { + push_hook_line_separator(lines); + let label = hook_event_label(group.key.event_name); + let hook_text = if group.count == 1 { + format!("Running {label} hook") + } else { + format!("Running {} {label} hooks", group.count) + }; + let mut header = vec![spinner(group.start_time, animations_enabled), " ".into()]; + if animations_enabled { + header.extend(shimmer_spans(&hook_text)); + } else { + header.push(hook_text.bold()); + } + if let Some(status_message) = &group.key.status_message + && !status_message.is_empty() + { + header.push(": ".into()); + header.push(status_message.clone().dim()); + } + lines.push(header.into()); +} + +fn push_hook_line_separator(lines: &mut Vec>) { + if !lines.is_empty() { + lines.push("".into()); + } +} + +fn earliest_instant(left: Option, right: Option) -> Option { + match (left, right) { + (Some(left), Some(right)) => Some(left.min(right)), + (Some(left), None) => Some(left), + (None, Some(right)) => Some(right), + (None, None) => None, + } +} + pub(crate) fn new_active_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { HookCell::new(run, animations_enabled) } From e2b361da140cd14b3425ec9984bcdbc7589a234f Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 15:23:25 -0700 Subject: [PATCH 07/20] Use warning color for completed hooks with warnings --- codex-rs/tui/src/history_cell.rs | 43 ++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 4b074ff94209..24c53cc9072e 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2111,13 +2111,7 @@ impl HookRunCell { if let Some(completed) = &self.completed { let status = format!("{:?}", completed.status).to_lowercase(); - let bullet = match completed.status { - HookRunStatus::Completed => "•".green().bold(), - HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => { - "•".red().bold() - } - HookRunStatus::Running => "•".into(), - }; + let bullet = hook_completed_bullet(completed); lines.push( vec![ bullet, @@ -2202,6 +2196,24 @@ fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { run.status == HookRunStatus::Completed && run.entries.is_empty() } +fn hook_completed_bullet(completed: &CompletedHookRun) -> Span<'static> { + match completed.status { + HookRunStatus::Completed => { + if completed + .entries + .iter() + .any(|entry| entry.kind == HookOutputEntryKind::Warning) + { + "•".yellow().bold() + } else { + "•".green().bold() + } + } + HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => "•".red().bold(), + HookRunStatus::Running => "•".into(), + } +} + fn hook_output_prefix(kind: HookOutputEntryKind) -> &'static str { match kind { HookOutputEntryKind::Warning => "warning: ", @@ -3282,6 +3294,23 @@ mod tests { std::env::temp_dir() } + #[test] + fn completed_hook_with_warning_uses_warning_bullet() { + let completed = CompletedHookRun { + status: HookRunStatus::Completed, + entries: vec![HookOutputEntry { + kind: HookOutputEntryKind::Warning, + text: "Heads up from the hook".to_string(), + }], + }; + + let bullet = hook_completed_bullet(&completed); + + assert_eq!(bullet.content.as_ref(), "•"); + assert_eq!(bullet.style.fg, Some(Color::Yellow)); + assert!(bullet.style.add_modifier.contains(Modifier::BOLD)); + } + fn stdio_server_config( command: &str, args: Vec<&str>, From b9865d6bb5136d6d989aabef4ab94adfbe9c3201 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 15:57:22 -0700 Subject: [PATCH 08/20] Fix hook status flush on turn start --- codex-rs/tui/src/chatwidget.rs | 16 ++++++- ...lushes_at_next_turn_boundary_snapshot.snap | 9 ++++ .../src/chatwidget/tests/status_and_layout.rs | 48 +++++++++++++++++++ codex-rs/tui/src/history_cell.rs | 16 +++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index ff00b8c7dd2a..af0270140487 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2296,7 +2296,7 @@ impl ChatWidget { self.quit_shortcut_key = None; self.update_task_running_state(); self.retry_status_header = None; - self.active_hook_cell = None; + self.flush_active_hook_cell_for_new_turn(); self.pending_status_indicator_restore = false; self.bottom_pane .set_interrupt_hint_visible(/*visible*/ true); @@ -4008,6 +4008,20 @@ impl ChatWidget { if cell.should_flush() && let Some(cell) = self.active_hook_cell.take() { + self.bump_active_cell_revision(); + self.needs_final_message_separator = true; + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(cell))); + } + } + + fn flush_active_hook_cell_for_new_turn(&mut self) { + let Some(mut cell) = self.active_hook_cell.take() else { + return; + }; + cell.prepare_for_turn_boundary_flush(); + self.bump_active_cell_revision(); + if !cell.is_empty() { self.needs_final_message_separator = true; self.app_event_tx .send(AppEvent::InsertHistoryCell(Box::new(cell))); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap new file mode 100644 index 000000000000..4f68fb353343 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"active hooks:\\n{}history:\\n{history}\", active_hook_blob(&chat))" +--- +active hooks: + +history: +• PreToolUse hook (blocked) + feedback: command blocked by policy diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 46fe13166ddb..7a5ef30f07e6 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1560,6 +1560,54 @@ async fn completed_hook_with_output_keeps_running_visible_for_floor() { ); } +#[tokio::test] +async fn completed_hook_with_output_flushes_at_next_turn_boundary() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(hook_started_event( + "pre-tool-use:0:/tmp/hooks.json:tool-call-1", + codex_protocol::protocol::HookEventName::PreToolUse, + Some("checking command"), + )); + reveal_running_hooks(&mut chat); + + chat.handle_codex_event(hook_completed_event( + "pre-tool-use:0:/tmp/hooks.json:tool-call-1", + codex_protocol::protocol::HookEventName::PreToolUse, + codex_protocol::protocol::HookRunStatus::Blocked, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Feedback, + text: "command blocked by policy".to_string(), + }], + )); + assert!( + drain_insert_history(&mut rx).is_empty(), + "completed hook should still be deferred before the next turn starts" + ); + + chat.handle_codex_event(Event { + id: "turn-start".into(), + msg: EventMsg::TurnStarted(TurnStartedEvent { + turn_id: "turn-2".to_string(), + started_at: None, + model_context_window: None, + collaboration_mode_kind: ModeKind::Default, + }), + }); + + let history = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_chatwidget_snapshot!( + "completed_hook_with_output_flushes_at_next_turn_boundary_snapshot", + format!( + "active hooks:\n{}history:\n{history}", + active_hook_blob(&chat) + ) + ); +} + #[tokio::test] async fn identical_parallel_running_hooks_collapse_to_count() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 24c53cc9072e..db7f18a088f5 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1822,6 +1822,16 @@ impl HookCell { !self.is_active() && !self.is_empty() } + pub(crate) fn prepare_for_turn_boundary_flush(&mut self) { + for run in &mut self.runs { + if run.completed.is_some() { + run.completed_reveal_deadline = None; + run.quiet_removal_deadline = None; + } + } + self.runs.retain(HookRunCell::has_persistent_output); + } + pub(crate) fn has_visible_running_run(&self) -> bool { self.runs .iter() @@ -2089,6 +2099,12 @@ impl HookRunCell { self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() } + fn has_persistent_output(&self) -> bool { + self.completed.as_ref().is_some_and(|completed| { + completed.status != HookRunStatus::Completed || !completed.entries.is_empty() + }) + } + fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { let label = hook_event_label(self.event_name); if self.should_display_running() { From d7f3e41d8177957a960235ac3339fb07922f0a7a Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 16:12:24 -0700 Subject: [PATCH 09/20] codex: fix CI failure on PR #17266 --- codex-rs/tui/src/chatwidget/tests/status_and_layout.rs | 4 +++- codex-rs/tui/src/history_cell.rs | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 7a5ef30f07e6..03384ac22bcd 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1803,7 +1803,9 @@ fn hook_completed_event( id: id.to_string(), msg: EventMsg::HookCompleted(codex_protocol::protocol::HookCompletedEvent { turn_id: None, - run: hook_run_summary(id, event_name, status, None, entries), + run: hook_run_summary( + id, event_name, status, /*status_message*/ None, entries, + ), }), } } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index db7f18a088f5..f6e4eddf2620 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2220,7 +2220,7 @@ fn hook_completed_bullet(completed: &CompletedHookRun) -> Span<'static> { .iter() .any(|entry| entry.kind == HookOutputEntryKind::Warning) { - "•".yellow().bold() + "•".bold() } else { "•".green().bold() } @@ -3311,7 +3311,7 @@ mod tests { } #[test] - fn completed_hook_with_warning_uses_warning_bullet() { + fn completed_hook_with_warning_uses_default_bold_bullet() { let completed = CompletedHookRun { status: HookRunStatus::Completed, entries: vec![HookOutputEntry { @@ -3323,7 +3323,7 @@ mod tests { let bullet = hook_completed_bullet(&completed); assert_eq!(bullet.content.as_ref(), "•"); - assert_eq!(bullet.style.fg, Some(Color::Yellow)); + assert_eq!(bullet.style.fg, None); assert!(bullet.style.add_modifier.contains(Modifier::BOLD)); } From f6963c59c0055c90f9031456a954db823c72c0e1 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 16:37:11 -0700 Subject: [PATCH 10/20] Simplify hook status linger behavior --- codex-rs/tui/src/chatwidget.rs | 44 +++++---- ...following_assistant_message_snapshot.snap} | 2 + ..._output_flushes_immediately_snapshot.snap} | 10 +- ...hook_output_survives_restart_snapshot.snap | 9 ++ codex-rs/tui/src/chatwidget/tests/helpers.rs | 8 -- .../src/chatwidget/tests/status_and_layout.rs | 92 +++++++++++++------ codex-rs/tui/src/history_cell.rs | 79 +++++----------- 7 files changed, 128 insertions(+), 116 deletions(-) rename codex-rs/tui/src/chatwidget/snapshots/{codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap => codex_tui__chatwidget__tests__completed_hook_output_precedes_following_assistant_message_snapshot.snap} (87%) rename codex-rs/tui/src/chatwidget/snapshots/{codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap => codex_tui__chatwidget__tests__completed_hook_with_output_flushes_immediately_snapshot.snap} (53%) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_same_id_hook_output_survives_restart_snapshot.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index af0270140487..325a7dc1aa26 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2296,7 +2296,9 @@ impl ChatWidget { self.quit_shortcut_key = None; self.update_task_running_state(); self.retry_status_header = None; - self.flush_active_hook_cell_for_new_turn(); + if self.active_hook_cell.take().is_some() { + self.bump_active_cell_revision(); + } self.pending_status_indicator_restore = false; self.bottom_pane .set_interrupt_hint_visible(/*visible*/ true); @@ -3951,6 +3953,7 @@ impl ChatWidget { fn on_hook_started(&mut self, event: codex_protocol::protocol::HookStartedEvent) { self.flush_answer_stream_with_separator(); + self.flush_completed_hook_output(); match self.active_hook_cell.as_mut() { Some(cell) => { cell.start_run(event.run); @@ -3992,10 +3995,32 @@ impl ChatWidget { } } } + self.flush_completed_hook_output(); self.finish_active_hook_cell_if_idle(); self.request_redraw(); } + fn flush_completed_hook_output(&mut self) { + let Some(completed_cell) = self + .active_hook_cell + .as_mut() + .and_then(HookCell::take_completed_persistent_runs) + else { + return; + }; + let active_cell_is_empty = self + .active_hook_cell + .as_ref() + .is_some_and(HookCell::is_empty); + if active_cell_is_empty { + self.active_hook_cell = None; + } + self.bump_active_cell_revision(); + self.needs_final_message_separator = true; + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(completed_cell))); + } + fn finish_active_hook_cell_if_idle(&mut self) { let Some(cell) = self.active_hook_cell.as_ref() else { return; @@ -4015,20 +4040,7 @@ impl ChatWidget { } } - fn flush_active_hook_cell_for_new_turn(&mut self) { - let Some(mut cell) = self.active_hook_cell.take() else { - return; - }; - cell.prepare_for_turn_boundary_flush(); - self.bump_active_cell_revision(); - if !cell.is_empty() { - self.needs_final_message_separator = true; - self.app_event_tx - .send(AppEvent::InsertHistoryCell(Box::new(cell))); - } - } - - fn prune_expired_quiet_hook_runs(&mut self) { + fn update_due_hook_visibility(&mut self) { let Some(cell) = self.active_hook_cell.as_mut() else { return; }; @@ -4108,7 +4120,7 @@ impl ChatWidget { } pub(crate) fn pre_draw_tick(&mut self) { - self.prune_expired_quiet_hook_runs(); + self.update_due_hook_visibility(); self.schedule_hook_timer_if_needed(); self.bottom_pane.pre_draw_tick(); if self.should_animate_terminal_title_spinner() { diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_output_precedes_following_assistant_message_snapshot.snap similarity index 87% rename from codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap rename to codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_output_precedes_following_assistant_message_snapshot.snap index 4f68fb353343..5e8360f015cd 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_at_next_turn_boundary_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_output_precedes_following_assistant_message_snapshot.snap @@ -7,3 +7,5 @@ active hooks: history: • PreToolUse hook (blocked) feedback: command blocked by policy + +• The hook feedback was applied. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_immediately_snapshot.snap similarity index 53% rename from codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap rename to codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_immediately_snapshot.snap index 4eed0417cd1d..d2673f54006c 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_keeps_running_visible_for_floor_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_hook_with_output_flushes_immediately_snapshot.snap @@ -1,6 +1,6 @@ --- source: tui/src/chatwidget/tests/status_and_layout.rs -expression: "format!(\"{running_snapshot}\\n\\n{completed_deferred_snapshot}\\n\\n{completed_revealed_snapshot}\")" +expression: "format!(\"{running_snapshot}\\n\\n{completed_snapshot}\")" --- running live hooks: @@ -8,13 +8,7 @@ live hooks: history: -completed deferred -live hooks: -• Running PreToolUse hook -history: - - -completed revealed +completed live hooks: history: diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_same_id_hook_output_survives_restart_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_same_id_hook_output_survives_restart_snapshot.snap new file mode 100644 index 000000000000..ebdca8fc7523 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__completed_same_id_hook_output_survives_restart_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests/status_and_layout.rs +expression: "format!(\"active hooks:\\n{}history:\\n{history}\", active_hook_blob(&chat))" +--- +active hooks: +• Running Stop hook: checking stop condition +history: +• Stop hook (stopped) + stop: continue with more context diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index 39a3f9cb3d71..94c267946e94 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -687,13 +687,6 @@ pub(super) fn reveal_running_hooks(chat: &mut ChatWidget) { chat.pre_draw_tick(); } -pub(super) fn reveal_completed_hooks(chat: &mut ChatWidget) { - if let Some(cell) = chat.active_hook_cell.as_mut() { - cell.reveal_completed_runs_now_for_test(); - } - chat.pre_draw_tick(); -} - pub(super) fn get_available_model(chat: &ChatWidget, model: &str) -> ModelPreset { let models = chat .model_catalog @@ -1051,7 +1044,6 @@ pub(super) async fn assert_hook_events_snapshot( }), }); - reveal_completed_hooks(&mut chat); let cells = drain_insert_history(&mut rx); let combined = cells .iter() diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 03384ac22bcd..b9fa51946676 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1517,7 +1517,7 @@ async fn blocked_and_failed_hooks_render_feedback_and_errors() { } #[tokio::test] -async fn completed_hook_with_output_keeps_running_visible_for_floor() { +async fn completed_hook_with_output_flushes_immediately() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.handle_codex_event(hook_started_event( @@ -1537,31 +1537,20 @@ async fn completed_hook_with_output_keeps_running_visible_for_floor() { text: "command blocked by policy".to_string(), }], )); - let completed_deferred_snapshot = - hook_live_and_history_snapshot(&chat, "completed deferred", ""); - assert!( - drain_insert_history(&mut rx).is_empty(), - "completed hook should stay live until the running row has met its visible floor" - ); - - reveal_completed_hooks(&mut chat); let history = drain_insert_history(&mut rx) .iter() .map(|lines| lines_to_single_string(lines)) .collect::(); - let completed_revealed_snapshot = - hook_live_and_history_snapshot(&chat, "completed revealed", &history); + let completed_snapshot = hook_live_and_history_snapshot(&chat, "completed", &history); assert_chatwidget_snapshot!( - "completed_hook_with_output_keeps_running_visible_for_floor_snapshot", - format!( - "{running_snapshot}\n\n{completed_deferred_snapshot}\n\n{completed_revealed_snapshot}" - ) + "completed_hook_with_output_flushes_immediately_snapshot", + format!("{running_snapshot}\n\n{completed_snapshot}") ); } #[tokio::test] -async fn completed_hook_with_output_flushes_at_next_turn_boundary() { +async fn completed_hook_output_precedes_following_assistant_message() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.handle_codex_event(hook_started_event( @@ -1580,32 +1569,79 @@ async fn completed_hook_with_output_flushes_at_next_turn_boundary() { text: "command blocked by policy".to_string(), }], )); + + complete_assistant_message( + &mut chat, + "msg-after-hook", + "The hook feedback was applied.", + None, + ); + + let history = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_chatwidget_snapshot!( + "completed_hook_output_precedes_following_assistant_message_snapshot", + format!( + "active hooks:\n{}history:\n{history}", + active_hook_blob(&chat) + ) + ); + let hook_index = history + .find("PreToolUse hook (blocked)") + .expect("hook feedback should be in history"); + let assistant_index = history + .find("The hook feedback was applied.") + .expect("assistant message should be in history"); assert!( - drain_insert_history(&mut rx).is_empty(), - "completed hook should still be deferred before the next turn starts" + hook_index < assistant_index, + "hook output should precede later assistant text: {history:?}" ); +} - chat.handle_codex_event(Event { - id: "turn-start".into(), - msg: EventMsg::TurnStarted(TurnStartedEvent { - turn_id: "turn-2".to_string(), - started_at: None, - model_context_window: None, - collaboration_mode_kind: ModeKind::Default, - }), - }); +#[tokio::test] +async fn completed_same_id_hook_output_survives_restart() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let hook_id = "stop:0:/tmp/hooks.json"; + + chat.handle_codex_event(hook_started_event( + hook_id, + codex_protocol::protocol::HookEventName::Stop, + Some("checking stop condition"), + )); + reveal_running_hooks(&mut chat); + chat.handle_codex_event(hook_completed_event( + hook_id, + codex_protocol::protocol::HookEventName::Stop, + codex_protocol::protocol::HookRunStatus::Stopped, + vec![codex_protocol::protocol::HookOutputEntry { + kind: codex_protocol::protocol::HookOutputEntryKind::Stop, + text: "continue with more context".to_string(), + }], + )); + chat.handle_codex_event(hook_started_event( + hook_id, + codex_protocol::protocol::HookEventName::Stop, + Some("checking stop condition"), + )); + reveal_running_hooks(&mut chat); let history = drain_insert_history(&mut rx) .iter() .map(|lines| lines_to_single_string(lines)) .collect::(); assert_chatwidget_snapshot!( - "completed_hook_with_output_flushes_at_next_turn_boundary_snapshot", + "completed_same_id_hook_output_survives_restart_snapshot", format!( "active hooks:\n{}history:\n{history}", active_hook_blob(&chat) ) ); + assert!( + history.contains("Stop hook (stopped)\n stop: continue with more context"), + "first hook output should not be overwritten: {history:?}" + ); } #[tokio::test] diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index f6e4eddf2620..607cdf700560 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1767,7 +1767,7 @@ pub(crate) struct HookCell { } const HOOK_RUN_REVEAL_DELAY: Duration = Duration::from_millis(300); -const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(1000); +const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(600); #[derive(Debug)] struct HookRunCell { @@ -1778,7 +1778,6 @@ struct HookRunCell { reveal_deadline: Instant, running_visible: bool, completed: Option, - completed_reveal_deadline: Option, quiet_removal_deadline: Option, } @@ -1822,14 +1821,21 @@ impl HookCell { !self.is_active() && !self.is_empty() } - pub(crate) fn prepare_for_turn_boundary_flush(&mut self) { - for run in &mut self.runs { - if run.completed.is_some() { - run.completed_reveal_deadline = None; - run.quiet_removal_deadline = None; + pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { + let mut completed = Vec::new(); + let mut remaining = Vec::new(); + for run in self.runs.drain(..) { + if run.has_persistent_output() { + completed.push(run); + } else { + remaining.push(run); } } - self.runs.retain(HookRunCell::has_persistent_output); + self.runs = remaining; + (!completed.is_empty()).then_some(Self { + runs: completed, + animations_enabled: self.animations_enabled, + }) } pub(crate) fn has_visible_running_run(&self) -> bool { @@ -1847,7 +1853,6 @@ impl HookCell { existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; existing.running_visible = false; existing.completed = None; - existing.completed_reveal_deadline = None; existing.quiet_removal_deadline = None; return; } @@ -1860,7 +1865,6 @@ impl HookCell { reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, running_visible: false, completed: None, - completed_reveal_deadline: None, quiet_removal_deadline: None, }); } @@ -1871,7 +1875,7 @@ impl HookCell { return false; }; if hook_run_is_quiet_success(&run) { - if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_complete() { + if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_quiet_success() { self.runs[index].quiet_removal_deadline = Some(deadline); } else { self.runs.remove(index); @@ -1886,8 +1890,6 @@ impl HookCell { status: run.status, entries: run.entries, }); - existing.completed_reveal_deadline = existing.completed_reveal_deadline_after_complete(); - existing.quiet_removal_deadline = None; true } @@ -1906,7 +1908,6 @@ impl HookCell { status: run.status, entries: run.entries, }), - completed_reveal_deadline: None, quiet_removal_deadline: None, }); } @@ -1915,8 +1916,7 @@ impl HookCell { let old_len = self.runs.len(); self.runs.retain(|run| { run.quiet_removal_deadline - .map(|deadline| now < deadline) - .unwrap_or(true) + .is_none_or(|deadline| now < deadline) }); self.runs.len() != old_len } @@ -1928,10 +1928,6 @@ impl HookCell { run.running_visible = true; changed = true; } - if run.should_reveal_completed(now) { - run.completed_reveal_deadline = None; - changed = true; - } } changed } @@ -1940,12 +1936,10 @@ impl HookCell { self.runs .iter() .filter_map(|run| { - run.quiet_removal_deadline - .or(run.completed_reveal_deadline) - .or_else(|| { - run.should_wait_to_reveal_running() - .then_some(run.reveal_deadline) - }) + run.quiet_removal_deadline.or_else(|| { + run.should_wait_to_reveal_running() + .then_some(run.reveal_deadline) + }) }) .min() } @@ -1959,15 +1953,6 @@ impl HookCell { } } - #[cfg(test)] - pub(crate) fn reveal_completed_runs_now_for_test(&mut self) { - for run in &mut self.runs { - if run.completed_reveal_deadline.is_some() { - run.completed_reveal_deadline = Some(Instant::now()); - } - } - } - #[cfg(test)] pub(crate) fn reveal_running_runs_now_for_test(&mut self) { let now = Instant::now(); @@ -2046,7 +2031,7 @@ impl Renderable for HookCell { } impl HookRunCell { - fn quiet_removal_deadline_after_complete(&self) -> Option { + fn quiet_removal_deadline_after_quiet_success(&self) -> Option { if !self.running_visible { return None; } @@ -2054,20 +2039,8 @@ impl HookRunCell { (Instant::now() < minimum_deadline).then_some(minimum_deadline) } - fn completed_reveal_deadline_after_complete(&self) -> Option { - if !self.running_visible { - return None; - } - let minimum_deadline = self.minimum_running_deadline(); - (Instant::now() < minimum_deadline).then_some(minimum_deadline) - } - - fn minimum_running_deadline(&self) -> Instant { - self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE - } - fn is_active(&self) -> bool { - self.completed.is_none() || self.completed_reveal_deadline.is_some() + self.completed.is_none() || self.quiet_removal_deadline.is_some() } fn should_wait_to_reveal_running(&self) -> bool { @@ -2078,14 +2051,8 @@ impl HookRunCell { self.should_wait_to_reveal_running() && now >= self.reveal_deadline } - fn should_reveal_completed(&self, now: Instant) -> bool { - self.completed_reveal_deadline - .map(|deadline| now >= deadline) - .unwrap_or(false) - } - fn should_display_running(&self) -> bool { - self.completed.is_none() || self.completed_reveal_deadline.is_some() + self.completed.is_none() } fn running_group_key(&self) -> Option { From b17900d022ecef9d5805626e6ab35952c034e94b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 16:43:58 -0700 Subject: [PATCH 11/20] Fix argument comment lint in TUI test --- codex-rs/tui/src/chatwidget/tests/status_and_layout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index b9fa51946676..d72b2d55f7bc 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1574,7 +1574,7 @@ async fn completed_hook_output_precedes_following_assistant_message() { &mut chat, "msg-after-hook", "The hook feedback was applied.", - None, + /*phase*/ None, ); let history = drain_insert_history(&mut rx) From b07a5fc3efee7dd02b73835f971c1f61c580ea97 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 17:03:13 -0700 Subject: [PATCH 12/20] Avoid reserving space for hidden hook runs --- codex-rs/tui/src/chatwidget.rs | 10 ++++++---- codex-rs/tui/src/history_cell.rs | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 325a7dc1aa26..d02a2fd3c506 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -10971,10 +10971,12 @@ impl ChatWidget { None => RenderableItem::Owned(Box::new(())), }; let active_hook_cell_renderable = match &self.active_hook_cell { - Some(cell) => RenderableItem::Borrowed(cell).inset(Insets::tlbr( - /*top*/ 1, /*left*/ 0, /*bottom*/ 0, /*right*/ 0, - )), - None => RenderableItem::Owned(Box::new(())), + Some(cell) if cell.should_render() => { + RenderableItem::Borrowed(cell).inset(Insets::tlbr( + /*top*/ 1, /*left*/ 0, /*bottom*/ 0, /*right*/ 0, + )) + } + _ => RenderableItem::Owned(Box::new(())), }; let mut flex = FlexRenderable::new(); flex.push(/*flex*/ 1, active_cell_renderable); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 607cdf700560..479a865637d4 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1821,6 +1821,10 @@ impl HookCell { !self.is_active() && !self.is_empty() } + pub(crate) fn should_render(&self) -> bool { + self.runs.iter().any(HookRunCell::should_render) + } + pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { let mut completed = Vec::new(); let mut remaining = Vec::new(); From ae4261660b5be8d82fb582de15ab3eede84aaabd Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 17:18:23 -0700 Subject: [PATCH 13/20] Refactor hook history cell rendering --- codex-rs/tui/src/history_cell.rs | 490 +------------------- codex-rs/tui/src/history_cell/hook_cell.rs | 510 +++++++++++++++++++++ 2 files changed, 516 insertions(+), 484 deletions(-) create mode 100644 codex-rs/tui/src/history_cell/hook_cell.rs diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 479a865637d4..67bde227d9c5 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -25,7 +25,6 @@ use crate::render::line_utils::line_to_static; use crate::render::line_utils::prefix_lines; use crate::render::line_utils::push_owned_lines; use crate::render::renderable::Renderable; -use crate::shimmer::shimmer_spans; use crate::style::proposed_plan_style; use crate::style::user_message_style; #[cfg(test)] @@ -65,11 +64,6 @@ use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::FileChange; -use codex_protocol::protocol::HookEventName; -use codex_protocol::protocol::HookOutputEntry; -use codex_protocol::protocol::HookOutputEntryKind; -use codex_protocol::protocol::HookRunStatus; -use codex_protocol::protocol::HookRunSummary; use codex_protocol::protocol::McpAuthStatus; use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::SessionConfiguredEvent; @@ -100,6 +94,12 @@ use tracing::error; use unicode_segmentation::UnicodeSegmentation; use unicode_width::UnicodeWidthStr; +mod hook_cell; + +pub(crate) use hook_cell::HookCell; +pub(crate) use hook_cell::new_active_hook_cell; +pub(crate) use hook_cell::new_completed_hook_cell; + /// Represents an event to display in the conversation history. Returns its /// `Vec>` representation to make it easier to display in a /// scrollable list. @@ -1760,467 +1760,6 @@ pub(crate) fn new_warning_event(message: String) -> PrefixedWrappedHistoryCell { PrefixedWrappedHistoryCell::new(message.yellow(), "⚠ ".yellow(), " ") } -#[derive(Debug)] -pub(crate) struct HookCell { - runs: Vec, - animations_enabled: bool, -} - -const HOOK_RUN_REVEAL_DELAY: Duration = Duration::from_millis(300); -const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(600); - -#[derive(Debug)] -struct HookRunCell { - id: String, - event_name: HookEventName, - status_message: Option, - start_time: Option, - reveal_deadline: Instant, - running_visible: bool, - completed: Option, - quiet_removal_deadline: Option, -} - -#[derive(Debug)] -struct CompletedHookRun { - status: HookRunStatus, - entries: Vec, -} - -#[derive(Debug, PartialEq, Eq)] -struct RunningHookGroupKey { - event_name: HookEventName, - status_message: Option, -} - -struct RunningHookGroup { - key: RunningHookGroupKey, - start_time: Option, - count: usize, -} - -impl HookCell { - pub(crate) fn new(run: HookRunSummary, animations_enabled: bool) -> Self { - let mut cell = Self { - runs: Vec::new(), - animations_enabled, - }; - cell.start_run(run); - cell - } - - pub(crate) fn is_empty(&self) -> bool { - self.runs.is_empty() - } - - pub(crate) fn is_active(&self) -> bool { - self.runs.iter().any(HookRunCell::is_active) - } - - pub(crate) fn should_flush(&self) -> bool { - !self.is_active() && !self.is_empty() - } - - pub(crate) fn should_render(&self) -> bool { - self.runs.iter().any(HookRunCell::should_render) - } - - pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { - let mut completed = Vec::new(); - let mut remaining = Vec::new(); - for run in self.runs.drain(..) { - if run.has_persistent_output() { - completed.push(run); - } else { - remaining.push(run); - } - } - self.runs = remaining; - (!completed.is_empty()).then_some(Self { - runs: completed, - animations_enabled: self.animations_enabled, - }) - } - - pub(crate) fn has_visible_running_run(&self) -> bool { - self.runs - .iter() - .any(|run| run.completed.is_none() && run.running_visible) - } - - pub(crate) fn start_run(&mut self, run: HookRunSummary) { - if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { - existing.event_name = run.event_name; - existing.status_message = run.status_message; - let now = Instant::now(); - existing.start_time = Some(now); - existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; - existing.running_visible = false; - existing.completed = None; - existing.quiet_removal_deadline = None; - return; - } - let now = Instant::now(); - self.runs.push(HookRunCell { - id: run.id, - event_name: run.event_name, - status_message: run.status_message, - start_time: Some(now), - reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, - running_visible: false, - completed: None, - quiet_removal_deadline: None, - }); - } - - /// Completes a run and returns whether the run was already present in this cell. - pub(crate) fn complete_run(&mut self, run: HookRunSummary) -> bool { - let Some(index) = self.runs.iter().position(|existing| existing.id == run.id) else { - return false; - }; - if hook_run_is_quiet_success(&run) { - if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_quiet_success() { - self.runs[index].quiet_removal_deadline = Some(deadline); - } else { - self.runs.remove(index); - } - return true; - } - let existing = &mut self.runs[index]; - existing.event_name = run.event_name; - existing.status_message = run.status_message; - existing.start_time = None; - existing.completed = Some(CompletedHookRun { - status: run.status, - entries: run.entries, - }); - true - } - - pub(crate) fn add_completed_run(&mut self, run: HookRunSummary) { - if hook_run_is_quiet_success(&run) { - return; - } - self.runs.push(HookRunCell { - id: run.id, - event_name: run.event_name, - status_message: run.status_message, - start_time: None, - reveal_deadline: Instant::now(), - running_visible: false, - completed: Some(CompletedHookRun { - status: run.status, - entries: run.entries, - }), - quiet_removal_deadline: None, - }); - } - - pub(crate) fn prune_expired_quiet_runs(&mut self, now: Instant) -> bool { - let old_len = self.runs.len(); - self.runs.retain(|run| { - run.quiet_removal_deadline - .is_none_or(|deadline| now < deadline) - }); - self.runs.len() != old_len - } - - pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { - let mut changed = false; - for run in &mut self.runs { - if run.should_reveal_running(now) { - run.running_visible = true; - changed = true; - } - } - changed - } - - pub(crate) fn next_timer_deadline(&self) -> Option { - self.runs - .iter() - .filter_map(|run| { - run.quiet_removal_deadline.or_else(|| { - run.should_wait_to_reveal_running() - .then_some(run.reveal_deadline) - }) - }) - .min() - } - - #[cfg(test)] - pub(crate) fn expire_quiet_runs_now_for_test(&mut self) { - for run in &mut self.runs { - if run.quiet_removal_deadline.is_some() { - run.quiet_removal_deadline = Some(Instant::now()); - } - } - } - - #[cfg(test)] - pub(crate) fn reveal_running_runs_now_for_test(&mut self) { - let now = Instant::now(); - for run in &mut self.runs { - if run.should_wait_to_reveal_running() { - run.reveal_deadline = now; - } - } - } - - fn display_lines_inner(&self) -> Vec> { - let mut lines = Vec::new(); - let mut running_group: Option = None; - for run in &self.runs { - if !run.should_render() { - continue; - } - if let Some(key) = run.running_group_key() { - match running_group.as_mut() { - Some(group) if group.key == key => { - group.count += 1; - group.start_time = earliest_instant(group.start_time, run.start_time); - } - Some(group) => { - push_running_hook_group(&mut lines, group, self.animations_enabled); - running_group = Some(RunningHookGroup::new(key, run.start_time)); - } - None => { - running_group = Some(RunningHookGroup::new(key, run.start_time)); - } - } - continue; - } - if let Some(group) = running_group.take() { - push_running_hook_group(&mut lines, &group, self.animations_enabled); - } - push_hook_line_separator(&mut lines); - run.push_display_lines(&mut lines, self.animations_enabled); - } - if let Some(group) = running_group { - push_running_hook_group(&mut lines, &group, self.animations_enabled); - } - lines - } -} - -impl HistoryCell for HookCell { - fn display_lines(&self, _width: u16) -> Vec> { - self.display_lines_inner() - } - - fn transcript_lines(&self, width: u16) -> Vec> { - self.display_lines(width) - } - - fn transcript_animation_tick(&self) -> Option { - let elapsed = self - .runs - .iter() - .find_map(|run| run.is_active().then_some(run.start_time).flatten())? - .elapsed(); - Some(elapsed.as_millis() as u64 / 600) - } -} - -impl Renderable for HookCell { - fn render(&self, area: Rect, buf: &mut Buffer) { - let lines = self.display_lines(area.width); - let paragraph = Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }); - paragraph.render(area, buf); - } - - fn desired_height(&self, width: u16) -> u16 { - HistoryCell::desired_height(self, width) - } -} - -impl HookRunCell { - fn quiet_removal_deadline_after_quiet_success(&self) -> Option { - if !self.running_visible { - return None; - } - let minimum_deadline = self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE; - (Instant::now() < minimum_deadline).then_some(minimum_deadline) - } - - fn is_active(&self) -> bool { - self.completed.is_none() || self.quiet_removal_deadline.is_some() - } - - fn should_wait_to_reveal_running(&self) -> bool { - self.completed.is_none() && !self.running_visible - } - - fn should_reveal_running(&self, now: Instant) -> bool { - self.should_wait_to_reveal_running() && now >= self.reveal_deadline - } - - fn should_display_running(&self) -> bool { - self.completed.is_none() - } - - fn running_group_key(&self) -> Option { - self.should_display_running().then(|| RunningHookGroupKey { - event_name: self.event_name, - status_message: self.status_message.clone(), - }) - } - - fn should_render(&self) -> bool { - self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() - } - - fn has_persistent_output(&self) -> bool { - self.completed.as_ref().is_some_and(|completed| { - completed.status != HookRunStatus::Completed || !completed.entries.is_empty() - }) - } - - fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { - let label = hook_event_label(self.event_name); - if self.should_display_running() { - let header_text = format!("Running {label} hook"); - let mut header = vec![spinner(self.start_time, animations_enabled), " ".into()]; - if animations_enabled { - header.extend(shimmer_spans(&header_text)); - } else { - header.push(header_text.bold()); - } - if let Some(status_message) = &self.status_message - && !status_message.is_empty() - { - header.push(": ".into()); - header.push(status_message.clone().dim()); - } - lines.push(header.into()); - return; - } - - if let Some(completed) = &self.completed { - let status = format!("{:?}", completed.status).to_lowercase(); - let bullet = hook_completed_bullet(completed); - lines.push( - vec![ - bullet, - " ".into(), - format!("{label} hook ({status})").into(), - ] - .into(), - ); - for entry in &completed.entries { - lines.push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); - } - } - } -} - -impl RunningHookGroup { - fn new(key: RunningHookGroupKey, start_time: Option) -> Self { - Self { - key, - start_time, - count: 1, - } - } -} - -fn push_running_hook_group( - lines: &mut Vec>, - group: &RunningHookGroup, - animations_enabled: bool, -) { - push_hook_line_separator(lines); - let label = hook_event_label(group.key.event_name); - let hook_text = if group.count == 1 { - format!("Running {label} hook") - } else { - format!("Running {} {label} hooks", group.count) - }; - let mut header = vec![spinner(group.start_time, animations_enabled), " ".into()]; - if animations_enabled { - header.extend(shimmer_spans(&hook_text)); - } else { - header.push(hook_text.bold()); - } - if let Some(status_message) = &group.key.status_message - && !status_message.is_empty() - { - header.push(": ".into()); - header.push(status_message.clone().dim()); - } - lines.push(header.into()); -} - -fn push_hook_line_separator(lines: &mut Vec>) { - if !lines.is_empty() { - lines.push("".into()); - } -} - -fn earliest_instant(left: Option, right: Option) -> Option { - match (left, right) { - (Some(left), Some(right)) => Some(left.min(right)), - (Some(left), None) => Some(left), - (None, Some(right)) => Some(right), - (None, None) => None, - } -} - -pub(crate) fn new_active_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { - HookCell::new(run, animations_enabled) -} - -pub(crate) fn new_completed_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { - let mut cell = HookCell { - runs: Vec::new(), - animations_enabled, - }; - cell.add_completed_run(run); - cell -} - -fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { - run.status == HookRunStatus::Completed && run.entries.is_empty() -} - -fn hook_completed_bullet(completed: &CompletedHookRun) -> Span<'static> { - match completed.status { - HookRunStatus::Completed => { - if completed - .entries - .iter() - .any(|entry| entry.kind == HookOutputEntryKind::Warning) - { - "•".bold() - } else { - "•".green().bold() - } - } - HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => "•".red().bold(), - HookRunStatus::Running => "•".into(), - } -} - -fn hook_output_prefix(kind: HookOutputEntryKind) -> &'static str { - match kind { - HookOutputEntryKind::Warning => "warning: ", - HookOutputEntryKind::Stop => "stop: ", - HookOutputEntryKind::Feedback => "feedback: ", - HookOutputEntryKind::Context => "hook context: ", - HookOutputEntryKind::Error => "error: ", - } -} - -fn hook_event_label(event_name: HookEventName) -> &'static str { - match event_name { - HookEventName::PreToolUse => "PreToolUse", - HookEventName::PostToolUse => "PostToolUse", - HookEventName::SessionStart => "SessionStart", - HookEventName::UserPromptSubmit => "UserPromptSubmit", - HookEventName::Stop => "Stop", - } -} - #[derive(Debug)] pub(crate) struct DeprecationNoticeCell { summary: String, @@ -3281,23 +2820,6 @@ mod tests { std::env::temp_dir() } - #[test] - fn completed_hook_with_warning_uses_default_bold_bullet() { - let completed = CompletedHookRun { - status: HookRunStatus::Completed, - entries: vec![HookOutputEntry { - kind: HookOutputEntryKind::Warning, - text: "Heads up from the hook".to_string(), - }], - }; - - let bullet = hook_completed_bullet(&completed); - - assert_eq!(bullet.content.as_ref(), "•"); - assert_eq!(bullet.style.fg, None); - assert!(bullet.style.add_modifier.contains(Modifier::BOLD)); - } - fn stdio_server_config( command: &str, args: Vec<&str>, diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs new file mode 100644 index 000000000000..32c8330b7633 --- /dev/null +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -0,0 +1,510 @@ +use super::HistoryCell; +use crate::exec_cell::spinner; +use crate::render::renderable::Renderable; +use crate::shimmer::shimmer_spans; +use codex_protocol::protocol::HookEventName; +use codex_protocol::protocol::HookOutputEntry; +use codex_protocol::protocol::HookOutputEntryKind; +use codex_protocol::protocol::HookRunStatus; +use codex_protocol::protocol::HookRunSummary; +use ratatui::prelude::*; +use ratatui::style::Stylize; +use ratatui::widgets::Paragraph; +use ratatui::widgets::Wrap; +use std::time::Duration; +use std::time::Instant; + +#[derive(Debug)] +pub(crate) struct HookCell { + runs: Vec, + animations_enabled: bool, +} + +const HOOK_RUN_REVEAL_DELAY: Duration = Duration::from_millis(300); +const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(600); + +#[derive(Debug)] +struct HookRunCell { + id: String, + event_name: HookEventName, + status_message: Option, + start_time: Option, + reveal_deadline: Instant, + running_visible: bool, + completed: Option, + quiet_removal_deadline: Option, +} + +#[derive(Debug)] +struct CompletedHookRun { + status: HookRunStatus, + entries: Vec, +} + +#[derive(Debug, PartialEq, Eq)] +struct RunningHookGroupKey { + event_name: HookEventName, + status_message: Option, +} + +struct RunningHookGroup { + key: RunningHookGroupKey, + start_time: Option, + count: usize, +} + +impl HookCell { + pub(crate) fn new(run: HookRunSummary, animations_enabled: bool) -> Self { + let mut cell = Self { + runs: Vec::new(), + animations_enabled, + }; + cell.start_run(run); + cell + } + + pub(crate) fn is_empty(&self) -> bool { + self.runs.is_empty() + } + + pub(crate) fn is_active(&self) -> bool { + self.runs.iter().any(HookRunCell::is_active) + } + + pub(crate) fn should_flush(&self) -> bool { + !self.is_active() && !self.is_empty() + } + + pub(crate) fn should_render(&self) -> bool { + self.runs.iter().any(HookRunCell::should_render) + } + + pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { + let mut completed = Vec::new(); + let mut remaining = Vec::new(); + for run in self.runs.drain(..) { + if run.has_persistent_output() { + completed.push(run); + } else { + remaining.push(run); + } + } + self.runs = remaining; + (!completed.is_empty()).then_some(Self { + runs: completed, + animations_enabled: self.animations_enabled, + }) + } + + pub(crate) fn has_visible_running_run(&self) -> bool { + self.runs + .iter() + .any(|run| run.completed.is_none() && run.running_visible) + } + + pub(crate) fn start_run(&mut self, run: HookRunSummary) { + if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { + existing.event_name = run.event_name; + existing.status_message = run.status_message; + let now = Instant::now(); + existing.start_time = Some(now); + existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; + existing.running_visible = false; + existing.completed = None; + existing.quiet_removal_deadline = None; + return; + } + let now = Instant::now(); + self.runs.push(HookRunCell { + id: run.id, + event_name: run.event_name, + status_message: run.status_message, + start_time: Some(now), + reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, + running_visible: false, + completed: None, + quiet_removal_deadline: None, + }); + } + + /// Completes a run and returns whether the run was already present in this cell. + pub(crate) fn complete_run(&mut self, run: HookRunSummary) -> bool { + let Some(index) = self.runs.iter().position(|existing| existing.id == run.id) else { + return false; + }; + if hook_run_is_quiet_success(&run) { + if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_quiet_success() { + self.runs[index].quiet_removal_deadline = Some(deadline); + } else { + self.runs.remove(index); + } + return true; + } + let existing = &mut self.runs[index]; + existing.event_name = run.event_name; + existing.status_message = run.status_message; + existing.start_time = None; + existing.completed = Some(CompletedHookRun { + status: run.status, + entries: run.entries, + }); + true + } + + pub(crate) fn add_completed_run(&mut self, run: HookRunSummary) { + if hook_run_is_quiet_success(&run) { + return; + } + self.runs.push(HookRunCell { + id: run.id, + event_name: run.event_name, + status_message: run.status_message, + start_time: None, + reveal_deadline: Instant::now(), + running_visible: false, + completed: Some(CompletedHookRun { + status: run.status, + entries: run.entries, + }), + quiet_removal_deadline: None, + }); + } + + pub(crate) fn prune_expired_quiet_runs(&mut self, now: Instant) -> bool { + let old_len = self.runs.len(); + self.runs.retain(|run| { + run.quiet_removal_deadline + .is_none_or(|deadline| now < deadline) + }); + self.runs.len() != old_len + } + + pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { + let mut changed = false; + for run in &mut self.runs { + if run.should_reveal_running(now) { + run.running_visible = true; + changed = true; + } + } + changed + } + + pub(crate) fn next_timer_deadline(&self) -> Option { + self.runs + .iter() + .filter_map(|run| { + run.quiet_removal_deadline.or_else(|| { + run.should_wait_to_reveal_running() + .then_some(run.reveal_deadline) + }) + }) + .min() + } + + #[cfg(test)] + pub(crate) fn expire_quiet_runs_now_for_test(&mut self) { + for run in &mut self.runs { + if run.quiet_removal_deadline.is_some() { + run.quiet_removal_deadline = Some(Instant::now()); + } + } + } + + #[cfg(test)] + pub(crate) fn reveal_running_runs_now_for_test(&mut self) { + let now = Instant::now(); + for run in &mut self.runs { + if run.should_wait_to_reveal_running() { + run.reveal_deadline = now; + } + } + } + + fn display_lines_inner(&self) -> Vec> { + let mut lines = Vec::new(); + let mut running_group: Option = None; + for run in &self.runs { + if !run.should_render() { + continue; + } + if let Some(key) = run.running_group_key() { + match running_group.as_mut() { + Some(group) if group.key == key => { + group.count += 1; + group.start_time = earliest_instant(group.start_time, run.start_time); + } + Some(group) => { + push_running_hook_group(&mut lines, group, self.animations_enabled); + running_group = Some(RunningHookGroup::new(key, run.start_time)); + } + None => { + running_group = Some(RunningHookGroup::new(key, run.start_time)); + } + } + continue; + } + if let Some(group) = running_group.take() { + push_running_hook_group(&mut lines, &group, self.animations_enabled); + } + push_hook_line_separator(&mut lines); + run.push_display_lines(&mut lines, self.animations_enabled); + } + if let Some(group) = running_group { + push_running_hook_group(&mut lines, &group, self.animations_enabled); + } + lines + } +} + +impl HistoryCell for HookCell { + fn display_lines(&self, _width: u16) -> Vec> { + self.display_lines_inner() + } + + fn transcript_lines(&self, width: u16) -> Vec> { + self.display_lines(width) + } + + fn transcript_animation_tick(&self) -> Option { + let elapsed = self + .runs + .iter() + .find_map(|run| run.is_active().then_some(run.start_time).flatten())? + .elapsed(); + Some(elapsed.as_millis() as u64 / 600) + } +} + +impl Renderable for HookCell { + fn render(&self, area: Rect, buf: &mut Buffer) { + let lines = self.display_lines(area.width); + let paragraph = Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }); + paragraph.render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + HistoryCell::desired_height(self, width) + } +} + +impl HookRunCell { + fn quiet_removal_deadline_after_quiet_success(&self) -> Option { + if !self.running_visible { + return None; + } + let minimum_deadline = self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE; + (Instant::now() < minimum_deadline).then_some(minimum_deadline) + } + + fn is_active(&self) -> bool { + self.completed.is_none() || self.quiet_removal_deadline.is_some() + } + + fn should_wait_to_reveal_running(&self) -> bool { + self.completed.is_none() && !self.running_visible + } + + fn should_reveal_running(&self, now: Instant) -> bool { + self.should_wait_to_reveal_running() && now >= self.reveal_deadline + } + + fn should_display_running(&self) -> bool { + self.completed.is_none() + } + + fn running_group_key(&self) -> Option { + self.should_display_running().then(|| RunningHookGroupKey { + event_name: self.event_name, + status_message: self.status_message.clone(), + }) + } + + fn should_render(&self) -> bool { + self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() + } + + fn has_persistent_output(&self) -> bool { + self.completed.as_ref().is_some_and(|completed| { + completed.status != HookRunStatus::Completed || !completed.entries.is_empty() + }) + } + + fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { + let label = hook_event_label(self.event_name); + if self.should_display_running() { + let hook_text = format!("Running {label} hook"); + push_running_hook_header( + lines, + &hook_text, + self.start_time, + self.status_message.as_deref(), + animations_enabled, + ); + return; + } + + if let Some(completed) = &self.completed { + let status = format!("{:?}", completed.status).to_lowercase(); + let bullet = hook_completed_bullet(completed); + lines.push( + vec![ + bullet, + " ".into(), + format!("{label} hook ({status})").into(), + ] + .into(), + ); + for entry in &completed.entries { + lines.push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); + } + } + } +} + +impl RunningHookGroup { + fn new(key: RunningHookGroupKey, start_time: Option) -> Self { + Self { + key, + start_time, + count: 1, + } + } +} + +fn push_running_hook_group( + lines: &mut Vec>, + group: &RunningHookGroup, + animations_enabled: bool, +) { + push_hook_line_separator(lines); + let label = hook_event_label(group.key.event_name); + let hook_text = if group.count == 1 { + format!("Running {label} hook") + } else { + format!("Running {} {label} hooks", group.count) + }; + push_running_hook_header( + lines, + &hook_text, + group.start_time, + group.key.status_message.as_deref(), + animations_enabled, + ); +} + +fn push_running_hook_header( + lines: &mut Vec>, + hook_text: &str, + start_time: Option, + status_message: Option<&str>, + animations_enabled: bool, +) { + let mut header = vec![spinner(start_time, animations_enabled), " ".into()]; + if animations_enabled { + header.extend(shimmer_spans(hook_text)); + } else { + header.push(hook_text.to_string().bold()); + } + if let Some(status_message) = status_message + && !status_message.is_empty() + { + header.push(": ".into()); + header.push(status_message.to_string().dim()); + } + lines.push(header.into()); +} + +fn push_hook_line_separator(lines: &mut Vec>) { + if !lines.is_empty() { + lines.push("".into()); + } +} + +fn earliest_instant(left: Option, right: Option) -> Option { + match (left, right) { + (Some(left), Some(right)) => Some(left.min(right)), + (Some(left), None) => Some(left), + (None, Some(right)) => Some(right), + (None, None) => None, + } +} + +pub(crate) fn new_active_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { + HookCell::new(run, animations_enabled) +} + +pub(crate) fn new_completed_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { + let mut cell = HookCell { + runs: Vec::new(), + animations_enabled, + }; + cell.add_completed_run(run); + cell +} + +fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { + run.status == HookRunStatus::Completed && run.entries.is_empty() +} + +fn hook_completed_bullet(completed: &CompletedHookRun) -> Span<'static> { + match completed.status { + HookRunStatus::Completed => { + if completed + .entries + .iter() + .any(|entry| entry.kind == HookOutputEntryKind::Warning) + { + "•".bold() + } else { + "•".green().bold() + } + } + HookRunStatus::Blocked | HookRunStatus::Failed | HookRunStatus::Stopped => "•".red().bold(), + HookRunStatus::Running => "•".into(), + } +} + +fn hook_output_prefix(kind: HookOutputEntryKind) -> &'static str { + match kind { + HookOutputEntryKind::Warning => "warning: ", + HookOutputEntryKind::Stop => "stop: ", + HookOutputEntryKind::Feedback => "feedback: ", + HookOutputEntryKind::Context => "hook context: ", + HookOutputEntryKind::Error => "error: ", + } +} + +fn hook_event_label(event_name: HookEventName) -> &'static str { + match event_name { + HookEventName::PreToolUse => "PreToolUse", + HookEventName::PostToolUse => "PostToolUse", + HookEventName::SessionStart => "SessionStart", + HookEventName::UserPromptSubmit => "UserPromptSubmit", + HookEventName::Stop => "Stop", + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use ratatui::style::Modifier; + + #[test] + fn completed_hook_with_warning_uses_default_bold_bullet() { + let completed = CompletedHookRun { + status: HookRunStatus::Completed, + entries: vec![HookOutputEntry { + kind: HookOutputEntryKind::Warning, + text: "Heads up from the hook".to_string(), + }], + }; + + let bullet = hook_completed_bullet(&completed); + + assert_eq!(bullet.content.as_ref(), "•"); + assert_eq!(bullet.style.fg, None); + assert!(bullet.style.add_modifier.contains(Modifier::BOLD)); + } +} From b5c9677bf357ea0f64f3eb66ff2c99631f40c5f4 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 17:50:09 -0700 Subject: [PATCH 14/20] introduce HookRunState enum --- codex-rs/tui/src/history_cell/hook_cell.rs | 306 ++++++++++++++------- 1 file changed, 203 insertions(+), 103 deletions(-) diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 32c8330b7633..1dafe380b583 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -28,11 +28,24 @@ struct HookRunCell { id: String, event_name: HookEventName, status_message: Option, - start_time: Option, - reveal_deadline: Instant, - running_visible: bool, - completed: Option, - quiet_removal_deadline: Option, + state: HookRunState, +} + +#[derive(Debug)] +enum HookRunState { + PendingReveal { + start_time: Instant, + reveal_deadline: Instant, + }, + Running { + start_time: Instant, + reveal_deadline: Instant, + }, + QuietLinger { + start_time: Instant, + removal_deadline: Instant, + }, + Completed(CompletedHookRun), } #[derive(Debug)] @@ -68,7 +81,7 @@ impl HookCell { } pub(crate) fn is_active(&self) -> bool { - self.runs.iter().any(HookRunCell::is_active) + self.runs.iter().any(|run| run.state.is_active()) } pub(crate) fn should_flush(&self) -> bool { @@ -76,14 +89,14 @@ impl HookCell { } pub(crate) fn should_render(&self) -> bool { - self.runs.iter().any(HookRunCell::should_render) + self.runs.iter().any(|run| run.state.should_render()) } pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { let mut completed = Vec::new(); let mut remaining = Vec::new(); for run in self.runs.drain(..) { - if run.has_persistent_output() { + if run.state.has_persistent_output() { completed.push(run); } else { remaining.push(run); @@ -97,33 +110,22 @@ impl HookCell { } pub(crate) fn has_visible_running_run(&self) -> bool { - self.runs - .iter() - .any(|run| run.completed.is_none() && run.running_visible) + self.runs.iter().any(|run| run.state.is_running_visible()) } pub(crate) fn start_run(&mut self, run: HookRunSummary) { + let now = Instant::now(); if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { existing.event_name = run.event_name; existing.status_message = run.status_message; - let now = Instant::now(); - existing.start_time = Some(now); - existing.reveal_deadline = now + HOOK_RUN_REVEAL_DELAY; - existing.running_visible = false; - existing.completed = None; - existing.quiet_removal_deadline = None; + existing.state = HookRunState::pending(now); return; } - let now = Instant::now(); self.runs.push(HookRunCell { id: run.id, event_name: run.event_name, status_message: run.status_message, - start_time: Some(now), - reveal_deadline: now + HOOK_RUN_REVEAL_DELAY, - running_visible: false, - completed: None, - quiet_removal_deadline: None, + state: HookRunState::pending(now), }); } @@ -133,8 +135,8 @@ impl HookCell { return false; }; if hook_run_is_quiet_success(&run) { - if let Some(deadline) = self.runs[index].quiet_removal_deadline_after_quiet_success() { - self.runs[index].quiet_removal_deadline = Some(deadline); + if self.runs[index].start_quiet_linger_after_success() { + return true; } else { self.runs.remove(index); } @@ -143,8 +145,7 @@ impl HookCell { let existing = &mut self.runs[index]; existing.event_name = run.event_name; existing.status_message = run.status_message; - existing.start_time = None; - existing.completed = Some(CompletedHookRun { + existing.state = HookRunState::Completed(CompletedHookRun { status: run.status, entries: run.entries, }); @@ -159,31 +160,23 @@ impl HookCell { id: run.id, event_name: run.event_name, status_message: run.status_message, - start_time: None, - reveal_deadline: Instant::now(), - running_visible: false, - completed: Some(CompletedHookRun { + state: HookRunState::Completed(CompletedHookRun { status: run.status, entries: run.entries, }), - quiet_removal_deadline: None, }); } pub(crate) fn prune_expired_quiet_runs(&mut self, now: Instant) -> bool { let old_len = self.runs.len(); - self.runs.retain(|run| { - run.quiet_removal_deadline - .is_none_or(|deadline| now < deadline) - }); + self.runs.retain(|run| !run.state.quiet_linger_expired(now)); self.runs.len() != old_len } pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { let mut changed = false; for run in &mut self.runs { - if run.should_reveal_running(now) { - run.running_visible = true; + if run.state.reveal_if_due(now) { changed = true; } } @@ -193,21 +186,14 @@ impl HookCell { pub(crate) fn next_timer_deadline(&self) -> Option { self.runs .iter() - .filter_map(|run| { - run.quiet_removal_deadline.or_else(|| { - run.should_wait_to_reveal_running() - .then_some(run.reveal_deadline) - }) - }) + .filter_map(|run| run.state.next_timer_deadline()) .min() } #[cfg(test)] pub(crate) fn expire_quiet_runs_now_for_test(&mut self) { for run in &mut self.runs { - if run.quiet_removal_deadline.is_some() { - run.quiet_removal_deadline = Some(Instant::now()); - } + run.expire_quiet_linger_now_for_test(); } } @@ -215,9 +201,7 @@ impl HookCell { pub(crate) fn reveal_running_runs_now_for_test(&mut self) { let now = Instant::now(); for run in &mut self.runs { - if run.should_wait_to_reveal_running() { - run.reveal_deadline = now; - } + run.reveal_running_now_for_test(now); } } @@ -225,21 +209,22 @@ impl HookCell { let mut lines = Vec::new(); let mut running_group: Option = None; for run in &self.runs { - if !run.should_render() { + if !run.state.should_render() { continue; } if let Some(key) = run.running_group_key() { match running_group.as_mut() { Some(group) if group.key == key => { group.count += 1; - group.start_time = earliest_instant(group.start_time, run.start_time); + group.start_time = + earliest_instant(group.start_time, run.state.start_time()); } Some(group) => { push_running_hook_group(&mut lines, group, self.animations_enabled); - running_group = Some(RunningHookGroup::new(key, run.start_time)); + running_group = Some(RunningHookGroup::new(key, run.state.start_time())); } None => { - running_group = Some(RunningHookGroup::new(key, run.start_time)); + running_group = Some(RunningHookGroup::new(key, run.state.start_time())); } } continue; @@ -270,7 +255,12 @@ impl HistoryCell for HookCell { let elapsed = self .runs .iter() - .find_map(|run| run.is_active().then_some(run.start_time).flatten())? + .find_map(|run| { + run.state + .is_active() + .then(|| run.state.start_time()) + .flatten() + })? .elapsed(); Some(elapsed.as_millis() as u64 / 600) } @@ -289,77 +279,187 @@ impl Renderable for HookCell { } impl HookRunCell { - fn quiet_removal_deadline_after_quiet_success(&self) -> Option { - if !self.running_visible { - return None; + fn start_quiet_linger_after_success(&mut self) -> bool { + let Some((start_time, removal_deadline)) = self.state.quiet_linger_after_success() else { + return false; + }; + self.state = HookRunState::QuietLinger { + start_time, + removal_deadline, + }; + true + } + + #[cfg(test)] + fn expire_quiet_linger_now_for_test(&mut self) { + if let HookRunState::QuietLinger { + removal_deadline, .. + } = &mut self.state + { + *removal_deadline = Instant::now(); } - let minimum_deadline = self.reveal_deadline + QUIET_HOOK_MIN_VISIBLE; - (Instant::now() < minimum_deadline).then_some(minimum_deadline) } - fn is_active(&self) -> bool { - self.completed.is_none() || self.quiet_removal_deadline.is_some() + #[cfg(test)] + fn reveal_running_now_for_test(&mut self, now: Instant) { + if let HookRunState::PendingReveal { + reveal_deadline, .. + } = &mut self.state + { + *reveal_deadline = now; + } } - fn should_wait_to_reveal_running(&self) -> bool { - self.completed.is_none() && !self.running_visible + fn running_group_key(&self) -> Option { + self.state + .is_running_visible() + .then(|| RunningHookGroupKey { + event_name: self.event_name, + status_message: self.status_message.clone(), + }) } - fn should_reveal_running(&self, now: Instant) -> bool { - self.should_wait_to_reveal_running() && now >= self.reveal_deadline + fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { + let label = hook_event_label(self.event_name); + match &self.state { + HookRunState::Running { start_time, .. } + | HookRunState::QuietLinger { start_time, .. } => { + let hook_text = format!("Running {label} hook"); + push_running_hook_header( + lines, + &hook_text, + Some(*start_time), + self.status_message.as_deref(), + animations_enabled, + ); + } + HookRunState::Completed(completed) => { + let status = format!("{:?}", completed.status).to_lowercase(); + let bullet = hook_completed_bullet(completed); + lines.push( + vec![ + bullet, + " ".into(), + format!("{label} hook ({status})").into(), + ] + .into(), + ); + for entry in &completed.entries { + lines + .push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); + } + } + HookRunState::PendingReveal { .. } => {} + } } +} - fn should_display_running(&self) -> bool { - self.completed.is_none() +impl HookRunState { + fn pending(start_time: Instant) -> Self { + Self::PendingReveal { + start_time, + reveal_deadline: start_time + HOOK_RUN_REVEAL_DELAY, + } } - fn running_group_key(&self) -> Option { - self.should_display_running().then(|| RunningHookGroupKey { - event_name: self.event_name, - status_message: self.status_message.clone(), - }) + fn is_active(&self) -> bool { + match self { + HookRunState::PendingReveal { .. } + | HookRunState::Running { .. } + | HookRunState::QuietLinger { .. } => true, + HookRunState::Completed(_) => false, + } } fn should_render(&self) -> bool { - self.completed.is_some() || self.running_visible || self.quiet_removal_deadline.is_some() + match self { + HookRunState::Running { .. } + | HookRunState::QuietLinger { .. } + | HookRunState::Completed(_) => true, + HookRunState::PendingReveal { .. } => false, + } } fn has_persistent_output(&self) -> bool { - self.completed.as_ref().is_some_and(|completed| { - completed.status != HookRunStatus::Completed || !completed.entries.is_empty() - }) + match self { + HookRunState::Completed(completed) => { + completed.status != HookRunStatus::Completed || !completed.entries.is_empty() + } + HookRunState::PendingReveal { .. } + | HookRunState::Running { .. } + | HookRunState::QuietLinger { .. } => false, + } } - fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { - let label = hook_event_label(self.event_name); - if self.should_display_running() { - let hook_text = format!("Running {label} hook"); - push_running_hook_header( - lines, - &hook_text, - self.start_time, - self.status_message.as_deref(), - animations_enabled, - ); - return; + fn start_time(&self) -> Option { + match self { + HookRunState::PendingReveal { start_time, .. } + | HookRunState::Running { start_time, .. } + | HookRunState::QuietLinger { start_time, .. } => Some(*start_time), + HookRunState::Completed(_) => None, } + } - if let Some(completed) = &self.completed { - let status = format!("{:?}", completed.status).to_lowercase(); - let bullet = hook_completed_bullet(completed); - lines.push( - vec![ - bullet, - " ".into(), - format!("{label} hook ({status})").into(), - ] - .into(), - ); - for entry in &completed.entries { - lines.push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); - } + fn is_running_visible(&self) -> bool { + matches!( + self, + HookRunState::Running { .. } | HookRunState::QuietLinger { .. } + ) + } + + fn reveal_if_due(&mut self, now: Instant) -> bool { + let HookRunState::PendingReveal { + start_time, + reveal_deadline, + } = self + else { + return false; + }; + if now < *reveal_deadline { + return false; + } + *self = HookRunState::Running { + start_time: *start_time, + reveal_deadline: *reveal_deadline, + }; + true + } + + fn next_timer_deadline(&self) -> Option { + match self { + HookRunState::PendingReveal { + reveal_deadline, .. + } => Some(*reveal_deadline), + HookRunState::QuietLinger { + removal_deadline, .. + } => Some(*removal_deadline), + HookRunState::Running { .. } | HookRunState::Completed(_) => None, } } + + fn quiet_linger_expired(&self, now: Instant) -> bool { + match self { + HookRunState::QuietLinger { + removal_deadline, .. + } => now >= *removal_deadline, + HookRunState::PendingReveal { .. } + | HookRunState::Running { .. } + | HookRunState::Completed(_) => false, + } + } + + fn quiet_linger_after_success(&self) -> Option<(Instant, Instant)> { + let HookRunState::Running { + start_time, + reveal_deadline, + .. + } = self + else { + return None; + }; + let minimum_deadline = *reveal_deadline + QUIET_HOOK_MIN_VISIBLE; + (Instant::now() < minimum_deadline).then_some((*start_time, minimum_deadline)) + } } impl RunningHookGroup { From 383fc5183cce3d1ad9b64afb37ba6a8771ca2c9b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 19:51:24 -0700 Subject: [PATCH 15/20] Simplify hook history cell --- codex-rs/tui/src/history_cell/hook_cell.rs | 64 ++++++++++++---------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 1dafe380b583..32c4b54deaa1 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -135,20 +135,22 @@ impl HookCell { return false; }; if hook_run_is_quiet_success(&run) { - if self.runs[index].start_quiet_linger_after_success() { - return true; - } else { + if !self.runs[index].start_quiet_linger_after_success() { self.runs.remove(index); } return true; } + let HookRunSummary { + event_name, + status_message, + status, + entries, + .. + } = run; let existing = &mut self.runs[index]; - existing.event_name = run.event_name; - existing.status_message = run.status_message; - existing.state = HookRunState::Completed(CompletedHookRun { - status: run.status, - entries: run.entries, - }); + existing.event_name = event_name; + existing.status_message = status_message; + existing.state = HookRunState::completed(status, entries); true } @@ -156,14 +158,19 @@ impl HookCell { if hook_run_is_quiet_success(&run) { return; } + let HookRunSummary { + id, + event_name, + status_message, + status, + entries, + .. + } = run; self.runs.push(HookRunCell { - id: run.id, - event_name: run.event_name, - status_message: run.status_message, - state: HookRunState::Completed(CompletedHookRun { - status: run.status, - entries: run.entries, - }), + id, + event_name, + status_message, + state: HookRunState::completed(status, entries), }); } @@ -176,9 +183,7 @@ impl HookCell { pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { let mut changed = false; for run in &mut self.runs { - if run.state.reveal_if_due(now) { - changed = true; - } + changed |= run.state.reveal_if_due(now); } changed } @@ -204,8 +209,10 @@ impl HookCell { run.reveal_running_now_for_test(now); } } +} - fn display_lines_inner(&self) -> Vec> { +impl HistoryCell for HookCell { + fn display_lines(&self, _width: u16) -> Vec> { let mut lines = Vec::new(); let mut running_group: Option = None; for run in &self.runs { @@ -218,15 +225,14 @@ impl HookCell { group.count += 1; group.start_time = earliest_instant(group.start_time, run.state.start_time()); + continue; } Some(group) => { push_running_hook_group(&mut lines, group, self.animations_enabled); - running_group = Some(RunningHookGroup::new(key, run.state.start_time())); - } - None => { - running_group = Some(RunningHookGroup::new(key, run.state.start_time())); } + None => {} } + running_group = Some(RunningHookGroup::new(key, run.state.start_time())); continue; } if let Some(group) = running_group.take() { @@ -240,12 +246,6 @@ impl HookCell { } lines } -} - -impl HistoryCell for HookCell { - fn display_lines(&self, _width: u16) -> Vec> { - self.display_lines_inner() - } fn transcript_lines(&self, width: u16) -> Vec> { self.display_lines(width) @@ -362,6 +362,10 @@ impl HookRunState { } } + fn completed(status: HookRunStatus, entries: Vec) -> Self { + Self::Completed(CompletedHookRun { status, entries }) + } + fn is_active(&self) -> bool { match self { HookRunState::PendingReveal { .. } From 22490e9ba598f48e131a20da1b4d67159bc1755b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 19:58:30 -0700 Subject: [PATCH 16/20] Simplify hook history cell state --- codex-rs/tui/src/chatwidget.rs | 2 +- codex-rs/tui/src/history_cell/hook_cell.rs | 185 ++++++++++----------- 2 files changed, 91 insertions(+), 96 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index d02a2fd3c506..6c3b9a812f7e 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -4045,7 +4045,7 @@ impl ChatWidget { return; }; let now = Instant::now(); - if cell.update_due_visibility(now) || cell.prune_expired_quiet_runs(now) { + if cell.advance_time(now) { self.bump_active_cell_revision(); } self.finish_active_hook_cell_if_idle(); diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 32c4b54deaa1..7096459a571f 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -37,7 +37,7 @@ enum HookRunState { start_time: Instant, reveal_deadline: Instant, }, - Running { + VisibleRunning { start_time: Instant, reveal_deadline: Instant, }, @@ -45,13 +45,10 @@ enum HookRunState { start_time: Instant, removal_deadline: Instant, }, - Completed(CompletedHookRun), -} - -#[derive(Debug)] -struct CompletedHookRun { - status: HookRunStatus, - entries: Vec, + Completed { + status: HookRunStatus, + entries: Vec, + }, } #[derive(Debug, PartialEq, Eq)] @@ -67,7 +64,7 @@ struct RunningHookGroup { } impl HookCell { - pub(crate) fn new(run: HookRunSummary, animations_enabled: bool) -> Self { + fn new_active(run: HookRunSummary, animations_enabled: bool) -> Self { let mut cell = Self { runs: Vec::new(), animations_enabled, @@ -76,6 +73,15 @@ impl HookCell { cell } + fn new_completed(run: HookRunSummary, animations_enabled: bool) -> Self { + let mut cell = Self { + runs: Vec::new(), + animations_enabled, + }; + cell.add_completed_run(run); + cell + } + pub(crate) fn is_empty(&self) -> bool { self.runs.is_empty() } @@ -113,6 +119,16 @@ impl HookCell { self.runs.iter().any(|run| run.state.is_running_visible()) } + pub(crate) fn advance_time(&mut self, now: Instant) -> bool { + let old_len = self.runs.len(); + let mut changed = false; + for run in &mut self.runs { + changed |= run.state.reveal_if_due(now); + } + self.runs.retain(|run| !run.state.quiet_linger_expired(now)); + changed || self.runs.len() != old_len + } + pub(crate) fn start_run(&mut self, run: HookRunSummary) { let now = Instant::now(); if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { @@ -135,7 +151,10 @@ impl HookCell { return false; }; if hook_run_is_quiet_success(&run) { - if !self.runs[index].start_quiet_linger_after_success() { + if !self.runs[index] + .state + .complete_quiet_success(Instant::now()) + { self.runs.remove(index); } return true; @@ -174,20 +193,6 @@ impl HookCell { }); } - pub(crate) fn prune_expired_quiet_runs(&mut self, now: Instant) -> bool { - let old_len = self.runs.len(); - self.runs.retain(|run| !run.state.quiet_linger_expired(now)); - self.runs.len() != old_len - } - - pub(crate) fn update_due_visibility(&mut self, now: Instant) -> bool { - let mut changed = false; - for run in &mut self.runs { - changed |= run.state.reveal_if_due(now); - } - changed - } - pub(crate) fn next_timer_deadline(&self) -> Option { self.runs .iter() @@ -219,27 +224,29 @@ impl HistoryCell for HookCell { if !run.state.should_render() { continue; } - if let Some(key) = run.running_group_key() { - match running_group.as_mut() { - Some(group) if group.key == key => { - group.count += 1; - group.start_time = - earliest_instant(group.start_time, run.state.start_time()); - continue; - } - Some(group) => { - push_running_hook_group(&mut lines, group, self.animations_enabled); - } - None => {} + + let Some(key) = run.running_group_key() else { + if let Some(group) = running_group.take() { + push_running_hook_group(&mut lines, &group, self.animations_enabled); } - running_group = Some(RunningHookGroup::new(key, run.state.start_time())); + push_hook_line_separator(&mut lines); + run.push_display_lines(&mut lines, self.animations_enabled); + continue; + }; + + if let Some(group) = running_group.as_mut() + && group.key == key + { + group.count += 1; + group.start_time = earliest_instant(group.start_time, run.state.start_time()); continue; } - if let Some(group) = running_group.take() { + + if let Some(group) = + running_group.replace(RunningHookGroup::new(key, run.state.start_time())) + { push_running_hook_group(&mut lines, &group, self.animations_enabled); } - push_hook_line_separator(&mut lines); - run.push_display_lines(&mut lines, self.animations_enabled); } if let Some(group) = running_group { push_running_hook_group(&mut lines, &group, self.animations_enabled); @@ -279,17 +286,6 @@ impl Renderable for HookCell { } impl HookRunCell { - fn start_quiet_linger_after_success(&mut self) -> bool { - let Some((start_time, removal_deadline)) = self.state.quiet_linger_after_success() else { - return false; - }; - self.state = HookRunState::QuietLinger { - start_time, - removal_deadline, - }; - true - } - #[cfg(test)] fn expire_quiet_linger_now_for_test(&mut self) { if let HookRunState::QuietLinger { @@ -322,7 +318,7 @@ impl HookRunCell { fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { let label = hook_event_label(self.event_name); match &self.state { - HookRunState::Running { start_time, .. } + HookRunState::VisibleRunning { start_time, .. } | HookRunState::QuietLinger { start_time, .. } => { let hook_text = format!("Running {label} hook"); push_running_hook_header( @@ -333,18 +329,18 @@ impl HookRunCell { animations_enabled, ); } - HookRunState::Completed(completed) => { - let status = format!("{:?}", completed.status).to_lowercase(); - let bullet = hook_completed_bullet(completed); + HookRunState::Completed { status, entries } => { + let status_text = format!("{status:?}").to_lowercase(); + let bullet = hook_completed_bullet(*status, entries); lines.push( vec![ bullet, " ".into(), - format!("{label} hook ({status})").into(), + format!("{label} hook ({status_text})").into(), ] .into(), ); - for entry in &completed.entries { + for entry in entries { lines .push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); } @@ -363,34 +359,34 @@ impl HookRunState { } fn completed(status: HookRunStatus, entries: Vec) -> Self { - Self::Completed(CompletedHookRun { status, entries }) + Self::Completed { status, entries } } fn is_active(&self) -> bool { match self { HookRunState::PendingReveal { .. } - | HookRunState::Running { .. } + | HookRunState::VisibleRunning { .. } | HookRunState::QuietLinger { .. } => true, - HookRunState::Completed(_) => false, + HookRunState::Completed { .. } => false, } } fn should_render(&self) -> bool { match self { - HookRunState::Running { .. } + HookRunState::VisibleRunning { .. } | HookRunState::QuietLinger { .. } - | HookRunState::Completed(_) => true, + | HookRunState::Completed { .. } => true, HookRunState::PendingReveal { .. } => false, } } fn has_persistent_output(&self) -> bool { match self { - HookRunState::Completed(completed) => { - completed.status != HookRunStatus::Completed || !completed.entries.is_empty() + HookRunState::Completed { status, entries } => { + *status != HookRunStatus::Completed || !entries.is_empty() } HookRunState::PendingReveal { .. } - | HookRunState::Running { .. } + | HookRunState::VisibleRunning { .. } | HookRunState::QuietLinger { .. } => false, } } @@ -398,16 +394,16 @@ impl HookRunState { fn start_time(&self) -> Option { match self { HookRunState::PendingReveal { start_time, .. } - | HookRunState::Running { start_time, .. } + | HookRunState::VisibleRunning { start_time, .. } | HookRunState::QuietLinger { start_time, .. } => Some(*start_time), - HookRunState::Completed(_) => None, + HookRunState::Completed { .. } => None, } } fn is_running_visible(&self) -> bool { matches!( self, - HookRunState::Running { .. } | HookRunState::QuietLinger { .. } + HookRunState::VisibleRunning { .. } | HookRunState::QuietLinger { .. } ) } @@ -422,7 +418,7 @@ impl HookRunState { if now < *reveal_deadline { return false; } - *self = HookRunState::Running { + *self = HookRunState::VisibleRunning { start_time: *start_time, reveal_deadline: *reveal_deadline, }; @@ -437,7 +433,7 @@ impl HookRunState { HookRunState::QuietLinger { removal_deadline, .. } => Some(*removal_deadline), - HookRunState::Running { .. } | HookRunState::Completed(_) => None, + HookRunState::VisibleRunning { .. } | HookRunState::Completed { .. } => None, } } @@ -447,22 +443,30 @@ impl HookRunState { removal_deadline, .. } => now >= *removal_deadline, HookRunState::PendingReveal { .. } - | HookRunState::Running { .. } - | HookRunState::Completed(_) => false, + | HookRunState::VisibleRunning { .. } + | HookRunState::Completed { .. } => false, } } - fn quiet_linger_after_success(&self) -> Option<(Instant, Instant)> { - let HookRunState::Running { + fn complete_quiet_success(&mut self, now: Instant) -> bool { + let HookRunState::VisibleRunning { start_time, reveal_deadline, .. } = self else { - return None; + return false; }; + let start_time = *start_time; let minimum_deadline = *reveal_deadline + QUIET_HOOK_MIN_VISIBLE; - (Instant::now() < minimum_deadline).then_some((*start_time, minimum_deadline)) + if now >= minimum_deadline { + return false; + } + *self = HookRunState::QuietLinger { + start_time, + removal_deadline: minimum_deadline, + }; + true } } @@ -535,27 +539,21 @@ fn earliest_instant(left: Option, right: Option) -> Option HookCell { - HookCell::new(run, animations_enabled) + HookCell::new_active(run, animations_enabled) } pub(crate) fn new_completed_hook_cell(run: HookRunSummary, animations_enabled: bool) -> HookCell { - let mut cell = HookCell { - runs: Vec::new(), - animations_enabled, - }; - cell.add_completed_run(run); - cell + HookCell::new_completed(run, animations_enabled) } fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { run.status == HookRunStatus::Completed && run.entries.is_empty() } -fn hook_completed_bullet(completed: &CompletedHookRun) -> Span<'static> { - match completed.status { +fn hook_completed_bullet(status: HookRunStatus, entries: &[HookOutputEntry]) -> Span<'static> { + match status { HookRunStatus::Completed => { - if completed - .entries + if entries .iter() .any(|entry| entry.kind == HookOutputEntryKind::Warning) { @@ -597,15 +595,12 @@ mod tests { #[test] fn completed_hook_with_warning_uses_default_bold_bullet() { - let completed = CompletedHookRun { - status: HookRunStatus::Completed, - entries: vec![HookOutputEntry { - kind: HookOutputEntryKind::Warning, - text: "Heads up from the hook".to_string(), - }], - }; + let entries = vec![HookOutputEntry { + kind: HookOutputEntryKind::Warning, + text: "Heads up from the hook".to_string(), + }]; - let bullet = hook_completed_bullet(&completed); + let bullet = hook_completed_bullet(HookRunStatus::Completed, &entries); assert_eq!(bullet.content.as_ref(), "•"); assert_eq!(bullet.style.fg, None); From aa15e953d537eba3b3436271e0a1211902b441af Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 21:17:08 -0700 Subject: [PATCH 17/20] Fix quiet hook linger timing --- codex-rs/tui/src/chatwidget/tests/helpers.rs | 7 +++++ .../src/chatwidget/tests/status_and_layout.rs | 28 ++++++++++++++++++ codex-rs/tui/src/history_cell/hook_cell.rs | 29 ++++++++++++++++--- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index 94c267946e94..9aea70000460 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -687,6 +687,13 @@ pub(super) fn reveal_running_hooks(chat: &mut ChatWidget) { chat.pre_draw_tick(); } +pub(super) fn reveal_running_hooks_after_delayed_redraw(chat: &mut ChatWidget) { + if let Some(cell) = chat.active_hook_cell.as_mut() { + cell.reveal_running_runs_after_delayed_redraw_for_test(); + } + chat.pre_draw_tick(); +} + pub(super) fn get_available_model(chat: &ChatWidget, model: &str) -> ModelPreset { let models = chat .model_catalog diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index d72b2d55f7bc..a0287ae3f8a2 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1476,6 +1476,34 @@ async fn completed_hook_with_no_entries_stays_out_of_history() { ); } +#[tokio::test] +async fn quiet_hook_linger_starts_when_delayed_redraw_reveals_hook() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(hook_started_event( + "post-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + Some("checking output policy"), + )); + assert!(drain_insert_history(&mut rx).is_empty()); + + reveal_running_hooks_after_delayed_redraw(&mut chat); + chat.handle_codex_event(hook_completed_event( + "post-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + codex_protocol::protocol::HookRunStatus::Completed, + Vec::new(), + )); + + assert!(drain_insert_history(&mut rx).is_empty()); + assert!( + active_hook_blob(&chat).contains("Running PostToolUse hook"), + "quiet hook should linger after the row becomes visible" + ); + expire_quiet_hook_linger(&mut chat); + assert_eq!(active_hook_blob(&chat), "\n"); +} + #[tokio::test] async fn blocked_and_failed_hooks_render_feedback_and_errors() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 7096459a571f..94fc056788be 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -39,7 +39,7 @@ enum HookRunState { }, VisibleRunning { start_time: Instant, - reveal_deadline: Instant, + visible_since: Instant, }, QuietLinger { start_time: Instant, @@ -214,6 +214,14 @@ impl HookCell { run.reveal_running_now_for_test(now); } } + + #[cfg(test)] + pub(crate) fn reveal_running_runs_after_delayed_redraw_for_test(&mut self) { + let now = Instant::now(); + for run in &mut self.runs { + run.reveal_running_after_delayed_redraw_for_test(now); + } + } } impl HistoryCell for HookCell { @@ -306,6 +314,19 @@ impl HookRunCell { } } + #[cfg(test)] + fn reveal_running_after_delayed_redraw_for_test(&mut self, now: Instant) { + if let HookRunState::PendingReveal { + reveal_deadline, .. + } = &mut self.state + { + let delayed_deadline = now + .checked_sub(QUIET_HOOK_MIN_VISIBLE + Duration::from_millis(100)) + .unwrap_or(now); + *reveal_deadline = delayed_deadline; + } + } + fn running_group_key(&self) -> Option { self.state .is_running_visible() @@ -420,7 +441,7 @@ impl HookRunState { } *self = HookRunState::VisibleRunning { start_time: *start_time, - reveal_deadline: *reveal_deadline, + visible_since: now, }; true } @@ -451,14 +472,14 @@ impl HookRunState { fn complete_quiet_success(&mut self, now: Instant) -> bool { let HookRunState::VisibleRunning { start_time, - reveal_deadline, + visible_since, .. } = self else { return false; }; let start_time = *start_time; - let minimum_deadline = *reveal_deadline + QUIET_HOOK_MIN_VISIBLE; + let minimum_deadline = *visible_since + QUIET_HOOK_MIN_VISIBLE; if now >= minimum_deadline { return false; } From 2a8e2ae1bea63f17601d5515f2ed583de464eaaa Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 9 Apr 2026 21:53:07 -0700 Subject: [PATCH 18/20] Scope hook serialization failures by tool call --- codex-rs/hooks/src/events/common.rs | 26 ++++++++++++ codex-rs/hooks/src/events/post_tool_use.rs | 49 +++++++++++++--------- codex-rs/hooks/src/events/pre_tool_use.rs | 49 +++++++++++++--------- 3 files changed, 84 insertions(+), 40 deletions(-) diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index 8b6ba138ee4e..305a1214adb5 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -3,6 +3,7 @@ use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; +use codex_protocol::protocol::HookRunSummary; use crate::engine::ConfiguredHandler; use crate::engine::dispatcher; @@ -69,6 +70,31 @@ pub(crate) fn serialization_failure_hook_events( .collect() } +pub(crate) fn serialization_failure_hook_events_for_tool_use( + handlers: Vec, + turn_id: Option, + error_message: String, + tool_use_id: &str, +) -> Vec { + serialization_failure_hook_events(handlers, turn_id, error_message) + .into_iter() + .map(|event| hook_completed_for_tool_use(event, tool_use_id)) + .collect() +} + +pub(crate) fn hook_completed_for_tool_use( + mut event: HookCompletedEvent, + tool_use_id: &str, +) -> HookCompletedEvent { + event.run = hook_run_for_tool_use(event.run, tool_use_id); + event +} + +pub(crate) fn hook_run_for_tool_use(mut run: HookRunSummary, tool_use_id: &str) -> HookRunSummary { + run.id = format!("{}:{tool_use_id}", run.id); + run +} + pub(crate) fn matcher_pattern_for_event( event_name: HookEventName, matcher: Option<&str>, diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index 96353a36001b..9df05338e66a 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -59,7 +59,9 @@ pub(crate) fn preview( Some(&request.tool_name), ) .into_iter() - .map(|handler| hook_run_for_tool_use(dispatcher::running_summary(&handler), request)) + .map(|handler| { + common::hook_run_for_tool_use(dispatcher::running_summary(&handler), &request.tool_use_id) + }) .collect() } @@ -100,11 +102,13 @@ pub(crate) async fn run( }) { Ok(input_json) => input_json, Err(error) => { - return serialization_failure_outcome(common::serialization_failure_hook_events( + let hook_events = common::serialization_failure_hook_events_for_tool_use( matched, - Some(request.turn_id), + Some(request.turn_id.clone()), format!("failed to serialize post tool use hook input: {error}"), - )); + &request.tool_use_id, + ); + return serialization_failure_outcome(hook_events); } }; @@ -137,7 +141,9 @@ pub(crate) async fn run( PostToolUseOutcome { hook_events: results .into_iter() - .map(|result| hook_completed_for_tool_use(result.completed, &request)) + .map(|result| { + common::hook_completed_for_tool_use(result.completed, &request.tool_use_id) + }) .collect(), should_stop, stop_reason, @@ -146,19 +152,6 @@ pub(crate) async fn run( } } -fn hook_completed_for_tool_use( - mut event: HookCompletedEvent, - request: &PostToolUseRequest, -) -> HookCompletedEvent { - event.run = hook_run_for_tool_use(event.run, request); - event -} - -fn hook_run_for_tool_use(mut run: HookRunSummary, request: &PostToolUseRequest) -> HookRunSummary { - run.id = format!("{}:{}", run.id, request.tool_use_id); - run -} - fn parse_completed( handler: &ConfiguredHandler, run_result: CommandRunResult, @@ -320,11 +313,11 @@ mod tests { use serde_json::json; use super::PostToolUseHandlerData; - use super::hook_completed_for_tool_use; use super::parse_completed; use super::preview; use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; + use crate::events::common; #[test] fn block_decision_stops_normal_processing() { @@ -496,11 +489,27 @@ mod tests { run_result(Some(0), "", ""), Some("turn-1".to_string()), ); - let completed = hook_completed_for_tool_use(parsed.completed, &request); + let completed = common::hook_completed_for_tool_use(parsed.completed, &request.tool_use_id); assert_eq!(completed.run.id, runs[0].id); } + #[test] + fn serialization_failure_run_ids_include_tool_use_id() { + let request = request_for_tool_use("tool-call-456"); + let runs = preview(&[handler()], &request); + + let completed = common::serialization_failure_hook_events_for_tool_use( + vec![handler()], + Some(request.turn_id.clone()), + "serialize failed".into(), + &request.tool_use_id, + ); + + assert_eq!(completed.len(), 1); + assert_eq!(completed[0].run.id, runs[0].id); + } + fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PostToolUse, diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index f6cc31187726..44c9021b998f 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -52,7 +52,9 @@ pub(crate) fn preview( Some(&request.tool_name), ) .into_iter() - .map(|handler| hook_run_for_tool_use(dispatcher::running_summary(&handler), request)) + .map(|handler| { + common::hook_run_for_tool_use(dispatcher::running_summary(&handler), &request.tool_use_id) + }) .collect() } @@ -90,11 +92,13 @@ pub(crate) async fn run( }) { Ok(input_json) => input_json, Err(error) => { - return serialization_failure_outcome(common::serialization_failure_hook_events( + let hook_events = common::serialization_failure_hook_events_for_tool_use( matched, - Some(request.turn_id), + Some(request.turn_id.clone()), format!("failed to serialize pre tool use hook input: {error}"), - )); + &request.tool_use_id, + ); + return serialization_failure_outcome(hook_events); } }; @@ -116,26 +120,15 @@ pub(crate) async fn run( PreToolUseOutcome { hook_events: results .into_iter() - .map(|result| hook_completed_for_tool_use(result.completed, &request)) + .map(|result| { + common::hook_completed_for_tool_use(result.completed, &request.tool_use_id) + }) .collect(), should_block, block_reason, } } -fn hook_completed_for_tool_use( - mut event: HookCompletedEvent, - request: &PreToolUseRequest, -) -> HookCompletedEvent { - event.run = hook_run_for_tool_use(event.run, request); - event -} - -fn hook_run_for_tool_use(mut run: HookRunSummary, request: &PreToolUseRequest) -> HookRunSummary { - run.id = format!("{}:{}", run.id, request.tool_use_id); - run -} - fn parse_completed( handler: &ConfiguredHandler, run_result: CommandRunResult, @@ -256,11 +249,11 @@ mod tests { use pretty_assertions::assert_eq; use super::PreToolUseHandlerData; - use super::hook_completed_for_tool_use; use super::parse_completed; use super::preview; use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; + use crate::events::common; #[test] fn permission_decision_deny_blocks_processing() { @@ -485,11 +478,27 @@ mod tests { run_result(Some(0), "", ""), Some("turn-1".to_string()), ); - let completed = hook_completed_for_tool_use(parsed.completed, &request); + let completed = common::hook_completed_for_tool_use(parsed.completed, &request.tool_use_id); assert_eq!(completed.run.id, runs[0].id); } + #[test] + fn serialization_failure_run_ids_include_tool_use_id() { + let request = request_for_tool_use("tool-call-123"); + let runs = preview(&[handler()], &request); + + let completed = common::serialization_failure_hook_events_for_tool_use( + vec![handler()], + Some(request.turn_id.clone()), + "serialize failed".into(), + &request.tool_use_id, + ); + + assert_eq!(completed.len(), 1); + assert_eq!(completed[0].run.id, runs[0].id); + } + fn handler() -> ConfiguredHandler { ConfiguredHandler { event_name: HookEventName::PreToolUse, From c3e319b3972167709c1711440bd4600b57057304 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 10 Apr 2026 10:30:26 -0700 Subject: [PATCH 19/20] add comments to hook_cell --- codex-rs/tui/src/history_cell/hook_cell.rs | 101 +++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 94fc056788be..26144ee65264 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -1,3 +1,15 @@ +//! History cell for hook execution. +//! +//! Hooks are intentionally quieter than normal tool calls. A hook that starts and finishes +//! successfully without output should not leave a transcript artifact, and very fast hooks should +//! not flash in the viewport. This cell keeps that policy local by treating each hook run as a +//! small rendering state machine: +//! +//! 1. New runs begin hidden in `PendingReveal`. +//! 2. Runs that outlive the reveal delay become visible and may be coalesced with adjacent runs. +//! 3. Visible quiet successes linger briefly so they do not disappear in the same frame they were +//! first drawn. +//! 4. Completed runs only persist when they have output or a non-success status. use super::HistoryCell; use crate::exec_cell::spinner; use crate::render::renderable::Renderable; @@ -16,37 +28,63 @@ use std::time::Instant; #[derive(Debug)] pub(crate) struct HookCell { + /// Hook runs that are active, lingering, or have persistent output to render. runs: Vec, + /// Mirrors the global animation setting so transcript rendering and viewport rendering agree. animations_enabled: bool, } +/// Minimum runtime before a hook is allowed to draw. +/// +/// Helps avoids a flash of text forwork that was effectively instant. const HOOK_RUN_REVEAL_DELAY: Duration = Duration::from_millis(300); + +/// Minimum time a quiet success remains on screen after becoming visible. +/// +/// This pairs with `HOOK_RUN_REVEAL_DELAY`: once the user has seen a hook row, keep it stable long +/// enough to read instead of removing it immediately when the success event arrives. const QUIET_HOOK_MIN_VISIBLE: Duration = Duration::from_millis(600); #[derive(Debug)] struct HookRunCell { + /// Stable protocol id used to match begin/end updates for the same hook invocation. id: String, + /// Hook event kind, kept outside `state` so a begin update can refresh metadata in place. event_name: HookEventName, + /// Optional hook-supplied detail shown next to the running header. status_message: Option, + /// Rendering lifecycle for this run. state: HookRunState, } #[derive(Debug)] enum HookRunState { + /// A newly-started run that is active but deliberately hidden until `reveal_deadline`. PendingReveal { + /// The original start time, used for spinner phase and grouping once revealed. start_time: Instant, + /// First instant at which the run may become visible. reveal_deadline: Instant, }, + /// A run that survived the reveal delay and is currently shown as running. VisibleRunning { + /// The original start time, used to keep animation timing stable across transitions. start_time: Instant, + /// First instant the run was actually rendered, used by quiet-success linger. visible_since: Instant, }, + /// A visible run that completed successfully without output but is still lingering briefly. QuietLinger { + /// The original start time, retained so the spinner does not jump during the linger frame. start_time: Instant, + /// Instant after which the quiet success can be removed entirely. removal_deadline: Instant, }, + /// A completed run with output or a status worth preserving in history. Completed { + /// Final protocol status for the hook invocation. status: HookRunStatus, + /// Hook output entries rendered below the completed header. entries: Vec, }, } @@ -57,13 +95,21 @@ struct RunningHookGroupKey { status_message: Option, } +/// Accumulator for adjacent running hooks that can share one status line. +/// +/// Grouping happens only while building display lines, the underlying runs stay separate so their +/// protocol ids and completion transitions remain independent. struct RunningHookGroup { + /// Shared event/status pair for every run in this display group. key: RunningHookGroupKey, + /// Earliest start time in the group, so the combined spinner reflects the oldest work. start_time: Option, + /// Number of adjacent runs represented by the group line. count: usize, } impl HookCell { + /// Creates a cell around a hook that has just started. fn new_active(run: HookRunSummary, animations_enabled: bool) -> Self { let mut cell = Self { runs: Vec::new(), @@ -73,6 +119,7 @@ impl HookCell { cell } + /// Creates a cell around an already-completed hook from transcript/history data. fn new_completed(run: HookRunSummary, animations_enabled: bool) -> Self { let mut cell = Self { runs: Vec::new(), @@ -86,18 +133,25 @@ impl HookCell { self.runs.is_empty() } + /// Returns true while any run can still change due to an end event or timer. pub(crate) fn is_active(&self) -> bool { self.runs.iter().any(|run| run.state.is_active()) } + /// Completed hook cells are flushed out of the active slot once no timers remain. pub(crate) fn should_flush(&self) -> bool { !self.is_active() && !self.is_empty() } + /// Returns whether this cell has at least one line worth drawing right now. pub(crate) fn should_render(&self) -> bool { self.runs.iter().any(|run| run.state.should_render()) } + /// Splits durable completed runs from ephemeral active-cell bookkeeping. + /// + /// Quiet successes are left behind so they can disappear from the active cell, while failures, + /// blocked/stopped hooks, and hooks with emitted output become a persistent history cell. pub(crate) fn take_completed_persistent_runs(&mut self) -> Option { let mut completed = Vec::new(); let mut remaining = Vec::new(); @@ -115,10 +169,12 @@ impl HookCell { }) } + /// Used by callers that need to know whether the active cell currently occupies viewport space. pub(crate) fn has_visible_running_run(&self) -> bool { self.runs.iter().any(|run| run.state.is_running_visible()) } + /// Advances reveal/removal timers and reports whether rendering should be refreshed. pub(crate) fn advance_time(&mut self, now: Instant) -> bool { let old_len = self.runs.len(); let mut changed = false; @@ -129,6 +185,10 @@ impl HookCell { changed || self.runs.len() != old_len } + /// Inserts or refreshes a started hook run. + /// + /// A duplicate begin event resets the reveal timer rather than adding a second row, because + /// matching by id is the invariant that keeps begin/end events paired. pub(crate) fn start_run(&mut self, run: HookRunSummary) { let now = Instant::now(); if let Some(existing) = self.runs.iter_mut().find(|existing| existing.id == run.id) { @@ -146,6 +206,9 @@ impl HookCell { } /// Completes a run and returns whether the run was already present in this cell. + /// + /// Quiet successes intentionally avoid persistent output. If they were never visible, they + /// disappear immediately; if they had already drawn, they move into `QuietLinger`. pub(crate) fn complete_run(&mut self, run: HookRunSummary) -> bool { let Some(index) = self.runs.iter().position(|existing| existing.id == run.id) else { return false; @@ -173,6 +236,9 @@ impl HookCell { true } + /// Adds a completed hook that did not pass through this live cell. + /// + /// This is used for replay/restoration paths where the final run summary is already known. pub(crate) fn add_completed_run(&mut self, run: HookRunSummary) { if hook_run_is_quiet_success(&run) { return; @@ -225,6 +291,7 @@ impl HookCell { } impl HistoryCell for HookCell { + /// Builds viewport lines while coalescing adjacent visible-running hooks. fn display_lines(&self, _width: u16) -> Vec> { let mut lines = Vec::new(); let mut running_group: Option = None; @@ -234,6 +301,8 @@ impl HistoryCell for HookCell { } let Some(key) = run.running_group_key() else { + // Completed runs keep their own output lines, so any pending running group must be + // emitted before drawing the completed run. if let Some(group) = running_group.take() { push_running_hook_group(&mut lines, &group, self.animations_enabled); } @@ -246,6 +315,8 @@ impl HistoryCell for HookCell { && group.key == key { group.count += 1; + // Preserve the earliest start time so grouped spinners do not reset when a later + // adjacent hook is folded into the same line. group.start_time = earliest_instant(group.start_time, run.state.start_time()); continue; } @@ -262,10 +333,12 @@ impl HistoryCell for HookCell { lines } + /// Hook transcript output matches viewport output. fn transcript_lines(&self, width: u16) -> Vec> { self.display_lines(width) } + /// Produces a coarse cache key for transcript overlays while hook animations are active. fn transcript_animation_tick(&self) -> Option { let elapsed = self .runs @@ -327,6 +400,7 @@ impl HookRunCell { } } + /// Returns the grouping key only for states that render as running. fn running_group_key(&self) -> Option { self.state .is_running_visible() @@ -336,6 +410,7 @@ impl HookRunCell { }) } + /// Appends the lines for a single, ungrouped hook run. fn push_display_lines(&self, lines: &mut Vec>, animations_enabled: bool) { let label = hook_event_label(self.event_name); match &self.state { @@ -362,6 +437,8 @@ impl HookRunCell { .into(), ); for entry in entries { + // Output entries are already short hook-authored strings; keep their prefixes + // explicit so warnings/stops/errors remain easy to scan in history. lines .push(format!(" {}{}", hook_output_prefix(entry.kind), entry.text).into()); } @@ -372,6 +449,7 @@ impl HookRunCell { } impl HookRunState { + /// Creates the hidden initial state for a live hook run. fn pending(start_time: Instant) -> Self { Self::PendingReveal { start_time, @@ -379,10 +457,12 @@ impl HookRunState { } } + /// Creates the persistent final state for a hook with visible output or a notable status. fn completed(status: HookRunStatus, entries: Vec) -> Self { Self::Completed { status, entries } } + /// Returns true while the run is still waiting for a completion event or timer cleanup. fn is_active(&self) -> bool { match self { HookRunState::PendingReveal { .. } @@ -392,6 +472,7 @@ impl HookRunState { } } + /// Returns true when this run contributes at least one line to the current render. fn should_render(&self) -> bool { match self { HookRunState::VisibleRunning { .. } @@ -401,6 +482,7 @@ impl HookRunState { } } + /// Returns true for completed runs that should survive outside the active cell. fn has_persistent_output(&self) -> bool { match self { HookRunState::Completed { status, entries } => { @@ -412,6 +494,9 @@ impl HookRunState { } } + /// Returns the original start time for active states. + /// + /// Completed runs no longer animate, so they intentionally have no start time. fn start_time(&self) -> Option { match self { HookRunState::PendingReveal { start_time, .. } @@ -421,6 +506,7 @@ impl HookRunState { } } + /// Returns true when the run should be treated as an in-progress row. fn is_running_visible(&self) -> bool { matches!( self, @@ -428,6 +514,10 @@ impl HookRunState { ) } + /// Reveals a pending run once its deadline has passed. + /// + /// Returns true only when this call changes the state, allowing timer callbacks to avoid + /// unnecessary redraws. fn reveal_if_due(&mut self, now: Instant) -> bool { let HookRunState::PendingReveal { start_time, @@ -446,6 +536,7 @@ impl HookRunState { true } + /// Returns the next state-machine deadline owned by this run. fn next_timer_deadline(&self) -> Option { match self { HookRunState::PendingReveal { @@ -458,6 +549,7 @@ impl HookRunState { } } + /// Returns true once a quiet success has lingered for long enough. fn quiet_linger_expired(&self, now: Instant) -> bool { match self { HookRunState::QuietLinger { @@ -469,6 +561,10 @@ impl HookRunState { } } + /// Converts a visible quiet success into a temporary linger state. + /// + /// Returns false when the success should be removed immediately: either it was never visible or + /// it has already stayed visible for the minimum duration. fn complete_quiet_success(&mut self, now: Instant) -> bool { let HookRunState::VisibleRunning { start_time, @@ -501,6 +597,7 @@ impl RunningHookGroup { } } +/// Emits one grouped running-hook status row. fn push_running_hook_group( lines: &mut Vec>, group: &RunningHookGroup, @@ -522,6 +619,7 @@ fn push_running_hook_group( ); } +/// Emits the animated or static header used by all running hook rows. fn push_running_hook_header( lines: &mut Vec>, hook_text: &str, @@ -544,12 +642,14 @@ fn push_running_hook_header( lines.push(header.into()); } +/// Adds a blank separator between hook blocks without leaving a leading blank line. fn push_hook_line_separator(lines: &mut Vec>) { if !lines.is_empty() { lines.push("".into()); } } +/// Combines optional instants while preserving the earliest known start time. fn earliest_instant(left: Option, right: Option) -> Option { match (left, right) { (Some(left), Some(right)) => Some(left.min(right)), @@ -567,6 +667,7 @@ pub(crate) fn new_completed_hook_cell(run: HookRunSummary, animations_enabled: b HookCell::new_completed(run, animations_enabled) } +/// Returns true for hook completions that should be invisible in history. fn hook_run_is_quiet_success(run: &HookRunSummary) -> bool { run.status == HookRunStatus::Completed && run.entries.is_empty() } From 1ee9a760f36135e17219812e4fcf00e4ea8d2e24 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Fri, 10 Apr 2026 13:01:26 -0700 Subject: [PATCH 20/20] Fix hook transcript overlay rendering --- codex-rs/tui/src/chatwidget.rs | 6 +- .../src/chatwidget/tests/status_and_layout.rs | 41 +++++++++++++ codex-rs/tui/src/history_cell/hook_cell.rs | 60 +++++++++++++++++-- 3 files changed, 99 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 6c3b9a812f7e..e567ac652347 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -10940,10 +10940,12 @@ impl ChatWidget { lines.extend(cell.transcript_lines(width)); } if let Some(hook_cell) = self.active_hook_cell.as_ref() { - if !lines.is_empty() { + // Compute hook lines first so hidden hooks do not add a separator. + let hook_lines = hook_cell.transcript_lines(width); + if !hook_lines.is_empty() && !lines.is_empty() { lines.push("".into()); } - lines.extend(hook_cell.transcript_lines(width)); + lines.extend(hook_lines); } (!lines.is_empty()).then_some(lines) } diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index a0287ae3f8a2..2d1183ddd6ff 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -1795,6 +1795,47 @@ async fn running_hook_does_not_displace_active_exec_cell() { ); } +#[tokio::test] +async fn hidden_active_hook_does_not_add_transcript_separator() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + begin_exec(&mut chat, "call-1", "echo done"); + let exec_only_line_count = chat + .active_cell_transcript_lines(/*width*/ 80) + .expect("active exec transcript lines") + .len(); + + chat.handle_codex_event(hook_started_event( + "post-tool-use:0:/tmp/hooks.json", + codex_protocol::protocol::HookEventName::PostToolUse, + Some("checking output policy"), + )); + let hidden_hook_transcript = chat + .active_cell_transcript_lines(/*width*/ 80) + .expect("active exec transcript lines"); + assert_eq!(hidden_hook_transcript.len(), exec_only_line_count); + + reveal_running_hooks(&mut chat); + let visible_hook_lines = chat + .active_hook_cell + .as_ref() + .expect("active hook cell") + .transcript_lines(/*width*/ 80); + let visible_hook_transcript = chat + .active_cell_transcript_lines(/*width*/ 80) + .expect("active exec and hook transcript lines"); + assert_eq!( + visible_hook_transcript.len(), + exec_only_line_count + 1 + visible_hook_lines.len() + ); + assert_eq!( + lines_to_single_string( + &visible_hook_transcript[exec_only_line_count..exec_only_line_count + 1], + ), + "\n" + ); +} + #[tokio::test] async fn hook_completed_before_reveal_renders_completed_without_running_flash() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/history_cell/hook_cell.rs b/codex-rs/tui/src/history_cell/hook_cell.rs index 26144ee65264..bedf80cc45e9 100644 --- a/codex-rs/tui/src/history_cell/hook_cell.rs +++ b/codex-rs/tui/src/history_cell/hook_cell.rs @@ -340,15 +340,14 @@ impl HistoryCell for HookCell { /// Produces a coarse cache key for transcript overlays while hook animations are active. fn transcript_animation_tick(&self) -> Option { + if !self.animations_enabled { + return None; + } let elapsed = self .runs .iter() - .find_map(|run| { - run.state - .is_active() - .then(|| run.state.start_time()) - .flatten() - })? + .filter(|run| run.state.is_running_visible()) + .find_map(|run| run.state.start_time())? .elapsed(); Some(elapsed.as_millis() as u64 / 600) } @@ -714,6 +713,7 @@ mod tests { use super::*; use pretty_assertions::assert_eq; use ratatui::style::Modifier; + use std::path::PathBuf; #[test] fn completed_hook_with_warning_uses_default_bold_bullet() { @@ -728,4 +728,52 @@ mod tests { assert_eq!(bullet.style.fg, None); assert!(bullet.style.add_modifier.contains(Modifier::BOLD)); } + + #[test] + fn pending_hook_does_not_animate_transcript() { + let cell = + HookCell::new_active(hook_run_summary("hook-1"), /*animations_enabled*/ true); + + assert_eq!(cell.transcript_animation_tick(), None); + } + + #[test] + fn visible_hook_animates_transcript_when_animations_enabled() { + let mut cell = + HookCell::new_active(hook_run_summary("hook-1"), /*animations_enabled*/ true); + cell.reveal_running_runs_now_for_test(); + cell.advance_time(Instant::now()); + + assert_eq!(cell.transcript_animation_tick(), Some(0)); + } + + #[test] + fn visible_hook_does_not_animate_transcript_when_animations_disabled() { + let mut cell = HookCell::new_active( + hook_run_summary("hook-1"), + /*animations_enabled*/ false, + ); + cell.reveal_running_runs_now_for_test(); + cell.advance_time(Instant::now()); + + assert_eq!(cell.transcript_animation_tick(), None); + } + + fn hook_run_summary(id: &str) -> HookRunSummary { + HookRunSummary { + id: id.to_string(), + event_name: HookEventName::PostToolUse, + handler_type: codex_protocol::protocol::HookHandlerType::Command, + execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, + scope: codex_protocol::protocol::HookScope::Turn, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + status: HookRunStatus::Running, + status_message: Some("checking output policy".to_string()), + started_at: 1, + completed_at: None, + duration_ms: None, + entries: Vec::new(), + } + } }