Skip to content

Commit da7b1e0

Browse files
AgentEnderFrozenPandaz
AgentEnder
authored and
FrozenPandaz
committed
fix(core): hitting [1] or [2] should remove pinned panes if they match the current task (#34433)
## Current Behavior Pressing `[1]` or `[2]` on a task that's already pinned to that slot **focuses** the pane instead of unpinning it. This means there's no way to unpin a single pane via keyboard — you can only nuke everything with `[0]`. This regression was introduced in #34175, which fixed a real problem: pressing Enter on an already-pinned task would unpin it, leaving focus on an invisible pane (a ghost pane — you're staring at nothing but the TUI thinks you're looking at output). The fix was to make the "already pinned to this pane" branch focus instead of unpin. The problem is that `[1]`, `[2]`, and Enter all flowed through the same function (`assign_current_task_to_pane`), so changing behavior for Enter changed it for everyone. One lock got swapped out, and every door started behaving the same way. ## Expected Behavior `[1]` and `[2]` are **toggles**: press once to pin, press again to unpin. Enter is a **display** action: show me this task's output and put my cursor there — if it's already visible somewhere, just take me to it. After this change: - **`[1]` / `[2]`** on an already-pinned task → unpins it (pane disappears, layout adjusts, focus returns to task list if no panes remain) - **Enter** on an already-pinned task → focuses whichever pane it's in (even if it's in pane 2 and you'd normally expect pane 1) - **Init** (startup restore of pinned tasks) → pure assignment, no toggling, no focusing ## Approach The old code had one function trying to serve three masters. Rather than adding a flag parameter (`should_toggle: bool`) — which would just be a boolean that lies about its intentions at every call site — the function was split along the actual semantic boundaries: | Function | Used by | "Already pinned here" behavior | |---|---|---| | `toggle_current_task_in_pane` | `[1]` / `[2]` keys | **Unpin** (toggle off) | | `assign_current_task_to_pane` | `init()` | No-op (task is where it should be) | | `display_and_focus_current_task_in_terminal_pane` | Enter key | **Focus** the existing pane | The shared logic — exiting spacebar mode, moving a task between panes, fresh-pinning — lives in two small helpers (`exit_spacebar_and_pin`, `move_or_pin_selection`) that both `toggle` and `assign` delegate to. The only code that differs is the "what do we do when it's already here?" branch, which is exactly the part that *should* differ. **Why not keep one function with a mode parameter?** Because the three behaviors aren't variations of the same action — they're genuinely different user intents. A toggle is "I changed my mind." A focus is "Take me there." An assignment is "Put this here." Encoding that as an enum parameter just moves the branching somewhere less obvious and makes the call sites harder to read. The function names now document the intent at the point of use, and there's no shared state to accidentally couple. **Why does Enter check all panes, not just pane 0?** Because if you pinned a task to pane 2 via `[2]` and then press Enter on it, the least surprising thing is to jump to where it already lives — not to silently duplicate it into pane 1 or ignore you. The task is already on screen; Enter means "show me." ## Related Issue(s) Fixes the regression introduced by #34175. (cherry picked from commit c407de6)
1 parent 3c5965f commit da7b1e0

1 file changed

Lines changed: 105 additions & 67 deletions

File tree

  • packages/nx/src/native/tui

‎packages/nx/src/native/tui/app.rs‎

Lines changed: 105 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -990,12 +990,12 @@ impl App {
990990
);
991991
}
992992
'1' => {
993-
self.assign_current_task_to_pane(0);
993+
self.toggle_current_task_in_pane(0);
994994
let _ = self.handle_pty_resize();
995995
// No need to debounce
996996
}
997997
'2' => {
998-
self.assign_current_task_to_pane(1);
998+
self.toggle_current_task_in_pane(1);
999999
let _ = self.handle_pty_resize();
10001000
// No need to debounce
10011001
}
@@ -1670,83 +1670,112 @@ impl App {
16701670
let _ = self.handle_pty_resize();
16711671
}
16721672

