diff --git a/cmux-tui/crates/cmux-tui-core/src/mux/layout_sizes.rs b/cmux-tui/crates/cmux-tui-core/src/mux/layout_sizes.rs index e01695be780b..21535fa53e5d 100644 --- a/cmux-tui/crates/cmux-tui-core/src/mux/layout_sizes.rs +++ b/cmux-tui/crates/cmux-tui-core/src/mux/layout_sizes.rs @@ -32,6 +32,9 @@ pub enum LayoutSizesError { Width { index: usize, error: ViewportWidthError }, /// The value at `index` is on another screen than the first value. OtherScreen { index: usize }, + /// The ratio at `index` is not a number (in-process callers; JSON cannot carry one). An infinite ratio is not + /// refused: it clamps, as `set-split-ratio` clamps it. + NanRatio { index: usize }, /// The commit failed (the store refused it); nothing changed. NotPersisted { reason: String }, } @@ -39,6 +42,7 @@ pub enum LayoutSizesError { impl LayoutSizesError { pub const BAD_COUNT_CODE: &'static str = "invalid-argument"; pub const OTHER_SCREEN_CODE: &'static str = "layout-sizes-other-screen"; + pub const NAN_RATIO_CODE: &'static str = "invalid-argument"; pub const NOT_PERSISTED_CODE: &'static str = "layout-sizes-not-persisted"; /// The single command's error code for a refused value, else the batch's own. @@ -48,6 +52,7 @@ impl LayoutSizesError { Self::Ratio { error, .. } => error.code(), Self::Width { error, .. } => error.code(), Self::OtherScreen { .. } => Self::OTHER_SCREEN_CODE, + Self::NanRatio { .. } => Self::NAN_RATIO_CODE, Self::NotPersisted { .. } => Self::NOT_PERSISTED_CODE, } } @@ -56,9 +61,10 @@ impl LayoutSizesError { pub fn index(&self) -> Option { match self { Self::BadCount { .. } | Self::NotPersisted { .. } => None, - Self::Ratio { index, .. } | Self::Width { index, .. } | Self::OtherScreen { index } => { - Some(*index) - } + Self::Ratio { index, .. } + | Self::Width { index, .. } + | Self::OtherScreen { index } + | Self::NanRatio { index } => Some(*index), } } } @@ -75,6 +81,9 @@ impl fmt::Display for LayoutSizesError { Self::OtherScreen { index } => { write!(formatter, "value {index} is on another screen than value 0") } + Self::NanRatio { index } => { + write!(formatter, "value {index}: the split ratio is not a number") + } Self::NotPersisted { reason } => { write!(formatter, "could not persist layout sizes: {reason}") } diff --git a/cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/layout_ops.rs b/cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/layout_ops.rs index fde758a4ab00..70583db26e9a 100644 --- a/cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/layout_ops.rs +++ b/cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/layout_ops.rs @@ -436,6 +436,11 @@ fn apply_layout_size_value( match value { LayoutSizeValue::SplitRatio { split, ratio } => { let refused = |error| LayoutSizesError::Ratio { index, error }; + // The clamp keeps a NaN (in-process callers; JSON cannot carry one), and the store cannot persist one. + // An infinite ratio (JSON `1e39` as f32) clamps below, as `set-split-ratio` clamps it. + if ratio.is_nan() { + return Err(LayoutSizesError::NanRatio { index }); + } let ratio = clamp_split_ratio(ratio); let Some((split_workspace, split_screen, owner)) = state.split_screens.get(&split).copied() diff --git a/cmux-tui/crates/cmux-tui-core/src/mux/tests/layout_sizes.rs b/cmux-tui/crates/cmux-tui-core/src/mux/tests/layout_sizes.rs index 782e995d71c7..ca9eb9e37d41 100644 --- a/cmux-tui/crates/cmux-tui-core/src/mux/tests/layout_sizes.rs +++ b/cmux-tui/crates/cmux-tui-core/src/mux/tests/layout_sizes.rs @@ -127,3 +127,186 @@ fn an_empty_or_oversized_batch_is_refused() { } assert_eq!(widths_revision_and_undo(&mux), before); } + +/// Three columns (the first pane and two panes right of it), the panes left to right. +fn three_columns(mux: &Arc) -> [PaneId; 3] { + let (first_pane, second_pane) = two_columns(mux); + let third = mux.new_pane_right(second_pane, 0.5, Some((18, 22))).unwrap(); + let third_pane = mux.with_state(|state| state.pane_of(third.id).unwrap()); + [first_pane, second_pane, third_pane] +} + +/// The split id of the column at `index` on the first screen. +fn column_split(mux: &Arc, index: usize) -> SplitId { + mux.with_state(|state| state.workspaces[0].screens[0].layout_columns[index].id) +} + +/// #34928 review C: a ratio value sets its column's width from the widths the same batch set before it (a column +/// split's ratio is the share of the columns left of it), and the batch is still one change and one undo step. +#[test] +fn a_ratio_value_uses_the_widths_the_batch_set_before_it() { + let mux = test_mux(); + let [first_pane, _, _] = three_columns(&mux); + let third_split = column_split(&mux, 2); + let (_, revision, undo) = widths_revision_and_undo(&mux); + + mux.set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[ + LayoutSizeValue::PaneWidth { pane: first_pane, width: 0.4 }, + LayoutSizeValue::SplitRatio { split: third_split, ratio: 0.5 }, + ], + 7, + 15, + ) + .unwrap(); + let (widths, after_revision, after_undo) = widths_revision_and_undo(&mux); + let left_of_third = widths[0] + widths[1]; + assert!( + (widths[2] - left_of_third).abs() < 1e-5, + "a ratio of 0.5 must make the third column as wide as the two left of it as the batch set them: {widths:?}" + ); + assert!((widths[0] - 0.4).abs() < 1e-6, "the batch's first width: {widths:?}"); + assert_eq!(after_revision, revision + 1, "the batch made more than one layout change"); + assert_eq!(after_undo, undo + 1, "the batch is not one undo step"); +} + +/// #34928 review B: a NaN ratio is refused in-process (the clamp keeps a NaN, and the store cannot persist one), and +/// nothing changes. +#[test] +fn a_nan_ratio_is_refused_and_changes_nothing() { + let mux = test_mux(); + let [first_pane, _, _] = three_columns(&mux); + let third_split = column_split(&mux, 2); + let before = widths_revision_and_undo(&mux); + let error = mux + .set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[ + LayoutSizeValue::PaneWidth { pane: first_pane, width: 0.4 }, + LayoutSizeValue::SplitRatio { split: third_split, ratio: f32::NAN }, + ], + 7, + 16, + ) + .expect_err("a NaN ratio was taken"); + assert!(matches!(error, LayoutSizesError::NanRatio { index: 1 }), "{error}"); + assert_eq!(error.code(), "invalid-argument"); + assert_eq!(widths_revision_and_undo(&mux), before, "a refused batch changed the layout"); +} + +/// #34928 review C: values on two screens are refused whole, with the index of the first value on another screen. +#[test] +fn values_on_two_screens_are_refused_whole() { + let mux = test_mux(); + let (first_pane, _) = two_columns(&mux); + let second = mux.new_screen(None, Some((80, 22))).unwrap(); + let second_pane = mux.with_state(|state| state.pane_of(second.id).unwrap()); + mux.new_pane_right(second_pane, 0.5, Some((38, 22))).unwrap(); + let before = widths_revision_and_undo(&mux); + + let error = mux + .set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[ + LayoutSizeValue::PaneWidth { pane: first_pane, width: 0.4 }, + LayoutSizeValue::PaneWidth { pane: second_pane, width: 0.4 }, + ], + 7, + 17, + ) + .unwrap_err(); + assert!(matches!(error, LayoutSizesError::OtherScreen { index: 1 }), "{error}"); + assert_eq!(error.code(), "layout-sizes-other-screen"); + assert_eq!(widths_revision_and_undo(&mux), before, "a refused batch changed the layout"); +} + +/// #34928 review C: a batch that changes nothing commits nothing: no layout change, no undo step, no event. +#[test] +fn a_batch_of_the_current_sizes_commits_nothing() { + let mux = test_mux(); + let (first_pane, right_pane) = two_columns(&mux); + let before = widths_revision_and_undo(&mux); + let values: Vec<_> = [first_pane, right_pane] + .iter() + .zip(&before.0) + .map(|(pane, width)| LayoutSizeValue::PaneWidth { pane: *pane, width: *width }) + .collect(); + let events = mux.subscribe(); + + mux.set_layout_sizes_in_transaction_checked_as(&Actor::Daemon, &values, 7, 18).unwrap(); + assert_eq!( + widths_revision_and_undo(&mux), + before, + "a batch of the current sizes changed the layout" + ); + while let Ok(event) = events.try_recv() { + assert!( + !matches!(event, MuxEvent::LayoutChanged(..)), + "a batch that changed nothing sent a layout-changed event" + ); + } +} + +/// #34928 review C: a batch that changes the layout sends exactly one layout-changed event. +#[test] +fn a_batch_sends_one_layout_changed_event() { + let mux = test_mux(); + let (first_pane, right_pane) = two_columns(&mux); + let events = mux.subscribe(); + + mux.set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[ + LayoutSizeValue::PaneWidth { pane: first_pane, width: 0.4 }, + LayoutSizeValue::PaneWidth { pane: right_pane, width: 0.6 }, + ], + 7, + 19, + ) + .unwrap(); + let mut transactions = Vec::new(); + while let Ok(event) = events.try_recv() { + if let MuxEvent::LayoutChanged(_, delta) = event { + transactions.push(delta.and_then(|delta| delta.transaction.clone())); + } + } + assert_eq!( + transactions, + vec![Some(Arc::::from("19"))], + "a batch of two values must send one layout-changed event that echoes its transaction" + ); +} + +/// #34928 review B, on the path it named: a tree split (no viewport column, so no implied width to refuse it) takes +/// the ratio as it is. A NaN ratio is refused there too; an infinite one (JSON `1e39` read as f32) clamps to 0.95, as +/// `set-split-ratio` clamps it (#35261 review). +#[test] +fn a_nan_ratio_on_a_tree_split_is_refused_and_an_infinite_one_clamps() { + let mux = test_mux(); + let (_, _, _, root_split, _) = seed_split_ratio_tree(&mux); + let root_ratio = |mux: &Arc| { + mux.with_state(|state| state.workspaces[0].screens[0].root.split_ratio(root_split)) + }; + let before = root_ratio(&mux); + assert!(before.is_some(), "the seed's root is a split"); + let error = mux + .set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[LayoutSizeValue::SplitRatio { split: root_split, ratio: f32::NAN }], + 7, + 20, + ) + .expect_err("a NaN ratio on a tree split was taken"); + assert!(matches!(error, LayoutSizesError::NanRatio { index: 0 }), "{error}"); + assert_eq!(root_ratio(&mux), before, "a refused ratio changed the split"); + + mux.set_layout_sizes_in_transaction_checked_as( + &Actor::Daemon, + &[LayoutSizeValue::SplitRatio { split: root_split, ratio: f32::INFINITY }], + 7, + 21, + ) + .expect("an infinite ratio is clamped, as set-split-ratio clamps it"); + assert_eq!(root_ratio(&mux), Some(0.95), "an infinite ratio did not clamp to 0.95"); +} diff --git a/cmux-tui/crates/cmux-tui-core/src/server.rs b/cmux-tui/crates/cmux-tui-core/src/server.rs index 0546e8ac1f78..194561db7c6c 100644 --- a/cmux-tui/crates/cmux-tui-core/src/server.rs +++ b/cmux-tui/crates/cmux-tui-core/src/server.rs @@ -317,6 +317,7 @@ mod url_open; #[cfg(test)] use capabilities::advertised_capabilities; use capabilities::identify_capabilities; +use cmd_panes::LayoutSizeWire; /// Maximum JSON payload accepted on the Unix JSON-lines control socket. const MAX_JSON_LINE_BYTES: usize = crate::REMOTE_CLIENT_MESSAGE_MAX_BYTES; const WORKSPACE_REGISTRY_CAPABILITY: &str = "workspace-registry-v1"; @@ -2404,23 +2405,6 @@ impl Command { } } -/// One value of `set-layout-sizes`: what `set-split-ratio` or `set-viewport-pane-width` sets. -#[derive(Debug, Clone, Copy, Deserialize)] -#[serde(untagged)] -enum LayoutSizeWire { - SplitRatio { split: SplitId, ratio: f32 }, - PaneWidth { pane: PaneId, width: f32 }, -} - -impl From for crate::mux::LayoutSizeValue { - fn from(value: LayoutSizeWire) -> Self { - match value { - LayoutSizeWire::SplitRatio { split, ratio } => Self::SplitRatio { split, ratio }, - LayoutSizeWire::PaneWidth { pane, width } => Self::PaneWidth { pane, width }, - } - } -} - /// A tab named by its numeric surface id or its public `tab_...` id. #[derive(Debug, Clone, Deserialize)] #[serde(untagged)] diff --git a/cmux-tui/crates/cmux-tui-core/src/server/cmd_panes.rs b/cmux-tui/crates/cmux-tui-core/src/server/cmd_panes.rs index 01b13e6ccf24..aadf9144824e 100644 --- a/cmux-tui/crates/cmux-tui-core/src/server/cmd_panes.rs +++ b/cmux-tui/crates/cmux-tui-core/src/server/cmd_panes.rs @@ -29,6 +29,25 @@ use serde_json::Value; use serde_json::json; use std::sync::Arc; +/// One value of `set-layout-sizes`: what `set-split-ratio` or `set-viewport-pane-width` sets. Exactly one of the two +/// objects: a value with both pairs, with any other field, or not an object (serde gives untagged struct variants no +/// sequence form) is refused, not half applied (#34928 review A, #35261 review). +#[derive(Debug, Clone, Copy, serde::Deserialize)] +#[serde(untagged, deny_unknown_fields)] +pub(super) enum LayoutSizeWire { + SplitRatio { split: SplitId, ratio: f32 }, + PaneWidth { pane: PaneId, width: f32 }, +} + +impl From for crate::mux::LayoutSizeValue { + fn from(value: LayoutSizeWire) -> Self { + match value { + LayoutSizeWire::SplitRatio { split, ratio } => Self::SplitRatio { split, ratio }, + LayoutSizeWire::PaneWidth { pane, width } => Self::PaneWidth { pane, width }, + } + } +} + pub(super) fn export_layout(mux: &Arc, screen: Option) -> anyhow::Result { mux.with_state(|state| export_layout_json(state, screen)) } @@ -139,7 +158,7 @@ pub(super) fn set_layout_sizes( mux: &Arc, client: u64, actor: Actor, - values: Vec, + values: Vec, transaction: u64, ) -> anyhow::Result { let values: Vec = values.into_iter().map(Into::into).collect(); diff --git a/cmux-tui/crates/cmux-tui-core/src/server/tests/layout_receipts_shutdown.rs b/cmux-tui/crates/cmux-tui-core/src/server/tests/layout_receipts_shutdown.rs index 30533ed6677f..0a6477deb07d 100644 --- a/cmux-tui/crates/cmux-tui-core/src/server/tests/layout_receipts_shutdown.rs +++ b/cmux-tui/crates/cmux-tui-core/src/server/tests/layout_receipts_shutdown.rs @@ -250,6 +250,62 @@ fn set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole() { assert_eq!(applied["id"], 43); assert_eq!(applied["ok"], true, "{applied}"); assert_eq!(widths(&mux), vec![0.4, 0.6]); + + // A ratio value: the second column's split at 0.5 makes it as wide as the column left of it (0.4). + let split = mux.with_state(|state| state.workspaces[0].screens[0].layout_columns[1].id); + handle_message( + &mux, + mux.local_test_client(7), + &json!({ + "id": 44, + "cmd": "set-layout-sizes", + "values": [{"split": split, "ratio": 0.5}], + "transaction": 3 + }) + .to_string(), + &writer, + ); + let ratio: Value = serde_json::from_str(&outbound.try_pop().unwrap()).unwrap(); + assert_eq!(ratio["id"], 44); + assert_eq!(ratio["ok"], true, "{ratio}"); + let after_ratio = widths(&mux); + assert!( + after_ratio.len() == 2 && (after_ratio[1] - after_ratio[0]).abs() < 1e-5, + "a ratio of 0.5 must make the second column as wide as the first: {after_ratio:?}" + ); + + // #34928 review A and #35261 review: a value that is not exactly one pair (both pairs, or an array) is refused as + // a malformed request, not half applied. + for (id, value) in [ + (45, json!({"split": split, "ratio": 0.6, "pane": right_pane, "width": 0.3})), + (46, json!([split, 0.6])), + ] { + handle_message( + &mux, + mux.local_test_client(7), + &json!({"id": id, "cmd": "set-layout-sizes", "values": [value], "transaction": 3}) + .to_string(), + &writer, + ); + let refused: Value = serde_json::from_str(&outbound.try_pop().unwrap()).unwrap(); + assert_eq!(refused["id"], id); + assert_eq!( + refused["ok"], false, + "a value that is not exactly one pair was taken: {refused}" + ); + assert!( + refused["error"].as_str().is_some_and(|error| error.starts_with("bad request: ")), + "{refused}" + ); + assert!(refused.get("error_code").is_none(), "{refused}"); + assert_eq!(widths(&mux), after_ratio, "a refused value changed a width"); + } +} + +/// #34928 review C: identify advertises the batch command. +#[test] +fn identify_advertises_the_layout_sizes_batch() { + assert!(advertised_capabilities(false).contains(&LAYOUT_SIZES_BATCH_CAPABILITY)); } #[test] diff --git a/cmux-tui/spec/commands.md b/cmux-tui/spec/commands.md index 8ef47ac3384b..c4b50c033173 100644 --- a/cmux-tui/spec/commands.md +++ b/cmux-tui/spec/commands.md @@ -2892,7 +2892,7 @@ Params: Result: `{}`. The commit's `layout-changed` event carries `transaction`. -Errors: a refused value answers with its single command's error and `error_code` (`layout-ratio-target-missing`, `layout-ratio-out-of-range`, `row-split-compat-readonly`, `viewport-width-out-of-range`, `viewport-column-not-found`), its message prefixed with `value : `. A value on another screen than the first returns `error_code:"layout-sizes-other-screen"`; no value or more than 64 returns `error_code:"invalid-argument"`; a commit the store refuses returns `error_code:"layout-sizes-not-persisted"`. In every error case nothing changed. +Errors: a refused value answers with its single command's error and `error_code` (`layout-ratio-target-missing`, `layout-ratio-out-of-range`, `row-split-compat-readonly`, `viewport-width-out-of-range`, `viewport-column-not-found`), its message prefixed with `value : `. A value on another screen than the first returns `error_code:"layout-sizes-other-screen"`; no value or more than 64 returns `error_code:"invalid-argument"`; a value that is not exactly `{split, ratio}` or `{pane, width}` (both pairs, or another field) is refused as a malformed request; a commit the store refuses returns `error_code:"layout-sizes-not-persisted"`. In every error case nothing changed. Example: diff --git a/cmux-tui/spec/events.md b/cmux-tui/spec/events.md index 1167b6efbeed..8f2b041375aa 100644 --- a/cmux-tui/spec/events.md +++ b/cmux-tui/spec/events.md @@ -1010,7 +1010,7 @@ object{event:"layout-changed",screen:Id,workspace?:Id,active_pane?:Id,zoomed_pan Meaning: A screen's pane geometry changed through split, close/collapse, ratio update, viewport column width, row heights, dock, apply-layout, swap, undo, tab drag, or zoom. The event is emitted once per settled command. -A server that advertises `layout-delta-v1` includes the screen's new layout in the event: `workspace`, and the layout fields of that `Screen` exactly as `list-workspaces` reports them at the commit (`active_pane`, `zoomed_pane`, `layout`, and when present `viewport_splits`, `viewport_base_width`, `columns`). It does not include `panes` or tabs; the pane and tab deltas carry those. Like `Screen`, the SDK schema does not list `viewport_splits`, `viewport_base_width` and `columns` yet. `transaction` is the numeric `transaction` of the request that caused the change (`set-split-ratio`, `set-viewport-pane-width`, `set-column-dock`, `set-row-heights`), as a decimal string, so the frontend that sent it can recognize the echo of its own gesture. A client patches that one screen's layout. When the event carries no layout (an older server), or names a pane the client does not have, clients re-fetch `export-layout` or `list-workspaces`. +A server that advertises `layout-delta-v1` includes the screen's new layout in the event: `workspace`, and the layout fields of that `Screen` exactly as `list-workspaces` reports them at the commit (`active_pane`, `zoomed_pane`, `layout`, and when present `viewport_splits`, `viewport_base_width`, `columns`). It does not include `panes` or tabs; the pane and tab deltas carry those. Like `Screen`, the SDK schema does not list `viewport_splits`, `viewport_base_width` and `columns` yet. `transaction` is the numeric `transaction` of the request that caused the change (`set-split-ratio`, `set-viewport-pane-width`, `set-layout-sizes`, `set-column-dock`, `set-row-heights`), as a decimal string, so the frontend that sent it can recognize the echo of its own gesture. A client patches that one screen's layout. When the event carries no layout (an older server), or names a pane the client does not have, clients re-fetch `export-layout` or `list-workspaces`. Example: