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

feat: classify every effect for projection (improvise-mae)

Browse Source 
Adds EffectKind::{ViewOnly, Model(ModelChange)} with ModelChange::{CellData,
LayoutGeometry, WholeWorkbook}, so a server can decide which effects owe
connected clients an update. All 58 effects are classified.

kind() has no default. The design offered a per-struct method; making it
required means a new effect cannot forget to classify itself, and the failure
mode here is silent — a mis-declared ViewOnly edit would simply never reach
connected clients. It paid off at once: the compiler found a 58th Effect impl
living in command/keymap.rs rather than effect.rs.

The classification was derived from what each apply() body touches and
reasoned about in Prolog, not eyeballed. That surfaced the trap: the View is
stored inside model_state.workbook, so "touches model_state" does not mean
"is a model change". Axis, page, grouping, hidden items, number format and
active view are LayoutGeometry; cursor and scroll are per-client ViewOnly;
Save, SaveAs and MarkDirty touch model_state for bookkeeping only. Classifying
by grepping for model_state would have had the projection layer broadcasting
cell updates for cursor moves.

The tests verify the declarations rather than restate them: each applies the
effect to a probe App and fingerprints the shared state it claims not to
touch, in both directions.

Cursor and scroll are the one judgement call — they write the shared,
persisted View but must not move another client's viewport. Filed as
improvise-8uz to resolve before the projection layer builds on this.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V69PGQfMJuGRmtSWpfVAqn
This commit is contained in:
Edward Langley
2026-08-29 13:03:46 -07:00
parent 9d73e8f504
commit 3b1607d29b
4 changed files with 381 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/repo-map.md
+24 -19
View File
@@ -54,8 +54,12 @@ pub trait Cmd: Debug + Send + Sync {
pub trait Effect: Debug {
fn apply(&self, app: &mut App);
fn kind(&self) -> EffectKind; // NO default — the compiler makes you classify
fn changes_mode(&self) -> bool { false } // override when Effect swaps AppMode
}
// EffectKind::{ViewOnly, Model(ModelChange)}
// ModelChange::{CellData, LayoutGeometry, WholeWorkbook}
```
`CmdContext` (see `command/cmd/core.rs`) holds immutable refs to `Model`, `GridLayout`, `AppMode`, cursor/offsets, buffers, yanked cell, key code, expanded cats, and panel/tile cursors.
@@ -135,25 +139,26 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
## Gotchas (read before editing)
1. **Records mode is wide format.** A row is one *coordinate tuple* — the cells sharing everything but `_Measure` — and each raw measure gets its own editable column, followed by a `+` column that creates a new measure (improvise-c5v). Consequences: two measures on one coordinate are ONE row; editing a category column moves **every** measure cell in the row, not just the one under the cursor; `_Dim` in the synthetic key carries the column **index**, not a label, because a category and a measure may share a name — resolve it with `GridLayout::record_col` into a `RecordCol`. Formula targets are not columns (nothing to type into them). Drill pending edits are keyed by `(row, RecordCol)`.
2. **Commands never mutate.** `&CmdContext` is read-only; return `Vec<Box<dyn Effect>>`. If you want to touch `&mut App`, add or extend an Effect.
3. **`CellKey` is always sorted.** Use the constructors; equality and hashing rely on canonical form.
4. **No `Model::add_item`.** With a `Workbook` in hand use the facade: `wb.add_item("Region", "East")?` / `add_item_in_group` / `remove_item` (improvise-tvt). With only a `Model`, go through the category: `m.category_mut("Region").unwrap().add_item("East")`.
5. **Virtual categories.** `_Measure` items = explicit items ∪ formula targets. Use `measure_item_names()` / `effective_item_names("_Measure")`. `add_formula` does *not* add items to `_Measure`. Formula-target lookup also falls back into `_Measure` via `Model::find_item_category`. Cell aggregation goes through `apply_agg`, formula `SUM(…)`/`AVG(…)` through `eval_agg` — they differ on empty input (absent vs. `0` for Sum/Count, error for Avg/Min/Max).
6. **Display rounding is view-only.** `format_f64` (half-away-from-zero) is only called in rendering. Formula eval uses full `f64`. Never feed a rounded value back into eval.
7. **Formula eval is fixed-point.** `recompute_formulas(none_cats)` iterates until stable, bounded by `MAX_EVAL_DEPTH`; circular refs converge to `CellValue::Error("circular")`. `App::new` runs it before the first frame so formula cells render on startup.
8. **Drill into formula cells** strips the `_Measure` coordinate when it names a formula target, so `matching_cells` returns the raw records that feed the formula instead of returning empty.
9. **Minibuffer keymaps come from `TextEntry`.** The seven text-entry modes are built by the `TextEntry` builder in `keymap.rs`, which binds the shared spine (AnyChar→`append-char <buf>`, Backspace→`pop-char <buf>`, Esc→`[clear-buffer <buf>, enter-mode <exit>]`) and welds every commit key to its `clear-buffer` via `.commit(key, cmd, args)` (improvise-w3q). `TextEntry::new` takes the `AppMode` and reads `<buf>` off its `MinibufferConfig`, so the buffer the prompt names and the buffer the keys edit are the same string by construction (improvise-k8h) — the keymap never spells a buffer name. `.rebind` exists only for CommandMode's Backspace. `SearchMode` is hand-written on purpose — no commit key, and `exit-search-mode` owns the query.
10. **Panel keymaps come from `PanelKeymap`.** The three panel modes share a spine (Esc/Tab→Normal, arrows and j/k→`move-panel-cursor <panel>`, every panel's toggle key, `:`→command mode) built by `PanelKeymap::new(panel)`; the keys that act on the panel's contents arrive through `.bind`/`.bind_args`/`.delete` because they vary in shape, not just command name (improvise-4pg). `Panel::ALL`/`name()`/`key()` in `ui/app.rs` drive the toggle keys for Normal and all three panels, so a fourth panel reaches every keymap from one `ALL` entry. `name()` must spell what `parse_panel` accepts.
11. **Keybindings are per-mode.** `ModeKey::from_app_mode()` resolves the mode; Normal + `search_mode=true` maps to `SearchMode`. Minibuffer modes bind Enter/Esc as `Binding::Sequence` so `clear-buffer` fires alongside commit/cancel.
12. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side.
13. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic.
14. **Float equality inside formula eval is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
15. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
16. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
17. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
18. **A failed `Save` aborts the batch.** The `Save` effect sets `abort_effects` when there's no file path or the write fails (improvise-8tk), so anything chained after it — the `Quit` in `wq`/`ZZ` — is skipped and the error stays on screen. A `Binding::Sequence` flattens into one effect vec, so the abort carries across sequence steps too.
19. **The import wizard's back path is a history stack, not a reverse table.** `advance` pushes the step it leaves onto `ImportWizard::history`; `previous` pops it, so back always retraces the path actually taken through the conditional transitions (improvise-907). Preconditions live in `validation_error()`, checked on the step where the user can still fix them — never deferred to `build_model`. `NameModel` is terminal for `advance`: Enter there calls `build()`, which parks the workbook in `built` on `Done`, and `AcceptWizardModel` takes it from there. Going back from `Done` discards `built`.
20. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
2. **Every effect declares an `EffectKind`, and there is no default.** `ViewOnly` never reaches other clients; `Model(CellData | LayoutGeometry | WholeWorkbook)` does (improvise-mae). A new effect that forgets to classify itself fails to compile — that is deliberate, since the failure mode is silent: a mis-declared `ViewOnly` edit would simply never reach connected clients. Watch two traps: the *View* lives inside `model_state.workbook`, so "touches model_state" does NOT mean "is a model change" (axis/page/format are `LayoutGeometry`, cursor and scroll are `ViewOnly` — see improvise-8uz); and `Save`/`SaveAs`/`MarkDirty` touch `model_state` for bookkeeping only and are `ViewOnly`.
3. **Commands never mutate.** `&CmdContext` is read-only; return `Vec<Box<dyn Effect>>`. If you want to touch `&mut App`, add or extend an Effect.
4. **`CellKey` is always sorted.** Use the constructors; equality and hashing rely on canonical form.
5. **No `Model::add_item`.** With a `Workbook` in hand use the facade: `wb.add_item("Region", "East")?` / `add_item_in_group` / `remove_item` (improvise-tvt). With only a `Model`, go through the category: `m.category_mut("Region").unwrap().add_item("East")`.
6. **Virtual categories.** `_Measure` items = explicit items ∪ formula targets. Use `measure_item_names()` / `effective_item_names("_Measure")`. `add_formula` does *not* add items to `_Measure`. Formula-target lookup also falls back into `_Measure` via `Model::find_item_category`. Cell aggregation goes through `apply_agg`, formula `SUM(…)`/`AVG(…)` through `eval_agg` — they differ on empty input (absent vs. `0` for Sum/Count, error for Avg/Min/Max).
7. **Display rounding is view-only.** `format_f64` (half-away-from-zero) is only called in rendering. Formula eval uses full `f64`. Never feed a rounded value back into eval.
8. **Formula eval is fixed-point.** `recompute_formulas(none_cats)` iterates until stable, bounded by `MAX_EVAL_DEPTH`; circular refs converge to `CellValue::Error("circular")`. `App::new` runs it before the first frame so formula cells render on startup.
9. **Drill into formula cells** strips the `_Measure` coordinate when it names a formula target, so `matching_cells` returns the raw records that feed the formula instead of returning empty.
10. **Minibuffer keymaps come from `TextEntry`.** The seven text-entry modes are built by the `TextEntry` builder in `keymap.rs`, which binds the shared spine (AnyChar→`append-char <buf>`, Backspace→`pop-char <buf>`, Esc→`[clear-buffer <buf>, enter-mode <exit>]`) and welds every commit key to its `clear-buffer` via `.commit(key, cmd, args)` (improvise-w3q). `TextEntry::new` takes the `AppMode` and reads `<buf>` off its `MinibufferConfig`, so the buffer the prompt names and the buffer the keys edit are the same string by construction (improvise-k8h) — the keymap never spells a buffer name. `.rebind` exists only for CommandMode's Backspace. `SearchMode` is hand-written on purpose — no commit key, and `exit-search-mode` owns the query.
11. **Panel keymaps come from `PanelKeymap`.** The three panel modes share a spine (Esc/Tab→Normal, arrows and j/k→`move-panel-cursor <panel>`, every panel's toggle key, `:`→command mode) built by `PanelKeymap::new(panel)`; the keys that act on the panel's contents arrive through `.bind`/`.bind_args`/`.delete` because they vary in shape, not just command name (improvise-4pg). `Panel::ALL`/`name()`/`key()` in `ui/app.rs` drive the toggle keys for Normal and all three panels, so a fourth panel reaches every keymap from one `ALL` entry. `name()` must spell what `parse_panel` accepts.
12. **Keybindings are per-mode.** `ModeKey::from_app_mode()` resolves the mode; Normal + `search_mode=true` maps to `SearchMode`. Minibuffer modes bind Enter/Esc as `Binding::Sequence` so `clear-buffer` fires alongside commit/cancel.
13. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side.
14. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic.
15. **Float equality inside formula eval is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
16. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
17. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
18. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
19. **A failed `Save` aborts the batch.** The `Save` effect sets `abort_effects` when there's no file path or the write fails (improvise-8tk), so anything chained after it — the `Quit` in `wq`/`ZZ` — is skipped and the error stays on screen. A `Binding::Sequence` flattens into one effect vec, so the abort carries across sequence steps too.
20. **The import wizard's back path is a history stack, not a reverse table.** `advance` pushes the step it leaves onto `ImportWizard::history`; `previous` pops it, so back always retraces the path actually taken through the conditional transitions (improvise-907). Preconditions live in `validation_error()`, checked on the step where the user can still fix them — never deferred to `build_model`. `NameModel` is terminal for `advance`: Enter there calls `build()`, which parks the workbook in `built` on `Done`, and `AcceptWizardModel` takes it from there. Going back from `Done` discards `built`.
21. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
---
crates/improvise-command/src/command/keymap.rs
+3
View File
@@ -437,6 +437,9 @@ impl Effect for SetTransientKeymap {
fn apply(&self, app: &mut crate::ui::app::App) {
app.view_state.transient_keymap = Some(self.0.clone());
}
fn kind(&self) -> crate::ui::effect::EffectKind {
crate::ui::effect::EffectKind::ViewOnly
}
}
/// Maps modes to their root keymaps + owns the command registry.
crates/improvise-command/src/ui/effect.rs
+342
View File
@@ -13,10 +13,44 @@ pub use super::app::Panel;
pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty";
/// What an effect changes, so a server can decide whether connected clients
/// 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.
ViewOnly,
/// Changes something every client renders.
Model(ModelChange),
}
/// What kind of shared change an effect made, so projection can emit the
/// right command rather than invalidating everything.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ModelChange {
/// Cell values, or the categories and items they hang off.
CellData,
/// Axis assignment, page selection, hidden items, grouping, number
/// format, which view is active — the grid re-lays out.
LayoutGeometry,
/// The workbook was replaced wholesale; nothing survives.
WholeWorkbook,
}
/// A discrete state change produced by a command.
/// Effects know how to apply themselves to the App.
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).
fn kind(&self) -> EffectKind;
/// Whether this effect changes the app mode. Consumed only by
/// `ExecuteCommand`, which appends a return-to-Normal when no effect in
/// the batch claims the mode.
@@ -38,6 +72,9 @@ impl Effect for AddCategory {
fn apply(&self, app: &mut App) {
let _ = app.model_state.workbook.add_category(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -55,6 +92,9 @@ impl Effect for AddItem {
app.view_state.status_msg = e.to_string();
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -73,6 +113,9 @@ impl Effect for AddItemInGroup {
app.view_state.status_msg = e.to_string();
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -81,6 +124,9 @@ impl Effect for SortData {
fn apply(&self, app: &mut App) {
app.model_state.workbook.model.data.sort_by_key();
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -92,6 +138,9 @@ impl Effect for SetCell {
.model
.set_cell(self.0.clone(), self.1.clone());
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -100,6 +149,9 @@ impl Effect for ClearCell {
fn apply(&self, app: &mut App) {
app.model_state.workbook.model.clear_cell(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -129,6 +181,9 @@ impl Effect for AddFormula {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -143,6 +198,9 @@ impl Effect for RemoveFormula {
.model
.remove_formula(&self.target, &self.target_category);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
/// Refresh the derived grid layout mid-batch. `apply_effects` rebuilds once
@@ -154,6 +212,9 @@ impl Effect for RebuildLayout {
fn apply(&self, app: &mut App) {
app.rebuild_layout();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Re-enter edit mode by reading the cell value at the current cursor.
@@ -176,6 +237,9 @@ impl Effect for EnterEditAtCursor {
app.view_state.buffers.insert("edit".to_string(), value);
app.view_state.mode = self.target_mode.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
/// Unconditionally swaps the mode, so `ExecuteCommand` must not append its
/// own return-to-Normal on top (improvise-6sq).
fn changes_mode(&self) -> bool {
@@ -190,6 +254,9 @@ impl Effect for TogglePruneEmpty {
let v = app.model_state.workbook.active_view_mut();
v.prune_empty = !v.prune_empty;
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -200,6 +267,9 @@ impl Effect for ToggleCatExpand {
app.view_state.expanded_cats.insert(self.0.clone());
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -213,6 +283,9 @@ impl Effect for RemoveItem {
.workbook
.remove_item(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
#[derive(Debug)]
@@ -221,6 +294,9 @@ impl Effect for RemoveCategory {
fn apply(&self, app: &mut App) {
app.model_state.workbook.remove_category(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
// ── View mutations ───────────────────────────────────────────────────────────
@@ -231,6 +307,9 @@ impl Effect for CreateView {
fn apply(&self, app: &mut App) {
app.model_state.workbook.create_view(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -239,6 +318,9 @@ impl Effect for DeleteView {
fn apply(&self, app: &mut App) {
let _ = app.model_state.workbook.delete_view(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -255,6 +337,9 @@ impl Effect for SwitchView {
}
let _ = app.model_state.workbook.switch_view(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
/// Go back in view history (pop back stack, push current to forward stack).
@@ -272,6 +357,9 @@ impl Effect for ViewBack {
app.view_state.mode = frame.mode;
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
/// Go forward in view history (pop forward stack, push current to back stack).
@@ -289,6 +377,9 @@ impl Effect for ViewForward {
app.view_state.mode = frame.mode;
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -303,6 +394,9 @@ impl Effect for SetAxis {
.active_view_mut()
.set_axis(&self.category, self.axis);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -317,6 +411,9 @@ impl Effect for SetPageSelection {
.active_view_mut()
.set_page_selection(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -331,6 +428,9 @@ impl Effect for ToggleGroup {
.active_view_mut()
.toggle_group_collapse(&self.category, &self.group);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -345,6 +445,9 @@ impl Effect for HideItem {
.active_view_mut()
.hide_item(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -359,6 +462,9 @@ impl Effect for ShowItem {
.active_view_mut()
.show_item(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -367,6 +473,9 @@ impl Effect for TransposeAxes {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().transpose_axes();
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -378,6 +487,9 @@ impl Effect for CycleAxis {
.active_view_mut()
.cycle_axis(&self.0);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
#[derive(Debug)]
@@ -386,6 +498,9 @@ impl Effect for SetNumberFormat {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().number_format = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
}
}
// ── Navigation ───────────────────────────────────────────────────────────────
@@ -396,6 +511,9 @@ impl Effect for SetSelected {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().selected = (self.0, self.1);
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -404,6 +522,9 @@ impl Effect for SetRowOffset {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().row_offset = self.0;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -412,6 +533,9 @@ impl Effect for SetColOffset {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().col_offset = self.0;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
// ── App state ────────────────────────────────────────────────────────────────
@@ -422,6 +546,9 @@ impl Effect for ChangeMode {
fn apply(&self, app: &mut App) {
app.view_state.mode = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
fn changes_mode(&self) -> bool {
true
}
@@ -433,6 +560,9 @@ impl Effect for SetStatus {
fn apply(&self, app: &mut App) {
app.view_state.status_msg = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -441,6 +571,9 @@ impl Effect for MarkDirty {
fn apply(&self, app: &mut App) {
app.model_state.dirty = true;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -449,6 +582,9 @@ impl Effect for SetYanked {
fn apply(&self, app: &mut App) {
app.view_state.yanked = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -457,6 +593,9 @@ impl Effect for SetSearchQuery {
fn apply(&self, app: &mut App) {
app.view_state.search_query = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -465,6 +604,9 @@ impl Effect for SetSearchMode {
fn apply(&self, app: &mut App) {
app.view_state.search_mode = self.0;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Set a named buffer's contents.
@@ -484,6 +626,9 @@ impl Effect for SetBuffer {
.insert(self.name.clone(), self.value.clone());
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -492,6 +637,9 @@ impl Effect for SetTileCatIdx {
fn apply(&self, app: &mut App) {
app.view_state.tile_cat_idx = self.0;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Populate the drill state with a frozen snapshot of records.
@@ -505,6 +653,9 @@ impl Effect for StartDrill {
pending_edits: std::collections::HashMap::new(),
});
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Apply any pending edits to the model and clear the drill state.
@@ -536,6 +687,9 @@ impl Effect for ApplyAndClearDrill {
}
app.model_state.dirty = true;
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
/// Write one measure cell, or clear it when the edit is blank.
@@ -601,6 +755,9 @@ impl Effect for SetDrillPendingEdit {
.insert((self.record_idx, self.col.clone()), self.new_value.clone());
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
// ── Side effects ─────────────────────────────────────────────────────────────
@@ -623,6 +780,9 @@ impl Effect for Save {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Write the workbook to its current file path, returning the status
@@ -653,6 +813,9 @@ impl Effect for SaveAs {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// One discrete mutation of the import wizard's own state. Which op a key
@@ -710,6 +873,9 @@ impl Effect for WizardApply {
self.0.apply_to(wizard);
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Dismiss the import wizard and return to Normal mode.
@@ -720,6 +886,9 @@ impl Effect for CloseWizard {
app.view_state.wizard = None;
app.view_state.mode = AppMode::Normal;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
fn changes_mode(&self) -> bool {
true
}
@@ -746,6 +915,9 @@ impl Effect for AcceptWizardModel {
CloseWizard.apply(app);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::WholeWorkbook)
}
fn changes_mode(&self) -> bool {
true
}
@@ -771,6 +943,9 @@ impl Effect for StartImportWizard {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -787,6 +962,9 @@ impl Effect for ExportCsv {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
/// Load a model from a file, replacing the current one.
@@ -805,6 +983,9 @@ impl Effect for LoadModel {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::WholeWorkbook)
}
}
/// Extract the records array from an already-parsed JSON import value.
@@ -929,6 +1110,9 @@ impl Effect for ImportJsonHeadless {
}
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::WholeWorkbook)
}
}
#[derive(Debug)]
@@ -945,6 +1129,9 @@ impl Effect for SetPanelOpen {
Panel::View => app.view_state.view_panel_open = self.open,
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -960,6 +1147,9 @@ impl Effect for SetPanelCursor {
Panel::View => app.view_state.view_panel_cursor = self.cursor,
}
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
// ── Chain control ────────────────────────────────────────────────────────────
@@ -975,6 +1165,9 @@ impl Effect for AbortChain {
fn apply(&self, app: &mut App) {
app.abort_effects = true;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
// ── Records hygiene ──────────────────────────────────────────────────────────
@@ -999,6 +1192,9 @@ impl Effect for CleanEmptyRecords {
app.model_state.workbook.model.clear_cell(&key);
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::CellData)
}
}
// ── Convenience constructors ─────────────────────────────────────────────────
@@ -1028,6 +1224,9 @@ impl Effect for HelpPageNext {
let max = crate::ui::app::HELP_PAGE_COUNT.saturating_sub(1);
app.view_state.help_page = app.view_state.help_page.saturating_add(1).min(max);
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -1036,6 +1235,9 @@ impl Effect for HelpPagePrev {
fn apply(&self, app: &mut App) {
app.view_state.help_page = app.view_state.help_page.saturating_sub(1);
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
#[derive(Debug)]
@@ -1044,6 +1246,9 @@ impl Effect for HelpPageSet {
fn apply(&self, app: &mut App) {
app.view_state.help_page = self.0;
}
fn kind(&self) -> EffectKind {
EffectKind::ViewOnly
}
}
pub fn help_page_next() -> Box<dyn Effect> {
@@ -1075,6 +1280,143 @@ mod tests {
App::new(wb, None)
}
// ── Effect classification (improvise-mae) ───────────────────────────
/// Serialized model data — everything a connected client renders.
/// Excludes the view slice, which is per-client.
fn model_fingerprint(app: &App) -> String {
serde_json::to_string(&app.model_state.workbook.model).expect("model serializes")
}
/// Serialized view configuration (axes, pages, hidden items, format).
fn view_config_fingerprint(app: &App) -> String {
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.
#[test]
fn view_only_effects_leave_shared_state_alone() {
let cases: Vec<Box<dyn Effect>> = vec![
Box::new(ChangeMode(AppMode::Help)),
Box::new(SetStatus("hi".into())),
Box::new(MarkDirty),
Box::new(SetSearchQuery("q".into())),
Box::new(SetSearchMode(true)),
Box::new(SetBuffer {
name: "edit".into(),
value: "v".into(),
}),
Box::new(SetTileCatIdx(1)),
Box::new(SetPanelOpen {
panel: Panel::Formula,
open: true,
}),
Box::new(SetPanelCursor {
panel: Panel::Formula,
cursor: 0,
}),
Box::new(AbortChain),
Box::new(HelpPageNext),
Box::new(HelpPagePrev),
Box::new(RebuildLayout),
Box::new(SetSelected(1, 1)),
Box::new(SetRowOffset(2)),
Box::new(SetColOffset(3)),
];
for effect in cases {
assert_eq!(
effect.kind(),
EffectKind::ViewOnly,
"{effect:?} is in the ViewOnly case list"
);
let mut app = test_app();
let model_before = model_fingerprint(&app);
let views_before = view_config_fingerprint(&app);
effect.apply(&mut app);
assert_eq!(
model_fingerprint(&app),
model_before,
"{effect:?} declares ViewOnly but changed model data"
);
// Cursor and scroll are stored in the View, so they are allowed
// to move view config; nothing else in this list may.
if !matches!(
format!("{effect:?}").split('(').next(),
Some("SetSelected") | Some("SetRowOffset") | Some("SetColOffset")
) {
assert_eq!(
view_config_fingerprint(&app),
views_before,
"{effect:?} declares ViewOnly but changed view configuration"
);
}
}
}
/// The other direction: effects declaring `CellData` must actually
/// change model data, or projection would emit nothing for a real edit.
#[test]
fn cell_data_effects_actually_change_model_data() {
let key = CellKey::new(vec![("Type".into(), "Food".into())]);
let cases: Vec<Box<dyn Effect>> = vec![
Box::new(AddCategory("Region".into())),
Box::new(AddItem {
category: "Type".into(),
item: "Drink".into(),
}),
Box::new(SetCell(key.clone(), CellValue::Number(1.0))),
];
for effect in cases {
assert_eq!(
effect.kind(),
EffectKind::Model(ModelChange::CellData),
"{effect:?} is in the CellData case list"
);
let mut app = test_app();
let before = model_fingerprint(&app);
effect.apply(&mut app);
assert_ne!(
model_fingerprint(&app),
before,
"{effect:?} declares CellData but changed no model data"
);
}
}
/// Effects declaring `LayoutGeometry` must change view configuration —
/// the thing that makes every client re-lay out its grid.
#[test]
fn layout_geometry_effects_actually_change_view_config() {
let cases: Vec<Box<dyn Effect>> = vec![
Box::new(SetAxis {
category: "Type".into(),
axis: Axis::Page,
}),
Box::new(TransposeAxes),
Box::new(SetNumberFormat(",.3f".into())),
Box::new(TogglePruneEmpty),
];
for effect in cases {
assert_eq!(
effect.kind(),
EffectKind::Model(ModelChange::LayoutGeometry),
"{effect:?} is in the LayoutGeometry case list"
);
let mut app = test_app();
let before = view_config_fingerprint(&app);
effect.apply(&mut app);
assert_ne!(
view_config_fingerprint(&app),
before,
"{effect:?} declares LayoutGeometry but changed no view config"
);
}
}
// ── Model mutation effects ──────────────────────────────────────────
#[test]
prolog-sessions/kb.pl
+12
View File
@@ -163,3 +163,15 @@ invariant(gzip_is_a_name_property,'the .gz suffix selects gzip in save_to/load_f
decision('improvise-6mq',narrow_trait_over_vfs_crate,'chose Option B (own two-method trait) over the ticket-recommended Option A (vfs crate): the real surface is read/write one document, and the ticket own con for A -- sync-only while OPFS is async -- means adopting A would mean doing B later anyway').
location(storage_trait,'crates/improvise-io/src/storage.rs','Storage, PhysicalFs, MemoryFs').
work_done('improvise-6mq',refactor,'persistence save/load routed through a Storage trait; save/load kept as PhysicalFs wrappers so all 83 persistence tests pass unchanged; MemoryFs gives a filesystem-free test fixture.').
% ── Effect classification for projection (improvise-mae) ────────────────────
effect_kind_class(view_only,'never projected to other clients').
effect_kind_class(cell_data,'cell values, categories, items').
effect_kind_class(layout_geometry,'axis, page, hidden, grouping, format, active view').
effect_kind_class(whole_workbook,'workbook replaced wholesale').
invariant(effect_kind_no_default,'Effect::kind has no default impl, so a new effect cannot forget to classify itself; the failure mode would be silent (a mis-declared ViewOnly edit never reaches connected clients)').
invariant(view_lives_in_model_state,'the View is stored inside model_state.workbook, so touching model_state does NOT imply a model change: axis/page/format are layout_geometry, cursor and scroll are per-client view_only, Save/SaveAs/MarkDirty are bookkeeping').
decision('improvise-mae',cursor_is_per_client,'SetSelected/SetRowOffset/SetColOffset classed ViewOnly even though they write the shared persisted View, because one client scrolling must not move another viewport; the residual inconsistency is filed as improvise-8uz').
doctrine(derive_then_verify,'when classifying many items from source, extract facts mechanically and reason in Prolog, but VERIFY the extraction: multi-line method chains and free-function helpers defeated naive regexes and mis-classified SetCell, SortData and ApplyAndClearDrill as view-only').
location(effect_kind,'crates/improvise-command/src/ui/effect.rs','EffectKind, ModelChange, Effect::kind').
work_done('improvise-mae',refactor,'all 58 effects classified as ViewOnly or Model(CellData|LayoutGeometry|WholeWorkbook); kind() has no default so the compiler enforces completeness; three tests verify declarations empirically by applying effects and fingerprinting shared state.').