fix(tui): restore working shimmer after preamble output (#10701)
## Problem When a turn streamed a preamble line before any tool activity, `ChatWidget` hid the status row while committing streamed lines and did not restore it until a later event (commonly `ExecCommandBegin`). During that idle gap, the UI looked finished even though the turn was still active. ## Mental model The bottom status row and transcript stream are separate progress affordances: - transcript stream shows committed output - status row (spinner/shimmer + header) shows liveness of an active turn While stream output is actively committing, hiding the status row is acceptable to avoid redundant visual noise. Once stream controllers go idle, an active turn must restore the status row immediately so liveness remains visible across preamble-to-tool gaps. ## Non-goals - No changes to streaming chunking policy or pacing. - No changes to final completion behavior (status still hides when task actually ends). - No refactor of status lifecycle ownership between `ChatWidget` and `BottomPane`. ## Tradeoffs - We keep the existing behavior of hiding the status row during active stream commits. - We add explicit restoration on the idle boundary when the task is still running. - This introduces one extra status update on idle transitions, which is small overhead but makes liveness semantics consistent. ## Architecture `run_commit_tick_with_scope` in `chatwidget.rs` now documents and enforces a two-phase contract: 1. For each committed streamed cell, hide status and append transcript output. 2. If controllers are present and all idle, restore status iff task is still running, preserving the current header. This keeps status ownership in `ChatWidget` while relying on `BottomPane` helpers: - `hide_status_indicator()` during active stream commits - `ensure_status_indicator()` + `set_status_header(current_status_header)` at stream-idle boundary Documentation pass additions: - Clarified the function-level contract and lifecycle intent in `run_commit_tick_with_scope`. - Added an explicit regression snapshot test comment describing the failing sequence. ## Observability Signal that the fix is present: - In the preamble-idle state, rendered output still includes `• Working (… esc to interrupt)`. - New snapshot: `codex_tui__chatwidget__tests__preamble_keeps_working_status.snap`. Debug path for future regressions: - Start at `run_commit_tick_with_scope` for hide/restore transitions. - Verify `bottom_pane.is_task_running()` at idle transition. - Confirm `current_status_header` continuity when status is recreated. - Use the new snapshot and targeted test sequence to reproduce deterministic preamble-idle behavior. ## Tests - Updated regression assertion: - `streaming_final_answer_keeps_task_running_state` now expects status widget to remain present while turn is running. - Renamed/updated behavioral regression: - `preamble_keeps_status_indicator_visible_until_exec_begin`. - Added snapshot regression coverage: - `preamble_keeps_working_status_snapshot`. - Snapshot file: `tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__preamble_keeps_working_status.snap`. Commands run: - `just fmt` - `cargo test -p codex-tui preamble_keeps_status_indicator_visible_until_exec_begin` - `cargo test -p codex-tui preamble_keeps_working_status_snapshot` ## Risks / Inconsistencies - Status visibility policy is still split across multiple event paths (`commit tick`, `turn complete`, `exec begin`), so future regressions can reintroduce ordering gaps. - Restoration depends on `is_task_running()` correctness; if task lifecycle flags drift, status behavior will drift too. - Snapshot proves rendered state, not animation cadence; cadence still relies on frame scheduling behavior elsewhere.
This commit is contained in:
parent
73f32840c6
commit
d876f3b94f
4 changed files with 339 additions and 240 deletions
532
MODULE.bazel.lock
generated
532
MODULE.bazel.lock
generated
File diff suppressed because one or more lines are too long
|
|
@ -1867,7 +1867,10 @@ impl ChatWidget {
|
|||
/// Runs a commit tick for the current stream queue snapshot.
|
||||
///
|
||||
/// `scope` controls whether this call may commit in smooth mode or only when catch-up
|
||||
/// is currently active.
|
||||
/// is currently active. While lines are actively streaming we hide the status row to avoid
|
||||
/// duplicate "in progress" affordances, but once all stream controllers go idle for this
|
||||
/// turn we restore the status row if the task is still running so users keep a live
|
||||
/// spinner/shimmer signal between preamble output and subsequent tool activity.
|
||||
fn run_commit_tick_with_scope(&mut self, scope: CommitTickScope) {
|
||||
let now = Instant::now();
|
||||
let outcome = run_commit_tick(
|
||||
|
|
@ -1883,6 +1886,10 @@ impl ChatWidget {
|
|||
}
|
||||
|
||||
if outcome.has_controller && outcome.all_idle {
|
||||
if self.bottom_pane.is_task_running() {
|
||||
self.bottom_pane.ensure_status_indicator();
|
||||
self.set_status_header(self.current_status_header.clone());
|
||||
}
|
||||
self.app_event_tx.send(AppEvent::StopCommitAnimation);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,11 @@
|
|||
---
|
||||
source: tui/src/chatwidget/tests.rs
|
||||
expression: terminal.backend()
|
||||
---
|
||||
" "
|
||||
"• Working (0s • esc to interrupt) "
|
||||
" "
|
||||
" "
|
||||
"› Ask Codex to do anything "
|
||||
" "
|
||||
" ? for shortcuts 100% context left "
|
||||
|
|
@ -1810,7 +1810,7 @@ async fn streaming_final_answer_keeps_task_running_state() {
|
|||
chat.on_commit_tick();
|
||||
|
||||
assert!(chat.bottom_pane.is_task_running());
|
||||
assert!(chat.bottom_pane.status_widget().is_none());
|
||||
assert!(chat.bottom_pane.status_widget().is_some());
|
||||
|
||||
chat.bottom_pane
|
||||
.set_composer_text("queued submission".to_string(), Vec::new(), Vec::new());
|
||||
|
|
@ -1832,7 +1832,7 @@ async fn streaming_final_answer_keeps_task_running_state() {
|
|||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_begin_restores_status_indicator_after_preamble() {
|
||||
async fn preamble_keeps_status_indicator_visible_until_exec_begin() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
|
||||
chat.on_task_started();
|
||||
|
|
@ -1842,7 +1842,7 @@ async fn exec_begin_restores_status_indicator_after_preamble() {
|
|||
chat.on_commit_tick();
|
||||
drain_insert_history(&mut rx);
|
||||
|
||||
assert_eq!(chat.bottom_pane.status_indicator_visible(), false);
|
||||
assert_eq!(chat.bottom_pane.status_indicator_visible(), true);
|
||||
assert_eq!(chat.bottom_pane.is_task_running(), true);
|
||||
|
||||
begin_exec(&mut chat, "call-1", "echo hi");
|
||||
|
|
@ -1850,6 +1850,27 @@ async fn exec_begin_restores_status_indicator_after_preamble() {
|
|||
assert_eq!(chat.bottom_pane.status_indicator_visible(), true);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn preamble_keeps_working_status_snapshot() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
chat.thread_id = Some(ThreadId::new());
|
||||
|
||||
// Regression sequence: a preamble line is committed to history before any exec/tool event.
|
||||
// The status row must remain visible so the spinner/shimmer still communicates "working".
|
||||
chat.on_task_started();
|
||||
chat.on_agent_message_delta("Preamble line\n".to_string());
|
||||
chat.on_commit_tick();
|
||||
drain_insert_history(&mut rx);
|
||||
|
||||
let height = chat.desired_height(80);
|
||||
let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height))
|
||||
.expect("create terminal");
|
||||
terminal
|
||||
.draw(|f| chat.render(f.area(), f.buffer_mut()))
|
||||
.expect("draw preamble + status widget");
|
||||
assert_snapshot!("preamble_keeps_working_status", terminal.backend());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn ctrl_c_shutdown_works_with_caps_lock() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue