fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #41 from fiddlerwoaroof/comments/command

Browse Source 
docs(command): move improvise-command's reasoning out of comments into context/
This commit is contained in:
Ed Langley
2026-09-20 10:43:18 -07:00
committed by GitHub
parent 04e4e3de78 288cbca8cf
commit 5f871f7035
26 changed files with 5026 additions and 3482 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/command.md
+1994
View File
File diff suppressed because it is too large Load Diff
context/test-evidence/command.md
+2168
View File
File diff suppressed because it is too large Load Diff
crates/improvise-command/src/command/cmd/cell.rs
+1 -2
View File
@@ -114,8 +114,7 @@ mod tests {
}
// ── Cell operations ──────────────────────────────────────────────────────────
// All cell commands take an explicit CellKey. The interactive spec fills it
// from ctx.cell_key(); the parser fills it from Cat/Item coordinate args.
// All cell commands take an explicit CellKey; see `context/code/command.md`.
/// Clear a cell.
#[derive(Debug)]
crates/improvise-command/src/command/cmd/commit.rs
+44 -141
View File
@@ -64,13 +64,8 @@ mod tests {
);
}
/// improvise-6t3: the commit pushed `set_status("Formula added")` *after*
/// `AddFormula`, so the parse error that `AddFormula::apply` writes to the
/// status bar was immediately overwritten. An unparseable formula was
/// dropped while the UI reported success.
///
/// The add effect is the only party that knows whether it worked, so it has
/// to get the last word on the status line.
/// improvise-6t3: `set_status("Formula added")` after `AddFormula` overwrote the
/// parse error, so an unparseable formula was dropped and reported a success.
#[test]
fn invalid_formula_reports_the_parse_error_not_success() {
let m = two_cat_model();
@@ -158,9 +153,8 @@ mod tests {
);
}
/// `CommitAndAdvance` must thread its `edit_mode` through to the
/// trailing `EnterEditAtCursor` effect so the post-commit re-edit lands
/// in the mode the keymap requested. The command never reads ctx.mode.
/// `CommitAndAdvance` threads its `edit_mode` to the trailing
/// `EnterEditAtCursor`; the command never reads `ctx.mode`.
#[test]
fn commit_and_advance_threads_edit_mode_to_enter_edit_at_cursor() {
let m = two_cat_model();
@@ -203,14 +197,8 @@ mod tests {
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
}
/// improvise-lae: the save prompt has to commit to `SaveAs` and not to
/// `ExportCsv`, which is what makes it a separate mode from
/// `ExportPrompt` rather than a second binding onto it — `ExportCsv`
/// would write CSV under whatever `.improv` name the user typed.
///
/// `SaveAs` and not `Save`, because `Save` writes the path the document
/// already has, and the whole reason this prompt exists is that there
/// isn't one.
/// improvise-lae: the save prompt commits to `SaveAs`, not `ExportCsv` (wrong
/// format) and not `Save` (there is no path yet).
#[test]
fn commit_save_writes_the_model_to_the_typed_path() {
let m = two_cat_model();
@@ -232,9 +220,8 @@ mod tests {
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
}
/// An empty prompt must not reach `SaveAs`: `SaveAs(PathBuf::from(""))`
/// asks the backend to write to `""`, and the user then reads whatever
/// I/O error that produces instead of "you typed nothing".
/// An empty prompt must not reach `SaveAs`: the user would read an I/O error
/// about `""` instead of "you typed nothing".
#[test]
fn commit_save_on_an_empty_path_writes_nothing() {
let m = two_cat_model();
@@ -255,9 +242,8 @@ mod tests {
// ── Commit commands (mode-specific buffer consumers) ────────────────────────
/// Commit a cell value: for synthetic records keys, stage in drill pending edits
/// in drill mode, or apply directly in plain records mode; for real keys, write
/// to the model.
/// Commit a cell value: a synthetic records key stages into the drill or applies
/// directly in plain records mode; a real key writes to the model (gotcha 19).
fn commit_regular_cell_value(key: &CellKey, value: &str, effects: &mut Vec<Box<dyn Effect>>) {
if value.is_empty() {
effects.push(Box::new(effect::ClearCell(key.clone())));
@@ -280,9 +266,8 @@ fn stage_drill_edit(record_idx: usize, col: RecordCol, value: &str) -> Box<dyn E
})
}
/// Rename one coordinate of a records row. In wide format a row is a group
/// of cells — one per measure — so the rename has to move every one of them,
/// not just the cell under the cursor (improvise-c5v).
/// Rename one coordinate of a records row. In wide format a row is a group of cells,
/// so the rename moves **every** one of them (improvise-c5v, gotcha 1).
fn rename_record_coordinate(
model: &crate::model::Model,
row: &RecordRow,
@@ -299,9 +284,8 @@ fn rename_record_coordinate(
item: value.to_string(),
}));
// Every cell in the row moves. A row seeded by `add-record-row` has no
// measure cells yet — its coordinates live on a lone measure-less cell,
// which must move too or the row loses its identity.
// Every cell in the row moves. A row seeded by `add-record-row` carries its
// coordinates on a lone measure-less cell, which must move too.
let cells: Vec<(CellKey, CellValue)> = if row.values.is_empty() {
model
.get_cell(&row.coords)
@@ -345,9 +329,8 @@ fn commit_plain_records_edit(
}
}
/// Typing a name into the trailing column creates that measure, which gives
/// it a real column to type values into. Adding a measure is a schema
/// change, so it applies immediately even inside a drill.
/// Typing a name into the trailing column creates that measure. A schema change, so
/// it applies immediately even inside a drill.
fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
let name = value.trim();
if name.is_empty() {
@@ -359,20 +342,8 @@ fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
}));
}
/// Typing a name into the `+cat` column creates that category, which gives
/// the row a coordinate column to type into (improvise-2zq).
///
/// It creates the column and **does not set the coordinate on the current
/// row**, which is the deliberate answer to the ticket's third question.
/// Two reasons. It is symmetric with the measure `+`, which creates an empty
/// column and writes no value. And setting it would move *every* measure
/// cell in the row onto the new coordinate (gotcha 1) — a data migration
/// across N cells attached to the same Enter that made a schema change, in
/// an app with no undo. The follow-up edit is one `l` away in the column
/// that just appeared.
///
/// Like `add_measure` this is a schema change, so it applies immediately
/// even inside a drill rather than being staged.
/// Typing a name into `+cat` creates the category **and its column**, and writes no
/// coordinate on the current row (improvise-2zq) — see `context/code/command.md`.
fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
let name = value.trim();
if name.is_empty() {
@@ -405,13 +376,8 @@ fn commit_cell_value(
commit_regular_cell_value(key, value, effects);
}
/// Whether committing `key` inserts a column, invalidating every column
/// index to its right.
///
/// Asked of the *layout* — `record_col` resolves the synthetic `_Dim`
/// position into what the column actually is — rather than re-derived from
/// the value or the cursor, so there is one answer to "what is this column"
/// (gotcha 1).
/// Whether committing `key` inserts a column, invalidating every index to its right.
/// Asked of the *layout*, so there is one answer to "what is this column" (gotcha 1).
fn inserts_a_column(ctx: &CmdContext, key: &CellKey) -> bool {
crate::view::synthetic_record_info(key)
.and_then(|(_, col_idx)| ctx.layout.record_col(col_idx))
@@ -427,9 +393,8 @@ pub enum AdvanceDir {
Right,
}
/// Return the normal-mode counterpart of an editing mode. Used by
/// `CommitAndAdvance` to compute the mode to land in if the advance
/// aborts (commit + exit editing at boundary).
/// The normal-mode counterpart of an editing mode, for the mode to land in if
/// `CommitAndAdvance`'s advance aborts.
fn exit_mode_for(edit_mode: &AppMode) -> AppMode {
match edit_mode {
AppMode::RecordsEditing { .. } => AppMode::RecordsNormal,
@@ -437,13 +402,8 @@ fn exit_mode_for(edit_mode: &AppMode) -> AppMode {
}
}
/// Commit a cell edit, advance the cursor, and re-enter edit mode.
/// Subsumes the old `CommitCellEdit` (Down) and `CommitAndAdvanceRight` (Right).
///
/// `edit_mode` is the editing mode to re-enter after advancing. The keymap
/// binding supplies this — the editing-mode keymap passes `editing` and the
/// records-editing keymap passes `records-editing`. The command itself
/// never inspects `ctx.mode`.
/// Commit a cell edit, advance the cursor, and re-enter `edit_mode` — which the
/// keymap binding supplies. The command never inspects `ctx.mode`.
#[derive(Debug)]
pub struct CommitAndAdvance {
pub key: CellKey,
@@ -461,40 +421,13 @@ impl Cmd for CommitAndAdvance {
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
// Pre-emptively drop to the normal counterpart of edit_mode. If the
// advance succeeds, the trailing `EnterEditAtCursor` below will lift
// us back into editing on the new cell. If the advance aborts
// (e.g. already at bottom-right on Enter), `EnterEditAtCursor` is
// skipped and we land in normal mode — which is the desired
// "Enter at bottom-right commits and exits" behavior.
//
// It goes **before** the commit, not after (improvise-2zq). The
// commit can abort the batch itself — `AddCategory` does so when the
// document is at `MAX_CATEGORIES` — and an abort before this ran
// left the app in the editing mode, where `draw_bottom_bar` renders
// the minibuffer instead of the status line and the refusal the
// effect had just written was never drawn. Tidy-up goes where an
// abort cannot reach it; same rule as `ExecuteCommand`'s.
// Drop to normal **before** the commit, which can abort: tidy-up goes where
// an abort cannot reach it (improvise-2zq, gotcha 21).
effects.push(effect::change_mode(exit_mode_for(&self.edit_mode)));
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
if inserts_a_column(ctx, &self.key) {
// A schema change that inserts a column moves every index to its
// right, and the advance below is computed from `ctx` — the
// *pre-commit* layout, because `apply_effects` rebuilds only
// after the whole batch (gotcha 23). On the last row `Enter`
// wraps to `col + 1`, which after the insert is `+cat` again, so
// the editor reopened category creation and a user naming two
// categories in a row never reached the column they had made.
// `Tab` reached it the same way.
//
// Not advancing is also the *useful* answer rather than merely
// the safe one: `add_category` inserts the new column exactly
// where the cursor already is, so staying put lands on it.
//
// The measure `+` shares none of this, and not by luck — it is
// the last column, so there is no column to its right for an
// index to shift into, and both advances are already special-
// cased at the bottom-right edge.
// An inserted column shifts every index right of it, and the advance is
// computed from the *pre-commit* layout (gotcha 23). Staying put lands on it.
effects.push(Box::new(effect::RebuildLayout));
return effects;
}
@@ -544,25 +477,21 @@ impl Cmd for CommitAndAdvance {
}
}
/// What a minibuffer commit produces. The variants differ only in the model
/// mutation they build and in where they leave the user — reading the buffer,
/// trimming it, bailing out when it is empty, marking the model dirty and
/// reporting are shared by [`CommitBuffer`].
/// What a minibuffer commit produces. The variants differ only in the mutation they
/// build and where they leave the user; the rest is [`CommitBuffer`]'s (gotcha 20).
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum CommitKind {
Formula,
Category,
/// Add an item to this category. Resolved from [`AppMode::ItemAdd`] when
/// the command is constructed, so `execute` stays a pure function of the
/// kind and its buffer.
/// Add an item to this category, resolved from [`AppMode::ItemAdd`] at
/// construction so `execute` stays pure over the kind and its buffer.
Item {
category: String,
},
}
/// Resolve a `commit-buffer` argument against the current mode. `Item` is the
/// only kind that needs anything from the mode — its target category — so a
/// script line (which has no live mode) can build the other two.
/// 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<CommitKind, String> {
match (arg, mode) {
("formula", _) => Ok(CommitKind::Formula),
@@ -594,9 +523,8 @@ impl CommitKind {
}
}
/// Where a *successful* commit lands the user. `None` keeps the minibuffer
/// open for the next entry — categories and items are entered in batches,
/// formulas are not.
/// Where a *successful* commit lands the user. `None` keeps the minibuffer open:
/// categories and items are entered in batches, formulas are not.
fn on_commit(&self) -> Option<AppMode> {
match self {
Self::Formula => Some(AppMode::FormulaPanel),
@@ -629,9 +557,8 @@ impl CommitKind {
}
}
/// Commit the contents of a minibuffer as a formula, a category or an item.
///
/// Buffer clearing is handled by the keymap (Enter → [commit, clear-buffer]).
/// Commit a minibuffer as a formula, a category or an item. Buffer clearing is the
/// keymap's (Enter → `[commit, clear-buffer]`).
#[derive(Debug)]
pub struct CommitBuffer {
pub kind: CommitKind,
@@ -648,9 +575,8 @@ impl Cmd for CommitBuffer {
}
let mut effects: Vec<Box<dyn Effect>> = vec![
effect::set_status(self.kind.status(name)),
// The add goes last on purpose: it is the only party that knows
// whether it worked, so the optimistic status above must not
// clobber the error it reports (improvise-6t3).
// 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),
];
effects.extend(self.kind.on_commit().map(effect::change_mode));
@@ -658,16 +584,8 @@ impl Cmd for CommitBuffer {
}
}
/// Commit a save path from the save buffer.
///
/// The sibling of [`CommitExport`], and deliberately not the same command:
/// the export prompt writes CSV, this one writes the *model* through
/// `SaveAs`, which also adopts the path so a later plain `Save` has one
/// (improvise-lae).
///
/// An empty path bails rather than reaching `SaveAs`, which would otherwise
/// ask the storage backend to write to `""` and report its error instead of
/// the one the user can act on.
/// Commit a save path. The sibling of [`CommitExport`] and deliberately not the same
/// command — that one writes CSV, this writes the model (improvise-lae).
#[derive(Debug)]
pub struct CommitSave;
impl Cmd for CommitSave {
@@ -682,23 +600,8 @@ impl Cmd for CommitSave {
effect::change_mode(AppMode::Normal),
];
}
// The mode change comes **first**, and that ordering is the whole
// reason the user sees a failed save (found by Copilot on PR #27).
//
// `SaveAs` aborts the batch when the write fails (improvise-8tk's
// other arm), so anything after it is skipped. With the order
// reversed the batch stopped while still in `SavePrompt` — and
// `draw_bottom_bar` renders the minibuffer *instead of* the status
// line, so the "Save error:" set by the effect was never drawn. The
// save appeared to do nothing at all.
//
// Leading with the mode change means the error lands in `Normal`,
// where the status bar is what the bottom row draws. It costs the
// typed path, which the welded `clear-buffer` would have taken on a
// success anyway and which `EnterMode` clears on the next Ctrl-W
// regardless (gotcha 11) — so nothing durable is lost, and the
// convention that the side effect runs last and its message wins is
// the one gotcha 20 already states.
// The mode change comes **first**, or a failed `SaveAs` aborts with the
// minibuffer still drawn over its error (Copilot, PR #27).
vec![
effect::change_mode(AppMode::Normal),
Box::new(effect::SaveAs(std::path::PathBuf::from(path))),
crates/improvise-command/src/command/cmd/core.rs
+10 -22
View File
@@ -10,11 +10,8 @@ use crate::ui::effect::{Effect, Panel};
use crate::view::{Axis, GridLayout, View};
use crate::workbook::Workbook;
/// Read-only context available to commands for decision-making.
///
/// Commands receive a `&Model` (pure data) and a `&View` (the active view).
/// The full `&Workbook` is also available for the rare command that needs
/// to enumerate all views (e.g. the view panel).
/// Read-only context available to commands for decision-making: a `&Model`, the
/// active `&View`, and the `&Workbook` for the rare command that enumerates views.
pub struct CmdContext<'a> {
pub model: &'a Model,
pub workbook: &'a Workbook,
@@ -118,10 +115,8 @@ pub trait Cmd: Debug + Send + Sync {
/// Factory that constructs a Cmd from text arguments (headless/script).
pub type ParseFn = fn(&[String]) -> Result<Box<dyn Cmd>, String>;
/// Factory that constructs a Cmd from the interactive context (keymap dispatch).
/// Receives both the keymap args and the interactive context so commands can
/// combine text arguments (e.g. panel name) with runtime state (e.g. whether
/// the panel is currently open).
/// Factory constructing a Cmd from the interactive context. It takes both the keymap
/// args and the context, so a command can combine text args with runtime state.
pub type InteractiveFn = fn(&[String], &CmdContext) -> Result<Box<dyn Cmd>, String>;
type BoxParseFn = Box<dyn Fn(&[String]) -> Result<Box<dyn Cmd>, String>>;
@@ -174,9 +169,8 @@ impl CmdRegistry {
});
}
/// Register a command that doesn't need interactive context.
/// When called interactively with args, delegates to parse.
/// When called interactively without args, returns an error.
/// Register a command that needs no interactive context: with args it delegates
/// to `parse`, and without them it errors.
pub fn register_pure(&mut self, prototype: &dyn Cmd, parse: ParseFn) {
self.entries.push(CmdEntry {
name: prototype.name(),
@@ -213,9 +207,8 @@ impl CmdRegistry {
Err(format!("Unknown command: {name}"))
}
/// Construct a command from interactive context (keymap dispatch).
/// Always calls the interactive constructor with both args and ctx,
/// so commands can combine text arguments with runtime state.
/// Construct a command from interactive context, always with both args and
/// `ctx`, so commands can combine text arguments with runtime state.
pub fn interactive(
&self,
name: &str,
@@ -269,13 +262,8 @@ pub(super) fn read_buffer(ctx: &CmdContext, name: &str) -> String {
}
}
/// The effect that writes a named buffer — the mirror of [`read_buffer`],
/// and the only place that knows "search" is not an entry in the buffers map
/// but its own `ViewState` field (improvise-gdl).
///
/// The knowledge used to sit inside the `SetBuffer` *effect*, where nothing
/// paired it with the read side and every caller had to know that passing
/// `"search"` meant something else than it said.
/// The effect that writes a named buffer — the mirror of [`read_buffer`], and the
/// only place that knows "search" is not in the buffers map (improvise-gdl).
pub(super) fn write_buffer(name: &str, value: String) -> Box<dyn Effect> {
if name == "search" {
Box::new(crate::ui::effect::SetSearchQuery(value))
crates/improvise-command/src/command/cmd/effect_cmds.rs
+1 -2
View File
@@ -83,8 +83,7 @@ mod tests {
}
// ── Parseable model-mutation commands ────────────────────────────────────────
// These are thin Cmd wrappers around effects, constructible from string args.
// They share the same execution path as keymap-dispatched commands.
// Thin Cmd wrappers around effects, on the same path as keymap dispatch.
macro_rules! effect_cmd {
($name:ident, $cmd_name:expr, $parse:expr, $exec:expr) => {
crates/improvise-command/src/command/cmd/fit.rs
+22 -124
View File
@@ -1,62 +1,5 @@
//! `fit-measure`: evolve a decision tree over what the active view shows and
//! store it as an ordinary improvise formula.
//!
//! After this command there is nothing GP-specific left in the document — the
//! rule is a `## Formulas` line like any other, recomputed by the fixed-point
//! evaluator, editable in the formula panel, drillable, and saved with no new
//! grammar (improvise-21d's enabling fact, and improvise-7na's answer).
//!
//! # The fit runs inside `execute`, and that is the architecture rather than a
//! shortcut
//!
//! A `Cmd` is pure over `&CmdContext` and returns effects; an `Effect` is the
//! only mutation site. A fit is **decision logic** — a pure function of
//! `(Model, View, target, seed, FitConfig)` that mutates nothing — so it
//! belongs on the command side, and what the effects carry is its *result*.
//! Putting the search inside an `Effect::apply` would have mixed an expensive
//! stochastic computation into the one place the app mutates, which is the
//! split's whole point.
//!
//! What this does **not** buy is asynchrony. The command runs on the thread
//! that draws the next frame, so a fit is bounded rather than cancellable:
//! [`FitRequest::default`] is a deliberately smaller search than the CLI's
//! (improvise-vx3's AC 4 asks for "cancellable **or** bounded"). Moving it off
//! the main loop is a change to `draw.rs`'s event loop, not to this file, and
//! nothing here forecloses it — the fit is already a pure function that could
//! be handed to a worker and have its `FitOutcome` posted back as the same
//! effects.
//!
//! # What a refusal is for
//!
//! `improvise_gp::fit_measure` fails rather than storing something misleading
//! — most sharply when the champion splits on an encoding artefact like
//! `Region=North`, which is not an item of anyone's document, so every cell of
//! the stored measure would be `Error("ref:…")` (improvise-8xhy, repo-map
//! gotcha 37d). This command is where a user actually meets that restriction,
//! so the refusal goes to the status line **whole**: it names the features that
//! could not be resolved, because "the fit failed" is not something a user can
//! act on and "it split on Region, which is a category rather than a measure"
//! is.
//!
//! # What registering it costs the browser, measured
//!
//! `improvise-command` is in the **worker** bundle, and the path from
//! `App::new` → `default_keymaps` → `default_registry` → this command's
//! prototype is live, so `gp-tree` links into it whether or not a browser ever
//! invokes `fit-measure`. `kb.pl` names keeping GP out of the wasm bundle as
//! `improvise-gp`'s reason to exist, so this is a cost to state rather than to
//! discover: built with `wasm-pack --release`, `improvise_worker_server_bg.wasm`
//! goes **1,341,130 → 1,420,470 bytes** (+79 KB, +5.9%), or **498 → 531 KB
//! gzipped** (+6.5%). `improvise-wasm-client` — the main-thread bundle — does
//! not depend on `improvise-command` at all and is **unchanged**.
//!
//! Accepted rather than feature-gated, for two reasons. The worker already
//! carries the whole `App`, persistence and the import wizard, so this is a
//! 6% addition to something an order of magnitude larger; and a browser fit is
//! a *wanted* end state, not dead weight — `improvise_gp::fit` exists where it
//! does precisely so a second host can run the same sequence. If the bundle
//! ever needs shrinking, a default-on cargo feature that `web/build.sh` turns
//! off is the lever, and this paragraph is the number to beat.
//! `fit-measure`: evolve a decision tree over the active view and store it as an
//! ordinary `## Formulas` line. The argument is in `context/code/command.md`.
use improvise_gp::fit::one_line;
use improvise_gp::{FitRequest, fit_measure, fitted_measure_name};
@@ -64,11 +7,8 @@ use improvise_gp::{FitRequest, fit_measure, fitted_measure_name};
use crate::command::cmd::core::{Cmd, CmdContext};
use crate::ui::effect::{self, Effect};
/// A seed as the status line spells it (`0x…`, either case) or in decimal.
///
/// The hex arm is not a convenience: `FitOutcome::status_line` prints `{:#x}`
/// and tells the user that is what reproduces the fit, so accepting only
/// decimal made the advertised round trip fail on the advertised value.
/// A seed as the status line spells it (`0x…`, either case) or in decimal. The hex
/// arm is not a convenience — `status_line` prints `{:#x}`.
fn parse_seed(text: &str) -> Result<u64, String> {
let parsed = match text.strip_prefix("0x").or_else(|| text.strip_prefix("0X")) {
Some(hex) => u64::from_str_radix(hex, 16),
@@ -97,16 +37,8 @@ impl FitMeasure {
}
}
/// `fit-measure <target> [<measure-name>] [<seed>]`.
///
/// The seed is an argument rather than a clock read for the reason the CLI
/// fixes its own default: two fits of the same document should agree, and a
/// user who wants a different search asks for one. It is reported back on
/// the status line so the fit that landed can be asked for again — **which
/// is why it is read in the form it is printed**, hexadecimal with a `0x`
/// prefix, as well as decimal. `status_line` spells it `{:#x}`, so a
/// decimal-only parser made the one instruction the status line gives a
/// user fail on the value the status line just showed them.
/// `fit-measure <target> [<measure-name>] [<seed>]`. The seed is an argument, not
/// a clock read, and is read in the form it is printed (gotcha 37b).
pub fn parse(args: &[String]) -> Result<Box<dyn Cmd>, String> {
let [target, rest @ ..] = args else {
return Err("fit-measure requires a target measure".into());
@@ -140,9 +72,8 @@ impl Cmd for FitMeasure {
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
match fit_measure(ctx.model, ctx.view, &self.target, &self.into, &self.request) {
// The report first and `AddFormula` last, so a formula the emitter
// spelled but the parser will not take leaves *its* message on
// screen rather than this command's optimistic one (gotcha 20).
// The report first and `AddFormula` last, so a formula the parser refuses
// leaves *its* message on screen, not this command's (gotcha 20).
Ok(outcome) => vec![
effect::set_status(outcome.status_line()),
Box::new(effect::AddFormula {
@@ -150,10 +81,8 @@ impl Cmd for FitMeasure {
target_category: outcome.formula.target_category.clone(),
}),
],
// Flattened like the success line: several refusals interpolate a
// document name, and a name may legally contain a newline
// (Copilot on PR #25). A failure is when a user is most reading
// the status bar, so it is the last place to let one split.
// Flattened: a refusal interpolates a document name, and a name may
// legally contain a newline (Copilot on PR #25).
Err(e) => vec![effect::set_status(one_line(format!("fit-measure: {e}")))],
}
}
@@ -171,16 +100,8 @@ mod tests {
const REGION: &str = "Region";
const TAG: &str = "Tag";
/// A document a tree can learn exactly — `Score` steps at `Cost` 50 — in
/// the units a real document uses.
///
/// **Every category is a `Label`**, so the only feature is the numeric
/// measure `Cost` and the champion cannot reach an unresolvable one. An
/// earlier version left `Region` regular with one item and claimed
/// `simplify` would prune the constant one-hot; it does not, because
/// `build_categorical` records `(0.0, 1.0)` for every one-hot whatever the
/// column holds — so these tests were passing on seed luck (Copilot on
/// PR #25). See `improvise_gp::fit`'s own fixture for the same correction.
/// A document a tree can learn exactly. **Every category is a `Label`**, or the
/// champion can reach an unresolvable feature — see the evidence.
fn workbook() -> crate::workbook::Workbook {
let mut wb = crate::workbook::Workbook::new("sales");
wb.add_category(REGION).expect("first category");
@@ -236,9 +157,8 @@ mod tests {
app
}
/// The end-to-end claim of improvise-vx3, asserted against a real
/// document: a command fits, the formula lands through the ordinary
/// `add_formula` path, and the new measure evaluates to numbers.
/// improvise-vx3 end to end against a real document: a command fits, the formula
/// lands through `add_formula`, and the new measure evaluates to numbers.
#[test]
fn the_command_installs_a_formula_the_document_evaluates() {
let app = run(&["Score"]);
@@ -268,10 +188,8 @@ mod tests {
assert!(run(&["Score"]).model_state.dirty);
}
/// improvise-vx3 is emphatic that the rule alone is not the deliverable:
/// a tidy tree that generalizes no better than the mean is worse than
/// useless. So the status line carries the held-out comparison, what the
/// rule leans on, and the seed that reproduces it.
/// The rule alone is not the deliverable, so the status line carries the held-out
/// comparison, what the rule leans on, and the seed that reproduces it.
#[test]
fn the_status_line_reports_quality_against_the_baseline() {
let app = run(&["Score"]);
@@ -284,9 +202,8 @@ mod tests {
}
}
/// A refusal reaches the user *whole*. "fit failed" is not actionable;
/// naming the measure that does not exist is (improvise-8xhy's argument,
/// arriving at the place a user meets it).
/// A refusal reaches the user *whole*: "fit failed" is not actionable, naming the
/// measure that does not exist is (improvise-8xhy).
#[test]
fn a_refusal_names_what_went_wrong_and_stores_nothing() {
let wb = workbook();
@@ -306,21 +223,8 @@ mod tests {
assert!(app.model_state.workbook.model.formulas().is_empty());
}
/// gotcha 37b: the seed is half of what reproduces a fit, so it is an
/// argument — and a different one really does search differently.
///
/// It asserts on the **rule**, never on the batch's debug string. The
/// first version compared `effects_debug` over the whole vec, which was
/// vacuous: `status_line` prints `seed {:#x}`, so the two strings differ
/// because the seed was *printed*, and `fit_measure` could have ignored
/// `request.seed` entirely with the test still green
/// (`doctrine(check_the_checker)`).
///
/// `AddFormula` has exactly two fields — the formula text and the target
/// category — and the seed reaches neither, so its `Debug` cannot differ
/// for the reason the batch's did. (A substring guard was tried and is
/// wrong: the seeds `1` and `99` occur inside perfectly ordinary
/// thresholds.)
/// gotcha 37b: a different seed really does search differently. It asserts on the
/// **rule**, never on the batch debug string — see the evidence.
#[test]
fn the_seed_argument_changes_the_search() {
let wb = workbook();
@@ -350,14 +254,8 @@ mod tests {
assert!(err.contains("is not a seed"), "{err}");
}
/// Copilot on PR #25. The status line advertises the seed as `{:#x}` and
/// tells the user that is what reproduces the fit, so the one instruction
/// it gives has to work on the value it just showed them — a decimal-only
/// parser made copying it back an error.
///
/// Asserted as a *round trip through the status line*, not against a
/// hand-written `"0x…"`: a literal would still pass if `status_line`
/// changed how it spells the number, which is the half that breaks.
/// Copilot on PR #25: the seed must round-trip *through the status line*, not
/// against a hand-written `"0x…"` that would survive a respelling.
#[test]
fn the_seed_the_status_line_prints_is_a_seed_this_command_accepts() {
let app = run(&["Score"]);
crates/improvise-command/src/command/cmd/grid.rs
+16 -27
View File
@@ -120,10 +120,8 @@ mod tests {
);
}
/// Drilling into a formula cell (e.g. Profit = Revenue - Cost) should
/// return the underlying data records, not an empty result set. The
/// formula target coordinate is stripped from the drill key so that
/// matching_cells finds the raw data backing the formula.
/// Drilling into a formula cell must return the underlying data records: the
/// target coordinate is stripped so `matching_cells` finds them (gotcha 10).
#[test]
fn drill_into_formula_cell_returns_data_records() {
use crate::formula::parse_formula;
@@ -172,10 +170,8 @@ mod tests {
);
}
/// improvise-dz8: a drilled coordinate is a constraint of the drill, not
/// a dimension the user may page through, so it lands on `Filter`. It
/// used to land on `Page`, which put it in the tile bar and under
/// `[`/`]` as if the user could move it.
/// improvise-dz8: a drilled coordinate is a constraint, not a dimension to page
/// through, so it lands on `Filter`. It used to land on `Page`.
#[test]
fn drilling_fixes_its_coordinates_on_filter_not_page() {
let m = two_cat_model();
@@ -297,9 +293,8 @@ impl Cmd for ViewNavigate {
}
}
/// Drill down into an aggregated cell: create a _Drill view with _Index on
/// Row and _Dim on Column (records/long-format view). Fixed coordinates
/// from the drilled cell become page filters.
/// Drill into an aggregated cell: a `_Drill` view with `_Index` on Row and `_Dim` on
/// Column. The drilled cell's fixed coordinates become filters (gotcha 27).
#[derive(Debug)]
pub struct DrillIntoCell {
pub key: crate::model::cell::CellKey,
@@ -312,9 +307,8 @@ impl Cmd for DrillIntoCell {
let drill_name = crate::view::DRILL_VIEW.to_string();
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
// If drilling into a formula cell, strip the formula target from the
// key so matching_cells finds the underlying raw data records instead
// of returning nothing.
// Drilling a formula cell strips the formula target from the key, or
// `matching_cells` returns nothing instead of the raw records (gotcha 10).
let drill_key = if let Some(measure_val) = self.key.get(MEASURE_CATEGORY) {
let is_formula_target = ctx.model.formulas().iter().any(|f| {
CategoryKind::of_name(&f.target_category).is_measure() && f.target == measure_val
@@ -365,9 +359,8 @@ impl Cmd for DrillIntoCell {
axis: crate::view::Axis::Column,
}));
// Fixed coords (from the drilled cell) -> Filter, an immutable
// constraint of this drill rather than a dimension to page through
// (improvise-dz8).
// Fixed coords go to Filter: an immutable constraint of this drill rather
// than a dimension to page through (improvise-dz8).
let fixed_cats: std::collections::HashSet<String> =
self.key.0.iter().map(|(c, _)| c.clone()).collect();
for (cat, item) in &self.key.0 {
@@ -380,9 +373,8 @@ impl Cmd for DrillIntoCell {
item: item.clone(),
}));
}
// Previously-aggregated categories (none_cats) stay on Axis::None so
// they don't filter records; they'll appear as columns in records mode.
// Skip virtual categories — we already set _Index/_Dim above.
// Previously-aggregated categories stay on `Axis::None` so they do not filter
// records; they appear as columns. `_Index`/`_Dim` are already set.
for cat in ctx.none_cats() {
if fixed_cats.contains(cat) || cat.starts_with('_') {
continue;
@@ -418,9 +410,8 @@ impl Cmd for TogglePruneEmpty {
}
}
/// Toggle between records mode and pivot mode using the view stack.
/// Entering records mode creates a `_Records` view and switches to it.
/// Leaving records mode navigates back to the previous view.
/// Toggle records mode through the view stack: entering creates a `_Records` view
/// and switches to it, leaving navigates back.
#[derive(Debug)]
pub struct ToggleRecordsMode;
impl Cmd for ToggleRecordsMode {
@@ -431,10 +422,8 @@ impl Cmd for ToggleRecordsMode {
let is_records = ctx.layout.is_records_mode();
if is_records {
// Leaving records mode: clean up any records with empty CellKeys
// (produced by AddRecordRow when no page filters are set) before
// restoring the previous view. This is the inverse of `SortData`
// that runs on entry.
// Leaving cleans up the empty-`CellKey` records `AddRecordRow` makes with
// no page filters — the inverse of the entry `SortData`.
return vec![
Box::new(effect::CleanEmptyRecords),
Box::new(effect::ViewBack),
crates/improvise-command/src/command/cmd/mode.rs
+20 -66
View File
@@ -37,9 +37,8 @@ mod tests {
assert_eq!(effects.len(), 2);
}
/// The `enter-export-prompt` shim is gone (improvise-s13); the generic
/// `enter-mode` carries the mode name instead, and clears the prompt's
/// buffer on the way in.
/// The `enter-export-prompt` shim is gone (improvise-s13): the generic
/// `enter-mode` carries the mode name and clears the prompt's buffer.
#[test]
fn enter_mode_export_prompt_sets_mode_and_clears_its_buffer() {
let m = two_cat_model();
@@ -58,14 +57,8 @@ mod tests {
);
}
/// improvise-gdl made `write_buffer` the single accessor for a
/// runtime-named buffer, because "search" is not an entry in the buffers
/// map but its own `ViewState` field. `EnterMode` built a `SetBuffer`
/// directly, so a mode whose `buffer_key` is "search" would clear a map
/// entry nobody reads and leave the previous query on screen — the exact
/// desync gdl closed. No shipped `AppMode` names that buffer yet, so the
/// mode here is constructed by hand; that is what makes the test a
/// demonstration rather than a restatement.
/// improvise-gdl: `EnterMode` built a `SetBuffer` directly, so a "search"-keyed
/// minibuffer would clear a map entry nobody reads. See the evidence.
#[test]
fn enter_mode_clears_a_search_keyed_minibuffer_through_write_buffer() {
use crate::ui::app::{MinibufColor, MinibufferConfig};
@@ -129,11 +122,8 @@ mod tests {
assert!(dbg.contains("Editing"), "Expected Editing mode, got: {dbg}");
}
/// EditOrDrill must trust its `edit_mode` parameter rather than checking
/// `ctx.mode` — the records-normal keymap supplies `records-editing`,
/// but the command itself never inspects the runtime mode. This is the
/// parallel of the (deleted) `enter_edit_mode_produces_editing_mode`
/// test for the records branch.
/// `EditOrDrill` trusts its `edit_mode` parameter rather than checking
/// `ctx.mode` — the records-normal keymap supplies `records-editing`.
#[test]
fn edit_or_drill_passes_records_editing_mode_through() {
let m = two_cat_model();
@@ -153,12 +143,8 @@ mod tests {
);
}
/// `EnterEditAtCursorCmd` must hand its `target_mode` straight through
/// to the `EnterEditAtCursor` effect — the keymap (records `o` sequence
/// or commit-and-advance) decides; the command never inspects ctx.
/// It fronts the pair with `RebuildLayout` so the cursor is read against
/// a layout that includes whatever ran earlier in the sequence
/// (improvise-ehz).
/// `EnterEditAtCursorCmd` hands `target_mode` straight through, and fronts the
/// pair with `RebuildLayout` so the cursor reads a current layout (improvise-ehz).
#[test]
fn enter_edit_at_cursor_cmd_passes_target_mode_to_effect() {
let m = two_cat_model();
@@ -232,10 +218,8 @@ impl Cmd for EnterMode {
}
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
// Clear the corresponding buffer when entering a text-entry mode.
// Through `write_buffer`, which is the only code that knows a
// runtime-named buffer may not be an entry in the buffers map
// (improvise-gdl).
// Clear the mode's buffer through `write_buffer`, the only code that knows a
// runtime-named buffer may not be in the buffers map (improvise-gdl).
if let Some(mb) = self.0.minibuffer() {
effects.push(write_buffer(&mb.buffer_key, String::new()));
}
@@ -273,18 +257,8 @@ impl Cmd for Quit {
}
}
/// Save then quit. The `Quit` only lands if the `Save` succeeds — a failed
/// save aborts the effect chain, so `wq`/`ZZ` leaves the user in the app
/// with the error visible rather than discarding the model (improvise-8tk).
///
/// **This looks like an `AAndB` command and is not one** (improvise-git).
/// `wq` is the *user's* name for the composition, not an invented internal
/// one: it is registered, so `:wq` reaches it, and `Quit`'s unsaved-changes
/// message advertises that spelling. A keymap `Binding::Sequence` cannot be
/// reached from the `:` line, so replacing this with one would either delete
/// the ex-command or leave two independently-written spellings of "save then
/// quit". One definition behind two surfaces is what this already is.
/// Pinned by `text_buffer::tests::colon_wq_saves_then_quits`.
/// Save then quit; the `Quit` only lands if the `Save` succeeds (improvise-8tk).
/// **Not an `AAndB` command** — `wq` is the user's name — see `context/code/command.md`.
#[derive(Debug)]
pub struct SaveAndQuit;
impl Cmd for SaveAndQuit {
@@ -298,33 +272,15 @@ impl Cmd for SaveAndQuit {
// ── Editing entry ───────────────────────────────────────────────────────
/// Smart dispatch for i/a: if the cursor is on an aggregated pivot cell
/// (categories on `Axis::None` and the cell is not a synthetic records-mode
/// row), drill into it instead of editing. Otherwise pre-fill the edit
/// buffer with the displayed cell value and enter `edit_mode`.
///
/// `edit_mode` is supplied by the keymap binding — the command itself is
/// mode-agnostic, so the records-normal keymap passes `records-editing`
/// while the normal keymap passes `editing`.
/// Smart dispatch for i/a: drill an aggregated pivot cell, else pre-fill the buffer
/// and enter `edit_mode` — which the keymap supplies, so this stays mode-agnostic.
#[derive(Debug)]
pub struct EditOrDrill {
pub edit_mode: AppMode,
}
impl EditOrDrill {
/// What to say when there is no cell under the cursor at all.
///
/// improvise-ct5: this used to say "cannot drill — no cell at cursor" in
/// both modes. Drilling is a pivot-mode concept, and in records mode —
/// where every regular category sits on `Axis::None`, so this branch is
/// the one an empty grid always takes — the user's model is "a table I
/// type into", with `o` one key away to add a row. Naming drilling there
/// described a mechanism the user is not using instead of the key that
/// would help.
///
/// The choice reads `edit_mode` — the argument the *keymap* supplies —
/// rather than asking `ctx` what mode it is in, so the command stays as
/// mode-agnostic as its doc comment claims and no runtime `is_records()`
/// check comes back (improvise-4ju).
/// What to say when there is no cell under the cursor. It reads `edit_mode`, the
/// keymap's argument, not `ctx` (improvise-ct5/4ju) — see `context/code/command.md`.
fn nothing_at_cursor(&self) -> &'static str {
if self.edit_mode.is_records() {
"No record at cursor — press o to add a row"
@@ -339,9 +295,8 @@ impl Cmd for EditOrDrill {
"edit-or-drill"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
// Only consider regular (non-virtual, non-label) categories on None
// as true aggregation. Virtuals like _Index/_Dim are always None in
// pivot mode and don't imply aggregation.
// Only **regular** categories on None are true aggregation: virtuals like
// `_Index`/`_Dim` are always None in pivot mode and imply nothing.
let regular_none = ctx.none_cats().iter().any(|c| {
ctx.model
.category(c)
@@ -368,9 +323,8 @@ impl Cmd for EditOrDrill {
}
}
/// Thin command wrapper around the `EnterEditAtCursor` effect so it can
/// participate in `Binding::Sequence`. `target_mode` is supplied as the
/// command argument by the keymap binding.
/// Thin command wrapper around the `EnterEditAtCursor` effect so it can join a
/// `Binding::Sequence`. `target_mode` is the keymap binding's argument.
#[derive(Debug)]
pub struct EnterEditAtCursorCmd {
pub target_mode: AppMode,
crates/improvise-command/src/command/cmd/navigation.rs
+8 -16
View File
@@ -4,8 +4,7 @@ use crate::view::Axis;
use super::core::{Cmd, CmdContext};
// ── Navigation commands ──────────────────────────────────────────────────────
// All navigation commands take explicit cursor state. The interactive spec
// fills position/bounds from context; the parser accepts them as args.
// All navigation commands take explicit cursor state; see `context/code/command.md`.
/// Shared viewport state for navigation commands.
#[derive(Debug, Clone, Default)]
@@ -154,9 +153,8 @@ impl Cmd for EnterAdvance {
} else if c < col_max {
(0, c + 1)
} else {
// Already at bottom-right — the advance premise no longer holds.
// Abort the rest of the batch so the caller's trailing effects
// (e.g. `CommitAndAdvance`'s `EnterEditAtCursor`) are skipped.
// Already at bottom-right: the advance premise no longer holds, so abort
// the rest of the batch and the caller's trailing effects are skipped.
return vec![Box::new(effect::AbortChain)];
};
viewport_effects(
@@ -246,11 +244,8 @@ impl Cmd for PagePrev {
}
}
/// Gather (cat_name, items, current_idx) for page-axis categories.
///
/// Deliberately `categories_on(Axis::Page)` and not `filtering_cats()`:
/// `Filter` coordinates constrain the data but are not navigable, so `[`
/// and `]` must not page them (improvise-dz8).
/// Page-axis categories as `(cat_name, items, current_idx)`. `categories_on(Page)`
/// and **not** `filtering_cats()`: Filter is not navigable (improvise-dz8).
pub(super) fn page_cat_data(ctx: &CmdContext) -> Vec<(String, Vec<String>, usize)> {
let view = ctx.view;
let page_cats: Vec<String> = view
@@ -290,9 +285,8 @@ mod tests {
use super::*;
use crate::command::cmd::test_helpers::*;
/// improvise-dz8: `[` and `]` page through Page-axis categories. A
/// coordinate the drill fixed sits on Filter and must be invisible to
/// them, or the user could page out of the drill they are inside.
/// improvise-dz8: a coordinate the drill fixed sits on Filter and must be
/// invisible to `[`/`]`, or the user could page out of the drill.
#[test]
fn page_cycling_skips_filter_coordinates() {
let mut m = three_cat_model_with_page();
@@ -367,9 +361,7 @@ mod tests {
);
}
/// At bottom-right `EnterAdvance` has no place to go, so it emits a
/// single `AbortChain` effect. Trailing effects in a `CommitAndAdvance`
/// batch (e.g. `EnterEditAtCursor`) are then skipped, which is how
/// At bottom-right `EnterAdvance` emits a single `AbortChain`, which is how
/// "Enter at bottom-right commits and exits editing" is realised.
#[test]
fn enter_advance_at_bottom_right_emits_abort_chain() {
crates/improvise-command/src/command/cmd/panel.rs
+10 -35
View File
@@ -103,9 +103,8 @@ mod tests {
);
}
/// Bug improvise-dqn: CyclePanelFocus always picked the first open panel,
/// ignoring the currently focused panel. With formula + category open and
/// focus on the formula panel, cycling must move to the category panel.
/// Bug improvise-dqn: `CyclePanelFocus` always picked the first open panel,
/// ignoring the currently focused one.
#[test]
fn cycle_panel_focus_advances_from_formula_to_category() {
let m = two_cat_model();
@@ -301,17 +300,8 @@ mod tests {
assert!(effects.is_empty());
}
/// improvise-aaw asked for this command to be replaced by a keymap
/// `Binding::Sequence [create-view X, switch-view X, enter-mode normal]`.
/// It cannot be: a `Sequence` carries **literal** args, and the name here
/// is computed from `ctx` — so a Sequence would hard-code one name and
/// the second `n` in the view panel would collide with the first.
///
/// The test was called `..._names_incrementally` and asserted only that
/// `CreateView`/`SwitchView`/`Normal` appeared — it never looked at the
/// name, which is the one thing that makes this command a command and
/// not a composition. Now it does, at two different view counts, because
/// a single count cannot tell "derived from the workbook" from "constant".
/// improvise-aaw: the name is computed from `ctx`, which a literal-arg `Sequence`
/// cannot do. Asserted at two view counts — see the evidence.
#[test]
fn create_and_switch_view_names_incrementally() {
let mut m = two_cat_model();
@@ -410,11 +400,8 @@ impl Cmd for TogglePanelVisibility {
}
}
/// Tab through open panels in order Formula → Category → View, wrapping.
/// The starting point is the panel currently focused (`ctx.mode`); from a
/// non-panel mode (e.g. Normal) focus goes to the first open panel. With a
/// single open panel this self-transitions back to it; with none open it is
/// a no-op.
/// Tab through open panels Formula → Category → View, wrapping, starting from the
/// focused one. One open panel self-transitions; none open is a no-op.
#[derive(Debug)]
pub struct CyclePanelFocus {
pub formula_open: bool,
@@ -601,9 +588,8 @@ impl Cmd for FilterToItem {
}
}
/// Delete the category or item at the panel cursor.
/// On a category header -> delete the whole category.
/// On an item row -> delete just that item.
/// Delete at the panel cursor: a category header deletes the whole category, an item
/// row deletes just that item.
#[derive(Debug)]
pub struct DeleteCategoryAtCursor;
impl Cmd for DeleteCategoryAtCursor {
@@ -658,19 +644,8 @@ impl Cmd for SwitchViewAtCursor {
}
}
/// Create a new view, switch to it, and return to Normal mode.
///
/// **Not a shim, despite the name** (improvise-aaw). The new view's name is
/// derived from `ctx` — `format!("View {}", views.len() + 1)` — and a keymap
/// `Binding::Sequence` can only carry literal args, so the proposed
/// `[create-view X, switch-view X, enter-mode normal]` would hard-code one
/// name and the second `n` in the view panel would collide with the first.
/// The derivation is the decision that makes this a command.
///
/// The ticket's two fallbacks are both behaviour changes wanting a product
/// decision, not refactors: prompting for a name is a new feature, and
/// making `create-view` switch by default changes `:create-view` for scripts
/// and duplicates what `grid.rs` does by hand for `_Drill`/`_Records`.
/// Create a new view, switch to it, return to Normal. **Not a shim, despite the
/// name** (improvise-aaw) — see `context/code/command.md`.
#[derive(Debug)]
pub struct CreateAndSwitchView;
impl Cmd for CreateAndSwitchView {
crates/improvise-command/src/command/cmd/registry.rs
+7 -29
View File
@@ -2,11 +2,8 @@ use crate::model::cell::CellKey;
use crate::ui::app::AppMode;
use crate::ui::effect::Panel;
/// Decode a mode-name string (as supplied by `enter-mode`/`edit-or-drill`
/// keymap bindings) into an `AppMode`.
/// The table itself is `AppMode::from_name`, beside `AppMode` in the
/// protocol crate: the spellings are keymap vocabulary and the keymap is
/// shared, so the browser client decodes `enter-mode` from the same list.
/// Decode a mode-name string into an `AppMode`. The table is `AppMode::from_name` in
/// the protocol crate: the spellings are keymap vocabulary and the keymap is shared.
fn parse_mode_name(s: &str) -> Result<AppMode, String> {
AppMode::from_name(s).ok_or_else(|| format!("Unknown mode: {s}"))
}
@@ -525,9 +522,7 @@ pub fn default_registry() -> CmdRegistry {
r.register_nullary(|| Box::new(CommandModeBackspace));
// ── Commit ───────────────────────────────────────────────────────────
// commit-cell-edit / commit-and-advance-right take a mode-name arg
// (e.g. "editing" or "records-editing") as args[0]. The keymap supplies
// it; the command never inspects ctx.mode.
// These take a mode-name arg as args[0]; the command never inspects ctx.mode.
r.register(
&CommitAndAdvance {
key: CellKey::new(vec![]),
@@ -651,21 +646,8 @@ mod tests {
.collect()
}
/// improvise-9cn / improvise-61f: every registration derives its name
/// from a prototype instance, so a typo in the name is impossible. What
/// the types do *not* catch is the commands whose `name()` branches on a
/// field — `ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`,
/// `CommitAndAdvance`. Their prototype and the value their constructor
/// builds are the same type, so registering `ViewNavigate { forward:
/// true }` while building `forward: false` compiles cleanly and files
/// the command under the wrong name, where dispatch silently misses.
/// Constructing every entry both ways and comparing names is the only
/// check that reaches that class.
///
/// Some entries are deliberately one-path — `commit-and-advance-right`
/// reads the edit buffer and cursor, so its parse fn always errors — but
/// every entry must be reachable by at least one path, or its name goes
/// unchecked.
/// improvise-9cn/61f: four commands name themselves off a *field*, so a wrong
/// variant compiles and files the command under the wrong name (gotcha 15).
#[test]
fn every_entry_constructs_a_command_with_its_registered_name() {
let r = default_registry();
@@ -706,12 +688,8 @@ mod tests {
);
}
/// A variant mix-up does not show up as a name mismatch — it shows up as
/// a collision. Registering `ViewNavigate { forward: false }` where
/// `forward: true` belongs files a second entry under "view-back" and
/// removes "view-forward" from the registry entirely; `parse` takes the
/// first match, so every surviving entry still agrees with its own name.
/// Uniqueness is what catches it.
/// A variant mix-up shows up as a **collision**, not a mismatch: the wrong entry
/// takes its sibling's name and the sibling vanishes. Uniqueness catches it.
#[test]
fn registered_names_are_unique() {
let r = default_registry();
crates/improvise-command/src/command/cmd/text_buffer.rs
+16 -67
View File
@@ -27,10 +27,8 @@ mod tests {
assert!(dbg.contains("he"), "Expected 'he' after pop, got: {dbg}");
}
/// Writing the "search" buffer means writing the search query, and
/// `write_buffer` is the one place that knows it (improvise-gdl). Every
/// buffer-writing command routes through it, so `clear-buffer search`
/// clears the query rather than parking an unread entry in the map.
/// Writing the "search" buffer means writing the query, and `write_buffer` is the
/// one place that knows it (improvise-gdl).
#[test]
fn writing_the_search_buffer_targets_the_search_query() {
assert!(
@@ -94,21 +92,8 @@ mod tests {
);
}
/// improvise-git proposed deleting the `SaveAndQuit` command and binding
/// `ZZ` to a `Binding::Sequence [w, force-quit]` instead — the repo's
/// "do not name a command AAndB" principle.
///
/// That premise is incomplete, and this test is why. `wq` is not only a
/// keybinding: it is a **registered command name**, so `:wq` reaches it
/// through the registry, and `Quit`'s own unsaved-changes message
/// advertises it ("Use :q! to force quit or :wq to save+quit"). A keymap
/// `Sequence` is not reachable from the `:` line, so deleting the command
/// deletes the ex-command with it.
///
/// Keeping the command *and* adding the Sequence would leave two
/// independently-written spellings of "save then quit", which is the
/// duplication this codebase keeps curing rather than creating. So this
/// pins the surface, and the conversion is declined on it.
/// improvise-git proposed replacing `SaveAndQuit` with a `Sequence`. `wq` is also
/// a *registered name* `:wq` reaches — see the evidence for the decline.
#[test]
fn colon_wq_saves_then_quits() {
let m = two_cat_model();
@@ -123,17 +108,8 @@ mod tests {
assert!(dbg.contains("Quit"), "`:wq` must then quit, got: {dbg}");
}
/// improvise-6sq: `ExecuteCommand` appends `change_mode(Normal)` unless
/// some effect reports `changes_mode()`. `EnterEditAtCursor` sets
/// `view_state.mode` but did not override it, so
/// `:enter-edit-at-cursor editing` produced
/// `[RebuildLayout, EnterEditAtCursor(Editing), ChangeMode(Normal)]` —
/// the trailing snap-back undid the edit mode and the user never got a
/// cursor in the minibuffer.
///
/// Asserting on `changes_mode()` alone is not enough here: the appended
/// `ChangeMode(Normal)` reports `true` itself, so the assertion has to
/// name the snap-back it is ruling out.
/// improvise-6sq: a trailing `ChangeMode(Normal)` undid the edit mode. The
/// assertion has to name the snap-back, not just check `changes_mode()`.
#[test]
fn colon_enter_edit_at_cursor_is_not_snapped_back_to_normal() {
let m = two_cat_model();
@@ -240,11 +216,8 @@ mod tests {
app.apply_effects(effects);
}
/// improvise-toc: `StartImportWizard` swaps the mode only when the file
/// parses, so a per-type `changes_mode()` cannot describe it. Reporting
/// `false` made `ExecuteCommand` append a return-to-Normal on top of the
/// success path: `:import d.json` opened the wizard and was immediately
/// snapped back, leaving the wizard drawn under the Normal keymap.
/// improvise-toc: `StartImportWizard` swaps the mode only on success, so a
/// per-type answer snapped `:import` straight back out of the wizard.
#[test]
fn colon_import_stays_in_the_wizard_on_success() {
let mut app = wizard_test_app();
@@ -257,9 +230,8 @@ mod tests {
);
}
/// The other half of the same bug: reporting `true` unconditionally would
/// strand the user in CommandMode when the import fails. A failed
/// `:import` must still fall back to Normal.
/// The other half: reporting `true` unconditionally would strand the user in
/// `CommandMode` when the import fails.
#[test]
fn colon_import_falls_back_to_normal_on_failure() {
let mut app = wizard_test_app();
@@ -272,10 +244,8 @@ mod tests {
);
}
/// The `ViewBack` half of improvise-toc: it restores the stored frame's
/// mode only when the back-stack is non-empty. With history present,
/// `:view-back` must keep the mode it restored; with an empty stack it
/// falls through to Normal like any other no-op command.
/// The `ViewBack` half of improvise-toc: it restores the stored frame's mode only
/// when the back-stack is non-empty, and otherwise falls through to Normal.
#[test]
fn colon_view_back_respects_whether_history_exists() {
use crate::ui::app::ViewFrame;
@@ -376,9 +346,8 @@ impl Cmd for CommandModeBackspace {
/// Execute the command in the "command" buffer (the `:` command line).
#[derive(Debug)]
/// Execute the `:` command buffer by delegating to the command registry.
/// The `:` prompt is just another frontend to the scripting language —
/// same parser as `improvise script`.
/// Execute the `:` command buffer through the registry. The `:` prompt is another
/// front end to the scripting language — the same parser as `improvise script`.
pub struct ExecuteCommand;
impl Cmd for ExecuteCommand {
fn name(&self) -> &'static str {
@@ -393,28 +362,8 @@ impl Cmd for ExecuteCommand {
match crate::command::parse::parse_line_with(ctx.registry, &raw) {
Ok(cmds) => {
// Leave the command line **first**, before anything that can
// fail runs.
//
// Still decided at apply time rather than here, because
// ViewBack, ViewForward and StartImportWizard move the mode
// only on some runs and no up-front answer is right
// (improvise-toc). Running it at the head does not weaken
// that: whatever a later effect sets, it sets *after* this,
// so the mode a `:` line ends in is unchanged in every case
// — the only difference is what happens when the batch never
// reaches the end.
//
// Which is the point. A failed `Save`/`SaveAs` sets
// `abort_effects` (gotcha 21), and an abort at the tail ate
// this restoration too, stranding the user in `CommandMode`
// with the stale command line drawn — where
// `draw_bottom_bar` renders the minibuffer *instead of* the
// status line, so the error the effect had just set was
// invisible. The abort must stop the *chain* (`:w bad . q`
// must not quit) and must not stop the *bookkeeping*, and
// ordering answers that without giving any effect a "runs
// anyway" flag: tidy-up goes where an abort cannot reach it.
// Leave the command line **first**, before anything that can fail:
// an abort must stop the chain, never the bookkeeping (gotcha 21).
let mut effects: Vec<Box<dyn Effect>> = vec![Box::new(
effect::ReturnToNormalUnlessMoved(ctx.mode.clone()),
)];
crates/improvise-command/src/command/cmd/tile.rs
+2 -3
View File
@@ -100,9 +100,8 @@ impl Cmd for MoveTileCursor {
}
}
/// Cycle or set the axis for the category at the tile cursor.
/// Stays in TileSelect mode so the user can adjust multiple tiles.
/// `axis: None` -> cycle, `axis: Some(a)` -> set to `a`.
/// Cycle or set the axis for the category at the tile cursor, staying in TileSelect.
/// `axis: None` cycles; `axis: Some(a)` sets.
#[derive(Debug)]
pub struct TileAxisOp {
pub axis: Option<Axis>,
crates/improvise-command/src/command/cmd/wizard.rs
+10 -19
View File
@@ -1,10 +1,5 @@
//! Import-wizard key handling.
//!
//! The wizard is a small step machine, and which key means what depends on
//! the step it is in. That decision is pure, so it lives here: the command
//! reads the wizard out of `CmdContext` and emits the discrete effects
//! (`WizardApply`, `CloseWizard`, `AcceptWizardModel`) that carry it out
//! (improvise-2wm).
//! Import-wizard key handling: which key means what depends on the step, and that
//! decision is pure, so it lives here (improvise-2wm, gotcha 22).
use crate::command::key::KeyCode;
use crate::import::wizard::{ImportWizard, WizardStep};
@@ -102,9 +97,8 @@ fn name_model_keys(key: KeyCode) -> Vec<Box<dyn Effect>> {
}
}
/// The confirmation step: the workbook is built and waiting. Enter installs
/// it, and both Esc and Left return to `NameModel` — unlike every other
/// step, Esc here does not throw the import away (improvise-907).
/// The confirmation step: Enter installs the built workbook, and both Esc and Left
/// return to `NameModel` — Esc here does not discard the import (improvise-907).
fn done_keys(key: KeyCode) -> Vec<Box<dyn Effect>> {
match key {
KeyCode::Enter => vec![Box::new(effect::AcceptWizardModel)],
@@ -123,9 +117,8 @@ fn cursor_keys(key: KeyCode) -> Vec<Box<dyn Effect>> {
}
}
/// Left steps back to the previous step, on every step. It is the one key
/// free everywhere — Backspace edits the name and the formula buffer, and
/// `h` is a printable character the name step needs (improvise-907).
/// Left steps back on every step. It is the one key free everywhere: Backspace edits
/// buffers and `h` is a printable the name step needs (improvise-907).
fn back_key(key: KeyCode) -> Vec<Box<dyn Effect>> {
match key {
KeyCode::Left => op(WizardOp::Back),
@@ -202,9 +195,8 @@ mod tests {
assert!(ops(&w, KeyCode::Enter).contains("ConfirmFormula"));
}
/// Enter on the name step builds the workbook into the wizard rather
/// than advancing the step machine; every other printable key edits the
/// name.
/// Enter on the name step builds the workbook rather than advancing the step
/// machine; every other printable key edits the name.
#[test]
fn name_model_enter_builds_the_model() {
let w = wizard_at(WizardStep::NameModel);
@@ -234,9 +226,8 @@ mod tests {
}
}
/// On the confirmation step Enter installs the built workbook and Esc
/// steps back — it does not throw the import away, and no key closes
/// the wizard outright (improvise-907, from improvise-c6j).
/// On the confirmation step Enter installs the built workbook and Esc steps back;
/// no key closes the wizard outright (improvise-907, from improvise-c6j).
#[test]
fn done_confirms_on_enter_and_steps_back_on_esc() {
let w = wizard_at(WizardStep::Done);
crates/improvise-command/src/command/keymap.rs
+40 -159
View File
@@ -1,14 +1,5 @@
//! The half of the keymap that needs a command registry.
//!
//! The **table** — `KeyPattern`, `Binding`, `Keymap`, `ModeKey`, `TextEntry`,
//! `PanelKeymap` and `Keymaps::default_keymaps()` — lives in
//! `improvise_protocol::keymap`, so the browser client looks up the same
//! bindings the terminal does rather than mirroring them. Read that module's
//! docs for why.
//!
//! What is here is what a thin client cannot have: resolving a bound *name*
//! into `Effect`s through a [`CmdRegistry`], and the `SetTransientPrefix`
//! effect that arms a prefix key.
//! The half of the keymap that needs a command registry: resolving a bound *name*
//! into effects. The **table** is `improvise_protocol::keymap` (gotchas 11-14).
use std::sync::Arc;
@@ -22,13 +13,8 @@ pub use improvise_protocol::keymap::{
Binding, KeyPattern, Keymap, Keymaps, ModeKey, PanelKeymap, TextEntry,
};
/// Effect that arms a prefix key, so the next keypress is dispatched against
/// that prefix's sub-keymap.
///
/// It records the *address* of the prefix — the key that opened it — rather
/// than the sub-keymap itself. `KeymapSet` already owns every keymap, so
/// holding an `Arc<Keymap>` in `ViewState` would only put a non-portable type
/// into what is meant to become portable, shared state (improvise-jc8).
/// Effect that arms a prefix key. It records the *address* of the prefix, not the
/// sub-keymap: `ViewState` must stay portable (improvise-jc8, gotcha 13).
#[derive(Debug)]
pub struct SetTransientPrefix(pub KeyCode, pub KeyModifiers);
@@ -52,53 +38,8 @@ fn run(
Some(cmd.execute(ctx))
}
/// Turn a binding into effects. The one place a keymap lookup becomes a
/// mutation, and the reason `Binding` itself can be registry-free data.
///
/// # Why a `Sequence` flattens, and why it stays that way (improvise-0hb)
///
/// Every step is executed against the **same pre-dispatch `ctx`**, and the
/// effects are concatenated into one vec that `App::apply_effects` runs as
/// one batch. So a later *command* cannot observe an earlier one's effects —
/// only a later *effect* can. improvise-0hb proposed closing that: apply
/// effects so far between steps, rebuild the layout, build a fresh
/// `CmdContext`, dispatch the next. The design pass says **no**, on three
/// findings.
///
/// **It would silently reopen improvise-8tk, a P1 data loss.**
/// `apply_effects` resets `abort_effects` at entry *and* again when it
/// breaks, because the flag is per batch. Applying between steps therefore
/// dispatches step 2 into a fresh batch with the flag cleared, so the `Quit`
/// after a failed `Save` would land — the exact silent discard 8tk fixed.
/// Gotcha 21's claim that the abort "carries across sequence steps" is true
/// *because* of this flattening, not in spite of it. It was also untested
/// until now: no shipped `Sequence` has a first step that can abort, and
/// `ZZ` is a prefix onto the single `wq` command.
/// `app.rs::a_sequence_whose_first_step_aborts_does_not_run_the_second` is
/// the guard, and it fails when the abort is made per-step.
///
/// **Almost nothing would benefit.** Of the shipped `Sequence` bindings,
/// all but one come from `TextEntry` and are either `[<commit>, clear-buffer
/// <buf>]` or `[clear-buffer <buf>, enter-mode <exit>]` — `clear-buffer` and
/// `enter-mode` take their operand as a literal arg and read nothing from
/// `ctx`, so a refreshed `ctx` would change no outcome. The single genuine
/// composition is records `o` = `[add-record-row, enter-edit-at-cursor]`,
/// and it already works, because `EnterEditAtCursorCmd` fronts its effect
/// with `RebuildLayout` and the effect re-reads the layout itself (gotcha
/// 23). Every Esc and every commit keypress would pay a layout rebuild and
/// a `CmdContext` construction — which recomputes column widths — to buy
/// that nothing.
///
/// **And the ticket's motivating rewrite does not typecheck as a binding
/// anyway.** improvise-git's `[(w, []), (force-quit, [])]` cannot dispatch:
/// `w` is `register_pure`, whose interactive constructor rejects an empty
/// arg list, so `run` returns `None` and the whole binding is dead. The
/// nullary spelling is `save`. `every_bound_command_name_is_registered`
/// checks exactly this (gotcha 15) and would have caught it.
///
/// If command-level transactionality is ever wanted, it wants its own
/// campaign: the abort must become a property of the *sequence* rather than
/// of the batch before any step is applied separately.
/// Turn a binding into effects. A `Sequence` **flattens** into one batch, and
/// improvise-0hb's proposal to change that was declined — `context/code/command.md`.
fn effects_of(
binding: &Binding,
registry: &CmdRegistry,
@@ -430,15 +371,8 @@ mod tests {
}
}
/// improvise-th7m: `F1` was bound twice on consecutive lines — first to a
/// bare `enter-mode` with no args, then to `enter-mode ["help"]`. The
/// first was dead, because `Keymap::bind` *inserts* and the later binding
/// wins; had it ever been the surviving one it would have failed
/// `require_args` at dispatch and made `F1` do nothing.
///
/// The risk in deleting a line of a duplicated pair is deleting the wrong
/// one, so this pins which binding `F1` is meant to carry: the same one
/// `?` carries, which is what identified it as the intended survivor.
/// improvise-th7m: `F1` was bound twice and the first was dead. This pins which
/// binding it is meant to carry — the one `?` carries.
#[test]
fn f1_and_question_mark_both_open_help() {
let ks = KeymapSet::default_keymaps();
@@ -477,16 +411,8 @@ mod tests {
assert!(search.lookup(KeyCode::Esc, KeyModifiers::NONE).is_some());
}
/// improvise-9cn: `Keymap::dispatch` resolves a binding through
/// `registry.interactive(name, args, ctx).ok()?`, so a binding is a
/// silent no-op key in two ways, not one: the name may not be
/// registered, *or* the args may not parse. Both walks live here.
///
/// The args half matters because improvise-s13 moved bindings out of the
/// nullary class the name check covers: `enter-mode ["export-prompt"]`
/// and `enter-mode ["formula-edit"]` carry a mode name that
/// `parse_mode_name` must accept, and a typo like `"export_prompt"`
/// compiles, registers, and produces a dead `Ctrl-e`.
/// improvise-9cn/s13: a binding is a silent no-op key in two ways — the name may
/// not be registered, *or* the args may not parse. Both walks are here.
#[test]
fn every_bound_command_name_is_registered() {
let ks = KeymapSet::default_keymaps();
@@ -496,9 +422,8 @@ mod tests {
let mut checked = 0usize;
for (mode, km) in ks.maps().iter() {
// Build the context in the mode whose keymap this is: a few
// parsers read `ctx.mode` (`commit-buffer item` resolves its
// target category off `AppMode::ItemAdd`).
// Build the context in the mode whose keymap this is: `commit-buffer
// item` resolves its target category off `AppMode::ItemAdd`.
let app_mode = app_mode_for(mode);
let mut ctx = test_helpers::make_ctx(&wb, &layout, ks.registry());
ctx.mode = &app_mode;
@@ -538,17 +463,8 @@ mod tests {
assert!(checked > 100, "expected to walk the keymaps, saw {checked}");
}
/// improvise-2hi: `AppMode::hint_text` is the status bar's promise about
/// what the keys do, and nothing tied it to the keymaps.
/// `only_quit_has_no_hint` asserts a hint is *present*, never that it is
/// *accurate*, so rebinding or dropping a key leaves the hint quietly
/// lying to the user. This walks every `Key:verb` token in every hint and
/// requires the key to resolve in that mode's keymap.
///
/// The hint is the source of truth for *which* keys are worth mentioning;
/// the keymap is the source of truth for what they do. Only the first
/// half is checkable from here — a hint that says `i:edit` where `i`
/// enters records mode still passes.
/// improvise-2hi: every `Key:verb` token in every hint must resolve in that
/// mode's keymap. Which keys are named, never what they do — see the evidence.
#[test]
fn every_hint_key_resolves_in_its_mode() {
let ks = KeymapSet::default_keymaps();
@@ -571,9 +487,8 @@ mod tests {
assert!(checked > 50, "expected to walk the hints, saw {checked}");
}
/// Every key a hint mentions. Anything it cannot classify is a panic,
/// not a skip: a hint token this does not understand is exactly the
/// token that would go unchecked.
/// Every key a hint mentions. An unclassifiable token is a **panic, not a skip**:
/// that is exactly the token that would go unchecked.
fn hint_keys(hint: &str) -> Vec<(KeyCode, KeyModifiers)> {
// Everything after the em dash is prose ("— type a category name").
let head = hint.split('—').next().unwrap_or_default();
@@ -588,9 +503,8 @@ mod tests {
keys.extend(hint_key_spec(spec, hint));
seen_hint = true;
}
// A colon-less word continues the previous hint's verb
// ("Enter:add & continue", ":q quit"). One with nothing to
// continue is a token this test does not understand.
// A colon-less word continues the previous hint's verb; one with
// nothing to continue is a token this test does not understand.
None => assert!(
seen_hint,
"hint token {token:?} in {hint:?} is not a Key:verb hint"
@@ -679,12 +593,8 @@ mod tests {
keys
}
/// improvise-jc8: an armed prefix is stored as the key that opened it and
/// resolved against the CURRENT MODE's keymap. That is only correct while
/// prefixes are flat — a `Prefix` binding nested inside a sub-keymap
/// would be resolved against the mode map, where it does not exist, and
/// the second half of the sequence would silently do nothing. Adding a
/// nested prefix must fail here rather than in the terminal.
/// improvise-jc8: an armed prefix resolves against the current mode's keymap, so
/// a nested prefix would silently do nothing. Fail here, not in the terminal.
#[test]
fn prefix_keymaps_are_flat() {
let ks = KeymapSet::default_keymaps();
@@ -739,10 +649,8 @@ mod tests {
}
}
/// improvise-4pg: the three panel keymaps were written out separately, so
/// a spine binding could be changed in one and forgotten in the others.
/// They share `PanelKeymap` now; this pins what "the spine" is, including
/// that each panel's cursor keys move its *own* cursor.
/// improvise-4pg: the three panel keymaps share `PanelKeymap` now; this pins what
/// "the spine" is, each panel's cursor keys included.
#[test]
fn every_panel_keymap_shares_the_same_spine() {
let ks = KeymapSet::default_keymaps();
@@ -779,9 +687,8 @@ mod tests {
}
}
/// Every panel's toggle key is bound in Normal and in all three panel
/// modes, so focus can move between any two and the focused one closes.
/// A fourth panel gets all four keymaps from one `Panel::ALL` entry.
/// Every panel's toggle key is bound in Normal and in all three panel modes. A
/// fourth panel gets all four keymaps from one `Panel::ALL` entry.
#[test]
fn every_mode_with_panel_toggles_binds_all_of_them() {
let ks = KeymapSet::default_keymaps();
@@ -808,10 +715,8 @@ mod tests {
}
}
/// `Panel::name()` has to spell what the registry's panel parser accepts.
/// A lookup-only assertion would not catch a mismatch — `dispatch`
/// swallows an unresolvable command with `.ok()?` — so the toggle key
/// must come back with real effects.
/// `Panel::name()` must spell what the registry's parser accepts, and `dispatch`
/// swallows a mismatch with `.ok()?` — so real effects must come back.
#[test]
fn panel_toggle_keys_dispatch_to_real_effects() {
use crate::command::cmd::test_helpers::*;
@@ -837,10 +742,8 @@ mod tests {
}
}
/// The seven modes that carry a `MinibufferConfig`, each with the keys
/// that commit or leave it. `SearchMode` is not among them: it is
/// `Normal` plus a flag, so it has no `AppMode` variant and no
/// minibuffer config to agree with.
/// The seven modes carrying a `MinibufferConfig`, with their commit/leave keys.
/// `SearchMode` is Normal plus a flag, so it has no config to agree with.
fn text_entry_modes_with_exits() -> Vec<(AppMode, &'static [KeyCode])> {
const COMMIT_STAY: &[KeyCode] = &[KeyCode::Esc, KeyCode::Enter, KeyCode::Tab];
const COMMIT_LEAVE: &[KeyCode] = &[KeyCode::Esc, KeyCode::Enter];
@@ -876,12 +779,8 @@ mod tests {
}
}
/// improvise-k8h: the buffer name used to be written on both sides of the
/// crate — `AppMode`'s `MinibufferConfig` names the buffer the prompt
/// describes, and the keymap names the buffer `append-char` edits. If
/// those disagree the mode prompts over one buffer while every keystroke
/// edits another, with nothing to notice. `TextEntry` now reads the name
/// off the mode, and this pins the result for every text-entry mode.
/// improvise-k8h: the prompt's buffer and the buffer `append-char` edits used to
/// be written separately, and could disagree with nothing to notice.
#[test]
fn every_text_entry_mode_types_into_the_buffer_its_prompt_names() {
let ks = KeymapSet::default_keymaps();
@@ -899,9 +798,8 @@ mod tests {
}
}
/// The list above must stay complete: every keymap that types into a
/// buffer belongs to a mode whose minibuffer names that same buffer.
/// Only `SearchMode`, which has no `AppMode` variant, is exempt.
/// The list above must stay complete: every keymap that types into a buffer
/// belongs to a text-entry mode. Only `SearchMode` is exempt.
#[test]
fn every_typing_keymap_belongs_to_a_text_entry_mode() {
let ks = KeymapSet::default_keymaps();
@@ -929,18 +827,14 @@ mod tests {
}
}
/// Every minibuffer mode types into exactly one buffer, and every key that
/// commits or leaves must clear that same buffer on the way — a buffer left
/// behind pre-fills the next entry with the last one. `SearchMode` is
/// excluded on purpose: it has no commit and its query is owned by
/// `exit-search-mode`, not by `clear-buffer`.
/// Every commit or leave key clears the buffer it typed into — one left behind
/// pre-fills the next entry. `SearchMode` is excluded on purpose.
#[test]
fn text_entry_modes_clear_the_buffer_they_type_into() {
let none = KeyModifiers::NONE;
let ks = KeymapSet::default_keymaps();
// The buffer is derived from each mode rather than restated here —
// `every_text_entry_mode_types_into_the_buffer_its_prompt_names`
// owns pinning what it should be (improvise-k8h).
// The buffer is derived from each mode rather than restated here; pinning
// what it should be is k8h's test above.
for (app_mode, exits) in text_entry_modes_with_exits() {
let mode = ModeKey::from_app_mode(&app_mode, false).unwrap();
let buffer = append_target(&ks, &app_mode);
@@ -960,10 +854,8 @@ mod tests {
}
}
/// improvise-6t3: the three commit commands collapsed into a single
/// `commit-buffer <kind>`. `dispatch` swallows an unresolvable command name
/// with `.ok()?`, so a lookup-only assertion would still pass had the
/// rename missed the registry — Enter has to come back with real effects.
/// improvise-6t3: `dispatch` swallows an unresolvable name with `.ok()?`, so a
/// lookup-only assertion would pass had the rename missed the registry.
#[test]
fn minibuffer_enter_dispatches_to_a_registered_commit() {
use crate::command::cmd::test_helpers::*;
@@ -1018,19 +910,8 @@ mod binding_dump {
use super::tests_support::*;
use super::*;
/// The instrument `doctrine(binding_dump_diff)` asks for: every
/// `(mode, key, modifier)` lookup across all sixteen modes, written to
/// the file named by `IMPROVISE_BINDING_DUMP`.
///
/// Most bindings have no direct test, so a green suite is not evidence
/// that a keymap refactor preserved them. Dump before, dump after, diff.
/// It is inert without the variable: a test that writes a file nobody
/// asked for is a surprise, not a check.
///
/// This is what proved the keymap move to `improvise-protocol` changed
/// nothing — 4021 resolved lookups, byte-identical either side — and
/// the diff was shown to be discriminating by renaming one binding and
/// watching eight lines move.
/// `doctrine(binding_dump_diff)`'s instrument: every lookup in every mode, to the
/// file `IMPROVISE_BINDING_DUMP` names. Inert without it — see the evidence.
#[test]
fn dump_every_binding() {
let Ok(path) = std::env::var("IMPROVISE_BINDING_DUMP") else {
crates/improvise-command/src/command/mod.rs
+4 -9
View File
@@ -1,14 +1,9 @@
//! Command layer — all model mutations go through this layer so they can be
//! replayed, scripted, and tested without the TUI.
//!
//! Commands are trait objects (`dyn Cmd`) that produce effects (`dyn Effect`).
//! The headless CLI (--cmd / --script) parses quasi-lisp text into effects
//! and applies them directly.
//! Command layer — every model mutation goes through it, so mutations can be
//! replayed, scripted and tested without the TUI. See `context/code/command.md`.
pub mod cmd;
/// Key types moved to `improvise-protocol` (improvise-cqi) — they ride in
/// `ViewState::transient_prefix`, so they must live where the wire types do.
/// Re-exported here so `crate::command::key::…` paths keep working.
/// Key types live in `improvise-protocol` (improvise-cqi) — they ride in
/// `ViewState::transient_prefix`. Re-exported so existing paths keep working.
pub mod key {
pub use improvise_protocol::key::*;
}
crates/improvise-command/src/command/parse.rs
+2 -6
View File
@@ -1,9 +1,5 @@
//! Quasi-lisp prefix command parser.
//!
//! Syntax: `word arg1 arg2 ...`
//! Multiple commands on one line separated by `.`
//! Coordinate pairs use `/`: `Category/Item`
//! Quoted strings supported: `"Profit = Revenue - Cost"`
//! Quasi-lisp prefix command parser: `word arg1 arg2 …`, `.` between commands,
//! `Category/Item` coordinate pairs, `"quoted strings"`.
use super::cmd::{Cmd, CmdRegistry, default_registry};
crates/improvise-command/src/lib.rs
+4 -9
View File
@@ -1,7 +1,5 @@
//! Command layer and semantic app state for improvise — no terminal deps.
//!
//! Holds the `Cmd`/`Effect` pipeline, keymaps, and the `App` state machine.
//! Rendering lives in the root `improvise` crate.
//! Command layer and semantic app state for improvise — no terminal deps. The
//! `Cmd`/`Effect` pipeline, keymaps, and the `App` state machine.
pub mod command;
pub mod ui;
@@ -12,9 +10,6 @@ pub use improvise_core::{format, model, view, workbook};
pub use improvise_formula as formula;
pub use improvise_io::{import, persistence, storage};
/// The wire vocabulary and the per-widget caches (improvise-rkeq).
///
/// Re-exported so a host — the ratatui binary, a wasm worker — can name a
/// widget cache without taking its own `improvise-protocol` dependency, and
/// so the cache types a host renders are the same ones `ui::project` builds.
/// The wire vocabulary and the per-widget caches (improvise-rkeq), re-exported so a
/// host can name a cache without its own `improvise-protocol` dependency.
pub use improvise_protocol as protocol;
crates/improvise-command/src/ui/app.rs
+171 -635
View File
File diff suppressed because it is too large Load Diff
crates/improvise-command/src/ui/cat_tree.rs
+6 -16
View File
@@ -1,10 +1,5 @@
//! The category panel's flattening.
//!
//! The derivation itself moved to `improvise_protocol::viewmodel`
//! (improvise-edp), because the browser client needs it and can only reach
//! the protocol crate. What is left here is the path that call sites in this
//! crate and in the TUI already spell, and the one convenience that needs an
//! `App`: building the cache before flattening it.
//! The category panel's flattening. The derivation itself is
//! `improvise_protocol::viewmodel` (improvise-edp); what is left here is the path.
pub use improvise_protocol::viewmodel::{CatTreeEntry, flatten_cat_tree};
@@ -13,9 +8,8 @@ use std::collections::HashSet;
use crate::model::Model;
use crate::view::View;
/// Build the flattened tree of categories and their items straight from the
/// model, for a host that holds the authoritative state rather than a folded
/// cache — the same shortcut `project::category_tree_of` is for the widgets.
/// Build the flattened tree straight from the model, for a host that holds the
/// authoritative state — the shortcut `project::category_tree_of` is for widgets.
pub fn build_cat_tree(model: &Model, view: &View, expanded: &HashSet<String>) -> Vec<CatTreeEntry> {
let cache = improvise_protocol::CategoryTreeCache {
categories: crate::ui::project::category_rows(model, view),
@@ -48,12 +42,8 @@ mod tests {
names.iter().map(|n| n.to_string()).collect()
}
/// improvise-vjms: the panel lists the virtual categories alongside the
/// regular ones, and `_Measure`'s items are *derived* — explicit items ∪
/// formula targets (gotcha 7). An implementation reading
/// `category("_Measure").items` passes every other assertion here and
/// silently drops `Profit`, which is the failure improvise-ete was
/// closed for proposing.
/// improvise-vjms: the panel lists the virtual categories, and `_Measure`'s items
/// are *derived* (gotcha 7). Reading `.items` passes every other assertion here.
#[test]
fn the_tree_carries_the_virtual_categories_and_measure_includes_formula_targets() {
let mut wb = workbook_with_categories(&[("Region", &["North", "South"])]);
crates/improvise-command/src/ui/effect.rs
+77 -206
View File
@@ -6,9 +6,8 @@ use crate::view::{Axis, RecordCol, RecordRow};
use super::app::{App, AppMode, ViewFrame};
/// `Panel` is defined next to `AppMode` (the two are inverses of each other:
/// `Panel::mode` / `AppMode::panel`) and re-exported here, where most callers
/// already look for it.
/// `Panel` is defined next to `AppMode` (`Panel::mode` / `AppMode::panel` are
/// inverses) and re-exported here, where most callers already look for it.
pub use super::app::Panel;
pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty";
@@ -17,12 +16,8 @@ pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates canno
/// have to be told about it (improvise-mae).
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum EffectKind {
/// Changes only the acting client's own view — cursor, scroll, mode,
/// panels, minibuffer text, drill staging, status line. Never projected.
///
/// Cursor and scroll live in the shared `View` (they are persisted in
/// `.improv`), but they are a per-client concern: one client scrolling
/// must not move another's viewport. See improvise-8uz.
/// Changes only the acting client's own view — cursor, scroll, mode, panels,
/// minibuffer, drill staging, status line. Never projected (improvise-8uz).
ViewOnly,
/// Changes something every client renders.
Model(ModelChange),
@@ -46,33 +41,13 @@ pub enum ModelChange {
pub trait Effect: Debug {
fn apply(&self, app: &mut App);
/// How this effect must be projected to other clients. Deliberately has
/// no default: a new effect that forgets to classify itself would
/// silently never reach connected clients, so the compiler asks
/// (improvise-mae).
/// How this effect must be projected. **No default**: a new effect that
/// forgot to classify itself would silently reach no client (improvise-mae).
fn kind(&self) -> EffectKind;
}
/// Declare a setter effect: a one-field newtype whose `apply` writes that
/// field into one place on the `App`.
///
/// The kind is a **mandatory** argument, even though every call site so far
/// answers `ViewOnly`. Hard-coding it reintroduced exactly the default that
/// `Effect::kind` deliberately does without (gotcha 3): `$place` is an
/// unconstrained expression, so
/// `simple_effect!(X(String) => |app| app.model_state.workbook.active_view_mut().number_format, …)`
/// is a textbook `LayoutGeometry` write that the macro would have reported as
/// `ViewOnly`, silently, with the whole suite green. Writing the answer per
/// call site is the same reasoning that gave `kind()` no default.
///
/// Within that, the macro is still the single hook for anything these setters
/// should all do — today none of them marks the document dirty or rebuilds the
/// layout, and the macro makes that one decision instead of fifteen silent
/// ones (improvise-8fy).
///
/// `Clone::clone` rather than `.clone()`: the field is `usize` or `bool` as
/// often as it is a `String`, and the call form keeps `clippy::clone_on_copy`
/// off the generated code.
/// A one-field newtype whose `apply` writes that field into one place on the
/// `App`. The kind argument is mandatory — see `context/code/command.md`.
macro_rules! simple_effect {
($(#[$meta:meta])* $name:ident($ty:ty) => |$app:ident| $place:expr, $kind:expr) => {
$(#[$meta])*
@@ -95,17 +70,8 @@ macro_rules! simple_effect {
pub struct AddCategory(pub String);
impl Effect for AddCategory {
fn apply(&self, app: &mut App) {
// A refusal aborts the batch, for the same reason a failed save does
// (gotcha 21): what follows assumes the category exists. From the
// `+cat` column (improvise-2zq) the rest of the batch would advance
// the cursor and re-open an editor on a column that was never
// created — and re-opening the minibuffer is also what would *hide*
// this message, since `draw_bottom_bar` renders the minibuffer
// instead of the status line.
//
// `MAX_CATEGORIES` is a reachable refusal on that path rather than a
// theoretical one, and the limit is checked in exactly one place —
// `Model::add_category` — so nothing here restates it.
// A refusal aborts the batch for gotcha 21's reason: what follows
// assumes the category exists (improvise-2zq).
if let Err(e) = app.model_state.workbook.add_category(&self.0) {
app.view_state.status_msg = e.to_string();
app.abort_effects = true;
@@ -202,9 +168,8 @@ impl Effect for AddFormula {
fn apply(&self, app: &mut App) {
match crate::formula::parse_formula(&self.raw, &self.target_category) {
Ok(formula) => {
// For non-_Measure targets, add the item to the category so it
// appears in the grid. _Measure targets are dynamically included
// via Model::measure_item_names().
// Non-_Measure targets need the item added so it appears in the
// grid; _Measure targets arrive via `measure_item_names()`.
if !crate::model::category::CategoryKind::of_name(&formula.target_category)
.is_measure()
{
@@ -242,9 +207,8 @@ impl Effect for RemoveFormula {
}
}
/// Refresh the derived grid layout mid-batch. `apply_effects` rebuilds once
/// after the whole batch, so any effect that must *read* the layout after an
/// earlier effect changed it needs this in front of it (improvise-ehz).
/// 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)]
pub struct RebuildLayout;
impl Effect for RebuildLayout {
@@ -256,16 +220,8 @@ impl Effect for RebuildLayout {
}
}
/// Re-enter edit mode by reading the cell value at the current cursor.
/// Used after commit+advance to continue data entry.
///
/// Reads the *current* layout — emit `RebuildLayout` first when an earlier
/// effect in the batch moved the cursor or changed the grid.
///
/// `target_mode` is supplied by the caller (keymap binding via
/// `EnterEditAtCursorCmd`, or `CommitAndAdvance` from its own `edit_mode`
/// field). The effect itself never inspects `app.view_state.mode` — the mode is decided
/// statically by whoever invoked us.
/// Re-enter edit mode from the cell at the cursor. `target_mode` is the caller's
/// decision, never `app.view_state.mode` — see `context/code/command.md`.
#[derive(Debug)]
pub struct EnterEditAtCursor {
pub target_mode: AppMode,
@@ -506,9 +462,8 @@ pub struct TransposeAxes;
impl Effect for TransposeAxes {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().transpose_axes();
// Swapping the axes renumbers every row and column, so the old
// indices name different cells. The reset used to live inside
// View::transpose_axes; it moved here with the cursor (improvise-8uz).
// Swapping the axes renumbers every row and column, so the old indices
// name different cells (improvise-8uz, gotcha 24).
app.reset_cursor();
}
fn kind(&self) -> EffectKind {
@@ -565,16 +520,8 @@ simple_effect!(SetColOffset(usize) => |app| app.cursor_mut().col_offset, EffectK
simple_effect!(ChangeMode(AppMode) => |app| app.view_state.mode, EffectKind::ViewOnly);
/// Return to `Normal` unless something earlier in this batch already moved
/// the mode away from `from` — the mode the batch started in.
///
/// `ExecuteCommand` used to decide this up front by asking each effect a
/// per-type `changes_mode()`. That cannot describe `ViewBack`, `ViewForward`
/// or `StartImportWizard`, which swap the mode only on some runs: answering
/// "no" snapped a successful `:import` straight back out of the wizard,
/// while answering "yes" would have stranded a failed one in CommandMode.
/// Reading the mode here, after the batch has run, needs no prediction
/// (improvise-toc).
/// Return to `Normal` unless something earlier in this batch already moved the
/// mode away from `from`. Observed, never predicted (improvise-toc, gotcha 31).
#[derive(Debug)]
pub struct ReturnToNormalUnlessMoved(pub AppMode);
impl Effect for ReturnToNormalUnlessMoved {
@@ -596,12 +543,8 @@ simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query, Effe
simple_effect!(SetSearchMode(bool) => |app| app.view_state.search_mode, EffectKind::ViewOnly);
/// Set a named buffer's contents.
///
/// Every name goes into the buffers map. The search query is not a buffer —
/// it has its own `ViewState` field and its own `SetSearchQuery` effect — and
/// commands reach the right one through `cmd::core::write_buffer`, next to
/// the `read_buffer` it mirrors (improvise-gdl).
/// Set a named buffer's contents. The search query is *not* a buffer; commands
/// pick between the two in `cmd::core::write_buffer` (improvise-gdl).
#[derive(Debug)]
pub struct SetBuffer {
pub name: String,
@@ -658,17 +601,13 @@ impl Effect for ApplyAndClearDrill {
RecordCol::Category(category) => {
apply_coordinate_rename(app, row, category, new_value);
}
// Never staged — `commit_cell_value` applies a new measure
// or category immediately, since a schema change is not a
// cell edit and has nothing to roll back with the drill.
// Never staged: `commit_cell_value` applies a new measure or
// category immediately — a schema change is not a cell edit.
RecordCol::NewMeasure | RecordCol::NewCategory => {}
}
}
// No `dirty = true` here: this declares `Model(CellData)` and
// `App::apply_effects` raises the flag off that (improvise-gv9r). The
// line that used to be here was the last survivor of `MarkDirty` —
// harmless, and a second home for the one thing that commit set out
// to give exactly one.
// `App::apply_effects` raises the flag off that (improvise-gv9r).
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
@@ -754,9 +693,8 @@ impl Effect for Save {
app.model_state.dirty = false;
app.view_state.status_msg = msg;
}
// A failed save aborts the rest of the batch, so the `Quit`
// that `wq`/`ZZ` chains after this never lands and the error
// stays on screen (improvise-8tk).
// A failed save aborts the rest of the batch, so the `Quit` that
// `wq`/`ZZ` chains never lands (improvise-8tk, gotcha 21).
Err(msg) => {
app.view_state.status_msg = msg;
app.abort_effects = true;
@@ -794,9 +732,8 @@ impl Effect for SaveAs {
app.model_state.dirty = false;
app.view_state.status_msg = format!("Saved to {}", self.0.display());
}
// The same abort `Save` owes (improvise-8tk). `SaveAs` is the
// other arm of that rule and was left out of the original fix,
// so `:w /bad/path . q` quit on a failed write.
// The same abort `Save` owes: `SaveAs` is the arm improvise-8tk
// left out, so `:w /bad/path . q` quit on a failed write.
Err(e) => {
app.view_state.status_msg = format!("Save error: {e}");
app.abort_effects = true;
@@ -808,9 +745,8 @@ impl Effect for SaveAs {
}
}
/// One discrete mutation of the import wizard's own state. Which op a key
/// means in a given step is decided by `HandleWizardKey`; this enum is just
/// the verb (improvise-2wm).
/// One discrete mutation of the import wizard's own state. Which op a key means
/// in a given step is `HandleWizardKey`'s call; this is just the verb (improvise-2wm).
#[derive(Debug, Clone, PartialEq)]
pub enum WizardOp {
Advance,
@@ -881,9 +817,8 @@ impl Effect for CloseWizard {
}
}
/// Commit the wizard: build a workbook from its proposals and install it.
/// On failure the wizard stays open carrying the error so the user can go
/// back and fix their field choices.
/// Commit the wizard: build a workbook from its proposals and install it. On
/// failure the wizard stays open carrying the error, so the user can go back.
#[derive(Debug)]
pub struct AcceptWizardModel;
impl Effect for AcceptWizardModel {
@@ -978,10 +913,8 @@ impl Effect for LoadModel {
}
}
/// Extract the records array from an already-parsed JSON import value.
/// Precedence: explicit `array_path` → root array → first auto-detected
/// array path. Pure — lets [`ImportJsonHeadless`] parse the file exactly
/// once and reuse the parsed value as the pipeline's `raw` (improvise-oaq).
/// The records array out of an already-parsed JSON import value: explicit
/// `array_path` → root array → first auto-detected. Pure (improvise-oaq).
fn json_import_records(
value: &serde_json::Value,
array_path: Option<&str>,
@@ -1022,9 +955,8 @@ impl Effect for ImportJsonHeadless {
.extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("csv"));
// Parse the file exactly once: `records` feeds analysis, `raw` is the
// parsed value reused for the pipeline (improvise-oaq). CSV warnings
// (short rows, improvise-k8i) are threaded into the status message.
// Parse the file exactly once: `records` feeds analysis, `raw` is reused
// as the pipeline's value (improvise-oaq). CSV warnings go to the status.
let mut warnings: Vec<String> = vec![];
let (records, raw) = if is_csv {
let bytes = match app.storage.read(&self.path) {
@@ -1152,11 +1084,8 @@ impl Effect for SetPanelCursor {
// ── Chain control ────────────────────────────────────────────────────────────
/// Signals `App::apply_effects` to skip the remaining effects in the batch.
/// The flag is reset at the start of every `apply_effects` call, so each
/// dispatch starts clean. Use this when a sequence's premise no longer
/// holds (e.g. "advance to next cell" at bottom-right) and later effects
/// (e.g. "re-enter editing there") should be short-circuited.
/// Skip the remaining effects in this `apply_effects` batch, for when a
/// sequence's premise no longer holds. The flag resets per dispatch.
#[derive(Debug)]
pub struct AbortChain;
impl Effect for AbortChain {
@@ -1170,10 +1099,8 @@ impl Effect for AbortChain {
// ── Records hygiene ──────────────────────────────────────────────────────────
/// Remove cells whose `CellKey` has no coordinates — these are meaningless
/// records that can only be produced by `AddRecordRow` when no page
/// filters are set. Pushed by `ToggleRecordsMode` when leaving records
/// mode, as the inverse of the `SortData` that runs on entry.
/// Remove cells whose `CellKey` has no coordinates — only `AddRecordRow` with no
/// page filters makes those. The inverse of the entry `SortData`.
#[derive(Debug)]
pub struct CleanEmptyRecords;
impl Effect for CleanEmptyRecords {
@@ -1276,11 +1203,8 @@ mod tests {
// ── Storage injection (improvise-fuj) ───────────────────────────────
/// improvise-fuj: every effect that touches a document goes through
/// `App::storage`, so the whole save/load/import/export surface works on
/// a backend that is not a filesystem. This is what a browser worker
/// will supply (improvise-i34); here `MemoryFs` stands in for it, and
/// the test would fail if any of these effects reached for `std::fs`.
/// improvise-fuj: the whole document surface runs on a `Storage` that is not
/// a filesystem. Fails if any of these effects reaches for `std::fs`.
#[test]
fn the_document_surface_runs_on_a_non_filesystem_backend() {
use crate::storage::{MemoryFs, Storage};
@@ -1359,11 +1283,8 @@ mod tests {
serde_json::to_string(&app.model_state.workbook.views).expect("views serialize")
}
/// improvise-mae: a declared `kind()` is only worth having if it matches
/// what the effect actually does. Rather than restate each declaration,
/// this applies the effect and checks the shared state it claims not to
/// touch is genuinely unchanged — a `ViewOnly` effect that quietly wrote
/// a cell would never reach connected clients.
/// improvise-mae: rather than restate each declaration, apply the effect and
/// check the shared state it claims not to touch is genuinely unchanged.
#[test]
fn view_only_effects_leave_shared_state_alone() {
let cases: Vec<Box<dyn Effect>> = vec![
@@ -1408,9 +1329,8 @@ mod tests {
model_before,
"{effect:?} declares ViewOnly but changed model data"
);
// No exemptions: since improvise-8uz moved the cursor out of
// View, SetSelected/SetRowOffset/SetColOffset touch nothing
// shared, so every effect in this list must clear the same bar.
// No exemptions: since improvise-8uz moved the cursor out of `View`,
// every effect in this list must clear the same bar.
assert_eq!(
view_config_fingerprint(&app),
views_before,
@@ -1419,15 +1339,8 @@ mod tests {
}
}
/// The structural effects, each pinned to the kind it must declare.
///
/// The bug this guards: `AddCategory` and `RemoveCategory` declared
/// `CellData`, but both go through `Workbook`, whose `on_category_added`
/// / `on_category_removed` rewrite **every** view's axis map. That is a
/// re-layout, so a client told only "cell data changed" would keep
/// rendering the old grid shape. The two "actually change" tests below
/// cannot see it — a category change moves model data *and* view config,
/// so it passes either list. Only pinning the declaration does.
/// The structural effects, each pinned to the kind it must declare. The two
/// "actually change" tests below cannot separate them — see the evidence.
#[test]
fn structural_effects_declare_their_kind() {
let cell = ModelChange::CellData;
@@ -1582,12 +1495,8 @@ mod tests {
assert!(items.contains(&"Electronics"));
}
/// Bug (improvise-djn): `AddCategory` discarded the `Result` from
/// `Workbook::add_category`, so pressing `n` in the category panel with
/// the 12-regular-category limit already reached reported the optimistic
/// `Added category "X"` its command had queued and added nothing. The
/// effect runs last precisely so its failure can overwrite that message
/// (improvise-6t3) — it just never wrote one.
/// Bug (improvise-djn): `AddCategory` discarded the `Result`, so the panel
/// reported its command's optimistic message and added nothing.
#[test]
fn add_category_past_the_limit_reports_the_failure() {
let mut app = test_app();
@@ -1651,9 +1560,8 @@ mod tests {
assert!(!app.model_state.workbook.model.formulas().is_empty());
}
/// Regression: AddFormula must add the target item to the target category
/// so the grid layout includes cells for it. Without this, a formula like
/// `Margin = Profit / Revenue` would be registered but invisible in the grid.
/// Regression: `AddFormula` must add the target item to the target category,
/// or the formula is registered but has no cells in the grid.
#[test]
fn add_formula_adds_target_item_to_category() {
let mut app = test_app();
@@ -1894,8 +1802,7 @@ mod tests {
assert_eq!(app.view_state.mode, AppMode::Help);
}
/// `AbortChain` must cause subsequent effects in the same
/// `apply_effects` batch to be skipped, and the flag must reset so the
/// `AbortChain` skips the rest of the batch, and the flag must reset so the
/// next dispatch starts clean.
#[test]
fn abort_chain_short_circuits_apply_effects() {
@@ -1920,10 +1827,8 @@ mod tests {
assert_eq!(app.view_state.status_msg, "next-batch");
}
/// BUG (improvise-8tk): `Save` reported failure only through the status
/// bar, so the `Quit` that `wq`/`ZZ` chains after it landed anyway and
/// the whole session was discarded without the user seeing the error.
/// A failed save must abort the rest of the batch.
/// BUG (improvise-8tk): `Save` reported failure only to the status bar, so
/// the chained `Quit` landed anyway and the session was discarded.
#[test]
fn save_without_file_path_aborts_the_chain() {
let mut app = test_app();
@@ -1965,24 +1870,8 @@ mod tests {
);
}
/// BUG (improvise-lae, found reviewing it): improvise-8tk fixed **one
/// arm** of a rule with two. `Save` aborts the batch when the write
/// fails; `SaveAs` reported the error to the status bar and let the
/// batch continue, which is the same silent discard 8tk closed — the
/// shape `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
/// names, and the three tests above are evidence about `Save` only.
///
/// It is reachable today: the `:` line composes with ` . `, so
/// `:w /bad/path . q` builds exactly this batch, and
/// `ExecuteCommand` concatenates the two commands' effects into one.
/// improvise-lae's new save prompt also commits to `SaveAs`, so the
/// arm was about to carry more traffic.
///
/// Note what the abort does and does not buy the prompt: the typed path
/// survives for the user to correct, but the message does not reach the
/// screen, because `draw_bottom_bar` draws the minibuffer instead of the
/// status line while a prompt is open. That is pre-existing and reported
/// separately; the data-loss half is what this fixes.
/// BUG (improvise-lae): improvise-8tk fixed **one arm** of a rule with two.
/// `:w /bad/path . q` builds exactly this batch — see the evidence.
#[test]
fn failed_save_as_aborts_the_chain() {
let mut app = test_app();
@@ -2009,8 +1898,7 @@ mod tests {
);
}
/// The positive arm: a `SaveAs` that succeeds must not abort, or every
/// `:w <path>` would swallow whatever followed it. Without this,
/// The positive arm: a `SaveAs` that succeeds must not abort. Without it,
/// "always abort" passes the test above.
#[test]
fn successful_save_as_lets_the_chain_continue() {
@@ -2088,10 +1976,8 @@ mod tests {
);
}
/// `EnterEditAtCursor` must use its `target_mode` field, *not* whatever
/// `app.view_state.mode` happens to be when applied. Previous implementation
/// branched on `app.view_state.mode.is_records()` — the parameterized version
/// trusts the caller (keymap or composing command).
/// `EnterEditAtCursor` uses its `target_mode` field, not whatever
/// `app.view_state.mode` happens to be when it is applied.
#[test]
fn enter_edit_at_cursor_uses_target_mode_not_app_mode() {
let mut app = test_app();
@@ -2123,12 +2009,8 @@ mod tests {
);
}
/// improvise-ehz: the edit buffer must be loaded from layout state that
/// already reflects earlier effects in the same batch. `apply_effects`
/// only rebuilds *after* the batch, so a preceding `RebuildLayout` is
/// what makes the cursor resolve to the post-mutation cell. Removing the
/// row-0 item slides a different cell under the cursor; the buffer must
/// show that cell's value, not the pre-batch one.
/// improvise-ehz: the edit buffer must read layout that already reflects
/// earlier effects in the batch, which is what a preceding `RebuildLayout` buys.
#[test]
fn enter_edit_at_cursor_reads_post_batch_layout() {
let mut app = test_app();
@@ -2234,10 +2116,8 @@ mod tests {
assert_eq!(app.view_state.buffers.get("edit").unwrap(), "hello");
}
/// `SetBuffer` no longer treats "search" as an alias for the search query
/// (improvise-gdl): it is a plain buffer write whatever the name says, and
/// the query is reached through `SetSearchQuery`. Commands pick between
/// the two in `cmd::core::write_buffer`.
/// `SetBuffer` does not treat "search" as an alias for the search query
/// (improvise-gdl); the query is reached through `SetSearchQuery`.
#[test]
fn set_buffer_does_not_special_case_the_search_name() {
let mut app = test_app();
@@ -2327,10 +2207,8 @@ mod tests {
StartDrill(records).apply(&mut app);
assert!(app.drill_state.is_some());
// Apply with no pending edits — should just clear state
// Through `apply_effects`, not `apply`: the dirty flag is raised
// off the `EffectKind` now (improvise-gv9r), so driving the effect
// directly would assert a flag nothing in production sets.
// Through `apply_effects`, not `apply`: the dirty flag is raised off the
// `EffectKind` (improvise-gv9r), so `apply` would assert nothing real.
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
assert!(app.drill_state.is_none());
assert!(
@@ -2373,9 +2251,8 @@ mod tests {
}
.apply(&mut app);
// Through `apply_effects`, not `apply`: the dirty flag is raised
// off the `EffectKind` now (improvise-gv9r), so driving the effect
// directly would assert a flag nothing in production sets.
// Through `apply_effects`, not `apply`: the dirty flag is raised off the
// `EffectKind` (improvise-gv9r), so `apply` would assert nothing real.
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
assert!(app.drill_state.is_none());
assert!(app.model_state.dirty);
@@ -2385,10 +2262,8 @@ mod tests {
);
}
/// improvise-c5v: in wide format a records row groups one cell per
/// measure, so renaming a coordinate has to move every one of them. The
/// long-format code moved only the cell under the cursor, which would
/// now split the row in half.
/// improvise-c5v: in wide format a records row groups one cell per measure,
/// so renaming a coordinate has to move every one of them.
#[test]
fn apply_and_clear_drill_coord_rename_moves_every_measure_in_the_row() {
let mut app = test_app();
@@ -2421,9 +2296,8 @@ mod tests {
}
.apply(&mut app);
// Through `apply_effects`, not `apply`: the dirty flag is raised
// off the `EffectKind` now (improvise-gv9r), so driving the effect
// directly would assert a flag nothing in production sets.
// Through `apply_effects`, not `apply`: the dirty flag is raised off the
// `EffectKind` (improvise-gv9r), so `apply` would assert nothing real.
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
assert!(app.model_state.dirty);
let renamed = |measure: &str| {
@@ -2482,9 +2356,8 @@ mod tests {
}
.apply(&mut app);
// Through `apply_effects`, not `apply`: the dirty flag is raised
// off the `EffectKind` now (improvise-gv9r), so driving the effect
// directly would assert a flag nothing in production sets.
// Through `apply_effects`, not `apply`: the dirty flag is raised off the
// `EffectKind` (improvise-gv9r), so `apply` would assert nothing real.
app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
assert_eq!(app.model_state.workbook.model.get_cell(&key), None);
}
@@ -2652,10 +2525,8 @@ mod tests {
// ── Headless import ─────────────────────────────────────────────────
/// BUG (improvise-k8i): ImportJsonHeadless parsed CSVs with `parse_csv`,
/// which sends short-row warnings to stderr — invisible inside the TUI.
/// Short rows must surface in the status message, naming the affected
/// column, never silently succeed.
/// BUG (improvise-k8i): CSV short-row warnings went to stderr, invisible
/// inside the TUI. They must surface in the status message.
#[test]
fn import_headless_csv_short_rows_surface_warning_in_status() {
let dir = tempfile::tempdir().unwrap();
crates/improvise-command/src/ui/host.rs
+91 -394
View File
@@ -1,48 +1,5 @@
//! The session loop: messages in, addressed messages out.
//!
//! A websocket is awkward to test, so nothing that decides anything lives in
//! one. [`Host`] is a pure-ish state machine over the authoritative `App` —
//! it takes one client's frame and returns the frames that owes every
//! client — and [`crate::transport`] is the shell that moves bytes in and
//! out of it. Every test in this crate drives `Host` directly; the socket is
//! exercised by running the binary.
//!
//! **One `App` per document, many clients per `App`.** The ticket
//! (improvise-q08) asked for one `App` per *connection*, "no shared
//! authoritative state across sessions yet". That premise no longer holds
//! (`doctrine(check_the_ticket_premise)`): `Sessions` exists precisely to
//! hold many [`ClientSession`](crate::ui::project::ClientSession)s
//! against one authoritative `App`, `apply_switch`/`apply_model` move every
//! mirror in lockstep rather than the asker's
//! (`doctrine(lockstep_binds_every_session_not_the_asker)`), and
//! `Applied::Diff::broadcast` exists only because other clients must hear
//! what this one did. An `App` per connection would make all of that dead
//! code and give two browser tabs on the same document two documents.
//!
//! Nor is it one `App` per *server*. improvise is a multi-document app, and
//! the unit of authority is the **document**: a `Host` is one `App`, one
//! `Sessions`, and the clients attached to them. That grain is what
//! `Sessions` was already built for — it never owns the `App`, every method
//! takes it as a parameter — and it is what makes concurrency scope for
//! free: sessions editing independent models cannot conflict, and the basis
//! versions ADR 001 reasons about live on the `App`, one sequence per
//! document (`document_generation` is the `App`'s).
//!
//! So a second document is a second `Host`, and hosting several means a
//! registry of them keyed by document id. That registry is
//! `improvise_ws_server::registry` (improvise-j31x), and it cost this file
//! **nothing**:
//! a `Host` was always a value rather than a singleton, and `handle` is
//! already addressed to one document by virtue of being a method. What the
//! registry adds above is the only thing a `Host` cannot supply — a
//! server-wide address, since `SessionId`s are minted per `Sessions` and
//! every document's first client is `SessionId(0)`.
//!
//! What sharing one document costs is honest and stated: there is no
//! per-client permission, no conflict resolution beyond last-writer-wins,
//! and a client that can connect can rewrite the document. ADR 001 owns
//! where that goes next; this host is the localhost single-user case that
//! ADR describes the starting point of.
//! The session loop: messages in, addressed messages out. One `App` per
//! *document*, many clients per `App` — see `context/code/command.md`.
use crate::ui::app::App;
use crate::ui::effect;
@@ -52,24 +9,16 @@ use improvise_protocol::{Command, Rejection};
use improvise_protocol::{ClientFrame, ServerFrame};
/// One outbound frame and the connection it goes to.
///
/// The host addresses by [`SessionId`] and knows nothing about sockets; the
/// transport owns the map from a session to a connection.
/// One outbound frame and the connection it goes to. The host addresses by
/// [`SessionId`]; the transport owns the map from a session to a connection.
#[derive(Debug, Clone, PartialEq)]
pub struct Addressed {
pub to: SessionId,
pub msg: ServerFrame,
}
/// One authoritative document and every client attached to it.
///
/// The unit a multi-document server would key its registry by: a `Host` is
/// a value, never a global, so hosting several is a map of these and not a
/// change to anything inside one.
///
/// Not `Debug`: `App` is not, and a `Host` printing the whole document into
/// a log line would not be wanted even if it were.
/// One authoritative document and every client attached to it — a value, never a
/// global. Not `Debug`: `App` is not, and a log line of the document is not wanted.
pub struct Host {
app: App,
sessions: Sessions,
@@ -87,17 +36,8 @@ impl Host {
&self.app
}
/// The authoritative `App`, for work the **host itself** initiates.
///
/// Deliberately narrow in intent, and not a second door: a *client's*
/// `Command` must go through [`Host::handle`], which routes it into
/// `Sessions::apply` — the one place a basis is checked and the mirrors
/// are kept in lockstep (gotchas 36, 43). What this is for is the work a
/// host does on nobody's behalf, where there is no session to address and
/// no claim to check: the browser worker writes the document back through
/// the `Save` effect after a batch dirties it, because saving is
/// `EffectKind::ViewOnly` and so reaches no wire vocabulary
/// (`improvise-worker-server::worker`, which carries the argument).
/// The authoritative `App`, for work the **host itself** initiates. Not a second
/// door: a client's `Command` goes through [`Host::handle`] (gotchas 36, 43).
pub fn app_mut(&mut self) -> &mut App {
&mut self.app
}
@@ -106,13 +46,8 @@ impl Host {
self.sessions.is_empty()
}
/// Attach a client that has reported an extent of `rows` x `cols`, and
/// hand back its bootstrap.
///
/// Total: there is no way for a connection to fail here, which is why
/// the transport reads the extent off the first frame before calling.
/// Nothing is owed to the *other* clients — a new connection changes no
/// document state and moves no mirror but its own.
/// Attach a client that has reported an extent, and hand back its bootstrap.
/// Total, and nothing is owed to the other clients.
pub fn connect(&mut self, rows: usize, cols: usize) -> (SessionId, Addressed) {
let (id, snapshot) = self.sessions.connect(&self.app, rows, cols);
(
@@ -124,11 +59,8 @@ impl Host {
)
}
/// A model command from a client that is fully caught up, for the tests
/// whose subject is not ADR 001's basis. It goes through `handle` like
/// any other frame; only the basis is supplied rather than folded, and
/// [`caught_up`] can do that because it holds the `App` (a client
/// cannot). The staleness tests build their own `Basis`.
/// A model command from a client that is fully caught up, for the tests whose
/// subject is not the basis. The staleness tests build their own `Basis`.
#[cfg(test)]
fn model(&mut self, id: SessionId, cmd: improvise_protocol::ModelCommand) -> Vec<Addressed> {
let wire = crate::ui::reduce::caught_up(&self.app, cmd);
@@ -145,43 +77,15 @@ impl Host {
match msg {
ClientFrame::Viewport { rows, cols } => self.resize(id, rows, cols),
ClientFrame::Commands(cmds) => self.commands(id, &cmds),
// This arm answers nothing, and since improvise-bpq2 that is the
// settled answer rather than a gap. Resolving a keypress here
// needs `reduce_key` against the authoritative `App`, which emits
// no `Command` a client could fold — so the two would diverge,
// which `improvise-worker-server`'s
// `a_key_resolved_on_the_host_moves_state_no_client_can_hear`
// demonstrates. bpq2 closed that by moving resolution to the
// client, so a key a client can answer never arrives as a `Key`
// frame at all, and one that does is a binding nothing on this
// side could answer without diverging. Growing the set is a
// matter of enriching the projection until the client can
// compose the command itself — see `improvise-wasm-client::keys`,
// which carries the list of what is still upstream.
// Answering nothing is the settled answer, not a gap: host-side key
// resolution diverges (improvise-bpq2, gotcha 47).
ClientFrame::Key { .. } => Vec::new(),
ClientFrame::Save => self.save(id),
}
}
/// Write the document back, because a client asked (improvise-368h).
///
/// **No session state moves and nobody else is told**, which is why this
/// does not go through `Sessions` at all: saving changes no document
/// state and no client's view slice, so there is no mirror to adopt and
/// nothing to broadcast. What moved is bytes in *this host's* storage —
/// the same fact that makes `effect::Save` `EffectKind::ViewOnly` and the
/// reason the `ModelCommand` derivation could never reach it (gotchas 3,
/// 38). Success is silent, on the same terms as any accepted command that
/// dirties no widget.
///
/// Both refusals are the host's to answer, not `Checked`'s: neither is
/// about a basis. The missing path is decided **before** anything runs,
/// because a save that cannot land should be refused rather than
/// attempted and then reported. The failed write can only be found by
/// trying, and `Save` signals it by aborting its batch (gotcha 21) — the
/// batch here is exactly `[Save]`, so an abort is that failure and
/// nothing else. Reporting a failed write as success is the thing this
/// exists to stop.
/// Write the document back, because a client asked (improvise-368h). No session
/// state moves and nobody else is told — see `context/code/command.md`.
fn save(&mut self, id: SessionId) -> Vec<Addressed> {
let refused = match self.app.model_state.file_path.is_none() {
true => Some(Rejection::NoDocumentPath),
@@ -201,37 +105,20 @@ impl Host {
.collect()
}
/// A resize dirties no document state at all — but it moves this
/// client's viewport, and `ClientSession::project` emits the grid when
/// the viewport moved even under `Dirty::NOTHING`. So the fan-out is the
/// same call as everywhere else, with nothing dirty.
/// A resize dirties no document state, but it moves this client's viewport, and
/// `project` emits the grid on that alone. Same fan-out, nothing dirty.
fn resize(&mut self, id: SessionId, rows: usize, cols: usize) -> Vec<Addressed> {
match self.sessions.get_mut(id) {
Some(session) => session.set_extent(rows, cols),
// A frame for a session that has already gone. Nothing to do,
// and nothing that could be done: there is no connection left to
// answer.
// A frame for a session that has already gone: there is no connection
// left to answer.
None => return Vec::new(),
}
self.project(Dirty::NOTHING)
}
/// A batch: **answered whole, then applied whole** (improvise-955n).
///
/// Order is why a batch exists at all: one keypress resolved on a client
/// is `SetSelected` plus the scroll offsets that follow it, and a host
/// that folded the selection but not the offset projects that client the
/// wrong window.
///
/// It used to apply in order and abandon at the first rejection, which
/// left a hole ADR 001's "not even partially" does not have: a refused
/// frame's *earlier* commands stayed applied. The check has moved from
/// the command to the frame, so there is no longer a moment at which
/// part of a frame has landed and the rest has been refused — the
/// refusal, if any, happens before anything is applied, and
/// `Sessions::apply_checked` cannot produce one.
/// [`CheckedBatch`] carries what a batch's basis *means* and the one
/// residue this does not close.
/// A batch: **answered whole, then applied whole** (improvise-955n). [`CheckedBatch`]
/// carries what a basis means and the one residue this does not close.
fn commands(&mut self, id: SessionId, cmds: &[Command]) -> Vec<Addressed> {
let batch = match CheckedBatch::against(&self.app, cmds) {
Ok(batch) => batch,
@@ -251,14 +138,12 @@ impl Host {
out
}
/// The one door (gotcha 36), and the match that follows it is the
/// mechanism (gotcha 39): asking for the `Dirty` means naming the case
/// where a `Snapshot` is owed instead.
/// The one door (gotcha 36); the match that follows is the mechanism (gotcha 39):
/// asking for the `Dirty` means naming the case where a `Snapshot` is owed.
fn command(&mut self, id: SessionId, cmd: Checked<'_>) -> Vec<Addressed> {
match self.sessions.apply_checked(&mut self.app, id, cmd) {
// The document was replaced. Nothing to diff against and nothing
// to broadcast — each snapshot already carries a whole view slice
// and the seed that reproduces the new document.
// The document was replaced: nothing to diff and nothing to broadcast,
// each snapshot carrying a whole view slice and its seed.
Applied::Rebased { snapshots } => snapshots
.into_iter()
.map(|(to, snapshot)| Addressed {
@@ -267,25 +152,14 @@ impl Host {
})
.collect(),
Applied::Diff { dirty, broadcast } => {
// The broadcast precedes the projections, so a client's view
// slice is in the state the grid it is about to be sent was
// built from. Dropping it for the bystanders leaves their
// slices naming the outgoing view while that grid comes from
// the incoming one — a stale mirror arriving through
// somebody else's keystroke (gotcha 36), which is the bug
// PR #10 was found to have. Who each item reaches is carried
// by the item, not decided here.
// The broadcast precedes the projections, so a client's slice is in
// the state the grid it is about to get was built from (gotcha 36).
let mut out = self.relay(id, broadcast);
out.extend(self.project(dirty));
out
}
// Unreachable through this door since improvise-955n — a frame
// is refused before any of it is applied, by `commands` — but
// `Applied` is the shape `Sessions::apply` answers in and a host
// that wants the `Dirty` must name every case (gotcha 39). The
// answer is the same one `commands` gives: nothing was applied,
// so nothing is owed to anybody else, and the asker is told only
// that its batch did not land and which class that was.
// Unreachable through this door since improvise-955n, but a host that
// wants the `Dirty` must name every case (gotcha 39).
Applied::Rejected { reason } => vec![Addressed {
to: id,
msg: ServerFrame::Rejected(reason),
@@ -293,9 +167,8 @@ impl Host {
}
}
/// Who hears a relayed command is the relay's own business, not the
/// host's guess: [`Broadcast`] names its audience and the match is what
/// makes the host honour it (improvise-gvnb).
/// Who hears a relayed command is the relay's own business: [`Broadcast`] names
/// its audience and the match is what makes the host honour it (improvise-gvnb).
fn relay(&self, from: SessionId, broadcast: Vec<Broadcast>) -> Vec<Addressed> {
broadcast
.into_iter()
@@ -316,9 +189,8 @@ impl Host {
.collect()
}
/// Drain the outbound projections — the ticket's "projection queue",
/// which turned out to be a call rather than a queue. Clients with
/// nothing to hear are omitted by `project_all` and so get no frame.
/// Drain the outbound projections. Clients with nothing to hear are omitted by
/// `project_all` and so get no frame.
fn project(&mut self, dirty: Dirty) -> Vec<Addressed> {
self.sessions
.project_all(&self.app, dirty)
@@ -365,9 +237,8 @@ mod tests {
Host::new(app)
}
/// A host whose document can be replaced out from under its clients: a
/// second document sits in the storage backend under a name `LoadModel`
/// can reach.
/// A host whose document can be replaced out from under its clients: a second
/// document in the storage backend under a name `LoadModel` can reach.
fn host_over_a_loadable_document() -> (Host, PathBuf) {
let storage = MemoryFs::new();
let mut other = Workbook::new("Other");
@@ -389,9 +260,8 @@ mod tests {
])
}
/// The same cell as a client names it: the opaque surrogate the server
/// projected on `CellDisplay::id`, which is the only handle a client has
/// (ADR 002). A `CellKey` is not constructible over there.
/// The same cell as a client names it: the projected surrogate, which is the
/// only handle a client has (ADR 002). A `CellKey` is not constructible there.
fn addressed(region: &str) -> RecordCell {
RecordCell {
record: RecordHash::of(&cell(region)),
@@ -399,22 +269,8 @@ mod tests {
}
}
/// The basis a caught-up client would state for a **frame** naming
/// `regions`: the two document tiers plus each named record's current
/// counter.
///
/// `reduce::caught_up` cannot stand in for this, and the difference is
/// exactly what a frame is: it builds the claim for **one command**, so a
/// frame's records beyond the first are simply absent from it and come
/// back `RecordUnstated`. A real client calls `BasisCache::claim` once
/// with every record its frame names, which is what this reproduces.
///
/// **Name only records that exist.** `claim` *omits* a record the cache
/// has never heard of; this always includes one, so pointed at a record
/// that is not there it fabricates `(hash, 0)` — a claim no real client
/// could compose, and precisely the input that flips the create
/// exemption in `CheckedBatch`. For a record the frame is *creating*,
/// leave it out, which is what a client would do.
/// The basis a caught-up client states for a **frame** naming `regions`. It
/// fabricates `(hash, 0)` for a record that is absent — see the evidence.
fn frame_basis(host: &Host, regions: &[&str]) -> improvise_protocol::Basis {
improvise_protocol::Basis {
document_generation: host.app().document_generation(),
@@ -476,9 +332,8 @@ mod tests {
// ── connect ─────────────────────────────────────────────────────────
/// AC 2: the bootstrap is sent on connect, addressed to the new client,
/// and it is not empty — a `Snapshot` whose seed carried nothing would
/// satisfy "a snapshot was sent" while leaving the client blank.
/// AC 2: the bootstrap is sent on connect, addressed to the new client, and it
/// is **not empty** — an empty seed would satisfy "a snapshot was sent".
#[test]
fn connecting_hands_that_client_a_snapshot_that_draws_the_document() {
let mut host = host();
@@ -497,9 +352,8 @@ mod tests {
);
}
/// The extent is what the snapshot is built at, so a client that says it
/// can see one row is not sent two. This is the property that makes
/// `Viewport` the handshake rather than an afterthought.
/// The extent is what the snapshot is built at, which is what makes `Viewport`
/// the handshake rather than an afterthought.
#[test]
fn the_reported_extent_bounds_the_snapshot() {
let mut host = host();
@@ -545,9 +399,8 @@ mod tests {
assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "42");
}
/// `doctrine(check_the_checker)`: the assertion above would also pass if
/// the seed happened to contain the value already. Inject the fault —
/// never send the command — and it must fail.
/// `doctrine(check_the_checker)`: inject the fault — never send the command —
/// and the assertion above must fail.
#[test]
fn without_the_command_the_cell_is_empty() {
let mut host = host();
@@ -555,9 +408,8 @@ mod tests {
assert_eq!(cell_text(&folded_grid(&[connect.msg]), 0, 0), "");
}
/// A cell edit is the whole document's, so a client that did not ask for
/// it is told about it too. The projection is per-session because the
/// viewports differ, not because the change does.
/// A cell edit is the whole document's, so a client that did not ask is told
/// too. The projection is per-session because viewports differ.
#[test]
fn a_bystander_is_projected_another_clients_edit() {
let mut host = host();
@@ -577,9 +429,8 @@ mod tests {
assert_eq!(cell_text(&folded_grid(&seen), 0, 0), "7");
}
/// A view-only command reaches nobody else. The counterpart to the test
/// above: if every command fanned out, that one would pass for the wrong
/// reason.
/// A view-only command reaches nobody else — the counterpart that keeps the
/// test above from passing for the wrong reason.
#[test]
fn a_scroll_reaches_only_the_client_that_scrolled() {
let mut host = host();
@@ -598,15 +449,8 @@ mod tests {
);
}
/// One keypress resolved on a client is up to three commands, and the
/// host must take all of them (improvise-bpq2).
///
/// The fixture is the shape that actually arrives: a cursor moved past
/// the bottom of a one-row window, so the selection *and* the offset
/// that follows it travel together. The assertion is the offset, not the
/// selection, because a host that applied only the first command would
/// still look right on `selected` — the window is what goes wrong, and
/// it goes wrong silently.
/// One keypress is up to three commands and the host must take all of them
/// (improvise-bpq2). The assertion is the *offset*, not the selection.
#[test]
fn a_batch_of_commands_is_applied_whole_and_in_order() {
let mut host = host();
@@ -629,10 +473,8 @@ mod tests {
);
}
/// A refused batch applies nothing *after* the refusal. The stronger
/// half — nothing *before* it either — is
/// `a_refused_batch_leaves_the_commands_before_it_unapplied`; this is
/// kept because the two halves fail independently.
/// A refused batch applies nothing *after* the refusal. Nothing *before* it is
/// the next test; the two halves fail independently.
#[test]
fn a_batch_is_abandoned_at_the_first_rejection() {
use improvise_protocol::{Basis, ModelCommand};
@@ -664,29 +506,8 @@ mod tests {
);
}
/// improvise-955n / improvise-08wb. **The bug**: ADR 001 is emphatic
/// that a stale batch is never applied, *not even partially*, and
/// improvise-ondj went to the trouble of a type (`Checked`) to make the
/// unchecked path unconstructible. But `Checked` is **per command**, and
/// a `ClientFrame::Commands` is a sequence of them: `Host::commands`
/// applied in order and abandoned at the first rejection, so a frame
/// whose *second* command was refused had already applied its first.
/// The guarantee was true of a single command and false of a batch.
///
/// The earlier command is a **view** command, which is not a
/// concession: it is the shape the one real producer emits
/// (`keys::commit` — one mutation among view commands), and it is still
/// observable, because the host folds it into that client's mirror.
/// A frame may carry more than one mutation; what it may not do is
/// apply some of them and refuse the rest, which is clause 4 and open.
///
/// **A refusal test needs an accepting one**
/// (`doctrine(a_helper_that_builds_the_world_the_code_wants_tests_nothing)`:
/// pair a refusal with an acceptance, or the refusal can pass by
/// refusing everything). The second half runs the same leading
/// `SetSelected` followed by a command whose basis is *current*, and
/// requires both to land. Without it, a `SetSelected` that simply never
/// worked would clear the first assertion and prove nothing at all.
/// improvise-955n/08wb: `Checked` is per **command**, so a frame whose second
/// command was refused had already applied its first. See the evidence.
#[test]
fn a_refused_batch_leaves_the_commands_before_it_unapplied() {
use improvise_protocol::ModelCommand;
@@ -698,11 +519,8 @@ mod tests {
let mut refused = host();
let (asker, _) = refused.connect(10, 10);
// A claim that is current in every respect but one, so the refusal
// can only be about the tier that was moved. Building it from
// `caught_up` and then bumping a single tier keeps the rest
// well-formed — a hand-built basis that is stale in three ways at
// once cannot tell which check answered.
// Built from `caught_up` and then bumped in a single tier, so the refusal
// can only be about the tier that moved.
let Command::Model { mut basis, cmd } =
crate::ui::reduce::caught_up(refused.app(), edit(42.0))
else {
@@ -766,29 +584,8 @@ mod tests {
);
}
/// improvise-955n. **The regression 90ea26e introduced, and the reason
/// clause 2 cannot be applied to all three tiers.**
///
/// Hoisting the basis check to the frame hoisted *all* of it, so every
/// mutation after the first was admitted with no record check at all.
/// Two clients, one record: A composes `[SetCell(North), SetCell(South)]`
/// against an honest current basis, B writes South, A's frame arrives —
/// and A's write to South landed on top of B's **silently**, which is
/// exactly the lost update ADR 001's record tier exists to prevent. At
/// the base commit the second command was checked and the frame refused.
///
/// **The per-command check was doing two jobs and the commit noticed
/// one.** "Is the client's picture of the *document* current" is a
/// frame-level fact and hoists correctly. "Is the client's picture of
/// *this record* current" is not: which records are at stake is a
/// property of the command, which is why `records_named` is an exhaustive
/// match over `ModelCommand` (gotcha 43). The incapability argument that
/// motivated clause 3 is true of records a frame **creates** and was
/// over-generalised to records it merely **writes** — and those are the
/// ones the tier is for.
///
/// Three arms, because a fix that refuses everything would pass the first
/// alone and a fix that checks nothing would pass the last two.
/// improvise-955n: hoisting the basis check to the frame hoisted *all* of it,
/// and the record tier does not hoist. A lost update — see the evidence.
#[test]
fn a_later_write_in_a_chain_still_answers_for_its_own_record() {
use improvise_protocol::ModelCommand;
@@ -818,10 +615,8 @@ mod tests {
},
];
// B moves South. `SetCell` is `CellData`, so neither the document nor
// the layout tier moves — only the record tier can discriminate here,
// which is what makes this a test of the record tier and not of the
// hoist in general.
// B moves South. `SetCell` is `CellData`, so only the record tier can
// discriminate here — which is what makes this a test of that tier.
moved.model(b, write("South", 99.0));
let answer = moved.handle(a, ClientFrame::Commands(frame));
@@ -890,26 +685,7 @@ mod tests {
);
// ── arm 3: the same record twice in one chain ───────────────────
// Legitimate and likely — two measures on one record is one record —
// and it works for a reason worth pinning rather than rediscovering:
// **every check happens before any command is applied**, so the
// second write is compared to the record's *pre-frame* counter, not
// to the one its own sibling just bumped. Were the checks
// interleaved with the applies, this frame would refuse itself with
// `RecordMoved`.
//
// **Read this before closing clause 4.** Interleaving is tempting
// there, because it would close the *silence*:
// `[RemoveItem(North), SetCell(North)]` under interleaving is refused
// `RecordGone` instead of silently half-applying, which is half of
// what clause 4 owes. So check-before-apply is not a free win — it
// buys the legitimate double-write below and **sells** the loud
// failure on a broken chain, and clause 4 has to pay that back some
// other way. Measured rather than argued: injecting the interleaving
// fails exactly two tests, this one and
// `a_frame_whose_later_mutation_fails_is_not_yet_undone`, and the
// second failing *is* the trade. This arm is what stops someone
// taking the trade without noticing they made it.
// **Read the evidence before closing clause 4**: interleaving sells this.
let mut twice = host();
let (asker, _) = twice.connect(10, 10);
twice.model(asker, write("North", 1.0));
@@ -945,21 +721,8 @@ mod tests {
);
}
/// **The capability the contract is for**: a chain of data-dependent
/// commands, applied in order, each seeing what the previous one left.
///
/// `[AddItem("East"), SetCell(the East record)]` is the archetype, and it
/// is the case a per-command basis check **cannot** admit however it is
/// ordered: a client cannot claim a counter for a record its own chain is
/// about to create, because `BasisCache::claim` filters to records the
/// cache has been told about. So the basis is answered once, on the first
/// command that carries one, and the rest of the chain runs against what
/// its predecessors left.
///
/// The second assertion is the one that earns the test. Asserting only
/// that the item exists would pass against a host that applied the first
/// command and dropped the second — which is exactly what the previous
/// rule did, loudly, and what the pre-frame check did, silently.
/// **The capability the contract is for**: a data-dependent chain, in order.
/// The second assertion is the one that earns it — see the evidence.
#[test]
fn a_data_dependent_chain_lands_in_order() {
use improvise_protocol::ModelCommand;
@@ -1010,35 +773,8 @@ mod tests {
);
}
/// **Characterization of the open hole (clause 4), not an endorsement.**
///
/// The contract says a frame is all-or-nothing. It is not yet:
/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is
/// not a rollback), so a chain whose later mutation fails leaves the
/// earlier one standing. `[RemoveItem("North"), SetCell(the North
/// record)]` is the shape: the removal lands, the write then resolves to
/// nothing, `model_effects` returns an **empty** batch, and the frame is
/// reported accepted having applied half of itself. Both the partial
/// application and the silence are open.
///
/// **This test did not pin that, and the way it failed is the repo's own
/// signature defect sitting on the one test whose job is to fail loudly
/// if anyone believes the hole is closed.** It asserted that North was
/// removed and that nothing was rejected — both true of any *wholly
/// successful* frame. The independent reviewer retargeted the second
/// command from the removed North to the live South, making the frame
/// succeed completely, and the test still passed. It could not tell
/// half-applied from fully-applied, so it would never have inverted when
/// clause 4 landed; it would have gone on passing. The missing assertion
/// is the one that names the hole: **the second mutation did not land.**
///
/// It is unreachable from any client here — the only producer of a
/// multi-command frame is `keys::commit`, which emits one mutation among
/// view commands — which is what makes it safe to document rather than
/// rush.
///
/// **This test must be inverted when clause 4 lands**: all three
/// assertions flip, because then nothing at all happens.
/// **Characterization of the open hole (clause 4), not an endorsement.** Invert
/// all three assertions when clause 4 lands — see the evidence.
#[test]
fn a_frame_whose_later_mutation_fails_is_not_yet_undone() {
use improvise_protocol::ModelCommand;
@@ -1052,10 +788,8 @@ mod tests {
value: CellValue::Number(1.0),
},
);
// The frame claims North at its current counter, so it passes the
// check and fails at *apply* time — which is the only way to reach
// the hole. A frame refused by the check never applies anything and
// would be testing clause 2 instead.
// The frame claims North at its current counter, so it passes the check and
// fails at *apply* time — the only way to reach the hole.
let basis = frame_basis(&host, &["North"]);
let answer = host.handle(
asker,
@@ -1111,10 +845,8 @@ mod tests {
// ── the broadcast ───────────────────────────────────────────────────
/// The bug Copilot found on PR #10: a bystander must fold the accepted
/// command itself, because its own view slice lives on the client and
/// only `reduce_view` over the same command keeps it in step with the
/// server's mirror (gotcha 36).
/// The bug Copilot found on PR #10: a bystander must fold the accepted command
/// itself, because its own view slice lives on the client (gotcha 36).
#[test]
fn a_bystander_is_sent_the_broadcast_and_the_asker_is_not() {
let mut host = host();
@@ -1142,12 +874,8 @@ mod tests {
);
}
/// The other audience, and the reason [`Broadcast`] carries one rather
/// than the relay deciding. A geometry change invalidates every cursor
/// into the document (improvise-gvnb), and the asker's is not exempt: a
/// `Command::Model` is the one kind a client cannot fold optimistically,
/// so nothing has reset it locally and skipping it would leave the very
/// client that asked positioned in a grid it no longer has.
/// The other audience, and the reason [`Broadcast`] carries one: a geometry
/// change invalidates every cursor, the asker's included (improvise-gvnb).
#[test]
fn a_geometry_command_relays_the_cursor_reset_to_the_asker_too() {
use improvise_protocol::ModelCommand;
@@ -1167,10 +895,8 @@ mod tests {
}
}
/// A refused batch is answered to the **asker and nobody else** (ADR
/// 001). Nothing happened, so there is nothing to tell the others — and
/// the asker is told only which class of staleness it was, because it
/// already holds what is current.
/// A refused batch is answered to the **asker and nobody else** (ADR 001):
/// nothing happened, so there is nothing to tell the others.
#[test]
fn a_stale_batch_is_refused_to_the_asker_alone() {
use improvise_protocol::{Basis, Rejection};
@@ -1204,9 +930,8 @@ mod tests {
);
}
/// And what the broadcast is *for*: a bystander that folds what it is
/// sent ends up naming the view the document accepted, so the cursor its
/// next grid is positioned by is read under the right key (gotcha 24).
/// And what the broadcast is *for*: a bystander that folds it names the view the
/// document accepted, so its next grid is positioned under the right key.
#[test]
fn folding_the_broadcast_moves_the_bystanders_active_view() {
let mut host = host();
@@ -1236,9 +961,8 @@ mod tests {
// ── the document swap ───────────────────────────────────────────────
/// AC of gotcha 39: a command that replaced the document owes *every*
/// client a fresh `Snapshot`, not a diff — including the client that did
/// not ask.
/// AC of gotcha 39: a command that replaced the document owes *every* client a
/// fresh `Snapshot`, not a diff — including the client that did not ask.
#[test]
fn a_document_swap_sends_every_client_a_fresh_snapshot() {
let (mut host, path) = host_over_a_loadable_document();
@@ -1261,9 +985,8 @@ mod tests {
}
}
/// `doctrine(check_the_checker)`: inject the fault the test targets — a
/// load that does not happen — and the snapshot must not appear. A test
/// that saw `Snapshot` on every command would pass the one above.
/// `doctrine(check_the_checker)`: inject the fault — a load that does not
/// happen — and the snapshot must not appear.
#[test]
fn a_load_that_fails_is_not_a_rebase() {
let (mut host, _) = host_over_a_loadable_document();
@@ -1308,18 +1031,8 @@ mod tests {
.map(|bytes| String::from_utf8(bytes).expect("the fixture writes text"))
}
/// improvise-368h. **The bug**: `effect::Save` is `EffectKind::ViewOnly`
/// — correctly, since it writes *this host's* path — so the derivation
/// that builds `ModelCommand` out of the `EffectKind::Model` effects
/// could not reach it (gotcha 38). There was no save on the wire at all, so
/// **no client on any transport could cause its host to write**: a
/// websocket session edited the document in memory and the file on disk
/// never moved unless a human was at a TUI that was not there.
///
/// The derivation was not wrong; it was complete over the mutations and
/// saving is not one. So the variant is *added* rather than derived, and
/// it carries no path: a client cannot see the host's storage, and which
/// document a host is hosting is the host's own fact.
/// improvise-368h: `effect::Save` is `ViewOnly`, so the `ModelCommand` derivation
/// could not reach it and no client could make its host write.
#[test]
fn a_client_can_ask_its_host_to_save() {
let (mut host, path) = host_with_a_path();
@@ -1351,14 +1064,8 @@ mod tests {
);
}
/// improvise-368h: the "what if there is no path" half. Not a panic and
/// not a silent no-op — the two answers the ticket ruled out — but the
/// vocabulary that already exists for "nothing happened and here is
/// why", addressed to the asker alone (gotcha 39).
///
/// The second client is not decoration: a refusal that leaked to the
/// bystanders would be telling them about a failure of somebody else's
/// host-local request, and this asserts it does not.
/// improvise-368h, the "no path" half: refused with a reason, to the asker alone
/// (gotcha 39). The second client asserts the refusal does not leak.
#[test]
fn a_save_with_nowhere_to_write_is_refused_and_told_only_to_the_asker() {
let mut host = host();
@@ -1381,14 +1088,8 @@ mod tests {
);
}
/// improvise-368h: a write that was attempted and failed must not be
/// reported as a success. `Save` signals failure by aborting its batch
/// (gotcha 21), and the batch here is exactly `[Save]`, so an abort is
/// that failure and nothing else.
///
/// The fixture fails for a reason that is real rather than mocked: a
/// `PhysicalFs` pointed at a directory that does not exist. It creates
/// nothing, so the test leaves no trace on disk.
/// improvise-368h: an attempted write that failed must not be reported as a
/// success. The fixture fails for a real reason — an absent directory.
#[test]
fn a_write_that_fails_is_refused_rather_than_reported_as_saved() {
let absent = std::env::temp_dir().join("improvise-no-such-directory-368h");
@@ -1440,9 +1141,8 @@ mod tests {
// ── the frames themselves ───────────────────────────────────────────
/// The wire is JSON to start with (the ticket's choice, kept), so both
/// directions must survive a round trip through it — including the one
/// point at which `ViewState` is serialized (gotcha 25).
/// The wire is JSON, so both directions must survive a round trip through it —
/// including the one point at which `ViewState` is serialized (gotcha 25).
#[test]
fn every_frame_survives_a_json_round_trip() {
let mut host = host();
@@ -1468,11 +1168,8 @@ mod tests {
assert_eq!(back, frame.msg);
}
// Inbound, with a payload-bearing variant on each side of the
// envelope (`doctrine(populate_before_roundtrip)`): a `Viewport` and
// a `Command` carrying a `CellValue::Text`, which is the arm the
// outbound half cannot reach — a text cell aggregates to nothing
// under a None-axis category, so it never becomes a `CellDisplay`.
// A payload-bearing variant on each side of the envelope
// (`doctrine(populate_before_roundtrip)`) — see the evidence.
for inbound in [
ClientFrame::Viewport { rows: 3, cols: 4 },
ClientFrame::command(crate::ui::reduce::caught_up(
crates/improvise-command/src/ui/project.rs
+227 -902
View File
File diff suppressed because it is too large Load Diff
crates/improvise-command/src/ui/reduce.rs
+69 -577
View File
@@ -1,55 +1,5 @@
//! The one place a command is applied to the authoritative state
//! (improvise-gxi).
//!
//! Every deployment mode — the native TUI, a websocket server, a browser
//! worker — reaches the model through the functions here, so there is one
//! answer to "what does this input do" rather than one per host.
//!
//! One door, reached two ways, because input arrives in two shapes and only
//! one of them is data on the wire:
//!
//! - [`reduce_full`] takes a [`Checked`], which is a [`Command`] — the wire
//! vocabulary — that the authoritative state has already answered for.
//! Both halves of that vocabulary are derived from the effects — the view
//! slice from the `ViewOnly` ones (`decision(improvise-cqi,
//! command_set_derived_not_invented)`) and the document from the
//! `EffectKind::Model` ones (improvise-kmgs) — so a command can now say
//! "set this cell" as well as "move my cursor".
//! - [`reduce_key`] takes a keypress and runs it through the keymap,
//! registry and effect pipeline. A keypress names an *intent* whose
//! meaning depends on the `CmdContext` (where the cursor is, what mode is
//! current); a `Command` names the mutation that intent resolved to. That
//! is why the key path survives the vocabulary rather than being replaced
//! by it.
//!
//! What is no longer two is the *mutation*: [`model_effects`] maps a
//! [`ModelCommand`] back to the effect it was derived from, and both paths
//! run it through `App::apply_effects`. Effects remain the only place the
//! `App` is written.
//!
//! Both return the commands to send onward: the projections for the widgets
//! whose content is the document's rather than one client's. The grid is
//! per-viewport and comes from [`Sessions::project_all`](super::project::Sessions::project_all)
//! instead — a host with connected clients uses the [`Dirty`] the inner
//! functions return and projects per session; a host with none, like the
//! native TUI, drops what it gets.
//!
//! **A wire command arrives here through [`Checked`], and nowhere else**
//! (improvise-ondj, ADR 001). The basis check is a property of the session
//! door rather than of the mutation path — a host holding the `App` *is* the
//! truth and not a claimant — but "the mutation path does not check" was
//! implemented as a second door rather than as an exemption: both functions
//! used to take a `&Command` and their `Command::Model { cmd, .. }` arm
//! discarded the basis with `..`, which is the syntax for ignoring a field
//! and costs no compile error. So the answer is not a guard but a type: the
//! only way to obtain a `Checked` over a `Command::Model` is
//! [`Checked::against`], which runs [`stale_basis`] against the
//! authoritative `App`. A host that wants to apply a client's batch must
//! face the [`Rejection`], and `Sessions::apply_checked` is the door a host
//! goes through — `Host::commands` answers the whole frame first with
//! [`CheckedBatch::against`], so `Sessions::apply` (which answers one command
//! and then dispatches) now has no production caller and is kept for the
//! single-command case its tests drive.
//! The one place a command is applied to the authoritative state, by
//! keypress or by wire (improvise-gxi). See `context/code/command.md`.
use improvise_core::model::record::RecordHash;
use improvise_protocol::{Basis, Command, ModelCommand, Rejection, reduce_view};
@@ -59,202 +9,20 @@ use crate::ui::app::App;
use crate::ui::effect::{self, Effect};
use crate::ui::project::{Dirty, document_projections};
/// A wire [`Command`] the authoritative state has answered for.
///
/// The one thing it makes unrepresentable: a [`Command::Model`] applied
/// without its basis being checked (improvise-ondj). The inner field is
/// private and [`Checked::against`] is the only constructor, so there is no
/// spelling of "apply this batch and ignore what it claims" — the `..` that
/// used to discard the basis is now a `Rejection` the caller must handle.
///
/// It is **not** a claim about which `App` was asked: a host with two
/// documents could check against one and apply to the other. That is not a
/// hole this closes, because a `Sessions` is already bound to one document's
/// generation sequence (ADR 001) and a `Registry` hands out one pair at a
/// time.
///
/// # Why the lifetime does not close it (improvise-7v62)
///
/// The obvious answer — make the lifetime the `&App`'s, so the borrow
/// checker forbids checking one document and applying to another — was
/// tried, and it is not merely inconvenient: it is **contradictory with what
/// the type is for**. A `Checked` is a proof discharged *before* a
/// mutation, and its whole job is to be carried across that mutation; a
/// borrow of the `App` is a claim that must stay live *during* it. The two
/// cannot both hold. Tying them makes every call site an `E0502`, starting
/// with the canonical one:
///
/// ```text
/// error[E0502]: cannot borrow `*app` as mutable because it is also borrowed as immutable
/// match Checked::against(app, cmd) {
/// --- immutable borrow occurs here
/// Ok(checked) => self.apply_checked(app, id, checked),
/// ^^^^^ mutable borrow occurs here
/// ```
///
/// A generative brand would express it without borrowing, but that needs a
/// closure-scoped API and would distort every host. The remaining option is
/// carrying a document identity, and `DocumentId` lives in the *server's*
/// registry: an `App` does not have one and does not want one, precisely
/// because the unit of authority is one document per `App`. So this is a
/// **deliberate won't-fix**, not an oversight. What keeps it unreachable is
/// stated above and is load-bearing — improvise-j31x made this a
/// multi-document server on purpose, and ADR 001 never evicts a document so
/// that a reconnecting client cannot have a stale basis accepted as current.
/// A wire [`Command`] whose basis the authoritative state has answered for
/// (improvise-ondj/7v62, ADR 001). See `context/code/command.md`.
pub struct Checked<'a>(&'a Command);
/// A whole [`ClientFrame::Commands`](improvise_protocol::ClientFrame) the
/// authoritative state has answered for — **every** command in it, before
/// any of them is applied (improvise-955n/improvise-08wb, ADR 001).
///
/// [`Checked`] alone was not enough, and the gap was exactly one level up
/// from the one improvise-ondj closed. A `Checked` answers for *one*
/// command against the `App` **as it stands**; a frame is a sequence of
/// them, applied in order, so a frame whose second command was refused had
/// already applied its first. ADR 001 says a stale batch is never applied,
/// *not even partially*, and that was true of a single command and false of
/// a batch.
///
/// # One basis, checked once, and then the chain runs
///
/// The contract, as the user stated it: *"a sequence of commands sent to the
/// server should assume that it's played in order from start to end and that
/// each command sees the state after the previous command runs"*, sent as
/// *"a single transaction from a given base"*. Four clauses:
///
/// 1. A frame states **one basis**, describing the base the client composed
/// against.
/// 2. That basis is checked **once**, against that base, before anything is
/// applied.
/// 3. Commands are applied **in order**, and command N sees the state command
/// N-1 left. Data dependence between them is expected, not an error.
/// 4. **All or nothing.** If any command fails, the frame leaves the document
/// exactly as the base described.
///
/// Clauses 1–3 are here. **Clause 4 is not yet implemented** — see the hole
/// below and the design note on PR #26.
///
/// # Two of the three tiers hoist, and the third does not
///
/// Clause 2 hoists the check, but **not every tier is a frame-level fact**,
/// and a first version of this hoisted all three. That was a lost update:
/// `document_generation` and `layout_version` describe the *document*, so
/// answering them once for the frame is exactly right — but the record tier
/// asks "is the client's picture of **this record** current", and which
/// records are at stake is a property of the *command*. That is why
/// [`records_named`] is an exhaustive match over [`ModelCommand`] and not
/// something a frame could summarise (gotcha 43).
///
/// Skipping it for later commands let `[SetCell(North), SetCell(South)]`
/// overwrite a South that somebody else had moved, silently, with the frame
/// reported accepted — the precise loss ADR 001's record tier exists to
/// prevent, and invisible to the other two tiers because a `SetCell` is
/// `CellData` and moves neither of them. So **every** mutation answers for
/// its own records, against the **frame's** basis: the one the first
/// basis-carrying command stated, not each command's own field, so a later
/// command cannot state a fresher claim than the frame was composed under.
///
/// ## Except the records the frame is creating
///
/// The argument that motivated clause 3 is true, and it is true of a
/// narrower thing than it was first applied to. A client cannot claim a
/// counter for a record its own chain is about to **create**, because
/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) filters to
/// records the cache has been told about — so `[AddItem(East),
/// SetCell(East)]` would be refused by a naive per-command check however it
/// is ordered. But that argument was then stretched to records the frame
/// merely **writes**, and those are the ones the tier is for.
///
/// So the exemption is stated over the *property* rather than over a list of
/// commands: a record that **neither resolves in the document nor is claimed
/// by the basis** is one the frame is about to create, and is not answered
/// for. Every other combination still is — including a record that does not
/// resolve but *is* claimed, which is a client believing in something that
/// has been removed, and is `RecordGone` as before. See
/// [`will_be_created_by_the_frame`].
///
/// What the base means is therefore: **what the client saw**. The transaction
/// promises the whole chain lands on top of it or none of it does. The
/// chain's internal coherence is the transaction's job, not the basis
/// check's.
///
/// # The hole clause 4 leaves, stated rather than hidden
///
/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a
/// rollback), so a frame whose second mutation fails leaves the first
/// applied. `[RemoveItem("North"), SetCell(the North record)]` is the
/// example: the removal lands, the write then resolves to nothing and
/// `model_effects` returns an **empty** batch, so the frame is reported
/// accepted having applied half of itself. Both halves of that are open —
/// the partial application and the silence.
///
/// The create exemption has a residue of the same kind and it is the same
/// hole: a later command naming a record that is neither present nor claimed
/// is *presumed* to be one the frame creates, so if the frame does not in
/// fact create it, the write resolves to nothing and silently does not
/// happen. That is `model_effects`' empty batch again, not a second defect,
/// and it closes when clause 4 makes a mid-chain failure reportable.
///
/// Both are **unreachable from any client in this workspace**: the only
/// producer of a multi-command frame is
/// `improvise-wasm-client::keys::commit`, which emits one mutation among view
/// commands. **Reachability is a reason not to rush a fix and not a reason
/// not to notice one** — the lost update above had the same reachability
/// argument as this hole and none of its documentation, which is how it
/// shipped. That is what makes it safe to
/// document rather than rush, and it is why an earlier "at most one document
/// mutation per frame" rule — which closed the hole by forbidding the
/// capability — was rejected.
///
/// # Two things the rule that was rejected got wrong, worth keeping
///
/// **Arity is not a property of a variant.** A future `Command::RunScript` is
/// one variant performing an unbounded number of mutations, so no count over
/// the vocabulary can be right: a classifier can honestly answer *whether* a
/// command changes the document, never *how many times*. A
/// `mutates_the_document` predicate written for the rejected rule is deleted
/// with it rather than kept unused — but it was made an **exhaustive match**
/// on review, because a `matches!` answers `false` for a future variant in
/// silence, and clause 4 will need that classifier back. Write it exhaustive
/// when it returns.
///
/// **The transaction boundary is not the frame.** Because one command may
/// already be a chain, clause 4 is a property the system needs *inside* a
/// command as much as across a frame of them, which is why the design note
/// puts the mechanism at `App::apply_effects` rather than here.
/// A whole frame, answered before any of it is applied (improvise-955n/08wb,
/// ADR 001). Its four clauses are in `context/code/command.md`.
pub struct CheckedBatch<'a>(Vec<Checked<'a>>);
impl<'a> CheckedBatch<'a> {
/// Answer for every command in a frame, against the state the frame was
/// composed against — which is the state *before* any of them runs.
///
/// The refusal is the frame's answer and nothing is applied. It is a
/// `Result` rather than a per-command verdict for the same reason
/// [`Checked`] is a type rather than a guard: the only enforceable
/// version of "answer before applying" is not being able to reach the
/// commands without having done so.
///
/// **The two document tiers are answered once**, on the first command
/// that carries a basis — clause 2 — and every later command is admitted
/// without re-asking them, which is clause 3. **The record tier is asked
/// of every mutation**, against that same frame basis, because it is not
/// a frame-level fact. A frame with no mutation answers trivially.
///
/// The `Basis` on a *later* `Command::Model` is ignored rather than
/// compared, and that is an honest interim rather than the end state:
/// clause 1 says the **frame** states one basis, but the wire still
/// carries a `Basis` inside each `Command::Model` — right when one
/// command was one transaction, and now one field per command describing
/// something that belongs to the frame. Moving it to
/// `ClientFrame::Commands` would make "one basis per frame"
/// unrepresentable-otherwise instead of a convention, and that protocol
/// change belongs with clause 4.
/// The document tiers are answered once, on the first command that
/// carries a basis; the record tier is asked of every mutation.
pub fn against(app: &App, cmds: &'a [Command]) -> Result<Self, Rejection> {
// The frame's basis: the one its first mutation stated. Later
// commands carry a `Basis` field too, and it is deliberately *not*
// read — clause 1 says the frame states one base, and reading each
// command's own would let a chain claim a fresher picture than it was
// composed under.
// The frame's basis is the one its first mutation stated; a later
// command's own `Basis` field is deliberately not read (clause 1).
let mut frame_basis: Option<&Basis> = None;
let mut checked = Vec::with_capacity(cmds.len());
for cmd in cmds {
@@ -283,21 +51,8 @@ impl<'a> IntoIterator for CheckedBatch<'a> {
}
impl<'a> Checked<'a> {
/// Answer for a command, against the state it would be applied to.
///
/// Total over the vocabulary, and only [`Command::Model`] can be
/// refused: everything else is either this client's own view slice or a
/// `SwitchView`, which carries no basis and cannot corrupt — the
/// workbook simply refuses a view it does not have (ADR 001 names that
/// second hole and leaves it open deliberately).
///
/// Saving is not in this vocabulary and deliberately so: it is
/// `ClientFrame::Save`, answered by the host, because it mutates no
/// shared state and has no basis to answer for (improvise-368h).
///
/// There is no argument that suppresses the answer. ADR 001's force is a
/// fresh batch under a fresh basis, composed by the client; it is not a
/// way past this constructor.
/// Answer for a command, against the state it would be applied to. Total
/// over the vocabulary; only [`Command::Model`] can be refused.
pub fn against(app: &App, cmd: &'a Command) -> Result<Self, Rejection> {
let refused = match cmd {
Command::Model { basis, cmd: model } => stale_basis(app, basis, model),
@@ -309,19 +64,8 @@ impl<'a> Checked<'a> {
}
}
/// A command in a frame whose **document** tiers have already been
/// answered (clause 2), answering only for its own records.
///
/// The two document tiers are not re-asked, which is clause 3: they
/// describe a document this command is not running against, since its
/// predecessors have moved it. The record tier *is* asked, because it is
/// a fact about the command rather than the frame — see
/// [`CheckedBatch`], where the lost update that taught this is recorded.
///
/// Private, and that is what keeps it from being a bypass: from outside
/// this module the only doors are still [`Checked::against`] and
/// [`CheckedBatch::against`], and the only caller is the loop that has
/// just answered the frame's basis.
/// A command in a frame whose document tiers are already answered
/// (clause 2), answering only for its own records. Private, not a bypass.
fn within_a_frame(app: &App, basis: &Basis, cmd: &'a Command) -> Result<Self, Rejection> {
let refused = match cmd {
Command::Model { cmd: model, .. } => stale_records_in_a_frame(app, basis, model),
@@ -333,52 +77,24 @@ impl<'a> Checked<'a> {
}
}
/// The command this proof is about.
///
/// It hands back a `&'a Command` rather than one borrowed from `&self`,
/// so a caller can dispatch on the variant and still move the proof into
/// the function it dispatched to. Crate-private: a proof that could be
/// unwrapped from outside would be a proof nobody had to obtain.
/// The command this proof is about, borrowed from `'a` rather than from
/// `&self` so a caller can dispatch and still move the proof onward.
pub(crate) fn command(&self) -> &'a Command {
self.0
}
}
/// Apply one checked wire command to the authoritative state, and say what
/// it dirtied.
///
/// `Command::SwitchView` does not go through `reduce_view` alone: the
/// document's "which view is active" and the client's must move together,
/// and `App::switch_to_view` is one of the only two writers allowed to touch
/// either (gotcha 29). It reaches that writer the way every other mutation
/// here does — through its effect — so the view history a switch records is
/// the same on both doors. Everything else is pure view slice.
///
/// An inbound `Command::Project` is ignored. Projections travel
/// server→client; a client that sends one is describing state it does not
/// own.
/// Apply one checked wire command to the authoritative state, and say what it
/// dirtied. An inbound `Command::Project` is ignored: projections travel one way.
pub fn apply_command(app: &mut App, cmd: Checked<'_>) -> Dirty {
match cmd.0 {
Command::Project(_) => Dirty::NOTHING,
Command::SwitchView(name) => {
// The mutation is `effect::SwitchView`'s, not a second copy of
// it: the effect records the view history the key path leaves
// behind, and `ModelCommand::ViewBack` pops exactly that stack.
// Switching by calling `App::switch_to_view` here instead left
// the stack empty, so a wire client could not go back where a
// keyboard one could — see
// `a_wire_view_switch_records_history_the_way_a_key_switch_does`.
// `apply_effects` rebuilds the layout for us.
// The mutation is `effect::SwitchView`'s, not a second copy: the
// effect records the history `ModelCommand::ViewBack` pops.
app.apply_effects(vec![Box::new(effect::SwitchView(name.clone()))]);
// The `Dirty` is deliberately narrower than the
// `Dirty::of(Model(LayoutGeometry))` that call returns. That
// table is a per-*kind* over-approximation — sound for the kind
// because `CreateView` and `DeleteView` wear it too — and a view
// switch is the one member that leaves the view list alone: its
// names and axes belong to the document, and which view is
// active is not in `ViewListCache` at all (improvise-pca). Over-
// projecting costs bandwidth, never staleness, so narrowing here
// is safe; widening `Dirty::of` for this one member would not be.
// Deliberately narrower than `Dirty::of(Model(LayoutGeometry))`:
// a switch leaves the view list alone (improvise-pca).
Dirty {
grid: true,
category_tree: true,
@@ -386,8 +102,8 @@ pub fn apply_command(app: &mut App, cmd: Checked<'_>) -> Dirty {
..Dirty::NOTHING
}
}
// The basis was answered for by `Checked::against`; there is no
// other way to have got here with this variant.
// The basis was answered for by `Checked::against`; there is no other
// way to have got here with this variant.
Command::Model { cmd, basis: _ } => {
let effects = model_effects(app, cmd);
app.apply_effects(effects)
@@ -399,29 +115,8 @@ pub fn apply_command(app: &mut App, cmd: Checked<'_>) -> Dirty {
}
}
/// The other half of the derivation: a [`ModelCommand`] back to the effect
/// it was derived from (improvise-kmgs).
///
/// This is the seam that keeps a command from becoming a second way to
/// mutate. A `ModelCommand` names a mutation; it does not perform one. The
/// effect it names is the same value the keymap path builds, and it is run
/// through the same [`App::apply_effects`], so both doors go through the
/// one mutation site and both get their [`Dirty`] from the effect's own
/// `EffectKind` rather than from a second classification that could drift.
///
/// The match is exhaustive on purpose: a new `ModelCommand` variant fails
/// the build here until it names an effect, exactly as a new effect fails
/// the build until it classifies itself (gotcha 3).
///
/// It takes the `App` because two variants name their operand by
/// **surrogate** rather than by coordinates (ADR 002), and resolving one is
/// a lookup against the authoritative state. Resolution failing is not an
/// error path bolted on: it is one of the two arms of staleness — the record
/// is gone, a category or item it named was removed — and the answer is an
/// empty batch, which applies nothing. `Sessions::apply` now reaches that
/// same conclusion first and typed, as `Rejection::RecordGone` — this stays
/// as the floor under the key path and under any host that bypasses the
/// session door.
/// The other half of the derivation: a [`ModelCommand`] back to the effect it
/// was derived from (improvise-kmgs). Exhaustive on purpose.
pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec<Box<dyn Effect>> {
let one: Box<dyn Effect> = match cmd {
ModelCommand::SetCell { cell, value } => {
@@ -515,26 +210,8 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec<Box<dyn Effect>> {
vec![one]
}
/// Which records a [`ModelCommand`] writes, decided by the **server**
/// (ADR 001, improvise-fkfg).
///
/// This is deliberately not "whichever records the client's basis mentions".
/// A client that simply left a record out of its claim would otherwise have
/// a **force by omission** — the record tier skipped, with no flag to
/// remove and nothing in the message saying so — and force is supposed to be
/// a re-composition against the current basis, never a bypass of the check.
/// The server decides what is checked; the client only supplies what it saw.
///
/// The match is exhaustive, and the long list of "names no record" is the
/// point rather than noise: a new variant that writes a record fails the
/// build here until it says so. That matters because the failure mode is
/// silent — an unlisted record-writing variant would simply never be checked
/// against its own tier, and the suite would stay green.
///
/// Only `CellData` variants that carry a [`RecordCell`] name a record. The
/// item and formula commands are `CellData` too but name no *record*: they
/// move the axes a record's coordinates are drawn from, which is what the
/// layout tier is for.
/// Which records a [`ModelCommand`] writes, decided by the **server**, never
/// read off the client's claim (ADR 001, improvise-fkfg). Exhaustive on purpose.
pub fn records_named(cmd: &ModelCommand) -> Vec<RecordHash> {
match cmd {
ModelCommand::SetCell { cell, .. } | ModelCommand::ClearCell(cell) => vec![cell.record],
@@ -566,24 +243,8 @@ pub fn records_named(cmd: &ModelCommand) -> Vec<RecordHash> {
}
}
/// Is this batch's basis still current? `None` means apply it.
///
/// The three tiers in order of how much they invalidate, so the answer a
/// client is given names the largest thing that moved rather than an
/// arbitrary one: a document swap is not usefully reported as "a record
/// moved".
///
/// The record tier separates **structural** from **data** staleness, and
/// keeping them apart is the whole reason ADR 002 put a maintained
/// `RecordHash` index in `DataStore` rather than regenerating one from the
/// layout. A record that does not resolve is *gone* — there is nothing to
/// write to and nothing coherent to force, because force never creates
/// structure. A record that resolves at a different version merely *changed*,
/// and force there is one keystroke.
///
/// Nothing here mutates, and there is no argument that makes it return
/// `None` regardless: a stale batch is never applied, and ADR 001's force is
/// a fresh batch under a fresh basis rather than a way past this function.
/// Is this batch's basis still current? `None` means apply it. The three tiers
/// are checked largest-first, so the answer names the largest thing that moved.
pub fn stale_basis(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejection> {
let document_moved = (basis.document_generation != app.document_generation())
.then_some(Rejection::DocumentReplaced);
@@ -597,11 +258,7 @@ pub fn stale_basis(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejec
}
/// The record tier for a command inside a frame whose document tiers are
/// already answered.
///
/// It reuses [`stale_record`] unchanged — one home for what each of the four
/// combinations means — and differs only in *which* records it asks about:
/// the ones the frame is about to create are skipped.
/// already answered, skipping the records the frame is about to create.
fn stale_records_in_a_frame(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejection> {
records_named(cmd)
.into_iter()
@@ -609,20 +266,8 @@ fn stale_records_in_a_frame(app: &App, basis: &Basis, cmd: &ModelCommand) -> Opt
.find_map(|hash| stale_record(app, basis, hash))
}
/// Is this a record the frame is about to create?
///
/// **A property, not a list of commands.** The document does not have it and
/// the basis claims nothing about it — which is precisely what "I am creating
/// this" looks like from here, because `BasisCache::claim` can only state a
/// counter for a record the client has been told about.
///
/// The near miss is the other half of that conjunction: a record that does
/// not resolve but *is* claimed is a client believing in something that has
/// been removed, and stays [`Rejection::RecordGone`]. Only the pair exempts.
///
/// It is asked **only inside a frame**, never of a lone command: a first
/// command has no predecessor that could have created anything, so
/// [`Checked::against`] still refuses a write to a record that is not there.
/// Is this a record the frame is about to create? **A property, not a list of
/// commands** — see `context/code/command.md`. Only the pair exempts.
fn will_be_created_by_the_frame(app: &App, basis: &Basis, hash: RecordHash) -> bool {
app.resolve_record(hash).is_none() && basis.record(hash).is_none()
}
@@ -637,20 +282,8 @@ fn stale_record(app: &App, basis: &Basis, hash: RecordHash) -> Option<Rejection>
}
}
/// The command a client that is **fully caught up** would compose.
///
/// It reads the tiers straight off the `App`, which looks like forging and
/// is not: it takes `&App`, so only the side that *is* the authoritative
/// state can call it, and for that side "the current basis" is not a claim
/// to be checked but a fact. A remote client has no `App` and no other
/// constructor — `BasisCache::claim` withholds until the server has spoken —
/// so nothing reachable from a wire can get here.
///
/// It exists for two callers: a host that resolves a command locally and
/// wants to push it through the same door a client's would take, and the
/// tests whose subject is not staleness. The tests whose subject *is*
/// staleness build the [`Basis`] by hand, so neither kind can pass by
/// accident through the other's convenience.
/// The command a client that is **fully caught up** would compose. It takes
/// `&App`, so nothing reachable from a wire can call it: not a bypass.
pub fn caught_up(app: &App, cmd: ModelCommand) -> Command {
Command::Model {
basis: Basis {
@@ -665,12 +298,8 @@ pub fn caught_up(app: &App, cmd: ModelCommand) -> Command {
}
}
/// Apply one checked wire command and return what every client must be told.
///
/// The signature the three hosts share. The returned commands are the
/// document-scoped projections; per-client grid updates need a session's
/// viewport and come from `Sessions::project_all`, driven by the [`Dirty`]
/// that [`apply_command`] returns.
/// Apply one checked wire command and return the document-scoped projections.
/// Per-client grid updates need a viewport and come from `Sessions::project_all`.
pub fn reduce_full(app: &mut App, cmd: Checked<'_>) -> Vec<Command> {
let dirty = apply_command(app, cmd);
document_projections(app, dirty)
@@ -706,11 +335,8 @@ mod tests {
App::new(wb, None)
}
/// For the tests whose subject is not staleness: a command the current
/// state accepts, which every command built by [`caught_up`] is by
/// construction. The tests whose subject *is* staleness call
/// [`Checked::against`] themselves and assert on the [`Rejection`], so
/// neither kind can pass by accident through the other's convenience.
/// A command the current state accepts. The staleness tests build their
/// own [`Rejection`] instead, so neither kind borrows the other's luck.
fn checked<'a>(app: &App, cmd: &'a Command) -> Checked<'a> {
Checked::against(app, cmd).expect("composed against the current state")
}
@@ -745,9 +371,7 @@ mod tests {
}
/// The two copies of "which view is active" must move together
/// (gotcha 29). Folding `SwitchView` through `reduce_view` alone would
/// move the client's and leave the document's behind, and the cursor map
/// would then be read under the wrong key.
/// (gotcha 29), or the cursor map is read under the wrong key.
#[test]
fn switch_view_moves_the_document_and_the_client_together() {
let mut app = app();
@@ -756,23 +380,8 @@ mod tests {
assert_eq!(app.model_state.workbook.active_view, "Second");
}
/// A wire-driven view switch owes the same history a key-driven one
/// leaves behind.
///
/// `ModelCommand::ViewBack` pops `view_state.view_back_stack`, and the
/// only thing that ever pushes onto it is `effect::SwitchView`. While
/// `apply_command` performed the switch itself — calling
/// `App::switch_to_view` directly rather than running the effect — the
/// stack stayed empty, so a client that switched over the wire could not
/// go back, though the same two keypresses could. Wave 4 is what made
/// that reachable: before `ModelCommand` there was no wire way to ask.
///
/// Nothing else in the suite could see it.
/// `switch_view_moves_the_document_and_the_client_together` checks where
/// a switch *landed*; this checks what it *recorded*.
///
/// Bug demonstrated 2026-09-19 (Copilot, PR #11): the `ViewBack` left the
/// document on "Second".
/// A wire-driven view switch owes the same `view_back_stack` history a
/// key-driven one leaves behind (Copilot, PR #11).
#[test]
fn a_wire_view_switch_records_history_the_way_a_key_switch_does() {
let mut app = app();
@@ -908,12 +517,8 @@ mod tests {
}
}
/// `app()` with a cell actually stored, so a surrogate for it resolves
/// through the index rather than only through the layout. The walks over
/// `every_model_command` run against this: a `SetCell` whose record
/// cannot be resolved produces *no effects*, and every one of those
/// tests asserts on the effects, so the fixture has to name something
/// real or they would all fail for a reason that is not theirs.
/// `app()` with a cell actually stored, so a `RecordCell` surrogate for it
/// resolves — otherwise every walk below fails for a reason not its own.
fn model_app() -> App {
let mut app = app();
app.model_state
@@ -924,12 +529,8 @@ mod tests {
app
}
/// One value per `ModelCommand` variant.
///
/// The enum is the derived set, so a test that walks "the set" has to
/// walk all of it. [`variant_name`] is the completeness guard: it matches
/// exhaustively, so a variant added to the protocol fails the build here
/// until it is listed below as well.
/// One value per `ModelCommand` variant. [`variant_name`] matches
/// exhaustively, so a new variant fails the build until it is listed here.
fn every_model_command() -> Vec<ModelCommand> {
vec![
ModelCommand::SetCell {
@@ -1031,15 +632,8 @@ mod tests {
}
}
/// The derivation, stated as a check: every `ModelCommand` names an
/// effect that declares `EffectKind::Model(..)`.
///
/// This is what "derived, not invented" means operationally
/// (`doctrine(derive_the_enum_from_the_effects)`). A variant pointed at a
/// `ViewOnly` effect would be a wire message that changed the document
/// for the client that sent it and for nobody else — `Dirty::of` would
/// report `NOTHING`, and no projection would ever correct the others
/// (gotcha 35).
/// The derivation, stated as a check: every `ModelCommand` names an effect
/// that declares `EffectKind::Model(..)` — not a `ViewOnly` one (gotcha 35).
#[test]
fn every_model_command_names_an_effect_that_declares_a_model_change() {
for cmd in every_model_command() {
@@ -1060,26 +654,12 @@ mod tests {
}
}
/// The two `Model` effects whose operand is **session-local residue**
/// (gotcha 30): one client's uncommitted drill staging, and the workbook
/// one client's half-finished import wizard built.
/// The two `Model` effects whose operand is session-local residue
/// (gotcha 30): one client's drill staging, one client's wizard workbook.
const SESSION_LOCAL_EFFECTS: [&str; 2] = ["ApplyAndClearDrill", "AcceptWizardModel"];
/// improvise-3x3u: the wire vocabulary is the *addressable* model
/// effects, not all of them.
///
/// `Sessions::apply` routes a `Command::Model` with no `SessionId`,
/// which is right exactly when the change is addressed to no one session
/// (gotcha 36). These two are addressed to precisely one, and it is not
/// the sender: an unaddressed `ApplyAndClearDrill` commits another
/// client's private typing (improvise-tom1) and an unaddressed
/// `AcceptWizardModel` swaps the document out of a dialogue the sender
/// never opened. Both were demonstrated against `Sessions::apply` before
/// the variants were removed.
///
/// Re-adding either variant means making it *addressed* -- carrying a
/// `SessionId`, with the staging map moved onto the session -- not
/// restoring the nullary form this test forbids.
/// improvise-3x3u: the wire vocabulary is the *addressable* model effects.
/// Re-adding either variant means addressing it, not restoring the nullary form.
#[test]
fn no_model_command_commits_another_clients_session_local_state() {
for cmd in every_model_command() {
@@ -1097,8 +677,7 @@ mod tests {
}
/// The guard above is only worth having while the effects it names still
/// exist and are still model changes -- otherwise it would pass by
/// naming nothing (`doctrine(check_the_checker)`).
/// exist and are still model changes (`doctrine(check_the_checker)`).
#[test]
fn the_session_local_effects_are_real_model_effects() {
let effects: [Box<dyn Effect>; 2] = [
@@ -1118,14 +697,8 @@ mod tests {
}
}
/// The type name of an effect, read off the leading identifier of its
/// derived `Debug`.
///
/// `Effect` is a trait object, so `std::any::type_name` is not reachable
/// through it; every effect derives `Debug`, and a derived `Debug` opens
/// with the type's own name for a unit struct, a tuple struct and a
/// braced struct alike. So the name is already on the value — this only
/// reads it back.
/// An effect's type name, off the leading identifier of its derived `Debug`.
/// `Effect` is a trait object, so `std::any::type_name` cannot reach it.
fn effect_type_name(effect: &dyn Effect) -> String {
format!("{effect:?}")
.split(|c: char| !c.is_alphanumeric() && c != '_')
@@ -1134,10 +707,8 @@ mod tests {
.to_string()
}
/// The reader above is itself a check, so it gets the injection too
/// (`doctrine(check_the_checker)`): one effect of each `Debug` shape,
/// because the leading identifier is followed by a different delimiter
/// in each — nothing, `(`, and ` {`.
/// The reader above is itself a check, so it gets the injection too: one
/// effect of each `Debug` shape — nothing, `(`, and ` {` after the name.
#[test]
fn the_effect_name_reader_reads_all_three_debug_shapes() {
assert_eq!(effect_type_name(&effect::TransposeAxes), "TransposeAxes");
@@ -1154,29 +725,8 @@ mod tests {
);
}
/// The mapping is name-for-name by construction, so state that: every
/// `ModelCommand` variant names the effect that carries its own name.
///
/// This is the check that separates an effect from its inverse.
/// [`every_model_command_names_an_effect_that_declares_a_model_change`]
/// cannot: `HideItem` and `ShowItem`, `AddItem` and `RemoveItem`,
/// `CreateView` and `DeleteView`, `SetCell` and `ClearCell` are each two
/// effects of the *same* `EffectKind`, so a swapped pair clears that bar
/// while inverting what the wire message does. And
/// [`a_model_command_lands_where_its_keypress_lands`] cannot reach them
/// either — most variants have no single keypress to compare against, so
/// it drives three of twenty-eight.
///
/// Proven by injection 2026-09-19: `HideItem` → `effect::ShowItem`,
/// `CreateView` → `effect::DeleteView` and `AddItem` →
/// `effect::RemoveItem` all left `cargo test --workspace` at 918 passing,
/// 0 failing. This test fails on each.
///
/// The name-for-name rule is a design constraint, not a coincidence, and
/// it is what makes the derivation checkable at all
/// (`doctrine(derive_the_enum_from_the_effects)`): a variant whose effect
/// must be named something else is a variant that stopped being derived,
/// and it should fail here and be argued about rather than pass quietly.
/// Every `ModelCommand` variant names the effect of the same name. This is
/// the check that separates an effect from its inverse — see the evidence.
#[test]
fn every_model_command_names_the_effect_of_the_same_name() {
for cmd in every_model_command() {
@@ -1202,22 +752,12 @@ mod tests {
assert_eq!(names.len(), total, "a variant is listed twice: {names:?}");
}
/// The point of the whole ticket: a model `Command` applied
/// authoritatively leaves the document exactly where the keypress bound
/// to the same mutation leaves it. That equivalence is what lets the two
/// entry points share one door.
///
/// The `Dirty` is compared too, because a host with connected clients
/// projects off it: a command that produced the right document and the
/// wrong `Dirty` would leave every other client stale.
/// A model `Command` applied authoritatively lands where the keypress bound
/// to the same mutation lands, `Dirty` included.
#[test]
fn a_model_command_lands_where_its_keypress_lands() {
/// Two categories on different axes, because the one-category
/// fixture cannot tell a transpose from a cycle: with only `Region`
/// on Row, swapping the axes and rotating Row→Column land on the
/// same view, and an equivalence test that cannot distinguish the
/// wrong effect from the right one proves nothing
/// (`doctrine(check_the_checker)`).
/// Two categories on different axes: a one-category fixture cannot tell
/// a transpose from a cycle (`doctrine(check_the_checker)`).
fn pivoted() -> App {
let mut app = app();
app.model_state.workbook.add_category("Month").unwrap();
@@ -1279,23 +819,8 @@ mod tests {
);
}
/// A wire `Command::Model` cannot reach the document without its basis
/// being checked (ADR 001, improvise-ondj).
///
/// Bug demonstrated 2026-09-19: `reduce_full` and `apply_command` — the
/// functions this module's own doc points every future host at, the
/// browser worker included — took a `&Command` and matched
/// `Command::Model { cmd, .. }`, so a batch composed against a document
/// that had since moved landed anyway. The `..` discarded the basis with
/// no compile error.
///
/// **The regression guard is the signature, not this test.** Both now
/// take a [`Checked`], whose only constructor runs [`stale_basis`], so
/// the version of this test that reproduced the bug —
/// `reduce_full(&mut app, &wire)` with a stale `wire` — no longer
/// compiles. What is left to assert is that the constructor refuses, and
/// that refusing leaves the document alone: there is no half-applied
/// batch, because the type is obtained before anything is applied.
/// A wire `Command::Model` cannot reach the document unchecked (ADR 001,
/// improvise-ondj). The regression guard is the signature, not this test.
#[test]
fn a_stale_wire_model_command_cannot_be_applied() {
let mut app = app();
@@ -1333,33 +858,8 @@ mod tests {
);
}
/// improvise-gv9r. **The bug**: a client's edit left the authoritative
/// document reporting *clean*, on every transport.
///
/// `MarkDirty` used to be a separate effect that the *command* layer
/// composed alongside the mutation, and improvise-kmgs derived
/// `ModelCommand` from the effects **one at a time** — so the wire got
/// `SetCell` without its companion. A derivation correct per element and
/// wrong for a set. "Save if dirty" was therefore a trigger that never
/// fired, and a host built on it loses every client edit silently; the
/// browser worker found that out the hard way and had to over-approximate
/// off the command variant instead.
///
/// The fix is the unrepresentable one: dirtiness is raised by
/// `App::apply_effects` off the same `EffectKind` the `Dirty` and the two
/// basis tiers are read from (gotcha 40), so no path can compose a
/// document change without it. There is nothing left to forget, and
/// `MarkDirty` is gone rather than left as a second answer.
///
/// The three levels are asserted separately because they are three
/// different effects: an effect that is only ever reached by one of them
/// would otherwise pass on a sibling's evidence. The negative arm — that
/// a `ViewOnly` effect leaves the document clean — is
/// `a_view_only_batch_leaves_the_document_clean` in `ui/app.rs`, and it
/// has to live there: a view-slice `Command` never reaches
/// `apply_effects` at all (`apply_command` folds it straight into
/// `ViewState`), so a negative written against this door would pass
/// however unconditional the raise was.
/// improvise-gv9r: a client's edit used to leave the document reporting
/// *clean* on every transport. All three `ModelChange` levels, separately.
#[test]
fn a_wire_model_command_marks_the_document_dirty() {
for cmd in [
@@ -1387,16 +887,8 @@ mod tests {
}
}
/// `SwitchView` is the one command with a foot in both halves, and it is
/// the reason this matters beyond cell edits: `Workbook::active_view`
/// persists as the `Initial View` line, so a switch really does put the
/// document out of step with the file.
///
/// It is tested on its own because the browser worker's persist trigger
/// now depends on it (improvise-gv9r): the worker used to name
/// `SwitchView` explicitly in a table of variants and now asks the
/// document. If this effect were ever reclassified `ViewOnly`, the
/// worker would silently stop writing view switches back.
/// `SwitchView` persists as the `Initial View` line, so it dirties the
/// document too — and the worker's persist trigger depends on that.
#[test]
fn a_wire_view_switch_marks_the_document_dirty() {
let mut app = app();
crates/improvise-command/src/ui/viewport.rs
+6 -16
View File
@@ -13,15 +13,12 @@ use crate::view::{AxisEntry, GridLayout};
pub const MIN_COL_WIDTH: u16 = 5;
pub const MAX_COL_WIDTH: u16 = 32;
// The row-header bounds moved to `improvise-protocol` with the rule that
// clamps to them (improvise-1125): a browser sizing its own row header must
// reach the same numbers, and it cannot depend on this crate.
// The row-header bounds live in `improvise-protocol` with the rule that clamps to
// them (improvise-1125): a browser must reach the same numbers.
pub use improvise_protocol::{MAX_ROW_HEADER_W, MIN_ROW_HEADER_W};
/// Compute adaptive column widths for pivot mode (header labels + cell values).
/// Header widths use the widest *individual* level label (not the joined
/// multi-level string), matching how the grid renderer draws each level on
/// its own row with repeat-suppression.
/// Adaptive column widths for pivot mode. Header widths use the widest *individual*
/// level label, not the joined multi-level string.
pub fn compute_col_widths(
model: &Model,
layout: &GridLayout,
@@ -80,15 +77,8 @@ pub fn compute_col_widths(
.collect()
}
/// The total row-header width for a layout.
///
/// The rule itself is `improvise_protocol::header_level_widths`, which is
/// where both front ends reach it (improvise-1125). This is the one caller
/// that holds a `GridLayout` rather than a projected frame —
/// `App::visible_cols`, deciding how many columns fit — and it is on the
/// keypress path, so it hands the rule **borrowed** tuples rather than
/// building a frame and cloning every row. It does not reach for a `View`
/// either, because a collapsed flag cannot change a width.
/// The total row-header width for a layout. The rule is the protocol's; this is the
/// keypress-path caller that borrows tuples — see `context/code/command.md`.
pub fn compute_row_header_width(layout: &GridLayout) -> u16 {
let items = layout.row_items.iter().filter_map(|e| match e {
AxisEntry::DataItem(v) => Some(v),