naps62/maestro
Archived
1
0
Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(maestro): sidebar j/k skips expanded group headers

Browse Source 
Group rows are not something to focus; the cards under them are one row
away. A collapsed group still stops the walk — its header is the only row
it has, so skipping it would leave the group unreachable.
This commit is contained in:
Miguel Palhas
2026-08-31 14:58:07 +01:00
parent 904a665432
commit 0421a153c2
2 changed files with 165 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/maestro-tui/src/app.rs
+72 -21
View File
@@ -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);
crates/maestro-tui/src/state.rs
+93 -14
View File
@@ -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);
}