From 3fc9e2dcfe1b1abe2390fd9c8079b3b749d17149 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 16:36:32 +0000 Subject: [PATCH] refactor(pane_tree): avoid redundant clone during push_tab_to_leaf 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 | 19 +++++++++++++------ 3 files changed, 17 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..d27d81c9 --- /dev/null +++ b/.jules/bolt.md @@ -0,0 +1,3 @@ +## 2024-08-03 - [Avoid clone in pane_tree's push_tab_to_leaf] +**Learning:** In Rust performance optimization, avoid eagerly allocating cloned data (like `.clone()` on strings or identifiers) when passed into recursive search functions (e.g., `push_tab_to_leaf`). We can pass the value by ownership and return it back in the `Err` variant of a `Result` on a cache miss (e.g., `Result<(), SurfaceId>`). +**Action:** Use this pattern `Result<(), T>` instead of `bool` when transferring ownership into nested tree traversals. This allows the caller to reuse the same allocation for subsequent loop iterations without calling `.clone()`. 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..067e7fc0 100644 --- a/crates/forktty-core/src/model/pane_tree.rs +++ b/crates/forktty-core/src/model/pane_tree.rs @@ -647,19 +647,26 @@ 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 = new_tab_id; + for child in children.iter_mut() { + match push_tab_to_leaf(child, near_surface_id, current) { + Ok(()) => return Ok(()), + Err(returned) => current = returned, + } + } + Err(current) + } } }