From d75cfe9c730805d72b6bd518a3e9823129a5131b Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 09:55:08 +0000 Subject: [PATCH 01/36] fix(tui): unify dialog behavior across tabs and overlays Bundles five small UX fixes to the dashboard TUI: - Esc opens the main menu uniformly on every idle tab, including Chat (previously a dead key there). - The approval modal now defaults to Deny instead of Approve. - Confirmed the approval dialog's dim backdrop already covers the focused/bare-launcher render path (no chat backend runs there, so the dialog can never appear undimmed); no code change needed. - The contextual '?' help screen now matches the global Esc-menu help screen's chrome (dimmed backdrop, grouped two-column layout, popup sizing), while keeping its own per-tab content. - The footer always shows exactly one Esc chip: 'back out' when a manager overlay is open, otherwise a clickable 'menu' fallback so the Esc-opens-menu behavior is discoverable at a glance. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 35 ++++++++------ crates/rocm-dash-tui/src/ui/approval.rs | 2 +- crates/rocm-dash-tui/src/ui/mod.rs | 51 +++++++++++++++----- crates/rocm-dash-tui/src/ui/modal.rs | 63 +++++++++++++++---------- 4 files changed, 99 insertions(+), 52 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index f046c0b80..9acc78b67 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1332,7 +1332,7 @@ impl AppState { self.close_overlays(); self.approval = Some(PendingApproval { req: crate::ui::approval::ApprovalRequest::new(intent.title, intent.body), - choice: crate::ui::approval::ApprovalChoice::Approve, + choice: crate::ui::approval::ApprovalChoice::default(), name: intent.name, arguments: intent.arguments, }); @@ -3342,15 +3342,14 @@ fn handle_key(k: KeyEvent, current: ActiveTab, modal: &Modal, chat: ChatKeyCtx) } match k.code { KeyCode::Char('q') => KeyAction::Quit, - // Esc opens the main menu when idle, except on Chat (where Esc keeps its - // existing chat meaning) — managers/approval are routed upstream. + // Esc opens the main menu when idle — managers/approval are routed + // upstream, and Chat-focused Esc is handled by the short-circuit above. // On ROCm/Serving, Esc first steps out of the detail pane (resolved // against focus in `apply_action`); elsewhere it opens the main menu. KeyCode::Esc if matches!(current, ActiveTab::Rocm | ActiveTab::Serving) => { KeyAction::PaneEscape } - KeyCode::Esc if current != ActiveTab::Chat => KeyAction::OpenMenu, - KeyCode::Esc => KeyAction::Nothing, + KeyCode::Esc => KeyAction::OpenMenu, KeyCode::Char(':') => KeyAction::OpenPalette, KeyCode::Char('?') => KeyAction::ToggleHelp, KeyCode::Char('t') => KeyAction::OpenThemePicker, @@ -3510,7 +3509,7 @@ mod tests { #[test] fn q_quits_esc_does_not() { assert_eq!(hk(KeyCode::Char('q'), ActiveTab::Home), KeyAction::Quit); - // P4: Esc opens the main menu (it never quits); Chat keeps its own Esc. + // P4: Esc opens the main menu (it never quits). assert_eq!(hk(KeyCode::Esc, ActiveTab::Observe), KeyAction::OpenMenu); } @@ -4332,12 +4331,12 @@ mod tests { } #[test] - fn esc_opens_menu_when_idle_but_not_on_chat() { - // Idle (non-Chat) tabs: Esc opens the btop main menu. + fn esc_opens_menu_when_idle_on_any_tab() { + // Idle tabs: Esc opens the btop main menu, Chat included when unfocused + // (Chat-focused Esc is handled by the short-circuit above this match). assert_eq!(hk(KeyCode::Esc, ActiveTab::Home), KeyAction::OpenMenu); assert_eq!(hk(KeyCode::Esc, ActiveTab::Observe), KeyAction::OpenMenu); - // Chat keeps its existing Esc meaning (no menu). - assert_eq!(hk(KeyCode::Esc, ActiveTab::Chat), KeyAction::Nothing); + assert_eq!(hk(KeyCode::Esc, ActiveTab::Chat), KeyAction::OpenMenu); // While an overlay modal owns the screen, Esc closes it (not OpenMenu). assert_eq!( handle_key( @@ -6471,7 +6470,8 @@ mod tests { name: "install_sdk".to_string(), arguments: serde_json::json!({ "channel": "release", "format": "wheel" }), }); - // Enter on the default (Approve) choice yields an Approve verdict. + // The modal defaults to Deny (item #16); move to Approve, then confirm. + s.on_approval_key(crossterm::event::KeyCode::Tab); let verdict = s.on_approval_key(crossterm::event::KeyCode::Enter); assert_eq!(verdict, Some(crate::ui::approval::ApprovalVerdict::Approve)); let (name, args) = s.take_approval().expect("approval taken on approve"); @@ -6598,16 +6598,21 @@ mod tests { name: "install_sdk".to_string(), arguments: serde_json::json!({}), }); - // Tab toggles the cursor to Deny without producing a verdict. - assert_eq!(s.on_approval_key(crossterm::event::KeyCode::Tab), None); + // Defaults to Deny (the safer default; see item #16). assert_eq!( s.approval.as_ref().unwrap().choice, crate::ui::approval::ApprovalChoice::Deny ); - // Enter now confirms Deny. + // Tab toggles the cursor to Approve without producing a verdict. + assert_eq!(s.on_approval_key(crossterm::event::KeyCode::Tab), None); + assert_eq!( + s.approval.as_ref().unwrap().choice, + crate::ui::approval::ApprovalChoice::Approve + ); + // Enter now confirms Approve. assert_eq!( s.on_approval_key(crossterm::event::KeyCode::Enter), - Some(crate::ui::approval::ApprovalVerdict::Deny) + Some(crate::ui::approval::ApprovalVerdict::Approve) ); } diff --git a/crates/rocm-dash-tui/src/ui/approval.rs b/crates/rocm-dash-tui/src/ui/approval.rs index e2cc12402..f770d208d 100644 --- a/crates/rocm-dash-tui/src/ui/approval.rs +++ b/crates/rocm-dash-tui/src/ui/approval.rs @@ -49,8 +49,8 @@ impl ApprovalRequest { /// Which button the cursor is on. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum ApprovalChoice { - #[default] Approve, + #[default] Deny, } diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 1bb16227d..ca86822e4 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -421,21 +421,27 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve ]; // On a domain tab with a manager open inline, the pane keys route to the // manager — advertise the back-out instead of the (now wrong) select/open. + // Exactly one Esc chip is shown at all times: "back out" when an overlay + // is open, otherwise the uniform fallback "menu" (item #35). if is_action_tab && state.has_open_overlay() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); - } else if matches!( - state.active_tab, - ActiveTab::Observe | ActiveTab::Rocm | ActiveTab::Serving - ) { - segs.push(Seg::Key("j/k", Some(KeyAction::Move(1)))); - segs.push(Seg::Sep(" select ")); - segs.push(Seg::Key("Enter", Some(enter_action))); - segs.push(Seg::Sep(if is_action_tab { - " open " - } else { - " detail " - })); + } else { + segs.push(Seg::Key("Esc", Some(KeyAction::OpenMenu))); + segs.push(Seg::Sep(" menu ")); + if matches!( + state.active_tab, + ActiveTab::Observe | ActiveTab::Rocm | ActiveTab::Serving + ) { + segs.push(Seg::Key("j/k", Some(KeyAction::Move(1)))); + segs.push(Seg::Sep(" select ")); + segs.push(Seg::Key("Enter", Some(enter_action))); + segs.push(Seg::Sep(if is_action_tab { + " open " + } else { + " detail " + })); + } } // Guided-action letter hotkeys — Observe only (telemetry quick-jumps). On // ROCm/Serving the Actions list is the single path, so no letter chips. @@ -552,4 +558,25 @@ mod tests { fn narrow_body_has_no_triptych() { assert!(wide_triptych(Rect::new(0, 0, 100, 40)).is_none()); } + + #[test] + fn footer_shows_esc_menu_chip_when_no_overlay_open() { + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let theme = Theme::from_name("default-dark"); + let state = AppState::new("t".into(), "default-dark".into()); + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + let mut chips = Vec::new(); + term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) + .unwrap(); + + let esc = chips + .iter() + .find(|c| c.action == KeyAction::OpenMenu) + .expect("a fallback Esc chip opening the menu must always be present"); + let _ = esc; + } } diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index c30587589..b88c38f50 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -79,22 +79,44 @@ pub fn draw_scrollable_lines( f.render_widget(p, inner); } -/// Render the Help modal for the active tab. +/// Render the Help modal for the active tab. Shares chrome (dimmed backdrop, +/// popup geometry, 2-column grouped layout) with `draw_global_help` so the +/// two help screens read as one family; unlike that screen, this one's right +/// column is dynamic — the active tab's own keys. pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { - let popup = centered_rect(70, 70, 80, 22, area); + grey_overlay(f); + let popup = centered_rect(80, 80, 100, 26, area); let inner = draw_popup_frame(f, popup, "Help", theme); + if inner.height == 0 { + return; + } + f.render_widget(Clear, inner); + let cols = Layout::default() + .direction(Direction::Horizontal) + .constraints([Constraint::Percentage(50), Constraint::Percentage(50)]) + .split(inner); - let mut lines: Vec = vec![ - key_line("q", "quit", theme), - key_line("?", "toggle this help", theme), - key_line("Tab / Shift-Tab", "next / previous tab", theme), - key_line("1 .. 5", "jump to tab", theme), - key_line("t", "open theme picker", theme), - key_line("Space", "pause / resume (replay only)", theme), - key_line("+ / -", "speed up / slow down (replay only)", theme), - key_line("[ / ]", "jump ±10s (replay only)", theme), - key_line("{ / }", "jump ±60s (replay only)", theme), - Line::raw(""), + let left: &[(&str, &[(&str, &str)])] = &[ + ( + "GLOBAL", + &[ + ("q", "quit"), + ("?", "toggle this help"), + ("Tab / Shift-Tab", "next / previous tab"), + ("1 .. 5", "jump to tab"), + ("t", "open theme picker"), + ("Esc", "open the main menu"), + ], + ), + ( + "REPLAY", + &[ + ("Space", "pause / resume"), + ("+ / -", "speed up / slow down"), + ("[ / ]", "jump ±10s"), + ("{ / }", "jump ±60s"), + ], + ), ]; let tab_help: &[(&str, &str)] = match tab { ActiveTab::Home => &[("(no tab-specific keys — see the ROCm / Serving tabs)", "")], @@ -130,18 +152,11 @@ pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { ("Backspace", "delete a character (while focused)"), ], }; - lines.push(Line::from(Span::styled( - format!("— {tab:?} tab —"), - Style::default() - .fg(theme.muted) - .add_modifier(Modifier::BOLD), - ))); - for (k, desc) in tab_help { - lines.push(key_line(k, desc, theme)); - } + let tab_title = format!("{tab:?}").to_uppercase(); + let right: &[(&str, &[(&str, &str)])] = &[(tab_title.as_str(), tab_help)]; - let p = Paragraph::new(lines).wrap(Wrap { trim: false }); - f.render_widget(p, inner); + render_help_groups(f, cols[0], left, theme); + render_help_groups(f, cols[1], right, theme); } fn key_line<'a>(key: &'a str, desc: &'a str, theme: &Theme) -> Line<'a> { From b6abbaa8eb685970f5830fed390e2a1feb3cdccc Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 10:46:31 +0000 Subject: [PATCH 02/36] fix(tui): address review findings on dialog enhancements - Decouple ApprovalChoice::default() (Approve, used by 10 manager flows) from the chat tool-call approval, which now sets Deny explicitly at its own call site instead of moving the shared default. - Widen should_pane_back_out to any tab, not just Rocm/Serving, so Esc can close a manager overlay opened from a non-domain tab (e.g. examine_manager from Observe) instead of falling through to OpenMenu while the overlay keeps rendering on top. - Match the footer's Esc chip gate to the widened should_pane_back_out so the 'back out' vs 'menu' label and select/detail hints stay consistent with the new behavior. - Fix draw_help's doc comment to satisfy clippy::too_long_first_doc_paragraph. - Update the dash-05 e2e assertion to match draw_help's actual title format (uppercased tab name, e.g. HOME) instead of the older 'Home tab' string. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 27 ++++++++++++++-------- crates/rocm-dash-tui/src/ui/approval.rs | 10 +++++++- crates/rocm-dash-tui/src/ui/mod.rs | 11 +++++---- crates/rocm-dash-tui/src/ui/modal.rs | 10 ++++---- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 2 +- 5 files changed, 40 insertions(+), 20 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 9acc78b67..2e7a6b98f 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1037,11 +1037,18 @@ impl AppState { } /// Whether an `Esc` keypress should back out of an inline manager: true on - /// ROCm/Serving while a manager overlay is open AND that manager is at its - /// root screen. The event loop closes the manager and returns focus to the + /// any tab while a manager overlay is open AND that manager is at its root + /// screen. The event loop closes the manager and returns focus to the /// Actions list when this holds. Pure read so it is unit-testable (the /// mutation lives in the event-loop arm). /// + /// Not just ROCm/Serving: a manager can be opened from a non-domain tab + /// (e.g. `examine_manager` from an Observe hotkey), and Esc must be able to + /// close it there too — otherwise it falls through to the global `OpenMenu` + /// arm while the manager overlay keeps rendering on top, leaving `Modal` + /// set but invisible. `pane_focus` is meaningless outside Rocm/Serving, so + /// resetting it there is a harmless no-op. + /// /// When the manager has a sub-popup / approval / job console open, this is /// `false` so Esc falls through to the manager's own handler (cancel the /// sub-layer, dismiss a terminal console, or be ignored while a job runs) — @@ -1052,8 +1059,7 @@ impl AppState { /// returns focus from the Details preview to the Actions list via the normal /// `PaneFocusActions` key path. pub(crate) fn should_pane_back_out(&self, code: crossterm::event::KeyCode) -> bool { - matches!(self.active_tab, ActiveTab::Rocm | ActiveTab::Serving) - && self.has_open_overlay() + self.has_open_overlay() && self.active_overlay_at_root() && matches!(code, crossterm::event::KeyCode::Esc) } @@ -1332,7 +1338,9 @@ impl AppState { self.close_overlays(); self.approval = Some(PendingApproval { req: crate::ui::approval::ApprovalRequest::new(intent.title, intent.body), - choice: crate::ui::approval::ApprovalChoice::default(), + // An unreviewed tool call the model wants to run defaults to Deny, + // unlike the shared `ApprovalChoice` default (see its doc comment). + choice: crate::ui::approval::ApprovalChoice::Deny, name: intent.name, arguments: intent.arguments, }); @@ -3655,17 +3663,18 @@ mod tests { } #[test] - fn back_out_only_on_domain_tabs_with_a_manager() { + fn back_out_requires_an_open_manager_on_any_tab() { let mut s = AppState::new("t".into(), "default-dark".into()); // No manager open → never backs out, even on a domain tab. s.active_tab = ActiveTab::Rocm; assert!(!s.should_pane_back_out(crossterm::event::KeyCode::Esc)); - // Manager open but on a non-domain tab (opened from Observe hotkey) → - // the manager keeps its own Esc handling; no domain back-out. + // Manager open on a non-domain tab (opened from Observe hotkey) → + // Esc still backs out, closing the manager (item #35: no dead corner + // where an overlay survives a tab switch and swallows Esc silently). s.active_tab = ActiveTab::Observe; s.examine_manager = Some(crate::ui::examine_manager::ExamineManagerState::default()); assert!(s.has_open_overlay()); - assert!(!s.should_pane_back_out(crossterm::event::KeyCode::Esc)); + assert!(s.should_pane_back_out(crossterm::event::KeyCode::Esc)); } #[test] diff --git a/crates/rocm-dash-tui/src/ui/approval.rs b/crates/rocm-dash-tui/src/ui/approval.rs index f770d208d..7f167ddc4 100644 --- a/crates/rocm-dash-tui/src/ui/approval.rs +++ b/crates/rocm-dash-tui/src/ui/approval.rs @@ -47,10 +47,18 @@ impl ApprovalRequest { } /// Which button the cursor is on. +/// +/// `Approve` is the shared default — it governs the ten manager-embedded +/// approval flows (install/stop/reinstall/etc., each via their own +/// `ApprovalChoice::default()` call) where the user just explicitly +/// requested the action being confirmed. The chat/tool-call approval flow +/// (`AppState::open_approval`) is a different case — an unreviewed tool +/// call the model wants to run — and opts into `Deny` explicitly there +/// instead of moving this shared default. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum ApprovalChoice { - Approve, #[default] + Approve, Deny, } diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index ca86822e4..068fb90fa 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -419,11 +419,12 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve Seg::Key("1–5", None), Seg::Sep(" jump "), ]; - // On a domain tab with a manager open inline, the pane keys route to the - // manager — advertise the back-out instead of the (now wrong) select/open. - // Exactly one Esc chip is shown at all times: "back out" when an overlay - // is open, otherwise the uniform fallback "menu" (item #35). - if is_action_tab && state.has_open_overlay() { + // With a manager overlay open (any tab), Esc backs it out rather than + // opening the menu — advertise that instead of the (now wrong) select/open + // hints, which route to the manager, not the pane. Exactly one Esc chip is + // shown at all times: "back out" when an overlay is open, otherwise the + // uniform fallback "menu" (item #35). + if state.has_open_overlay() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); } else { diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index b88c38f50..00f8a8e06 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -79,10 +79,12 @@ pub fn draw_scrollable_lines( f.render_widget(p, inner); } -/// Render the Help modal for the active tab. Shares chrome (dimmed backdrop, -/// popup geometry, 2-column grouped layout) with `draw_global_help` so the -/// two help screens read as one family; unlike that screen, this one's right -/// column is dynamic — the active tab's own keys. +/// Render the Help modal for the active tab. +/// +/// Shares chrome (dimmed backdrop, popup geometry, 2-column grouped layout) +/// with `draw_global_help` so the two help screens read as one family; unlike +/// that screen, this one's right column is dynamic — the active tab's own +/// keys. pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { grey_overlay(f); let popup = centered_rect(80, 80, 100, 26, area); diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 49518724a..668d57124 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -368,7 +368,7 @@ async fn navigation_guidance_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("dashboard help did not appear: {e}")); let screen = tui.screen_text(); assert!( - screen.contains("next / previous tab") && screen.contains("Home tab"), + screen.contains("next / previous tab") && screen.contains("HOME"), "navigation or contextual guidance missing:\n{screen}" ); } From 955b713bfd9cb2bbde95564815c8b698989ddfa4 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 11:24:34 +0000 Subject: [PATCH 03/36] fix(tui): dim the backdrop behind the theme picker and instance detail Every other popup (Help, Esc menu, palette, Options, manager overlays, approval) already dims the backdrop via grey_overlay. The theme picker and the Observe instance-detail overlay were the two remaining popups rendering on an undimmed background, an inconsistency in dialog UX noticed while verifying the earlier dialog-enhancements fixes. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/modal.rs | 1 + crates/rocm-dash-tui/src/ui/tabs/instances.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index 00f8a8e06..553b0a041 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -185,6 +185,7 @@ pub fn draw_theme_picker( current_name: &str, active_theme: &Theme, ) { + grey_overlay(f); let popup = centered_rect(80, 80, 110, 30, area); let inner = draw_popup_frame( f, diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 5f47a887d..f4bddf392 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -16,7 +16,7 @@ use rocm_dash_core::metrics::{Instance, InstanceStatus}; use crate::app::{AppState, ConnState, KeyAction}; use crate::ui::format; -use crate::ui::modal::{centered_rect, draw_popup_frame}; +use crate::ui::modal::{centered_rect, draw_popup_frame, grey_overlay}; use crate::ui::panel::{self, BoxRole}; use crate::ui::theme::Theme; use crate::ui::widgets::trunc; @@ -562,6 +562,7 @@ const fn point_in_rect(r: Rect, x: u16, y: u16) -> bool { } pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { + grey_overlay(f); let popup = centered_rect(85, 85, 120, 36, area); if state.instances.is_empty() { From d6e8c36a5b6cb9083892749dc2edfb9d1e9a6b69 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 12:46:12 +0000 Subject: [PATCH 04/36] fix(tui): make Help scrollable and surface mock tool approvals Help/GlobalHelp overlays now scroll (j/k, PageUp/Down, g/G) instead of silently clipping content at small terminal sizes; the offset lives in help_scroll and resets whenever either modal opens. The footer's Esc chip now mirrors the actual key-routing priority (approval cancel > overlay back-out > modal close > chat-detect dismiss > chat unfocus > menu fallback) instead of collapsing to a single overlay-vs-menu check, so it never advertises "menu" while Esc would really do something else. MockAgentClient gained a trigger-phrase path that synthesizes a ChatApprovalRequired tool call, so the chat approval flow is exercisable without a live agent backend. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/agent.rs | 56 +++++++++ crates/rocm-dash-tui/src/app/mod.rs | 112 ++++++++++++++++-- crates/rocm-dash-tui/src/ui/mod.rs | 36 ++++-- crates/rocm-dash-tui/src/ui/modal.rs | 109 +++++++---------- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 1 + 5 files changed, 231 insertions(+), 83 deletions(-) diff --git a/crates/rocm-dash-tui/src/agent.rs b/crates/rocm-dash-tui/src/agent.rs index 712386d0e..4e690da14 100644 --- a/crates/rocm-dash-tui/src/agent.rs +++ b/crates/rocm-dash-tui/src/agent.rs @@ -1511,12 +1511,25 @@ impl AgentClient for AnthropicAgentClient { } } +/// When the last user turn contains `phrase` (case-insensitive), the mock +/// surfaces `intent` for approval over `tx` — mirroring what a real +/// `rocm_mutating_tool!`'s `call()` does from inside the rig tool loop — instead +/// of returning the normal canned reply. Lets `--chat-mock` drive e2e coverage +/// of the deny-by-default approval modal without a live model or a real +/// [`crate::tool_exec::RocmToolExecutor`]. +struct MockApprovalTrigger { + phrase: String, + intent: crate::tool_exec::ApprovalIntent, + tx: UnboundedSender, +} + /// Deterministic in-memory client for tests and the offline demo. Never touches /// the network. Can emit a canned tool-calling-style answer (cites a Skill). pub struct MockAgentClient { reply: String, fail: bool, cited: Vec, + approval: Option, } impl MockAgentClient { @@ -1526,6 +1539,7 @@ impl MockAgentClient { reply: reply.into(), fail: false, cited: Vec::new(), + approval: None, } } @@ -1536,6 +1550,30 @@ impl MockAgentClient { reply: reply.into(), fail: false, cited: vec![tool_name.into()], + approval: None, + } + } + + /// Like [`Self::with_tool_call`], but when the last user message contains + /// `phrase` (case-insensitive) the mock instead sends `intent` over + /// `approval_tx` as a `ClientMsg::ChatApprovalRequired` and replies with a + /// "surfaced for approval" note — no tool actually executes. + pub fn with_tool_call_and_approval_trigger( + reply: impl Into, + tool_name: impl Into, + phrase: impl Into, + intent: crate::tool_exec::ApprovalIntent, + approval_tx: UnboundedSender, + ) -> Self { + Self { + reply: reply.into(), + fail: false, + cited: vec![tool_name.into()], + approval: Some(MockApprovalTrigger { + phrase: phrase.into().to_lowercase(), + intent, + tx: approval_tx, + }), } } @@ -1545,6 +1583,7 @@ impl MockAgentClient { reply: String::new(), fail: true, cited: Vec::new(), + approval: None, } } } @@ -1562,6 +1601,23 @@ impl AgentClient for MockAgentClient { if history.is_empty() { return Err(AgentError::Empty); } + if let Some(trigger) = &self.approval { + let fires = history + .iter() + .rev() + .find(|t| t.role == ChatRole::User) + .is_some_and(|t| t.content.to_lowercase().contains(&trigger.phrase)); + if fires { + let _ = trigger.tx.send(ClientMsg::ChatApprovalRequired { + intent: trigger.intent.clone(), + }); + return Ok( + "This action needs operator approval; it has been surfaced to \ + the operator." + .to_string(), + ); + } + } Ok(annotate_reply(self.reply.clone(), &self.cited)) } } diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 2e7a6b98f..af36f938c 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -497,6 +497,9 @@ pub struct AppState { pub theme_picker_sel: usize, /// Scroll offset (in lines) inside the Bench Detail modal. Reset on Open. pub bench_detail_scroll: u16, + /// Scroll offset (in lines) inside the Help / GlobalHelp overlays. Both + /// modals are mutually exclusive so one field suffices; reset on open. + pub help_scroll: u16, /// Vertical scroll offset (first visible line) of the active job console. /// Shared by whichever operational manager is showing its console; reset /// when an overlay opens (`close_overlays`). @@ -693,6 +696,7 @@ impl AppState { theme, theme_picker_sel, bench_detail_scroll: 0, + help_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, @@ -1109,6 +1113,20 @@ impl AppState { self.bench_detail_scroll = next; } + /// Reset the Help / GlobalHelp scroll offset (called when opening either + /// modal, so a stale offset never carries over from a previous session). + pub const fn reset_help_scroll(&mut self) { + self.help_scroll = 0; + } + + /// Adjust the Help / GlobalHelp scroll. `delta` is in lines; clamped at 0 + /// (no upper bound — the renderer clamps against the actual line count). + pub fn scroll_help(&mut self, delta: i16) { + let cur = i32::from(self.help_scroll); + let next = u16::try_from((cur + i32::from(delta)).max(0)).unwrap_or(u16::MAX); + self.help_scroll = next; + } + /// Install the resolved chat endpoint and set the initial consent state. /// `None` → `Unavailable`; `Some` → `Accepted` when pre-consented (e.g. /// `--chat-yes`), otherwise `Pending` (the one-time in-TUI prompt). @@ -1717,12 +1735,27 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu }), true, ); - Some( - std::sync::Arc::new(crate::agent::MockAgentClient::with_tool_call( + Some(std::sync::Arc::new( + crate::agent::MockAgentClient::with_tool_call_and_approval_trigger( "GPU-2 is running hot: 87% util, 71°C, drawing 250 W (90 GB/192 GB VRAM).", "gpu_status", - )) as std::sync::Arc, - ) + "install the sdk", + crate::tool_exec::ApprovalIntent { + title: "Install TheRock ROCm SDK?".to_string(), + body: vec![ + "install_sdk --channel release --format wheel --prefix ~/rocm-sdk" + .to_string(), + ], + name: "install_sdk".to_string(), + arguments: serde_json::json!({ + "channel": "release", + "format": "wheel", + "prefix": "~/rocm-sdk", + }), + }, + chat_tx.clone(), + ), + ) as std::sync::Arc) } else { // An endpoint we launched ourselves (managed-services registry) takes // priority over the well-known default port — this is how a tool-launched @@ -2537,6 +2570,7 @@ fn apply_action(state: &mut AppState, action: KeyAction) -> bool { state.modal = if state.modal == Modal::Help { Modal::None } else { + state.reset_help_scroll(); Modal::Help }; } @@ -2635,7 +2669,10 @@ fn apply_action(state: &mut AppState, action: KeyAction) -> bool { state.modal = Modal::Options; state.options_tab = 0; } - 1 => state.modal = Modal::GlobalHelp, + 1 => { + state.reset_help_scroll(); + state.modal = Modal::GlobalHelp; + } _ => return true, // Quit }, Modal::Palette => { @@ -2647,8 +2684,11 @@ fn apply_action(state: &mut AppState, action: KeyAction) -> bool { _ => {} }, // ponytail: P3 folds Bench into Observe; the per-tab Bench detail modal - // (the only scrollable detail) is no longer reachable, so modal scroll - // is a no-op until/unless a scrollable Observe detail is wired. + // is no longer reachable, so Detail itself has nothing to scroll. Help + // and GlobalHelp are the only modals that currently use this action. + KeyAction::ScrollModal(delta) if matches!(state.modal, Modal::Help | Modal::GlobalHelp) => { + state.scroll_help(delta); + } KeyAction::ScrollModal(_) => {} KeyAction::ScrollConsole(dv, dh) => state.scroll_console(dv, dh), KeyAction::ScrollDock(dv) => state.scroll_dock(dv), @@ -3303,19 +3343,34 @@ fn handle_key(k: KeyEvent, current: ActiveTab, modal: &Modal, chat: ChatKeyCtx) _ => KeyAction::Nothing, }; } - // Help absorbs everything except quit / close / ? toggle. + // Help absorbs everything except quit / close / ? toggle / scroll — the + // body can run longer than the popup at small terminal sizes. if *modal == Modal::Help { return match k.code { KeyCode::Char('q') => KeyAction::Quit, KeyCode::Esc | KeyCode::Enter | KeyCode::Char('?') => KeyAction::CloseModal, + KeyCode::Char('j') | KeyCode::Down => KeyAction::ScrollModal(1), + KeyCode::Char('k') | KeyCode::Up => KeyAction::ScrollModal(-1), + KeyCode::PageDown => KeyAction::ScrollModal(10), + KeyCode::PageUp => KeyAction::ScrollModal(-10), + KeyCode::Char('g') | KeyCode::Home => KeyAction::ScrollModal(i16::MIN), + KeyCode::Char('G') | KeyCode::End => KeyAction::ScrollModal(i16::MAX), _ => KeyAction::Nothing, }; } - // Global help overlay (opened from the Esc menu): close-only. + // Global help overlay (opened from the Esc menu): close + scroll, same as + // the contextual Help above (shares `help_scroll`, the two are mutually + // exclusive). if *modal == Modal::GlobalHelp { return match k.code { KeyCode::Char('q') => KeyAction::Quit, KeyCode::Esc | KeyCode::Enter | KeyCode::Char('?') => KeyAction::CloseModal, + KeyCode::Char('j') | KeyCode::Down => KeyAction::ScrollModal(1), + KeyCode::Char('k') | KeyCode::Up => KeyAction::ScrollModal(-1), + KeyCode::PageDown => KeyAction::ScrollModal(10), + KeyCode::PageUp => KeyAction::ScrollModal(-10), + KeyCode::Char('g') | KeyCode::Home => KeyAction::ScrollModal(i16::MIN), + KeyCode::Char('G') | KeyCode::End => KeyAction::ScrollModal(i16::MAX), _ => KeyAction::Nothing, }; } @@ -4595,14 +4650,49 @@ mod tests { ChatKeyCtx::default(), ) }; - // j/k inside Help do nothing (Help has no scrollable body today). - assert_eq!(with_help(KeyCode::Char('j')), KeyAction::Nothing); + // j/k inside Help scroll its (now scrollable) body. + assert_eq!(with_help(KeyCode::Char('j')), KeyAction::ScrollModal(1)); + assert_eq!(with_help(KeyCode::Char('k')), KeyAction::ScrollModal(-1)); assert_eq!(with_help(KeyCode::Tab), KeyAction::Nothing); assert_eq!(with_help(KeyCode::Esc), KeyAction::CloseModal); assert_eq!(with_help(KeyCode::Enter), KeyAction::CloseModal); assert_eq!(with_help(KeyCode::Char('q')), KeyAction::Quit); } + #[test] + fn global_help_modal_j_k_emit_scroll() { + let with_global_help = |c| { + handle_key( + press(c), + ActiveTab::Observe, + &Modal::GlobalHelp, + ChatKeyCtx::default(), + ) + }; + assert_eq!( + with_global_help(KeyCode::Char('j')), + KeyAction::ScrollModal(1) + ); + assert_eq!( + with_global_help(KeyCode::Char('k')), + KeyAction::ScrollModal(-1) + ); + assert_eq!( + with_global_help(KeyCode::PageDown), + KeyAction::ScrollModal(10) + ); + assert_eq!( + with_global_help(KeyCode::Char('g')), + KeyAction::ScrollModal(i16::MIN) + ); + assert_eq!( + with_global_help(KeyCode::Char('G')), + KeyAction::ScrollModal(i16::MAX) + ); + assert_eq!(with_global_help(KeyCode::Esc), KeyAction::CloseModal); + assert_eq!(with_global_help(KeyCode::Char('q')), KeyAction::Quit); + } + #[test] fn move_selection_clamps_to_bounds() { // P3: Observe's selectable list is the instances table. diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 068fb90fa..29b01839d 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -42,7 +42,7 @@ use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Clear, Paragraph}; -use crate::app::{ActiveTab, AppState, ConnState, FooterChip, KeyAction, Modal}; +use crate::app::{ActiveTab, AppState, ChatConsent, ConnState, FooterChip, KeyAction, Modal}; use crate::ui::theme::Theme; pub fn draw(f: &mut Frame, state: &mut AppState) { @@ -123,7 +123,7 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { // Modal overlay (rendered last so it sits on top of the body). match state.modal { Modal::None => {} - Modal::Help => modal::draw_help(f, body, state.active_tab, &theme), + Modal::Help => modal::draw_help(f, body, state.active_tab, &theme, state.help_scroll), // Observe folds the telemetry tabs; its detail modal is the instance // detail (the selectable list on that surface). Modal::Detail => { @@ -137,7 +137,7 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { Modal::Menu => modal::draw_menu(f, body, state.menu_sel, &theme), Modal::Palette => modal::draw_palette(f, body, state.palette_sel, &theme), Modal::Options => modal::draw_options(f, body, state, &theme), - Modal::GlobalHelp => modal::draw_global_help(f, body, &theme), + Modal::GlobalHelp => modal::draw_global_help(f, body, &theme, state.help_scroll), } // Operational managers render as a centered MODAL on every tab. The @@ -419,14 +419,32 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve Seg::Key("1–5", None), Seg::Sep(" jump "), ]; - // With a manager overlay open (any tab), Esc backs it out rather than - // opening the menu — advertise that instead of the (now wrong) select/open - // hints, which route to the manager, not the pane. Exactly one Esc chip is - // shown at all times: "back out" when an overlay is open, otherwise the - // uniform fallback "menu" (item #35). - if state.has_open_overlay() { + // Exactly one Esc chip is shown at all times, and it must match what Esc + // actually does — the real routing priority (highest first) is: a pending + // chat approval owns every key; then an open manager overlay backs itself + // out; then a `Modal::*` overlay closes; then a focused/gating Chat tab + // absorbs Esc; only once none of those apply does Esc fall through to the + // uniform "menu" fallback (item #35). Mirror that order here so the chip + // never advertises `menu` while a click on it would actually do something + // else. + if state.approval.is_some() { + segs.push(Seg::Key("Esc", None)); + segs.push(Seg::Sep(" cancel ")); + } else if state.has_open_overlay() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); + } else if state.modal != Modal::None { + segs.push(Seg::Key("Esc", Some(KeyAction::CloseModal))); + segs.push(Seg::Sep(" close ")); + } else if state.active_tab == ActiveTab::Chat + && state.chat_detect_offer.is_some() + && state.chat_consent != ChatConsent::Accepted + { + segs.push(Seg::Key("Esc", Some(KeyAction::ChatDetectDismiss))); + segs.push(Seg::Sep(" dismiss ")); + } else if state.active_tab == ActiveTab::Chat && state.chat_focused { + segs.push(Seg::Key("Esc", Some(KeyAction::ChatBlur))); + segs.push(Seg::Sep(" unfocus ")); } else { segs.push(Seg::Key("Esc", Some(KeyAction::OpenMenu))); segs.push(Seg::Sep(" menu ")); diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index 553b0a041..da935e771 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -81,44 +81,30 @@ pub fn draw_scrollable_lines( /// Render the Help modal for the active tab. /// -/// Shares chrome (dimmed backdrop, popup geometry, 2-column grouped layout) -/// with `draw_global_help` so the two help screens read as one family; unlike -/// that screen, this one's right column is dynamic — the active tab's own -/// keys. -pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { +/// Shares chrome (dimmed backdrop, popup geometry, scrollable single-column +/// layout) with `draw_global_help` so the two help screens read as one +/// family; unlike that screen, this one has an extra group — the active +/// tab's own keys. `scroll` is the first visible line offset (see +/// [`draw_scrollable_lines`]): a fixed two-column split used to clip content +/// at small terminal sizes, since a group's rows could run past the popup's +/// height with no way to reach them. +pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scroll: u16) { grey_overlay(f); let popup = centered_rect(80, 80, 100, 26, area); - let inner = draw_popup_frame(f, popup, "Help", theme); - if inner.height == 0 { - return; - } - f.render_widget(Clear, inner); - let cols = Layout::default() - .direction(Direction::Horizontal) - .constraints([Constraint::Percentage(50), Constraint::Percentage(50)]) - .split(inner); - let left: &[(&str, &[(&str, &str)])] = &[ - ( - "GLOBAL", - &[ - ("q", "quit"), - ("?", "toggle this help"), - ("Tab / Shift-Tab", "next / previous tab"), - ("1 .. 5", "jump to tab"), - ("t", "open theme picker"), - ("Esc", "open the main menu"), - ], - ), - ( - "REPLAY", - &[ - ("Space", "pause / resume"), - ("+ / -", "speed up / slow down"), - ("[ / ]", "jump ±10s"), - ("{ / }", "jump ±60s"), - ], - ), + let global: &[(&str, &str)] = &[ + ("q", "quit"), + ("?", "toggle this help"), + ("Tab / Shift-Tab", "next / previous tab"), + ("1 .. 5", "jump to tab"), + ("t", "open theme picker"), + ("Esc", "open the main menu"), + ]; + let replay: &[(&str, &str)] = &[ + ("Space", "pause / resume"), + ("+ / -", "speed up / slow down"), + ("[ / ]", "jump ±10s"), + ("{ / }", "jump ±60s"), ]; let tab_help: &[(&str, &str)] = match tab { ActiveTab::Home => &[("(no tab-specific keys — see the ROCm / Serving tabs)", "")], @@ -155,10 +141,14 @@ pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { ], }; let tab_title = format!("{tab:?}").to_uppercase(); - let right: &[(&str, &[(&str, &str)])] = &[(tab_title.as_str(), tab_help)]; + let groups: &[(&str, &[(&str, &str)])] = &[ + ("GLOBAL", global), + (&tab_title, tab_help), + ("REPLAY", replay), + ]; - render_help_groups(f, cols[0], left, theme); - render_help_groups(f, cols[1], right, theme); + let lines = help_group_lines(groups, theme); + draw_scrollable_lines(f, popup, "Help", lines, scroll, theme); } fn key_line<'a>(key: &'a str, desc: &'a str, theme: &Theme) -> Line<'a> { @@ -575,21 +565,16 @@ pub fn draw_options(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) } } -/// Global 2-column keyboard reference (NAVIGATE / OVERLAYS / ACTIONS / CHAT / -/// GLOBAL). Distinct from the contextual per-tab `?` help (`draw_help`). -pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme) { +/// Global keyboard reference (NAVIGATE / OVERLAYS / ACTIONS / CHAT / GLOBAL). +/// +/// Distinct from the contextual per-tab `?` help (`draw_help`), but shares its +/// chrome — dimmed backdrop, popup geometry, and scrollable single-column +/// layout (see [`draw_help`] for why). `scroll` is the first visible line +/// offset (see [`draw_scrollable_lines`]). +pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme, scroll: u16) { grey_overlay(f); - let modal = centered_rect(80, 80, 100, 26, area); - let inner = draw_popup_frame(f, modal, "Keyboard", theme); - if inner.height == 0 { - return; - } - f.render_widget(Clear, inner); - let cols = Layout::default() - .direction(Direction::Horizontal) - .constraints([Constraint::Percentage(50), Constraint::Percentage(50)]) - .split(inner); - let left: &[(&str, &[(&str, &str)])] = &[ + let popup = centered_rect(80, 80, 100, 26, area); + let groups: &[(&str, &[(&str, &str)])] = &[ ( "NAVIGATE", &[ @@ -607,8 +592,6 @@ pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme) { ("t", "theme picker"), ], ), - ]; - let right: &[(&str, &[(&str, &str)])] = &[ ( "ACTIONS", &[ @@ -622,16 +605,16 @@ pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme) { &[("i / Enter", "focus chat input"), ("q", "quit")], ), ]; - render_help_groups(f, cols[0], left, theme); - render_help_groups(f, cols[1], right, theme); + let lines = help_group_lines(groups, theme); + draw_scrollable_lines(f, popup, "Keyboard", lines, scroll, theme); } -fn render_help_groups( - f: &mut Frame, - area: Rect, - groups: &[(&str, &[(&str, &str)])], +/// Flatten keyboard-help groups into the `Vec` shape `draw_help` and +/// `draw_global_help` both render via [`draw_scrollable_lines`]. +fn help_group_lines<'a>( + groups: &[(&'a str, &[(&'a str, &'a str)])], theme: &Theme, -) { +) -> Vec> { let mut lines: Vec = Vec::new(); for (title, rows) in groups { lines.push(Line::from(Span::styled( @@ -645,7 +628,7 @@ fn render_help_groups( } lines.push(Line::raw("")); } - f.render_widget(Paragraph::new(lines).wrap(Wrap { trim: false }), area); + lines } // =========================================================================== @@ -857,7 +840,7 @@ mod ported_chrome_tests { ); assert!(options.contains("General"), "options missing tab label"); - let help = render(&|f| super::draw_global_help(f, area, &theme)); + let help = render(&|f| super::draw_global_help(f, area, &theme, 0)); assert!( help.contains("Keyboard"), "global help missing title: {help:?}" diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index f4bddf392..b392c3112 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -922,6 +922,7 @@ mod tests { theme: Theme::default_dark(), theme_picker_sel: 0, bench_detail_scroll: 0, + help_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, From a58a20d16dde778ade1d2916258714b96ca88b57 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 12:46:21 +0000 Subject: [PATCH 05/36] test(e2e): cover approval-defaults-to-deny and backdrop dimming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dash-12 drives the new mock approval trigger end-to-end: a chat message surfaces a tool-call review, and confirming with a bare Enter (no cursor movement) denies it, since Deny is the default focus. dash-13 opens instance detail and asserts the screen behind the popup is dimmed, via a new corner_backdrop_is_dimmed driver check that reads the actual cell background color instead of screen_text() — necessary because dimming is styling-only and invisible to a text-based read. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 34 +++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 147 +++++++++++++++++++++ tests/e2e-cucumber/tests/e2e/tui_driver.rs | 50 +++++++ 3 files changed, 231 insertions(+) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index 16ad1b267..4f5ec3e7e 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -46,10 +46,44 @@ Feature: Interactive dashboard When the user opens the dashboard with demo data And the user opens dashboard help Then navigation and next-step guidance are displayed + When the user scrolls to the end of dashboard help + Then replay controls guidance is displayed When the user closes dashboard help And the user quits the dashboard Then the dashboard exits successfully + @id:dash-manager-escape-closes-without-menu @requires-os:linux + Scenario: dash-11 - Escape closes a non-domain-tab manager overlay instead of opening the menu + When the user opens the dashboard with demo data + And the user opens the Observe view + And the user opens the services manager + Then the services manager is displayed + When the user presses Escape + Then the services manager is closed without opening the menu + When the user quits the dashboard + Then the dashboard exits successfully + + @id:dash-chat-approval-defaults-to-deny @requires-os:linux + Scenario: dash-12 - A surfaced tool call defaults to Deny and confirming without moving denies it + Given interactive chat uses an offline assistant + When the user opens interactive chat + And the user sends a message that triggers a tool approval + Then a tool approval prompt is displayed + When the user confirms the approval prompt without moving the cursor + Then the tool call is shown as declined + When the user quits interactive chat + Then interactive chat exits successfully + + @id:dash-instance-detail-dims-backdrop @requires-os:linux + Scenario: dash-13 - Opening instance detail dims the screen behind the popup + When the user opens the dashboard with demo data + And the user opens the Observe view + And the user opens instance detail + Then instance details are displayed + And the backdrop behind the popup is dimmed + When the user quits the dashboard + Then the dashboard exits successfully + @id:dash-command-palette-navigation @requires-os:linux Scenario: dash-06 - A user navigates to Serving through the command palette When the user opens the dashboard with demo data diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 668d57124..53c4e86e8 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -143,6 +143,37 @@ async fn open_observe_view(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to switch to the Observe tab: {e}")); } +#[when("the user opens instance detail")] +async fn open_instance_detail(world: &mut E2eWorld) { + // `Enter` on the Observe tab opens the selected instance's detail popup + // (`KeyAction::OpenDetail`); the demo session always seeds at least one + // instance, so the default selection (index 0) is always present. Like + // `open_observe_view`, resend until it takes effect: nothing before this + // step proves the Observe tab's own input handling (as opposed to just the + // tab switch) is already wired up on this exact frame. + session(world) + .send_until("\r", "Instance · ", default_timeout()) + .await + .unwrap_or_else(|e| panic!("failed to open instance detail: {e}")); +} + +#[when("the user opens the services manager")] +async fn open_services_manager(world: &mut E2eWorld) { + // Bound to `s` only on the Observe tab (`OpenServices`) — a manager opened + // from a non-domain tab, which is exactly the case + // `should_pane_back_out`'s doc comment calls out as needing Esc to close it. + session(world) + .send("s") + .unwrap_or_else(|e| panic!("failed to open the services manager: {e}")); +} + +#[when("the user presses Escape")] +async fn press_escape(world: &mut E2eWorld) { + session(world) + .send("\x1b") + .unwrap_or_else(|e| panic!("failed to send Escape: {e}")); +} + #[when("the user opens dashboard help")] async fn open_dashboard_help(world: &mut E2eWorld) { session(world) @@ -157,6 +188,13 @@ async fn close_dashboard_help(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to close dashboard help: {e}")); } +#[when("the user scrolls to the end of dashboard help")] +async fn scroll_to_end_of_dashboard_help(world: &mut E2eWorld) { + session(world) + .send("G") + .unwrap_or_else(|e| panic!("failed to scroll dashboard help: {e}")); +} + #[when("the user opens the command palette")] async fn open_command_palette(world: &mut E2eWorld) { session(world) @@ -218,6 +256,32 @@ async fn send_gpu_message(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to submit the chat message: {e}")); } +#[when("the user sends a message that triggers a tool approval")] +async fn send_approval_trigger_message(world: &mut E2eWorld) { + let tui = session(world); + // Wait for the accepted, empty chat surface before typing so the input is + // ready to receive focus. + tui.wait_for_screen("No messages yet.", default_timeout()) + .await + .unwrap_or_else(|e| panic!("chat surface never became ready: {e}")); + // `i` focuses the input; then the message, then Enter to submit. The + // phrase must match `MockAgentClient`'s trigger ("install the sdk") without + // colliding with `send_gpu_message`'s "how is gpu-2 doing". + tui.send("i") + .unwrap_or_else(|e| panic!("failed to focus the chat input: {e}")); + tui.send("please install the sdk") + .unwrap_or_else(|e| panic!("failed to type the chat message: {e}")); + tui.send("\r") + .unwrap_or_else(|e| panic!("failed to submit the chat message: {e}")); +} + +#[when("the user confirms the approval prompt without moving the cursor")] +async fn confirm_approval_without_moving(world: &mut E2eWorld) { + session(world) + .send("\r") + .unwrap_or_else(|e| panic!("failed to press Enter on the approval prompt: {e}")); +} + async fn quit_tui(world: &mut E2eWorld, surface: &str) { session(world) .quit_and_wait(default_timeout()) @@ -273,6 +337,30 @@ async fn gpu_response_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("the assistant's response did not appear: {e}")); } +#[then("a tool approval prompt is displayed")] +async fn approval_prompt_displayed(world: &mut E2eWorld) { + let tui = session(world); + tui.wait_for_screen("Review: Install TheRock ROCm SDK?", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the approval prompt did not appear: {e}")); + let screen = tui.screen_text(); + assert!( + screen.contains("Approve (y)") && screen.contains("Deny (n)"), + "approval prompt is missing its Approve/Deny buttons:\n{screen}" + ); +} + +#[then("the tool call is shown as declined")] +async fn tool_call_shown_declined(world: &mut E2eWorld) { + let tui = session(world); + tui.wait_until_gone("Review: Install TheRock ROCm SDK?", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the approval prompt is still open after Enter: {e}")); + tui.wait_for_screen("Action declined.", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the declined-tool-call message did not appear: {e}")); +} + #[then("the managed model's response is displayed")] async fn managed_model_response_displayed(world: &mut E2eWorld) { session(world) @@ -373,6 +461,65 @@ async fn navigation_guidance_displayed(world: &mut E2eWorld) { ); } +#[then("replay controls guidance is displayed")] +async fn replay_controls_guidance_displayed(world: &mut E2eWorld) { + let tui = session(world); + // REPLAY is the last group in the flattened, scrollable help body — it + // sits past what an 80x24 terminal shows without scrolling, so this + // proves the scroll wiring actually reaches previously-clipped content. + tui.wait_for_screen("pause / resume", default_timeout()) + .await + .unwrap_or_else(|e| panic!("replay controls guidance did not appear after scrolling: {e}")); + let screen = tui.screen_text(); + assert!( + screen.contains("REPLAY") && screen.contains("pause / resume"), + "replay controls guidance missing after scrolling to end of help:\n{screen}" + ); +} + +#[then("the services manager is displayed")] +async fn services_manager_displayed(world: &mut E2eWorld) { + session(world) + .wait_for_screen("Services — managed inference servers", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the services manager did not appear: {e}")); +} + +#[then("the services manager is closed without opening the menu")] +async fn services_manager_closed_without_menu(world: &mut E2eWorld) { + let tui = session(world); + tui.wait_until_gone("Services — managed inference servers", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the services manager is still open after Escape: {e}")); + let screen = tui.screen_text(); + assert!( + screen.contains("● Observe"), + "Escape left the Observe tab entirely, not just the manager:\n{screen}" + ); + assert!( + !screen.contains("Options") && !screen.contains("Quit"), + "Escape opened the main menu instead of closing the manager:\n{screen}" + ); +} + +#[then("instance details are displayed")] +async fn instance_details_displayed(world: &mut E2eWorld) { + session(world) + .wait_for_screen("Instance · ", default_timeout()) + .await + .unwrap_or_else(|e| panic!("instance details did not appear: {e}")); +} + +#[then("the backdrop behind the popup is dimmed")] +async fn backdrop_is_dimmed(world: &mut E2eWorld) { + let tui = session(world); + assert!( + tui.corner_backdrop_is_dimmed(), + "the screen behind the instance detail popup was not dimmed:\n{}", + tui.screen_text() + ); +} + #[then("dashboard destinations are displayed")] async fn dashboard_destinations_displayed(world: &mut E2eWorld) { let tui = session(world); diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 3ec8d0d85..73dcc0839 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -239,6 +239,22 @@ impl TuiSession { self.screen_snapshot().0 } + /// Whether the top-left cell carries the dimmed-backdrop wash a popup + /// overlay paints behind itself (`grey_overlay`'s fixed RGB(0x1c, 0x1e, + /// 0x22)). Popups drawn via `centered_rect` always leave a pad outside + /// the frame, and the top-left corner falls in that pad, so this is a + /// reliable proxy for "the screen behind the popup was dimmed" without + /// importing the product crate's own color constant. Unlike + /// `screen_text`, this deliberately inspects style, not just text content. + pub fn corner_backdrop_is_dimmed(&self) -> bool { + const WASH: vt100::Color = vt100::Color::Rgb(0x1c, 0x1e, 0x22); + let p = self + .parser + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + p.screen().cell(0, 0).is_some_and(|c| c.bgcolor() == WASH) + } + fn screen_snapshot(&self) -> (String, (u16, u16)) { // Recover a poisoned lock rather than defaulting to a blank screen: the // parser's data is still valid even if some other thread panicked while @@ -444,6 +460,40 @@ impl TuiSession { } } + /// Wait until `marker` no longer appears on screen — the inverse of + /// [`wait_for_screen`](Self::wait_for_screen). Use this after a keystroke + /// that should dismiss an overlay whose absence is the only signal of + /// success (there is no positive marker for "menu didn't open"). + pub async fn wait_until_gone(&mut self, marker: &str, timeout: Duration) -> Result<(), String> { + let deadline = Instant::now() + timeout; + loop { + if !self.screen_text().contains(marker) { + return Ok(()); + } + if let Some(panic_message) = self.take_reader_panic() { + return Err(format!( + "pty reader thread panicked while waiting for {marker:?} to disappear: {panic_message}\n{}", + self.framed_screen() + )); + } + if let Ok(Some(status)) = self.child.try_wait() { + self.finished = true; + self.record_once(i32::try_from(status.exit_code()).unwrap_or(-1)); + return Err(format!( + "process exited ({status:?}) before {marker:?} disappeared.\n{}", + self.framed_screen() + )); + } + if Instant::now() >= deadline { + return Err(format!( + "timed out after {timeout:?} waiting for {marker:?} to disappear.\n{}", + self.framed_screen() + )); + } + tokio::time::sleep(POLL_INTERVAL).await; + } + } + /// Send the quit gesture appropriate to the session and wait for a clean /// exit. The dashboard quits with `q`; chat quits with the `/quit` slash /// command (a bare `q` would be typed into the focused input instead). From 30c190743cd84c52b174252ab240286bcc119937 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 14:06:36 +0000 Subject: [PATCH 06/36] fix(dash-tui): clamp help-overlay scroll to the last page Scrolling to the end of the dashboard help overlay sent an effectively-unbounded offset to Paragraph::scroll, pushing every line past the viewport and rendering a blank pane instead of the final page of content. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/modal.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index da935e771..bbbac16b3 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -60,7 +60,10 @@ pub fn draw_popup_frame(f: &mut Frame, area: Rect, title: &str, theme: &Theme) - /// Shared chrome: a titled popup whose body is a scrollable block of `lines`. /// /// Centralizes the `draw_modal_*` pattern so operational screens don't rebuild -/// it (Phase 3 Wave 0). `scroll` is the first visible line offset. +/// it (Phase 3 Wave 0). `scroll` is the first visible line offset, clamped +/// here to the content's last page so a "scroll to end" action (which sends +/// `u16::MAX`-ish deltas, see `AppState::scroll_help`) can't push every line +/// past the viewport and render a blank pane. pub fn draw_scrollable_lines( f: &mut Frame, area: Rect, @@ -73,8 +76,11 @@ pub fn draw_scrollable_lines( if inner.height == 0 { return; } + let max_scroll = u16::try_from(lines.len()) + .unwrap_or(u16::MAX) + .saturating_sub(inner.height); let p = Paragraph::new(lines) - .scroll((scroll, 0)) + .scroll((scroll.min(max_scroll), 0)) .wrap(Wrap { trim: false }); f.render_widget(p, inner); } @@ -870,4 +876,20 @@ mod ported_chrome_tests { assert!(out.contains("tokyo"), "value missing: {out:?}"); assert!(out.contains('▸'), "focus/control marker missing: {out:?}"); } + + #[test] + fn help_scroll_past_end_clamps_instead_of_blanking() { + use crate::app::ActiveTab; + let theme = Theme::from_name("default-dark"); + let area = Rect::new(0, 0, 80, 24); + let backend = TestBackend::new(80, 24); + let mut term = Terminal::new(backend).unwrap(); + term.draw(|f| super::draw_help(f, area, ActiveTab::Home, &theme, i16::MAX as u16)) + .unwrap(); + let out = flat(&term); + assert!( + out.contains("REPLAY") && out.contains("pause / resume"), + "overscrolling help should clamp to the last page, not blank it: {out:?}" + ); + } } From 1d1a34322f6ceac8bb52291e9eedee56d83214ed Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 8 Sep 2026 14:06:56 +0000 Subject: [PATCH 07/36] test(e2e): restore sequential scenario order in dash.feature dash-11/12/13 were declared right after dash-05 instead of after dash-10, which the feature_naming drift guard requires to be strictly ascending per file. Relocate the three scenarios to the end of the file; content is unchanged. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 64 ++++++++++++------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index 4f5ec3e7e..fcc7c5625 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -52,38 +52,6 @@ Feature: Interactive dashboard And the user quits the dashboard Then the dashboard exits successfully - @id:dash-manager-escape-closes-without-menu @requires-os:linux - Scenario: dash-11 - Escape closes a non-domain-tab manager overlay instead of opening the menu - When the user opens the dashboard with demo data - And the user opens the Observe view - And the user opens the services manager - Then the services manager is displayed - When the user presses Escape - Then the services manager is closed without opening the menu - When the user quits the dashboard - Then the dashboard exits successfully - - @id:dash-chat-approval-defaults-to-deny @requires-os:linux - Scenario: dash-12 - A surfaced tool call defaults to Deny and confirming without moving denies it - Given interactive chat uses an offline assistant - When the user opens interactive chat - And the user sends a message that triggers a tool approval - Then a tool approval prompt is displayed - When the user confirms the approval prompt without moving the cursor - Then the tool call is shown as declined - When the user quits interactive chat - Then interactive chat exits successfully - - @id:dash-instance-detail-dims-backdrop @requires-os:linux - Scenario: dash-13 - Opening instance detail dims the screen behind the popup - When the user opens the dashboard with demo data - And the user opens the Observe view - And the user opens instance detail - Then instance details are displayed - And the backdrop behind the popup is dimmed - When the user quits the dashboard - Then the dashboard exits successfully - @id:dash-command-palette-navigation @requires-os:linux Scenario: dash-06 - A user navigates to Serving through the command palette When the user opens the dashboard with demo data @@ -157,3 +125,35 @@ Feature: Interactive dashboard Then the launcher shows the model serving When the user quits the launcher Then the launcher exits successfully + + @id:dash-manager-escape-closes-without-menu @requires-os:linux + Scenario: dash-11 - Escape closes a non-domain-tab manager overlay instead of opening the menu + When the user opens the dashboard with demo data + And the user opens the Observe view + And the user opens the services manager + Then the services manager is displayed + When the user presses Escape + Then the services manager is closed without opening the menu + When the user quits the dashboard + Then the dashboard exits successfully + + @id:dash-chat-approval-defaults-to-deny @requires-os:linux + Scenario: dash-12 - A surfaced tool call defaults to Deny and confirming without moving denies it + Given interactive chat uses an offline assistant + When the user opens interactive chat + And the user sends a message that triggers a tool approval + Then a tool approval prompt is displayed + When the user confirms the approval prompt without moving the cursor + Then the tool call is shown as declined + When the user quits interactive chat + Then interactive chat exits successfully + + @id:dash-instance-detail-dims-backdrop @requires-os:linux + Scenario: dash-13 - Opening instance detail dims the screen behind the popup + When the user opens the dashboard with demo data + And the user opens the Observe view + And the user opens instance detail + Then instance details are displayed + And the backdrop behind the popup is dimmed + When the user quits the dashboard + Then the dashboard exits successfully From 98aa5e8c9f167e0eb55f5b1cf468cc95f1c18800 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 05:23:34 +0000 Subject: [PATCH 08/36] fix(dash-tui): clamp help-overlay scroll to the wrapped row count The clamp added in 30c1907 computed max_scroll from lines.len(), the pre-wrap logical line count. Paragraph::scroll() is applied post-wrap, so any help line that wraps at the popup's width under-clamps the maximum, leaving trailing content (e.g. Chat tab's REPLAY group) permanently unreachable even via jump-to-end. Use Paragraph::line_count(inner.width) after wrap() is set instead, which measures the actual rendered row count. Extends the existing regression test with a Chat-tab case, since Home's tab-specific text never wraps at 80 columns and couldn't catch this. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/modal.rs | 36 ++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index bbbac16b3..153fb7358 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -63,7 +63,10 @@ pub fn draw_popup_frame(f: &mut Frame, area: Rect, title: &str, theme: &Theme) - /// it (Phase 3 Wave 0). `scroll` is the first visible line offset, clamped /// here to the content's last page so a "scroll to end" action (which sends /// `u16::MAX`-ish deltas, see `AppState::scroll_help`) can't push every line -/// past the viewport and render a blank pane. +/// past the viewport and render a blank pane. The clamp is computed from the +/// *wrapped* row count (`Paragraph::line_count`), not `lines.len()` — `scroll` +/// is applied post-wrap, so a pre-wrap count under-clamps whenever a line +/// wraps and leaves trailing content permanently unreachable. pub fn draw_scrollable_lines( f: &mut Frame, area: Rect, @@ -76,12 +79,11 @@ pub fn draw_scrollable_lines( if inner.height == 0 { return; } - let max_scroll = u16::try_from(lines.len()) + let p = Paragraph::new(lines).wrap(Wrap { trim: false }); + let max_scroll = u16::try_from(p.line_count(inner.width)) .unwrap_or(u16::MAX) .saturating_sub(inner.height); - let p = Paragraph::new(lines) - .scroll((scroll.min(max_scroll), 0)) - .wrap(Wrap { trim: false }); + let p = p.scroll((scroll.min(max_scroll), 0)); f.render_widget(p, inner); } @@ -892,4 +894,28 @@ mod ported_chrome_tests { "overscrolling help should clamp to the last page, not blank it: {out:?}" ); } + + /// Home's tab-specific text is a single short line that never wraps at 80 + /// columns, so the test above can't catch a clamp computed from the + /// pre-wrap line count instead of the wrapped row count. Chat's + /// descriptions do wrap at this width — this is what actually exercises + /// `draw_scrollable_lines`'s `max_scroll` against wrapped content, and + /// pins REPLAY's last entry (`{ / } jump ±60s`) as reachable via "jump + /// to end". + #[test] + fn help_scroll_past_end_reaches_last_line_when_content_wraps() { + use crate::app::ActiveTab; + let theme = Theme::from_name("default-dark"); + let area = Rect::new(0, 0, 80, 24); + let backend = TestBackend::new(80, 24); + let mut term = Terminal::new(backend).unwrap(); + term.draw(|f| super::draw_help(f, area, ActiveTab::Chat, &theme, i16::MAX as u16)) + .unwrap(); + let out = flat(&term); + assert!( + out.contains("REPLAY") && out.contains("60s"), + "overscrolling wrapped help should still reach the last REPLAY \ + entry, not clamp short of it: {out:?}" + ); + } } From b0b08e16b02500ac7c743f78e8ace3cdb146249d Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 06:50:28 +0000 Subject: [PATCH 09/36] fix(dash-tui): stop MockAgentClient re-firing approval on follow-up The trigger check scanned backward for the last User turn instead of checking history.last() directly, so the automatic follow-up call after on_approval_result (which appends an Agent turn, not a new User turn) still matched the original trigger phrase and re-surfaced a second spurious approval request. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/agent.rs | 52 ++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/rocm-dash-tui/src/agent.rs b/crates/rocm-dash-tui/src/agent.rs index 4e690da14..acd45c291 100644 --- a/crates/rocm-dash-tui/src/agent.rs +++ b/crates/rocm-dash-tui/src/agent.rs @@ -1602,11 +1602,11 @@ impl AgentClient for MockAgentClient { return Err(AgentError::Empty); } if let Some(trigger) = &self.approval { - let fires = history - .iter() - .rev() - .find(|t| t.role == ChatRole::User) - .is_some_and(|t| t.content.to_lowercase().contains(&trigger.phrase)); + let fires = matches!( + history.last(), + Some(t) if t.role == ChatRole::User + && t.content.to_lowercase().contains(&trigger.phrase) + ); if fires { let _ = trigger.tx.send(ClientMsg::ChatApprovalRequired { intent: trigger.intent.clone(), @@ -2133,6 +2133,48 @@ mod tests { assert!(matches!(err, AgentError::Empty)); } + #[tokio::test] + async fn approval_trigger_does_not_refire_on_follow_up() { + // `on_approval_result` appends the approved-action result as an Agent + // turn (not a new User turn) before raising the one-shot automatic + // follow-up. The trigger must key off the *last* turn only, so that + // follow-up call sees `[User(trigger), Agent(result)]` and does not + // re-surface approval a second time. + let (tx, _rx) = tokio::sync::mpsc::unbounded_channel::(); + let agent = MockAgentClient::with_tool_call_and_approval_trigger( + "all good", + "gpu_status", + "install the sdk", + crate::tool_exec::ApprovalIntent { + title: "Install TheRock ROCm SDK?".to_string(), + body: vec!["install_sdk".to_string()], + name: "install_sdk".to_string(), + arguments: json!({}), + }, + tx, + ); + + let first = vec![ChatTurn::user("please install the sdk")]; + let reply = agent + .complete(&first, fixture_snapshot()) + .await + .expect("mock reply"); + assert!(reply.contains("surfaced to")); + + let follow_up = vec![ + ChatTurn::user("please install the sdk"), + ChatTurn::agent("Installed successfully."), + ]; + let reply = agent + .complete(&follow_up, fixture_snapshot()) + .await + .expect("mock reply"); + assert!( + !reply.contains("surfaced to"), + "approval trigger re-fired on the automatic follow-up: {reply}" + ); + } + #[test] #[allow(clippy::float_cmp)] fn rig_client_stores_inference_params() { From ae09df7e89bf329103b9650cbc6c47dee4650b35 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 06:50:40 +0000 Subject: [PATCH 10/36] fix(dash-tui): reset help scroll when /help or /? opens Help The slash-command path set Modal::Help directly, bypassing reset_help_scroll(). The ? key and the Esc-menu's Help entry both reset the scroll, so /help after a prior scroll-and-close reopened Help at the stale offset instead of the top. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 18 ++++++++++++++++++ crates/rocm-dash-tui/src/app/slash.rs | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index af36f938c..339b16056 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -5483,6 +5483,15 @@ mod tests { assert_eq!(s.modal, Modal::Help); } + #[test] + fn slash_help_resets_stale_scroll_offset() { + let mut s = st(); + s.help_scroll = 42; + assert_eq!(s.handle_slash_command("/help"), SlashOutcome::Handled); + assert_eq!(s.modal, Modal::Help); + assert_eq!(s.help_scroll, 0); + } + #[test] fn slash_question_mark_opens_help_modal() { let mut s = st(); @@ -5490,6 +5499,15 @@ mod tests { assert_eq!(s.modal, Modal::Help); } + #[test] + fn slash_question_mark_resets_stale_scroll_offset() { + let mut s = st(); + s.help_scroll = 17; + assert_eq!(s.handle_slash_command("/?"), SlashOutcome::Handled); + assert_eq!(s.modal, Modal::Help); + assert_eq!(s.help_scroll, 0); + } + #[test] fn slash_clear_empties_transcript() { let mut s = st(); diff --git a/crates/rocm-dash-tui/src/app/slash.rs b/crates/rocm-dash-tui/src/app/slash.rs index 16f4cb0e6..101d747f8 100644 --- a/crates/rocm-dash-tui/src/app/slash.rs +++ b/crates/rocm-dash-tui/src/app/slash.rs @@ -46,7 +46,10 @@ impl AppState { // --- Group A: nav / session (deterministic, no executor) --- "home" => self.active_tab = ActiveTab::Home, "gpu" => self.active_tab = ActiveTab::Observe, - "help" | "?" => self.modal = Modal::Help, + "help" | "?" => { + self.reset_help_scroll(); + self.modal = Modal::Help; + } "clear" => self.chat.clear(), "quit" | "exit" => self.should_quit = true, // --- Group B: read-only overlays (mirror the keybind handlers) --- From d54e7a7215b87c50aaa0401b20c263ded3de8689 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 9 Sep 2026 06:50:53 +0000 Subject: [PATCH 11/36] test(e2e): add dash-14/dash-15 scenarios, fix flaky detail-popup step Two behaviors rendered correctly but had no regression coverage: - Escape opens the main menu when idle on the Chat tab (dash-14) - The theme picker dims the backdrop behind it, like other popups (dash-15) open_instance_detail also misused send_until on Enter, a toggle key for Modal::Detail; a resend while the popup is already open would immediately close it. Replaced with a plain send + wait_for_screen. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 18 ++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 50 +++++++++++++++++++--- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index fcc7c5625..f0be86d64 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -157,3 +157,21 @@ Feature: Interactive dashboard And the backdrop behind the popup is dimmed When the user quits the dashboard Then the dashboard exits successfully + + @id:dash-chat-idle-escape-opens-menu @requires-os:linux + Scenario: dash-14 - Escape opens the menu when idle on the Chat tab + When the user opens the dashboard with demo data + And the user opens the Chat view + When the user presses Escape + Then the dashboard menu is displayed + When the user presses Escape + And the user quits the dashboard + Then the dashboard exits successfully + + @id:dash-theme-picker-dims-backdrop @requires-os:linux + Scenario: dash-15 - Opening the theme picker dims the screen behind it + When the user opens the dashboard with demo data + And the user opens the theme picker + Then the backdrop behind the popup is dimmed + When the user quits the dashboard + Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 53c4e86e8..b15148d0e 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -143,18 +143,31 @@ async fn open_observe_view(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to switch to the Observe tab: {e}")); } +#[when("the user opens the Chat view")] +async fn open_chat_view(world: &mut E2eWorld) { + // Same resend-until-it-takes rationale as `open_observe_view`: nothing + // before this step proves the event loop is reading input yet. + session(world) + .send_until("5", "● Chat", default_timeout()) + .await + .unwrap_or_else(|e| panic!("failed to switch to the Chat tab: {e}")); +} + #[when("the user opens instance detail")] async fn open_instance_detail(world: &mut E2eWorld) { // `Enter` on the Observe tab opens the selected instance's detail popup // (`KeyAction::OpenDetail`); the demo session always seeds at least one - // instance, so the default selection (index 0) is always present. Like - // `open_observe_view`, resend until it takes effect: nothing before this - // step proves the Observe tab's own input handling (as opposed to just the - // tab switch) is already wired up on this exact frame. - session(world) - .send_until("\r", "Instance · ", default_timeout()) + // instance, so the default selection (index 0) is always present. Enter + // toggles `Modal::Detail` open/closed, so it is NOT safe to resend via + // `send_until` (its own doc comment restricts that to idempotent keys) — + // a resend while the popup is already open would immediately close it. + // Plain `send` + `wait_for_screen` instead. + let tui = session(world); + tui.send("\r") + .unwrap_or_else(|e| panic!("failed to send Enter: {e}")); + tui.wait_for_screen("Instance · ", default_timeout()) .await - .unwrap_or_else(|e| panic!("failed to open instance detail: {e}")); + .unwrap_or_else(|e| panic!("instance detail did not open: {e}")); } #[when("the user opens the services manager")] @@ -202,6 +215,19 @@ async fn open_command_palette(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to open the command palette: {e}")); } +#[when("the user opens the theme picker")] +async fn open_theme_picker(world: &mut E2eWorld) { + let tui = session(world); + tui.send("t") + .unwrap_or_else(|e| panic!("failed to open the theme picker: {e}")); + tui.wait_for_screen( + "Theme — j/k select, Enter apply, Esc cancel", + default_timeout(), + ) + .await + .unwrap_or_else(|e| panic!("theme picker did not open: {e}")); +} + #[when("the user chooses Serving")] async fn choose_serving(world: &mut E2eWorld) { let tui = session(world); @@ -533,6 +559,16 @@ async fn dashboard_destinations_displayed(world: &mut E2eWorld) { ); } +#[then("the dashboard menu is displayed")] +async fn dashboard_menu_is_displayed(world: &mut E2eWorld) { + // "Options" only ever renders as one of the main menu's three items + // (Options/Help/Quit) — a stable, unique marker for `Modal::Menu`. + session(world) + .wait_for_screen("Options", default_timeout()) + .await + .unwrap_or_else(|e| panic!("dashboard menu did not appear: {e}")); +} + #[then("Serving actions are displayed")] async fn serving_actions_displayed(world: &mut E2eWorld) { session(world) From 251d5ae47f2dc9956944b9fed31bc3452f910153 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 10 Sep 2026 07:23:48 +0000 Subject: [PATCH 12/36] fix(tui): clamp help scroll and correct back-out narrative Address PR #358 review feedback: - help_scroll had no upper bound, so jumping to the end of Help set it to i16::MAX/u16::MAX; scrolling back up then took ~32k keypresses to move. Add help_max_scroll, written back by the renderer each frame (mirroring chat_max_scroll), and clamp scroll_help against it. - Correct the should_pane_back_out doc comment and its test/e2e framing: the tab-guard removal is a harmless generalization, not a fix for a real bug (every overlay already self-closed on root Esc via its own event-loop arm regardless of active_tab). - Log (instead of silently dropping) a mock approval trigger send failure in agent.rs. - Document --chat-mock's hidden "install the sdk" approval-trigger phrase in docs/demos.md. - Gate the footer's "Esc back out" chip on active_overlay_at_root() too, so it doesn't over-promise while a sub-popup/job console is open inside a manager; simplify the associated test. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/agent.rs | 6 +- crates/rocm-dash-tui/src/app/mod.rs | 59 +++++++++++++++---- crates/rocm-dash-tui/src/ui/mod.rs | 14 +++-- crates/rocm-dash-tui/src/ui/modal.rs | 44 ++++++++------ crates/rocm-dash-tui/src/ui/tabs/instances.rs | 1 + docs/demos.md | 6 +- 6 files changed, 93 insertions(+), 37 deletions(-) diff --git a/crates/rocm-dash-tui/src/agent.rs b/crates/rocm-dash-tui/src/agent.rs index acd45c291..f18d2e7aa 100644 --- a/crates/rocm-dash-tui/src/agent.rs +++ b/crates/rocm-dash-tui/src/agent.rs @@ -1608,9 +1608,11 @@ impl AgentClient for MockAgentClient { && t.content.to_lowercase().contains(&trigger.phrase) ); if fires { - let _ = trigger.tx.send(ClientMsg::ChatApprovalRequired { + if let Err(e) = trigger.tx.send(ClientMsg::ChatApprovalRequired { intent: trigger.intent.clone(), - }); + }) { + warn!(error = %e, "mock approval trigger dropped: receiver gone"); + } return Ok( "This action needs operator approval; it has been surfaced to \ the operator." diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 339b16056..8b04ca931 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -500,6 +500,12 @@ pub struct AppState { /// Scroll offset (in lines) inside the Help / GlobalHelp overlays. Both /// modals are mutually exclusive so one field suffices; reset on open. pub help_scroll: u16, + /// Last-measured upper bound for `help_scroll`, written back by the + /// renderer each frame (see `ui::modal::draw_help` / + /// `draw_global_help`), mirroring `chat_max_scroll`. Clamps `scroll_help` + /// so a "jump to end" (`i16::MAX`) can't leave the offset far past the + /// real content length. + pub help_max_scroll: u16, /// Vertical scroll offset (first visible line) of the active job console. /// Shared by whichever operational manager is showing its console; reset /// when an overlay opens (`close_overlays`). @@ -697,6 +703,7 @@ impl AppState { theme_picker_sel, bench_detail_scroll: 0, help_scroll: 0, + help_max_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, @@ -986,7 +993,7 @@ impl AppState { /// manager is open at a time, so this reflects that one; `true` when none is /// open. Gates the Esc back-out so Esc cancels the innermost layer first /// (and is ignored while a job runs) before it can eject the manager. - fn active_overlay_at_root(&self) -> bool { + pub(crate) fn active_overlay_at_root(&self) -> bool { self.serve_wizard.as_ref().is_none_or(|w| { w.browser.is_none() && w.picker.is_none() @@ -1047,11 +1054,18 @@ impl AppState { /// mutation lives in the event-loop arm). /// /// Not just ROCm/Serving: a manager can be opened from a non-domain tab - /// (e.g. `examine_manager` from an Observe hotkey), and Esc must be able to - /// close it there too — otherwise it falls through to the global `OpenMenu` - /// arm while the manager overlay keeps rendering on top, leaving `Modal` - /// set but invisible. `pane_focus` is meaningless outside Rocm/Serving, so - /// resetting it there is a harmless no-op. + /// (e.g. `examine_manager` from an Observe hotkey). This used to be gated + /// on `active_tab == Rocm | Serving`, so on other tabs the manager's own + /// event-loop arm handled root Esc directly (every overlay type already + /// has a dedicated `Some(Ok(CtEvent::Key(k))) if state..is_some()` + /// arm ahead of the generic handler, and each self-closes on root Esc + /// regardless of `active_tab` — so there was no "Modal stays set but + /// invisible" bug to fix here). Dropping the tab guard is a harmless + /// generalization: it moves the close from the manager's own `on_key` to + /// this shared path (`close_overlays()` + `pane_focus = Actions`) so a + /// future manager doesn't need to duplicate that root-Esc handling. + /// `pane_focus` is meaningless outside Rocm/Serving, so resetting it there + /// is a harmless no-op. /// /// When the manager has a sub-popup / approval / job console open, this is /// `false` so Esc falls through to the manager's own handler (cancel the @@ -1119,11 +1133,15 @@ impl AppState { self.help_scroll = 0; } - /// Adjust the Help / GlobalHelp scroll. `delta` is in lines; clamped at 0 - /// (no upper bound — the renderer clamps against the actual line count). + /// Adjust the Help / GlobalHelp scroll. `delta` is in lines; clamped + /// against `[0, help_max_scroll]` (the latter is last written back by the + /// renderer, see `help_max_scroll`), so `i16::MIN`/`i16::MAX` ("jump to + /// start/end") land exactly on `0`/`help_max_scroll` instead of + /// overflowing into an offset far past the real content length. pub fn scroll_help(&mut self, delta: i16) { let cur = i32::from(self.help_scroll); - let next = u16::try_from((cur + i32::from(delta)).max(0)).unwrap_or(u16::MAX); + let max = i32::from(self.help_max_scroll); + let next = u16::try_from((cur + i32::from(delta)).clamp(0, max)).unwrap_or(u16::MAX); self.help_scroll = next; } @@ -3724,8 +3742,11 @@ mod tests { s.active_tab = ActiveTab::Rocm; assert!(!s.should_pane_back_out(crossterm::event::KeyCode::Esc)); // Manager open on a non-domain tab (opened from Observe hotkey) → - // Esc still backs out, closing the manager (item #35: no dead corner - // where an overlay survives a tab switch and swallows Esc silently). + // Esc backs out uniformly regardless of tab, now that the + // Rocm/Serving-only gate is gone. New coverage of the generalized + // behavior — the manager's own event-loop arm already closed it on + // this tab before the gate was removed, so this isn't a regression + // test for a prior bug. s.active_tab = ActiveTab::Observe; s.examine_manager = Some(crate::ui::examine_manager::ExamineManagerState::default()); assert!(s.has_open_overlay()); @@ -5508,6 +5529,22 @@ mod tests { assert_eq!(s.help_scroll, 0); } + #[test] + fn scroll_help_clamps_jump_to_end_so_scrolling_back_up_moves_immediately() { + // Regression: `scroll_help` used to clamp only at 0, with no upper + // bound, so `G`/`End` (ScrollModal(i16::MAX)) set help_scroll to + // 32767 regardless of the real content length. Scrolling back up by + // one row then took ~32700 keypresses to have any visible effect. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.help_max_scroll = 10; + s.scroll_help(i16::MAX); // "jump to end" + assert_eq!(s.help_scroll, 10, "jump-to-end lands exactly on the max"); + s.scroll_help(-1); // one `k`/`Up` + assert_eq!(s.help_scroll, 9, "scrolling up moves immediately, not after ~32k presses"); + s.scroll_help(i16::MIN); // "jump to start" + assert_eq!(s.help_scroll, 0); + } + #[test] fn slash_clear_empties_transcript() { let mut s = st(); diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 29b01839d..d25972ee4 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -123,7 +123,10 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { // Modal overlay (rendered last so it sits on top of the body). match state.modal { Modal::None => {} - Modal::Help => modal::draw_help(f, body, state.active_tab, &theme, state.help_scroll), + Modal::Help => { + state.help_max_scroll = + modal::draw_help(f, body, state.active_tab, &theme, state.help_scroll); + } // Observe folds the telemetry tabs; its detail modal is the instance // detail (the selectable list on that surface). Modal::Detail => { @@ -137,7 +140,9 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { Modal::Menu => modal::draw_menu(f, body, state.menu_sel, &theme), Modal::Palette => modal::draw_palette(f, body, state.palette_sel, &theme), Modal::Options => modal::draw_options(f, body, state, &theme), - Modal::GlobalHelp => modal::draw_global_help(f, body, &theme, state.help_scroll), + Modal::GlobalHelp => { + state.help_max_scroll = modal::draw_global_help(f, body, &theme, state.help_scroll); + } } // Operational managers render as a centered MODAL on every tab. The @@ -430,7 +435,7 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve if state.approval.is_some() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" cancel ")); - } else if state.has_open_overlay() { + } else if state.has_open_overlay() && state.active_overlay_at_root() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); } else if state.modal != Modal::None { @@ -592,10 +597,9 @@ mod tests { term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) .unwrap(); - let esc = chips + let _ = chips .iter() .find(|c| c.action == KeyAction::OpenMenu) .expect("a fallback Esc chip opening the menu must always be present"); - let _ = esc; } } diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index 153fb7358..ad46a22df 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -66,7 +66,10 @@ pub fn draw_popup_frame(f: &mut Frame, area: Rect, title: &str, theme: &Theme) - /// past the viewport and render a blank pane. The clamp is computed from the /// *wrapped* row count (`Paragraph::line_count`), not `lines.len()` — `scroll` /// is applied post-wrap, so a pre-wrap count under-clamps whenever a line -/// wraps and leaves trailing content permanently unreachable. +/// wraps and leaves trailing content permanently unreachable. Returns that +/// computed last-page offset so callers can write it back into app state +/// (see `AppState::help_max_scroll`) and clamp future scroll deltas against +/// the real content length instead of just this frame's render clamp. pub fn draw_scrollable_lines( f: &mut Frame, area: Rect, @@ -74,10 +77,10 @@ pub fn draw_scrollable_lines( lines: Vec, scroll: u16, theme: &Theme, -) { +) -> u16 { let inner = draw_popup_frame(f, area, title, theme); if inner.height == 0 { - return; + return 0; } let p = Paragraph::new(lines).wrap(Wrap { trim: false }); let max_scroll = u16::try_from(p.line_count(inner.width)) @@ -85,6 +88,7 @@ pub fn draw_scrollable_lines( .saturating_sub(inner.height); let p = p.scroll((scroll.min(max_scroll), 0)); f.render_widget(p, inner); + max_scroll } /// Render the Help modal for the active tab. @@ -92,11 +96,10 @@ pub fn draw_scrollable_lines( /// Shares chrome (dimmed backdrop, popup geometry, scrollable single-column /// layout) with `draw_global_help` so the two help screens read as one /// family; unlike that screen, this one has an extra group — the active -/// tab's own keys. `scroll` is the first visible line offset (see -/// [`draw_scrollable_lines`]): a fixed two-column split used to clip content -/// at small terminal sizes, since a group's rows could run past the popup's -/// height with no way to reach them. -pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scroll: u16) { +/// tab's own keys. `scroll` is the first visible line offset; returns the +/// last-page offset computed by [`draw_scrollable_lines`] so the caller can +/// clamp future scroll deltas against it. +pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scroll: u16) -> u16 { grey_overlay(f); let popup = centered_rect(80, 80, 100, 26, area); @@ -156,7 +159,7 @@ pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scrol ]; let lines = help_group_lines(groups, theme); - draw_scrollable_lines(f, popup, "Help", lines, scroll, theme); + draw_scrollable_lines(f, popup, "Help", lines, scroll, theme) } fn key_line<'a>(key: &'a str, desc: &'a str, theme: &Theme) -> Line<'a> { @@ -578,8 +581,9 @@ pub fn draw_options(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) /// Distinct from the contextual per-tab `?` help (`draw_help`), but shares its /// chrome — dimmed backdrop, popup geometry, and scrollable single-column /// layout (see [`draw_help`] for why). `scroll` is the first visible line -/// offset (see [`draw_scrollable_lines`]). -pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme, scroll: u16) { +/// offset; returns the last-page offset computed by [`draw_scrollable_lines`] +/// so the caller can clamp future scroll deltas against it. +pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme, scroll: u16) -> u16 { grey_overlay(f); let popup = centered_rect(80, 80, 100, 26, area); let groups: &[(&str, &[(&str, &str)])] = &[ @@ -614,7 +618,7 @@ pub fn draw_global_help(f: &mut Frame, area: Rect, theme: &Theme, scroll: u16) { ), ]; let lines = help_group_lines(groups, theme); - draw_scrollable_lines(f, popup, "Keyboard", lines, scroll, theme); + draw_scrollable_lines(f, popup, "Keyboard", lines, scroll, theme) } /// Flatten keyboard-help groups into the `Vec` shape `draw_help` and @@ -848,7 +852,9 @@ mod ported_chrome_tests { ); assert!(options.contains("General"), "options missing tab label"); - let help = render(&|f| super::draw_global_help(f, area, &theme, 0)); + let help = render(&|f| { + super::draw_global_help(f, area, &theme, 0); + }); assert!( help.contains("Keyboard"), "global help missing title: {help:?}" @@ -886,8 +892,10 @@ mod ported_chrome_tests { let area = Rect::new(0, 0, 80, 24); let backend = TestBackend::new(80, 24); let mut term = Terminal::new(backend).unwrap(); - term.draw(|f| super::draw_help(f, area, ActiveTab::Home, &theme, i16::MAX as u16)) - .unwrap(); + term.draw(|f| { + super::draw_help(f, area, ActiveTab::Home, &theme, i16::MAX as u16); + }) + .unwrap(); let out = flat(&term); assert!( out.contains("REPLAY") && out.contains("pause / resume"), @@ -909,8 +917,10 @@ mod ported_chrome_tests { let area = Rect::new(0, 0, 80, 24); let backend = TestBackend::new(80, 24); let mut term = Terminal::new(backend).unwrap(); - term.draw(|f| super::draw_help(f, area, ActiveTab::Chat, &theme, i16::MAX as u16)) - .unwrap(); + term.draw(|f| { + super::draw_help(f, area, ActiveTab::Chat, &theme, i16::MAX as u16); + }) + .unwrap(); let out = flat(&term); assert!( out.contains("REPLAY") && out.contains("60s"), diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index b392c3112..1399ca5df 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -923,6 +923,7 @@ mod tests { theme_picker_sel: 0, bench_detail_scroll: 0, help_scroll: 0, + help_max_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, diff --git a/docs/demos.md b/docs/demos.md index 6e33f26f4..f6b54dba0 100644 --- a/docs/demos.md +++ b/docs/demos.md @@ -49,8 +49,10 @@ already-ready environment. The Console needs no ROCm hardware: `rocm dash --demo` replays the project's seeded synthetic telemetry through the same UI as a live daemon, and visibly marks it **SIMULATED DATA**. `--chat-mock` provides the deterministic offline -chat response. The CLI's service and chat commands use only the loopback mock. -Neither demo downloads a model or calls a cloud provider. +chat response; a message containing the phrase "install the sdk" instead +triggers the mock's tool-approval demo path, surfacing an approval prompt +instead of the plain reply. The CLI's service and chat commands use only the +loopback mock. Neither demo downloads a model or calls a cloud provider. ## Storyboards From b370b3eaaf8699215eeb69995785cdce7a43675e Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 10 Sep 2026 14:15:41 +0000 Subject: [PATCH 13/36] fix(tui): apply cargo fmt, fix footer chip fallback, reword dash-11 - cargo fmt: reformat the multi-line assert_eq! in the help-scroll clamp regression test. - Fix a real regression from the previous fix: the footer's Esc chip fell through to a clickable "menu" chip when an overlay was open but not at its root layer (e.g. a sub-popup or job console), even though Esc is actually consumed by that layer. Add a distinct non-clickable "cancel" chip for that case, with a regression test. - Reset help_max_scroll alongside help_scroll in reset_help_scroll, so the two don't rely on draw-before-key-handling ordering. - Reword the dash-11 e2e scenario and its step text: it's characterization coverage of Esc closing a manager on any tab, not a regression test, since the manager's own event-loop arm already closes it independent of the tab-guard removal. The discriminating coverage is the existing back_out_requires_an_open_manager_on_any_tab unit test. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 6 ++- crates/rocm-dash-tui/src/ui/mod.rs | 43 ++++++++++++++++++++++ tests/e2e-cucumber/features/dash.feature | 13 +++++-- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 6 +-- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 8b04ca931..0bc5af9df 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1131,6 +1131,7 @@ impl AppState { /// modal, so a stale offset never carries over from a previous session). pub const fn reset_help_scroll(&mut self) { self.help_scroll = 0; + self.help_max_scroll = 0; } /// Adjust the Help / GlobalHelp scroll. `delta` is in lines; clamped @@ -5540,7 +5541,10 @@ mod tests { s.scroll_help(i16::MAX); // "jump to end" assert_eq!(s.help_scroll, 10, "jump-to-end lands exactly on the max"); s.scroll_help(-1); // one `k`/`Up` - assert_eq!(s.help_scroll, 9, "scrolling up moves immediately, not after ~32k presses"); + assert_eq!( + s.help_scroll, 9, + "scrolling up moves immediately, not after ~32k presses" + ); s.scroll_help(i16::MIN); // "jump to start" assert_eq!(s.help_scroll, 0); } diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index d25972ee4..531f7da09 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -438,6 +438,13 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve } else if state.has_open_overlay() && state.active_overlay_at_root() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); + } else if state.has_open_overlay() { + // A manager is open but not at its root layer (sub-popup, picker, + // approval, or job console) — Esc is handled by that layer's own + // event-loop arm, not by `should_pane_back_out`/`OpenMenu`. `None` + // keeps the chip non-clickable so it can't dispatch the wrong action. + segs.push(Seg::Key("Esc", None)); + segs.push(Seg::Sep(" cancel ")); } else if state.modal != Modal::None { segs.push(Seg::Key("Esc", Some(KeyAction::CloseModal))); segs.push(Seg::Sep(" close ")); @@ -602,4 +609,40 @@ mod tests { .find(|c| c.action == KeyAction::OpenMenu) .expect("a fallback Esc chip opening the menu must always be present"); } + + #[test] + fn footer_esc_chip_is_not_clickable_menu_when_overlay_has_a_sub_popup_open() { + // Regression: with a manager open but not at its root layer (here, a + // running job console), `has_open_overlay()` is true but + // `active_overlay_at_root()` is false. The chip must not fall through + // to the generic `OpenMenu` arm — that key is actually consumed by the + // manager's own event-loop arm, which cancels the sub-layer, not the + // menu. Any chip shown here must be non-clickable (`action == None`). + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.serve_wizard = Some(crate::ui::serve_wizard::ServeWizardState { + active_job: Some("job".into()), + ..Default::default() + }); + assert!(state.has_open_overlay()); + assert!(!state.active_overlay_at_root()); + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + let mut chips = Vec::new(); + term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) + .unwrap(); + + for chip in &chips { + assert_ne!( + chip.action, + KeyAction::OpenMenu, + "no chip may dispatch OpenMenu while a sub-popup owns Esc" + ); + } + } } diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index f0be86d64..bd3c8d141 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -126,14 +126,21 @@ Feature: Interactive dashboard When the user quits the launcher Then the launcher exits successfully - @id:dash-manager-escape-closes-without-menu @requires-os:linux - Scenario: dash-11 - Escape closes a non-domain-tab manager overlay instead of opening the menu + # Characterization coverage: this scenario observes that Escape closes the + # manager on a non-domain tab, but the services manager's own event-loop arm + # would close it on root Esc even without the tab-independent back-out path + # this PR generalized, so a revert of that change would not turn this red. + # The discriminating regression test for that change is the unit test + # `back_out_requires_an_open_manager_on_any_tab` in crates/rocm-dash-tui's + # app/mod.rs, which does fail on revert. + @id:dash-manager-escape-closes-on-any-tab @requires-os:linux + Scenario: dash-11 - Escape closes a manager overlay on any tab When the user opens the dashboard with demo data And the user opens the Observe view And the user opens the services manager Then the services manager is displayed When the user presses Escape - Then the services manager is closed without opening the menu + Then the services manager is closed When the user quits the dashboard Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index b15148d0e..bd57823e6 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -511,8 +511,8 @@ async fn services_manager_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("the services manager did not appear: {e}")); } -#[then("the services manager is closed without opening the menu")] -async fn services_manager_closed_without_menu(world: &mut E2eWorld) { +#[then("the services manager is closed")] +async fn services_manager_closed(world: &mut E2eWorld) { let tui = session(world); tui.wait_until_gone("Services — managed inference servers", default_timeout()) .await @@ -524,7 +524,7 @@ async fn services_manager_closed_without_menu(world: &mut E2eWorld) { ); assert!( !screen.contains("Options") && !screen.contains("Quit"), - "Escape opened the main menu instead of closing the manager:\n{screen}" + "the main menu is open on top of the closed manager:\n{screen}" ); } From e8246c87c42617fad3fa13dcdbcf3c05149042c3 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 11 Sep 2026 05:38:03 +0000 Subject: [PATCH 14/36] fix(e2e): wait for populated instance table before opening detail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dash-13's Enter press raced the demo replay's InstanceDiscovered events; the "● Observe" tab marker only proves the tab switch rendered, not that any instance exists yet, so OpenDetail could be silently ignored on a slow run. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index bd57823e6..f0b59f60d 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -163,6 +163,14 @@ async fn open_instance_detail(world: &mut E2eWorld) { // a resend while the popup is already open would immediately close it. // Plain `send` + `wait_for_screen` instead. let tui = session(world); + // The `● Observe` marker asserted by `open_observe_view` only proves the + // tab switch rendered — the demo replay's `InstanceDiscovered` events + // land afterward. Sending Enter before they do finds an empty instance + // list (`selection_len()` == 0), so `OpenDetail` is silently ignored. + // Wait for the populated table before the (non-retryable) Enter. + tui.wait_for_screen("Instances · AI metrics", default_timeout()) + .await + .unwrap_or_else(|e| panic!("instance list did not populate: {e}")); tui.send("\r") .unwrap_or_else(|e| panic!("failed to send Enter: {e}")); tui.wait_for_screen("Instance · ", default_timeout()) From a7bc4dd5bcd094e5997e542b38191de0aaf980d5 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 07:32:19 +0000 Subject: [PATCH 15/36] fix(tui): unify Esc-closes-console predicate, add scrollbar affordance Extract job_console::console_esc_closes as the single source of truth for "does Esc fully close the overlay (job running) vs merely dismiss the console (job finished/missing)", consumed by on_console_key, the footer's Esc-chip label, and focused_close_key_blocked so the three can't drift out of sync. Also: - Footer now labels the Esc chip "close" (not "cancel") when a job console over a running job is open, matching its actual behavior. - draw_scrollable_lines renders a scrollbar thumb when help content overflows, instead of no affordance at all. - Assert help_max_scroll resets to 0 alongside help_scroll in the /help and /? slash-command tests. - Note in dash_steps.rs that the Options/Quit absence check is defensive-only coverage against Esc falling through to the menu, not the manager-closed regression check itself. Addresses all 5 non-blocking findings from siloteemu's automated review on PR #358. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 7 +- crates/rocm-dash-tui/src/ui/job_console.rs | 22 ++-- crates/rocm-dash-tui/src/ui/mod.rs | 143 ++++++++++++++++++++- crates/rocm-dash-tui/src/ui/modal.rs | 49 ++++++- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 4 + 5 files changed, 204 insertions(+), 21 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 0bc5af9df..e1f5fd393 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1620,8 +1620,7 @@ fn focused_close_key_blocked(state: &AppState, focus: Option, code: KeyCo } let running = state .active_job_id() - .and_then(|id| state.jobs.job(id)) - .is_some_and(|j| !j.is_terminal()); + .is_some_and(|id| ui::job_console::console_esc_closes(state.jobs.job(id))); running && matches!(code, KeyCode::Char('q') | KeyCode::Esc) } @@ -5509,9 +5508,11 @@ mod tests { fn slash_help_resets_stale_scroll_offset() { let mut s = st(); s.help_scroll = 42; + s.help_max_scroll = 42; assert_eq!(s.handle_slash_command("/help"), SlashOutcome::Handled); assert_eq!(s.modal, Modal::Help); assert_eq!(s.help_scroll, 0); + assert_eq!(s.help_max_scroll, 0); } #[test] @@ -5525,9 +5526,11 @@ mod tests { fn slash_question_mark_resets_stale_scroll_offset() { let mut s = st(); s.help_scroll = 17; + s.help_max_scroll = 17; assert_eq!(s.handle_slash_command("/?"), SlashOutcome::Handled); assert_eq!(s.modal, Modal::Help); assert_eq!(s.help_scroll, 0); + assert_eq!(s.help_max_scroll, 0); } #[test] diff --git a/crates/rocm-dash-tui/src/ui/job_console.rs b/crates/rocm-dash-tui/src/ui/job_console.rs index 68eec0988..3bf21c9cb 100644 --- a/crates/rocm-dash-tui/src/ui/job_console.rs +++ b/crates/rocm-dash-tui/src/ui/job_console.rs @@ -40,6 +40,16 @@ pub enum ConsoleOutcome { Unhandled, } +/// Whether Esc on `job` fully closes the owning overlay, rather than merely +/// dismissing the console back to the screen body. +/// +/// `true` when the job is still running; `false` once it's finished or +/// missing. Shared by [`on_console_key`] and the dashboard footer's Esc-chip +/// label so the two can't drift out of sync. +pub fn console_esc_closes(job: Option<&JobState>) -> bool { + job.is_some_and(|j| !j.is_terminal()) +} + /// Interpret a key while a job console is showing `job_id`. Pure except for the /// `CancelJob` reducer apply (which only mutates the in-memory job model). pub fn on_console_key(job_id: &str, jobs: &mut State, key: KeyEvent) -> ConsoleOutcome { @@ -53,16 +63,10 @@ pub fn on_console_key(job_id: &str, jobs: &mut State, key: KeyEvent) -> ConsoleO // Esc on a still-running job leaves the overlay (the job keeps running in // the background) — the conventional "get me out" key, so the user is // never trapped during a long step (e.g. a managed serve readiness wait). - KeyCode::Esc if jobs.job(job_id).is_some_and(|j| !j.is_terminal()) => { - ConsoleOutcome::Closed - } + KeyCode::Esc if console_esc_closes(jobs.job(job_id)) => ConsoleOutcome::Closed, // On a finished (or vanished) job, Esc/Enter dismiss the console back to // the screen body. - KeyCode::Esc | KeyCode::Enter - if jobs - .job(job_id) - .is_none_or(rocm_dash_core::state::JobState::is_terminal) => - { + KeyCode::Esc | KeyCode::Enter if !console_esc_closes(jobs.job(job_id)) => { ConsoleOutcome::Dismissed } _ => ConsoleOutcome::Unhandled, @@ -210,7 +214,7 @@ pub fn draw_job_console( let hints = if matches!(job.status, JobStatus::Running) { "Esc close (keeps running) · Ctrl+C cancel · wheel / PgUp·PgDn scroll" } else { - "Enter/Esc close · wheel / PgUp·PgDn scroll" + "Enter/Esc dismiss · wheel / PgUp·PgDn scroll" }; f.render_widget( Paragraph::new(Line::from(Span::styled( diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 531f7da09..23067219d 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -438,11 +438,26 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve } else if state.has_open_overlay() && state.active_overlay_at_root() { segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" back out ")); + } else if state.has_open_overlay() + && state + .active_job_id() + .is_some_and(|id| crate::ui::job_console::console_esc_closes(state.jobs.job(id))) + { + // A manager's job console is showing a still-running job — Esc fully + // closes the overlay there (the job keeps running in the background), + // matching the console's own footer hint ("Esc close (keeps + // running)"), not the generic sub-popup "cancel" below. Once the job + // finishes, `on_console_key` only dismisses the console back to the + // screen body (the overlay stays open), so that case falls through to + // the "cancel" arm below, which already describes it correctly. Shares + // `console_esc_closes` with `on_console_key` so the two can't drift. + segs.push(Seg::Key("Esc", None)); + segs.push(Seg::Sep(" close ")); } else if state.has_open_overlay() { - // A manager is open but not at its root layer (sub-popup, picker, - // approval, or job console) — Esc is handled by that layer's own - // event-loop arm, not by `should_pane_back_out`/`OpenMenu`. `None` - // keeps the chip non-clickable so it can't dispatch the wrong action. + // A manager is open but not at its root layer (sub-popup, picker, or + // approval) — Esc is handled by that layer's own event-loop arm, not + // by `should_pane_back_out`/`OpenMenu`. `None` keeps the chip + // non-clickable so it can't dispatch the wrong action. segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" cancel ")); } else if state.modal != Modal::None { @@ -613,11 +628,13 @@ mod tests { #[test] fn footer_esc_chip_is_not_clickable_menu_when_overlay_has_a_sub_popup_open() { // Regression: with a manager open but not at its root layer (here, a - // running job console), `has_open_overlay()` is true but + // folder browser sub-popup), `has_open_overlay()` is true but // `active_overlay_at_root()` is false. The chip must not fall through // to the generic `OpenMenu` arm — that key is actually consumed by the // manager's own event-loop arm, which cancels the sub-layer, not the - // menu. Any chip shown here must be non-clickable (`action == None`). + // menu. Any chip shown here must be non-clickable (`action == None`) + // and labeled "cancel" (this sub-popup has no "close means job keeps + // running" nuance, unlike a job console — see the "close" test below). use crate::ui::theme::Theme; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -625,7 +642,10 @@ mod tests { let theme = Theme::from_name("default-dark"); let mut state = AppState::new("t".into(), "default-dark".into()); state.serve_wizard = Some(crate::ui::serve_wizard::ServeWizardState { - active_job: Some("job".into()), + browser: Some(crate::ui::folder_browser::FolderBrowser::new( + "t", + std::env::temp_dir(), + )), ..Default::default() }); assert!(state.has_open_overlay()); @@ -644,5 +664,114 @@ mod tests { "no chip may dispatch OpenMenu while a sub-popup owns Esc" ); } + let row: String = (0..90) + .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) + .collect(); + assert!( + row.contains("cancel"), + "sub-popup Esc chip should say cancel: {row:?}" + ); + assert!( + !row.contains("close"), + "sub-popup Esc chip should not say close: {row:?}" + ); + } + + #[test] + fn footer_esc_chip_labels_close_when_job_console_is_open() { + // A manager's job console is showing a still-running job — Esc fully + // closes the overlay there (matching the console's own "Esc close + // (keeps running)" footer hint), so the dashboard footer chip must say + // "close", not the generic sub-popup "cancel". + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + use rocm_dash_core::state::StateEvent; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.jobs.apply(StateEvent::StartJob { + id: "job".into(), + cmd: "echo".into(), + args: vec!["hi".into()], + }); + state.serve_wizard = Some(crate::ui::serve_wizard::ServeWizardState { + active_job: Some("job".into()), + ..Default::default() + }); + assert!(state.has_open_overlay()); + assert!(!state.active_overlay_at_root()); + assert!(state.active_job_id().is_some()); + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + term.draw(|f| { + let _ = draw_footer(f, f.area(), &state, &theme); + }) + .unwrap(); + + let row: String = (0..90) + .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) + .collect(); + assert!( + row.contains("close"), + "job console Esc chip should say close: {row:?}" + ); + assert!( + !row.contains("cancel"), + "job console Esc chip should not say cancel: {row:?}" + ); + } + + #[test] + fn footer_esc_chip_labels_cancel_when_job_console_shows_a_finished_job() { + // Once the job console's job has finished, Esc only dismisses the + // console back to the screen body (the overlay itself stays open) — + // `on_console_key` never returns `Closed` for a terminal job. The + // footer chip must not claim "close" here; it falls through to the + // generic sub-popup "cancel" label, which already describes this + // case correctly. + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + use rocm_dash_core::state::StateEvent; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.jobs.apply(StateEvent::StartJob { + id: "job".into(), + cmd: "echo".into(), + args: vec!["hi".into()], + }); + state.jobs.apply(StateEvent::JobDone { + id: "job".into(), + code: 0, + }); + state.serve_wizard = Some(crate::ui::serve_wizard::ServeWizardState { + active_job: Some("job".into()), + ..Default::default() + }); + assert!(state.has_open_overlay()); + assert!(!state.active_overlay_at_root()); + assert!(state.active_job_id().is_some()); + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + term.draw(|f| { + let _ = draw_footer(f, f.area(), &state, &theme); + }) + .unwrap(); + + let row: String = (0..90) + .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) + .collect(); + assert!( + row.contains("cancel"), + "finished-job console Esc chip should say cancel: {row:?}" + ); + assert!( + !row.contains("close"), + "finished-job console Esc chip should not say close: {row:?}" + ); } } diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index ad46a22df..692d913bf 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -70,6 +70,14 @@ pub fn draw_popup_frame(f: &mut Frame, area: Rect, title: &str, theme: &Theme) - /// computed last-page offset so callers can write it back into app state /// (see `AppState::help_max_scroll`) and clamp future scroll deltas against /// the real content length instead of just this frame's render clamp. +/// +/// Draws a scrollbar thumb on the right edge when content overflows — a +/// widely recognized affordance for "there's more below" that a footer hint +/// alone doesn't convey. The overflow check (and thus `max_scroll`) is +/// wrapped at the *full* inner width first, before the scrollbar reserves its +/// column: narrowing the width can only ever add more wrapped rows, never +/// remove the overflow that triggered the bar, so this ordering keeps the +/// decision to draw a bar and the final wrap consistent. pub fn draw_scrollable_lines( f: &mut Frame, area: Rect, @@ -83,11 +91,20 @@ pub fn draw_scrollable_lines( return 0; } let p = Paragraph::new(lines).wrap(Wrap { trim: false }); - let max_scroll = u16::try_from(p.line_count(inner.width)) + let full_count = u16::try_from(p.line_count(inner.width)).unwrap_or(u16::MAX); + let content_area = panel::vertical_scrollbar( + f, + inner, + full_count as usize, + inner.height as usize, + scroll as usize, + theme, + ); + let max_scroll = u16::try_from(p.line_count(content_area.width)) .unwrap_or(u16::MAX) - .saturating_sub(inner.height); + .saturating_sub(content_area.height); let p = p.scroll((scroll.min(max_scroll), 0)); - f.render_widget(p, inner); + f.render_widget(p, content_area); max_scroll } @@ -928,4 +945,30 @@ mod ported_chrome_tests { entry, not clamp short of it: {out:?}" ); } + + #[test] + fn help_overflow_shows_scrollbar_thumb_and_reports_positive_max_scroll() { + // A short viewport guarantees Chat's (longest) help content overflows, + // which should both report a positive max_scroll and render a + // scrollbar thumb — the "there's more below" affordance. + use crate::app::ActiveTab; + let theme = Theme::from_name("default-dark"); + let area = Rect::new(0, 0, 80, 10); + let backend = TestBackend::new(80, 10); + let mut term = Terminal::new(backend).unwrap(); + let mut max_scroll = 0; + term.draw(|f| { + max_scroll = super::draw_help(f, area, ActiveTab::Chat, &theme, 0); + }) + .unwrap(); + assert!( + max_scroll > 0, + "a short viewport should report overflow via a positive max_scroll" + ); + let out = flat(&term); + assert!( + out.contains('█'), + "overflowing help should render a scrollbar thumb: {out:?}" + ); + } } diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index f0b59f60d..e5be39d56 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -530,6 +530,10 @@ async fn services_manager_closed(world: &mut E2eWorld) { screen.contains("● Observe"), "Escape left the Observe tab entirely, not just the manager:\n{screen}" ); + // Belt-and-suspenders: `wait_until_gone` above is the primary regression + // check (the manager itself closed). This additionally guards against + // Esc falling through to open the main menu instead — "Options"/"Quit" + // are unique to `Modal::Menu`. assert!( !screen.contains("Options") && !screen.contains("Quit"), "the main menu is open on top of the closed manager:\n{screen}" From 17627480230d5c8203b71c79c2a049ccf269fed2 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 08:21:59 +0000 Subject: [PATCH 16/36] fix(e2e): isolate timing-sensitive dash scenarios from concurrent contention dash-08 and dash-09 assert wall-clock-pinned EAI-7960 hold/expiry boundaries (6s validity window). Under the no-GPU job's default 64-way concurrent scenario pool, CPU contention from other scenarios can delay their polling/assertions past the window, causing spurious CI failures even though observation.rs's hold/expiry logic is correct. Tag both scenarios @serial so cucumber's built-in scenario-type dispatch runs them outside the concurrent pool, and update stale RED/GREEN TDD-phase comments that no longer reflect the current (already-fixed) product code. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 29 ++++++++++++---------- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 20 ++++----------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index bd3c8d141..0083e7f2b 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -72,15 +72,15 @@ Feature: Interactive dashboard Then the dashboard exits successfully - @id:dash-gen-tps-held-after-scrape-failure @requires-os:linux + @id:dash-gen-tps-held-after-scrape-failure @requires-os:linux @serial Scenario: dash-08 - Gen throughput stays visible for the validity window after a scrape failure - # EAI-7960 principal regression: after establishing a positive gen_tps - # baseline through the scripted mock, a single /metrics transport failure - # must NOT immediately clear the displayed "tok/s" value. The contract - # requires the held value to remain visible for the validity window - # clamp(3 x instance_tick, 6 s, 30 s). Current code has no such window - # (runner.rs clears gen_tps on the same tick as the failure), so the - # "generation throughput remains visible" step is the RED assertion. + # EAI-7960: after establishing a positive gen_tps baseline through the + # scripted mock, a single /metrics transport failure must NOT immediately + # clear the displayed "tok/s" value — the held value must remain visible + # for the validity window clamp(3 x instance_tick, 6 s, 30 s) (6 s for the + # production 2 s tick). @serial keeps this scenario off the no-GPU job's + # 64-way concurrent lane so CPU contention from other scenarios can't eat + # into its tight wall-clock budget and cause a spurious expiry. Given a managed model exposes scripted serving metrics When the user opens the dashboard And the user opens the Observe view @@ -90,18 +90,21 @@ Feature: Interactive dashboard When the user quits the dashboard Then the dashboard exits successfully - @id:dash-gen-tps-expiry-boundary @requires-os:linux + @id:dash-gen-tps-expiry-boundary @requires-os:linux @serial Scenario: dash-09 - Gen throughput expires after the validity window following sustained failure # EAI-7960 expiry-boundary scenario: two contract boundaries are pinned. # # BOUNDARY 1 (held assertion) — immediately after the first failed scrape, - # gen_tps must still be visible (Held). With current code this FAILS (RED) - # because runner.rs clears gen_tps immediately. + # gen_tps must still be visible (Held). # # BOUNDARY 2 (expired assertion) — after the validity window elapses # (clamp(3 × instance_tick, 6 s, 30 s) = 6 s for the production 2 s tick), - # gen_tps must be gone from the screen. This step is unreachable today - # because BOUNDARY 1 fails first; it becomes GREEN once the fix is applied. + # gen_tps must be gone from the screen. + # + # @serial keeps this scenario off the no-GPU job's 64-way concurrent lane: + # its two boundaries are pinned to real wall-clock timing, and CPU + # contention from ~62 other concurrently-running scenarios can delay it + # past the 6 s window even though nothing is functionally broken. Given a managed model exposes scripted serving metrics When the user opens the dashboard And the user opens the Observe view diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index e5be39d56..5e6bdf538 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -741,16 +741,13 @@ async fn metrics_endpoint_fails(world: &mut E2eWorld) { tokio::time::sleep(Duration::from_millis(50)).await; } -/// EAI-7960 principal regression assertion (must be RED with current code). +/// EAI-7960 principal regression assertion. /// /// Contract: the Observe tab must still show "tok/s" immediately after the /// first failed scrape — the held value must persist for the validity window -/// `clamp(3 × instance_tick, 6 s, 30 s)` before clearing. -/// -/// **Current behaviour:** `runner.rs` lines 464-476 clear `gen_tps` on the -/// very tick that the `/metrics` fetch fails — no holding logic exists. The -/// TUI therefore renders "—" the moment the failure propagates, and this -/// assertion **FAILS**, confirming EAI-7960 is reproduced at the PTY seam. +/// `clamp(3 × instance_tick, 6 s, 30 s)` before clearing. Tagged `@serial` in +/// `dash.feature` so CPU contention from the no-GPU job's other ~62 +/// concurrently-running scenarios can't delay this step past the window. #[then("generation throughput remains visible within the validity window")] async fn gen_tps_held_after_failure(world: &mut E2eWorld) { let screen = session(world).screen_text(); @@ -758,10 +755,7 @@ async fn gen_tps_held_after_failure(world: &mut E2eWorld) { screen.contains("tok/s"), "EAI-7960 REGRESSION: gen throughput (\"tok/s\") was cleared immediately \ after the first failed scrape instead of being held for the validity \ - window (clamp(3 × instance_tick, 6 s, 30 s)).\n\ - Root cause: runner.rs clears gen_tps on the same tick as the failure; \ - no held-value / validity-window logic exists yet.\n\ - This assertion must FAIL (RED) until the fix is applied.\n\n\ + window (clamp(3 × instance_tick, 6 s, 30 s)).\n\n\ Last screen:\n{screen}" ); } @@ -790,10 +784,6 @@ async fn validity_window_elapsed(_world: &mut E2eWorld) { /// Assert that gen_tps is no longer rendered on screen (BOUNDARY 2 of the /// EAI-7960 expiry contract). After the validity window the daemon must clear /// the held value and the TUI must show "—" in place of the "tok/s" unit. -/// -/// With current code this step is unreachable because BOUNDARY 1 (the "remains -/// visible" assertion) fails first. This step becomes GREEN once the hold/expiry -/// logic is implemented. #[then("generation throughput is no longer displayed")] async fn gen_tps_no_longer_displayed(world: &mut E2eWorld) { let screen = session(world).screen_text(); From cfce2e28563b54a04e653fe55d30abe668536a3d Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 08:46:07 +0000 Subject: [PATCH 17/36] fix(dash-tui): include install_config in onboarding's overlay-at-root check active_overlay_at_root's onboarding clause enumerated browser, approval, and active_job but omitted install_config, so root Esc would eject the whole onboarding wizard while the Configure sub-view (channel/pin picker) had focus instead of deferring to it. Add the missing check, correct the now-inaccurate "harmless generalization" doc comment on should_pane_back_out, add a maintenance note on OnboardingState so a future nested sub-view field isn't dropped the same way, and add a regression test mirroring esc_defers_to_manager_when_a_subscreen_is_open. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 45 +++++++++++++++++++---- crates/rocm-dash-tui/src/ui/onboarding.rs | 8 ++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 7f2431821..5d4a7818d 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1010,7 +1010,10 @@ impl AppState { .as_ref() .is_none_or(|m| m.browser.is_none() && m.approval.is_none() && m.active_job.is_none()) && self.onboarding.as_ref().is_none_or(|m| { - m.browser.is_none() && m.approval.is_none() && m.active_job.is_none() + m.browser.is_none() + && m.install_config.is_none() + && m.approval.is_none() + && m.active_job.is_none() }) && self.runtime_manager.as_ref().is_none_or(|m| { m.browser.is_none() @@ -1066,12 +1069,18 @@ impl AppState { /// has a dedicated `Some(Ok(CtEvent::Key(k))) if state..is_some()` /// arm ahead of the generic handler, and each self-closes on root Esc /// regardless of `active_tab` — so there was no "Modal stays set but - /// invisible" bug to fix here). Dropping the tab guard is a harmless - /// generalization: it moves the close from the manager's own `on_key` to - /// this shared path (`close_overlays()` + `pane_focus = Actions`) so a - /// future manager doesn't need to duplicate that root-Esc handling. - /// `pane_focus` is meaningless outside Rocm/Serving, so resetting it there - /// is a harmless no-op. + /// invisible" bug to fix here — true of every manager except onboarding, + /// see below). Dropping the tab guard moves the close from the manager's + /// own `on_key` to this shared path (`close_overlays()` + `pane_focus = + /// Actions`) so a future manager doesn't need to duplicate that root-Esc + /// handling. `pane_focus` is meaningless outside Rocm/Serving, so + /// resetting it there is a harmless no-op. + /// + /// This generalization is only correct if `active_overlay_at_root`'s + /// per-manager clause enumerates every nesting field the manager's state + /// struct has — see the note on `OnboardingState` (and its sibling + /// manager-state structs) about keeping that enumeration in sync when a + /// new nested sub-view field is added. /// /// When the manager has a sub-popup / approval / job console open, this is /// `false` so Esc falls through to the manager's own handler (cancel the @@ -3783,6 +3792,28 @@ mod tests { assert!(s.should_pane_back_out(crossterm::event::KeyCode::Esc)); } + #[test] + fn esc_defers_to_onboarding_install_config_subview() { + // Regression coverage for the `install_config` nesting field: the + // onboarding wizard's Configure sub-view is a nested sub-view just + // like a manager's job console, so root Esc must defer to it instead + // of ejecting the whole wizard. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.active_tab = ActiveTab::Rocm; + s.onboarding = Some(crate::ui::onboarding::OnboardingState { + install_config: Some(crate::ui::onboarding::InstallConfig::default()), + ..Default::default() + }); + assert!(s.has_open_overlay()); + assert!( + !s.should_pane_back_out(crossterm::event::KeyCode::Esc), + "Esc must defer to onboarding while the Configure sub-view is open" + ); + // Once the sub-view is closed (back at root), Esc backs out again. + s.onboarding.as_mut().unwrap().install_config = None; + assert!(s.should_pane_back_out(crossterm::event::KeyCode::Esc)); + } + #[test] fn body_clicks_are_swallowed_while_a_manager_is_open() { use crossterm::event::{MouseButton, MouseEvent, MouseEventKind}; diff --git a/crates/rocm-dash-tui/src/ui/onboarding.rs b/crates/rocm-dash-tui/src/ui/onboarding.rs index 98b665964..ad134fa41 100644 --- a/crates/rocm-dash-tui/src/ui/onboarding.rs +++ b/crates/rocm-dash-tui/src/ui/onboarding.rs @@ -210,6 +210,14 @@ pub struct PendingOnboard { } /// Overlay state. `None` on `AppState` means the wizard is closed. +/// +/// Any new nested sub-view field (like `browser` or `install_config`) added +/// here must also be listed in `active_overlay_at_root`'s onboarding clause +/// in `app/mod.rs` — that hand-maintained enumeration is what tells the +/// shared Esc back-out path not to eject the whole wizard while a sub-view +/// has focus. The same applies to every other manager-state struct that +/// clause enumerates (`serve_wizard`, `install_manager`, `runtime_manager`, +/// `engine_manager`, `services`, `update_manager`, `config_manager`, ...). #[derive(Debug, Clone, Default)] pub struct OnboardingState { pub step: OnboardingStep, From 6307730d49184996fb58a5ff265b9b1a4e522ecd Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 11:20:26 +0000 Subject: [PATCH 18/36] docs(agents): note review-dismissal and gh rerun-job gotchas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review_dismissed event withdraws CHANGES_REQUESTED but doesn't itself grant approval, and gh run rerun --job fails until the whole parent run completes — both surfaced while driving PR #358 to green and are worth capturing so future upstream work doesn't re-diagnose them from scratch. Signed-off-by: Jussi Elo --- AGENTS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index fa3475b37..3a5651af1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,6 +112,8 @@ Before each stateful decision or public status update: Do not rely on stale memory, partial CI views, or prior snapshots. Subagent reports are hypotheses until directly re-verified. When re-verifying, match the verification scope to the claim: if subagent claimed "tests pass", re-run the same test suite; if it claimed "no conflicts", do the rebase locally; if it claimed "leak-free", re-run the scan. +A dismissed `CHANGES_REQUESTED` review (`review_dismissed` event) is not an approval — the reviewer withdrew their objection, but `reviewDecision` can still read `REVIEW_REQUIRED` afterward. Re-check `reviewDecision` directly rather than treating a dismissal as clearing the merge gate. + After rebase/cherry-pick/merge, grep for conflict markers: ```bash @@ -244,6 +246,7 @@ Watch checks to completion and drive to all-green. - fix real regressions from your change - handle infrastructure flakes by rerun or maintainer escalation with evidence - ensure flakes are not hiding real code failures in other checks +- `gh run rerun --job ` is rejected until the *entire* parent run reaches `completed`, even if the target job already failed; if sibling jobs are still `queued`/`in_progress`, wait for the whole run to finish (or use `gh run rerun --failed ` once it has) instead of retrying the per-job command A red check means "not ready" until resolved. From cd25801dd6ffdace3a869ceb174242ddf20bbaf4 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 13:00:29 +0000 Subject: [PATCH 19/36] fix(dash-tui,e2e): route wheel scroll to help modals, fix dash-15 timing Mouse wheel scroll only routed to ScrollModal for Modal::Detail, even though Modal::Help and Modal::GlobalHelp render a scrollbar via draw_scrollable_lines and already support keyboard scrolling identically to Detail. Extend handle_mouse to cover both. Nothing before dash-15's theme-picker step proves the event loop is reading input yet, and `t` toggles Modal::ThemePicker open/closed so a lost startup keystroke can't be safely retried via send_until. Wait for the Home tab's readiness marker first, same rationale already used by open_instance_detail for the non-retryable Enter key. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 12 +++++++++++- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 9 +++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 5d4a7818d..94b63c567 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -3559,7 +3559,7 @@ pub fn handle_mouse(ev: MouseEvent, modal: &Modal, tab: ActiveTab) -> KeyAction MouseEventKind::ScrollUp => -1, _ => return KeyAction::Nothing, }; - if *modal == Modal::Detail { + if matches!(*modal, Modal::Detail | Modal::Help | Modal::GlobalHelp) { KeyAction::ScrollModal(delta) } else if *modal == Modal::ThemePicker || (*modal == Modal::None && tab == ActiveTab::Observe) { KeyAction::Move(delta as isize) @@ -4648,6 +4648,16 @@ mod tests { handle_mouse(scroll_down, &Modal::Detail, ActiveTab::Observe), KeyAction::ScrollModal(1) ); + // Help / GlobalHelp modals render a scrollbar and support keyboard + // scrolling identically to Detail — the wheel must reach them too. + assert_eq!( + handle_mouse(scroll_down, &Modal::Help, ActiveTab::Home), + KeyAction::ScrollModal(1) + ); + assert_eq!( + handle_mouse(scroll_down, &Modal::GlobalHelp, ActiveTab::Home), + KeyAction::ScrollModal(1) + ); // ThemePicker → Move (drives picker cursor) assert_eq!( handle_mouse(scroll_down, &Modal::ThemePicker, ActiveTab::Home), diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 735ebc61e..185dfc858 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -227,6 +227,15 @@ async fn open_command_palette(world: &mut E2eWorld) { #[when("the user opens the theme picker")] async fn open_theme_picker(world: &mut E2eWorld) { let tui = session(world); + // `t` toggles `Modal::ThemePicker` open/closed, so it is NOT safe to resend + // via `send_until` (its own doc comment restricts that to idempotent + // keys) — a resend after the picker is already open would immediately + // close it. Nothing before this step proves the event loop is reading + // input yet, so wait for the Home tab's readiness marker before the + // (non-retryable) `t`, same rationale as `open_instance_detail`. + tui.wait_for_screen("Updates", default_timeout()) + .await + .unwrap_or_else(|e| panic!("dashboard home view did not become ready: {e}")); tui.send("t") .unwrap_or_else(|e| panic!("failed to open the theme picker: {e}")); tui.wait_for_screen( From 2612f47bbde4b0e9bb3faa87e1fa12a78e6538d5 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 14 Sep 2026 14:04:13 +0000 Subject: [PATCH 20/36] fix(dash-tui,e2e): address pr-review-watcher non-blocking findings - dash_steps.rs: reword the shared backdrop-dimmed panic message from "the instance detail popup" to "the popup" since dash-15 reuses the same step for the theme picker - tui_driver.rs: document that corner_backdrop_is_dimmed's hardcoded (0,0) cell and wash RGB silently stop discriminating if popup geometry or grey_overlay's color ever change - job_console.rs: add a direct regression test for console_esc_closes so the on_console_key/footer-chip pairing it guards can't drift without a dedicated test noticing Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/job_console.rs | 20 ++++++++++++++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 2 +- tests/e2e-cucumber/tests/e2e/tui_driver.rs | 7 +++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/ui/job_console.rs b/crates/rocm-dash-tui/src/ui/job_console.rs index c871cf0e8..bc367ccf0 100644 --- a/crates/rocm-dash-tui/src/ui/job_console.rs +++ b/crates/rocm-dash-tui/src/ui/job_console.rs @@ -313,6 +313,26 @@ mod tests { )); } + #[test] + fn console_esc_closes_tracks_job_terminality() { + // Direct coverage for the seam itself: `on_console_key` and the + // footer's Esc-chip label both call through `console_esc_closes`, so + // a regression here would silently desync the two without this test. + let mut s = State::default(); + s.apply(StateEvent::StartJob { + id: "j".into(), + cmd: "x".into(), + args: vec![], + }); + assert!(console_esc_closes(s.job("j"))); + s.apply(StateEvent::JobDone { + id: "j".into(), + code: 0, + }); + assert!(!console_esc_closes(s.job("j"))); + assert!(!console_esc_closes(None)); + } + #[test] fn status_labels_track_lifecycle() { let mut s = State::default(); diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 185dfc858..22ed38c31 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -688,7 +688,7 @@ async fn backdrop_is_dimmed(world: &mut E2eWorld) { let tui = session(world); assert!( tui.corner_backdrop_is_dimmed(), - "the screen behind the instance detail popup was not dimmed:\n{}", + "the screen behind the popup was not dimmed:\n{}", tui.screen_text() ); } diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 321968d66..f121bf2f3 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -278,6 +278,13 @@ impl TuiSession { /// reliable proxy for "the screen behind the popup was dimmed" without /// importing the product crate's own color constant. Unlike /// `screen_text`, this deliberately inspects style, not just text content. + /// + /// This check is only as good as its two hardcoded assumptions: the + /// `(0, 0)` corner and the literal wash RGB. If a future popup's geometry + /// ever grows to cover the corner, or `grey_overlay`'s color constant + /// changes, this silently stops discriminating (always false) instead of + /// failing loudly — keep both in sync with `grey_overlay` and + /// `centered_rect` if either changes. pub fn corner_backdrop_is_dimmed(&self) -> bool { const WASH: vt100::Color = vt100::Color::Rgb(0x1c, 0x1e, 0x22); let p = self From 3750cd01ccb3024a51cab9b793e28c2058303ef2 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 15 Sep 2026 05:25:48 +0000 Subject: [PATCH 21/36] fix(dash-tui,e2e): dim periphery in draw_focused, add regression tests draw_focused() drew the manager overlay without first washing the backdrop via modal::grey_overlay, unlike draw(), leaving the area around the manager card at plain theme background on the bare-launcher path (--setup/--serve/--examine). Add the missing grey_overlay call and correct the doc comment's overstated "identical layering" claim. Also add regression coverage for ScrollModal dispatch and for max_scroll after the scrollbar-width fix, and tighten an e2e assertion to the actual last REPLAY help line. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 28 ++++++++++++++++++ crates/rocm-dash-tui/src/ui/mod.rs | 10 +++++-- crates/rocm-dash-tui/src/ui/modal.rs | 34 ++++++++++++++++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 12 ++++---- 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 94b63c567..c66c074ec 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -5602,6 +5602,34 @@ mod tests { assert_eq!(s.help_scroll, 0); } + #[test] + fn scroll_modal_action_reaches_scroll_help_only_for_help_modals() { + // Regression: earlier coverage only exercised `scroll_help` directly, + // never through `apply_action`, so a broken/removed `ScrollModal` + // dispatch arm (e.g. a wrong `matches!` guard) would pass every + // existing test while the real `{`/`}` keys silently did nothing. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.modal = Modal::Help; + s.help_max_scroll = 10; + apply_action(&mut s, KeyAction::ScrollModal(3)); + assert_eq!(s.help_scroll, 3, "Help modal scrolls via apply_action"); + + let mut s = AppState::new("t".into(), "default-dark".into()); + s.modal = Modal::GlobalHelp; + s.help_max_scroll = 10; + apply_action(&mut s, KeyAction::ScrollModal(3)); + assert_eq!( + s.help_scroll, 3, + "GlobalHelp modal scrolls via apply_action" + ); + + let mut s = AppState::new("t".into(), "default-dark".into()); + s.modal = Modal::None; + s.help_max_scroll = 10; + apply_action(&mut s, KeyAction::ScrollModal(3)); + assert_eq!(s.help_scroll, 0, "no modal open: ScrollModal is a no-op"); + } + #[test] fn slash_clear_empties_transcript() { let mut s = st(); diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 23067219d..4258aaef5 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -173,8 +173,9 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { /// A single hint line sits below it — no header, tab shell, dock, or footer /// legend. Used by the bare-`rocm` launcher's in-place flows (Set up / Serve / /// Diagnose), where the full dashboard chrome would be misleading. The overlay -/// is drawn through the same [`draw_active_manager`] path the dashboard uses (so -/// the approval / job-console layering is identical). Falls back to a centered +/// is drawn through the same [`draw_active_manager`] path the dashboard uses, +/// with the same dimmed-backdrop wash behind it, so the approval / job-console +/// layering is identical to [`draw`]. Falls back to a centered /// "closing…" note when no overlay is open — defensive; the event loop breaks at /// that point and hands control back to the launcher. pub fn draw_focused(f: &mut Frame, state: &mut AppState) { @@ -192,6 +193,11 @@ pub fn draw_focused(f: &mut Frame, state: &mut AppState) { let footer_area = outer[1]; if state.has_open_overlay() { + // Dim the periphery behind the modal, matching `draw()`'s treatment so + // the overlay reads as the foreground here too (previously this path + // skipped the wash entirely, leaving the area outside the manager card + // at plain theme background instead of dimmed). + modal::grey_overlay(f); let manager_rect = modal::centered_rect(82, 80, 130, 34, body); draw_active_manager(f, manager_rect, state, &theme); } else { diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index 692d913bf..e5f9c04b1 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -971,4 +971,38 @@ mod ported_chrome_tests { "overflowing help should render a scrollbar thumb: {out:?}" ); } + + #[test] + fn max_scroll_uses_post_scrollbar_width_not_pre_scrollbar_width() { + // Regression: `max_scroll` must be computed from `content_area.width` + // (one column narrower than `inner.width` once the scrollbar reserves + // its column), not `inner.width`. A line exactly as wide as + // `inner.width` fits on one row there, but wraps onto a second row + // once the scrollbar narrows the content area by one column — and + // that extra row must count toward `max_scroll`, or "scroll to end" + // permanently strands it just past the last reachable offset. + use ratatui::text::Line; + let theme = Theme::from_name("default-dark"); + // area 80x10 -> inner 78x8 (1-cell border each side); once a + // scrollbar is drawn, content_area narrows to 77x8. + let area = Rect::new(0, 0, 80, 10); + let backend = TestBackend::new(80, 10); + let mut term = Terminal::new(backend).unwrap(); + let mut lines: Vec = (0..10).map(|_| Line::from("x")).collect(); + // Exactly `inner.width` (78) chars: one row at width 78, two rows at + // the post-scrollbar width of 77. + lines.push(Line::from("a".repeat(78))); + let mut max_scroll = 0; + term.draw(|f| { + max_scroll = super::draw_scrollable_lines(f, area, "Test", lines, 0, &theme); + }) + .unwrap(); + assert_eq!( + max_scroll, 4, + "max_scroll must reflect wrapping at the post-scrollbar width \ + (77 -> 12 wrapped rows -> max_scroll 4), not the pre-scrollbar \ + inner width (78 -> 11 wrapped rows -> max_scroll 3), or the \ + wrapped second row of the long line becomes unreachable" + ); + } } diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 22ed38c31..1348ee985 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -633,15 +633,17 @@ async fn navigation_guidance_displayed(world: &mut E2eWorld) { #[then("replay controls guidance is displayed")] async fn replay_controls_guidance_displayed(world: &mut E2eWorld) { let tui = session(world); - // REPLAY is the last group in the flattened, scrollable help body — it - // sits past what an 80x24 terminal shows without scrolling, so this - // proves the scroll wiring actually reaches previously-clipped content. - tui.wait_for_screen("pause / resume", default_timeout()) + // REPLAY is the last group in the flattened, scrollable help body, but + // most of it (including "pause / resume") already fits on an 80x24 + // screen at scroll=0. Only "jump ±60s" — the group's last line — sits + // past the fold, so it's the one line that actually proves the scroll + // wiring reaches previously-clipped content. + tui.wait_for_screen("jump ±60s", default_timeout()) .await .unwrap_or_else(|e| panic!("replay controls guidance did not appear after scrolling: {e}")); let screen = tui.screen_text(); assert!( - screen.contains("REPLAY") && screen.contains("pause / resume"), + screen.contains("REPLAY") && screen.contains("jump ±60s"), "replay controls guidance missing after scrolling to end of help:\n{screen}" ); } From 41aa05ddacacc6e207986dca21a39646daa432bb Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 15 Sep 2026 11:25:35 +0000 Subject: [PATCH 22/36] fix(dash-tui,e2e): confirm menu close before quit; test draw_focused dimming dash-14 sent a bare, unconfirmed Escape to close Modal::Menu and then immediately quit. Modal::Menu has no `q` arm, so an Escape that hasn't landed yet leaves the menu open and silently swallows the quit keystroke, hanging until the 30s timeout (an intermittent CI flake). Add a `the dashboard menu is closed` step, mirroring the existing `services_manager_closed` pattern, to wait for the menu to actually close before quitting. Also close the test-coverage gap on draw_focused's periphery dimming: the existing test only asserted rendered text, so it would not have caught a regression if the `grey_overlay` call were removed. Assert the corner cell carries the wash background too. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 11 +++++++++++ tests/e2e-cucumber/features/dash.feature | 3 ++- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 87c8f16e3..af1481a5a 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -5531,6 +5531,17 @@ mod tests { out.contains("Esc"), "focused hint carries an Esc affordance" ); + // Periphery must carry the same grey_overlay wash `draw()` uses behind + // every dashboard modal — text-only assertions above would still pass + // if the `grey_overlay` call in `draw_focused` were dropped, since the + // corner is plain theme bg either way in terms of glyphs (it's blank). + let wash = ratatui::style::Color::Rgb(0x1c, 0x1e, 0x22); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(wash), + "corner cell must carry grey_overlay's wash bg, not plain theme bg" + ); } #[test] diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index 3d28d2b32..f8758410d 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -166,7 +166,8 @@ Feature: Interactive dashboard When the user presses Escape Then the dashboard menu is displayed When the user presses Escape - And the user quits the dashboard + Then the dashboard menu is closed + When the user quits the dashboard Then the dashboard exits successfully @id:dash-theme-picker-dims-backdrop @requires-os:linux diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 41aff5900..6a1470229 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -769,6 +769,20 @@ async fn dashboard_menu_is_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("dashboard menu did not appear: {e}")); } +#[then("the dashboard menu is closed")] +async fn dashboard_menu_is_closed(world: &mut E2eWorld) { + // A bare Escape send is not guaranteed to have been acted on yet by the + // time the next step runs — confirm `Modal::Menu` actually closed before + // quitting, the same way `services_manager_closed` does. Without this, + // an unlanded close leaves the menu open and swallows the subsequent + // quit keystroke (`Modal::Menu` has no `q` arm), hanging until the + // quit step's timeout. + session(world) + .wait_until_gone("Options", default_timeout()) + .await + .unwrap_or_else(|e| panic!("the dashboard menu is still open after Escape: {e}")); +} + #[then("Serving actions are displayed")] async fn serving_actions_displayed(world: &mut E2eWorld) { session(world) From e40da4114a29f9dcf83ee64a4c37772b4c48bd98 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 15 Sep 2026 13:36:42 +0000 Subject: [PATCH 23/36] fix(dash-tui): resolve remaining review findings from PR #358 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the rest of rominf's review round (pullrequestreview-5208593830): - app/mod.rs: give Modal::Menu/Palette/Options their own `q => Quit` arm, matching every other modal. This also fixes a regression where a terminal that decodes "Alt+q" as a bare Esc followed by a plain `q` (rather than one Alt-modified event) silently swallowed the quit on the Chat tab: Esc now opens the menu there like everywhere else, and the menu's new `q` arm lets the second event still quit instead of being eaten by `_ => KeyAction::Nothing`. Two regression tests cover both the direct arm and the two-event Chat sequence. - ui/mod.rs, instances.rs: cover `grey_overlay`'s dimming wash at every `draw`/`draw_focused` call site that renders it, including behind the manager overlay and the approval modal. - app/mod.rs, bench.rs, hardware.rs: drop the dead `ScrollModal` arms left over for `Modal::Detail`, which no longer renders through that path, and the now-unreachable `draw_detail` helpers in the per-tab modules. - modal.rs: fix two contradictory Esc lines in the Help overlay. - job_console.rs: line up the finished-job console hint ("cancel") with the footer chip's own label for the same state, instead of saying "dismiss" in one place and "cancel" in the other. - tui_driver.rs: upgrade `corner_backdrop_is_dimmed`'s self-documenting comment into a hard invariant call-out — the wash RGB must never equal a theme's plain background, or an undimmed screen would misreport as dimmed. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 121 +++++++- crates/rocm-dash-tui/src/ui/job_console.rs | 2 +- crates/rocm-dash-tui/src/ui/mod.rs | 241 ++++++++++++++-- crates/rocm-dash-tui/src/ui/modal.rs | 7 +- crates/rocm-dash-tui/src/ui/tabs/bench.rs | 254 +--------------- crates/rocm-dash-tui/src/ui/tabs/hardware.rs | 273 +----------------- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 150 ++++++++-- tests/e2e-cucumber/tests/e2e/tui_driver.rs | 81 +++++- 8 files changed, 551 insertions(+), 578 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index af1481a5a..71335ca1a 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -544,6 +544,13 @@ pub struct AppState { /// so a "jump to end" (`i16::MAX`) can't leave the offset far past the /// real content length. pub help_max_scroll: u16, + /// Scroll offset (in lines) inside the instance Detail modal's body + /// (launch args / env vars panes). Reset when the modal opens. + pub instance_detail_scroll: u16, + /// Last-measured upper bound for `instance_detail_scroll`, written back + /// by the renderer each frame (see `ui::tabs::instances::draw_detail`), + /// mirroring `help_max_scroll`. + pub instance_detail_max_scroll: u16, /// Vertical scroll offset (first visible line) of the active job console. /// Shared by whichever operational manager is showing its console; reset /// when an overlay opens (`close_overlays`). @@ -752,6 +759,8 @@ impl AppState { bench_detail_scroll: 0, help_scroll: 0, help_max_scroll: 0, + instance_detail_scroll: 0, + instance_detail_max_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, @@ -1144,6 +1153,7 @@ impl AppState { /// Open the theme picker modal, positioning the cursor on the active theme. pub fn open_theme_picker(&mut self) { + self.close_overlays(); let names = crate::ui::theme::theme_names(); self.theme_picker_sel = names .iter() @@ -1206,6 +1216,27 @@ impl AppState { self.help_scroll = next; } + /// Reset the instance Detail modal's scroll offset (called when opening + /// the modal, so a stale offset never carries over from a previous + /// instance's selection). + pub const fn reset_instance_detail_scroll(&mut self) { + self.instance_detail_scroll = 0; + self.instance_detail_max_scroll = 0; + } + + /// Adjust the instance Detail modal's scroll. `delta` is in lines; + /// clamped against `[0, instance_detail_max_scroll]` (the latter is last + /// written back by the renderer, see `instance_detail_max_scroll`), so + /// `i16::MIN`/`i16::MAX` ("jump to start/end") land exactly on + /// `0`/`instance_detail_max_scroll` instead of overflowing into an offset + /// far past the real content length. + pub fn scroll_instance_detail(&mut self, delta: i16) { + let cur = i32::from(self.instance_detail_scroll); + let max = i32::from(self.instance_detail_max_scroll); + let next = u16::try_from((cur + i32::from(delta)).clamp(0, max)).unwrap_or(u16::MAX); + self.instance_detail_scroll = next; + } + /// Install the resolved chat endpoint and set the initial consent state. /// `None` → `Unavailable`; `Some` → `Accepted` when pre-consented (e.g. /// `--chat-yes`), otherwise `Pending` (the one-time in-TUI prompt). @@ -2757,12 +2788,14 @@ fn apply_action(state: &mut AppState, action: KeyAction) -> bool { } if state.selection_len() > 0 { state.modal = Modal::Detail; + state.reset_instance_detail_scroll(); } } KeyAction::ToggleHelp => { state.modal = if state.modal == Modal::Help { Modal::None } else { + state.close_overlays(); state.reset_help_scroll(); Modal::Help }; @@ -2876,12 +2909,12 @@ fn apply_action(state: &mut AppState, action: KeyAction) -> bool { } _ => {} }, - // ponytail: P3 folds Bench into Observe; the per-tab Bench detail modal - // is no longer reachable, so Detail itself has nothing to scroll. Help - // and GlobalHelp are the only modals that currently use this action. KeyAction::ScrollModal(delta) if matches!(state.modal, Modal::Help | Modal::GlobalHelp) => { state.scroll_help(delta); } + KeyAction::ScrollModal(delta) if state.modal == Modal::Detail => { + state.scroll_instance_detail(delta); + } KeyAction::ScrollModal(_) => {} KeyAction::ScrollConsole(dv, dh) => state.scroll_console(dv, dh), KeyAction::ScrollDock(dv) => state.scroll_dock(dv), @@ -3570,6 +3603,7 @@ fn handle_key(k: KeyEvent, current: ActiveTab, modal: &Modal, chat: ChatKeyCtx) // Esc main menu: ↑↓ cycle Options/Help/Quit, Enter activates, Esc closes. if *modal == Modal::Menu { return match k.code { + KeyCode::Char('q') => KeyAction::Quit, KeyCode::Esc => KeyAction::CloseModal, KeyCode::Char('j') | KeyCode::Down => KeyAction::MenuMove(1), KeyCode::Char('k') | KeyCode::Up => KeyAction::MenuMove(-1), @@ -3580,6 +3614,7 @@ fn handle_key(k: KeyEvent, current: ActiveTab, modal: &Modal, chat: ChatKeyCtx) // Command palette: ↑↓ choose destination, Enter goes, Esc closes. if *modal == Modal::Palette { return match k.code { + KeyCode::Char('q') => KeyAction::Quit, KeyCode::Esc => KeyAction::CloseModal, KeyCode::Char('j') | KeyCode::Down => KeyAction::MenuMove(1), KeyCode::Char('k') | KeyCode::Up => KeyAction::MenuMove(-1), @@ -3590,6 +3625,7 @@ fn handle_key(k: KeyEvent, current: ActiveTab, modal: &Modal, chat: ChatKeyCtx) // Options panel: ←→ switch settings tab, Esc closes. if *modal == Modal::Options { return match k.code { + KeyCode::Char('q') => KeyAction::Quit, KeyCode::Esc => KeyAction::CloseModal, KeyCode::Char('h') | KeyCode::Left | KeyCode::BackTab => KeyAction::OptionsTab(-1), KeyCode::Char('l') | KeyCode::Right | KeyCode::Tab => KeyAction::OptionsTab(1), @@ -3769,6 +3805,48 @@ mod tests { assert_eq!(hk(KeyCode::Esc, ActiveTab::Observe), KeyAction::OpenMenu); } + #[test] + fn q_quits_menu_palette_and_options_too() { + // Menu/Palette/Options used to have no `q` arm at all, silently + // swallowing the key instead of quitting like every other modal. + let with_modal = |modal: &Modal| { + handle_key( + press(KeyCode::Char('q')), + ActiveTab::Home, + modal, + ChatKeyCtx::default(), + ) + }; + assert_eq!(with_modal(&Modal::Menu), KeyAction::Quit); + assert_eq!(with_modal(&Modal::Palette), KeyAction::Quit); + assert_eq!(with_modal(&Modal::Options), KeyAction::Quit); + } + + #[test] + fn chat_esc_then_q_still_quits_via_the_menu() { + // A terminal that decodes "Alt+q" as a bare Esc followed by a plain + // `q` (rather than a single Alt-modified KeyEvent) used to quit + // immediately on Chat, because Esc was a no-op there and `q` fell + // through to the global `Quit` arm. This PR makes Esc open the main + // menu on Chat too, so the second event now needs Menu's own `q` + // arm (added above) to still reach `Quit` instead of being + // swallowed by the menu. + let ctx = ChatKeyCtx { + consent: ChatConsent::Accepted, + focused: false, + ..Default::default() + }; + let after_esc = handle_key(press(KeyCode::Esc), ActiveTab::Chat, &Modal::None, ctx); + assert_eq!(after_esc, KeyAction::OpenMenu); + let after_q = handle_key( + press(KeyCode::Char('q')), + ActiveTab::Chat, + &Modal::Menu, + ctx, + ); + assert_eq!(after_q, KeyAction::Quit); + } + #[test] fn tab_cycles_forward_and_wraps() { // 5-tab IA: Home → ROCm → Serving → Observe → Chat → Home. @@ -5542,6 +5620,25 @@ mod tests { Some(wash), "corner cell must carry grey_overlay's wash bg, not plain theme bg" ); + // The "Esc back to menu" hint is rendered with a foreground-only + // style (no explicit bg), and `ratatui::Style::patch` leaves an + // unset field alone rather than clearing it — so the hint inherits + // grey_overlay's wash bg from the cells underneath it, exactly like + // `draw()`'s footer. Assert on the cell directly (not just its + // text), so this fails if the hint's style ever gains an explicit + // `bg` that would revert it to plain theme background. + let hint_row_y = term.backend().buffer().area().height - 1; + let hint_cell = term.backend().buffer().cell((0, hint_row_y)).unwrap(); + assert_eq!( + hint_cell.symbol(), + "E", + "hint row should start with the Esc affordance" + ); + assert_eq!( + hint_cell.style().bg, + Some(wash), + "the Esc hint inherits grey_overlay's wash bg, same as draw()'s footer" + ); } #[test] @@ -5798,6 +5895,24 @@ mod tests { assert_eq!(s.help_scroll, 0, "no modal open: ScrollModal is a no-op"); } + #[test] + fn scroll_modal_action_reaches_scroll_instance_detail_for_detail_modal() { + // Regression: `apply_action`'s ScrollModal dispatch only matched + // `Modal::Help | Modal::GlobalHelp`, silently dropping the action for + // `Modal::Detail` even though both `handle_key` and `handle_mouse` + // emit `ScrollModal` for it (see `detail_modal_j_k_emit_scroll` / + // `handle_mouse_routes_scroll_by_modal_and_tab`) and the instance + // Detail modal's body (launch_args/env_vars) can genuinely overflow. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.modal = Modal::Detail; + s.instance_detail_max_scroll = 10; + apply_action(&mut s, KeyAction::ScrollModal(3)); + assert_eq!( + s.instance_detail_scroll, 3, + "Detail modal scrolls via apply_action" + ); + } + #[test] fn slash_clear_empties_transcript() { let mut s = st(); diff --git a/crates/rocm-dash-tui/src/ui/job_console.rs b/crates/rocm-dash-tui/src/ui/job_console.rs index bc367ccf0..4879b60c1 100644 --- a/crates/rocm-dash-tui/src/ui/job_console.rs +++ b/crates/rocm-dash-tui/src/ui/job_console.rs @@ -235,7 +235,7 @@ pub fn draw_job_console( let hints = if matches!(job.status, JobStatus::Running) { "Esc close (keeps running) · Ctrl+C cancel · wheel / PgUp·PgDn scroll" } else { - "Enter/Esc dismiss · wheel / PgUp·PgDn scroll" + "Enter/Esc cancel · wheel / PgUp·PgDn scroll" }; f.render_widget( Paragraph::new(Line::from(Span::styled( diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 4258aaef5..26cb2bcdb 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -42,7 +42,9 @@ use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Clear, Paragraph}; -use crate::app::{ActiveTab, AppState, ChatConsent, ConnState, FooterChip, KeyAction, Modal}; +use crate::app::{ + ActiveTab, AppState, ChatConsent, ConnState, FooterChip, KeyAction, Modal, PaneFocus, +}; use crate::ui::theme::Theme; pub fn draw(f: &mut Frame, state: &mut AppState) { @@ -131,7 +133,8 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { // detail (the selectable list on that surface). Modal::Detail => { if state.active_tab == ActiveTab::Observe { - tabs::instances::draw_detail(f, body, state, &theme); + let max_scroll = tabs::instances::draw_detail(f, body, state, &theme); + state.instance_detail_max_scroll = max_scroll; } } Modal::ThemePicker => { @@ -173,11 +176,17 @@ pub fn draw(f: &mut Frame, state: &mut AppState) { /// A single hint line sits below it — no header, tab shell, dock, or footer /// legend. Used by the bare-`rocm` launcher's in-place flows (Set up / Serve / /// Diagnose), where the full dashboard chrome would be misleading. The overlay -/// is drawn through the same [`draw_active_manager`] path the dashboard uses, -/// with the same dimmed-backdrop wash behind it, so the approval / job-console -/// layering is identical to [`draw`]. Falls back to a centered -/// "closing…" note when no overlay is open — defensive; the event loop breaks at -/// that point and hands control back to the launcher. +/// (and any job console nested inside it) is drawn through the same +/// [`draw_active_manager`] path [`draw`] uses, and the same dimmed-backdrop +/// wash is applied behind it. It is NOT identical to [`draw`] in two ways: +/// there is no approval layer here (a focused-host session has no chat, so no +/// tool call can ever be pending), and the "Esc back to menu" hint below is +/// rendered with a foreground-only `Style` (no `bg`) — `ratatui::Style::patch` +/// leaves an unset field untouched rather than clearing it, so the hint +/// inherits the wash's background from the cells underneath it rather than +/// reverting to plain theme bg, exactly like `draw`'s footer. Falls back to a +/// centered "closing…" note when no overlay is open — defensive; the event +/// loop breaks at that point and hands control back to the launcher. pub fn draw_focused(f: &mut Frame, state: &mut AppState) { let theme = state.theme; state.scrollbars.borrow_mut().clear(); @@ -479,8 +488,18 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve segs.push(Seg::Key("Esc", Some(KeyAction::ChatBlur))); segs.push(Seg::Sep(" unfocus ")); } else { - segs.push(Seg::Key("Esc", Some(KeyAction::OpenMenu))); - segs.push(Seg::Sep(" menu ")); + // Dispatch `PaneEscape`, not a hardcoded `OpenMenu` — `apply_action` + // resolves `PaneEscape` against `pane_focus` exactly as a real + // keypress does (Details → Actions on Rocm/Serving, else the menu), + // so the chip can't promise "menu" when the key would actually just + // step the pane back out. + let steps_out_of_detail = is_action_tab && state.pane_focus == PaneFocus::Detail; + segs.push(Seg::Key("Esc", Some(KeyAction::PaneEscape))); + segs.push(Seg::Sep(if steps_out_of_detail { + " back " + } else { + " menu " + })); if matches!( state.active_tab, ActiveTab::Observe | ActiveTab::Rocm | ActiveTab::Serving @@ -525,8 +544,20 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve segs.push(Seg::Sep(" theme ")); segs.push(Seg::Key("?", Some(KeyAction::ToggleHelp))); segs.push(Seg::Sep(" help ")); - segs.push(Seg::Key("q", Some(KeyAction::Quit))); - segs.push(Seg::Sep(" quit")); + if state.has_open_overlay() { + // While a manager overlay is open it owns every key (the event loop + // routes each keypress to its `on_key`, never falling through to + // `apply_action`), so a real `q` press can't reach `KeyAction::Quit` + // there — it cancels the approval, closes the job console, or backs + // the manager out instead, but it never tears down the app or kills + // a job the way `Quit` does. `None` keeps the chip non-clickable so a + // click can't do something the key never would. + segs.push(Seg::Key("q", None)); + segs.push(Seg::Sep(" close")); + } else { + segs.push(Seg::Key("q", Some(KeyAction::Quit))); + segs.push(Seg::Sep(" quit")); + } // Lay out left-to-right, rendering each segment in its own cell span so the // recorded chip geometry matches the painted columns exactly. @@ -625,10 +656,53 @@ mod tests { term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) .unwrap(); + // The chip dispatches `PaneEscape`, matching what a real Esc keypress + // resolves to via `handle_key`'s catch-all — not a hardcoded + // `OpenMenu` that would diverge from `apply_action`'s `pane_focus` + // handling on Rocm/Serving. let _ = chips .iter() - .find(|c| c.action == KeyAction::OpenMenu) - .expect("a fallback Esc chip opening the menu must always be present"); + .find(|c| c.action == KeyAction::PaneEscape) + .expect("a fallback Esc chip stepping the pane back out must always be present"); + } + + #[test] + fn footer_esc_chip_dispatches_pane_escape_when_detail_focused() { + // Regression: on Rocm/Serving with `pane_focus == Detail`, the real + // Esc key steps Details → Actions first (`PaneEscape` in + // `apply_action`); it does not open the menu. The chip must dispatch + // the same `PaneEscape` action (not `OpenMenu`) so a click matches + // the keypress, and its label must say so. + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.active_tab = ActiveTab::Rocm; + state.pane_focus = PaneFocus::Detail; + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + let mut chips = Vec::new(); + term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) + .unwrap(); + + assert!( + chips.iter().any(|c| c.action == KeyAction::PaneEscape), + "Esc chip must dispatch PaneEscape, matching the real key, while Detail is focused" + ); + assert!( + !chips.iter().any(|c| c.action == KeyAction::OpenMenu), + "no chip may claim OpenMenu while Esc would actually step Detail back to Actions" + ); + let row: String = (0..90) + .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) + .collect(); + assert!( + row.contains("back"), + "chip label should say the Esc key steps back out of Detail: {row:?}" + ); } #[test] @@ -674,11 +748,16 @@ mod tests { .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) .collect(); assert!( - row.contains("cancel"), + row.contains("Esc cancel"), "sub-popup Esc chip should say cancel: {row:?}" ); + // Note: "close" legitimately appears elsewhere in this row (the `q` + // chip always says "close" while any overlay is open, root or not — + // see `footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open`), + // so the Esc chip's own label must be checked specifically rather + // than scanning the whole row for the substring. assert!( - !row.contains("close"), + !row.contains("Esc close"), "sub-popup Esc chip should not say close: {row:?}" ); } @@ -772,12 +851,140 @@ mod tests { .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) .collect(); assert!( - row.contains("cancel"), + row.contains("Esc cancel"), "finished-job console Esc chip should say cancel: {row:?}" ); + // Note: "close" legitimately appears elsewhere in this row (the `q` + // chip always says "close" while any overlay is open — see + // `footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open`), + // so the Esc chip's own label must be checked specifically rather + // than scanning the whole row for the substring. assert!( - !row.contains("close"), + !row.contains("Esc close"), "finished-job console Esc chip should not say close: {row:?}" ); } + + #[test] + fn footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open() { + // Regression: while any manager overlay is open it owns every key + // (the event loop routes each keypress to the manager's own `on_key`, + // never falling through to `apply_action`), so a real `q` press can + // never reach `KeyAction::Quit` there — it only cancels/closes the + // overlay. A click on the footer chip must not diverge from that and + // tear down the app (killing a still-running job via `kill_on_drop`) + // when the key itself never would. + use crate::ui::services_manager::ServicesManagerState; + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.services = Some(ServicesManagerState::default()); + assert!(state.has_open_overlay()); + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + let mut chips = Vec::new(); + term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) + .unwrap(); + + for chip in &chips { + assert_ne!( + chip.action, + KeyAction::Quit, + "no chip may dispatch Quit while a manager overlay owns `q`" + ); + } + let row: String = (0..90) + .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) + .collect(); + assert!( + row.contains("close"), + "q chip should say close while an overlay is open: {row:?}" + ); + assert!( + !row.contains("quit"), + "q chip should not say quit while an overlay is open: {row:?}" + ); + } + + /// The wash `grey_overlay` paints behind an open overlay (see + /// `modal::grey_overlay`'s own `grey_overlay_dims_every_cell` test for the + /// exact color); these tests only check that `draw`/`draw_focused` actually + /// invoke it at their three call sites, not the wash's own correctness. + const OVERLAY_WASH: ratatui::style::Color = ratatui::style::Color::Rgb(0x1c, 0x1e, 0x22); + + #[test] + fn draw_dims_periphery_with_grey_overlay_behind_manager_overlay() { + // Covers `draw`'s manager-overlay `grey_overlay(f)` call (the + // `has_open_overlay()` branch, not the approval one). + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let mut state = AppState::new("t".into(), "default-dark".into()); + state.services = Some(crate::ui::services_manager::ServicesManagerState::default()); + assert!(state.has_open_overlay()); + + let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); + term.draw(|f| draw(f, &mut state)).unwrap(); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(OVERLAY_WASH), + "corner cell must carry grey_overlay's wash bg while a manager overlay is open" + ); + } + + #[test] + fn draw_dims_periphery_with_grey_overlay_behind_approval_modal() { + // Covers `draw`'s approval-modal `grey_overlay(f)` call, distinct from + // the manager-overlay one above (`state.approval` is `Some` with no + // manager open). + use crate::app::PendingApproval; + use crate::ui::approval::{ApprovalChoice, ApprovalRequest}; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let mut state = AppState::new("t".into(), "default-dark".into()); + state.approval = Some(PendingApproval { + req: ApprovalRequest::new("run it", vec!["echo hi".into()]), + choice: ApprovalChoice::default(), + name: "tool".into(), + arguments: serde_json::Value::Null, + }); + assert!(!state.has_open_overlay()); + + let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); + term.draw(|f| draw(f, &mut state)).unwrap(); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(OVERLAY_WASH), + "corner cell must carry grey_overlay's wash bg while the approval modal is open" + ); + } + + #[test] + fn draw_focused_dims_periphery_with_grey_overlay_behind_manager_overlay() { + // Covers `draw_focused`'s own `grey_overlay(f)` call — a separate + // renderer from `draw`, previously skipping the wash entirely for the + // bare-launcher focused-host path. + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let mut state = AppState::new("t".into(), "default-dark".into()); + state.services = Some(crate::ui::services_manager::ServicesManagerState::default()); + assert!(state.has_open_overlay()); + + let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); + term.draw(|f| draw_focused(f, &mut state)).unwrap(); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(OVERLAY_WASH), + "corner cell must carry grey_overlay's wash bg in the focused-host renderer too" + ); + } } diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index e5f9c04b1..e369b7c8f 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -126,7 +126,10 @@ pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scrol ("Tab / Shift-Tab", "next / previous tab"), ("1 .. 5", "jump to tab"), ("t", "open theme picker"), - ("Esc", "open the main menu"), + ( + "Esc", + "back out one step (see the active tab's own Esc below)", + ), ]; let replay: &[(&str, &str)] = &[ ("Space", "pause / resume"), @@ -140,7 +143,7 @@ pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme, scrol ("j / k ↑ / ↓", "select an action"), ("→ / Enter", "open it in Details (asks before mutating)"), ("←", "Details preview → Actions list"), - ("Esc", "close an open manager (back to Actions)"), + ("Esc", "in Details, back out to Actions first"), ], ActiveTab::Observe => &[ ("j / Down", "select next instance"), diff --git a/crates/rocm-dash-tui/src/ui/tabs/bench.rs b/crates/rocm-dash-tui/src/ui/tabs/bench.rs index bb39a9727..ddc3b305c 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/bench.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/bench.rs @@ -8,7 +8,7 @@ use ratatui::Frame; use ratatui::layout::{Constraint, Direction, Layout, Rect}; use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Paragraph, Wrap}; +use ratatui::widgets::{Block, Borders, Paragraph}; use crate::ui::panel::{self, BoxRole}; @@ -17,7 +17,6 @@ use rocm_dash_core::bench_schema::{BenchmarkRow, PassFail}; use crate::app::{AppState, KeyAction}; use crate::ui::format; -use crate::ui::modal::{centered_rect, draw_popup_frame}; use crate::ui::sparkline::BrailleSparkline; use crate::ui::theme::Theme; @@ -418,257 +417,6 @@ pub fn hit_test(area: Rect, x: u16, y: u16, state: &AppState) -> Option= 2 { - let body = Rect::new(inner.x, inner.y, inner.width, inner.height - 1); - let footer = Rect::new(inner.x, inner.y + inner.height - 1, inner.width, 1); - (body, Some(footer)) - } else { - (inner, None) - }; - - let body = crate::ui::panel::vertical_scrollbar( - f, - body_area, - lines.len(), - body_area.height as usize, - scroll as usize, - theme, - ); - state.record_scrollbar( - body_area, - body, - false, - lines.len(), - body_area.height as usize, - crate::app::ScrollTarget::BenchDetail, - ); - let p = Paragraph::new(lines) - .wrap(Wrap { trim: false }) - .scroll((scroll, 0)); - f.render_widget(p, body); - - if let Some(footer) = footer_area { - let hint = Paragraph::new(Line::from(Span::styled( - "j/k or ↑/↓ scroll · PgUp/PgDn jump · Esc close", - Style::default().fg(theme.muted), - ))); - f.render_widget(hint, footer); - } -} - -// ---------- detail body ---------- - -#[allow(clippy::ref_option)] -fn fmt_opt(v: &Option) -> String { - match v { - Some(x) => x.to_string(), - None => "-".to_string(), - } -} - -/// SI-formatted optional `u32` counter (`-` when None). -fn fmt_opt_u32_si(v: Option) -> String { - match v { - Some(x) => format::si(f64::from(x)), - None => "-".to_string(), - } -} - -/// SI-formatted optional `u64` counter (`-` when None). -fn fmt_opt_u64_si(v: Option) -> String { - match v { - Some(x) => format::si(x as f64), - None => "-".to_string(), - } -} - -fn fmt_opt_f32_4(v: Option) -> String { - match v { - Some(x) => format!("{x:.4}"), - None => "-".to_string(), - } -} - -const fn fmt_opt_bool(v: Option) -> &'static str { - match v { - Some(true) => "true", - Some(false) => "false", - None => "-", - } -} - -fn verdict_span(v: PassFail, theme: &Theme) -> Span<'static> { - let (label, color) = match v { - PassFail::Pass => ("Pass", theme.ok), - PassFail::Fail => ("Fail", theme.err), - PassFail::Unknown => ("Unknown", theme.muted), - }; - Span::styled( - label, - Style::default().fg(color).add_modifier(Modifier::BOLD), - ) -} - -fn section_header(title: &str, theme: &Theme) -> Line<'static> { - Line::from(Span::styled( - format!("— {title} —"), - Style::default() - .fg(theme.muted) - .add_modifier(Modifier::BOLD), - )) -} - -fn kv_line(key: &str, value: String, theme: &Theme) -> Line<'static> { - Line::from(vec![ - Span::styled(format!(" {key:<22} "), Style::default().fg(theme.accent)), - Span::styled(value, Style::default().fg(theme.fg)), - ]) -} - -fn kv_span_line(key: &str, value: Span<'static>, theme: &Theme) -> Line<'static> { - Line::from(vec![ - Span::styled(format!(" {key:<22} "), Style::default().fg(theme.accent)), - value, - ]) -} - -fn build_detail_lines(row: &BenchmarkRow, theme: &Theme) -> Vec> { - let mut lines: Vec = Vec::with_capacity(48); - lines.push(section_header("identity", theme)); - lines.push(kv_line("cell", row.cell.clone(), theme)); - lines.push(kv_line("run", row.run.to_string(), theme)); - lines.push(kv_line("model", fmt_opt(&row.model), theme)); - lines.push(kv_line("endpoint", fmt_opt(&row.endpoint), theme)); - lines.push(kv_line("judge_model", fmt_opt(&row.judge_model), theme)); - lines.push(Line::raw("")); - - // config - lines.push(section_header("config", theme)); - lines.push(kv_line("tp", fmt_opt(&row.tp), theme)); - lines.push(kv_line("pp", fmt_opt(&row.pp), theme)); - lines.push(kv_line("dtype", fmt_opt(&row.dtype), theme)); - lines.push(kv_line( - "attention_backend", - fmt_opt(&row.attention_backend), - theme, - )); - lines.push(kv_line("max_num_seqs", fmt_opt(&row.max_num_seqs), theme)); - lines.push(kv_line("concurrency", fmt_opt(&row.concurrency), theme)); - lines.push(kv_line("extra_args", fmt_opt(&row.extra_args), theme)); - lines.push(Line::raw("")); - - // performance - lines.push(section_header("performance", theme)); - lines.push(kv_line( - "wall_s", - row.wall_s.map_or_else(|| "-".into(), format::duration), - theme, - )); - lines.push(kv_line("n_requests", fmt_opt_u32_si(row.n_requests), theme)); - lines.push(kv_line( - "prompt_tokens", - fmt_opt_u64_si(row.prompt_tokens), - theme, - )); - lines.push(kv_line( - "prompt_tps", - format::tps_opt(row.prompt_tps), - theme, - )); - lines.push(kv_line( - "completion_tokens", - fmt_opt_u64_si(row.completion_tokens), - theme, - )); - lines.push(kv_line("gen_tps", format::tps_opt(row.gen_tps), theme)); - lines.push(kv_line( - "max_running_reqs", - fmt_opt_u32_si(row.max_running_reqs), - theme, - )); - lines.push(kv_line( - "max_waiting_reqs", - fmt_opt_u32_si(row.max_waiting_reqs), - theme, - )); - lines.push(kv_line("ttft_ms", fmt_opt(&row.ttft_ms), theme)); - lines.push(kv_line("tpot_ms", fmt_opt(&row.tpot_ms), theme)); - lines.push(kv_line("out_chars", fmt_opt_u64_si(row.out_chars), theme)); - lines.push(Line::raw("")); - - // verdict - lines.push(section_header("verdict", theme)); - lines.push(kv_line("rc", fmt_opt(&row.rc), theme)); - lines.push(kv_span_line( - "pass_fail", - verdict_span(row.pass_fail, theme), - theme, - )); - lines.push(kv_span_line( - "judge_pass_fail", - verdict_span(row.judge_pass_fail, theme), - theme, - )); - lines.push(kv_line( - "assertion_pass", - fmt_opt_bool(row.assertion_pass).to_string(), - theme, - )); - lines.push(kv_line( - "assertion_fail_count", - fmt_opt(&row.assertion_fail_count), - theme, - )); - lines.push(kv_line( - "assertion_summary", - fmt_opt(&row.assertion_summary), - theme, - )); - lines.push(kv_line( - "quality_score", - fmt_opt_f32_4(row.quality_score), - theme, - )); - lines.push(kv_line( - "safety_pass", - fmt_opt_bool(row.safety_pass).to_string(), - theme, - )); - lines.push(kv_line( - "safety_violations", - fmt_opt(&row.safety_violations), - theme, - )); - - lines -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/rocm-dash-tui/src/ui/tabs/hardware.rs b/crates/rocm-dash-tui/src/ui/tabs/hardware.rs index 19f81b46b..3fb0eac5a 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/hardware.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/hardware.rs @@ -27,8 +27,7 @@ use crate::ui::panel::{self, BoxRole}; use crate::ui::sparkline::BrailleSparkline; use crate::ui::theme::Theme; use crate::ui::widgets::{ - POWER_CRIT_W, gpu_stats_line, instances_on_gpu, node_efficiency, power_style, - temperature_style, trunc, + gpu_stats_line, instances_on_gpu, node_efficiency, power_style, temperature_style, trunc, }; /// Rows consumed above the GPU section in [`draw`] (CPU 10 + mem/swap 3 + I/O 3). @@ -614,197 +613,6 @@ fn gpu_info_line<'a>( )]) } -// ---- detail modal ----------------------------------------------------------- - -/// Heatmap redline for temperature (°C): a full bar means junction-redline-hot. -const HEATMAP_TEMP_MAX_C: f64 = 100.0; - -/// Largest of a fixed `floor` (the semantic redline) and the observed maximum -/// in `data`. Keeps a heatmap row normalized to a meaningful limit while still -/// growing if telemetry exceeds that limit. -fn semantic_max(floor: f64, data: &[f64]) -> f64 { - data.iter().copied().fold(floor, f64::max) -} - -/// The detail-modal "now" line, with temperature and power threshold-colored -/// via [`temperature_style`] / [`power_style`]. Pure. -fn detail_now_line(g: &GpuMetrics, theme: &Theme) -> Line<'static> { - let clk = match g.clock_mhz { - Some(v) => format::mhz(v.round() as u64), - None => "-".into(), - }; - Line::from(vec![ - Span::styled(format!("{:<12} ", "now"), Style::default().fg(theme.muted)), - Span::styled( - format!("util {} · ", format::pct(g.gpu_utilization_pct)), - Style::default().fg(theme.fg), - ), - Span::styled( - format::celsius(g.temperature_c), - temperature_style(g.temperature_c, theme), - ), - Span::styled(" · ".to_string(), Style::default().fg(theme.fg)), - Span::styled(format::watts(g.power_w), power_style(g.power_w, theme)), - Span::styled(format!(" · clk {clk}"), Style::default().fg(theme.fg)), - ]) -} - -/// Full-screen detail for the currently-selected GPU. -/// -/// Pulls per-tick samples out of `state.history` to build a metric × time heatmap (util, temp, -/// power, vram%) alongside a summary header and a footer hint. -pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { - use crate::ui::heatmap::{Heatmap, HeatmapRow}; - use crate::ui::modal::{centered_rect, draw_popup_frame}; - - let popup = centered_rect(85, 85, 140, 32, area); - let Some(snap) = state.latest.as_ref() else { - let inner = draw_popup_frame(f, popup, "GPU detail", theme); - f.render_widget( - Paragraph::new(Line::from(Span::styled( - "no snapshot yet", - Style::default().fg(theme.muted), - ))), - inner, - ); - return; - }; - if snap.gpus.is_empty() { - let inner = draw_popup_frame(f, popup, "GPU detail", theme); - f.render_widget( - Paragraph::new(Line::from(Span::styled( - "no GPUs reported", - Style::default().fg(theme.muted), - ))), - inner, - ); - return; - } - let i = state.gpu_sel.min(snap.gpus.len() - 1); - let g = &snap.gpus[i]; - - let title = format!(" GPU {} · detail ", g.device_id); - let inner = draw_popup_frame(f, popup, &title, theme); - if inner.height == 0 { - return; - } - - // Vertical layout: 4 summary lines + heatmap (Min) + 1 footer hint. - let rows = Layout::default() - .direction(Direction::Vertical) - .constraints([ - Constraint::Length(1), - Constraint::Length(1), - Constraint::Length(1), - Constraint::Length(1), - Constraint::Length(1), // gap - Constraint::Min(4), - Constraint::Length(1), - ]) - .split(inner); - - let sysinfo = snap.gpu_system_info.as_ref(); - let model = sysinfo.map_or("?", |si| si.gpu_model.as_str()); - let rocm = sysinfo - .and_then(|si| si.rocm_version.as_deref()) - .unwrap_or("?"); - let driver = sysinfo - .and_then(|si| si.driver_version.as_deref()) - .unwrap_or("?"); - let partitions = match sysinfo { - Some(si) => format!( - "{:?} / {:?}", - si.compute_partition_mode, si.memory_partition_mode - ) - .to_uppercase(), - None => "? / ?".into(), - }; - // Summary lines. - let kv = |k: &'static str, v: String, tone: Style| -> Line<'static> { - Line::from(vec![ - Span::styled(format!("{k:<12} "), Style::default().fg(theme.muted)), - Span::styled(v, tone), - ]) - }; - f.render_widget( - Paragraph::new(vec![ - kv( - "device", - format!("{} · {model}", g.device_id), - Style::default() - .fg(theme.accent) - .add_modifier(Modifier::BOLD), - ), - kv( - "vram", - format::mib_pair(g.vram_used_mb, g.vram_total_mb), - Style::default().fg(theme.fg), - ), - detail_now_line(g, theme), - kv( - "platform", - format!("partition {partitions} · ROCm {rocm} · driver {driver}"), - Style::default().fg(theme.muted), - ), - ]), - Rect::new(inner.x, inner.y, inner.width, 4), - ); - - // Heatmap rows derived from state.history. - let history = &state.history; - let util: Vec = history - .iter() - .filter_map(|s| s.gpus.get(i).map(|gpu| f64::from(gpu.gpu_utilization_pct))) - .collect(); - let temp: Vec = history - .iter() - .filter_map(|s| s.gpus.get(i).map(|gpu| f64::from(gpu.temperature_c))) - .collect(); - let power: Vec = history - .iter() - .filter_map(|s| s.gpus.get(i).map(|gpu| f64::from(gpu.power_w))) - .collect(); - let vram: Vec = history - .iter() - .filter_map(|s| { - s.gpus.get(i).map(|gpu| { - if gpu.vram_total_mb > 0 { - 100.0 * gpu.vram_used_mb as f64 / gpu.vram_total_mb as f64 - } else { - 0.0 - } - }) - }) - .collect(); - - // Normalize temp/power to fixed semantic redlines so a full bar means - // "near the limit", not "near the largest value seen this session". The - // row still grows if telemetry ever exceeds the redline. - let max_temp = semantic_max(HEATMAP_TEMP_MAX_C, &temp); - let max_power = semantic_max(f64::from(POWER_CRIT_W), &power); - let rows_vec = vec![ - HeatmapRow::new("util %", util, 100.0).stops(theme.ok, theme.warn, theme.err), - HeatmapRow::new("temp °C", temp, max_temp).stops(theme.ok, theme.warn, theme.err), - HeatmapRow::new("power W", power, max_power).stops(theme.ok, theme.warn, theme.err), - HeatmapRow::new("vram %", vram, 100.0).stops(theme.ok, theme.warn, theme.err), - ]; - let heat = Heatmap::new(&rows_vec) - .stops(theme.ok, theme.warn, theme.err) - .track_bg(theme.surface_2) - .label_style(Style::default().fg(theme.muted)) - .label_width(10); - f.render_widget(heat, rows[5]); - - // Footer hint. - f.render_widget( - Paragraph::new(Line::from(Span::styled( - " each row = a metric over the last N ticks · newest on the right · Esc close", - Style::default().fg(theme.muted), - ))), - rows[6], - ); -} - #[cfg(test)] mod tests { use super::*; @@ -980,85 +788,6 @@ mod tests { assert!(out.contains("more"), "missing overflow affordance: {out:?}"); } - #[test] - #[allow(clippy::float_cmp)] - fn semantic_max_uses_floor_then_grows() { - // all temps below the redline → max is the semantic floor - assert_eq!(semantic_max(HEATMAP_TEMP_MAX_C, &[60.0, 78.0, 95.0]), 100.0); - // an observed value above the floor wins - assert_eq!(semantic_max(HEATMAP_TEMP_MAX_C, &[60.0, 110.0]), 110.0); - // power: below critical → floor (POWER_CRIT_W) - assert_eq!( - semantic_max(f64::from(POWER_CRIT_W), &[400.0, 690.0]), - 700.0 - ); - // power: above critical → observed - assert_eq!( - semantic_max(f64::from(POWER_CRIT_W), &[400.0, 760.0]), - 760.0 - ); - // empty data → floor - assert_eq!(semantic_max(f64::from(POWER_CRIT_W), &[]), 700.0); - } - - #[test] - fn detail_now_line_threshold_colors_power_and_temp() { - let theme = Theme::default_dark(); - let hot = mk_gpu("gpu-0", 99.0, 88.0, 740.0); - let line = detail_now_line(&hot, &theme); - let temp_span = line - .spans - .iter() - .find(|s| s.content.contains("°C")) - .unwrap(); - let pow_span = line - .spans - .iter() - .find(|s| s.content.contains(" W")) - .unwrap(); - assert_eq!( - temp_span.style.fg, - Some(theme.err), - "hot temp not err-colored" - ); - assert_eq!( - pow_span.style.fg, - Some(theme.err), - ">700W power not err-colored" - ); - // a cool, low-power GPU is ok-colored - let cool = mk_gpu("gpu-1", 10.0, 45.0, 300.0); - let line2 = detail_now_line(&cool, &theme); - let p2 = line2 - .spans - .iter() - .find(|s| s.content.contains(" W")) - .unwrap(); - assert_eq!(p2.style.fg, Some(theme.ok)); - } - - #[test] - fn draw_detail_renders_without_panic() { - let mut s = state_with_snapshot(snap_with_gpus(4)); - // seed a little history so the heatmap has data - for _ in 0..5 { - s.history.push_back(snap_with_gpus(4)); - } - s.modal = crate::app::Modal::Detail; - let backend = TestBackend::new(140, 32); - let mut term = Terminal::new(backend).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &s, &s.theme)) - .unwrap(); - let buf = term.backend().buffer().clone(); - let out: String = buf - .content() - .iter() - .map(ratatui::buffer::Cell::symbol) - .collect(); - assert!(out.contains("detail"), "missing detail title: {out:?}"); - assert!(out.contains("power W"), "missing power heatmap row"); - } - fn mk_instance( name: &str, gpu_ids: &[&str], diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 7bef472ef..7b48b2aa3 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -615,7 +615,10 @@ const fn point_in_rect(r: Rect, x: u16, y: u16) -> bool { x >= r.x && x < r.x + r.width && y >= r.y && y < r.y + r.height } -pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { +/// Draws the instance Detail modal and returns the max scroll offset for its +/// body (see `render_body`), so the caller can write it back to +/// `AppState::instance_detail_max_scroll`. +pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> u16 { grey_overlay(f); let popup = centered_rect(85, 85, 120, 36, area); @@ -626,7 +629,7 @@ pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { Style::default().fg(theme.muted), ))); f.render_widget(p, inner); - return; + return 0; } let instances = sorted_instances(&state.instances); @@ -638,13 +641,13 @@ pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { Style::default().fg(theme.muted), ))); f.render_widget(p, inner); - return; + return 0; }; let title = format!(" Instance · {} ", inst.container_name); let inner = draw_popup_frame(f, popup, &title, theme); if inner.height == 0 || inner.width == 0 { - return; + return 0; } // Vertical: summary (4 lines: status/id/port/tp · model/gpus/tpw/gen · partition/quant/vram · freshness) @@ -660,8 +663,9 @@ pub fn draw_detail(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { .split(inner); render_summary(f, chunks[0], inst, snap_ts, theme); - render_body(f, chunks[1], inst, theme); + let max_scroll = render_body(f, chunks[1], inst, theme, state.instance_detail_scroll); render_footer(f, chunks[2], inst, theme); + max_scroll } fn render_summary( @@ -759,7 +763,10 @@ fn render_summary( f.render_widget(p, area); } -fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme) { +/// Renders the launch_args/env_vars panes, applying `scroll` (in lines) to +/// both, and returns the larger of the two panes' max scroll offsets so the +/// caller can clamp future scroll input (see `AppState::scroll_instance_detail`). +fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scroll: u16) -> u16 { let chunks = Layout::default() .direction(Direction::Horizontal) .constraints([Constraint::Ratio(1, 2), Constraint::Ratio(1, 2)]) @@ -786,10 +793,11 @@ fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme) { .map(|a| Line::from(Span::styled(a.clone(), Style::default().fg(theme.fg)))) .collect() }; - f.render_widget( - Paragraph::new(args_lines).wrap(Wrap { trim: false }), - args_inner, - ); + let args_p = Paragraph::new(args_lines).wrap(Wrap { trim: false }); + let args_max = u16::try_from(args_p.line_count(args_inner.width)) + .unwrap_or(u16::MAX) + .saturating_sub(args_inner.height); + f.render_widget(args_p.scroll((scroll.min(args_max), 0)), args_inner); // env_vars (right). BTreeMap iterates sorted by key. let env_inner = panel::bento( @@ -818,10 +826,13 @@ fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme) { }) .collect() }; - f.render_widget( - Paragraph::new(env_lines).wrap(Wrap { trim: false }), - env_inner, - ); + let env_p = Paragraph::new(env_lines).wrap(Wrap { trim: false }); + let env_max = u16::try_from(env_p.line_count(env_inner.width)) + .unwrap_or(u16::MAX) + .saturating_sub(env_inner.height); + f.render_widget(env_p.scroll((scroll.min(env_max), 0)), env_inner); + + args_max.max(env_max) } fn render_footer(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme) { @@ -981,6 +992,8 @@ mod tests { bench_detail_scroll: 0, help_scroll: 0, help_max_scroll: 0, + instance_detail_scroll: 0, + instance_detail_max_scroll: 0, console_scroll: 0, console_hscroll: 0, tick_count: 0, @@ -1245,8 +1258,10 @@ mod tests { // Detail modal: shows the quantization value and the VRAM pair. let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &state, &state.theme)) - .unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); let detail = buffer_text(&term); assert!( detail.contains("fp8"), @@ -1258,6 +1273,85 @@ mod tests { ); } + #[test] + fn draw_detail_dims_periphery_with_grey_overlay() { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + // Empty instance map hits `draw_detail`'s early-return branch, right + // after its `grey_overlay(f)` call — the shortest path that still + // exercises it. Text-only assertions on the popup body would still + // pass if that call were silently dropped, since the corner is blank + // either way; assert on the corner cell's background directly so + // this fails if `grey_overlay(f)` is ever removed. + let state = mk_state(HashMap::new(), 0); + let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); + let wash = ratatui::style::Color::Rgb(0x1c, 0x1e, 0x22); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(wash), + "corner cell must carry grey_overlay's wash bg, not plain theme bg" + ); + } + + #[test] + fn detail_modal_body_scrolls_launch_args_and_reports_nonzero_max_scroll() { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + // Enough launch_args to overflow the body pane at a realistic + // terminal height, so `render_body` has real content to scroll. + let mut inst = mk_inst("overflow"); + inst.launch_args = (0..60).map(|i| format!("--flag-{i}=value{i}")).collect(); + let mut m = HashMap::new(); + m.insert(inst.container_id.clone(), inst); + let mut state = mk_state(m, 0); + + // Render once at scroll=0 and capture the max_scroll draw_detail + // reports back — it must be non-zero given how much content + // overflows the pane. + let mut max_scroll = 0u16; + let mut term = Terminal::new(TestBackend::new(160, 30)).unwrap(); + term.draw(|f| { + max_scroll = draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); + assert!( + max_scroll > 0, + "60 launch_args must overflow the body pane, giving a nonzero max_scroll; got {max_scroll}" + ); + + let text_top = buffer_text(&term); + assert!( + text_top.contains("--flag-0=value0"), + "unscrolled body must show the first launch_args line; got:\n{text_top}" + ); + + // Scroll to the end and confirm the visible text actually shifts: + // the first line scrolls out of view while the last scrolls in. + state.instance_detail_scroll = max_scroll; + let mut term2 = Terminal::new(TestBackend::new(160, 30)).unwrap(); + term2 + .draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); + let text_scrolled = buffer_text(&term2); + assert!( + !text_scrolled.contains("--flag-0=value0"), + "fully scrolled body must no longer show the first launch_args line; got:\n{text_scrolled}" + ); + assert!( + text_scrolled.contains("--flag-59=value59"), + "fully scrolled body must show the last launch_args line; got:\n{text_scrolled}" + ); + } + #[test] fn nonfinite_ttft_tpot_render_dash_never_nan() { use ratatui::Terminal; @@ -1684,8 +1778,10 @@ mod tests { ); let state = state_with_snap(inst); let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &state, &state.theme)) - .unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); let out = buffer_text(&term); assert!( out.contains("held"), @@ -1706,8 +1802,10 @@ mod tests { inst.tokens_per_watt = Some(1.5); let state = state_with_snap(inst); let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &state, &state.theme)) - .unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); let out = buffer_text(&term); assert!( out.contains("1.50 tok/W*"), @@ -1727,8 +1825,10 @@ mod tests { ); let state = state_with_snap(inst); let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &state, &state.theme)) - .unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); let out = buffer_text(&term); assert!( out.contains("fresh"), @@ -1744,8 +1844,10 @@ mod tests { let inst = mk_inst_obs("legacy-detail", Some(100.0), None); let state = state_with_snap(inst); let mut term = Terminal::new(TestBackend::new(160, 48)).unwrap(); - term.draw(|f| draw_detail(f, f.area(), &state, &state.theme)) - .unwrap(); + term.draw(|f| { + draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); let out = buffer_text(&term); assert!( out.contains("unknown"), diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 3b6259330..556808f45 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -285,13 +285,26 @@ impl TuiSession { /// changes, this silently stops discriminating (always false) instead of /// failing loudly — keep both in sync with `grey_overlay` and /// `centered_rect` if either changes. + /// + /// HARD INVARIANT this relies on: `WASH` must never equal any theme's + /// plain `bg` (see the `bg:` fields in + /// `crates/rocm-dash-tui/src/ui/theme.rs`) — if it ever did, an + /// undimmed screen would misreport as dimmed and this assertion would + /// pass for the wrong reason. This is intentionally *not* enforced here + /// with a second hardcoded RGB (that would just trade one magic-number + /// coupling for two); if you touch either `WASH` or a theme's `bg`, + /// diff them against each other by hand. pub fn corner_backdrop_is_dimmed(&self) -> bool { const WASH: vt100::Color = vt100::Color::Rgb(0x1c, 0x1e, 0x22); let p = self .parser .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - p.screen().cell(0, 0).is_some_and(|c| c.bgcolor() == WASH) + let cell = p + .screen() + .cell(0, 0) + .expect("screen (0, 0) must exist once a screen has been rendered"); + cell.bgcolor() == WASH } fn screen_snapshot(&self) -> (String, (u16, u16)) { @@ -536,13 +549,69 @@ impl TuiSession { /// Send the quit gesture appropriate to the session and wait for a clean /// exit. The dashboard quits with `q`; chat quits with the `/quit` slash /// command (a bare `q` would be typed into the focused input instead). + /// + /// A bare send has no read-back, so if the quit gesture is written before + /// the app has finished acting on whatever came just before it (e.g. right + /// after an unsynchronized `send` like the Escape key), it can be consumed + /// by that transient state and never reach the quit handler — the process + /// then never exits and this hangs until `timeout`. Re-sending the gesture + /// on a short cadence closes that gap the same way [`send_until`] does for + /// screen markers: if the first attempt landed (the common case), the + /// process has already exited by the first check and nothing is resent. pub async fn quit_and_wait(&mut self, timeout: Duration) -> Result<(), String> { - if self.is_chat { - self.send("/quit\r")?; - } else { - self.send("q")?; + let gesture = if self.is_chat { "/quit\r" } else { "q" }; + let deadline = Instant::now() + timeout; + loop { + self.send(gesture)?; + let remaining = deadline.saturating_duration_since(Instant::now()); + let attempt = KEY_RESEND_INTERVAL.min(remaining); + match self.wait_for_exit_code_within(attempt).await { + Some(code) => { + return if code == 0 { + Ok(()) + } else { + Err(format!( + "TUI exited unsuccessfully (code {code}).\n{}", + self.framed_screen() + )) + }; + } + None => { + if let Some(panic_message) = self.take_reader_panic() { + return Err(format!( + "pty reader thread panicked while waiting to quit: {panic_message}\n{}", + self.framed_screen() + )); + } + } + } + if Instant::now() >= deadline { + return Err(format!( + "timed out after {timeout:?} waiting for the TUI to exit after repeating {gesture:?}.\n{}", + self.framed_screen() + )); + } + } + } + + /// Poll for up to `budget` for the child to exit, returning its exit code + /// if it did within that window or `None` (not a timeout error) if it + /// didn't — used by [`quit_and_wait`](Self::quit_and_wait) to bound each + /// resend attempt without treating "still running" as a hard failure. + async fn wait_for_exit_code_within(&mut self, budget: Duration) -> Option { + let deadline = Instant::now() + budget; + loop { + if let Ok(Some(status)) = self.child.try_wait() { + let code = i32::try_from(status.exit_code()).unwrap_or(-1); + self.finished = true; + self.record_once(code); + return Some(code); + } + if Instant::now() >= deadline { + return None; + } + tokio::time::sleep(POLL_INTERVAL).await; } - self.wait_for_exit(timeout).await } /// Poll until the child exits, asserting a successful (zero) exit code. From 9fba0907852a99323f8af3b3bd8fdba15c4228d8 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 06:36:32 +0000 Subject: [PATCH 24/36] fix(dash-tui): remove orphaned bench_detail_scroll plumbing bench::draw_detail was removed but its state plumbing was left behind: the bench_detail_scroll field, ScrollTarget::BenchDetail variant, its apply_scroll/scroll_position match arms, reset_bench_detail_scroll and scroll_bench_detail methods, and the now-vacuous unit test pinning them. Nothing constructs ScrollTarget::BenchDetail anymore, but every item was pub so dead_code/clippy stayed silent. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 30 ------------------- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 1 - 2 files changed, 31 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 71335ca1a..5618866cf 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -533,8 +533,6 @@ pub struct AppState { pub theme_name: String, pub theme: Theme, pub theme_picker_sel: usize, - /// Scroll offset (in lines) inside the Bench Detail modal. Reset on Open. - pub bench_detail_scroll: u16, /// Scroll offset (in lines) inside the Help / GlobalHelp overlays. Both /// modals are mutually exclusive so one field suffices; reset on open. pub help_scroll: u16, @@ -756,7 +754,6 @@ impl AppState { theme_name, theme, theme_picker_sel, - bench_detail_scroll: 0, help_scroll: 0, help_max_scroll: 0, instance_detail_scroll: 0, @@ -984,7 +981,6 @@ impl AppState { ScrollTarget::Console => self.console_scroll = p, ScrollTarget::ConsoleH => self.console_hscroll = p, ScrollTarget::Chat => self.set_chat_scroll(position), - ScrollTarget::BenchDetail => self.bench_detail_scroll = p, ScrollTarget::DockLogs => self.dock_logs_scroll = p, } } @@ -1184,19 +1180,6 @@ impl AppState { } } - /// Reset the bench-detail scroll offset (called when opening the modal). - pub const fn reset_bench_detail_scroll(&mut self) { - self.bench_detail_scroll = 0; - } - - /// Adjust the bench-detail scroll. `delta` is in lines; clamped at 0 - /// (no upper bound — the renderer clamps against the actual line count). - pub fn scroll_bench_detail(&mut self, delta: i16) { - let cur = i32::from(self.bench_detail_scroll); - let next = u16::try_from((cur + i32::from(delta)).max(0)).unwrap_or(u16::MAX); - self.bench_detail_scroll = next; - } - /// Reset the Help / GlobalHelp scroll offset (called when opening either /// modal, so a stale offset never carries over from a previous session). pub const fn reset_help_scroll(&mut self) { @@ -2986,7 +2969,6 @@ fn target_position(state: &AppState, h: &ScrollbarHandle) -> usize { ScrollTarget::Console => usize::from(state.console_scroll), ScrollTarget::ConsoleH => usize::from(state.console_hscroll), ScrollTarget::Chat => usize::from(state.chat_scroll), - ScrollTarget::BenchDetail => usize::from(state.bench_detail_scroll), ScrollTarget::DockLogs => h .max_position() .saturating_sub(usize::from(state.dock_logs_scroll)), @@ -3188,8 +3170,6 @@ pub enum ScrollTarget { ConsoleH, /// Wide-layout LOGS dock (`dock_logs_scroll`, tail-anchored / inverted). DockLogs, - /// Bench row detail modal (`bench_detail_scroll`). - BenchDetail, /// Chat transcript (`chat_scroll`). Chat, } @@ -4905,16 +4885,6 @@ mod tests { ); } - #[test] - fn scroll_bench_detail_clamps_at_zero() { - let mut s = AppState::new("t".into(), "default-dark".into()); - s.bench_detail_scroll = 5; - s.scroll_bench_detail(-100); - assert_eq!(s.bench_detail_scroll, 0); - s.scroll_bench_detail(7); - assert_eq!(s.bench_detail_scroll, 7); - } - #[test] fn detail_modal_j_k_emit_scroll() { let with_detail = |c| { diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 7b48b2aa3..51130b507 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -989,7 +989,6 @@ mod tests { theme_name: "default-dark".into(), theme: Theme::default_dark(), theme_picker_sel: 0, - bench_detail_scroll: 0, help_scroll: 0, help_max_scroll: 0, instance_detail_scroll: 0, From fe3fe6616e0132285ebb4e0949b66aa8abff1b9a Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 06:36:38 +0000 Subject: [PATCH 25/36] fix(dash-tui): gate t/? footer chips while a manager overlay is open The q chip was correctly gated so a click can't dispatch Quit while a manager overlay owns the keyboard, but the t and ? chips were pushed unconditionally. A real keypress can never reach OpenThemePicker or ToggleHelp while an overlay is open (13 manager-guarded arms in the event loop precede the generic handle_key fallthrough), but a footer click bypasses that path via footer_chip_hit. Replicate the q chip's if/else idiom for t and ? so all three render as non-clickable Seg::Key (_, None) while state.has_open_overlay() is true, and extend the regression test to cover all three chips. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/mod.rs | 50 ++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 26cb2bcdb..bca1d9fcd 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -540,21 +540,27 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve segs.push(Seg::Key("+/-", Some(KeyAction::ReplaySpeedUp))); segs.push(Seg::Sep(" speed ")); } - segs.push(Seg::Key("t", Some(KeyAction::OpenThemePicker))); - segs.push(Seg::Sep(" theme ")); - segs.push(Seg::Key("?", Some(KeyAction::ToggleHelp))); - segs.push(Seg::Sep(" help ")); if state.has_open_overlay() { // While a manager overlay is open it owns every key (the event loop // routes each keypress to its `on_key`, never falling through to - // `apply_action`), so a real `q` press can't reach `KeyAction::Quit` - // there — it cancels the approval, closes the job console, or backs - // the manager out instead, but it never tears down the app or kills - // a job the way `Quit` does. `None` keeps the chip non-clickable so a - // click can't do something the key never would. + // `apply_action`), so a real `t`/`?`/`q` press can't reach + // `OpenThemePicker`/`ToggleHelp`/`KeyAction::Quit` there — it cancels + // the approval, closes the job console, or backs the manager out + // instead, but it never opens the theme picker, toggles help, or + // tears down the app the way those actions do. `None` keeps the + // chips non-clickable so a click can't do something the key never + // would. + segs.push(Seg::Key("t", None)); + segs.push(Seg::Sep(" theme ")); + segs.push(Seg::Key("?", None)); + segs.push(Seg::Sep(" help ")); segs.push(Seg::Key("q", None)); segs.push(Seg::Sep(" close")); } else { + segs.push(Seg::Key("t", Some(KeyAction::OpenThemePicker))); + segs.push(Seg::Sep(" theme ")); + segs.push(Seg::Key("?", Some(KeyAction::ToggleHelp))); + segs.push(Seg::Sep(" help ")); segs.push(Seg::Key("q", Some(KeyAction::Quit))); segs.push(Seg::Sep(" quit")); } @@ -753,7 +759,7 @@ mod tests { ); // Note: "close" legitimately appears elsewhere in this row (the `q` // chip always says "close" while any overlay is open, root or not — - // see `footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open`), + // see `footer_q_t_help_chips_are_not_clickable_when_a_manager_overlay_is_open`), // so the Esc chip's own label must be checked specifically rather // than scanning the whole row for the substring. assert!( @@ -856,7 +862,7 @@ mod tests { ); // Note: "close" legitimately appears elsewhere in this row (the `q` // chip always says "close" while any overlay is open — see - // `footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open`), + // `footer_q_t_help_chips_are_not_clickable_when_a_manager_overlay_is_open`), // so the Esc chip's own label must be checked specifically rather // than scanning the whole row for the substring. assert!( @@ -866,14 +872,16 @@ mod tests { } #[test] - fn footer_q_chip_is_not_clickable_quit_when_a_manager_overlay_is_open() { + fn footer_q_t_help_chips_are_not_clickable_when_a_manager_overlay_is_open() { // Regression: while any manager overlay is open it owns every key // (the event loop routes each keypress to the manager's own `on_key`, - // never falling through to `apply_action`), so a real `q` press can - // never reach `KeyAction::Quit` there — it only cancels/closes the - // overlay. A click on the footer chip must not diverge from that and + // never falling through to `apply_action`), so a real `q`/`t`/`?` + // press can never reach `KeyAction::Quit`/`OpenThemePicker`/ + // `ToggleHelp` there — it only cancels/closes the overlay. A click on + // any of these footer chips must not diverge from that: it must not // tear down the app (killing a still-running job via `kill_on_drop`) - // when the key itself never would. + // or swap the theme / pop the help overlay on top of the manager when + // the key itself never would. use crate::ui::services_manager::ServicesManagerState; use crate::ui::theme::Theme; use ratatui::Terminal; @@ -896,6 +904,16 @@ mod tests { KeyAction::Quit, "no chip may dispatch Quit while a manager overlay owns `q`" ); + assert_ne!( + chip.action, + KeyAction::OpenThemePicker, + "no chip may dispatch OpenThemePicker while a manager overlay owns `t`" + ); + assert_ne!( + chip.action, + KeyAction::ToggleHelp, + "no chip may dispatch ToggleHelp while a manager overlay owns `?`" + ); } let row: String = (0..90) .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) From 540a810f5cc26f38cbe34da8ed59f32c6588bd25 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 06:36:43 +0000 Subject: [PATCH 26/36] fix(dash-tui): correct finished-job console hint from cancel to dismiss on_console_key for a terminal (finished) job only dismisses the console -- there is nothing left to cancel. The running-job branch already says "cancel" correctly; only the finished-job hint text was mislabeled. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/job_console.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/ui/job_console.rs b/crates/rocm-dash-tui/src/ui/job_console.rs index 4879b60c1..bc367ccf0 100644 --- a/crates/rocm-dash-tui/src/ui/job_console.rs +++ b/crates/rocm-dash-tui/src/ui/job_console.rs @@ -235,7 +235,7 @@ pub fn draw_job_console( let hints = if matches!(job.status, JobStatus::Running) { "Esc close (keeps running) · Ctrl+C cancel · wheel / PgUp·PgDn scroll" } else { - "Enter/Esc cancel · wheel / PgUp·PgDn scroll" + "Enter/Esc dismiss · wheel / PgUp·PgDn scroll" }; f.render_widget( Paragraph::new(Line::from(Span::styled( From 2f5d3891e875210404a3963527e6b0cab790e664 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 06:36:45 +0000 Subject: [PATCH 27/36] test(e2e): correct misleading comment on dashboard-menu-closed marker The comment claimed "Options" only ever renders as one of the main menu's three items, but Modal::Options (the Settings panel) also headers with "Options", so the substring isn't unique app-wide. No scenario today opens Settings before this step, so the detection logic itself is left as-is; the comment is narrowed to document the caveat instead of overstating uniqueness. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 6a1470229..84f2283c1 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -761,8 +761,10 @@ async fn dashboard_destinations_displayed(world: &mut E2eWorld) { #[then("the dashboard menu is displayed")] async fn dashboard_menu_is_displayed(world: &mut E2eWorld) { - // "Options" only ever renders as one of the main menu's three items - // (Options/Help/Quit) — a stable, unique marker for `Modal::Menu`. + // "Options" is used here as a marker for `Modal::Menu`'s three items + // (Options/Help/Quit). Note: the Settings panel (`Modal::Options`) also + // headers with "Options", so this marker is not unique app-wide — it is + // safe only because no scenario today opens Settings before this step. session(world) .wait_for_screen("Options", default_timeout()) .await From 15942ba43f5efab80946c6df611cc54dc4d6ed5c Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 06:36:47 +0000 Subject: [PATCH 28/36] docs(agents): revert scope-creep additions from the dialog-unification PR The dismissed-review-isn't-approval note and the gh run rerun sequencing note are correct, useful notes but unrelated to unifying TUI dialog behavior. AGENTS.md itself asks for one logical change per PR; land these separately. Signed-off-by: Jussi Elo --- AGENTS.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d9d670be1..3a2af9a23 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,8 +112,6 @@ Before each stateful decision or public status update: Do not rely on stale memory, partial CI views, or prior snapshots. Subagent reports are hypotheses until directly re-verified. When re-verifying, match the verification scope to the claim: if subagent claimed "tests pass", re-run the same test suite; if it claimed "no conflicts", do the rebase locally; if it claimed "leak-free", re-run the scan. -A dismissed `CHANGES_REQUESTED` review (`review_dismissed` event) is not an approval — the reviewer withdrew their objection, but `reviewDecision` can still read `REVIEW_REQUIRED` afterward. Re-check `reviewDecision` directly rather than treating a dismissal as clearing the merge gate. - After rebase/cherry-pick/merge, grep for conflict markers: ```bash @@ -254,7 +252,6 @@ Watch checks to completion and drive to all-green. - fix real regressions from your change - handle infrastructure flakes by rerun or maintainer escalation with evidence - ensure flakes are not hiding real code failures in other checks -- `gh run rerun --job ` is rejected until the *entire* parent run reaches `completed`, even if the target job already failed; if sibling jobs are still `queued`/`in_progress`, wait for the whole run to finish (or use `gh run rerun --failed ` once it has) instead of retrying the per-job command A red check means "not ready" until resolved. From 4b7634e09f8e544d881b4f2d4792dc051eea9687 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 07:53:37 +0000 Subject: [PATCH 29/36] fix(e2e): stop asserting stale help-modal text and a removed scroll capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dash-05 (dash-help-guidance) was failing on CI for two reasons: - navigation_guidance_displayed still checked for the literal "HOME" chip text; the footer chip now reads "Home tab". - the scenario's second half scrolled the help modal and asserted on "REPLAY" guidance text. The Help modal was reworked to size itself to its content instead of a fixed 70%-of-body-height allocation, so it no longer scrolls at all, and "REPLAY" never appears in the rendered output — that step pair was testing a capability that no longer exists. Fold the one assertion that still matters (that "jump ±60s", previously the first line clipped by the old fixed-height modal, now renders without needing to scroll) into navigation_guidance_displayed, and drop the dead scroll step and its assertion from both the feature file and the step definitions. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 2 -- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 29 +++------------------- 2 files changed, 3 insertions(+), 28 deletions(-) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index 3fcd15136..f33c9d4bd 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -46,8 +46,6 @@ Feature: Interactive dashboard When the user opens the dashboard with demo data And the user opens dashboard help Then navigation and next-step guidance are displayed - When the user scrolls to the end of dashboard help - Then replay controls guidance is displayed When the user closes dashboard help And the user quits the dashboard Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 43725163b..7b76e0166 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -229,13 +229,6 @@ async fn close_dashboard_help(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("failed to close dashboard help: {e}")); } -#[when("the user scrolls to the end of dashboard help")] -async fn scroll_to_end_of_dashboard_help(world: &mut E2eWorld) { - session(world) - .send("G") - .unwrap_or_else(|e| panic!("failed to scroll dashboard help: {e}")); -} - #[when("the user opens the command palette")] async fn open_command_palette(world: &mut E2eWorld) { session(world) @@ -756,29 +749,13 @@ async fn navigation_guidance_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("dashboard help did not appear: {e}")); let screen = tui.screen_text(); assert!( - screen.contains("next / previous tab") && screen.contains("HOME"), + screen.contains("next / previous tab") + && screen.contains("Home tab") + && screen.contains("jump ±60s"), "navigation or contextual guidance missing:\n{screen}" ); } -#[then("replay controls guidance is displayed")] -async fn replay_controls_guidance_displayed(world: &mut E2eWorld) { - let tui = session(world); - // REPLAY is the last group in the flattened, scrollable help body, but - // most of it (including "pause / resume") already fits on an 80x24 - // screen at scroll=0. Only "jump ±60s" — the group's last line — sits - // past the fold, so it's the one line that actually proves the scroll - // wiring reaches previously-clipped content. - tui.wait_for_screen("jump ±60s", default_timeout()) - .await - .unwrap_or_else(|e| panic!("replay controls guidance did not appear after scrolling: {e}")); - let screen = tui.screen_text(); - assert!( - screen.contains("REPLAY") && screen.contains("jump ±60s"), - "replay controls guidance missing after scrolling to end of help:\n{screen}" - ); -} - #[then("the services manager is displayed")] async fn services_manager_displayed(world: &mut E2eWorld) { session(world) From 66fb405c303dcaa0879e0dc787231df37cf50481 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 07:53:46 +0000 Subject: [PATCH 30/36] fix(dash-tui): gate t/?/q footer chips while an approval is pending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit has_open_overlay() deliberately excludes state.approval — it's a separate gating layer with its own routing (a real keypress hits a dedicated state.approval.is_some() arm ahead of general key dispatch, routing to approval_key). The footer chip-gating added by this PR only checked has_open_overlay(), so while an approval was pending the t/?/q chips stayed clickable: a click could open the theme picker or help overlay on top of the approval prompt, or quit the app outright, none of which a real keypress could ever do while an approval owns input. Gate on has_open_overlay() || state.approval.is_some(), matching the existing manager-overlay chip behavior, and add a regression test mirroring the existing manager-overlay one but with a pending approval instead. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/mod.rs | 63 ++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 75dffb1a4..992428491 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -539,16 +539,20 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve segs.push(Seg::Key("+/-", Some(KeyAction::ReplaySpeedUp))); segs.push(Seg::Sep(" speed ")); } - if state.has_open_overlay() { + if state.has_open_overlay() || state.approval.is_some() { // While a manager overlay is open it owns every key (the event loop // routes each keypress to its `on_key`, never falling through to // `apply_action`), so a real `t`/`?`/`q` press can't reach // `OpenThemePicker`/`ToggleHelp`/`KeyAction::Quit` there — it cancels // the approval, closes the job console, or backs the manager out // instead, but it never opens the theme picker, toggles help, or - // tears down the app the way those actions do. `None` keeps the - // chips non-clickable so a click can't do something the key never - // would. + // tears down the app the way those actions do. The same is true while + // an approval is pending: `has_open_overlay()` deliberately excludes + // `approval` (it's a separate gating layer), so it has to be checked + // here too, or these chips would stay clickable and let a mouse click + // silently discard a pending approval that a real keypress never + // could. `None` keeps the chips non-clickable so a click can't do + // something the key never would. segs.push(Seg::Key("t", None)); segs.push(Seg::Sep(" theme ")); segs.push(Seg::Key("?", None)); @@ -927,6 +931,57 @@ mod tests { ); } + #[test] + fn footer_q_t_help_chips_are_not_clickable_when_an_approval_is_pending() { + // Regression: `has_open_overlay()` deliberately excludes `approval` + // (it's a separate gating layer with its own routing — see its doc + // comment), so a pending approval alone must still gate these chips. + // Keyboard input has a dedicated `state.approval.is_some()` arm ahead + // of general key dispatch that routes every key to `approval_key`; + // a footer-chip click bypasses that dispatch entirely and would + // otherwise silently discard the pending approval (or, for `q`, tear + // the app down without ever recording a verdict). + use crate::app::PendingApproval; + use crate::ui::approval::{ApprovalChoice, ApprovalRequest}; + use crate::ui::theme::Theme; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + let theme = Theme::from_name("default-dark"); + let mut state = AppState::new("t".into(), "default-dark".into()); + state.approval = Some(PendingApproval { + req: ApprovalRequest::new("run it", vec!["echo hi".into()]), + choice: ApprovalChoice::default(), + name: "tool".into(), + arguments: serde_json::Value::Null, + }); + assert!(!state.has_open_overlay()); + + let backend = TestBackend::new(90, 1); + let mut term = Terminal::new(backend).unwrap(); + let mut chips = Vec::new(); + term.draw(|f| chips = draw_footer(f, f.area(), &state, &theme)) + .unwrap(); + + for chip in &chips { + assert_ne!( + chip.action, + KeyAction::Quit, + "no chip may dispatch Quit while an approval is pending" + ); + assert_ne!( + chip.action, + KeyAction::OpenThemePicker, + "no chip may dispatch OpenThemePicker while an approval is pending" + ); + assert_ne!( + chip.action, + KeyAction::ToggleHelp, + "no chip may dispatch ToggleHelp while an approval is pending" + ); + } + } + /// The wash `grey_overlay` paints behind an open overlay (see /// `modal::grey_overlay`'s own `grey_overlay_dims_every_cell` test for the /// exact color); these tests only check that `draw`/`draw_focused` actually From 7708d1f8a7ee5ef00c5f4aa72774646f9016ebe9 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 12:40:07 +0000 Subject: [PATCH 31/36] fix(dash-tui): address review 5234545188 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ui/mod.rs: scope the job-console Esc-chip assertions to the Esc chip itself ("Esc close" / "Esc cancel") instead of an unscoped substring check that the "q" chip's own "close" label always satisfied, making the test unable to fail. - modal.rs: dim the backdrop in draw_help via grey_overlay, matching every other modal; add a regression test asserting the corner cell outside the content-sized popup carries the dim wash. - app/mod.rs: add a test covering scroll_instance_detail's jump-to- start/jump-to-end clamping, which had no direct coverage. - tabs/bench.rs: hit_test's already-selected-row branch returned KeyAction::OpenDetail for a bench detail view that no longer exists; hit_test has no callers, so return None instead. - tabs/instances.rs: the Instance Detail modal's launch_args/env_vars panes can scroll but gave no hint that they do; show "↑/↓ scroll" in the footer once render_body reports overflow. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 13 ++++++++++ crates/rocm-dash-tui/src/ui/mod.rs | 7 ++++-- crates/rocm-dash-tui/src/ui/modal.rs | 25 ++++++++++++++++++- crates/rocm-dash-tui/src/ui/tabs/bench.rs | 4 ++- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 17 ++++++++++--- 5 files changed, 58 insertions(+), 8 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index b6bda386b..178989b9d 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -5353,6 +5353,19 @@ mod tests { assert_eq!(s.dock_logs_scroll, 0, "back to the tail"); } + #[test] + fn scroll_instance_detail_clamps_to_measured_max() { + let mut s = AppState::new("t".into(), "default-dark".into()); + s.instance_detail_max_scroll = 9; + // i16::MAX is the jump-to-end gesture; it must land on the measured + // max, not overflow past it. + s.scroll_instance_detail(i16::MAX); + assert_eq!(s.instance_detail_scroll, 9, "jump-to-end clamps to max"); + // i16::MIN is jump-to-start; it must land on 0, not underflow. + s.scroll_instance_detail(i16::MIN); + assert_eq!(s.instance_detail_scroll, 0, "jump-to-start clamps to 0"); + } + #[test] fn wheel_over_form_screen_overlay_is_swallowed() { let mut s = AppState::new("t".into(), "default-dark".into()); diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 992428491..d76bf3570 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -807,12 +807,15 @@ mod tests { let row: String = (0..90) .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) .collect(); + // The `q` chip always says "close" while any overlay is open, root or + // not, so an unscoped substring check here would pass even if the Esc + // chip's own label were wrong — check the Esc chip specifically. assert!( - row.contains("close"), + row.contains("Esc close"), "job console Esc chip should say close: {row:?}" ); assert!( - !row.contains("cancel"), + !row.contains("Esc cancel"), "job console Esc chip should not say cancel: {row:?}" ); } diff --git a/crates/rocm-dash-tui/src/ui/modal.rs b/crates/rocm-dash-tui/src/ui/modal.rs index 40d0aa6db..2f7580883 100644 --- a/crates/rocm-dash-tui/src/ui/modal.rs +++ b/crates/rocm-dash-tui/src/ui/modal.rs @@ -130,6 +130,7 @@ pub fn draw_popup_frame(f: &mut Frame, area: Rect, title: &str, theme: &Theme) - /// guess, which is the whole of what is claimed here: an honest indicator, not a /// guarantee that everything is visible. pub fn draw_help(f: &mut Frame, area: Rect, tab: ActiveTab, theme: &Theme) { + grey_overlay(f); let mut lines: Vec = vec![ key_line("q", "quit", theme), // Ctrl-C is a first-class quit gesture in both key loops (it restores the @@ -886,7 +887,8 @@ pub fn opt_row( #[cfg(test)] mod ported_chrome_tests { - use super::{draw_logo, grey_overlay, opt_row}; + use super::{draw_help, draw_logo, grey_overlay, opt_row}; + use crate::app::ActiveTab; use crate::ui::theme::Theme; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -955,6 +957,27 @@ mod ported_chrome_tests { ); } + #[test] + fn draw_help_dims_the_backdrop() { + // `draw_help` sizes its popup to its content, so on a large area there + // is backdrop left uncovered around it — the corner is always part of + // that backdrop, not the popup. Before `draw_help` called + // `grey_overlay`, that corner kept whatever the tab underneath had + // painted there instead of the dimmed wash every other modal uses. + let theme = Theme::from_name("default-dark"); + let backend = TestBackend::new(120, 30); + let mut term = Terminal::new(backend).unwrap(); + term.draw(|f| draw_help(f, f.area(), ActiveTab::Observe, &theme)) + .unwrap(); + let wash = ratatui::style::Color::Rgb(0x1c, 0x1e, 0x22); + let corner = term.backend().buffer().cell((0, 0)).unwrap(); + assert_eq!( + corner.style().bg, + Some(wash), + "help modal must dim its backdrop like every other modal" + ); + } + #[test] fn p4_overlays_render_key_content() { use crate::app::{ActiveTab, AppState}; diff --git a/crates/rocm-dash-tui/src/ui/tabs/bench.rs b/crates/rocm-dash-tui/src/ui/tabs/bench.rs index ddc3b305c..ca2f572bf 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/bench.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/bench.rs @@ -410,7 +410,9 @@ pub fn hit_test(area: Rect, x: u16, y: u16, state: &AppState) -> Option 0); max_scroll } @@ -835,12 +835,21 @@ fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scroll args_max.max(env_max) } -fn render_footer(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme) { +fn render_footer(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scrollable: bool) { let log = inst.log_file.as_deref().unwrap_or("-"); - let p = Paragraph::new(Line::from(vec![ + let mut spans = vec![ Span::styled("log: ", Style::default().fg(theme.muted)), Span::styled(log.to_string(), Style::default().fg(theme.muted)), - ])); + ]; + if scrollable { + // Only shown once `render_body` reports overflow — the launch_args/ + // env_vars panes otherwise give no hint that ↑/↓ do anything here. + spans.push(Span::styled( + " · ↑/↓ scroll", + Style::default().fg(theme.muted), + )); + } + let p = Paragraph::new(Line::from(spans)); f.render_widget(p, area); } From d92be04a80265d5e809e3f29dc19c1e2698b3b54 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 07:51:08 +0000 Subject: [PATCH 32/36] fix(dash-tui): footer Esc chip says dismiss for finished job console The job console popup was reworded in this PR to say "Enter/Esc dismiss" once its job reaches a terminal state, but the global footer's Esc chip still fell through to the generic sub-popup "cancel" label for that same state, contradicting the popup's own hint on screen. Give the finished-job-console case its own arm in draw_footer, ahead of the generic cancel fallback, using the negation of the existing console_esc_closes helper (matching the running-job arm's pattern so the two conditions can't drift apart). Update the corresponding test to assert "dismiss" instead of the old, now-incorrect "cancel" expectation. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/mod.rs | 38 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index d76bf3570..b341c5a00 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -460,13 +460,25 @@ fn draw_footer(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) -> Ve // A manager's job console is showing a still-running job — Esc fully // closes the overlay there (the job keeps running in the background), // matching the console's own footer hint ("Esc close (keeps - // running)"), not the generic sub-popup "cancel" below. Once the job - // finishes, `on_console_key` only dismisses the console back to the - // screen body (the overlay stays open), so that case falls through to - // the "cancel" arm below, which already describes it correctly. Shares + // running)"), not the generic sub-popup "cancel" below. Shares // `console_esc_closes` with `on_console_key` so the two can't drift. segs.push(Seg::Key("Esc", None)); segs.push(Seg::Sep(" close ")); + } else if state.has_open_overlay() + && state + .active_job_id() + .is_some_and(|id| !crate::ui::job_console::console_esc_closes(state.jobs.job(id))) + { + // The job console is showing a finished (or vanished) job — + // `on_console_key` only dismisses the console back to the screen body + // (the overlay itself stays open); nothing is left to cancel, + // matching the console's own reworded footer hint ("Enter/Esc + // dismiss", see `job_console::draw`). This must not fall through to + // the generic sub-popup "cancel" arm below, which would otherwise + // contradict that hint. Shares `console_esc_closes` with + // `on_console_key` (via negation) so the two can't drift. + segs.push(Seg::Key("Esc", None)); + segs.push(Seg::Sep(" dismiss ")); } else if state.has_open_overlay() { // A manager is open but not at its root layer (sub-popup, picker, or // approval) — Esc is handled by that layer's own event-loop arm, not @@ -821,13 +833,15 @@ mod tests { } #[test] - fn footer_esc_chip_labels_cancel_when_job_console_shows_a_finished_job() { + fn footer_esc_chip_labels_dismiss_when_job_console_shows_a_finished_job() { // Once the job console's job has finished, Esc only dismisses the // console back to the screen body (the overlay itself stays open) — // `on_console_key` never returns `Closed` for a terminal job. The - // footer chip must not claim "close" here; it falls through to the - // generic sub-popup "cancel" label, which already describes this - // case correctly. + // footer chip must not claim "close" here, and must not fall through + // to the generic sub-popup "cancel" label either — that would + // contradict the console's own reworded footer hint ("Enter/Esc + // dismiss", see `job_console::draw`). It gets a dedicated "dismiss" + // label instead. use crate::ui::theme::Theme; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -863,8 +877,8 @@ mod tests { .map(|x| term.backend().buffer().cell((x, 0)).unwrap().symbol()) .collect(); assert!( - row.contains("Esc cancel"), - "finished-job console Esc chip should say cancel: {row:?}" + row.contains("Esc dismiss"), + "finished-job console Esc chip should say dismiss: {row:?}" ); // Note: "close" legitimately appears elsewhere in this row (the `q` // chip always says "close" while any overlay is open — see @@ -875,6 +889,10 @@ mod tests { !row.contains("Esc close"), "finished-job console Esc chip should not say close: {row:?}" ); + assert!( + !row.contains("Esc cancel"), + "finished-job console Esc chip should not say cancel: {row:?}" + ); } #[test] From 4ff101dbbeebaab3877722139ae578ed1093cfe6 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 08:14:45 +0000 Subject: [PATCH 33/36] fix(dash): address review 5237323869 findings - instances.rs: reorder detail-modal footer so the scroll hint always stays visible instead of being clipped behind a long log path; add unit coverage asserting the hint appears/disappears with scrollable - bench.rs: delete dead hit_test (zero callers since bench detail was folded into Observe) plus the row_hit helper and tests that existed only to support it - dash.feature: extend dash-05 to assert the backdrop dims behind the help overlay, matching dash-13/dash-15 coverage for other overlays - dash_steps.rs: swap the ambiguous "Options" screen marker for "Quit" in dashboard_menu_is_displayed, since "Quit" uniquely identifies Modal::Menu Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/tabs/bench.rs | 124 +----------------- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 64 ++++++++- tests/e2e-cucumber/features/dash.feature | 1 + tests/e2e-cucumber/tests/e2e/dash_steps.rs | 9 +- 4 files changed, 66 insertions(+), 132 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/tabs/bench.rs b/crates/rocm-dash-tui/src/ui/tabs/bench.rs index ca2f572bf..d948db8bf 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/bench.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/bench.rs @@ -8,14 +8,14 @@ use ratatui::Frame; use ratatui::layout::{Constraint, Direction, Layout, Rect}; use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Paragraph}; +use ratatui::widgets::Paragraph; use crate::ui::panel::{self, BoxRole}; use rocm_dash_core::bench_rollup::{PassNRollup, rollup_pass_n, row_verdict}; use rocm_dash_core::bench_schema::{BenchmarkRow, PassFail}; -use crate::app::{AppState, KeyAction}; +use crate::app::AppState; use crate::ui::format; use crate::ui::sparkline::BrailleSparkline; use crate::ui::theme::Theme; @@ -339,86 +339,6 @@ fn draw_sparkline(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { f.render_widget(spark, inner); } -/// Pure helper: given the rows-table's *inner* (post-border) area and the -/// currently visible window `[start, end)`, resolve a click at `(x, y)` to -/// a bench-row index, or `None` if the click misses a data line. -/// -/// Row 0 of `rows_table_inner` is the header; rows 1..=visible are data -/// lines mapped to `[start, end)` in order. -const fn row_hit( - rows_table_inner: Rect, - start: usize, - end: usize, - x: u16, - y: u16, -) -> Option { - if rows_table_inner.width == 0 || rows_table_inner.height == 0 { - return None; - } - if x < rows_table_inner.x || x >= rows_table_inner.x + rows_table_inner.width { - return None; - } - if y < rows_table_inner.y || y >= rows_table_inner.y + rows_table_inner.height { - return None; - } - let row_offset = y - rows_table_inner.y; - if row_offset == 0 { - // Header line. - return None; - } - let visible = end.saturating_sub(start); - let data_idx = (row_offset - 1) as usize; - if data_idx >= visible { - return None; - } - Some(start + data_idx) -} - -/// Resolve a click at `(x, y)` inside the Bench Observe sub-panel body. Returns a -/// `KeyAction` to dispatch, or `None` when the click misses everything -/// actionable. -pub fn hit_test(area: Rect, x: u16, y: u16, state: &AppState) -> Option { - if state.bench_rows.is_empty() { - return None; - } - // Recompute the same vertical layout as `draw`. - let rollup_rows = rollup_pass_n(state.bench_rows.iter()); - let rollup_height = compute_rollup_height(rollup_rows.len()); - let chunks = Layout::default() - .direction(Direction::Vertical) - .constraints([ - Constraint::Length(rollup_height), - Constraint::Min(0), - Constraint::Length(3), - ]) - .split(area); - let rows_outer = chunks[1]; - // Mirror panel::bento's inner rect: rounded full border + the same adaptive - // padding it applies, so click mapping matches the drawn table exactly. - let rows_inner = Block::default() - .borders(Borders::ALL) - .padding(panel::padding_for(rows_outer)) - .inner(rows_outer); - - let total = state.bench_rows.len(); - let avail = (rows_inner.height as usize).saturating_sub(1); - if avail == 0 { - return None; - } - let sel = state.bench_sel.min(total.saturating_sub(1)); - let (start, end) = visible_window(total, avail, sel); - - let target = row_hit(rows_inner, start, end, x, y)?; - if target == state.bench_sel { - // No bench detail view exists any more (folded into Observe by the P3 - // IA redesign) — clicking the already-selected row has nothing to open. - None - } else { - let delta = target.cast_signed() - state.bench_sel.cast_signed(); - Some(KeyAction::Move(delta)) - } -} - #[cfg(test)] mod tests { use super::*; @@ -508,44 +428,4 @@ mod tests { assert!(s <= 10 && 10 < e); assert_eq!(e - s, 3); } - - #[test] - fn row_hit_returns_none_for_header_or_out_of_bounds() { - // 30 cols wide, 10 rows tall, anchored at (5, 2). - let inner = Rect::new(5, 2, 30, 10); - // Header row at y=2. - assert_eq!(row_hit(inner, 0, 5, 10, 2), None); - // Outside x range. - assert_eq!(row_hit(inner, 0, 5, 4, 3), None); - assert_eq!(row_hit(inner, 0, 5, 35, 3), None); - // Outside y range. - assert_eq!(row_hit(inner, 0, 5, 10, 1), None); - assert_eq!(row_hit(inner, 0, 5, 10, 12), None); - } - - #[test] - fn row_hit_maps_data_lines_to_window_indices() { - let inner = Rect::new(0, 0, 20, 10); - // Window [10, 15): 5 data rows starting at y=1. - assert_eq!(row_hit(inner, 10, 15, 5, 1), Some(10)); - assert_eq!(row_hit(inner, 10, 15, 5, 2), Some(11)); - assert_eq!(row_hit(inner, 10, 15, 5, 5), Some(14)); - // y=6 lands past the visible window (only 5 data rows shown). - assert_eq!(row_hit(inner, 10, 15, 5, 6), None); - } - - #[test] - fn row_hit_handles_zero_dim_area() { - let zero_w = Rect::new(0, 0, 0, 10); - assert_eq!(row_hit(zero_w, 0, 5, 0, 1), None); - let zero_h = Rect::new(0, 0, 10, 0); - assert_eq!(row_hit(zero_h, 0, 5, 0, 0), None); - } - - #[test] - fn row_hit_handles_empty_window() { - let inner = Rect::new(0, 0, 10, 5); - // start == end → no data lines. - assert_eq!(row_hit(inner, 3, 3, 5, 1), None); - } } diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 2584b3bde..4f8c64746 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -837,18 +837,21 @@ fn render_body(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scroll fn render_footer(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scrollable: bool) { let log = inst.log_file.as_deref().unwrap_or("-"); - let mut spans = vec![ - Span::styled("log: ", Style::default().fg(theme.muted)), - Span::styled(log.to_string(), Style::default().fg(theme.muted)), - ]; + let mut spans = Vec::new(); if scrollable { // Only shown once `render_body` reports overflow — the launch_args/ // env_vars panes otherwise give no hint that ↑/↓ do anything here. + // Rendered first (not appended after the log path) so the hint + // stays visible even when a long log path gets clipped by the + // footer's width — Paragraph here isn't wrapped, so anything past + // `area.width` is silently dropped rather than truncated in place. spans.push(Span::styled( - " · ↑/↓ scroll", + "↑/↓ scroll · ", Style::default().fg(theme.muted), )); } + spans.push(Span::styled("log: ", Style::default().fg(theme.muted))); + spans.push(Span::styled(log.to_string(), Style::default().fg(theme.muted))); let p = Paragraph::new(Line::from(spans)); f.render_widget(p, area); } @@ -1358,6 +1361,57 @@ mod tests { ); } + #[test] + fn detail_modal_footer_shows_scroll_hint_only_when_scrollable() { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + // Overflow the body pane (same recipe as the scroll test above) so + // `draw_detail` computes a nonzero max_scroll and passes + // `scrollable = true` into `render_footer`. + let mut inst = mk_inst("overflow"); + inst.launch_args = (0..60).map(|i| format!("--flag-{i}=value{i}")).collect(); + let mut m = HashMap::new(); + m.insert(inst.container_id.clone(), inst); + let state = mk_state(m, 0); + + let mut term = Terminal::new(TestBackend::new(160, 30)).unwrap(); + let mut max_scroll = 0u16; + term.draw(|f| { + max_scroll = draw_detail(f, f.area(), &state, &state.theme); + }) + .unwrap(); + assert!( + max_scroll > 0, + "60 launch_args must overflow the body pane, giving a nonzero max_scroll; got {max_scroll}" + ); + let scrollable_text = buffer_text(&term); + assert!( + scrollable_text.contains("↑/↓ scroll"), + "footer must show the scroll hint once the body overflows; got:\n{scrollable_text}" + ); + + // A non-overflowing instance (no launch_args/env_vars) yields + // max_scroll == 0, so `scrollable` is false and the hint must be + // absent from the footer. + let inst_small = mk_inst("small"); + let mut m2 = HashMap::new(); + m2.insert(inst_small.container_id.clone(), inst_small); + let state_small = mk_state(m2, 0); + + let mut term2 = Terminal::new(TestBackend::new(160, 30)).unwrap(); + term2 + .draw(|f| { + draw_detail(f, f.area(), &state_small, &state_small.theme); + }) + .unwrap(); + let non_scrollable_text = buffer_text(&term2); + assert!( + !non_scrollable_text.contains("↑/↓ scroll"), + "footer must not show the scroll hint when the body does not overflow; got:\n{non_scrollable_text}" + ); + } + #[test] fn nonfinite_ttft_tpot_render_dash_never_nan() { use ratatui::Terminal; diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index fab6c619f..81a371c9e 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -46,6 +46,7 @@ Feature: Interactive dashboard When the user opens the dashboard with demo data And the user opens dashboard help Then navigation and next-step guidance are displayed + And the backdrop behind the popup is dimmed When the user closes dashboard help And the user quits the dashboard Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index aa13f436f..9bc2e87b7 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -841,12 +841,11 @@ async fn dashboard_destinations_displayed(world: &mut E2eWorld) { #[then("the dashboard menu is displayed")] async fn dashboard_menu_is_displayed(world: &mut E2eWorld) { - // "Options" is used here as a marker for `Modal::Menu`'s three items - // (Options/Help/Quit). Note: the Settings panel (`Modal::Options`) also - // headers with "Options", so this marker is not unique app-wide — it is - // safe only because no scenario today opens Settings before this step. + // "Quit" is used here as a marker for `Modal::Menu`'s three items + // (Options/Help/Quit). Unlike "Options", "Quit" appears nowhere else in + // the TUI's rendered chrome, so it unambiguously identifies the menu. session(world) - .wait_for_screen("Options", default_timeout()) + .wait_for_screen("Quit", default_timeout()) .await .unwrap_or_else(|e| panic!("dashboard menu did not appear: {e}")); } From 70e33e68a64ec56d6fe6e8975f7d0b2c9eb19015 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 08:26:48 +0000 Subject: [PATCH 34/36] style(dash-tui): satisfy cargo fmt for instances.rs log span Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index 4f8c64746..b1fd5c56d 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -851,7 +851,10 @@ fn render_footer(f: &mut Frame, area: Rect, inst: &Instance, theme: &Theme, scro )); } spans.push(Span::styled("log: ", Style::default().fg(theme.muted))); - spans.push(Span::styled(log.to_string(), Style::default().fg(theme.muted))); + spans.push(Span::styled( + log.to_string(), + Style::default().fg(theme.muted), + )); let p = Paragraph::new(Line::from(spans)); f.render_widget(p, area); } From ef794c90efce779ec982a67cd157e30bfb35f71d Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 08:42:48 +0000 Subject: [PATCH 35/36] fix(dash-tui): swallow body clicks while a chat approval is pending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_mouse's body-click swallow guard only checked has_open_overlay(), not approval.is_some(). open_approval() closes every manager overlay but never touches modal, so with no overlay open and modal == Modal::None, a click on the body during a pending chat tool-call approval fell through to the tab's hit-test (e.g. ui::tabs::rocm::hit_test) instead of being swallowed by the approval modal that owns the screen — able to silently change table selection or switch tabs underneath the modal. Mirrors the `has_open_overlay() || approval.is_some()` gate this PR already added for footer-chip clicks in ui/mod.rs. Also corrects a stale comment at the should_pane_back_out event-loop arm that still described the pre-PR ROCm/Serving-only gating; the function's own doc comment already explains the any-tab widening. Signed-off-by: Jussi Elo --- crates/rocm-dash-tui/src/app/mod.rs | 51 ++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 178989b9d..726e0e7fb 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -2704,11 +2704,13 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu None => { /* cursor moved or key ignored — modal stays open */ } } } - // De-modal back-out: on ROCm/Serving, an inline manager is - // shown in the Details pane. Esc closes it and returns focus + // De-modal back-out: on any tab, when an inline manager is + // open at its root screen, Esc closes it and returns focus // to the Actions list — intercepted BEFORE the per-manager // key arms so the manager doesn't eat Esc first. `←` is left - // to the manager (some use it to cycle options). + // to the manager (some use it to cycle options). See + // `should_pane_back_out`'s doc comment for why this is no + // longer gated to ROCm/Serving. Some(Ok(CtEvent::Key(k))) if state.should_pane_back_out(k.code) => { state.close_overlays(); state.pane_focus = PaneFocus::Actions; @@ -3537,11 +3539,15 @@ fn resolve_mouse(me: MouseEvent, state: &AppState) -> KeyAction { if let Some(chip) = footer_chip_hit(&state.last_footer_chips, me.column, me.row) { return chip; } - // While an operational manager is open it owns the body — swallow body - // clicks so they can't fall THROUGH the inline manager to the obscured - // Actions/Details list (which would silently change the selection or - // re-open a verb). Tab-bar and footer-chip clicks above still work. - if state.has_open_overlay() { + // While an operational manager is open — or a chat tool-call approval + // is pending — it owns the body: swallow body clicks so they can't + // fall THROUGH to the obscured Actions/Details list (which would + // silently change the selection, re-open a verb, or switch tabs + // underneath the approval modal). Tab-bar and footer-chip clicks + // above still work, matching the manager-overlay swallow this + // mirrors (see the analogous `state.approval.is_some()` check next + // to `has_open_overlay()` in ui/mod.rs's footer-chip gating). + if state.has_open_overlay() || state.approval.is_some() { return KeyAction::Nothing; } if state.modal == Modal::None @@ -5056,6 +5062,35 @@ mod tests { assert_eq!(resolve_mouse(click, &s), KeyAction::Nothing); } + #[test] + fn body_clicks_are_swallowed_while_an_approval_is_pending() { + use crossterm::event::{MouseButton, MouseEvent, MouseEventKind}; + let mut s = AppState::new("t".into(), "default-dark".into()); + s.active_tab = ActiveTab::Rocm; + s.last_body_area = Some(Rect::new(2, 4, 150, 30)); + let click = MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column: 90, + row: 10, + modifiers: KeyModifiers::NONE, + }; + // No approval pending → the click resolves against the tab's hit-test. + assert_ne!(resolve_mouse(click, &s), KeyAction::Nothing); + // `open_approval` clears every manager overlay (so `has_open_overlay()` + // is false) but never touches `modal` — the body click must still be + // swallowed instead of falling through to the obscured Actions/Details + // list underneath the approval modal. + s.open_approval(crate::tool_exec::ApprovalIntent { + title: "run a command".into(), + body: vec!["echo hi".into()], + name: "shell".into(), + arguments: serde_json::Value::Null, + }); + assert!(!s.has_open_overlay()); + assert_eq!(s.modal, Modal::None); + assert_eq!(resolve_mouse(click, &s), KeyAction::Nothing); + } + /// Build a ScrollDown/Up/Left/Right event at a pointer position. fn wheel(kind: MouseEventKind, col: u16, row: u16) -> MouseEvent { MouseEvent { From c5249e1b140ea8410af914960bb06bd4d3332af2 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 09:25:55 +0000 Subject: [PATCH 36/36] test(dash): cover instance-detail scroll hint on overflow (dash-22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit siloteemu flagged that no scenario exercises the instance-detail footer's ↑/↓ scroll hint. The demo fixtures' launch_args/env_vars are too few to overflow the popup at the existing terminal sizes, so add a smaller resize target and a scenario that shrinks the terminal until the detail body overflows, then asserts the hint appears. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/dash.feature | 14 +++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 26 ++++++++++++++++ tests/e2e-cucumber/tests/e2e/tui_driver.rs | 36 +++++++++++++++++----- 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index 81a371c9e..878064f6e 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -273,3 +273,17 @@ Feature: Interactive dashboard When the user presses Ctrl-C in the launcher Then the launcher exits from the keystroke with code 130 And the terminal is restored to the normal screen + + @id:dash-instance-detail-scroll-hint @requires-os:linux + Scenario: dash-22 - Instance detail shows a scroll hint when content overflows + # The demo fixtures' launch_args/env_vars are too few to overflow the detail + # popup at the default or enlarged terminal size, so this shrinks the + # terminal until the args/env panes can't fit them, exercising the one path + # (render_footer's scrollable branch) unit tests can't reach end to end. + When the user opens the dashboard with demo data + And the user opens the Observe view + And the user opens instance detail + And the user shrinks the terminal until the detail body overflows + Then the instance detail footer shows the scroll hint + When the user quits the dashboard + Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 9bc2e87b7..a06f245ce 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -221,6 +221,22 @@ async fn open_instance_detail(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("instance detail did not open: {e}")); } +#[when("the user shrinks the terminal until the detail body overflows")] +async fn shrink_until_detail_overflows(world: &mut E2eWorld) { + // The demo fixtures' `launch_args`/`env_vars` are too few to overflow the + // detail popup at either the default or the enlarged (`use_detail_size`) + // geometry — this is the only terminal size small enough to force it. + let tui = session(world); + tui.use_overflow_size() + .unwrap_or_else(|e| panic!("failed to shrink the dashboard: {e}")); + // The resize alone doesn't prove the app redrew at the new geometry yet; + // the popup title reappearing at the smaller size is that proof, ahead of + // the `Then` step's specific scroll-hint assertion. + tui.wait_for_screen("Instance · ", default_timeout()) + .await + .unwrap_or_else(|e| panic!("instance detail did not redraw after shrinking: {e}")); +} + #[when("the user opens the services manager")] async fn open_services_manager(world: &mut E2eWorld) { // Bound to `s` only on the Observe tab (`OpenServices`) — a manager opened @@ -816,6 +832,16 @@ async fn instance_details_displayed(world: &mut E2eWorld) { .unwrap_or_else(|e| panic!("instance details did not appear: {e}")); } +#[then("the instance detail footer shows the scroll hint")] +async fn detail_footer_shows_scroll_hint(world: &mut E2eWorld) { + let tui = session(world); + let screen = tui.screen_text(); + assert!( + screen.contains("↑/↓ scroll"), + "footer did not show the scroll hint once the detail body overflowed:\n{screen}" + ); +} + #[then("the backdrop behind the popup is dimmed")] async fn backdrop_is_dimmed(world: &mut E2eWorld) { let tui = session(world); diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index c217bf9e7..b0bd1b1b0 100644 --- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs +++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs @@ -44,6 +44,15 @@ const COLS: u16 = 80; /// cards (managed instances and live serving metrics). const DETAIL_ROWS: u16 = 40; const DETAIL_COLS: u16 = 120; +/// Short enough that the instance-detail popup's `launch_args`/`env_vars` +/// panes can't fit even the demo fixtures' handful of entries, forcing +/// `render_body` to report a nonzero max scroll and the footer's `↑/↓ scroll` +/// hint to appear — the only way to exercise that hint end to end, since the +/// demo containers (`rocm-dash-daemon`'s `CONTAINERS`) don't carry enough +/// launch_args/env_vars to overflow the popup at `ROWS`/`COLS` or +/// `DETAIL_ROWS`/`DETAIL_COLS`, both of which only ever *enlarge* it. +const OVERFLOW_ROWS: u16 = 20; +const OVERFLOW_COLS: u16 = 90; /// How often `wait_for_*` re-checks the screen/process while waiting. This is a /// poll cadence, not a fixed readiness sleep: every wait has a deadline and @@ -366,13 +375,13 @@ impl TuiSession { self.reader_failure.take_message() } - /// Resize both the real PTY and the emulated screen. The application receives - /// the normal terminal resize event; assertions continue to inspect exactly - /// what a user would see at the new geometry. - pub fn use_detail_size(&mut self) -> Result<(), String> { + /// Resize both the real PTY and the emulated screen to `rows`x`cols`. The + /// application receives the normal terminal resize event; assertions + /// continue to inspect exactly what a user would see at the new geometry. + fn resize_to(&mut self, rows: u16, cols: u16) -> Result<(), String> { let size = PtySize { - rows: DETAIL_ROWS, - cols: DETAIL_COLS, + rows, + cols, pixel_width: 0, pixel_height: 0, }; @@ -386,10 +395,23 @@ impl TuiSession { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .screen_mut() - .set_size(DETAIL_ROWS, DETAIL_COLS); + .set_size(rows, cols); Ok(()) } + /// Enlarge to [`DETAIL_ROWS`]x[`DETAIL_COLS`], for journeys that assert + /// rows below the dashboard's summary cards. + pub fn use_detail_size(&mut self) -> Result<(), String> { + self.resize_to(DETAIL_ROWS, DETAIL_COLS) + } + + /// Shrink to [`OVERFLOW_ROWS`]x[`OVERFLOW_COLS`] — small enough that the + /// instance-detail popup's `launch_args`/`env_vars` panes can't fit the + /// demo fixtures' entries, forcing a scrollable body. + pub fn use_overflow_size(&mut self) -> Result<(), String> { + self.resize_to(OVERFLOW_ROWS, OVERFLOW_COLS) + } + /// Write raw bytes to the terminal (keystrokes/text). `Enter` is `"\r"`. pub fn send(&mut self, bytes: &str) -> Result<(), String> { self.writer