1673+
/// Toggle pin state: if the selected task is already in this pane, unpin it;
1674+
/// otherwise pin it (moving from the other pane if needed). Used by [1]/[2] keys.
1675+
fn toggle_current_task_in_pane(&mut self, pane_idx: usize) -> Option<()> {
1676+
let selection = self.selection_manager.lock().get_selection().cloned()?;
1677+
1678+
if self.spacebar_mode {
1679+
self.exit_spacebar_and_pin(selection, pane_idx);
1680+
} else if self.pane_tasks[pane_idx].as_ref() == Some(&selection) {
1681+
// Already pinned to this pane — unpin it
1682+
self.pane_tasks[pane_idx] = None;
1683+
self.clear_pane_pty_reference(pane_idx);
1684+
self.dependency_view_states[pane_idx] = None;
1685+
1686+
let pinned_count = self.pane_tasks.iter().filter(|t| t.is_some()).count();
1687+
match pinned_count {
1688+
0 => {
1689+
self.update_focus(Focus::TaskList);
1690+
self.set_spacebar_mode(false, Some(SelectionMode::TrackByPosition));
1691+
self.layout_manager
1692+
.set_pane_arrangement(PaneArrangement::None);
1693+
}
1694+
1 => self
1695+
.layout_manager
1696+
.set_pane_arrangement(PaneArrangement::Single),
1697+
_ => self
1698+
.layout_manager
1699+
.set_pane_arrangement(PaneArrangement::Double),
1700+
}
1701+
1702+
self.dispatch_action(Action::UnpinSelection(pane_idx));
1703+
} else {
1704+
self.move_or_pin_selection(selection, pane_idx);
1705+
}
1706+
1707+
self.finalize_pane_assignment();
1708+
Some(())
1709+
}
1710+
1711+
/// Pin the selected task to a pane. If the task is already pinned to the other
1712+
/// pane, it moves it. Never unpins, never focuses. Used by init and as a
1713+
/// building block for display_and_focus.
16731714
fn assign_current_task_to_pane(&mut self, pane_idx: usize) -> Option<()> {
1674-
// Extract selected item (task or batch) to end the immutable borrow
16751715
let selection = self.selection_manager.lock().get_selection().cloned()?;
16761716

1677-
// If we're in spacebar mode, clear the spacebar placeholder before pinning
1678-
// In spacebar mode, pane_tasks[0] is a placeholder that may hold a stale task
16791717
if self.spacebar_mode {
1680-
// Clear the spacebar placeholder in pane 0
1681-
self.pane_tasks[0] = None;
1682-
self.clear_pane_pty_reference(0);
1683-
self.dependency_view_states[0] = None;
1718+
self.exit_spacebar_and_pin(selection, pane_idx);
1719+
} else {
1720+
self.move_or_pin_selection(selection, pane_idx);
1721+
}
16841722

1685-
// Exit spacebar mode first
1686-
self.set_spacebar_mode(false, Some(SelectionMode::TrackByName));
1723+
self.finalize_pane_assignment();
1724+
Some(())
1725+
}
1726+
1727+
/// Shared helper: exit spacebar mode and pin the selection to the target pane.
1728+
fn exit_spacebar_and_pin(&mut self, selection: SelectionEntry, pane_idx: usize) {
1729+
self.pane_tasks[0] = None;
1730+
self.clear_pane_pty_reference(0);
1731+
self.dependency_view_states[0] = None;
1732+
1733+
self.set_spacebar_mode(false, Some(SelectionMode::TrackByName));
1734+
1735+
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1736+
self.pane_tasks[pane_idx] = Some(selection);
1737+
self.layout_manager
1738+
.set_pane_arrangement(PaneArrangement::Single);
1739+
}
1740+
1741+
/// Shared helper: move a selection from the other pane or pin a fresh selection.
1742+
fn move_or_pin_selection(&mut self, selection: SelectionEntry, pane_idx: usize) {
1743+
let other_pane_idx = 1 - pane_idx;
1744+
if self.pane_tasks[other_pane_idx].as_ref() == Some(&selection) {
1745+
// Moving from the other pane
1746+
self.pane_tasks[other_pane_idx] = None;
1747+
self.clear_pane_pty_reference(other_pane_idx);
1748+
self.dependency_view_states[other_pane_idx] = None;
1749+
self.dispatch_action(Action::UnpinSelection(other_pane_idx));
16871750

1688-
// Dispatch action before moving selection
1689-
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1690-
self.pane_tasks[pane_idx] = Some(selection);
16911751
self.layout_manager
16921752
.set_pane_arrangement(PaneArrangement::Single);
1753+
1754+
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1755+
self.pane_tasks[pane_idx] = Some(selection);
16931756
} else {
1694-
// Check if the item is already pinned to the OTHER pane
1695-
let other_pane_idx = 1 - pane_idx;
1696-
if self.pane_tasks[other_pane_idx].as_ref() == Some(&selection) {
1697-
// Clear the other pane - item is "moving" to the new pane
1698-
self.pane_tasks[other_pane_idx] = None;
1699-
self.clear_pane_pty_reference(other_pane_idx);
1700-
self.dependency_view_states[other_pane_idx] = None;
1701-
self.dispatch_action(Action::UnpinSelection(other_pane_idx));
1702-
1703-
// Adjust layout since we now only have one pane
1704-
self.layout_manager
1705-
.set_pane_arrangement(PaneArrangement::Single);
1757+
// Fresh pin
1758+
self.cleanup_pane_completed_batch(pane_idx);
17061759

1707-
// Dispatch action before moving selection, then assign
1708-
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1709-
self.pane_tasks[pane_idx] = Some(selection);
1710-
} else if self.pane_tasks[pane_idx].as_ref() == Some(&selection) {
1711-
// Task is already pinned to this pane - just focus it
1712-
self.update_focus(Focus::MultipleOutput(pane_idx));
1713-
return None; // Early return - no need to update layout or resize
1714-
} else {
1715-
// Pin the item to the specified pane
1716-
// Cleanup old completed batch before replacing
1717-
self.cleanup_pane_completed_batch(pane_idx);
1718-
1719-
// Dispatch action before moving selection
1720-
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1721-
self.pane_tasks[pane_idx] = Some(selection);
1722-
self.update_focus(Focus::TaskList);
1723-
1724-
// Exit spacebar mode when pinning
1725-
// When pinning an item, use name-based selection
1726-
self.set_spacebar_mode(false, Some(SelectionMode::TrackByName));
1727-
1728-
// Set pane arrangement based on count of pinned tasks
1729-
let pinned_count = self.pane_tasks.iter().filter(|t| t.is_some()).count();
1730-
match pinned_count {
1731-
0 => self
1732-
.layout_manager
1733-
.set_pane_arrangement(PaneArrangement::None),
1734-
1 => self
1735-
.layout_manager
1736-
.set_pane_arrangement(PaneArrangement::Single),
1737-
_ => self
1738-
.layout_manager
1739-
.set_pane_arrangement(PaneArrangement::Double),
1740-
}
1760+
self.dispatch_action(Action::PinSelection(selection.clone(), pane_idx));
1761+
self.pane_tasks[pane_idx] = Some(selection);
1762+
self.update_focus(Focus::TaskList);
17411763

1742-
// Already assigned above, so just continue to layout update
1743-
self.finalize_pane_assignment();
1744-
return Some(());
1764+
self.set_spacebar_mode(false, Some(SelectionMode::TrackByName));
1765+
1766+
let pinned_count = self.pane_tasks.iter().filter(|t| t.is_some()).count();
1767+
match pinned_count {
1768+
0 => self
1769+
.layout_manager
1770+
.set_pane_arrangement(PaneArrangement::None),
1771+
1 => self
1772+
.layout_manager
1773+
.set_pane_arrangement(PaneArrangement::Single),
1774+
_ => self
1775+
.layout_manager
1776+
.set_pane_arrangement(PaneArrangement::Double),
17451777
}
17461778
}
1747-
1748-
self.finalize_pane_assignment();
1749-
Some(())
17501779
}
17511780

17521781
fn finalize_pane_assignment(&mut self) {
@@ -1911,9 +1940,18 @@ impl App {
19111940
if force_spacebar_mode {
19121941
self.toggle_output_visibility();
19131942
} else {
1943+
// If already pinned to a pane, just focus it rather than reassigning
1944+
let selection = self.selection_manager.lock().get_selection().cloned();
1945+
if let Some(ref sel) = selection {
1946+
if let Some(pane_idx) = self.pane_tasks.iter().position(|t| t.as_ref() == Some(sel))
1947+
{
1948+
self.update_focus(Focus::MultipleOutput(pane_idx));
1949+
return;
1950+
}
1951+
}
19141952
self.assign_current_task_to_pane(0);
19151953
}
1916-
// Unlike in standard spacebar mode, also set focus to the pane, if applicable (if the user pressed enter a second time, there will be no visible panes)
1954+
// Focus the pane if one is now visible
19171955
if self.has_visible_panes() {
19181956
self.update_focus(Focus::MultipleOutput(0));
19191957
}

0 commit comments

Comments
 (0)