mirror of
https://github.com/openai/codex.git
synced 2026-10-26 18:38:47 +00:00
Drive TUI stream animation from the foreground loop (#40644)
## Why Stream animation ticks generated on a background thread can accumulate while the TUI is busy, causing stale ticks to run after the animation stops or the chat widget is replaced. ## What changed - Replace the animation thread and `CommitTick` app event with a Tokio interval owned by the foreground event loop. - Delay missed ticks instead of replaying a burst, and let pending app events run before animation work. - Drop the interval when animation stops or the chat widget is replaced, while preserving `CommitTick` session-log entries. ## Testing Add a paused-time test covering stable pacing, missed ticks, restart behavior, and chat-widget replacement. GitOrigin-RevId: d0561d3f8962b5e05a728463e76aa8ec83cd5a31
This commit is contained in:
committed by
copyberry
parent
a9e447a69d
commit
a95391160a
@@ -190,8 +190,6 @@ use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::thread;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
use tokio::select;
|
||||
@@ -568,8 +566,8 @@ pub(crate) struct App {
|
||||
pub(crate) keymap: RuntimeKeymap,
|
||||
pub(crate) key_chord_matcher: KeyChordMatcher,
|
||||
|
||||
/// Controls the animation thread that sends CommitTick events.
|
||||
pub(crate) commit_anim_running: Arc<AtomicBool>,
|
||||
/// The foreground loop owns stream pacing; stopped animations have no timer.
|
||||
pub(crate) commit_animation: Option<tokio::time::Interval>,
|
||||
// Shared across ChatWidget instances so invalid status-line config warnings only emit once.
|
||||
status_line_invalid_items_warned: Arc<AtomicBool>,
|
||||
// Shared across ChatWidget instances so invalid terminal-title config warnings only emit once.
|
||||
|
||||
@@ -616,26 +616,17 @@ impl App {
|
||||
self.insert_pending_usage_output_after_stream_shutdown(tui);
|
||||
}
|
||||
AppEvent::StartCommitAnimation => {
|
||||
if self
|
||||
.commit_anim_running
|
||||
.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed)
|
||||
.is_ok()
|
||||
{
|
||||
let tx = self.app_event_tx.clone();
|
||||
let running = self.commit_anim_running.clone();
|
||||
thread::spawn(move || {
|
||||
while running.load(Ordering::Relaxed) {
|
||||
thread::sleep(COMMIT_ANIMATION_TICK);
|
||||
tx.send(AppEvent::CommitTick);
|
||||
}
|
||||
});
|
||||
}
|
||||
self.commit_animation.get_or_insert_with(|| {
|
||||
let mut interval = tokio::time::interval_at(
|
||||
tokio::time::Instant::now() + COMMIT_ANIMATION_TICK,
|
||||
COMMIT_ANIMATION_TICK,
|
||||
);
|
||||
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);
|
||||
interval
|
||||
});
|
||||
}
|
||||
AppEvent::StopCommitAnimation => {
|
||||
self.commit_anim_running.store(false, Ordering::Release);
|
||||
}
|
||||
AppEvent::CommitTick => {
|
||||
self.chat_widget.on_commit_tick();
|
||||
self.commit_animation = None;
|
||||
}
|
||||
AppEvent::Exit(mode) => {
|
||||
if matches!(mode, ExitMode::ShutdownFirst | ExitMode::ShutdownAfterInterrupt) {
|
||||
|
||||
@@ -439,6 +439,7 @@ impl App {
|
||||
/// This helper copies every known nickname/role from `AgentNavigationState` into the
|
||||
/// replacement widget so that replayed collab items render agent names immediately.
|
||||
pub(super) fn replace_chat_widget(&mut self, mut chat_widget: ChatWidget) {
|
||||
self.commit_animation = None;
|
||||
// Transfer the last-written terminal title to the replacement widget
|
||||
// so it knows what OSC title is currently displayed. Without this, the
|
||||
// new widget would redundantly clear and rewrite the same title, causing
|
||||
|
||||
@@ -504,7 +504,7 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
transcript_reflow: TranscriptReflowState::default(),
|
||||
initial_history_replay_buffer: None,
|
||||
scrollback_has_older_history: false,
|
||||
commit_anim_running: Arc::new(AtomicBool::new(false)),
|
||||
commit_animation: None,
|
||||
status_line_invalid_items_warned: status_line_invalid_items_warned.clone(),
|
||||
terminal_title_invalid_items_warned: terminal_title_invalid_items_warned.clone(),
|
||||
skill_load_warnings: SkillLoadWarningState::default(),
|
||||
@@ -812,6 +812,18 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
app.chat_widget.refresh_terminal_title();
|
||||
AppRunControl::Continue
|
||||
}
|
||||
() = async {
|
||||
match app.commit_animation.as_mut() {
|
||||
Some(interval) => {
|
||||
interval.tick().await;
|
||||
}
|
||||
None => std::future::pending().await,
|
||||
}
|
||||
}, if !has_pending_app_events => {
|
||||
crate::session_log::log_commit_tick();
|
||||
app.chat_widget.on_commit_tick();
|
||||
AppRunControl::Continue
|
||||
}
|
||||
};
|
||||
if App::should_stop_waiting_for_initial_session(
|
||||
waiting_for_initial_session_configured,
|
||||
|
||||
@@ -46,7 +46,7 @@ pub(super) async fn make_test_app() -> App {
|
||||
enhanced_keys_supported: false,
|
||||
keymap: crate::keymap::RuntimeKeymap::defaults(),
|
||||
key_chord_matcher: crate::keymap::KeyChordMatcher::default(),
|
||||
commit_anim_running: Arc::new(AtomicBool::new(false)),
|
||||
commit_animation: None,
|
||||
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
skill_load_warnings: SkillLoadWarningState::default(),
|
||||
|
||||
@@ -22,6 +22,8 @@ mod safety_buffering;
|
||||
mod session_lifecycle_requests;
|
||||
mod session_summary;
|
||||
mod startup;
|
||||
#[path = "tests/stream_animation_tests.rs"]
|
||||
mod stream_animation_tests;
|
||||
#[path = "tests/thread_usage.rs"]
|
||||
mod thread_usage;
|
||||
#[path = "tests/turn_submission.rs"]
|
||||
@@ -5433,7 +5435,7 @@ async fn make_test_app() -> App {
|
||||
enhanced_keys_supported: false,
|
||||
keymap: crate::keymap::RuntimeKeymap::defaults(),
|
||||
key_chord_matcher: crate::keymap::KeyChordMatcher::default(),
|
||||
commit_anim_running: Arc::new(AtomicBool::new(false)),
|
||||
commit_animation: None,
|
||||
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
skill_load_warnings: SkillLoadWarningState::default(),
|
||||
@@ -5513,7 +5515,7 @@ async fn make_test_app_with_channels() -> (
|
||||
enhanced_keys_supported: false,
|
||||
keymap: crate::keymap::RuntimeKeymap::defaults(),
|
||||
key_chord_matcher: crate::keymap::KeyChordMatcher::default(),
|
||||
commit_anim_running: Arc::new(AtomicBool::new(false)),
|
||||
commit_animation: None,
|
||||
status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)),
|
||||
skill_load_warnings: SkillLoadWarningState::default(),
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
use super::*;
|
||||
use futures::FutureExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
async fn stream_animation_preserves_pacing_and_discards_stopped_ticks() -> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let mut app_server = start_config_write_test_app_server(&app).await?;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
let (replacement, _tx, _rx, _ops) = make_chatwidget_manual_with_sender().await;
|
||||
time::pause();
|
||||
let started_at = time::Instant::now();
|
||||
|
||||
app.handle_event(&mut tui, &mut app_server, AppEvent::StartCommitAnimation)
|
||||
.await?;
|
||||
time::advance(COMMIT_ANIMATION_TICK / 2).await;
|
||||
app.handle_event(&mut tui, &mut app_server, AppEvent::StartCommitAnimation)
|
||||
.await?;
|
||||
time::advance(COMMIT_ANIMATION_TICK).await;
|
||||
let interval = app.commit_animation.as_mut().expect("animation started");
|
||||
assert_eq!(
|
||||
interval.tick().now_or_never(),
|
||||
Some(started_at + COMMIT_ANIMATION_TICK)
|
||||
);
|
||||
|
||||
// A blocked UI consumes one overdue tick, not a burst of obsolete ticks.
|
||||
time::advance(COMMIT_ANIMATION_TICK * 4).await;
|
||||
assert!(interval.tick().now_or_never().is_some());
|
||||
assert!(interval.tick().now_or_never().is_none());
|
||||
let next_tick = time::Instant::now() + COMMIT_ANIMATION_TICK;
|
||||
time::advance(COMMIT_ANIMATION_TICK + Duration::from_millis(/*millis*/ 1)).await;
|
||||
assert_eq!(interval.tick().now_or_never(), Some(next_tick));
|
||||
|
||||
// Restart while the previous timer is overdue; only the new deadline survives.
|
||||
time::advance(COMMIT_ANIMATION_TICK * 3).await;
|
||||
app.handle_event(&mut tui, &mut app_server, AppEvent::StopCommitAnimation)
|
||||
.await?;
|
||||
assert!(app.commit_animation.is_none());
|
||||
app.handle_event(&mut tui, &mut app_server, AppEvent::StartCommitAnimation)
|
||||
.await?;
|
||||
let interval = app.commit_animation.as_mut().expect("animation restarted");
|
||||
assert!(interval.tick().now_or_never().is_none());
|
||||
let next_tick = time::Instant::now() + COMMIT_ANIMATION_TICK;
|
||||
time::advance(COMMIT_ANIMATION_TICK + Duration::from_millis(/*millis*/ 1)).await;
|
||||
assert_eq!(interval.tick().now_or_never(), Some(next_tick));
|
||||
|
||||
app.replace_chat_widget(replacement);
|
||||
assert!(app.commit_animation.is_none());
|
||||
time::resume();
|
||||
Ok(())
|
||||
}
|
||||
@@ -941,7 +941,6 @@ pub(crate) enum AppEvent {
|
||||
|
||||
StartCommitAnimation,
|
||||
StopCommitAnimation,
|
||||
CommitTick,
|
||||
|
||||
/// Update the current reasoning effort in the running app and widget.
|
||||
UpdateReasoningEffort(Option<ReasoningEffort>),
|
||||
|
||||
@@ -123,6 +123,20 @@ pub(crate) fn log_inbound_app_event(event: &AppEvent) {
|
||||
log_inbound_app_event_with(&LOGGER, event);
|
||||
}
|
||||
|
||||
/// Keep the session-log format even though ticks no longer use the app-event queue.
|
||||
pub(crate) fn log_commit_tick() {
|
||||
if !LOGGER.is_enabled() {
|
||||
return;
|
||||
}
|
||||
let value = json!({
|
||||
"ts": now_ts(),
|
||||
"dir": "to_tui",
|
||||
"kind": "app_event",
|
||||
"variant": "CommitTick",
|
||||
});
|
||||
LOGGER.write_json_line(value);
|
||||
}
|
||||
|
||||
fn log_inbound_app_event_with(logger: &SessionLogger, event: &AppEvent) {
|
||||
// Log only if enabled
|
||||
if !logger.is_enabled() {
|
||||
|
||||
Reference in New Issue
Block a user