From a62c53a77eb901a2736c0dade0a71c5725ccf9aa Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Tue, 23 Jun 2026 23:41:35 +0700 Subject: [PATCH] feat(tui): improve multi-target lifecycle management and UI clarity Refine the TUI event loop to explicitly handle different exit conditions and ensure sessions stop correctly on idle timeouts or interruptions. Update UI labels to clarify when metrics or headers refer specifically to the first target in multi-target sessions. --- crates/irtt-cli/src/cmd/tui/run.rs | 94 +++++++++++++++++++++++-- crates/irtt-cli/src/cmd/tui/ui.rs | 109 ++++++++++++++++++++++++++--- 2 files changed, 188 insertions(+), 15 deletions(-) diff --git a/crates/irtt-cli/src/cmd/tui/run.rs b/crates/irtt-cli/src/cmd/tui/run.rs index d89ad34..27ce0a3 100644 --- a/crates/irtt-cli/src/cmd/tui/run.rs +++ b/crates/irtt-cli/src/cmd/tui/run.rs @@ -169,7 +169,7 @@ fn run_group_tui( let mut saw_target_event = false; let mut last_event_at = Instant::now(); - loop { + let exit = loop { if is_shutdown_requested(shutdown_requested) { interrupted = true; session.stop(); @@ -195,19 +195,25 @@ fn run_group_tui( } Ok(None) => { if interrupted { - break; + break GroupLoopExit::Interrupted; } if terminal_targets.len() >= expected_target_count { - break; + break GroupLoopExit::AllTargetsTerminal; } if should_join_group_after_idle(&args, saw_target_event, last_event_at) { - break; + break GroupLoopExit::IdleGraceElapsed; } wait_for_tui_activity(None, next_render, state, terminal, shutdown_requested)?; thread::sleep(IDLE_SLEEP); } - Err(EventSubscriptionError::Disconnected) => break, + Err(EventSubscriptionError::Disconnected) => { + break GroupLoopExit::SubscriptionDisconnected + } } + }; + + if exit.should_stop_before_join() { + session.stop(); } if interrupted { @@ -224,7 +230,17 @@ fn run_group_tui( } if outcome.end_reason == ManagedGroupEndReason::Cancelled && !interrupted { - return Err("managed client group was cancelled".into()); + return match exit { + GroupLoopExit::IdleGraceElapsed => { + Err("managed client group stayed idle before all targets completed".into()) + } + GroupLoopExit::SubscriptionDisconnected => { + Err("managed client group event subscription disconnected before completion".into()) + } + GroupLoopExit::Interrupted | GroupLoopExit::AllTargetsTerminal => { + Err("managed client group was cancelled".into()) + } + }; } state.set_status(TuiStatus::Complete); @@ -232,6 +248,23 @@ fn run_group_tui( Ok(()) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum GroupLoopExit { + Interrupted, + AllTargetsTerminal, + IdleGraceElapsed, + SubscriptionDisconnected, +} + +impl GroupLoopExit { + fn should_stop_before_join(self) -> bool { + matches!( + self, + Self::Interrupted | Self::IdleGraceElapsed | Self::SubscriptionDisconnected + ) + } +} + fn is_terminal_target_event(event: &ClientEvent) -> bool { matches!( event, @@ -348,3 +381,52 @@ fn tui_wait_duration( }; send_wait.min(render_wait).min(TUI_WAIT_SLICE) } + +#[cfg(test)] +mod tests { + use super::*; + use clap::Parser; + + fn parse(args: &[&str]) -> TuiArgs { + let mut argv = vec!["irtt-tui"]; + argv.extend_from_slice(args); + TuiArgs::try_parse_from(argv).unwrap() + } + + #[test] + fn continuous_multi_target_idle_does_not_stop_after_events() { + let args = parse(&["127.0.0.1:2112", "127.0.0.2:2112"]); + let old_event_at = Instant::now() - estimated_group_completion_grace(&args) - IDLE_SLEEP; + + assert!(!should_join_group_after_idle(&args, true, old_event_at)); + } + + #[test] + fn finite_or_unopened_group_can_leave_after_idle_grace() { + let finite = parse(&["--duration", "1s", "127.0.0.1:2112", "127.0.0.2:2112"]); + let finite_old_event_at = + Instant::now() - estimated_group_completion_grace(&finite) - IDLE_SLEEP; + assert!(should_join_group_after_idle( + &finite, + true, + finite_old_event_at + )); + + let continuous = parse(&["127.0.0.1:2112", "127.0.0.2:2112"]); + let unopened_old_event_at = + Instant::now() - estimated_group_completion_grace(&continuous) - IDLE_SLEEP; + assert!(should_join_group_after_idle( + &continuous, + false, + unopened_old_event_at + )); + } + + #[test] + fn protective_group_exits_stop_before_joining() { + assert!(GroupLoopExit::Interrupted.should_stop_before_join()); + assert!(GroupLoopExit::IdleGraceElapsed.should_stop_before_join()); + assert!(GroupLoopExit::SubscriptionDisconnected.should_stop_before_join()); + assert!(!GroupLoopExit::AllTargetsTerminal.should_stop_before_join()); + } +} diff --git a/crates/irtt-cli/src/cmd/tui/ui.rs b/crates/irtt-cli/src/cmd/tui/ui.rs index 4623e6c..3d223ab 100644 --- a/crates/irtt-cli/src/cmd/tui/ui.rs +++ b/crates/irtt-cli/src/cmd/tui/ui.rs @@ -642,7 +642,7 @@ fn draw_large(frame: &mut Frame<'_>, area: Rect, state: &TuiState, snapshot: &Sn frame.render_widget(header(state, HeaderDensity::Large), top[0]); frame.render_widget(packet_panel(state, snapshot, top[1].height), top[1]); - frame.render_widget(timing_panel(snapshot), middle[0]); + frame.render_widget(timing_panel(state, snapshot), middle[0]); render_graph_area(frame, middle[1], state); frame.render_widget(recent_events_panel(state, bottom[0].height), bottom[0]); frame.render_widget(sample_panel(state, snapshot), bottom[1]); @@ -683,6 +683,9 @@ enum HeaderDensity { fn header(state: &TuiState, density: HeaderDensity) -> Paragraph<'_> { let selected = state.selected_target(); + let selected_label = selected + .map(|target| target.label.as_str()) + .unwrap_or("target"); let remote = selected .and_then(|target| target.remote.as_deref()) .unwrap_or("-"); @@ -711,7 +714,11 @@ fn header(state: &TuiState, density: HeaderDensity) -> Paragraph<'_> { state.status.label() )), ]), - Line::from(format!("remote: {remote}")), + Line::from(if state.is_multi_target() { + format!("first target: {selected_label} remote: {remote}") + } else { + format!("remote: {remote}") + }), ]; match density { HeaderDensity::Large => { @@ -745,8 +752,11 @@ fn header(state: &TuiState, density: HeaderDensity) -> Paragraph<'_> { } fn full_graph_header(state: &TuiState, snapshot: &Snapshot) -> Paragraph<'static> { - let remote = state - .selected_target() + let selected = state.selected_target(); + let selected_label = selected + .map(|target| target.label.as_str()) + .unwrap_or("target"); + let remote = selected .and_then(|target| target.remote.as_deref()) .unwrap_or("-"); let elapsed = format_duration(state.started_at.elapsed()); @@ -757,13 +767,27 @@ fn full_graph_header(state: &TuiState, snapshot: &Snapshot) -> Paragraph<'static .map(|sample| format_ns_i128(Some(sample.effective_ns))) .unwrap_or_else(|| "-".to_owned()); + let target_context = if state.is_multi_target() { + format!("first target {selected_label} remote {remote}") + } else { + format!("remote {remote}") + }; + Paragraph::new(Line::from(format!( - "irtt-rs {} remote {remote} elapsed {elapsed} sent {} replies {} last {last}", + "irtt-rs {} {target_context} elapsed {elapsed} sent {} replies {} last {last}", state.status.label(), format_count(packets.packets_sent), format_count(packets.unique_replies) ))) - .block(Block::default().title("session").borders(Borders::ALL)) + .block( + Block::default() + .title(if state.is_multi_target() { + "session - first target" + } else { + "session" + }) + .borders(Borders::ALL), + ) } fn packet_panel(state: &TuiState, snapshot: &Snapshot, panel_height: u16) -> Paragraph<'static> { @@ -852,7 +876,7 @@ fn target_table_panel(state: &TuiState, panel_height: u16) -> Paragraph<'static> .wrap(Wrap { trim: false }) } -fn timing_panel(snapshot: &Snapshot) -> Paragraph<'_> { +fn timing_panel(state: &TuiState, snapshot: &Snapshot) -> Paragraph<'static> { let mut lines = vec![Line::from(format!( "{:<18} {:>5} {:>9} {:>9} {:>9} {:>9}", "metric", "n", "min", "mean", "max", "stddev" @@ -878,7 +902,15 @@ fn timing_panel(snapshot: &Snapshot) -> Paragraph<'_> { push_time_line(&mut lines, "timer error", &snapshot.timer_error); Paragraph::new(lines) - .block(Block::default().title("timing").borders(Borders::ALL)) + .block( + Block::default() + .title(if state.is_multi_target() { + "timing - first target" + } else { + "timing" + }) + .borders(Borders::ALL), + ) .wrap(Wrap { trim: false }) } @@ -1291,7 +1323,15 @@ fn sample_panel(state: &TuiState, snapshot: &Snapshot) -> Paragraph<'static> { )), Line::from(format!("last warning: {warning}")), ]) - .block(Block::default().title("sample").borders(Borders::ALL)) + .block( + Block::default() + .title(if state.is_multi_target() { + "sample - first target" + } else { + "sample" + }) + .borders(Borders::ALL), + ) .wrap(Wrap { trim: true }) } @@ -1771,6 +1811,57 @@ mod tests { .is_some_and(|warning| warning.contains("WrongToken"))); } + #[test] + fn multi_target_global_status_waits_for_all_targets_terminal() { + let mut state = + TuiState::with_target_labels(TuiConfig::default(), ["a".to_owned(), "b".to_owned()]); + + state.process_target_event(&target_event( + "a", + ClientEvent::NoTestCompleted { + remote: remote(), + negotiated: NegotiatedParams { + params: irtt_proto::Params::default(), + restrictions: Vec::new(), + }, + at: ts(Duration::ZERO), + }, + )); + + assert_eq!(state.targets[0].status, TargetStatus::NoTest); + assert_eq!(state.targets[1].status, TargetStatus::Opening); + assert_eq!(state.status, TuiStatus::Running); + + state.process_target_event(&target_event( + "b", + ClientEvent::SessionClosed { + remote: remote(), + token: 0xabc, + at: ts(Duration::ZERO), + }, + )); + + assert_eq!(state.targets[1].status, TargetStatus::Closed); + assert_eq!(state.status, TuiStatus::Complete); + } + + #[test] + fn reset_clears_graph_history_for_all_targets() { + let mut state = + TuiState::with_target_labels(TuiConfig::default(), ["a".to_owned(), "b".to_owned()]); + + state.process_target_event(&target_event("a", reply(1, 1_000_000))); + state.process_target_event(&target_event("b", reply(2, 2_000_000))); + state.clear_visible_history(); + + assert!(state.targets[0].graph_history.is_empty()); + assert!(state.targets[1].graph_history.is_empty()); + assert_eq!( + state.recent_events.back().map(String::as_str), + Some("visible graph history reset") + ); + } + #[test] fn warning_updates_recent_events_and_last_warning() { let mut state = TuiState::default();