Panics · P03 Layout: panes and splits meet stale ids, lagging sizes and mid-drag changes without a panic

src/layout/grid.rs

@@ -22,7 +22,7 @@ fn shortest(heights: &[Pixels]) -> usize {
     heights
         .iter()
         .enumerate()
-        .min_by(|a, b| a.1.partial_cmp(b.1).expect("heights are finite"))
+        .min_by(|a, b| f32::from(*a.1).total_cmp(&f32::from(*b.1)))
         .map(|(ix, _)| ix)
         .expect("at least one column")
 }
@@ -182,6 +182,11 @@ mod tests {
 
     use super::{columns_for, fit, shortest};
 
+    #[test]
+    fn a_nan_height_never_stops_the_shortest_column() {
+        assert_eq!(shortest(&[px(f32::NAN), px(4.0), px(2.0)]), 2);
+    }
+
     #[test]
     fn what_fits_sits_centered() {
         assert_eq!(fit(px(100.0), px(3.0), px(2.0)), (20, px(1.0)));

src/layout/panes.rs

@@ -118,16 +118,23 @@ impl PaneLayout {
         true
     }
 
-    fn set_sizes(&mut self, path: &[usize], shares: &[f32]) {
+    /// Sets the split at `path`; false if the tree no longer has it.
+    fn set_sizes(&mut self, path: &[usize], shares: &[f32]) -> bool {
         let PaneLayout::Split {
             sizes, children, ..
         } = self
         else {
-            panic!("pane layout path points past a leaf");
+            return false;
         };
         match path.split_first() {
-            None => *sizes = shares.to_vec(),
-            Some((ix, rest)) => children[*ix].set_sizes(rest, shares),
+            None if shares.len() == sizes.len() => {
+                *sizes = shares.to_vec();
+                true
+            }
+            None => false,
+            Some((ix, rest)) => children
+                .get_mut(*ix)
+                .is_some_and(|child| child.set_sizes(rest, shares)),
         }
     }
 }
@@ -162,21 +169,30 @@ impl PaneGroup {
         out
     }
 
-    /// Opens a new pane beside `pane` and focuses it.
-    pub fn split(&mut self, pane: PaneId, axis: Axis, cx: &mut Context<Self>) -> PaneId {
+    /// Opens a new pane beside `pane` and focuses it; `None` if `pane` is gone.
+    pub fn split(&mut self, pane: PaneId, axis: Axis, cx: &mut Context<Self>) -> Option<PaneId> {
         let new = self.next;
-        assert!(self.root.split(pane, axis, new), "no pane {pane} to split");
+        if !self.root.split(pane, axis, new) {
+            log::error!("pane group: no pane {pane} to split");
+            return None;
+        }
         self.next = new.checked_add(1).expect("pane ids exhausted");
         self.focused = new;
         log::info!("pane group: split {pane} -> {new} ({axis:?})");
         cx.notify();
-        new
+        Some(new)
     }
 
-    /// Panics on the last pane; its close button is disabled.
+    /// Closes `pane`; the last pane, or one already gone, stays as it is.
     pub fn close(&mut self, pane: PaneId, cx: &mut Context<Self>) {
-        assert!(self.panes().len() > 1, "cannot close the last pane");
-        assert!(self.root.close(pane), "no pane {pane} to close");
+        if self.panes().len() < 2 {
+            log::error!("pane group: pane {pane} is the last; it stays");
+            return;
+        }
+        if !self.root.close(pane) {
+            log::error!("pane group: no pane {pane} to close");
+            return;
+        }
         if self.focused == pane {
             self.focused = self.panes()[0];
         }
@@ -185,6 +201,10 @@ impl PaneGroup {
     }
 
     pub fn focus(&mut self, pane: PaneId, cx: &mut Context<Self>) {
+        if !self.panes().contains(&pane) {
+            log::error!("pane group: no pane {pane} to focus");
+            return;
+        }
         self.focused = pane;
         cx.notify();
     }
@@ -225,7 +245,9 @@ impl PaneGroup {
                 let mut split = SplitPane::new(gpui::SharedString::from(key), *axis, min)
                     .sizes(sizes)
                     .on_resize(cx.listener(move |group: &mut Self, shares: &[f32], _, _| {
-                        group.root.set_sizes(&at, shares)
+                        if !group.root.set_sizes(&at, shares) {
+                            log::error!("pane group: no split at {at:?} for {shares:?}");
+                        }
                     }));
                 for (ix, child) in children.iter().enumerate() {
                     let mut child_path = path.clone();
@@ -412,6 +434,16 @@ mod tests {
         assert_eq!(sizes(&kept), [0.25, 0.75]);
     }
 
+    #[test]
+    fn a_stale_split_path_sets_nothing() {
+        let mut root = PaneLayout::Pane(1);
+        root.split(1, Axis::Horizontal, 2);
+        assert!(!root.set_sizes(&[0], &[0.5, 0.5]), "a leaf");
+        assert!(!root.set_sizes(&[4], &[0.5, 0.5]), "past the children");
+        assert!(!root.set_sizes(&[], &[1.0]), "a share too few");
+        assert!(root.set_sizes(&[], &[0.3, 0.7]));
+    }
+
     #[test]
     fn close_folds_single_children() {
         let mut root = PaneLayout::Pane(1);

src/layout/split.rs

@@ -137,20 +137,30 @@ impl RenderOnce for SplitPane {
     fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement {
         let count = self.panes.len();
         assert!(count >= 2, "split pane {:?} needs two panes", self.id);
-        let weights = self.sizes.unwrap_or_else(|| vec![1.0; count]);
-        assert_eq!(
-            weights.len(),
-            count,
-            "split pane {:?}: one size per pane",
-            self.id
-        );
-        let total = check_weights(&weights)
-            .unwrap_or_else(|error| panic!("split pane {:?}: {error:#}", self.id));
-        let normalized: Vec<f32> = weights.iter().map(|weight| weight / total).collect();
-        let fractions = use_seeded(self.id.clone(), normalized, window, cx);
+        let even = vec![1.0 / count as f32; count];
+        let normalized = match self.sizes.clone() {
+            None => even,
+            Some(weights) if weights.len() != count => {
+                log::error!(
+                    "split pane {:?}: {} sizes for {count} panes; even shares",
+                    self.id,
+                    weights.len()
+                );
+                even
+            }
+            Some(weights) => match check_weights(&weights) {
+                Ok(total) => weights.iter().map(|weight| weight / total).collect(),
+                Err(error) => {
+                    log::error!("split pane {:?}: {error:#}; even shares", self.id);
+                    even
+                }
+            },
+        };
+        let seed = (self.sizes, normalized);
+        let fractions = use_seeded(self.id.clone(), seed, window, cx);
         let owner = fractions.entity_id();
         let on_resize = self.on_resize;
-        let shares = fractions.read(cx).value.clone();
+        let shares = fractions.read(cx).value.1.clone();
         let (axis, min) = (self.axis, self.min);
         let mut root = div().id(self.id).size_full().flex().on_drag_move(
             move |event: &DragMoveEvent<DividerDrag>, window, cx| {
@@ -158,6 +168,13 @@ impl RenderOnce for SplitPane {
                 if drag.owner != owner {
                     return;
                 }
+                if drag.start.len() != count {
+                    log::error!(
+                        "split pane: a drag over {} panes ends at {count}",
+                        drag.start.len()
+                    );
+                    return;
+                }
                 let (along, extent) = match axis {
                     Axis::Horizontal => (event.event.position.x, event.bounds.size.width),
                     Axis::Vertical => (event.event.position.y, event.bounds.size.height),
@@ -179,7 +196,7 @@ impl RenderOnce for SplitPane {
                     report(&next, window, cx);
                 }
                 fractions.update(cx, |shares, cx| {
-                    shares.value = next;
+                    shares.value.1 = next;
                     cx.notify();
                 });
             },

src/layout/tests.rs

@@ -1,12 +1,12 @@
 use std::time::Duration;
 
 use gpui::{
-    AnyElement, App, Bounds, Context, InteractiveElement, IntoElement, Modifiers, MouseButton,
-    ParentElement, Pixels, Point, Render, Styled, TestAppContext, VisualTestContext, Window, div,
-    point, px,
+    AnyElement, App, AppContext, Axis, Bounds, Context, InteractiveElement, IntoElement, Modifiers,
+    MouseButton, ParentElement, Pixels, Point, Render, Styled, TestAppContext, VisualTestContext,
+    Window, div, point, px,
 };
 
-use super::Drawer;
+use super::{Drawer, PaneGroup, SplitPane};
 use crate::theme::Theme;
 
 type Build = Box<dyn Fn(&mut Window, &mut App) -> AnyElement>;
@@ -126,3 +126,131 @@ fn a_released_drawer_glides_back(cx: &mut TestAppContext) {
         "the panel glides back from {lift:?} below rest"
     );
 }
+
+#[gpui::test]
+fn a_pane_group_leaves_a_gone_or_last_pane_as_it_is(cx: &mut TestAppContext) {
+    let group = cx.new(|_| PaneGroup::new(|_, _, _| div().into_any_element()));
+    group.update(cx, |group, cx| {
+        assert_eq!(group.split(9, Axis::Horizontal, cx), None);
+        group.close(1, cx);
+        assert_eq!(group.panes(), [1], "the last pane stays");
+        let second = group.split(1, Axis::Horizontal, cx).expect("pane 1 splits");
+        group.close(second, cx);
+        group.close(second, cx);
+        group.focus(second, cx);
+        assert_eq!((group.panes(), group.focused()), (vec![1], 1));
+    });
+}
+
+/// Three panes given two sizes.
+struct Lagging;
+
+impl Render for Lagging {
+    fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        let pane = |name: &'static str| div().debug_selector(move || name.into()).size_full();
+        div().w(px(300.0)).h(px(100.0)).child(
+            SplitPane::new("lagging", Axis::Horizontal, px(10.0))
+                .sizes(&[1.0, 3.0])
+                .pane(pane("first"))
+                .pane(pane("second"))
+                .pane(pane("third")),
+        )
+    }
+}
+
+#[gpui::test]
+fn sizes_that_lag_their_panes_split_evenly(cx: &mut TestAppContext) {
+    cx.update(Theme::init);
+    let (_, cx) = cx.add_window_view(|_, _| Lagging);
+    cx.run_until_parked();
+    let widths =
+        ["first", "second", "third"].map(|name| cx.debug_bounds(name).expect(name).size.width);
+    assert_eq!(widths[0], widths[2]);
+    assert_eq!(widths[1], widths[2]);
+}
+
+/// Two panes that may grow a third, with sizes that may turn invalid.
+struct Changing {
+    grown: bool,
+    invalid: bool,
+}
+
+impl Render for Changing {
+    fn render(&mut self, _: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+        let pane = |name: &'static str| div().debug_selector(move || name.into()).size_full();
+        let sizes = if self.invalid { [0.0, 0.0] } else { [1.0, 1.0] };
+        let mut split = SplitPane::new("changing", Axis::Horizontal, px(10.0))
+            .sizes(&sizes)
+            .pane(pane("first"))
+            .pane(pane("second"));
+        if self.grown {
+            split = split.pane(pane("third"));
+        }
+        div().w(px(300.0)).h(px(100.0)).child(split)
+    }
+}
+
+fn drag_divider(cx: &mut VisualTestContext) -> Point<Pixels> {
+    cx.run_until_parked();
+    let first = cx.debug_bounds("first").expect("first");
+    let second = cx.debug_bounds("second").expect("second");
+    let grab = point((first.right() + second.left()) / 2.0, px(50.0));
+    cx.simulate_mouse_move(grab, None, Modifiers::none());
+    cx.simulate_mouse_down(grab, MouseButton::Left, Modifiers::none());
+    for step in [8.0, 8.0, 16.0] {
+        cx.simulate_mouse_move(
+            grab + point(px(step), px(0.0)),
+            MouseButton::Left,
+            Modifiers::none(),
+        );
+        cx.run_until_parked();
+    }
+    let moved = cx.debug_bounds("first").expect("first").size.width;
+    assert!(moved > first.size.width, "the drag moved the divider");
+    grab
+}
+
+#[gpui::test]
+fn a_pane_added_mid_drag_drops_the_drag(cx: &mut TestAppContext) {
+    cx.update(Theme::init);
+    let (host, cx) = cx.add_window_view(|_, _| Changing {
+        grown: false,
+        invalid: false,
+    });
+    let grab = drag_divider(cx);
+    host.update(cx, |host, cx| {
+        host.grown = true;
+        cx.notify();
+    });
+    cx.run_until_parked();
+    cx.simulate_mouse_move(
+        grab + point(px(24.0), px(0.0)),
+        MouseButton::Left,
+        Modifiers::none(),
+    );
+    cx.run_until_parked();
+    assert!(cx.debug_bounds("third").is_some());
+}
+
+#[gpui::test]
+fn invalid_sizes_after_a_drag_split_evenly(cx: &mut TestAppContext) {
+    cx.update(Theme::init);
+    let (host, cx) = cx.add_window_view(|_, _| Changing {
+        grown: false,
+        invalid: false,
+    });
+    let grab = drag_divider(cx);
+    cx.simulate_mouse_up(
+        grab + point(px(16.0), px(0.0)),
+        MouseButton::Left,
+        Modifiers::none(),
+    );
+    host.update(cx, |host, cx| {
+        host.invalid = true;
+        cx.notify();
+    });
+    cx.run_until_parked();
+    let first = cx.debug_bounds("first").expect("first").size.width;
+    let second = cx.debug_bounds("second").expect("second").size.width;
+    assert_eq!(first, second);
+}

tasks/panics.md

@@ -17,7 +17,7 @@ Each item reads every site in its folders, fixes what the rules name, updates ca
 - [x] P00 Policy in AGENTS.md
 - [x] P01 Primitives — `src/primitives`, `src/lib.rs`, `src/assets.rs`
 - [x] P02 Typography — `src/typography`
-- [ ] P03 Layout — `src/layout`
+- [x] P03 Layout — `src/layout`
 - [ ] P04 Window & Shell — `src/shell`
 - [ ] P05 Buttons & Actions — `src/buttons`
 - [ ] P06 Forms — `src/forms`
@@ -67,3 +67,4 @@ One line per item: what changed, what stayed and why, review rounds.
 - P00: AGENTS.md holds the three rules; this list follows the PRD's chapters. Review: 2 rounds (round 1 found the runtime clause missing).
 - P01: `init` returns `anyhow::Result`; a missing app icon logs once and shows `Icon::broken`. Kept: `layer.rs` expects (gpui's element order), `checked_ratio` (one value, the caller's). Review: 2 rounds.
 - P02: `Link::new` takes its handler; `system_zone` logs once and reads as jiff's unknown zone, which `datetime` marks UTC; an oversized highlight query logs and marks nothing; a cut line's paint fault logs. Kept: caller contracts (speed, digits, step, keystroke, heading level, pattern) and the exponent parse. Review: 3 rounds.
+- P03: `PaneGroup` leaves a gone or last pane and logs (`split` returns `Option`); a stale split path sets nothing; `SplitPane` splits evenly when sizes lag or fail, seeds by the owner's raw sizes, and drops a drag over another pane count; masonry sorts with `total_cmp`. Kept: dock ids (panels never leave), one-value contracts, pane-id overflow, workspace JSON. Review: 2 rounds.