diff --git a/crates/maestro-tui/src/app.rs b/crates/maestro-tui/src/app.rs index c0f19d7..81d136e 100644 --- a/crates/maestro-tui/src/app.rs +++ b/crates/maestro-tui/src/app.rs @@ -353,22 +353,8 @@ impl App { Action::ScrollPanePageDown => { self.scroll_active_pane(-(self.pane_page_rows() as isize)) } - Action::SidebarNext => { - let order = visible_sidebar_order( - &self.catalog.snapshot(), - &self.state.collapsed_sidebar_groups, - ); - self.state.move_sidebar_highlight(&order, 1); - self.state.focus = Focus::Sidebar; - } - Action::SidebarPrev => { - let order = visible_sidebar_order( - &self.catalog.snapshot(), - &self.state.collapsed_sidebar_groups, - ); - self.state.move_sidebar_highlight(&order, -1); - self.state.focus = Focus::Sidebar; - } + Action::SidebarNext => self.step_sidebar_highlight(1), + Action::SidebarPrev => self.step_sidebar_highlight(-1), Action::ToggleSidebarGroup => self.toggle_highlighted_sidebar_group(), Action::ToggleSidebar => self.state.toggle_sidebar(), Action::ShowHelp => { @@ -459,6 +445,21 @@ impl App { snap.sessions.first().map(|s| s.id) } + /// `j`/`k`: step the sidebar highlight one row. Group headers are skipped + /// — a directory is not something to focus, and the cards under it are one + /// row away anyway. A *collapsed* group is the exception: its header is the + /// only row it has, so skipping it would make the group unreachable. + fn step_sidebar_highlight(&mut self, delta: i32) { + let collapsed = self.state.collapsed_sidebar_groups.clone(); + let order = visible_sidebar_order(&self.catalog.snapshot(), &collapsed); + self.state + .move_sidebar_highlight(&order, delta, |item| match item { + SidebarItem::Group(key) => collapsed.contains(key), + SidebarItem::Session(_) => true, + }); + self.state.focus = Focus::Sidebar; + } + fn toggle_highlighted_sidebar_group(&mut self) { let key = match self.state.sidebar_highlight.clone() { Some(SidebarItem::Group(key)) => Some(key), @@ -1338,11 +1339,9 @@ mod tests { app.catalog.apply_event(CatalogEvent::Snapshot { sessions: vec![test_session(sid, "/repo")], }); - app.dispatch(Action::SidebarNext); - assert_eq!( - app.state.sidebar_highlight, - Some(SidebarItem::Group(PathBuf::from("/repo"))) - ); + // `j` never lands on an expanded group, so reach it the way a click + // would. + app.state.sidebar_highlight = Some(SidebarItem::Group(PathBuf::from("/repo"))); assert!(app.dispatch(Action::AttachHighlighted).is_empty()); assert!(app .state @@ -1357,6 +1356,58 @@ mod tests { assert!(app.state.overlay().is_none()); } + #[test] + fn sidebar_navigation_skips_expanded_group_headers() { + use maestro_protocol::CatalogEvent; + + let a = Uuid::from_u128(20); + let b = Uuid::from_u128(21); + let mut app = make_app(); + app.catalog.apply_event(CatalogEvent::Snapshot { + sessions: vec![test_session(a, "/one"), test_session(b, "/two")], + }); + + // Two groups, one card each: `j` visits the cards and nothing else. + app.dispatch(Action::SidebarNext); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(a))); + app.dispatch(Action::SidebarNext); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(b))); + app.dispatch(Action::SidebarNext); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(a))); + app.dispatch(Action::SidebarPrev); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(b))); + } + + /// A collapsed group hides its cards, so its header is the only row left + /// to land on — skipping it would strand the group. + #[test] + fn sidebar_navigation_stops_on_a_collapsed_group() { + use maestro_protocol::CatalogEvent; + + let a = Uuid::from_u128(22); + let b = Uuid::from_u128(23); + let mut app = make_app(); + app.catalog.apply_event(CatalogEvent::Snapshot { + sessions: vec![test_session(a, "/one"), test_session(b, "/two")], + }); + app.dispatch(Action::SidebarNext); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(a))); + // `c` on a card collapses the group that holds it. + app.dispatch(Action::ToggleSidebarGroup); + assert_eq!( + app.state.sidebar_highlight, + Some(SidebarItem::Group(PathBuf::from("/one"))) + ); + + app.dispatch(Action::SidebarNext); + assert_eq!(app.state.sidebar_highlight, Some(SidebarItem::Session(b))); + app.dispatch(Action::SidebarNext); + assert_eq!( + app.state.sidebar_highlight, + Some(SidebarItem::Group(PathBuf::from("/one"))) + ); + } + #[test] fn collapsing_the_group_containing_a_card_highlight_keeps_a_visible_group_highlight() { let sid = Uuid::from_u128(15); diff --git a/crates/maestro-tui/src/state.rs b/crates/maestro-tui/src/state.rs index 32ac0d1..154d680 100644 --- a/crates/maestro-tui/src/state.rs +++ b/crates/maestro-tui/src/state.rs @@ -165,21 +165,42 @@ impl AppState { self.active_tab().session() } - /// Move the sidebar highlight by `delta` (positive = down) within the - /// rendered item sequence. Wraps. No-op on empty. - pub fn move_sidebar_highlight(&mut self, order: &[SidebarItem], delta: i32) { - if order.is_empty() { + /// Move the sidebar highlight one step through the rendered item + /// sequence, in the direction of `delta` (positive = down), landing only + /// on items `is_stop` accepts. Only the sign of `delta` matters. Wraps. + /// No-op when `order` holds no stop. + /// + /// Anchoring walks the *full* rendered order, not just the stops, so a + /// highlight parked on a skipped row — an expanded group header, which a + /// mouse click can still select — steps to its neighbour instead of + /// jumping to the far end of the list. + pub fn move_sidebar_highlight( + &mut self, + order: &[SidebarItem], + delta: i32, + is_stop: impl Fn(&SidebarItem) -> bool, + ) { + if order.is_empty() || delta == 0 { return; } let len = order.len() as i32; + let step = delta.signum(); let cur = self .sidebar_highlight .as_ref() .and_then(|id| order.iter().position(|x| x == id)) .map(|i| i as i32) - .unwrap_or(-1); - let next = (cur + delta).rem_euclid(len) as usize; - self.sidebar_highlight = Some(order[next].clone()); + // Nothing highlighted: start one off the end we walk from, so the + // first step lands on the first (or last) item. + .unwrap_or(if step > 0 { -1 } else { 0 }); + let mut idx = cur; + for _ in 0..len { + idx = (idx + step).rem_euclid(len); + if is_stop(&order[idx as usize]) { + self.sidebar_highlight = Some(order[idx as usize].clone()); + return; + } + } } /// Toggle a group. When collapsing a group containing the current card, @@ -232,28 +253,86 @@ mod tests { ]; // No prior highlight + delta +1 lands on first item. - state.move_sidebar_highlight(&order, 1); + state.move_sidebar_highlight(&order, 1, |_| true); assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(a))); - state.move_sidebar_highlight(&order, 1); + state.move_sidebar_highlight(&order, 1, |_| true); assert_eq!( state.sidebar_highlight, Some(SidebarItem::Group("repo".into())) ); - state.move_sidebar_highlight(&order, 1); + state.move_sidebar_highlight(&order, 1, |_| true); assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(b))); - // Wraps forward. - state.move_sidebar_highlight(&order, 1); + state.move_sidebar_highlight(&order, 1, |_| true); assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(c))); + // Wraps forward. + state.move_sidebar_highlight(&order, 1, |_| true); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(a))); // Wraps backward. - state.move_sidebar_highlight(&order, -1); + state.move_sidebar_highlight(&order, -1, |_| true); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(c))); + } + + #[test] + fn move_sidebar_highlight_skips_rows_the_filter_rejects() { + let mut state = AppState::new(); + let a = Uuid::from_u128(1); + let b = Uuid::from_u128(2); + let order = vec![ + SidebarItem::Group("repo".into()), + SidebarItem::Session(a), + SidebarItem::Group("other".into()), + SidebarItem::Session(b), + ]; + let sessions_only = |item: &SidebarItem| matches!(item, SidebarItem::Session(_)); + + state.move_sidebar_highlight(&order, 1, sessions_only); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(a))); + state.move_sidebar_highlight(&order, 1, sessions_only); assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(b))); + state.move_sidebar_highlight(&order, -1, sessions_only); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(a))); + + // Backwards from nothing lands on the last stop, not the last row. + state.sidebar_highlight = None; + state.move_sidebar_highlight(&order, -1, sessions_only); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(b))); + } + + /// A highlight sitting on a skipped row still moves to its neighbour. + #[test] + fn move_sidebar_highlight_anchors_on_a_skipped_row() { + let mut state = AppState::new(); + let a = Uuid::from_u128(1); + let b = Uuid::from_u128(2); + let order = vec![ + SidebarItem::Session(a), + SidebarItem::Group("repo".into()), + SidebarItem::Session(b), + ]; + let sessions_only = |item: &SidebarItem| matches!(item, SidebarItem::Session(_)); + + state.sidebar_highlight = Some(SidebarItem::Group("repo".into())); + state.move_sidebar_highlight(&order, 1, sessions_only); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(b))); + + state.sidebar_highlight = Some(SidebarItem::Group("repo".into())); + state.move_sidebar_highlight(&order, -1, sessions_only); + assert_eq!(state.sidebar_highlight, Some(SidebarItem::Session(a))); + } + + #[test] + fn move_sidebar_highlight_is_noop_when_nothing_is_a_stop() { + let mut state = AppState::new(); + let order = vec![SidebarItem::Group("repo".into())]; + state.move_sidebar_highlight(&order, 1, |_| false); + assert_eq!(state.sidebar_highlight, None); } #[test] fn move_sidebar_highlight_is_noop_on_empty_order() { let mut state = AppState::new(); - state.move_sidebar_highlight(&[], 1); + state.move_sidebar_highlight(&[], 1, |_| true); assert_eq!(state.sidebar_highlight, None); }