From 3c3b90ec643518274859f54652d35029f7aa6b5f Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:45:58 +0000 Subject: [PATCH] Optimize push_tab_to_leaf to avoid clone Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com> --- .jules/bolt.md | 3 +++ crates/forktty-core/src/model.rs | 2 +- crates/forktty-core/src/model/pane_tree.rs | 24 ++++++++++++++++------ 3 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 .jules/bolt.md diff --git a/.jules/bolt.md b/.jules/bolt.md new file mode 100644 index 00000000..d1549eed --- /dev/null +++ b/.jules/bolt.md @@ -0,0 +1,3 @@ +## 2026-07-28 - Avoid eager cloning in recursive search +**Learning:** In Rust, to avoid redundant allocations when passing owned values (like `String` or large aliases like `SurfaceId`) into recursive search functions (e.g., tree traversals) that might fail to consume them, pass the value by ownership and return it back in the `Err` variant of a `Result` on a cache miss (e.g., `Result<(), T>`). This allows the caller to reuse the same allocation for subsequent loop iterations without calling `.clone()`. +**Action:** Use this pattern to eliminate `.clone()` calls inside `.any()` closures during tree traversals. diff --git a/crates/forktty-core/src/model.rs b/crates/forktty-core/src/model.rs index 3ba52a03..0a0fc3d9 100644 --- a/crates/forktty-core/src/model.rs +++ b/crates/forktty-core/src/model.rs @@ -1307,7 +1307,7 @@ impl WorkspaceModel { .workspaces .get_mut(&workspace_id) .expect("workspace verified above"); - if !push_tab_to_leaf(&mut workspace.pane_tree, near_surface_id, new_id.clone()) { + if push_tab_to_leaf(&mut workspace.pane_tree, near_surface_id, new_id.clone()).is_err() { return None; } workspace.focused_surface_id = new_id.clone(); diff --git a/crates/forktty-core/src/model/pane_tree.rs b/crates/forktty-core/src/model/pane_tree.rs index 2a3f7fc5..1bf611b6 100644 --- a/crates/forktty-core/src/model/pane_tree.rs +++ b/crates/forktty-core/src/model/pane_tree.rs @@ -643,23 +643,35 @@ pub(super) fn set_leaf_active_for_surface(node: &mut PaneNode, surface_id: &str) /// Push `new_tab_id` to the tabs of the leaf containing `near_surface_id`. /// Sets `active` to the new tab's index. Returns `true` if found. +/// Push `new_tab_id` to the tabs of the leaf containing `near_surface_id`. +/// Sets `active` to the new tab's index. +/// +/// Returns `Ok(())` if the insertion succeeded, or `Err(new_tab_id)` if the +/// leaf wasn't found in this branch, threading ownership back to avoid `clone()`. pub(super) fn push_tab_to_leaf( node: &mut PaneNode, near_surface_id: &str, new_tab_id: SurfaceId, -) -> bool { +) -> Result<(), SurfaceId> { match node { PaneNode::Leaf { tabs, active } => { if tabs.iter().any(|id| id == near_surface_id) { tabs.push(new_tab_id); *active = tabs.len() - 1; - true + Ok(()) } else { - false + Err(new_tab_id) } } - PaneNode::Split { children, .. } => children - .iter_mut() - .any(|child| push_tab_to_leaf(child, near_surface_id, new_tab_id.clone())), + PaneNode::Split { children, .. } => { + let mut current_id = new_tab_id; + for child in children { + match push_tab_to_leaf(child, near_surface_id, current_id) { + Ok(()) => return Ok(()), + Err(id) => current_id = id, + } + } + Err(current_id) + } } }