fix(layout): keep focus history for directional pane navigation - #2265
fix(layout): keep focus history for directional pane navigation#2265haphamdev wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthrough
ChangesPane navigation and focus lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Navigation
participant TileLayout
participant find_in_direction
Navigation->>TileLayout: read focus_history()
Navigation->>find_in_direction: pass focused pane, direction, panes, and FocusHistory
find_in_direction->>find_in_direction: filter adjacent candidates and rank by MRU or geometry
find_in_direction-->>Navigation: return target pane
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Directional Pane Navigation with Focus HistoryFind the next pane to focus when moving Assumptions
Core ideaDirection navigation is two decisions:
The adjacency filter is what makes Examples 2 and 3 work; the MRU pick is what
Then: Data model & layoutAlgorithmComplexity: How each step works
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac961af3-8d8f-4da9-b75e-647321d9ade3
📒 Files selected for processing (5)
src/app/actions.rssrc/app/api/panes.rssrc/app/input/navigate.rssrc/layout.rssrc/workspace/tab.rs
Greptile SummaryThe PR adds per-layout focus history so directional pane navigation can return to the most recently focused adjacent pane while preserving geometry-only behavior for swaps.
Confidence Score: 5/5The PR appears safe to merge within the scope of this follow-up review. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/layout.rs | Adds focus-history storage and integrates it into directional target selection and pane lifecycle operations. |
| src/workspace/tab.rs | Routes targeted splits, pane insertion, removal, and spawn rollback through the new focus-aware layout primitives. |
| src/workspace.rs | Passes explicit split targets and focus intent through workspace-level pane creation and move operations. |
| src/app/api/panes.rs | Applies history-aware navigation to pane APIs while retaining geometry-only directional swaps. |
| src/app/input/navigate.rs | Uses active-tab focus history for navigation and an empty history for swaps. |
| src/app/actions.rs | Updates action-driven directional focus to consult the active layout history while leaving swaps geometric. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
FocusChange[Pane focus changes] --> History[TileLayout FocusHistory]
Navigate[Directional navigation] --> Candidates[Nearest row or column candidates]
History --> Candidates
Candidates --> MRU[Most recently focused candidate]
Swap[Directional swap] --> Geometry[Geometry-only selection]
Reviews (5): Last reviewed commit: "fix(layout): don't record focus history ..." | Re-trigger Greptile
c8394f9 to
17616d6
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57dbf564-61df-463d-9e8e-5f37c3426568
📒 Files selected for processing (5)
src/app/actions.rssrc/app/api/panes.rssrc/app/input/navigate.rssrc/layout.rssrc/workspace/tab.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/layout.rs (1)
163-165: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not record temporary focus during a no-focus split.
With
PaneSplitParams.focus == false,src/workspace.rsLine 862 temporarily callsfocus_pane(pane_id), this split recordsnew_id, andsrc/workspace.rsLine 919 restores the previous pane throughfocus_pane. These transitions are not visible focus changes.Both panes receive MRU stamps. Directional navigation can then select the new pane although the request did not focus it.
Thread the focus intent through the split path. Record history only after a real focus change. Add a regression test for
pane.splitwithfocus: false.Also applies to: 236-239
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f0d348b-aca0-4031-838a-3af0f0f6e272
📒 Files selected for processing (4)
src/app/api/panes.rssrc/layout.rssrc/workspace.rssrc/workspace/tab.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/workspace/tab.rs
Directional focus (focus_pane_*/navigate_pane_* keybinds, navigate-mode arrows, and the pane.focus_direction API) was stateless: find_in_direction re-derived the nearest pane purely from geometry, so leaving a split subtree and returning snapped to the same geometric winner instead of the pane last used there. Add a per-pane monotonic FocusHistory on TileLayout, recorded on every real focus change, and make find_in_direction prefer the most-recently-focused pane among the nearest-column/row candidates, falling back to the existing geometric tiebreak when none has history. Focus navigation and the pane.neighbor query use live history; directional swaps stay geometry-only, matching the TUI swap path, via a dedicated directional_swap_target helper that passes an empty history so swap targets never depend on unrelated focus movements. Route pane removal through a new TileLayout::close_pane primitive that records a new focus only when the removed pane was actually focused. This replaces the focus_pane/close_focused/focus_pane restore dance in detach_pane/take_pane_for_move and the split rollback path, which otherwise stamped a bystander pane as most-recently-focused and corrupted the MRU memory whenever a background pane was closed or died while focus was elsewhere.
insert_pane_near unconditionally stamped the moved pane as focused, so a pane relocated via pane.move with focus:false became eligible for MRU directional navigation despite never being visibly focused. The move path restored the prior focus afterward, leaving a phantom history stamp that could hijack directional focus/neighbor selection toward the moved pane. Thread the focus intent through insert_pane_near / insert_existing_pane / insert_moved_pane_into_tab so the moved pane is focused and recorded only when the move requests focus; a focus:false move leaves focus and history untouched. This also removes the focus/restore dance in handle_pane_move.
pane.split with focus:false temporarily focused the split target and the new pane, then restored focus, leaving phantom MRU stamps on both. A focus:true split of a non-focused pane likewise stamped the target it was never resting on. Directional navigation could then jump to a pane the split never focused. Add TileLayout::split_pane(target, direction, ratio, focus) that focuses and records the new pane only when focus is requested, and thread the focus intent through split_focused_with_runtime and the workspace split path, dropping the focus_pane targeting/restore dance. The focused-split convenience wrappers become test-only.
039b3f2 to
8092bb0
Compare
Fixed |
Directional focus (focus_pane_/navigate_pane_ keybinds, navigate-mode arrows, and the pane.focus_direction API) was stateless: find_in_direction re-derived the nearest pane purely from geometry, so leaving a split subtree and returning snapped to the same geometric winner instead of the pane last used there.
Add a per-pane monotonic FocusHistory on TileLayout, recorded on every real focus change, and make find_in_direction prefer the most-recently-focused pane among the nearest-column/row candidates, falling back to the existing geometric tiebreak when none has history. Swap-in-direction and the pure-geometry tests pass an empty history, preserving current behavior.
Route pane removal through a new TileLayout::close_pane primitive that records a new focus only when the removed pane was actually focused. This replaces the focus_pane/close_focused/focus_pane restore dance in detach_pane/take_pane_for_move and the split rollback path, which otherwise stamped a bystander pane as most-recently-focused and corrupted the MRU memory whenever a background pane was closed or died while focus was elsewhere.