From 0f3d06afa2b3bbbf33c8a15fb7e7a048afcd3925 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:31:24 -0700 Subject: [PATCH 1/8] WIP improvise-km71: edit a formula from the panel Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/command/cmd/commit.rs | 34 ++- .../improvise-command/src/command/cmd/core.rs | 17 ++ .../src/command/cmd/panel.rs | 21 +- .../src/command/cmd/registry.rs | 7 +- crates/improvise-command/src/ui/app.rs | 218 ++++++++++++++++++ crates/improvise-command/src/ui/effect.rs | 54 +++++ crates/improvise-command/src/ui/project.rs | 25 +- crates/improvise-command/src/ui/reduce.rs | 13 ++ crates/improvise-core/src/model/types.rs | 77 ++++++- crates/improvise-formula/src/ast.rs | 43 ++++ crates/improvise-formula/src/lib.rs | 2 +- crates/improvise-protocol/src/help.rs | 7 + crates/improvise-protocol/src/keymap.rs | 2 + crates/improvise-protocol/src/lib.rs | 27 ++- crates/improvise-protocol/src/reduce.rs | 16 +- crates/improvise-protocol/src/render.rs | 51 ++-- crates/improvise-protocol/src/resolve.rs | 96 +++++--- crates/improvise-wasm-client/src/render.rs | 4 +- .../improvise-wasm-client/tests/lockstep.rs | 54 +++++ src/ui/formula_panel.rs | 18 +- 20 files changed, 711 insertions(+), 75 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 86e4e55d..b9b85bb0 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1,5 +1,7 @@ +use crate::formula::FormulaId; use crate::model::category::MEASURE_CATEGORY; use crate::model::cell::CellKey; +use crate::protocol::FormulaSlot; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; use crate::view::RecordCol; @@ -806,6 +808,12 @@ impl Cmd for CommitAndAdvance { #[derive(Debug, Clone, PartialEq, Eq)] pub enum CommitKind { Formula, + /// Edit the formula the editor was opened on, read off + /// [`AppMode::FormulaEdit`]'s slot at construction (improvise-km71). + ReplaceFormula { + id: FormulaId, + was: String, + }, Category, /// Add an item to this category, resolved from [`AppMode::ItemAdd`] at /// construction so `execute` stays pure over the kind and its buffer. @@ -814,10 +822,20 @@ pub enum CommitKind { }, } -/// Resolve a `commit-buffer` argument against the current mode. Only `Item` needs -/// the mode, so a script line can still build the other two. +/// Resolve a `commit-buffer` argument against the current mode. `Item` and a +/// formula edit need the mode, so a script line can build only the others. pub fn parse_commit_kind(arg: &str, mode: &AppMode) -> Result { match (arg, mode) { + ( + "formula", + AppMode::FormulaEdit { + slot: FormulaSlot::Existing { id, was }, + .. + }, + ) => Ok(CommitKind::ReplaceFormula { + id: id.clone(), + was: was.clone(), + }), ("formula", _) => Ok(CommitKind::Formula), ("category", _) => Ok(CommitKind::Category), ("item", AppMode::ItemAdd { category, .. }) => Ok(CommitKind::Item { @@ -832,7 +850,7 @@ impl CommitKind { /// The minibuffer this kind consumes. pub fn buffer(&self) -> &'static str { match self { - Self::Formula => "formula", + Self::Formula | Self::ReplaceFormula { .. } => "formula", Self::Category => "category", Self::Item { .. } => "item", } @@ -842,7 +860,7 @@ impl CommitKind { /// minibuffer. fn on_empty(&self) -> AppMode { match self { - Self::Formula => AppMode::FormulaPanel, + Self::Formula | Self::ReplaceFormula { .. } => AppMode::FormulaPanel, Self::Category | Self::Item { .. } => AppMode::CategoryPanel, } } @@ -851,7 +869,7 @@ impl CommitKind { /// categories and items are entered in batches, formulas are not. fn on_commit(&self) -> Option { match self { - Self::Formula => Some(AppMode::FormulaPanel), + Self::Formula | Self::ReplaceFormula { .. } => Some(AppMode::FormulaPanel), Self::Category | Self::Item { .. } => None, } } @@ -859,6 +877,7 @@ impl CommitKind { fn status(&self, name: &str) -> String { match self { Self::Formula => "Formula added".to_string(), + Self::ReplaceFormula { .. } => "Formula updated".to_string(), Self::Category => format!("Added category \"{name}\""), Self::Item { .. } => format!("Added \"{name}\""), } @@ -872,6 +891,11 @@ impl CommitKind { raw: name.to_string(), target_category: "_Measure".to_string(), }), + Self::ReplaceFormula { id, was } => Box::new(effect::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: name.to_string(), + }), Self::Category => Box::new(effect::AddCategory(name.to_string())), Self::Item { category } => Box::new(effect::AddItem { category: category.clone(), diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 9944f2de..867acb6e 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -236,6 +236,23 @@ impl CmdRegistry { }); } + /// Register a command whose interactive arm ALWAYS reads the context, and + /// whose text arm is `parse` — for a mode that carries a capture (km71). + pub fn register_contextual( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + interactive: FallbackFn, + ) { + self.entries.push(CmdEntry { + name: prototype.name(), + parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), + interactive: Box::new(move |args, ctx| { + interactive(args, ctx).map(|c| Box::new(c) as Box) + }), + }); + } + /// Register a command that needs no interactive context: BOTH arms are /// `parse`, so neither can demote the other — `context/code/command.md`. pub fn register_pure(&mut self, prototype: &dyn Cmd, parse: ParseFn) { diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 6793c910..4cc3dc70 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -4,7 +4,7 @@ use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect, Panel, ViewTarget}; use crate::view::Axis; -use super::core::{Cmd, CmdContext}; +use super::core::{Cmd, CmdContext, write_buffer}; #[cfg(test)] mod tests { @@ -685,6 +685,25 @@ impl Cmd for DeleteFormulaAtCursor { } } +/// Open the formula editor on the formula under the cursor, seeded with its +/// text and naming it from here on — the commit never re-reads the cursor. +#[derive(Debug)] +pub struct EditFormulaAtCursor; +impl Cmd for EditFormulaAtCursor { + fn name(&self) -> &'static str { + "edit-formula-at-cursor" + } + fn execute(&self, ctx: &CmdContext) -> Vec> { + match ctx.model.formulas().get(ctx.formula_cursor) { + Some(f) => vec![ + effect::change_mode(AppMode::formula_replace(f.id(), f.raw.clone())), + write_buffer("formula", f.raw.clone()), + ], + None => vec![], + } + } +} + // ── Category panel commands ───────────────────────────────────────────────── /// Cycle the axis assignment of the category at the cursor. diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index dbd86d68..25a3c98b 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -370,6 +370,7 @@ pub fn default_registry() -> CmdRegistry { None, ); r.register_nullary(|| Box::new(DeleteFormulaAtCursor)); + r.register_nullary(|| Box::new(EditFormulaAtCursor)); r.register_nullary(|| Box::new(AddRecordRow)); r.register_nullary(|| Box::new(TogglePruneEmpty)); r.register_nullary(|| Box::new(ToggleRecordsMode)); @@ -466,7 +467,7 @@ pub fn default_registry() -> CmdRegistry { |_| Err("commit-and-advance-right requires context".into()), Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); - r.register( + r.register_contextual( &CommitBuffer { kind: CommitKind::Category, }, @@ -475,11 +476,11 @@ pub fn default_registry() -> CmdRegistry { let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; Ok(CommitBuffer { kind }) }, - Some(|args, ctx| { + |args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; Ok(CommitBuffer { kind }) - }), + }, ); r.register_nullary(|| Box::new(CommitExport)); r.register_nullary(|| Box::new(CommitSave)); diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 4969f2cb..e6fe3ee1 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -4323,3 +4323,221 @@ mod basis_tiers { } } } + +/// Editing a formula from the panel (improvise-km71), driven through +/// `handle_key` at a NONZERO cursor over three formulas. +#[cfg(test)] +mod formula_edit_tests { + use super::*; + use crate::formula::parse_formula; + + const PROFIT: &str = "Profit = Revenue - Cost"; + const TAX: &str = "Tax = Revenue * 0.1"; + const MARGIN: &str = "Margin = Profit / Revenue"; + + fn app_with_three_formulas() -> App { + let mut wb = Workbook::new("T"); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + for raw in [PROFIT, TAX, MARGIN] { + wb.model.add_formula(parse_formula(raw, "_Measure").unwrap()); + } + let mut app = App::new(wb, None); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + app + } + + fn press(app: &mut App, code: KeyCode) { + app.handle_key(KeyEvent::new(code, KeyModifiers::NONE)) + .unwrap(); + } + + /// Clear the minibuffer by keys and type `text`, as a user would. + fn retype(app: &mut App, text: &str) { + let len = app.view_state.buffers.get("formula").map_or(0, String::len); + for _ in 0..len { + press(app, KeyCode::Backspace); + } + for c in text.chars() { + press(app, KeyCode::Char(c)); + } + } + + fn raws(app: &App) -> Vec { + app.model_state + .workbook + .model + .formulas() + .iter() + .map(|f| f.raw.clone()) + .collect() + } + + fn buffer(app: &App) -> Option<&str> { + app.view_state.buffers.get("formula").map(String::as_str) + } + + fn open_editor(app: &mut App) { + press(app, KeyCode::Enter); + assert!( + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }), + "Enter on a formula must open the editor, got {:?}", + app.view_state.mode + ); + } + + fn in_editor(app: &App) -> bool { + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }) + } + + #[test] + fn enter_opens_the_editor_seeded_with_the_formula_under_the_cursor() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + assert_eq!(buffer(&app), Some(TAX), "cursor 1 is Tax, not the first"); + } + + #[test] + fn e_opens_the_same_editor() { + let mut app = app_with_three_formulas(); + press(&mut app, KeyCode::Char('e')); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some(TAX)); + } + + #[test] + fn committing_an_edit_replaces_the_formula_in_place() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Tax = Revenue * 0.2", MARGIN]); + assert_eq!(app.view_state.mode, AppMode::FormulaPanel); + } + + /// The rival "add instead of replace" passes the test above, because + /// `add_formula` replaces by target. A rename is where it cannot hide. + #[test] + fn changing_the_target_renames_the_formula_in_its_own_place() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Levy = Revenue * 0.1"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Levy = Revenue * 0.1", MARGIN]); + let measures = app.model_state.workbook.model.measure_item_names(); + assert!( + !measures.contains(&"Tax".to_string()), + "Tax was a formula target only, so it goes: {measures:?}" + ); + } + + #[test] + fn a_rename_says_which_formulas_still_name_the_old_target() { + let mut app = app_with_three_formulas(); + app.view_state.formula_cursor = 0; + open_editor(&mut app); + retype(&mut app, "Gross = Revenue - Cost"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app)[0], "Gross = Revenue - Cost"); + let status = &app.view_state.status_msg; + assert!( + status.contains("Profit") && status.contains("Margin"), + "Margin still refers to Profit, and the user must be told: {status:?}" + ); + } + + #[test] + fn a_rename_onto_another_formulas_target_refuses_and_keeps_both() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Margin = Revenue * 0.1"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert!(app.view_state.status_msg.contains("Margin")); + } + + #[test] + fn a_parse_error_keeps_the_editor_the_text_and_the_formula() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = = "); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Tax = = ")); + assert!( + app.view_state.status_msg.contains("Formula error"), + "{:?}", + app.view_state.status_msg + ); + } + + /// The editor names its formula when it OPENS. Deleting an earlier one + /// moves the edited formula to index 0 and puts `Margin` under cursor 1. + #[test] + fn an_edit_lands_on_its_formula_after_an_earlier_one_is_deleted_meanwhile() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let model = &mut app.model_state.workbook.model; + model.remove_formula("Profit", "_Measure"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec!["Tax = Revenue * 0.2", MARGIN]); + } + + #[test] + fn an_edit_whose_formula_was_deleted_meanwhile_refuses_out_loud() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let model = &mut app.model_state.workbook.model; + model.remove_formula("Tax", "_Measure"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, MARGIN], "nothing re-added"); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Tax = Revenue * 0.2")); + assert!(app.last_batch_refused()); + } + + #[test] + fn an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = Revenue * 0.2"); + let theirs = parse_formula("Tax = Revenue * 0.3", "_Measure").unwrap(); + app.model_state.workbook.model.add_formula(theirs); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Tax = Revenue * 0.3", MARGIN]); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert!(app.last_batch_refused()); + } + + #[test] + fn esc_leaves_the_formula_untouched() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + retype(&mut app, "Tax = 0"); + press(&mut app, KeyCode::Esc); + assert_eq!(raws(&app), vec![PROFIT, TAX, MARGIN]); + assert_eq!(app.view_state.mode, AppMode::FormulaPanel); + } + + /// `n` still adds: the replace slot must not leak into a fresh editor. + #[test] + fn a_new_formula_after_an_edit_is_added_not_replaced() { + let mut app = app_with_three_formulas(); + open_editor(&mut app); + press(&mut app, KeyCode::Esc); + press(&mut app, KeyCode::Char('n')); + assert_eq!(buffer(&app), Some(""), "a new formula starts empty"); + retype(&mut app, "Levy = Revenue * 0.3"); + press(&mut app, KeyCode::Enter); + assert_eq!( + raws(&app), + vec![PROFIT, TAX, MARGIN, "Levy = Revenue * 0.3"] + ); + } +} diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 6eefb8f9..629c7ba9 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -2,6 +2,8 @@ use std::collections::BTreeMap; use std::fmt::Debug; use std::path::PathBuf; +use crate::formula::FormulaId; +use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; use crate::view::{Axis, RecordCol, RecordRow}; @@ -246,6 +248,58 @@ impl Effect for RemoveFormula { } } +/// Edit the formula `id` names in place. Every refusal stops the batch, so the +/// editor stays open over the user's text — `context/code/command.md`. +#[derive(Debug)] +pub struct ReplaceFormula { + pub id: FormulaId, + pub was: String, + pub raw: String, +} +impl Effect for ReplaceFormula { + fn apply(&self, app: &mut App) { + match self.replace(&mut app.model_state.workbook.model) { + Ok(Some(status)) => app.view_state.status_msg = status, + Ok(None) => {} + Err(why) => refuse_and_stop(app, why), + } + } + fn kind(&self) -> EffectKind { + EffectKind::Model(ModelChange::CellData) + } +} + +impl ReplaceFormula { + /// The replacement, and what a rename leaves naming the old target. + fn replace(&self, model: &mut Model) -> Result, String> { + let old = model + .formula(&self.id) + .filter(|f| f.raw == self.was) + .ok_or_else(|| format!("\"{}\" was changed or removed meanwhile", self.was))?; + let old_target = old.target.clone(); + let new = crate::formula::parse_formula(&self.raw, &old.target_category) + .map_err(|e| format!("Formula error: {e}"))?; + let new_target = new.target.clone(); + model + .replace_formula(&self.id, new) + .map_err(|e| e.to_string())?; + Ok((new_target != old_target).then(|| renamed(model, &old_target, &new_target))) + } +} + +fn renamed(model: &Model, old: &str, new: &str) -> String { + let still: Vec<&str> = model + .formulas() + .iter() + .filter(|f| f.mentions(old)) + .map(|f| f.target.as_str()) + .collect(); + match still.is_empty() { + true => format!("Renamed {old} to {new}"), + false => format!("Renamed {old} to {new}; {old} is still used by {}", still.join(", ")), + } +} + /// Refresh the derived grid layout mid-batch: `apply_effects` rebuilds only once /// after the batch, so an effect that *reads* it needs this first (improvise-ehz). #[derive(Debug)] diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index ff23ca77..7f6ad873 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -15,7 +15,7 @@ use improvise_io::import::analyzer::{DateComponent, FieldKind}; use improvise_io::import::wizard::{ImportWizard, WizardStep}; use improvise_protocol::{ AxisFrame, Basis, BasisCache, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, - CellId, Command, CreateKind, DocumentCache, FormulaCache, FrameEntry, FrameGroup, GridCache, + CellId, Command, CreateKind, DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, GridUpdate, Projection, RecordCell, Rejection, Snapshot, TileBarCache, TileEntry, ViewListCache, ViewState, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, WizardProposal, WizardStepKind, basis, reduce_view, @@ -371,14 +371,15 @@ pub fn category_rows(model: &Model, view: &View) -> Vec { .collect() } -/// The raw text of every formula, which is all the formula panel draws. -pub fn formula_texts(app: &App) -> Vec { +/// Every formula's text, which the panel draws, and its identity, which an +/// edit names it by (improvise-km71). +pub fn formula_entries(app: &App) -> Vec { app.model_state .workbook .model .formulas() .iter() - .map(|f| f.raw.clone()) + .map(FormulaEntry::from) .collect() } @@ -430,10 +431,10 @@ pub fn category_tree_in(app: &App, view: &View) -> CategoryTreeCache { } } -/// The formula panel's cache: the raw text of every formula. +/// The formula panel's cache: every formula's text and identity. pub fn formulas_of(app: &App) -> FormulaCache { FormulaCache { - formulas: formula_texts(app), + formulas: formula_entries(app), } } @@ -1742,8 +1743,8 @@ mod tests { client.fold(&projections); } assert_eq!( - client.formulas.formulas, - vec!["Total = Revenue".to_string()], + client.formulas.formulas.iter().map(|f| f.raw.as_str()).collect::>(), + vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); @@ -1757,7 +1758,7 @@ mod tests { ); assert_eq!( client.formulas.formulas, - Vec::::new(), + Vec::::new(), "the client is still showing a formula the document no longer has" ); } @@ -1782,8 +1783,8 @@ mod tests { let mut client = ClientCaches::default(); client.fold(&snapshot.seed); assert_eq!( - client.formulas.formulas, - vec!["Total = Revenue".to_string()], + client.formulas.formulas.iter().map(|f| f.raw.as_str()).collect::>(), + vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); @@ -1806,7 +1807,7 @@ mod tests { ); assert_eq!( client.formulas.formulas, - Vec::::new(), + Vec::::new(), "the client is still showing a formula the document no longer has" ); } diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index e59e54cf..a8658c9d 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -178,6 +178,11 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { target: target.clone(), target_category: target_category.clone(), }), + ModelCommand::ReplaceFormula { id, was, raw } => Box::new(effect::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: raw.clone(), + }), ModelCommand::CleanEmptyRecords => Box::new(effect::CleanEmptyRecords), ModelCommand::SortData => Box::new(effect::SortData), @@ -685,6 +690,13 @@ pub(crate) mod tests { target: "Profit".into(), target_category: "_Measure".into(), }, + ModelCommand::ReplaceFormula { + id: crate::formula::parse_formula("Profit = Revenue", "_Measure") + .unwrap() + .id(), + was: "Profit = Revenue".into(), + raw: "Profit = Revenue - Cost".into(), + }, ModelCommand::CleanEmptyRecords, ModelCommand::SortData, ModelCommand::AddCategory("Product".into()), @@ -749,6 +761,7 @@ pub(crate) mod tests { ModelCommand::RemoveItem { .. } => "RemoveItem", ModelCommand::AddFormula { .. } => "AddFormula", ModelCommand::RemoveFormula { .. } => "RemoveFormula", + ModelCommand::ReplaceFormula { .. } => "ReplaceFormula", ModelCommand::CleanEmptyRecords => "CleanEmptyRecords", ModelCommand::SortData => "SortData", ModelCommand::AddCategory(_) => "AddCategory", diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index ea394dbe..30d671b9 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use super::category::{Category, CategoryId, CategoryKind, MEASURE_CATEGORY}; use super::cell::{CellKey, CellValue, DataStore}; -use crate::formula::{AggFunc, Expr, Filter, Formula}; +use crate::formula::{AggFunc, Expr, Filter, Formula, FormulaId}; const MAX_CATEGORIES: usize = 12; @@ -417,6 +417,31 @@ impl Model { &self.formulas } + pub fn formula(&self, id: &FormulaId) -> Option<&Formula> { + self.formulas.iter().find(|f| f.id() == *id) + } + + /// Put `formula` where `id` is, keeping its position; refuses rather than + /// let a new target displace another formula — `context/code/core.md` §14. + pub fn replace_formula(&mut self, id: &FormulaId, formula: Formula) -> Result<()> { + let new_id = formula.id(); + if new_id != *id && self.formula(&new_id).is_some() { + return Err(anyhow!("A formula for {} already exists", formula.target)); + } + let pos = self + .formulas + .iter() + .position(|f| f.id() == *id) + .ok_or_else(|| anyhow!("That formula no longer exists"))?; + if !CategoryKind::of_name(&formula.target_category).is_measure() + && let Some(cat) = self.categories.get_mut(&formula.target_category) + { + cat.add_item(&formula.target); + } + self.formulas[pos] = formula; + Ok(()) + } + /// The formula that owns `key`, if one does. **The one home** of "is this /// cell derived" — every other answer to it is a second derivation. pub fn formula_for(&self, key: &CellKey) -> Option<&Formula> { @@ -1861,6 +1886,56 @@ mod formula_tests { assert_eq!(m.evaluate(&k), None); } + fn three_formulas() -> Model { + let mut m = revenue_cost_model(); + for raw in ["Profit = Revenue - Cost", "Tax = Revenue * 0.1", "Margin = Profit / Revenue"] { + m.add_formula(parse_formula(raw, "_Measure").unwrap()); + } + m + } + + fn targets(m: &Model) -> Vec<&str> { + m.formulas.iter().map(|f| f.target.as_str()).collect() + } + + #[test] + fn replace_formula_renames_in_place_and_evaluates_under_the_new_name() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + m.replace_formula(&id, parse_formula("Levy = Revenue * 0.5", "_Measure").unwrap()) + .unwrap(); + assert_eq!(targets(&m), vec!["Profit", "Levy", "Margin"]); + let levy = coord(&[("_Measure", "Levy"), ("Region", "East")]); + assert_eq!(m.evaluate(&levy).and_then(|v| v.as_f64()), Some(500.0)); + } + + #[test] + fn replace_formula_refuses_to_displace_another_formula() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + let onto_margin = parse_formula("Margin = Revenue", "_Measure").unwrap(); + assert!(m.replace_formula(&id, onto_margin).is_err()); + assert_eq!(targets(&m), vec!["Profit", "Tax", "Margin"]); + } + + #[test] + fn replace_formula_refuses_an_id_that_names_nothing() { + let mut m = three_formulas(); + let id = m.formulas[1].id(); + m.remove_formula("Tax", "_Measure"); + let tax = parse_formula("Tax = Revenue", "_Measure").unwrap(); + assert!(m.replace_formula(&id, tax).is_err()); + assert_eq!(targets(&m), vec!["Profit", "Margin"]); + } + + #[test] + fn a_formula_mentions_what_its_expression_or_its_where_names() { + let margin = parse_formula("Margin = Profit / Revenue", "_Measure").unwrap(); + assert!(margin.mentions("Profit") && !margin.mentions("Tax")); + let scoped = parse_formula("East = SUM(Revenue) WHERE Region = \"East\"", "_Measure"); + assert!(scoped.unwrap().mentions("Region")); + } + #[test] fn sum_aggregation_across_region() { let mut m = revenue_cost_model(); diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 1fe4bad1..7ef59693 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -87,4 +87,47 @@ impl Formula { self.format = format; self } + + /// Whether this formula's expression or its `WHERE` names `name`. + pub fn mentions(&self, name: &str) -> bool { + self.expr.mentions(name) || self.filter.as_ref().is_some_and(|f| f.names(name)) + } + + /// Which formula this is. The one constructor of a [`FormulaId`]. + pub fn id(&self) -> FormulaId { + FormulaId { + target: self.target.clone(), + target_category: self.target_category.clone(), + } + } +} + +/// Which formula an edit means, opaque so its content can change in one place: +/// today a target and its category — `context/code/core.md` §14. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub struct FormulaId { + target: String, + target_category: String, +} + +impl Expr { + /// Whether this expression names `name` anywhere, as a reference or as + /// either half of a coordinate filter. + pub fn mentions(&self, name: &str) -> bool { + match self { + Expr::Number(_) => false, + Expr::Ref(r) => r == name, + Expr::BinOp(_, l, r) => l.mentions(name) || r.mentions(name), + Expr::UnaryMinus(e) => e.mentions(name), + Expr::Agg(_, e, f) => e.mentions(name) || f.as_ref().is_some_and(|f| f.names(name)), + Expr::If(c, t, e) => c.mentions(name) || t.mentions(name) || e.mentions(name), + Expr::Is(f) => f.names(name), + } + } +} + +impl Filter { + fn names(&self, name: &str) -> bool { + self.category == name || self.item == name + } } diff --git a/crates/improvise-formula/src/lib.rs b/crates/improvise-formula/src/lib.rs index 1ebc5bc8..741811f9 100644 --- a/crates/improvise-formula/src/lib.rs +++ b/crates/improvise-formula/src/lib.rs @@ -2,6 +2,6 @@ pub mod ast; pub mod parser; pub mod quote; -pub use ast::{AggFunc, BinOp, Expr, Filter, Formula}; +pub use ast::{AggFunc, BinOp, Expr, Filter, Formula, FormulaId}; pub use parser::parse_formula; pub use quote::{is_bare_name, quote_name}; diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 017bff2a..d3ad0577 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -101,6 +101,7 @@ pub(crate) fn advertised(mode: &AppMode) -> Vec<(&'static str, Option vec![ ("enter-mode", arg("formula-edit"), "new"), + ("edit-formula-at-cursor", None, "edit"), ("delete-formula-at-cursor", None, "delete"), ("move-panel-cursor", None, "nav"), ("enter-mode", arg("normal"), "back"), @@ -718,6 +719,12 @@ fn panels(keys: KeysRow) -> Vec { Some(&["formula-edit"]), "New formula", ), + keys( + &ModeKey::FormulaPanel, + "edit-formula-at-cursor", + None, + "Edit selected formula", + ), keys( &ModeKey::FormulaPanel, "delete-formula-at-cursor", diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 6b7aef43..3231cd18 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -766,6 +766,8 @@ impl Keymaps { "enter-mode", vec!["formula-edit".into()], ) + .bind(KeyCode::Enter, "edit-formula-at-cursor") + .bind(KeyCode::Char('e'), "edit-formula-at-cursor") .delete("delete-formula-at-cursor") .finish(), ), diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 1e178f92..878b59a2 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -30,7 +30,7 @@ pub use keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey}; pub use reduce::{Command, HelpPage, ModelCommand, records_named, reduce_view}; pub use render::{ AxisFrame, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, CellId, CreateKind, - DocumentCache, FormulaCache, FrameEntry, FrameGroup, GridCache, GridExtent, GridUpdate, + DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, GridUpdate, MAX_ROW_HEADER_W, MIN_ROW_HEADER_W, Projection, RecordCell, Snapshot, TileBarCache, TileEntry, ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, WizardProposal, WizardStepKind, header_level_widths, @@ -48,6 +48,9 @@ pub use improvise_core::view::layout::PageAxis; // without depending on `improvise-core` directly. pub use improvise_core::model::cell::CellValue; +// On the wire in `ModelCommand::ReplaceFormula` and in `AppMode::FormulaEdit`. +pub use improvise_core::formula::FormulaId; + /// Number of pages in the Help screen. Lives here with the `help_page` state /// it bounds, so help-page effects stay terminal-independent. pub const HELP_PAGE_COUNT: usize = 5; @@ -139,6 +142,7 @@ pub enum AppMode { }, FormulaEdit { minibuf: MinibufferConfig, + slot: FormulaSlot, }, FormulaPanel, CategoryPanel, @@ -176,6 +180,15 @@ pub enum AppMode { }, } +/// What a formula editor commits to, captured when it OPENS and never +/// recomputed at commit — `context/code/protocol.md` §1 (improvise-km71). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub enum FormulaSlot { + New, + /// Replace the formula `id` names, provided it still reads `was`. + Existing { id: FormulaId, was: String }, +} + /// `Normal` and `RecordsNormal` are one mode over two layouts, so the mode an /// end holds must answer for the grid it holds — `context/code/protocol.md`. pub fn settled_mode(mode: &AppMode, records: bool) -> AppMode { @@ -333,12 +346,22 @@ impl AppMode { } pub fn formula_edit() -> Self { + Self::formula_editor("formula: ", FormulaSlot::New) + } + + /// The editor opened on an existing formula, which it names from here on. + pub fn formula_replace(id: FormulaId, was: String) -> Self { + Self::formula_editor("edit formula: ", FormulaSlot::Existing { id, was }) + } + + fn formula_editor(prompt: &str, slot: FormulaSlot) -> Self { Self::FormulaEdit { minibuf: MinibufferConfig { buffer_key: "formula".into(), - prompt: "formula: ".into(), + prompt: prompt.into(), color: MinibufColor::Cyan, }, + slot, } } diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 9e816f01..c7ba84dd 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -10,7 +10,7 @@ use improvise_core::view::Axis; use crate::basis::Basis; use crate::render::{Projection, RecordCell}; -use crate::{AppMode, HELP_PAGE_COUNT, KeyCode, KeyModifiers, Panel, ViewState}; +use crate::{AppMode, FormulaId, HELP_PAGE_COUNT, KeyCode, KeyModifiers, Panel, ViewState}; use improvise_core::model::record::RecordHash; /// One message on the wire, in either direction. @@ -75,7 +75,7 @@ pub enum Command { Project(Projection), } -/// A mutation of the shared document: 24 variants, derived not invented (§2). A +/// A mutation of the shared document: 25 variants, derived not invented (§2). A /// view-configuration variant **names its view** (improvise-lltt). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum ModelCommand { @@ -107,6 +107,13 @@ pub enum ModelCommand { target: String, target_category: String, }, + /// Edit the formula `id` names in place, if it still reads `was` — an edit + /// of that formula, not a remove-then-add (improvise-km71). + ReplaceFormula { + id: FormulaId, + was: String, + raw: String, + }, CleanEmptyRecords, SortData, @@ -176,6 +183,7 @@ pub fn records_named(cmd: &ModelCommand) -> Vec { | ModelCommand::RemoveItem { .. } | ModelCommand::AddFormula { .. } | ModelCommand::RemoveFormula { .. } + | ModelCommand::ReplaceFormula { .. } | ModelCommand::CleanEmptyRecords | ModelCommand::SortData | ModelCommand::AddCategory(_) @@ -587,6 +595,9 @@ mod tests { fn surrogate(_: &RecordHash) -> usize { 1 } + fn formula(_: &FormulaId) -> usize { + 1 + } fn record(RecordCell { record, measure }: &RecordCell) -> usize { surrogate(record) + optional_name(measure) } @@ -621,6 +632,7 @@ mod tests { target, target_category, } => name(target) + name(target_category), + ModelCommand::ReplaceFormula { id, was, raw } => formula(id) + name(was) + name(raw), ModelCommand::CleanEmptyRecords | ModelCommand::SortData => 0, ModelCommand::AddCategory(category) | ModelCommand::RemoveCategory(category) diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 6397136e..65e758a3 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -9,7 +9,8 @@ use improvise_core::model::record::{AggregateHash, RecordHash}; use improvise_core::view::Axis; use improvise_core::view::layout::PageAxis; -use crate::ViewState; +use crate::{FormulaId, ViewState}; +use improvise_core::formula::Formula; /// A widget's local copy of what it draws. **Total and deterministic**: an /// update naming something absent is dropped, never a panic. @@ -728,17 +729,42 @@ impl WidgetCache for CategoryTreeCache { } } -/// The formula panel's copy of the formula list (improvise-pca). One -/// mutation: add and delete are the only effects, and both send whole. +/// The formula panel's copy of the formula list (improvise-pca). Every +/// formula effect sends it whole. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct FormulaCache { - pub formulas: Vec, + pub formulas: Vec, +} + +/// One formula as a client holds it: its text, and the identity an edit names +/// it by, which the text alone cannot give a client (improvise-km71). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct FormulaEntry { + pub id: FormulaId, + pub raw: String, +} + +impl From<&Formula> for FormulaEntry { + fn from(f: &Formula) -> Self { + Self { + id: f.id(), + raw: f.raw.clone(), + } + } +} + +/// A `_Measure` formula's entry, identified exactly as the host identifies it. +#[cfg(any(test, feature = "test-support"))] +impl From<&str> for FormulaEntry { + fn from(raw: &str) -> Self { + Self::from(&improvise_core::formula::parse_formula(raw, "_Measure").expect(raw)) + } } impl WidgetCache for FormulaCache { - type Update = Vec; + type Update = Vec; - fn apply(&mut self, update: &Vec) { + fn apply(&mut self, update: &Vec) { self.formulas = update.clone(); } } @@ -874,8 +900,8 @@ pub struct WizardCache { pub enum Projection { Grid(GridUpdate), CategoryTree(CategoryTreeUpdate), - /// The raw text of every formula. - Formulas(Vec), + /// Every formula: its text and its identity. + Formulas(Vec), ViewList(Vec), TileBar(Vec), /// Name, dirty marker and where the host keeps it (improvise-z754). @@ -1482,11 +1508,8 @@ mod tests { #[test] fn the_list_caches_hold_the_last_update() { let formulas = FormulaCache::folded(&[ - vec!["Profit = Revenue - Cost".to_string()], - vec![ - "Profit = Revenue - Cost".to_string(), - "Tax = Revenue * 0.1".to_string(), - ], + vec!["Profit = Revenue - Cost".into()], + vec!["Profit = Revenue - Cost".into(), "Tax = Revenue * 0.1".into()], ]); assert_eq!(formulas.formulas.len(), 2); @@ -1842,7 +1865,7 @@ mod tests { assert_eq!(booted, (grid, formulas)); assert_eq!(booted.0.cell(0, 0).unwrap().text, "5"); - assert_eq!(booted.1.formulas, vec!["Tax = Revenue * 0.1"]); + assert_eq!(booted.1.formulas, vec!["Tax = Revenue * 0.1".into()]); } /// Two subscribers, each folding only what is addressed to it — the diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index b44921aa..3a72af64 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -8,8 +8,8 @@ use crate::frame::ClientFrame; use crate::keymap::{Binding, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, axis_change_status, flatten_cat_tree}; use crate::{ - AppMode, CellId, CellValue, Command, CreateKind, GridExtent, HelpPage, KeyCode, KeyEvent, - ModelCommand, Panel, RecordCell, ViewState, records_named, + AppMode, CellId, CellValue, Command, CreateKind, FormulaId, FormulaSlot, GridExtent, HelpPage, + KeyCode, KeyEvent, ModelCommand, Panel, RecordCell, ViewState, records_named, }; /// The virtual category a new measure is filed under, spelled as @@ -186,8 +186,8 @@ fn click_view_row(state: &ClientState, index: usize) -> Option> { Some(commands) } -/// The formula panel binds **no** `Enter`, so the rule yields navigation and -/// nothing else. That is the rule applying, not an omission — clients.md. +/// Focus the panel, move its cursor, then open the editor on the row — what +/// `F` then `Enter` do. `edit-formula-at-cursor` is the activation (km71). fn click_formula_row(state: &ClientState, index: usize) -> Option> { state.formulas.formulas.get(index)?; let mut commands = focus(state, Panel::Formula); @@ -195,9 +195,26 @@ fn click_formula_row(state: &ClientState, index: usize) -> Option> panel: Panel::Formula, cursor: index, }); + // Off `index`, never the cursor: `SetPanelCursor` is not folded yet. + commands.extend(edit_formula_at(state, index)); Some(commands) } +/// `EditFormulaAtCursor::execute` at an arbitrary index: open the editor on +/// that formula, naming it, and seed the buffer with its text. +fn edit_formula_at(state: &ClientState, index: usize) -> Vec { + match state.formulas.formulas.get(index) { + Some(entry) => vec![ + Command::ChangeMode(AppMode::formula_replace( + entry.id.clone(), + entry.raw.clone(), + )), + write_buffer("formula", entry.raw.clone()), + ], + None => vec![], + } +} + /// Focus the panel, move its cursor, then activate — the three things the /// keyboard needs three keystrokes for, and `filter-to-item` is the activation. fn click_category_row(state: &ClientState, index: usize) -> Option> { @@ -455,6 +472,9 @@ fn composed( "Move cursor to a category header to change axis".to_string(), )], }), + // ── the formula panel ──────────────────────────────────────── + "edit-formula-at-cursor" => Some(edit_formula_at(state, state.view.formula_cursor)), + "open-item-add-at-cursor" => Some(match cat_at_cursor(state) { Some(cat) => vec![Command::ChangeMode(AppMode::item_add(cat))], None => vec![Command::SetStatus( @@ -1261,9 +1281,10 @@ fn parse_axis(s: &str) -> Option { } /// A mirror of `commit.rs`'s `CommitKind`, resolved against the mode exactly -/// as `parse_commit_kind` does — only `Item` needs the mode. +/// as `parse_commit_kind` does — `Item` and a formula edit need the mode. enum CommitKind { Formula, + ReplaceFormula { id: FormulaId, was: String }, Category, Item { category: String }, } @@ -1271,6 +1292,16 @@ enum CommitKind { impl CommitKind { fn of(arg: &str, mode: &AppMode) -> Option { match (arg, mode) { + ( + "formula", + AppMode::FormulaEdit { + slot: FormulaSlot::Existing { id, was }, + .. + }, + ) => Some(Self::ReplaceFormula { + id: id.clone(), + was: was.clone(), + }), ("formula", _) => Some(Self::Formula), ("category", _) => Some(Self::Category), ("item", AppMode::ItemAdd { category, .. }) => Some(Self::Item { @@ -1282,7 +1313,7 @@ impl CommitKind { fn buffer(&self) -> &'static str { match self { - Self::Formula => "formula", + Self::Formula | Self::ReplaceFormula { .. } => "formula", Self::Category => "category", Self::Item { .. } => "item", } @@ -1290,14 +1321,14 @@ impl CommitKind { fn on_empty(&self) -> AppMode { match self { - Self::Formula => AppMode::FormulaPanel, + Self::Formula | Self::ReplaceFormula { .. } => AppMode::FormulaPanel, Self::Category | Self::Item { .. } => AppMode::CategoryPanel, } } fn on_commit(&self) -> Option { match self { - Self::Formula => Some(AppMode::FormulaPanel), + Self::Formula | Self::ReplaceFormula { .. } => Some(AppMode::FormulaPanel), Self::Category | Self::Item { .. } => None, } } @@ -1305,6 +1336,7 @@ impl CommitKind { fn status(&self, name: &str) -> String { match self { Self::Formula => "Formula added".to_string(), + Self::ReplaceFormula { .. } => "Formula updated".to_string(), Self::Category => format!("Added category \"{name}\""), Self::Item { .. } => format!("Added \"{name}\""), } @@ -1316,6 +1348,11 @@ impl CommitKind { raw: name.to_string(), target_category: "_Measure".to_string(), }, + Self::ReplaceFormula { id, was } => ModelCommand::ReplaceFormula { + id: id.clone(), + was: was.clone(), + raw: name.to_string(), + }, Self::Category => ModelCommand::AddCategory(name.to_string()), Self::Item { category } => ModelCommand::AddItem { category: category.clone(), @@ -1869,11 +1906,12 @@ mod tests { ); } - /// The formula panel binds no `Enter`, so the rule yields navigation and - /// nothing else. A mode change here would be an invented activation. + /// The formula panel's `Enter` edits (improvise-km71), so a click opens the + /// editor on the row CLICKED — `Margin`, where the cursor is not. #[test] - fn a_formula_row_click_navigates_and_activates_nothing() { + fn a_formula_row_click_navigates_and_opens_the_editor_on_that_row() { let state = with_panels(); + let margin = state.formulas.formulas[1].clone(); let commands = click(&state, Click::FormulaRow(1)); assert_eq!( commands, @@ -1887,28 +1925,30 @@ mod tests { panel: Panel::Formula, cursor: 1 }, + Command::ChangeMode(AppMode::formula_replace(margin.id, margin.raw.clone())), + Command::SetBuffer { + name: "formula".into(), + value: margin.raw, + }, ] ); } - /// The asymmetry is a fact about the keymap, so it is asserted against - /// the keymap — not against a reading of it. See `context/code/clients.md`. + /// A click mirrors `Enter`, so the tie is asserted against what `Enter` + /// resolves to, not against a reading of it. See `context/code/clients.md`. #[test] - fn the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does() { - let keymaps = Keymaps::shared(); + fn a_formula_row_click_ends_with_what_enter_does_on_that_row() { + let mut state = with_panels(); + state.view.mode = AppMode::FormulaPanel; + state.view.formula_cursor = 1; + let KeyOutcome::Local(enter) = resolve(&state, ev(KeyCode::Enter)) else { + panic!("Enter in the formula panel resolves here"); + }; + assert!(!enter.is_empty(), "Enter on a formula must do something"); + let clicked = click(&with_panels(), Click::FormulaRow(1)); assert!( - keymaps - .lookup(&ModeKey::FormulaPanel, KeyCode::Enter, KeyModifiers::NONE) - .is_none(), - "if FormulaPanel gains an Enter binding, click_formula_row must \ - activate too — the click mirrors Enter, and this is the only \ - thing tying the two together" - ); - assert!( - keymaps - .lookup(&ModeKey::ViewPanel, KeyCode::Enter, KeyModifiers::NONE) - .is_some(), - "and the view panel's click activates because its Enter does" + clicked.ends_with(&enter), + "the click's activation must be Enter's: {clicked:?} vs {enter:?}" ); } @@ -2120,7 +2160,7 @@ mod tests { "Net = Profit", ] .into_iter() - .map(str::to_string) + .map(crate::FormulaEntry::from) .collect(); state.views.views = ["Default", "Second", "Third"] .into_iter() diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 24ca5e93..79f8d5a5 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -377,8 +377,8 @@ fn formula_rows(state: &ClientState) -> Option> { .formulas .iter() .enumerate() - .map(|(i, text)| RenderFormulaRow { - text: text.clone(), + .map(|(i, entry)| RenderFormulaRow { + text: entry.raw.clone(), selected: i == state.view.formula_cursor, }) .collect() diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 294b3fc0..2ad83dd2 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1950,3 +1950,57 @@ fn every_projected_edit_text_commits_back_to_its_value() { } } } + +const FORMULAS: [&str; 3] = [ + "Profit = Revenue * 2", + "Tax = Revenue * 0.1", + "Margin = Profit / Revenue", +]; + +fn add_formulas(app: &mut App) { + for raw in FORMULAS { + let f = improvise_command::formula::parse_formula(raw, "_Measure").unwrap(); + app.model_state.workbook.model.add_formula(f); + } + app.rebuild_layout(); +} + +fn raws(app: &App) -> Vec { + let formulas = app.model_state.workbook.model.formulas(); + formulas.iter().map(|f| f.raw.clone()).collect() +} + +/// The formula panel's edit (improvise-km71), opened at a NONZERO cursor over +/// three formulas, typed into and committed, with both ends agreeing throughout. +#[test] +fn editing_a_formula_from_the_panel_agrees_with_the_host() { + let mut app = furnished_host(); + add_formulas(&mut app); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let mut twin = twin_of(&app); + add_formulas(&mut twin); + let mut client = furnished_client(&app, (2, 1)); + client.formulas = improvise_command::ui::project::formulas_of(&app); + + both_ends(&mut app, &mut twin, &mut client, "Enter"); + assert_eq!( + client.view.buffers.get("formula"), + app.view_state.buffers.get("formula"), + "the two ends seeded the editor differently" + ); + assert_eq!( + app.view_state.buffers.get("formula").map(String::as_str), + Some(FORMULAS[1]), + "Enter must seed the formula under cursor 1" + ); + for key in ["Backspace", "2", "Enter"] { + both_ends(&mut app, &mut twin, &mut client, key); + } + assert_eq!( + raws(&app), + vec![FORMULAS[0], "Tax = Revenue * 0.2", FORMULAS[2]], + "the commit must replace the formula it opened on, in place" + ); +} diff --git a/src/ui/formula_panel.rs b/src/ui/formula_panel.rs index 247eaa0e..e83f07f9 100644 --- a/src/ui/formula_panel.rs +++ b/src/ui/formula_panel.rs @@ -30,7 +30,7 @@ impl PanelContent for FormulaContent<'_> { } fn title(&self) -> &str { - " Formulas [n]ew [d]elete " + " Formulas [n]ew [e]dit [d]elete " } fn item_count(&self) -> usize { @@ -42,7 +42,7 @@ impl PanelContent for FormulaContent<'_> { } fn render_item(&self, index: usize, is_selected: bool, inner: Rect, buf: &mut Buffer) { - let raw = &self.formulas.formulas[index]; + let raw = &self.formulas.formulas[index].raw; let style = if is_selected { Style::default() .fg(Color::Black) @@ -79,6 +79,8 @@ impl PanelContent for FormulaContent<'_> { #[cfg(test)] mod tests { use super::*; + use crate::formula::parse_formula; + use crate::protocol::FormulaEntry; use crate::ui::app::App; use crate::ui::panel::Panel; use crate::workbook::Workbook; @@ -101,7 +103,10 @@ mod tests { /// improvise-8l0c — `context/test-evidence/tui.md`. fn render_formulas(raws: &[&str], width: u16) -> Buffer { let cache = FormulaCache { - formulas: raws.iter().map(|s| (*s).to_string()).collect(), + formulas: raws + .iter() + .map(|s| FormulaEntry::from(&parse_formula(s, "_Measure").unwrap())) + .collect(), }; let area = Rect::new(0, 0, width, 5); let mut buf = Buffer::empty(area); @@ -151,7 +156,12 @@ mod tests { .map(|f| f.raw.clone()) .collect(); assert_eq!(was.len(), 2, "fixture must carry formulas"); - assert_eq!(was, project::formulas_of(&app).formulas); + let cached: Vec = project::formulas_of(&app) + .formulas + .into_iter() + .map(|f| f.raw) + .collect(); + assert_eq!(was, cached); } /// Pins the drawn rows, so a change to how the cache is read shows up. From a91db1d85749f9c80ec9ca88e2b388ac156f0475 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:42:33 -0700 Subject: [PATCH 2/8] improvise-km71: edit the formula under the cursor, in place Enter or e in the formula panel opens FormulaEdit on the formula under the cursor, seeded with its text. The editor names the formula by a FormulaId captured when it opens, and commits through a new ReplaceFormula effect and ModelCommand. The edit keeps the formula's position, renames it in place when the target changes, and refuses out loud if the formula was removed or rewritten meanwhile, if the new target belongs to another formula, or if the text does not parse. A refusal leaves the editor open over the user's text. FormulaCache now carries each formula's id, so the browser composes the same edit, and a formula-row click opens the editor on the clicked row. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 50 +++++++++---- context/code/command.md | 41 +++++++++++ context/code/core.md | 51 +++++++++++++ context/code/protocol.md | 23 +++++- context/test-evidence/clients.md | 73 +++++++++++-------- context/test-evidence/command.md | 40 ++++++++++ .../src/command/cmd/commit.rs | 47 +++++++----- .../improvise-command/src/command/cmd/core.rs | 17 ----- .../src/command/cmd/registry.rs | 6 +- crates/improvise-command/src/ui/app.rs | 3 +- crates/improvise-command/src/ui/effect.rs | 5 +- crates/improvise-command/src/ui/project.rs | 20 +++-- crates/improvise-core/src/model/types.rs | 15 +++- crates/improvise-formula/src/ast.rs | 2 +- crates/improvise-protocol/src/lib.rs | 13 ++-- crates/improvise-protocol/src/render.rs | 5 +- crates/improvise-protocol/src/resolve.rs | 4 +- .../improvise-wasm-client/tests/lockstep.rs | 39 ++++++++++ harness/tests/contract.spec.js | 11 +-- 19 files changed, 354 insertions(+), 111 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index e88653f3..8005654a 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -995,20 +995,39 @@ folded when the resolver runs — the trap that reads as "the cursor lands on `Plan` and the document switches to `Default`", silent and wrong in the half a user does not watch. -The second clause of rule 1 is what makes the formula panel look inconsistent -and is in fact the rule holding. `ModeKey::ViewPanel` binds `Enter` to -`switch-view-at-cursor`; `ModeKey::FormulaPanel` binds `a`/`n`/`o` and -`d`/`Delete` and **no `Enter` at all**. So a view-row click switches and a -formula-row click only navigates. Opening the editor on a formula click would -be an activation *invented for the pointer that the keyboard does not have* — -a second derivation of what a formula row means, which is the disease §5 names. -**The guard for this must be aimed at the keymap, not at the click**: asserting -that the click emits no `FormulaEdit` only restates the author's reading, and a -reviewer demonstrated the gap by adding `Enter -> enter-mode formula-edit` to -`FormulaPanel` and watching the whole workspace stay green while the click and -the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does` -asserts the binding's absence and names `click_formula_row` as what must change -if it ever appears. +The second clause of rule 1 is why a formula-row click opens the editor. Until +improvise-km71, `ModeKey::FormulaPanel` bound `a`/`n`/`o` and `d`/`Delete` and +**no `Enter` at all**, so a formula-row click only navigated, and opening the +editor on a click would have been an activation *invented for the pointer that +the keyboard did not have*. km71 bound `Enter` (and `e`) to +`edit-formula-at-cursor`, so the rule now yields the editor on the clicked row, +and `click_formula_row` ends with `edit_formula_at(index)` exactly as +`click_view_row` ends with `switch_view_at(index)`. **The guard is aimed at what +`Enter` resolves to, not at the keymap and not at the click**: +`a_formula_row_click_ends_with_what_enter_does_on_that_row` resolves `Enter` at +cursor 1 and asserts the click on row 1, made with the cursor at 0, *ends with* +those commands. The earlier guard asserted only that `Enter` was unbound, which +tied the two by a comment; this one fails if either side changes without the +other, and it separates "reads the clicked index" from "reads the cursor", +because the two point at different rows. The superseded test was +`the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. + +Editing needs the client to name *which* formula, and `FormulaCache` used to +carry only the raw text, from which neither the target nor its category is +recoverable without reimplementing `parse_formula` on the client — the gap +`decision(improvise-e24v)` recorded for `delete-formula-at-cursor`. km71 closed +it the gotcha 47 way, by **enriching the projection**: `FormulaCache::formulas` +is `Vec`. The id is opaque on purpose +(`context/code/core.md` §14). `delete-formula-at-cursor` is still declined in +the browser; it now needs only its resolver. + +One divergence is pinned rather than closed: a rename. The host's +`ReplaceFormula` rewrites the status to say what was renamed and which formulas +still name the old target; the client cannot know the old target (the id is +opaque) and folds the optimistic "Formula updated". +`renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host` asserts +both sides of that, as `a_formula_the_client_cannot_parse_diverges_on_the_status_line` +does for a parse error. **The browser assertion that PR #60 dropped stays dropped, and improvise-qf57 is the decision rather than the oversight.** `#grid input` `toHaveCount(0)` was @@ -1025,6 +1044,9 @@ running the spec: `#grid input` `toHaveCount(0)` **passes**, and the run fails on the `#mode` assertion below it. Restoring it would add an assertion that cannot fail — the same defect this file already records one line down, where an explanatory `assert!` was removed for never being able to be the failing one. +Since improvise-km71 the activation is real rather than invented, so the spec's +`#mode` assertion reads `"FORMULA"` and it also asserts that `#minibuffer` holds +the clicked formula's text; the argument about `#grid input` is unchanged. **Rule 2 — a click is all of its intents or none of them** (improvise-hkr8). Each `click_*` returns `Option>` and `click` does the single diff --git a/context/code/command.md b/context/code/command.md index d6d2756f..d4415b62 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3391,3 +3391,44 @@ fixed default, not a clock read. The list is checked in both directions. An unlisted command that diverges fails the test, and so does a listed one that does not diverge. A stale entry therefore cannot linger, and a new registration is covered the day it lands. + +## Editing a formula: the capture is on the mode, read in `execute` (improvise-km71) + +`edit-formula-at-cursor` (Enter or `e` in the formula panel) opens +`AppMode::FormulaEdit` with `slot: FormulaSlot::Existing { id, was }` and seeds +the `formula` buffer with the formula's `raw`. That slot is the capture: which +formula, and what it read when the editor opened. Nothing at commit time looks +at the cursor again. + +**Where `commit-buffer formula` reads the slot matters, and the registry is the +wrong place.** The obvious move was to resolve it in `parse_commit_kind`, the +way `Item` takes its category from `AppMode::ItemAdd`. That does not work, and +it fails silently: the interactive arm is built **from `parse`** +(`context/code/cli.md` §8), and `parse` accepts `formula` under +`AppMode::Normal`, so the mode-reading fallback is never reached. `Item` only +works that way because `parse` *refuses* `item`. The first build of km71 did +exactly this, and every edit became an add. `add_formula` upserts by target, so +an edit that kept its target still landed in place, and only the rename tests +and the concurrency tests showed it. Making the interactive arm read the mode +first would break improvise-yf7p's guarantee that an accepted argument list is +never overridden by context. So `CommitBuffer::execute` asks +`CommitKind::in_mode(ctx.mode)`. That is a read of context in `execute`, which +is the answer yf7p gave to `toggle-panel-visibility`. A script line stays an +add, and cannot name an edit, because the slot does not exist outside the +editor. + +**`effect::ReplaceFormula` refuses with `refuse_and_stop`, not `refuse`.** The +Enter binding is `[commit-buffer formula, clear-buffer formula]`, and +`CommitBuffer` puts the change back to `FormulaPanel` *after* the mutation. A +refusal that stops the batch therefore leaves the editor open with the user's +text and the reason in the minibuffer line (`draw_bottom_bar` draws the status +beside the minibuffer). That covers all four refusals: the formula is gone, it +was rewritten since the editor opened (`was`), the new text does not parse, or +the new target belongs to another formula. `AddFormula`'s parse error only +`refuse`s, so a bad *new* formula still closes the editor and clears the text. +That asymmetry is improvise-v4hg's to close, not this ticket's. + +On success the command's optimistic "Formula updated" stands, except on a +rename, where the effect rewrites the status to name the old and new targets +and any formula that still mentions the old one (`context/code/core.md` §18). + diff --git a/context/code/core.md b/context/code/core.md index 3e209485..2f844f6d 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -1232,3 +1232,54 @@ What this does **not** fix: a text cell whose text reads as a number. `"007"` seeds `"007"`, and `typed()` classifies that as `Number(7.0)`. The seed is exact, but the classifier has no way to know the cell was text. That is improvise-ixxx. + +## 18. Which formula an edit means (improvise-km71) + +A `Formula` has no stored id. It lives in `Model::formulas: Vec` and is +identified, in practice and by `add_formula`'s upsert, by `(target, +target_category)`. **`FormulaId` is that pair behind one opaque type**: its +fields are private, `Formula::id()` is its only constructor, and nothing +outside `improvise-formula` can read a name out of it. That is deliberate. The +stable-identity work (improvise-sboz, ADR 005) will make a formula's identity +something other than its names, and every signature that already says +`FormulaId` — `Model::formula`, `Model::replace_formula`, +`ModelCommand::ReplaceFormula`, `FormulaSlot::Existing`, `FormulaEntry` — then +changes by changing this one struct. A name pair spread across those signatures +would have made the swap a sweep. + +**The index is not a candidate.** A position is what `formula_cursor` is, and +the editor must not recompute its target from the cursor at commit +(improvise-ixxx/2wu3's lesson): another client deleting an earlier formula +shifts every index after it, and the commit would land on a neighbour. It would +also put a position on the wire, which `every_model_command_field_is_position_free` +makes a compile error. + +**An edit is its own operation, never remove-then-add and never +`add_formula`.** `add_formula` upserts on `(target, target_category)`: fed an +edit that changes the target, it appends a second formula and leaves the old +one; fed an edit onto another formula's target, it silently replaces *that* +formula. `Model::replace_formula(id, formula)` writes into the slot `id` names, +so the position (and with it `_Measure`'s item order, since +`measure_item_names` appends formula targets in formula order) is kept, and it +**refuses** when the new target belongs to a different formula. + +**A target change is a rename of the same formula.** The id travels with the +edit, so `Profit = …` edited to `Gross = …` is the formula that was `Profit`, +in the same place. That is the least surprising reading of "edit this row", and +it is what ADR 005 proposes. What a rename cannot carry under name identity is +everything else that names the old target: other formulas' references, a +view's hidden list, a `measure_agg` entry. Those cannot follow without rewriting +`raw` text the parser does not print back, so the edit says so out loud: +`ReplaceFormula` reports which formulas still mention the old name +(`Formula::mentions`, over the expression and its `WHERE`), and their cells +show the `ref` error rather than a silent wrong number. Under a stable id those +references are what sboz makes follow. + +**Concurrency is checked twice.** `FormulaSlot::Existing { id, was }` is +captured when the editor opens: the id must still resolve, and the formula must +still read `was`. Either failing means another client removed, renamed or +rewrote it meanwhile, and the edit is refused with the batch stopped, so the +editor stays open over the user's text (gotcha 21). `was` is not redundant +with a stable id: a concurrent *replace* keeps the id and changes the text, +which only `was` can see. + diff --git a/context/code/protocol.md b/context/code/protocol.md index c79f0e56..7fd79e2e 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -84,6 +84,20 @@ naming it would produce a mode with nothing to add to. `Quit` and `start-import-wizard` — which do work beyond changing the mode, so `enter-mode` would be a way to reach half of each. +### `AppMode::FormulaEdit` carries what it will commit to (improvise-km71) + +`FormulaEdit { minibuf, slot }` is one mode for two editors. `slot` is +`FormulaSlot::New` for `a`/`n`/`o` (`enter-mode formula-edit`, which +`from_name` still builds) or `FormulaSlot::Existing { id, was }` for +`edit-formula-at-cursor`. It is one variant and not a second `AppMode` because +the keys, the buffer and the panel are identical: a second variant would add a +`ModeKey`, a keymap and a hint for no difference in what the keys do. The slot +is `ItemAdd`'s `category` again, a capture made when the mode is entered. Like +`ItemAdd` it is not nameable from `from_name`, since a script line has no +formula to open. It must be portable and snapshot-safe (§1 above). `FormulaId` +is plain serde data, and a fresh client that inherits an open edit inherits +exactly the claim the editor made. The commit re-checks that claim. + ### `Panel` `Panel::ALL`/`name()`/`key()` drive the toggle keys for Normal *and* all @@ -164,9 +178,12 @@ attempt in a fourth place is the disease. - The **model half** is `ModelCommand`: every `EffectKind::Model(..)` effect, one variant apiece, carrying that effect's own fields (improvise-kmgs). -The count is 29 model effects, minus three: `SwitchView`, which `Command` -already names and routes, and `ApplyAndClearDrill`/`AcceptWizardModel`, which -are **not addressable** (improvise-3x3u). That leaves **26**. A variant with +The count is 27 model effects, minus two: `ApplyAndClearDrill` and +`AcceptWizardModel`, which are **not addressable** (improvise-3x3u). That +leaves **25**. `SwitchView`, `ViewBack` and `ViewForward` are no longer model +effects at all (improvise-jytm), so they are not subtracted. This paragraph +said "29 minus three, 26" until improvise-km71 added `ReplaceFormula`, and +that figure was already stale: the enum had 24. A variant with no data mirrors a nullary effect whose operand is document state the authoritative `App` already holds. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 664107f6..863f8947 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1485,39 +1485,29 @@ view, or with a click on the row already under the cursor, the bug is invisible — the same reason `a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row` clicks Product rather than `_Index`. -### a_formula_row_click_navigates_and_activates_nothing +### a_formula_row_click_navigates_and_opens_the_editor_on_that_row -The negative arm, and it is a statement about the *rule* rather than about the -formula panel. The rule is "go there, then do what `Enter` does there"; -`ModeKey::FormulaPanel`'s keymap binds `a`/`n`/`o` to `enter-mode formula-edit` -and `d`/`Delete` to delete, and **no `Enter` at all**. So the rule yields -navigation and nothing else, and that is the rule applying rather than the fix -stopping half way. +Formerly `a_formula_row_click_navigates_and_activates_nothing`. That was the +negative arm of the rule "go there, then do what `Enter` does there", true while +`ModeKey::FormulaPanel` bound no `Enter`. improvise-km71 bound `Enter` (and +`e`) to `edit-formula-at-cursor`, so the same rule now yields the editor on the +clicked row. The test asserts the exact command vector. The clicked row is +`Margin` at index 1 and the cursor stays at 0, so a click that read the cursor +would open `Profit`. -This test asserts the *exact* command vector, which is the whole of what it can -say. It once carried a second, explanatory `assert!` that no `FormulaEdit` mode -change appears — **which could never have been the failing assertion**, because -the `assert_eq!` above it pins the same three commands and aborts first. A -reviewer demonstrated it by injecting a `ChangeMode(FormulaEdit)` and watching -the panic land on the `assert_eq!` with the explanatory message never printed. -It was a comment wearing an `assert!`, and the argument now lives in -`context/code/clients.md` where a comment run cannot hold it. +The old version once carried an explanatory `assert!` that no `FormulaEdit` +mode change appeared, which could never have been the failing assertion, +because the `assert_eq!` above it pinned the same commands and aborted first. +That lesson stands: an `assert!` after an exact `assert_eq!` on the same vector +is a comment. -### the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does - -The guard that the removed assertion was pretending to be. The asymmetry — a -view-row click activates, a formula-row click does not — is a fact about the -**keymap**, and nothing tied the click to it. The reviewer proved the gap by -adding `Enter -> enter-mode formula-edit` to `ModeKey::FormulaPanel` and running -the whole workspace: **1381 passing, nothing failed**, while `click_formula_row` -still activated nothing. The click and the keyboard then disagree about what a -formula row does, silently. - -Injecting that same binding now fails this test and only this test. The failure -message names `click_formula_row` as what must change, because the person who -adds an `Enter` binding is not the person reading `resolve.rs`'s click arms. The -positive half (`ViewPanel` *does* bind `Enter`) is there so the test cannot pass -by asserting that no panel binds anything. +The guard that tied the click to the keymap was +`the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. A reviewer +showed that nothing else tied them: adding an `Enter` binding to `FormulaPanel` +left 1381 tests passing while `click_formula_row` still activated nothing. That +test is replaced by `a_formula_row_click_ends_with_what_enter_does_on_that_row`, +below, which compares the click with what `Enter` actually resolves to rather +than asserting a binding's absence. #### The view and formula row clicks in the browser (improvise-iioi) @@ -4640,3 +4630,26 @@ it fails naming `End` and `$`. The general point: the fixture that made the laundering invisible (the edge) is also the one that makes a wrong-direction jump visible, so moving off the edge fixed one blind spot and opened the other, and each needs its own question. + +### a_formula_row_click_ends_with_what_enter_does_on_that_row + +improvise-km71, replacing `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`. +The old guard asserted that `Enter` was unbound, which tied the click to the +keyboard only by the message in its `assert!`. This one resolves `Enter` at +cursor 1 and asserts that the click on row 1, made with the cursor at 0, ends +with the same commands. Rival: `click_formula_row` activates at +`state.view.formula_cursor` instead of the clicked index. This test and +`a_formula_row_click_navigates_and_opens_the_editor_on_that_row` both fail. The +cursor and the index differ in the fixture, which is what lets them fail. + +### renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host + +improvise-km71. `editing_a_formula_from_the_panel_agrees_with_the_host` alone +cannot separate a browser that composes `ReplaceFormula` from one that composes +`AddFormula`: the upsert lands a same-target edit in place on the twin too. +Rival: `CommitKind::of` ignores the slot. Both lockstep tests fail. The edit +test fails only on the status line ("Formula added" against "Formula +updated"). This test fails on the **document**, which is the property. It also +pins a real divergence: on a rename the host says what was renamed and the +client cannot, because `FormulaId` is opaque and it does not know the old +target. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 9b60a2e9..cb00ae9c 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5459,3 +5459,43 @@ one context, which is why the old single-context law was vacuous, and differ across two independently built ones. If a future refactor made the fixture share one map, or share one `RandomState`, the second assertion fails. Without this test the law would then pass everything and look healthy. + +### changing_the_target_renames_the_formula_in_its_own_place + +improvise-km71. This is the test that separates "edit" from "add". The rival +was the first build: `commit-buffer formula` was resolved in `parse_commit_kind`, +where the registry's parse-first rule never reaches the mode, so every edit +became an `AddFormula`. `committing_an_edit_replaces_the_formula_in_place` +**passed** under that rival, because `add_formula` upserts by +`(target, target_category)` and an edit that keeps its target lands in the same +slot. Injected on purpose (the commit emits `AddFormula` for the replace kind): +this test, the rename-status test, the collision test, the parse-error test and +both concurrency-refusal tests fail, and the in-place test passes. A rename is the input the upsert cannot fake. The fixture has three +formulas with the cursor on index 1, so "the one under the cursor" is not also +"the first" or "the last". + +### an_edit_lands_on_its_formula_after_an_earlier_one_is_deleted_meanwhile + +improvise-km71. The capture is made when the editor opens and never re-read +from the cursor. Rival: `ReplaceFormula` resolves its formula from +`formula_cursor` at commit. The fixture deletes `Profit` (index 0) while the +editor is open on `Tax` (index 1), so cursor 1 now names `Margin`. The rival +rewrote `Margin`, and this test fails, along with +`an_edit_whose_formula_was_deleted_meanwhile_refuses_out_loud` and +`an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud`. The other nine +formula-edit tests pass under it, because none of them moves the list. + +### an_edit_whose_formula_was_changed_meanwhile_refuses_out_loud + +improvise-km71. Another client rewrote `Tax`'s text but kept its target, so the +id still resolves. Only the `was` check can see this. Rival: the `was` filter +deleted. This test is the only one that fails. That is the case for keeping +`was` after improvise-sboz gives formulas a stable id: a concurrent replace +keeps the id. + +### a_rename_onto_another_formulas_target_refuses_and_keeps_both + +improvise-km71. Rival: `Model::replace_formula`'s collision check disabled. +This test and `replace_formula_refuses_to_displace_another_formula` fail. +Without the check, renaming `Tax` to `Margin` leaves two formulas with one +identity, and `formula(&id)` then finds the first. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index b9b85bb0..6894cb30 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -808,8 +808,8 @@ impl Cmd for CommitAndAdvance { #[derive(Debug, Clone, PartialEq, Eq)] pub enum CommitKind { Formula, - /// Edit the formula the editor was opened on, read off - /// [`AppMode::FormulaEdit`]'s slot at construction (improvise-km71). + /// Edit the formula the editor was opened on. Never parsed: `execute` reads + /// it off the mode's slot, the capture made at open (improvise-km71). ReplaceFormula { id: FormulaId, was: String, @@ -822,20 +822,10 @@ pub enum CommitKind { }, } -/// Resolve a `commit-buffer` argument against the current mode. `Item` and a -/// formula edit need the mode, so a script line can build only the others. +/// Resolve a `commit-buffer` argument against the current mode. Only `Item` needs +/// the mode, so a script line can still build the other two. pub fn parse_commit_kind(arg: &str, mode: &AppMode) -> Result { match (arg, mode) { - ( - "formula", - AppMode::FormulaEdit { - slot: FormulaSlot::Existing { id, was }, - .. - }, - ) => Ok(CommitKind::ReplaceFormula { - id: id.clone(), - was: was.clone(), - }), ("formula", _) => Ok(CommitKind::Formula), ("category", _) => Ok(CommitKind::Category), ("item", AppMode::ItemAdd { category, .. }) => Ok(CommitKind::Item { @@ -847,6 +837,24 @@ pub fn parse_commit_kind(arg: &str, mode: &AppMode) -> Result CommitKind { + match (self, mode) { + ( + Self::Formula, + AppMode::FormulaEdit { + slot: FormulaSlot::Existing { id, was }, + .. + }, + ) => Self::ReplaceFormula { + id: id.clone(), + was: was.clone(), + }, + (kind, _) => kind.clone(), + } + } + /// The minibuffer this kind consumes. pub fn buffer(&self) -> &'static str { match self { @@ -916,18 +924,19 @@ impl Cmd for CommitBuffer { "commit-buffer" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let raw = read_buffer(ctx, self.kind.buffer()); + let kind = self.kind.in_mode(ctx.mode); + let raw = read_buffer(ctx, kind.buffer()); let name = raw.trim(); if name.is_empty() { - return vec![effect::change_mode(self.kind.on_empty())]; + return vec![effect::change_mode(kind.on_empty())]; } let mut effects: Vec> = vec![ - effect::set_status(self.kind.status(name)), + effect::set_status(kind.status(name)), // The add goes last: it is the only party that knows whether it worked, // so the optimistic status must not clobber its error (improvise-6t3). - self.kind.add_effect(name), + kind.add_effect(name), ]; - effects.extend(self.kind.on_commit().map(effect::change_mode)); + effects.extend(kind.on_commit().map(effect::change_mode)); effects } } diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 867acb6e..9944f2de 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -236,23 +236,6 @@ impl CmdRegistry { }); } - /// Register a command whose interactive arm ALWAYS reads the context, and - /// whose text arm is `parse` — for a mode that carries a capture (km71). - pub fn register_contextual( - &mut self, - prototype: &C, - parse: fn(&[String]) -> Result, - interactive: FallbackFn, - ) { - self.entries.push(CmdEntry { - name: prototype.name(), - parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), - interactive: Box::new(move |args, ctx| { - interactive(args, ctx).map(|c| Box::new(c) as Box) - }), - }); - } - /// Register a command that needs no interactive context: BOTH arms are /// `parse`, so neither can demote the other — `context/code/command.md`. pub fn register_pure(&mut self, prototype: &dyn Cmd, parse: ParseFn) { diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 25a3c98b..802912fb 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -467,7 +467,7 @@ pub fn default_registry() -> CmdRegistry { |_| Err("commit-and-advance-right requires context".into()), Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); - r.register_contextual( + r.register( &CommitBuffer { kind: CommitKind::Category, }, @@ -476,11 +476,11 @@ pub fn default_registry() -> CmdRegistry { let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; Ok(CommitBuffer { kind }) }, - |args, ctx| { + Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; Ok(CommitBuffer { kind }) - }, + }), ); r.register_nullary(|| Box::new(CommitExport)); r.register_nullary(|| Box::new(CommitSave)); diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index e6fe3ee1..7eaaeda2 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -4340,7 +4340,8 @@ mod formula_edit_tests { wb.add_item("_Measure", "Revenue").unwrap(); wb.add_item("_Measure", "Cost").unwrap(); for raw in [PROFIT, TAX, MARGIN] { - wb.model.add_formula(parse_formula(raw, "_Measure").unwrap()); + wb.model + .add_formula(parse_formula(raw, "_Measure").unwrap()); } let mut app = App::new(wb, None); app.view_state.formula_panel_open = true; diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 629c7ba9..d74326c6 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -296,7 +296,10 @@ fn renamed(model: &Model, old: &str, new: &str) -> String { .collect(); match still.is_empty() { true => format!("Renamed {old} to {new}"), - false => format!("Renamed {old} to {new}; {old} is still used by {}", still.join(", ")), + false => format!( + "Renamed {old} to {new}; {old} is still used by {}", + still.join(", ") + ), } } diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 7f6ad873..ddaa5256 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -15,9 +15,9 @@ use improvise_io::import::analyzer::{DateComponent, FieldKind}; use improvise_io::import::wizard::{ImportWizard, WizardStep}; use improvise_protocol::{ AxisFrame, Basis, BasisCache, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, - CellId, Command, CreateKind, DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, - GridExtent, GridUpdate, Projection, RecordCell, Rejection, Snapshot, TileBarCache, TileEntry, - ViewListCache, ViewState, ViewSummary, WidgetCache, WizardCache, WizardDateField, + CellId, Command, CreateKind, DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, + GridCache, GridExtent, GridUpdate, Projection, RecordCell, Rejection, Snapshot, TileBarCache, + TileEntry, ViewListCache, ViewState, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, WizardProposal, WizardStepKind, basis, reduce_view, }; @@ -1743,7 +1743,12 @@ mod tests { client.fold(&projections); } assert_eq!( - client.formulas.formulas.iter().map(|f| f.raw.as_str()).collect::>(), + client + .formulas + .formulas + .iter() + .map(|f| f.raw.as_str()) + .collect::>(), vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); @@ -1783,7 +1788,12 @@ mod tests { let mut client = ClientCaches::default(); client.fold(&snapshot.seed); assert_eq!( - client.formulas.formulas.iter().map(|f| f.raw.as_str()).collect::>(), + client + .formulas + .formulas + .iter() + .map(|f| f.raw.as_str()) + .collect::>(), vec!["Total = Revenue"], "the client must be holding the formula before it can lose it" ); diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 30d671b9..e3772f1d 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -422,7 +422,7 @@ impl Model { } /// Put `formula` where `id` is, keeping its position; refuses rather than - /// let a new target displace another formula — `context/code/core.md` §14. + /// let a new target displace another formula — `context/code/core.md` §18. pub fn replace_formula(&mut self, id: &FormulaId, formula: Formula) -> Result<()> { let new_id = formula.id(); if new_id != *id && self.formula(&new_id).is_some() { @@ -1888,7 +1888,11 @@ mod formula_tests { fn three_formulas() -> Model { let mut m = revenue_cost_model(); - for raw in ["Profit = Revenue - Cost", "Tax = Revenue * 0.1", "Margin = Profit / Revenue"] { + for raw in [ + "Profit = Revenue - Cost", + "Tax = Revenue * 0.1", + "Margin = Profit / Revenue", + ] { m.add_formula(parse_formula(raw, "_Measure").unwrap()); } m @@ -1902,8 +1906,11 @@ mod formula_tests { fn replace_formula_renames_in_place_and_evaluates_under_the_new_name() { let mut m = three_formulas(); let id = m.formulas[1].id(); - m.replace_formula(&id, parse_formula("Levy = Revenue * 0.5", "_Measure").unwrap()) - .unwrap(); + m.replace_formula( + &id, + parse_formula("Levy = Revenue * 0.5", "_Measure").unwrap(), + ) + .unwrap(); assert_eq!(targets(&m), vec!["Profit", "Levy", "Margin"]); let levy = coord(&[("_Measure", "Levy"), ("Region", "East")]); assert_eq!(m.evaluate(&levy).and_then(|v| v.as_f64()), Some(500.0)); diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 7ef59693..1022ea25 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -103,7 +103,7 @@ impl Formula { } /// Which formula an edit means, opaque so its content can change in one place: -/// today a target and its category — `context/code/core.md` §14. +/// today a target and its category — `context/code/core.md` §18. #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct FormulaId { target: String, diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 878b59a2..e9e6116d 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -30,10 +30,10 @@ pub use keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey}; pub use reduce::{Command, HelpPage, ModelCommand, records_named, reduce_view}; pub use render::{ AxisFrame, CategoryRow, CategoryTreeCache, CategoryTreeUpdate, CellDisplay, CellId, CreateKind, - DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, GridUpdate, - MAX_ROW_HEADER_W, MIN_ROW_HEADER_W, Projection, RecordCell, Snapshot, TileBarCache, TileEntry, - ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, - WizardProposal, WizardStepKind, header_level_widths, + DocumentCache, FormulaCache, FormulaEntry, FrameEntry, FrameGroup, GridCache, GridExtent, + GridUpdate, MAX_ROW_HEADER_W, MIN_ROW_HEADER_W, Projection, RecordCell, Snapshot, TileBarCache, + TileEntry, ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, + WizardFieldKind, WizardProposal, WizardStepKind, header_level_widths, }; pub use resolve::{ Click, KeyOutcome, Request, click, read_buffer, resolve, typed, typed_cell_command, @@ -186,7 +186,10 @@ pub enum AppMode { pub enum FormulaSlot { New, /// Replace the formula `id` names, provided it still reads `was`. - Existing { id: FormulaId, was: String }, + Existing { + id: FormulaId, + was: String, + }, } /// `Normal` and `RecordsNormal` are one mode over two layouts, so the mode an diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 65e758a3..e2e324ad 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -1509,7 +1509,10 @@ mod tests { fn the_list_caches_hold_the_last_update() { let formulas = FormulaCache::folded(&[ vec!["Profit = Revenue - Cost".into()], - vec!["Profit = Revenue - Cost".into(), "Tax = Revenue * 0.1".into()], + vec![ + "Profit = Revenue - Cost".into(), + "Tax = Revenue * 0.1".into(), + ], ]); assert_eq!(formulas.formulas.len(), 2); diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 3a72af64..f63bb798 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -1280,8 +1280,8 @@ fn parse_axis(s: &str) -> Option { } } -/// A mirror of `commit.rs`'s `CommitKind`, resolved against the mode exactly -/// as `parse_commit_kind` does — `Item` and a formula edit need the mode. +/// A mirror of `commit.rs`'s `CommitKind`, resolved against the mode as +/// `parse_commit_kind` then `CommitKind::in_mode` do; there is no registry here. enum CommitKind { Formula, ReplaceFormula { id: FormulaId, was: String }, diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 2ad83dd2..3bc41dbd 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -2004,3 +2004,42 @@ fn editing_a_formula_from_the_panel_agrees_with_the_host() { "the commit must replace the formula it opened on, in place" ); } + +/// A rename through the browser: the one input that separates "replace" from +/// "add", since `add_formula` upserts by target. The status diverges on purpose. +#[test] +fn renaming_a_formula_from_the_panel_lands_the_same_document_as_the_host() { + let mut app = furnished_host(); + add_formulas(&mut app); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let mut twin = twin_of(&app); + add_formulas(&mut twin); + let mut client = furnished_client(&app, (2, 1)); + client.formulas = improvise_command::ui::project::formulas_of(&app); + both_ends(&mut app, &mut twin, &mut client, "Enter"); + for view in [&mut app.view_state, &mut twin.view_state, &mut client.view] { + view.buffers + .insert("formula".into(), "Levy = Revenue * 0.1".into()); + } + + let event = BrowserKey::new("Enter").to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("Enter in the formula editor resolves here"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut twin, &cmds); + reduce_key(&mut app, event); + + assert_eq!(document_text(&twin), document_text(&app)); + assert_eq!( + raws(&app), + vec![FORMULAS[0], "Levy = Revenue * 0.1", FORMULAS[2]] + ); + assert_eq!(client.view.mode, app.view_state.mode); + assert_eq!(client.view.status_msg, "Formula updated"); + assert_eq!(app.view_state.status_msg, "Renamed Tax to Levy"); +} diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 718a4121..8c7e2faa 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -250,15 +250,16 @@ test.describe("a pointer event reaches the client and its answer is painted", () await expect(page.locator("#views li.active")).toContainText("Season"); await expect(page.locator("#views li.selected")).toContainText("Season"); - // A formula row: navigation only, because FormulaPanel binds no Enter and - // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. - // `#grid input` toHaveCount(0) belongs here and is deliberately absent: - // it cannot fail, measured by injection (improvise-qf57, clients.md). + // A formula row: FormulaPanel's Enter edits the formula (improvise-km71) + // and a click mirrors Enter, so the click opens the editor on that row. + // `#grid input` toHaveCount(0) is deliberately absent (improvise-qf57). await page.keyboard.press("Shift+F"); await expect(page.locator("#formulas li").first()).toBeVisible(); await page.keyboard.press("Escape"); + const formula = await page.locator("#formulas li").first().innerText(); await page.locator("#formulas li").first().click(); - await expect(page.locator("#mode")).toHaveText("FORMULAS"); + await expect(page.locator("#mode")).toHaveText("FORMULA"); + await expect(page.locator("#minibuffer")).toContainText(formula.trim()); await expect(page.locator("#formulas li.selected")).toHaveCount(1); }); From 531cf4954c58618f28482cb54ef41aa8f69fedf6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:57:04 -0700 Subject: [PATCH 3/8] improvise-kb1d: refuse renaming a formula outside _Measure A regular category's formula target is a real item, and replace_formula added the new target without removing the old, stranding it as an empty item and appending the new one. Until formulas have a stable identity (improvise-sboz), a target change outside _Measure is refused and the editor stays open. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/app.rs | 35 ++++++++++++++++++++++++ crates/improvise-core/src/model/types.rs | 30 +++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 7eaaeda2..5ac40a44 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -4541,4 +4541,39 @@ mod formula_edit_tests { vec![PROFIT, TAX, MARGIN, "Levy = Revenue * 0.3"] ); } + + /// improvise-kb1d: a Region-targeted formula at index 1. Renaming it would + /// strand `Total` as an empty item, so the edit refuses and stays open. + #[test] + fn renaming_a_formula_outside_measure_refuses_and_keeps_the_editor() { + let mut wb = Workbook::new("T"); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").unwrap(); + for (raw, cat) in [ + (PROFIT, "_Measure"), + ("Total = East + West", "Region"), + (MARGIN, "_Measure"), + ] { + wb.model.add_formula(parse_formula(raw, cat).unwrap()); + } + let mut app = App::new(wb, None); + app.view_state.formula_panel_open = true; + app.view_state.mode = AppMode::FormulaPanel; + app.view_state.formula_cursor = 1; + let regions = |app: &App| { + let region = app.model_state.workbook.model.category("Region").unwrap(); + region.ordered_item_names().iter().map(|s| s.to_string()).collect::>() + }; + let before = regions(&app); + open_editor(&mut app); + retype(&mut app, "Sum = East + West"); + press(&mut app, KeyCode::Enter); + assert_eq!(raws(&app), vec![PROFIT, "Total = East + West", MARGIN]); + assert_eq!(regions(&app), before, "no ghost item, nothing appended"); + assert!(in_editor(&app), "{:?}", app.view_state.mode); + assert_eq!(buffer(&app), Some("Sum = East + West")); + } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index e3772f1d..f48f24ef 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -425,7 +425,14 @@ impl Model { /// let a new target displace another formula — `context/code/core.md` §18. pub fn replace_formula(&mut self, id: &FormulaId, formula: Formula) -> Result<()> { let new_id = formula.id(); - if new_id != *id && self.formula(&new_id).is_some() { + let renames = new_id != *id; + // A regular category's target is a real item; renaming it is sboz's (kb1d). + if renames && !CategoryKind::of_name(&formula.target_category).is_measure() { + return Err(anyhow!( + "Renaming a formula outside _Measure is not supported yet; delete and re-add it" + )); + } + if renames && self.formula(&new_id).is_some() { return Err(anyhow!("A formula for {} already exists", formula.target)); } let pos = self @@ -1935,6 +1942,27 @@ mod formula_tests { assert_eq!(targets(&m), vec!["Profit", "Margin"]); } + /// improvise-kb1d: a Region-targeted formula at index 1. A rename would + /// leave `Total` as an empty item and append `Sum`, so it is refused. + #[test] + fn replace_formula_refuses_to_rename_a_target_outside_measure() { + let mut m = three_formulas(); + m.formulas.insert(1, parse_formula("Total = East + West", "Region").unwrap()); + let shape = |m: &Model| { + let region = m.category("Region").unwrap().ordered_item_names(); + let owned: Vec = region.iter().map(|s| s.to_string()).collect(); + (targets(m).iter().map(|s| s.to_string()).collect::>(), owned) + }; + let before = shape(&m); + let id = m.formulas[1].id(); + let sum = parse_formula("Sum = East + West", "Region").unwrap(); + assert!(m.replace_formula(&id, sum).is_err()); + assert_eq!(shape(&m), before, "nothing moved, nothing added"); + let edit = parse_formula("Total = East - West", "Region").unwrap(); + m.replace_formula(&id, edit).unwrap(); + assert_eq!(targets(&m), vec!["Profit", "Total", "Tax", "Margin"]); + } + #[test] fn a_formula_mentions_what_its_expression_or_its_where_names() { let margin = parse_formula("Margin = Profit / Revenue", "_Measure").unwrap(); From 77a38255762b70f503a20eec9bd424633eb53c2a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:01:41 -0700 Subject: [PATCH 4/8] improvise-wdmp: the asker's own placement survives the reset its reframe earned A browser frame that reframes and then places the cursor was folded whole by the client before the relayed ResetCursor arrived, so the reset undid the placement while the mirror (reset, then placed, in frame order) kept it. Host::mutating_frame now relays to the asker every later cursor-placing command of the frame after its reset, so the client ends where its mirror and the TUI do. Excluding the asker from the reset was rejected: a frame that places nothing would reset the mirror silently (gotcha 24). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 28 ++++++- context/test-evidence/command.md | 24 ++++++ crates/improvise-command/src/ui/host.rs | 97 ++++++++++++++++++++++++- 3 files changed, 145 insertions(+), 4 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 60ac7de9..bc294cce 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2274,9 +2274,31 @@ would be sent and every client would stay stale *permanently* rather than until next scroll: a partial fix strictly worse than none. Broadcasting only would leave the server projecting each client a window it is no longer looking at. -The audience is `Broadcast::ToAll`, and the asker is not an exception. Its mirror -was reset here like everyone else's, and it folded nothing optimistically, because -a `Command::Model` is not something a client can apply to itself. +The audience is every session on a reframed view, each addressed with +`Broadcast::To` (improvise-nynk), and the asker is not an exception: a transpose +invalidates its own position too. What an earlier version of this paragraph got +wrong is the claim that the asker "folded nothing optimistically". A +`Command::Model` is not something a client can apply to itself, but the **rest of +its frame** is, and a browser frame routinely places the cursor *after* the +mutation. The `+cat` commit is `[ChangeMode, AddCategory, SetSelected]`. The +mirror folds that in order: reset, then placed, which is exactly the order the TUI +applies the same batch in. The client folded all of it **before** hearing the +reset, so the relayed `ResetCursor` arrived last and threw the placement away. +The client sat at the origin while its mirror, and every projection built from +it, sat at the placement (improvise-wdmp). + +So the host answers the asker in the order the mirror saw. Once the asker has been +told `ResetCursor`, `Host::mutating_frame` relays back to it every **later** +cursor-placing command of the same frame (`SetSelected`, `SetRowOffset`, +`SetColOffset`), ahead of that command's projections. All three are absolute, so +folding them twice is idempotent. Excluding the asker from the reset, which the +ticket proposed, was rejected. For a frame that places nothing after the reframe +it is gotcha 24's own "strictly worse than nothing": the mirror is reset and the +client never hears it. `a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed` +runs both shapes. One residue is stated rather than handled: a frame that +reframes and then *switches views* before placing would reset the view the +client ends on rather than the one it reframed. No resolver composes that frame +today. What is *not* broadcast is a position: another client's transpose must not move where I am looking — that is UI and it is mine (improvise-g0f). It may only declare diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index a4c92338..387f3cd1 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5697,3 +5697,27 @@ host's `Normal`, and nothing else different. `install_workbook` needs no change of its own, because both of its callers (`LoadModel`, `AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. +### a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed + +improvise-wdmp. A browser frame that reframes the grid and then places the cursor, +for example the records `+cat` commit `[ChangeMode, AddCategory, SetSelected]`, is +folded by the client all at once and before any answer arrives. The host's mirror +folds the same frame in order. `adopt_cursor_epoch` resets the mirror at the +mutation, and the later `SetSelected` places it. The relayed `ResetCursor` +reached the client **after** its own placement and undid it. So the client sat at +the origin while its mirror, and every projection built from it, sat at the +placement. That is a split, not merely the jump the ticket described. The test +drives a real `Host` and a real `ClientState` through `fold_outgoing`, then folds +every frame addressed to the asker. It seats the cursor off the origin first, so +a reset that never arrived is visible. It runs two shapes: with a placement +after the reframe (expect `(1, 0)`), and without one (expect `(0, 0)`, gvnb's +reset). In both, the client's cursor must equal its mirror's. At the base the +placed case failed: the client was at `(0, 0)` and the mirror at `(1, 0)`. + +Two rivals. First, the echo disabled, which is the pre-fix behaviour: the placed +case fails. Second, the ticket's suggested fix, which drops the asker's +`ResetCursor` from the relay: the unplaced case fails, because the mirror was +reset and the client never heard. That rival also fails +`a_geometry_command_relays_the_cursor_reset_to_the_asker_too` and the +ws-server's `the_other_document_would_also_have_a_cursor_to_invalidate`. + diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 78aac189..6efb7eaf 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -162,6 +162,7 @@ impl Host { let undo = self.app.snapshot(); let mirrors = self.sessions.clone(); let mut out = Vec::new(); + let mut asker_reset = false; for cmd in batch { if let Some(reason) = reduce::broken_chain(&self.app, cmd.command()) { return self.undo_frame(id, undo, mirrors, reason); @@ -169,7 +170,23 @@ impl Host { // Only a mutation runs `apply_effects`, which resets both flags — so // after anything else they are STALE. A refusal need not abort (cz8d). let mutation = matches!(cmd.command(), Command::Model { .. }); - out.extend(self.command(id, cmd)); + let command = cmd.command(); + let mut answered = self.command(id, cmd); + // The asker folded this placement BEFORE hearing its reset, so it is owed + // again after it (improvise-wdmp, `context/code/command.md`). + if asker_reset && places_cursor(command) { + answered.insert( + 0, + Addressed { + to: id, + msg: ServerFrame::Commands(vec![command.clone()]), + }, + ); + } + asker_reset |= answered + .iter() + .any(|a| a.to == id && is_cursor_reset(&a.msg)); + out.extend(answered); let failed = self.app.last_batch_refused() || self.app.last_batch_aborted(); if mutation && failed { return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); @@ -266,6 +283,19 @@ impl Host { } } +/// A command that writes this client's cursor, and so survives a reset only if +/// it is folded after it. +fn places_cursor(cmd: &Command) -> bool { + matches!( + cmd, + Command::SetSelected { .. } | Command::SetRowOffset(_) | Command::SetColOffset(_) + ) +} + +fn is_cursor_reset(msg: &ServerFrame) -> bool { + matches!(msg, ServerFrame::Commands(cmds) if cmds.contains(&Command::ResetCursor)) +} + /// One file per way `ImportJsonHeadless` can fail, plus one that imports; the /// `absent.*` paths are left unwritten. Shared by the frame and key-path tests. #[cfg(test)] @@ -1452,6 +1482,71 @@ mod tests { } } + /// improvise-wdmp: the asker's own frame placed its cursor after the reframe, + /// and the relayed reset undid it. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed() { + use improvise_protocol::{AppMode, ClientState}; + + // With a placement after the reframe, and without one: the asker ends where + // the TUI would, and its client agrees with its mirror either way. + for (placed, expected) in [(true, (1, 0)), (false, (0, 0))] { + let mut host = host(); + let (asker, snapshot) = host.connect(10, 10); + let (bystander, _) = host.connect(10, 10); + let mut client = ClientState::default(); + match snapshot.msg { + ServerFrame::Snapshot(s) => client.bootstrap(*s), + other => panic!("setup: connect answers a Snapshot, got {other:?}"), + } + let mut send = |host: &mut Host, client: &mut ClientState, frame: Vec| { + client.fold_outgoing(&frame); + let answer = host.handle(asker, ClientFrame::Commands(frame)); + for msg in to(&answer, asker) { + match msg { + ServerFrame::Commands(cmds) => cmds.iter().for_each(|c| client.fold(c)), + ServerFrame::Accepted => client.accepted(), + other => panic!("the frame must be accepted: {other:?}"), + } + } + answer + }; + // Off the origin first, so a reset that never arrived is visible. + send( + &mut host, + &mut client, + vec![Command::SetSelected { row: 1, col: 0 }], + ); + + // The `+cat` commit's shape: tidy-up, the schema change, then (maybe) + // the cursor put back where the user was (resolve.rs::create_column). + let add = ModelCommand::AddCategory("Product".into()); + let mut frame = vec![ + Command::ChangeMode(AppMode::Normal), + crate::ui::reduce::caught_up(host.app(), add), + ]; + if placed { + frame.push(Command::SetSelected { row: 1, col: 0 }); + } + let answer = send(&mut host, &mut client, frame); + assert!( + to(&answer, bystander).contains(&ServerFrame::Commands(vec![Command::ResetCursor])), + "setup: the frame must really reframe, or nothing here is at stake" + ); + + let mirror = host.sessions.get(asker).unwrap().view().cursor(); + assert_eq!( + mirror.selected, expected, + "setup, placed={placed}: the mirror" + ); + assert_eq!( + client.view.cursor(), + mirror, + "placed={placed}: the asker's client and its mirror disagree" + ); + } + } + /// A refused batch is answered to the **asker and nobody else** (ADR 001): /// nothing happened, so there is nothing to tell the others. #[test] From f3fb83ee45a81d65753b8d7ad77ee56a42216326 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:02:14 -0700 Subject: [PATCH 5/8] improvise-kb1d: document the refusal; v4hg has closed the add/edit asymmetry Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 16 +++++++++++++--- context/code/core.md | 5 ++++- context/test-evidence/command.md | 9 +++++++++ crates/improvise-command/src/ui/app.rs | 6 +++++- crates/improvise-core/src/model/types.rs | 8 ++++++-- 5 files changed, 37 insertions(+), 7 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index e713576d..c160440d 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3474,9 +3474,19 @@ refusal that stops the batch therefore leaves the editor open with the user's text and the reason in the minibuffer line (`draw_bottom_bar` draws the status beside the minibuffer). That covers all four refusals: the formula is gone, it was rewritten since the editor opened (`was`), the new text does not parse, or -the new target belongs to another formula. `AddFormula`'s parse error only -`refuse`s, so a bad *new* formula still closes the editor and clears the text. -That asymmetry is improvise-v4hg's to close, not this ticket's. +the new target belongs to another formula, or the rename is of a formula +outside `_Measure` (improvise-kb1d, below). `AddFormula`'s parse error stops the +batch the same way since improvise-v4hg (#213), so a bad *new* formula also +leaves the editor open with its text; adding and editing now agree. + +**A target change outside `_Measure` is refused (improvise-kb1d).** A regular +category's formula target is a real item of that category, not a derived one. +`replace_formula` added the new target item and left the old one, so renaming +`Total = East + West [Region]` to `Sum` left `Total` as an empty item and +appended `Sum` at the end. Renaming the item in place was the alternative and +was not taken: an item's name is its identity in cells, views, hidden lists and +groups, so that rename is the whole of improvise-sboz, not a small fix. Editing +such a formula without changing its target still works. On success the command's optimistic "Formula updated" stands, except on a rename, where the effect rewrites the status to name the old and new targets diff --git a/context/code/core.md b/context/code/core.md index 2f844f6d..478ab155 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -1273,7 +1273,10 @@ view's hidden list, a `measure_agg` entry. Those cannot follow without rewriting `ReplaceFormula` reports which formulas still mention the old name (`Formula::mentions`, over the expression and its `WHERE`), and their cells show the `ref` error rather than a silent wrong number. Under a stable id those -references are what sboz makes follow. +references are what sboz makes follow. **This holds for `_Measure` targets only.** +Outside `_Measure` a target is a real item, and a rename is refused until sboz +(improvise-kb1d, `context/code/command.md`). A rename also leaves stored cells under +the old `_Measure` name in `## Data`, invisible because the formula shadowed them. **Concurrency is checked twice.** `FormulaSlot::Existing { id, was }` is captured when the editor opens: the id must still resolve, and the formula must diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 2d306a36..99a44245 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5764,3 +5764,12 @@ host's `Normal`, and nothing else different. `install_workbook` needs no change of its own, because both of its callers (`LoadModel`, `AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. +### renaming_a_formula_outside_measure_refuses_and_keeps_the_editor + +improvise-kb1d, found by the pre-merge review of #214 and by ChatGPT Codex (P1). +The fixture has a Region-targeted formula at index 1, between two `_Measure` +formulas, because every other km71 test used `_Measure`, whose items are +derived from the formula list and cannot show a stranded item. Rival: the guard +in `Model::replace_formula` disabled. This test fails on the formula list, and +`replace_formula_refuses_to_rename_a_target_outside_measure` in core fails too. + diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 4b878fe9..2c3f8dc2 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -4996,7 +4996,11 @@ mod formula_edit_tests { app.view_state.formula_cursor = 1; let regions = |app: &App| { let region = app.model_state.workbook.model.category("Region").unwrap(); - region.ordered_item_names().iter().map(|s| s.to_string()).collect::>() + region + .ordered_item_names() + .iter() + .map(|s| s.to_string()) + .collect::>() }; let before = regions(&app); open_editor(&mut app); diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index f48f24ef..8b858e86 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -1947,11 +1947,15 @@ mod formula_tests { #[test] fn replace_formula_refuses_to_rename_a_target_outside_measure() { let mut m = three_formulas(); - m.formulas.insert(1, parse_formula("Total = East + West", "Region").unwrap()); + m.formulas + .insert(1, parse_formula("Total = East + West", "Region").unwrap()); let shape = |m: &Model| { let region = m.category("Region").unwrap().ordered_item_names(); let owned: Vec = region.iter().map(|s| s.to_string()).collect(); - (targets(m).iter().map(|s| s.to_string()).collect::>(), owned) + ( + targets(m).iter().map(|s| s.to_string()).collect::>(), + owned, + ) }; let before = shape(&m); let id = m.formulas[1].id(); From ab1c9818296aabac348ba45ef8fa7c87d0ea39af Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:05:15 -0700 Subject: [PATCH 6/8] improvise-wdmp: drop an unused mut clippy flagged Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 6efb7eaf..033ceacb 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1499,7 +1499,7 @@ mod tests { ServerFrame::Snapshot(s) => client.bootstrap(*s), other => panic!("setup: connect answers a Snapshot, got {other:?}"), } - let mut send = |host: &mut Host, client: &mut ClientState, frame: Vec| { + let send = |host: &mut Host, client: &mut ClientState, frame: Vec| { client.fold_outgoing(&frame); let answer = host.handle(asker, ClientFrame::Commands(frame)); for msg in to(&answer, asker) { From bd49de6b485d4c2ee0482632843ef1a2de2e5735 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:32:06 -0700 Subject: [PATCH 7/8] improvise-4vw8: click_formula opens the editor, as Enter does The doc comment still said a formula-row click was navigation only. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-wasm-client/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index e02f7cf9..21ea8364 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -134,8 +134,8 @@ impl Client { self.clicked(Click::ViewRow(index)) } - /// The user clicked row `index` of the formula panel. Navigation only — - /// that panel binds no `Enter` (improvise-iioi). + /// The user clicked row `index` of the formula panel: focus it, move there, + /// then open the editor on that formula, as `Enter` does (improvise-km71). #[wasm_bindgen] pub fn click_formula(&mut self, index: usize) -> Result, JsError> { self.clicked(Click::FormulaRow(index)) From 6d5b4e328a7ab93975096f0ab738c41e4fbd6831 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:50:53 -0700 Subject: [PATCH 8/8] improvise-km71: serde back in improvise-formula for FormulaId alone tt22 removed the formula crate's serde dependency with the legacy JSON loader, keeping serde only where a type crosses the wire. FormulaId crosses the wire, so it is the third such type. No derive goes back on the formula AST. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- Cargo.lock | 1 + context/code/formula-and-io.md | 6 +++++- crates/improvise-formula/Cargo.toml | 2 ++ crates/improvise-formula/src/ast.rs | 2 +- harness/opfs-contract/Cargo.lock | 1 + 5 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b80ea5bd..961d6d68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,6 +897,7 @@ dependencies = [ "pest_derive", "pest_meta", "proptest", + "serde", ] [[package]] diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index eacb06f1..58617a67 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -488,8 +488,12 @@ The serde derives that existed only for that path went with it: `Workbook`, `DataStore`'s manual impls and the formula AST. So did indexmap's `serde` feature in `improvise-core`, `improvise-io` and the root crate, and `improvise-formula`'s serde dependency. Each was removed and the whole workspace rebuilt, so the -compiler named every other consumer. Two remain, and both are the wire: +compiler named every other consumer. Two remained, and both are the wire: `Axis` (whose `rename_all = "lowercase"` is its wire spelling) and `PageAxis`. +A third arrived with improvise-km71 by the same rule: `FormulaId`, which crosses +the wire in `ModelCommand::ReplaceFormula`, `FormulaSlot` and `FormulaEntry`. +So `improvise-formula` depends on serde again, for that one type. No derive went +back on the formula AST. `CellValue` was never a candidate, since it is carried on the wire too. JSON *import* (`StartImportWizard`, `ImportJsonHeadless`) is a separate feature and is untouched. diff --git a/crates/improvise-formula/Cargo.toml b/crates/improvise-formula/Cargo.toml index d7459be8..00bf407c 100644 --- a/crates/improvise-formula/Cargo.toml +++ b/crates/improvise-formula/Cargo.toml @@ -10,6 +10,8 @@ repository = "https://github.com/fiddlerwoaroof/improvise" anyhow = "1" pest = "2.9.2" pest_derive = "2.9.2" +# For FormulaId alone, which crosses the wire (improvise-km71); no AST derive. +serde = { version = "1", features = ["derive"] } [dev-dependencies] pest_meta = "2.9.2" diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index b8028b56..ee1305b5 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -101,7 +101,7 @@ impl Formula { /// Which formula an edit means, opaque so its content can change in one place: /// today a target and its category — `context/code/core.md` §18. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] pub struct FormulaId { target: String, target_category: String, diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index 385b3e29..579594c6 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -196,6 +196,7 @@ dependencies = [ "anyhow", "pest", "pest_derive", + "serde", ] [[package]]