From 91f20bbca367545f7820f833f5c867527bf48952 Mon Sep 17 00:00:00 2001 From: Edwin Date: Sat, 22 Aug 2026 11:20:11 -0700 Subject: [PATCH] fix(daemon): let move-down enter the only collapsed project below MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reordering a session down did nothing whenever every project below it was collapsed, while reordering the same session up worked. The asymmetry is structural, not incidental: the ungrouped region always renders first and can never be collapsed, so an upward skip always has somewhere to land, but a downward skip needs an *expanded* region below. In the steady state of a working fleet — one project expanded, the rest collapsed — the last session of the first project could never move down at all, with project rows sitting plainly beneath it and the TUI reporting "nothing to reorder past". When the collapsed-skip runs out of regions, fall back to the adjacent one, expand it, and land the session at its top. Expanding is what keeps the visible model honest: dropping the session into a collapsed project would look like deleting it from the list. Spec 0007's jump is untouched — whenever any visible region exists beyond a collapsed one, the collapsed project is still skipped whole and its members and collapse state are left alone. A no-op from move-down now means the true bottom of the list, so the existing status message is accurate rather than misleading. Move-up needs no counterpart; that branch is unreachable for the reason above, so it is documented in the code rather than written untestable. --- crates/daemon/src/session.rs | 139 +++++++++++++++++- ...st-actions-target-visible-user-sessions.md | 2 + ...eorder-never-dead-ends-at-a-visible-row.md | 42 ++++++ 3 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 specs/0209-reorder-never-dead-ends-at-a-visible-row.md diff --git a/crates/daemon/src/session.rs b/crates/daemon/src/session.rs index dee701dd..22bb2498 100644 --- a/crates/daemon/src/session.rs +++ b/crates/daemon/src/session.rs @@ -5772,11 +5772,14 @@ impl SessionManager { /// /// Collapsed groups are skipped at boundaries: their members are hidden, /// so the session jumps the whole project in one step instead of swapping - /// with each hidden member. + /// with each hidden member. When *every* region below is collapsed the + /// skip has nowhere to land, so move-down enters the nearest one and + /// expands it rather than refusing — otherwise the last session of the + /// first project could never move down at all in the common steady state + /// of one expanded project and the rest collapsed. /// /// No-op at the absolute top (ungrouped session #0) or bottom (last - /// member of last group), or when the only regions in the move direction - /// are collapsed groups. + /// member of last group). /// /// Returns whether a move actually happened, so callers can tell a real /// reorder apart from hitting a boundary — both look identical from the @@ -5968,8 +5971,35 @@ impl SessionManager { // skipping collapsed projects. let next = groups::region_below_skipping_collapsed(me.group_id.as_deref(), &all_groups); - let Some(next_region) = next else { - return Ok(false); + let next_region = match next { + Some(region) => region, + None => { + // Every region below is a collapsed project, so the + // skip has nowhere to land. Refusing here makes the key + // look broken rather than bounded: the steady state of + // a working fleet is one expanded project with the rest + // collapsed, which leaves the last session of the first + // project unable to move down *at all* even though + // project rows sit plainly below it. Land in the + // nearest one instead and expand it, so the session + // stays visible where the user dropped it — the + // rendered list keeps matching the reorder model. + // + // Move-up needs no counterpart: the ungrouped region + // always renders above every project and can never be + // collapsed, so the upward skip always has somewhere to + // land. That asymmetry is why moving up kept working + // here while moving down did nothing. + let Some(collapsed_region) = + groups::region_below(me.group_id.as_deref(), &all_groups) + else { + return Ok(false); + }; + if let Some(gid) = collapsed_region.as_deref() { + self.set_group_collapsed(gid, false).await?; + } + collapsed_region + } }; self.move_session_into_region(&me.id, &next_region, RegionEdge::Top, &all_sessions) .await?; @@ -10127,6 +10157,105 @@ mod tests { assert!(mover.position > 0, "should land below su-top (pos 0)"); } + #[tokio::test] + async fn move_session_down_expands_the_only_collapsed_region_below() { + use construct_protocol::{MoveDirection, SessionKind}; + use tempfile::tempdir; + + let tmp = tempdir().expect("tempdir"); + let storage = + Arc::new(crate::storage::Storage::new(tmp.path().join("data")).expect("storage")); + let config = Arc::new(crate::config::Config::default()); + let (mgr, _remote_rx, _restart_rx) = + SessionManager::new(storage, config, tmp.path().join("run")) + .await + .expect("session manager"); + + // The steady state of a working fleet: one expanded project on top, + // every project below it collapsed. Nothing the skip can land on. + insert_group(&mgr, "gexp", 0, false).await; + insert_group(&mgr, "gcol-1", 1, true).await; + insert_group(&mgr, "gcol-2", 2, true).await; + for (id, position, group) in [ + ("ge-1", 0, Some("gexp".to_string())), + ("ge-mover", 1, Some("gexp".to_string())), + ("gc1-1", 0, Some("gcol-1".to_string())), + ("gc2-1", 0, Some("gcol-2".to_string())), + ] { + mgr.sessions.write().await.insert( + id.into(), + synthetic_entry_with_group(id, SessionKind::User, position, group), + ); + } + + // Moving down from the bottom of `gexp` must not be a no-op just + // because every region below is collapsed: two project rows are + // plainly visible below the session, so refusing looks like a broken + // key. Land in the nearest one and expand it so the session stays + // where the user dropped it. + let moved = mgr + .move_session("ge-mover", MoveDirection::Down) + .await + .expect("move down"); + assert!(moved, "move down must report a real reorder"); + + let sessions = mgr.list().await; + let mover = sessions.iter().find(|s| s.id == "ge-mover").unwrap(); + assert_eq!(mover.group_id.as_deref(), Some("gcol-1")); + assert!(mover.position < 0, "should land above gc1-1 (pos 0)"); + + let groups = mgr.list_groups().await; + let gcol1 = groups.iter().find(|g| g.id == "gcol-1").unwrap(); + assert!( + !gcol1.collapsed, + "the entered project must expand so the moved session stays visible", + ); + // Only the project the session entered expands; the rest keep the + // collapse state the user chose. + let gcol2 = groups.iter().find(|g| g.id == "gcol-2").unwrap(); + assert!(gcol2.collapsed, "untouched projects stay collapsed"); + } + + #[tokio::test] + async fn move_session_down_stops_at_the_last_project() { + use construct_protocol::{MoveDirection, SessionKind}; + use tempfile::tempdir; + + let tmp = tempdir().expect("tempdir"); + let storage = + Arc::new(crate::storage::Storage::new(tmp.path().join("data")).expect("storage")); + let config = Arc::new(crate::config::Config::default()); + let (mgr, _remote_rx, _restart_rx) = + SessionManager::new(storage, config, tmp.path().join("run")) + .await + .expect("session manager"); + + insert_group(&mgr, "glast", 0, false).await; + for (id, position, group) in [ + ("gl-1", 0, Some("glast".to_string())), + ("gl-mover", 1, Some("glast".to_string())), + ] { + mgr.sessions.write().await.insert( + id.into(), + synthetic_entry_with_group(id, SessionKind::User, position, group), + ); + } + + // The last member of the last project is the true bottom of the list: + // there is no region below at all, collapsed or otherwise, so the + // boundary still reports "nothing to reorder past". + let moved = mgr + .move_session("gl-mover", MoveDirection::Down) + .await + .expect("move down"); + assert!(!moved, "the absolute bottom is still a no-op"); + + let sessions = mgr.list().await; + let mover = sessions.iter().find(|s| s.id == "gl-mover").unwrap(); + assert_eq!(mover.group_id.as_deref(), Some("glast")); + assert_eq!(mover.position, 1); + } + #[tokio::test] async fn install_memory_env_sets_global_and_project_paths() { use tempfile::tempdir; diff --git a/specs/0007-session-list-actions-target-visible-user-sessions.md b/specs/0007-session-list-actions-target-visible-user-sessions.md index 5d4de4fa..ea9972d3 100644 --- a/specs/0007-session-list-actions-target-visible-user-sessions.md +++ b/specs/0007-session-list-actions-target-visible-user-sessions.md @@ -28,3 +28,5 @@ This does not forbid commands that operate on all sessions. Such commands must b ## Examples If a project is collapsed, moving a visible session down should jump over that collapsed block rather than targeting one of its hidden sessions. + +See 0209 for the boundary case where every region in the move direction is collapsed and the skip has nowhere to land. diff --git a/specs/0209-reorder-never-dead-ends-at-a-visible-row.md b/specs/0209-reorder-never-dead-ends-at-a-visible-row.md new file mode 100644 index 00000000..61020b66 --- /dev/null +++ b/specs/0209-reorder-never-dead-ends-at-a-visible-row.md @@ -0,0 +1,42 @@ +# 0209-reorder-never-dead-ends-at-a-visible-row + +Status: accepted +Date: 2026-08-22 +Area: ux +Scope: Applies to session-list reorder when the only regions in the move direction are collapsed projects. + +## Decision + +A reorder command must not refuse while rows the user can plainly see remain in the direction of travel. Skipping collapsed projects (see 0007) is an optimization over hidden *contents*, not a reason to treat a visible project row as the end of the list. When skipping would leave nowhere to land because every region beyond is collapsed, the session enters the nearest one and that project expands, so the session stays visible where the user dropped it. + +Reorder refuses only at a true boundary: the top of the first region, or the last position of the last region, where nothing at all lies beyond. + +## Reason + +Collapse state is asymmetric by construction. The ungrouped region always renders first and can never be collapsed, so moving up always has somewhere to land, while moving down needs an *expanded* region below. The steady state of a working fleet is one expanded project with the rest collapsed — which made the last session of the first project unable to move down at all, while nine project rows sat visibly beneath it. + +That asymmetry is invisible to the user. One direction of the same key worked and the other silently did nothing, which reads as a broken binding rather than a bounded list. + +Expanding the entered project is what keeps the visible model honest: a reorder that dropped the session into a collapsed project would appear to delete it from the list. + +## Consequences + +Reorder may change a session's project membership, and may change a project's collapse state. Both were already possible — entering an adjacent expanded project has always re-parented the session — so the rule is that a move which lands somewhere must leave that landing spot visible. + +A no-op result from reorder now carries real information: it means the true end of the list, and clients may say so plainly. + +Future region kinds must state whether they can be collapsed and where they render, since a region that is both collapsible and terminal would reintroduce the dead end. + +## Non-Goals + +This does not weaken 0007. Whenever any visible region exists beyond a collapsed one, the collapsed project is still jumped in a single step and its members and collapse state are left untouched. + +This says nothing about auto-collapsing a project the session leaves. + +## Examples + +Projects below the selection are all collapsed: moving the bottom session down enters the first of them, which expands; the rest keep the collapse state the user chose. + +A collapsed project sits between the session and an expanded one: moving down still jumps the collapsed project entirely and lands in the expanded one, leaving the skipped project collapsed. + +The last session of the last project moves down: nothing lies beyond, so the command reports that there is nothing to reorder past.