fix(layout): return focus to the pane a split was opened from

[jondkinney]

Current behavior

Closing a focused pane moves focus to the next pane in tree order:

// src/layout.rs, close_focused
let pos = ids.iter().position(|id| *id == target).unwrap();
let new_focus = if pos + 1 < ids.len() { ids[pos + 1] } else { ids[pos - 1] };

pane_ids() is a depth-first walk of the BSP tree, so the pane that gets focus has no spatial or temporal relationship to where I was. When I open a split beside the pane I'm working in and then close it, focus lands on some unrelated pane rather than the one I opened it from.

Reproduction

1. Three or more panes in a tab
2. Focus a pane, open a split beside it
3. Close the split

Focus goes to whichever pane follows it in the tree, not the pane the split was opened from.

Expected

Focus returns to the pane it came from. herdr already models this elsewhere: it tracks the previous focus and exposes it as last_pane. The close path just doesn't consult it.

The change

TileLayout records the pane focus came from, and close_focused prefers it. It falls back to the existing tree-order behavior when there is no history, when the history names the pane being closed, or when it names a pane that has since gone away. The history lives in TileLayout rather than reusing AppState::previous_pane_focus, because that one can name a pane in another workspace or tab — closing a pane could then throw focus out of the current tab. A layout is per-tab, so that can't happen here.

A one-slot history is only sound if internal focus excursions never write it, and review (thanks, Greptile) showed the codebase had several focus-then-restore dances that would corrupt it: removing a background pane, rolling back a failed split, the targeted / unfocused workspace split, and the focus: false pane move. Rather than patch each site, those edits now go through target-taking primitives that don't move focus at all:

• close_pane(id) removes a background pane directly; detach_pane and take_pane_for_move stop focus-close-refocusing.
• split_pane(target, ...) splits without focusing. The runtime split path focuses the new pane only after the spawn succeeds, so a failed split is a pure rollback, and targeted / unfocused splits stop fabricating history.
• insert_pane_near takes the focus intent, so an unfocused pane move leaves the target tab's history alone.

The invariant is documented on the field: only a real focus move writes prev_focus. Fallout: the layout-level split_focused* helpers are now #[cfg(test)] (production splits all flow through Tab, which owns spawn-failure rollback), and three Tab split wrappers that lost their last caller are removed.

resize_pane swaps focus via direct field writes for its measurement and so never records history.

Tests

Eleven cases in src/layout.rs pin the behavior: returning to the previous pane and to the opener of a split; tree-order fallback with no history, spent history, or a removed remembered pane; and one per excursion — background close keeps the focused pane's history, a failed-split rollback preserves it, an unfocused insert and a targeted split_pane leave focus and history untouched, and resize does not disturb the target. The excursion tests are constructed so tree order and the expected pane differ, so each fails against the focus-dance code.

The existing api_pane_move_existing_tab_no_focus_preserves_previous_target_focus characterization test pins the API-visible behavior of the rewired move path.

cargo fmt --check clean, no compiler warnings in either build. Full suite locally: 2935 passing; the only failures are pre-existing environment-dependent git tests that fail identically on unmodified master on my machine.

On classification

I've filed this as a fix because the current target looks like a traversal artifact rather than an intended interaction — nothing picks that pane, it's just where the depth-first walk lands. If you read it as a behavior change instead, say so and I'll move it to a Discussion.

[coderabbitai]

📝 Walkthrough

Walkthrough

TileLayout now tracks previous focus and supports target-based splitting, focus-aware insertion, and direct pane removal. Tab, Workspace, and pane APIs pass target and focus state through these operations. Tests cover restoration, fallback, rollback, invalid targets, background removal, and resizing.

Changes

Pane focus and layout operations

Layer / File(s)	Summary
Track and restore layout focus src/layout.rs	TileLayout stores previous focus, supports target-based splitting and focus-aware insertion, and restores valid previous focus after pane closure. Tests cover history invalidation, fallback, consumption, background removal, and resizing.
Route target-aware pane creation src/workspace/tab.rs, src/workspace.rs	Shell and argv splits now accept a target pane and focus flag. Missing targets return an error. Spawn failures remove the new pane through close_pane.
Apply focus-aware pane moves src/app/api/panes.rs, src/workspace.rs, src/workspace/tab.rs	Pane moves pass focus directly to insertion. Pane moves and detachment close specified panes without temporary focus changes. Recovery tests cover invalid targets.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Workspace
  participant Tab
  participant TileLayout
  participant Runtime
  Workspace->>Tab: Request target-based pane split
  Tab->>TileLayout: Create pane near target
  Tab->>Runtime: Spawn shell or argv command
  Runtime-->>Tab: Return spawn result
  Tab->>TileLayout: Remove pane on spawn failure
  Tab->>TileLayout: Focus new pane when requested

Loading

Possibly related PRs

• herdrdev/herdr#2265: Builds on the same focus-history and target-aware pane APIs.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: restoring focus to the pane from which a split was opened.
Description check	✅ Passed	The description directly explains the focus-restoration change, its implementation, affected operations, and test coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The PR makes pane-close focus restoration layout-local and rewires temporary pane operations so they do not corrupt that history.

• Adds one-slot previous-focus tracking and prefers it when closing the focused pane.
• Introduces target-taking split and close operations that avoid temporary focus changes.
• Preserves focus history during failed split rollback, background pane removal, unfocused splits, and unfocused pane moves.
• Adds focused regression coverage for the corrected lifecycle paths.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the two previously reported focus-history corruption paths now use direct target-based mutations that preserve focus and history during background removal and failed split rollback.

Important Files Changed

Filename	Overview
src/layout.rs	Adds previous-focus tracking, direct target-based tree mutations, and regression tests covering close, split, insert, resize, and rollback behavior.
src/workspace/tab.rs	Routes split and removal operations through non-focusing layout primitives and applies focus only after successful runtime creation.
src/workspace.rs	Passes split targets and focus intent directly into Tab operations instead of performing temporary focus excursions.
src/app/api/panes.rs	Propagates pane-move focus intent into insertion so unfocused moves preserve the destination tab’s focus history.

_{Reviews (5): Last reviewed commit: "Merge branch 'master' into focus-on-clos..." | Re-trigger Greptile}

[jondkinney]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 38 minutes.

[jondkinney]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 15 minutes.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 75c49ebb-4e82-42aa-a3d0-60bd2c60fc00

📥 Commits

Reviewing files that changed from the base of the PR and between 93e0086 and 8bf2a5a.

📒 Files selected for processing (4)

• src/app/api/panes.rs
• src/layout.rs
• src/workspace.rs
• src/workspace/tab.rs

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle a stale focus without panicking.

Line 211 panics if focus is absent from the layout tree. Line 341 restores focus without validating that root contains it, so stale saved state can crash pane closing. Handle the missing position before calculating the fallback, or normalize focus in from_saved.

Proposed fix

-        let pos = ids.iter().position(|id| *id == target).unwrap();
+        let Some(pos) = ids.iter().position(|id| *id == target) else {
+            return false;
+        };

As per coding guidelines, “Do not use unwrap() in production Rust code.”

Also applies to: 338-343

Source: Coding guidelines