From d1ce2ecbc319edfeda4a4bac2932e8a3754d9f1a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 16:36:03 -0700 Subject: [PATCH 1/3] fix(web): an advertised key is never silent, a formula is visible, and +cat commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-rmvx, improvise-fdt6, improvise-gcvb — three faces of the same thing: a browser offering something and then doing nothing. rmvx. `the_command_names_that_still_need_the_host_are_these` was built with `in_mode(AppMode::Normal)`, so it only ever asked about Normal-mode bindings: `add-record-row` (RecordsNormal) and `delete-formula-at-cursor` (FormulaPanel) were outside its reach entirely and could never have failed it. A named list that silently scopes itself to one mode is a guard that looks broader than it is. `every_advertised_command_answers_in_its_own_mode` crosses `help::advertised` — command NAMES, so no hint string is tokenised — against `resolve` in every mode, over a fixture furnished enough that an `Upstream` can only mean an unhandled name. It found a seventh instance the ticket had not predicted: `view-back` is advertised in RecordsNormal. Six names now decline out loud, each with its own true subject, out of one `declined` table; `stranding` returns the subject rather than a bool, so `:` says the command line is unavailable instead of doing nothing. A declining step takes its whole `Binding::Sequence` with it, which is what keeps `o` ("add a record row, then edit it") from opening an editor on a row nobody created. fdt6. `commit-buffer formula` already resolved; what did not exist was any way to SEE the typing. `RenderModel::editing` is scoped to the two cell-editing modes and reads the "edit" buffer by name, so FormulaEdit, CategoryAdd, ItemAdd and the three prompts opened and swallowed every character. `RenderModel:: minibuffer` is `AppMode::minibuffer()` plus the buffer that config names, read through `improvise_protocol::read_buffer` so gotcha 11 keeps its one home, and `web/app.js` paints it. gcvb. The records-mode early return was broader than its reason. A `CellId::Create` commits the typed NAME as one existing `ModelCommand`, reads no `record_col` and writes no cell, so the return narrows rather than disappears. `+cat` must NOT advance: an inserted column renumbers every index to its right and the advance was computed from the pre-commit layout, so a client that advanced anyway would land one column off, silently. Both halves proved by injection against `CommitAndAdvance` in `tests/lockstep.rs`. Every new guard was proved by injecting the fault it names. The one worth repeating: `web/tests/render_surface.test.js` went green over a page that painted nothing, because the string `model.minibuffer` was inside a function nothing called. The static check is a completeness check, never an execution check — `harness/tests/keys.spec.js` caught it in Chromium on the first run. 1394 → 1397 Rust tests; harness 17 → 19 passed, 3 skipped; node 26. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/clients.md | 84 ++++- context/test-evidence/clients.md | 101 ++++++ crates/improvise-protocol/src/help.rs | 49 +-- crates/improvise-protocol/src/keymap.rs | 27 +- crates/improvise-protocol/src/lib.rs | 2 +- crates/improvise-protocol/src/resolve.rs | 305 ++++++++++++++---- crates/improvise-wasm-client/src/render.rs | 53 +++ .../improvise-wasm-client/tests/lockstep.rs | 66 ++++ harness/tests/keys.spec.js | 68 ++++ web/app.js | 12 + web/index.html | 7 + web/style.css | 18 ++ 12 files changed, 695 insertions(+), 97 deletions(-) create mode 100644 harness/tests/keys.spec.js diff --git a/context/code/clients.md b/context/code/clients.md index ecf86993..df92f0b2 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -456,7 +456,36 @@ immutable `CmdContext`; the commands are then folded in order. `the_command_names_that_still_need_the_host_are_these` names them, and `Host::handle` answers a `Key` frame with an empty vec, so each one is a binding that **does nothing at all** in a browser -(`doctrine(an_upstream_key_is_a_black_hole)`). The classes: +(`doctrine(an_upstream_key_is_a_black_hole)`). + +**An upstream key that the app also ADVERTISES is a different severity, and it +had its own class of defect** (improvise-rmvx). A key nobody offers doing +nothing is a gap; a key named in the mode's own hint line doing nothing reads as +*broken*. The two are crossed by `every_advertised_command_answers_in_its_own +_mode`, which asks `help::advertised` — a table of command **names**, so the +cross-reference is exact and nothing tokenises the rendered string — against +`resolve` in every mode, over a fixture furnished enough that an `Upstream` can +only mean an unhandled name. The old guard asked only about `Normal`, which is +why `add-record-row` and `delete-formula-at-cursor` were outside its reach +entirely; `view-back` in RecordsNormal was a seventh instance nobody had +noticed. + +`declined` is the answer, and it is one list rather than an arm apiece: six +command names, each with **its own true subject**. Wording matters here more +than it looks — the `>` fix was nearly routed through a message about +aggregates, which would have been true-sounding about the wrong thing and worse +than silence, because a reader who is told something plausible stops looking. +`step` checks `declined` first, so `composed` has no arm for any of them and the +two lists cannot overlap. + +A declining step takes its **whole sequence** with it: `o` in RecordsNormal is +`[add-record-row, enter-edit-at-cursor]`, and the second step's premise is false +once the first has declined. Returning only the decline, rather than the decline +plus whatever composed before it, is what keeps `Binding::Sequence`'s "every +step or none of them" true — an abort is not a rollback (gotcha 21), and this +client has no way to undo a `Command` it has already put on the wire. + +The classes that remain: - **Prefix keys.** `Command::SetTransientPrefix` exists and this client could arm one; what it cannot do is what the *second* key then runs, which is @@ -474,13 +503,23 @@ that **does nothing at all** in a browser in the formula panel, not in the grid. - **Anything touching storage or an arbitrary command line.** The host's by definition. -- **The records-mode commit.** A wide-records row is one coordinate tuple and - editing a category column moves every measure cell in it; - `commit_plain_records_edit` needs `GridLayout::record_col` to know which. +- **The records-mode commit** — *except the two creating columns*. A wide-records + row is one coordinate tuple and editing a category column moves every measure + cell in it; `commit_plain_records_edit` needs `GridLayout::record_col` to know + which. A `CellId::Create` does not: it commits the typed NAME as one existing + `ModelCommand` (`AddItem` into `_Measure`, or `AddCategory`), reads no + `record_col` and writes no cell, so improvise-gcvb narrowed the early return + rather than removing it. The `+cat` half **must not advance**: an inserted + column renumbers every index to its right and the advance was computed from + the pre-commit layout (gotcha 23), so `CommitAndAdvance` returns straight + after it and a client that advanced anyway would land one column off, + silently. `AddCategory` can also be refused by the 12-category limit and sets + `abort_effects`; the mode change therefore goes **first**, where an abort + cannot reach it (gotcha 21). - **Drill.** Session-local residue, and the one case that does **not** go upstream silently — see below. -### `strands_the_user`: a prompt this client can enter but cannot commit +### `stranding`: a prompt this client can enter but cannot commit `enter-mode` is trivially resolvable here, and that is the trap: the user reaches the prompt, types a path, presses Enter — and the commit resolves to @@ -497,10 +536,12 @@ meaningless: the worker persists on its own behalf whenever the document is dirty, so the document *is* the OPFS entry and there is no Ctrl-S semantics to offer. -The predicate is named as a property rather than matched at the call site -(`doctrine(two_questions_one_enum)`): "can this client finish what entering the -mode starts" is not the same question as "is this a text-entry mode", and a -third such prompt belongs here. Found by Copilot on PR #27 against the newly +It returns the **subject** rather than a bool (improvise-rmvx): the refusal is +right and the silence was not, so `:` now says the command line is unavailable +instead of doing nothing at all. The predicate is named as a property rather +than matched at the call site (`doctrine(two_questions_one_enum)`): "can this +client finish what entering the mode starts" is not the same question as "is +this a text-entry mode", and a third such prompt belongs here. Found by Copilot on PR #27 against the newly added `SavePrompt`; `ExportPrompt` had the identical trap already and was fixed with it — fixing one arm of a rule with two is the shape `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names. @@ -946,6 +987,19 @@ Four smaller decisions that would be re-derived wrongly: broken grid. `editing` is `AppMode::is_editing()` plus the `edit` buffer: without it, a browser that resolved `i` locally would change its mode indicator and then swallow every keystroke invisibly. +- **`minibuffer` is the same argument, for the other six text-entry modes** + (improvise-fdt6). `editing` drives the *inline cell editor*, so it is scoped to + the two cell-editing modes and reads the `"edit"` buffer by name — which left + FormulaEdit, CategoryAdd, ItemAdd, SavePrompt, ExportPrompt and CommandMode + opening a prompt and swallowing every character typed into it. `minibuffer` is + `AppMode::minibuffer()` plus **the buffer that config names**, read with + `improvise_protocol::read_buffer` so the `"search"` special case keeps its one + home (gotcha 11); `None` is "no text-entry mode is open", so the page asks + nothing about the mode. The colour stays semantic — `MinibufColor` crosses and + the CSS class is the page's last translation, exactly as `draw.rs::minibuf_fg` + is the TUI's (gotcha 26). It is drawn as a line of its own rather than *in + place of* `#status` as the TUI's bottom bar does, because `#status` also + carries the transport's failures and a repaint must not erase one (gotcha 48). `RenderCell::edit_text` is the stored value, not the formatted one: seeding an editor from `text` and committing turns `1200` into the string `"1,200"`. The @@ -1303,6 +1357,7 @@ it about", and the browser half has four: | `RenderModel` → DOM | every field the render model projects reaches an element | `web/tests/render_surface.test.js` (statically) **and** `harness/tests/contract.spec.js` (on the screen) | | JS → wasm | the calls the page makes exist, with the arity the bundle declares, and their answers are painted | `web/tests/bundle/wasm_surface.test.js` (the surface) **and** `contract.spec.js`'s pointer test (the wiring) | | page → peer | the handshake goes out first, a frame is answered whole, a failure is named, **and the page routes each failure through the call that disarms the watchdog** | `harness/tests/shell.spec.js`, `harness/tests/drill.spec.js` | +| key → screen | a key the mode's hint line ADVERTISES produces something the user can see, and a text-entry mode paints what is typed into it | `harness/tests/keys.spec.js` | | worker → OPFS | `storage_contract`, run against a real `OpfsFs` | `harness/tests/opfs.spec.js` | **A unit test of a rule does not test the wiring that reaches it**, and the @@ -1383,6 +1438,17 @@ enough to make twice, from different sides: widget and asserts the element is *on the screen*. Node cannot see that, and the moment a reduction would have to weaken it, it stays here. +**And the static half is weaker than it reads, which is worth knowing before +leaning on it.** `render_surface.test.js` asks whether the string +`model.` occurs anywhere in the page's code — so a field *mentioned* by a +function nothing ever calls passes. That happened on improvise-fdt6: the new +`minibuffer` field had a `paintMinibuffer` reading it, and the call site and the +`getElementById` were both missing, so the node suite went green over a page +that still painted nothing. `keys.spec.js` failed on its first run, in Chromium, +naming the hidden element. The static check is a **completeness** check — it +tells you a field was not forgotten — and never an **execution** check; only +`harness/` is that (gotcha 48). + What is deliberately **not** in Playwright any more is what each key or click *means*: `improvise_protocol::resolve` decides that and its tests are where the meaning is pinned. One instance of "a resolved change repaints" is kept, because diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 05947528..f77e7b86 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1703,3 +1703,104 @@ not the text, so a cell drawn blank passed. The assertion was strengthened until the injection failed, which is the only reason it is worth having — and it is the same lesson as the eleven tests above: a class is a flag, and a flag is not a pixel. + +### every_advertised_command_answers_in_its_own_mode + +improvise-rmvx. Gotcha 47 says an upstream key is a black hole, and +`the_command_names_that_still_need_the_host_are_these` is the guard that was +supposed to keep the list of them honest. It was built with +`in_mode(AppMode::Normal)`, so it only ever asked about Normal-mode bindings: +`add-record-row` (RecordsNormal) and `delete-formula-at-cursor` (FormulaPanel) +were outside its reach entirely and could never have failed it. **A named list +that silently scopes itself to one mode is a guard that looks broader than it +is.** + +This asks it of every mode, and it asks it the only way that is not +over-reporting. The naive version resolves every advertised name against an +EMPTY `ClientState` and flags `edit-or-drill`, `commit-cell-edit`, +`create-and-switch-view` and `page-prev`/`page-next` — but those are *handled* +names refused by STATE, which is correct behaviour and not this bug. So the +fixture is `furnished`: every widget has something in it, a basis is published, +and a window is announced, so an `Upstream` can only mean the name is +unhandled. + +Two things it does NOT do, each because the first attempt did and was wrong. +It does not call `step` with the advertised args: `advertised` carries `None` +for "any binding of this command", and `step`'s numeric and mode arguments are +mandatory, so that reports `move-selection`, `move-panel-cursor` and +`move-tile-cursor` as gaps in every mode. And it does not tokenise the rendered +hint string: `help::advertised` is a table of command NAMES, so the +cross-reference is exact and anything reading the rendered text is solving a +harder problem than exists. The keys come off `Keymap::keys_running`, which +`labels_for` now shares so "which keys" and "how are they spelled" cannot give +two answers. + +**The injection**: delete the `"toggle-prune-empty"` arm from `declined`. The +test fails naming `NORMAL: Char('P')` and `RECORDS: Char('P')` — both modes, +which is the widening the ticket was about, and a build that ran rather than a +compile error. + +What it found that the ticket did not predict: `view-back` is advertised in +RecordsNormal (`<`, captioned "back"), so it was a seventh instance of the same +class. The ticket had it filed under the Normal-mode host list instead, where +being upstream is not a defect. + +The five that remain are listed by name with a reason each. Three of them — +`commit-export`, `commit-save`, `execute-command` — are reachable only from a +mode `declined` refuses at the door, so a browser never stands in front of the +hint; the other two are improvise-gcvb's records-cell commit, which needs +`record_col`. + +### a_locally_committed_creating_column_agrees_with_the_host + +improvise-gcvb. `resolve`'s commit returned `None` for the whole of records +mode, and the reason given was sound for the coordinate and measure columns — +a category column renames every measure cell in the row, which needs +`record_col` (gotcha 1). The guard was broader than its reason: a cell at the +cursor now projects `CellId::Create`, and committing one is a single existing +`ModelCommand` over the typed NAME that reads no `record_col` and writes no +cell. + +Both halves are proved by injection, and the second is the one that matters. +Putting the broad `is_records()` early return back makes the test fail at "the +client must answer this key locally". Deleting the `CreateKind::Category` early +return — so `+cat` advances like any other column — fails with the client in +`RecordsEditing` where the host is in `RecordsNormal`: the TUI returns straight +after a `+cat` commit with a `RebuildLayout` and no cursor move, because an +inserted column renumbers every index to its right and the advance was computed +from the pre-commit layout (gotcha 23). A client that advanced anyway would land +one column off, silently. + +`both_ends` compares the document as `.improv` text, the mode, the status and +every cursor, so neither half can pass by agreeing on the wrong thing. The +fixture drives *both* hosts into records mode with the `R` key rather than +hand-building a `_Records` view, because `twin_of` copies only the session +slice and a twin without the view would refuse the commit for the wrong reason. + +### every_minibuffer_mode_paints_what_is_being_typed + +improvise-fdt6. `commit-buffer formula` already resolved on the client — that +half of the ticket was closed by an earlier wave — so the browser could enter +FormulaEdit, type a formula and commit it. What did not exist was any way to +SEE it: `RenderModel::editing` is populated only when `mode.is_editing()`, which +is the two *cell*-editing modes, and it reads the `"edit"` buffer by name. Every +character typed into FormulaEdit, CategoryAdd, ItemAdd, SavePrompt, +ExportPrompt or CommandMode went into a buffer nothing painted. The mode opened, +the user typed, and the screen did not move — which is exactly the dead-room +shape the ticket names, arrived at from the rendering side rather than the +resolution side. + +The value is read with `improvise_protocol::read_buffer` rather than +`buffers.get`, so the `"search"` special case (gotcha 11) keeps its one home. + +**The injection**: `.filter(|_| state.view.mode.is_editing())` on the new +field, which is the pre-fix behaviour exactly. It fails naming `"formula"`. + +**And the static check was not enough, which is the part worth keeping.** +`web/tests/render_surface.test.js` asserts that every `pub` field of +`RenderModel` appears as `model.` somewhere in `app.js`. It passed while +the page was still broken: the string `model.minibuffer` was inside +`paintMinibuffer`, and `paintMinibuffer` was never called and `el.minibuffer` +was never looked up. A field can be *mentioned* by a function nothing invokes. +`harness/tests/keys.spec.js` caught it on the first run, in the browser, which +is the whole argument for that directory existing (gotcha 48). diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 20ec0e86..1a5828a5 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -53,7 +53,7 @@ fn hint_tail(mode: &AppMode) -> &'static str { /// What each mode advertises: command names and captions, never keys. The /// keys come off the keymap, so a rebinding reaches the status bar for free. -fn advertised(mode: &AppMode) -> Vec<(&'static str, Option>, &'static str)> { +pub(crate) fn advertised(mode: &AppMode) -> Vec<(&'static str, Option>, &'static str)> { let arg = |a: &str| Some(vec![a.to_string()]); match mode { AppMode::Normal => vec![ @@ -782,32 +782,35 @@ fn commands() -> Vec { .collect() } +/// Every `AppMode`, as one list. Shared with `resolve`'s tests so a +/// cross-reference cannot silently scope itself to one mode (improvise-rmvx). +#[cfg(test)] +pub(crate) fn all_modes() -> Vec { + vec![ + AppMode::Normal, + AppMode::editing(), + AppMode::formula_edit(), + AppMode::FormulaPanel, + AppMode::CategoryPanel, + AppMode::category_add(), + AppMode::item_add("Region".into()), + AppMode::ViewPanel, + AppMode::TileSelect, + AppMode::ImportWizard, + AppMode::export_prompt(), + AppMode::save_prompt(), + AppMode::command_mode(), + AppMode::Help, + AppMode::Quit, + AppMode::RecordsNormal, + AppMode::records_editing(), + ] +} + #[cfg(test)] mod tests { use super::*; - fn all_modes() -> Vec { - vec![ - AppMode::Normal, - AppMode::editing(), - AppMode::formula_edit(), - AppMode::FormulaPanel, - AppMode::CategoryPanel, - AppMode::category_add(), - AppMode::item_add("Region".into()), - AppMode::ViewPanel, - AppMode::TileSelect, - AppMode::ImportWizard, - AppMode::export_prompt(), - AppMode::save_prompt(), - AppMode::command_mode(), - AppMode::Help, - AppMode::Quit, - AppMode::RecordsNormal, - AppMode::records_editing(), - ] - } - /// The property the old hand-written `hint_text` needed a 60-line /// tokenizer to approximate. See `context/test-evidence/protocol.md`. #[test] diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 437b5a8e..1345933e 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -208,22 +208,35 @@ impl Keymap { &self.bindings } - /// Every key label here or in a parent that runs `command`. `args` of - /// `None` accepts any; §8 says why that is the useful granularity. - pub fn labels_for(&self, command: &str, args: Option<&[String]>) -> Vec { - let mut labels: Vec = self + /// Every key here or in a parent that runs `command`. `args` of `None` + /// accepts any; §8 says why that is the useful granularity. + pub fn keys_running( + &self, + command: &str, + args: Option<&[String]>, + ) -> Vec<(KeyCode, KeyModifiers)> { + let mut keys: Vec<(KeyCode, KeyModifiers)> = self .bindings .iter() .filter(|(_, binding)| runs(binding, command, args)) .filter_map(|(pattern, _)| match pattern { - KeyPattern::Key(code, mods) => Some(key_label(code, *mods)), + KeyPattern::Key(code, mods) => Some((*code, *mods)), KeyPattern::AnyChar | KeyPattern::Any => None, }) .collect(); if let Some(parent) = &self.parent { - labels.extend(parent.labels_for(command, args)); + keys.extend(parent.keys_running(command, args)); } - labels + keys + } + + /// The same keys, spelled. One walk, so a caller asking "which keys" and + /// a caller asking "how are they written" cannot get different answers. + pub fn labels_for(&self, command: &str, args: Option<&[String]>) -> Vec { + self.keys_running(command, args) + .iter() + .map(|(code, mods)| key_label(code, *mods)) + .collect() } /// Look up the binding for a key in this keymap's own bindings. diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 8ba13e21..2df8695f 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -29,7 +29,7 @@ pub use render::{ ViewListCache, ViewSummary, WidgetCache, WizardCache, WizardDateField, WizardFieldKind, WizardProposal, WizardStepKind, header_level_widths, }; -pub use resolve::{Click, KeyOutcome, click, resolve, typed_cell_command}; +pub use resolve::{Click, KeyOutcome, click, read_buffer, resolve, typed_cell_command}; pub use viewmodel::{CatTreeEntry, flatten_cat_tree, view_axis_summary}; // `improvise-core`'s, because the index that resolves one lives in // `DataStore`. Re-exported so a client depends on the protocol alone. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index ce9174dd..2dcb03c0 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -7,10 +7,14 @@ use crate::client::ClientState; use crate::keymap::{Binding, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, flatten_cat_tree}; use crate::{ - AppMode, CellId, CellValue, Command, GridExtent, HelpPage, KeyCode, KeyEvent, ModelCommand, - Panel, RecordCell, ViewState, records_named, + AppMode, CellId, CellValue, Command, CreateKind, GridExtent, HelpPage, KeyCode, KeyEvent, + ModelCommand, Panel, RecordCell, ViewState, records_named, }; +/// The virtual category a new measure is filed under, spelled as +/// `commit.rs::add_measure` spells it. +const MEASURE_CATEGORY: &str = "_Measure"; + /// What this client decided to do with a keypress. #[derive(Debug, Clone, PartialEq)] pub enum KeyOutcome { @@ -54,8 +58,8 @@ pub fn resolve(state: &ClientState, event: KeyEvent) -> KeyOutcome { // the second key then runs is not. See `context/code/clients.md`. Binding::Prefix(_) => KeyOutcome::Upstream(event), Binding::Cmd { name, args } => match step(state, event, name, args) { - Some(commands) => KeyOutcome::of(commands), - None => KeyOutcome::Upstream(event), + Step::Local(commands) | Step::Declined(commands) => KeyOutcome::of(commands), + Step::Upstream => KeyOutcome::Upstream(event), }, // All or nothing, each step resolved against the *unchanged* state. // See `context/code/clients.md`. @@ -185,7 +189,8 @@ fn focus(state: &ClientState, panel: Panel) -> Vec { commands } -/// Every step of a `Binding::Sequence`, or none of them. +/// Every step of a `Binding::Sequence`, or none of them. A declining step takes +/// the whole sequence with it — `context/code/clients.md` (gotcha 21). fn sequence( state: &ClientState, event: KeyEvent, @@ -193,14 +198,61 @@ fn sequence( ) -> Option> { let mut commands = Vec::new(); for (name, args) in steps { - commands.extend(step(state, event, name, args)?); + match step(state, event, name, args) { + Step::Local(more) => commands.extend(more), + Step::Declined(said) => return Some(said), + Step::Upstream => return None, + } } Some(commands) } +/// What one command name resolved to. `Declined` is composed here like +/// `Local`, and differs only in what it does to a sequence. +enum Step { + Local(Vec), + Declined(Vec), + Upstream, +} + +/// One name, answered. A decline is checked first, so [`composed`] has no arm +/// for any of them and the two lists cannot overlap. +fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Step { + match declined(name, args) { + Some(said) => Step::Declined(vec![Command::SetStatus(said)]), + None => match composed(state, event, name, args) { + Some(commands) => Step::Local(commands), + None => Step::Upstream, + }, + } +} + +/// The commands this client cannot compose and answers **out loud**; the one +/// home for the wording. Why silence was worse: clients.md (improvise-rmvx). +fn declined(name: &str, args: &[String]) -> Option { + let what = match name { + "drill-into-cell" => "drill", + "toggle-prune-empty" => "hiding empty rows and columns", + "toggle-records-mode" => "records mode", + "add-record-row" => "adding a record row", + "delete-formula-at-cursor" => "deleting a formula", + "view-back" => "stepping back to the previous view", + // A door this client will not open still has to say so: entering is + // resolvable and the commit behind it is not. + "enter-mode" => stranding(&AppMode::from_name(args.first()?)?)?, + _ => return None, + }; + Some(format!("{what} is not available in the browser yet")) +} + /// The subset of command names this client can turn into `Command`s; `None` /// means "the host's". Each arm mirrors its `Cmd` line for line — clients.md. -fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Option> { +fn composed( + state: &ClientState, + event: KeyEvent, + name: &str, + args: &[String], +) -> Option> { match name { // ── the grid cursor ────────────────────────────────────────── "move-selection" => grid_move(state, GridMove::Relative(num(args, 0)?, num(args, 1)?)), @@ -232,13 +284,7 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op "clear-buffer" => Some(vec![write_buffer(args.first()?, String::new())]), // ── mode entry ─────────────────────────────────────────────── - "enter-mode" => { - let mode = AppMode::from_name(args.first()?)?; - match strands_the_user(&mode) { - true => None, - false => Some(entered(mode)), - } - } + "enter-mode" => Some(entered(AppMode::from_name(args.first()?)?)), // ── panels ─────────────────────────────────────────────────── "cycle-panel-focus" => Some(cycle_panel_focus(&state.view)), @@ -280,12 +326,6 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op cmd: ModelCommand::TransposeAxes, }]), - // The hint line advertises `>`, and an upstream key is a black hole - // (gotcha 47), so it said nothing at all. Interim: improvise-gd28. - "drill-into-cell" => Some(vec![Command::SetStatus( - "drill is not available in the browser yet".to_string(), - )]), - // ── editing ────────────────────────────────────────────────── "edit-or-drill" => edit_or_drill(state, AppMode::from_name(args.first()?)?), "commit-cell-edit" => commit(state, AppMode::from_name(args.first()?)?, Advance::Down), @@ -380,7 +420,7 @@ fn num(args: &[String], i: usize) -> Option { // The named buffers. A runtime-named buffer may not be an entry in `buffers` // at all: "search" is `ViewState::search_query` (improvise-gdl). -fn read_buffer(view: &ViewState, name: &str) -> String { +pub fn read_buffer(view: &ViewState, name: &str) -> String { match name { "search" => view.search_query.clone(), other => view.buffers.get(other).cloned().unwrap_or_default(), @@ -397,13 +437,15 @@ fn write_buffer(name: &str, value: String) -> Command { } } -/// A prompt this client can *enter* but cannot *commit*: a dead room is worse -/// than a locked door. All three arms, and who may delete each: clients.md. -fn strands_the_user(mode: &AppMode) -> bool { - matches!( - mode, - AppMode::SavePrompt { .. } | AppMode::ExportPrompt { .. } | AppMode::CommandMode { .. } - ) +/// A prompt this client can *enter* but cannot *commit*; `Some` is the subject +/// to decline by name. A dead room is worse than a locked door: clients.md. +fn stranding(mode: &AppMode) -> Option<&'static str> { + match mode { + AppMode::SavePrompt { .. } => Some("saving to a path"), + AppMode::ExportPrompt { .. } => Some("exporting"), + AppMode::CommandMode { .. } => Some("the command line"), + _ => None, + } } /// `EnterMode::execute`: clear the incoming mode's minibuffer, then change. @@ -630,27 +672,82 @@ enum Advance { /// `CommitAndAdvance::execute`, for the one case a thin client can compose: a /// plain grid cell. The three-way fork and the mirror order: clients.md. fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option> { - let cursor = state.view.cursor(); let (window_rows, window_cols) = state.window; if window_rows == 0 || window_cols == 0 { return None; } - // Records mode is the fork this client does not have: a category column - // renames every measure cell in the row, which needs `record_col`. + let text = read_buffer(&state.view, "edit"); + let id = cell_at_cursor(state)?.id.as_ref()?; + // Records mode needs `record_col` (gotcha 1) — except the two CREATING + // columns, which name nothing but themselves (improvise-gcvb). + if let CellId::Create(kind) = id { + return create_column(state, edit_mode, advance, kind, &text); + } if state.view.mode.is_records() { return None; } - let text = read_buffer(&state.view, "edit"); let mut commands = vec![Command::Model { basis: state.basis.claim([record_at_cursor(state)?])?, - cmd: typed_cell_command(cell_at_cursor(state)?.id.as_ref()?, &text)?, + cmd: typed_cell_command(id, &text)?, }]; commands.push(Command::ChangeMode(AppMode::Normal)); + commands.extend(advanced(state, edit_mode, advance)?); + Some(commands) +} +/// `CommitAndAdvance` over a creating column: a schema change over the typed +/// NAME. Why +cat does not advance: clients.md (improvise-gcvb). +fn create_column( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + kind: &CreateKind, + text: &str, +) -> Option> { + // `exit_mode_for`: the tidy-up goes FIRST, where an aborting + // `AddCategory` cannot reach it (gotcha 21). + let mut commands = vec![Command::ChangeMode(match state.view.mode.is_records() { + true => AppMode::RecordsNormal, + false => AppMode::Normal, + })]; + let name = text.trim(); + if !name.is_empty() { + commands.push(model( + state, + match kind { + CreateKind::Measure => ModelCommand::AddItem { + category: MEASURE_CATEGORY.to_string(), + item: name.to_string(), + }, + CreateKind::Category => ModelCommand::AddCategory(name.to_string()), + }, + )?); + } + let (row, col) = state.view.cursor().selected; + commands.push(Command::SetSelected { row, col }); + // An inserted column renumbers every index to its right, so staying put is + // what lands on the new one (improvise-2zq, gotcha 23). + if matches!(kind, CreateKind::Category) { + return Some(commands); + } + commands.extend(advanced(state, edit_mode, advance)?); + Some(commands) +} + +/// The advance and the re-entry `CommitAndAdvance` ends in. `None` where the +/// host would emit `AddRecordRow`, which this client cannot name (gotcha 1). +fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let row_max = rows.saturating_sub(1); let col_max = cols.saturating_sub(1); - let (row, col) = cursor.selected; + let (row, col) = state.view.cursor().selected; + if matches!(advance, Advance::Right) + && state.view.mode.is_records() + && row >= row_max + && col >= col_max + { + return None; + } let landed = match advance { Advance::Down if row < row_max => Some((row + 1, col)), Advance::Down if col < col_max => Some((0, col + 1)), @@ -662,15 +759,14 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Some((row, (col + 1).min(col_max))), }; let Some((nr, nc)) = landed else { - return Some(commands); + return Some(Vec::new()); }; - commands.extend(scrolled_to(state, nr, nc)); + let mut commands = scrolled_to(state, nr, nc); // `EnterEditAtCursor`: seed from where we landed, then change mode. The // new cell is a different cell, so a stale cache is correct here. - let seeded = seed_at(state, nr, nc); commands.push(Command::SetBuffer { name: "edit".to_string(), - value: seeded, + value: seed_at(state, nr, nc), }); commands.push(Command::ChangeMode(edit_mode)); Some(commands) @@ -1557,6 +1653,102 @@ mod tests { // ── what is bound and what is not ─────────────────────────────── + /// A client whose every widget has something in it, so an `Upstream` names + /// an **unhandled command** and never a state that said no (improvise-rmvx). + fn furnished(mode: AppMode) -> ClientState { + let mut state = client_on(record_cell()); + state.view.mode = mode; + state.grid.page_coords = vec![crate::PageAxis { + cat: "Quarter".into(), + selected: "Q1".into(), + items: Some(vec!["Q1".into(), "Q2".into()]), + }]; + crate::WidgetCache::apply( + &mut state.categories, + &crate::CategoryTreeUpdate::Categories(vec![crate::CategoryRow { + name: "Region".into(), + items: vec!["North".into(), "South".into()], + axis: Axis::Row, + }]), + ); + state.formulas.formulas = vec!["Profit = Revenue - Cost".into()]; + state.views.views = vec![ + crate::ViewSummary { + name: "Default".into(), + axes: Vec::new(), + }, + crate::ViewSummary { + name: "Second".into(), + axes: Vec::new(), + }, + ]; + state.tiles.tiles = vec![crate::TileEntry { + category: "Region".into(), + axis: Axis::Row, + }]; + state + .view + .buffers + .insert("formula".into(), "Profit = Revenue".into()); + state + .view + .buffers + .insert("category".into(), "Product".into()); + state.view.buffers.insert("item".into(), "Shirts".into()); + state.view.buffers.insert("edit".into(), "42".into()); + state + } + + /// improvise-rmvx. An advertised key that resolves upstream is a black hole + /// (gotcha 47). See `context/test-evidence/clients.md`. + #[test] + fn every_advertised_command_answers_in_its_own_mode() { + let mut unanswered = Vec::new(); + for mode in crate::help::all_modes() { + let Some(mode_key) = ModeKey::from_app_mode(&mode, false) else { + continue; + }; + let Some(keymap) = Keymaps::shared().get(&mode_key) else { + continue; + }; + let state = furnished(mode.clone()); + for (command, args, caption) in crate::help::advertised(&mode) { + for (code, mods) in keymap.keys_running(command, args.as_deref()) { + let event = KeyEvent::new(code, mods); + if resolve(&state, event) == KeyOutcome::Upstream(event) { + unanswered.push(format!( + "{}: {:?} → `{command} {args:?}` ({caption})", + crate::mode_label(&mode), + code + )); + } + } + } + } + unanswered.sort(); + unanswered.dedup(); + // The five that remain, each with the reason it is not a black hole. + // Closing one has to come here and say so; opening a sixth fails. + let expected = [ + // Reached only from a mode `declined` refuses at the door, so a + // browser is never standing in front of this hint. + "COMMAND: Enter → `execute-command None` (run)", + "EXPORT: Enter → `commit-export None` (export)", + "SAVE AS: Enter → `commit-save None` (save)", + // A records cell commit needs `record_col` (gotcha 1); only the two + // creating columns resolve, and this fixture is on a record cell. + "RECORDS INSERT: Enter → `commit-cell-edit None` (commit)", + "RECORDS INSERT: Tab → `commit-and-advance-right None` (commit+right)", + ]; + let mut expected: Vec = expected.iter().map(|s| s.to_string()).collect(); + expected.sort(); + assert_eq!( + unanswered, expected, + "advertised and unanswerable: anything here that is not in the \ + named list above reads as broken rather than as unavailable" + ); + } + /// The command **names** this client cannot execute, one by one, so that /// closing one of these gaps has to come here and say so. Evidence file. #[test] @@ -1570,14 +1762,11 @@ mod tests { (ev(KeyCode::Char('Z')), ""), (ev(KeyCode::Char('H')), "hide-selected-row-item"), (ev(KeyCode::Char('z')), "toggle-group-under-cursor"), - // `>` is NOT here any more: it answers with a status of its own, - // because it is advertised and silence read as broken (gd28). - (ev(KeyCode::Char('<')), "view-back"), + // `>`, `<`, `R`, `P`, Ctrl-W and Ctrl-E are NOT here any more: each + // answers with a status of its own (improvise-gd28, improvise-rmvx). (ev(KeyCode::Char('/')), "search"), (ev(KeyCode::Char('n')), "search-navigate"), (ev(KeyCode::Char('N')), "search-or-category-add"), - (ev(KeyCode::Char('R')), "toggle-records-mode"), - (ev(KeyCode::Char('P')), "toggle-prune-empty"), (ev(KeyCode::Enter), "enter-advance"), ( KeyEvent::new(KeyCode::Char('s'), KeyModifiers::CONTROL), @@ -1587,16 +1776,6 @@ mod tests { KeyEvent::new(KeyCode::Char('q'), KeyModifiers::CONTROL), "force-quit", ), - // The two doors this client refuses rather than opens — - // `enter-mode` is resolvable, the commit behind it is not. - ( - KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL), - "enter-mode save-prompt (commit-save is the host's)", - ), - ( - KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), - "enter-mode export-prompt (commit-export is the host's)", - ), ] { assert_eq!( resolve(&state, key), @@ -1638,19 +1817,31 @@ mod tests { #[test] fn a_prompt_this_client_cannot_commit_is_never_entered() { let state = in_mode(AppMode::Normal); - for (key, mode) in [ + for (key, mode, subject) in [ ( KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL), "save-prompt", + "saving", ), ( KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), "export-prompt", + "exporting", ), + (ev(KeyCode::Char(':')), "command-mode", "command line"), ] { - assert_eq!( - resolve(&state, key), - KeyOutcome::Upstream(key), + let KeyOutcome::Local(said) = resolve(&state, key) else { + panic!("{mode} must be refused *out loud*, not in silence"); + }; + let [Command::SetStatus(msg)] = &said[..] else { + panic!("{mode}: expected one status, got {said:?}"); + }; + assert!( + msg.contains(subject), + "{mode}: the refusal must name its own subject, got {msg}" + ); + assert!( + !said.iter().any(|c| matches!(c, Command::ChangeMode(_))), "{mode} must not be entered by a client that cannot commit it" ); } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index e792f090..30a20693 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -55,6 +55,20 @@ pub struct RenderModel { /// What the user has typed so far while a keyboard edit is open on the /// selected cell. The decision is made here — `context/code/clients.md`. pub editing: Option, + /// The bottom-bar prompt while ANY text-entry mode is open, cell edits + /// included. `None` elsewhere, so the page asks nothing about the mode. + pub minibuffer: Option, +} + +/// The prompt, what has been typed into it, and the colour. WHICH buffer is +/// the mode's own answer (gotcha 11) — never a name this module spells. +#[derive(Debug, Clone, PartialEq, Serialize)] +pub struct RenderMinibuffer { + pub prompt: String, + pub value: String, + /// Semantic, as `MinibufColor` is: the last translation to a CSS class is + /// the page's, exactly as `draw.rs::minibuf_fg` is the TUI's (gotcha 26). + pub color: improvise_protocol::MinibufColor, } /// A grid row: either a group header spanning the row, or data. An enum so a @@ -251,6 +265,11 @@ pub fn render_model(state: &ClientState) -> RenderModel { .mode .is_editing() .then(|| state.view.buffers.get("edit").cloned().unwrap_or_default()), + minibuffer: state.view.mode.minibuffer().map(|mb| RenderMinibuffer { + prompt: mb.prompt.clone(), + value: improvise_protocol::read_buffer(&state.view, &mb.buffer_key), + color: mb.color, + }), } } @@ -813,6 +832,40 @@ mod tests { ); } + /// improvise-fdt6. A room the browser could enter, typing into a buffer + /// nothing painted. See `context/test-evidence/clients.md`. + #[test] + fn every_minibuffer_mode_paints_what_is_being_typed() { + for mode in [ + improvise_protocol::AppMode::formula_edit(), + improvise_protocol::AppMode::category_add(), + improvise_protocol::AppMode::item_add("Region".into()), + improvise_protocol::AppMode::editing(), + ] { + let config = mode.minibuffer().expect("a text-entry mode").clone(); + let mut client = client_with_grid(ViewState::default()); + assert_eq!( + client.model().minibuffer, + None, + "Normal has no minibuffer to paint" + ); + client.state_mut().fold(&Command::ChangeMode(mode.clone())); + client.state_mut().fold(&Command::SetBuffer { + name: config.buffer_key.clone(), + value: "Profit = Revenue - Cost".into(), + }); + let painted = client + .model() + .minibuffer + .unwrap_or_else(|| panic!("{:?} must paint its minibuffer", config.buffer_key)); + assert_eq!(painted.prompt, config.prompt); + assert_eq!( + painted.value, "Profit = Revenue - Cost", + "the page must draw what the user typed, not an empty line" + ); + } + } + /// The whole model crosses as JSON, so the page needs no bindings. #[test] fn the_render_model_crosses_as_json() { diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 65334060..7bafab85 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1070,3 +1070,69 @@ fn page_selections(app: &App) -> Vec<(String, Option)> { .map(|cat| (cat.to_string(), view.page_selection(cat).map(String::from))) .collect() } + +/// Both hosts driven into records mode the way a user gets there, so the +/// `_Records` view and its `_Index`/`_Dim` axes are the real ones. +fn records_pair() -> (App, App) { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let mut app = furnished_host(); + let mut twin = furnished_host(); + reduce_key(&mut app, r); + reduce_key(&mut twin, r); + (app, twin) +} + +/// Where the client's own projected grid puts a creating column. Asked of the +/// projection, never re-derived from the axes — that is ADR 002's whole point. +fn creating_cell(client: &ClientState, kind: improvise_protocol::CreateKind) -> (usize, usize) { + for (r, row) in client.grid.cells.iter().enumerate() { + for (c, cell) in row.iter().enumerate() { + if cell.id.as_ref() == Some(&improvise_protocol::CellId::Create(kind)) { + return (r, c); + } + } + } + panic!("records mode must project a {kind:?} creating column"); +} + +/// Put all three ends on one cell with one buffer, mid-edit. +fn typing_at( + app: &mut App, + twin: &mut App, + client: &mut ClientState, + at: (usize, usize), + text: &str, +) { + for host in [&mut *app, &mut *twin] { + host.view_state.mode = AppMode::records_editing(); + host.view_state.cursor_mut().selected = at; + host.view_state.buffers.insert("edit".into(), text.into()); + host.rebuild_layout(); + } + client.view = app.view_state.clone(); +} + +/// improvise-gcvb. The records-mode early return was broader than its reason. +/// Why `+cat` must NOT advance: `context/test-evidence/clients.md`. +#[test] +fn a_locally_committed_creating_column_agrees_with_the_host() { + for (kind, typed) in [ + (improvise_protocol::CreateKind::Category, "Season"), + (improvise_protocol::CreateKind::Measure, "Cost"), + ] { + let (mut app, mut twin) = records_pair(); + let mut client = furnished_client(&app, (12, 12)); + let at = creating_cell(&client, kind); + typing_at(&mut app, &mut twin, &mut client, at, typed); + client = furnished_client(&app, (12, 12)); + client.view = app.view_state.clone(); + + both_ends(&mut app, &mut twin, &mut client, "Enter"); + + assert!( + document_text(&app).contains(typed), + "{kind:?}: the commit must reach the document: {}", + document_text(&app) + ); + } +} diff --git a/harness/tests/keys.spec.js b/harness/tests/keys.spec.js new file mode 100644 index 00000000..7939dec0 --- /dev/null +++ b/harness/tests/keys.spec.js @@ -0,0 +1,68 @@ +// Keys the page ADVERTISES, executed. Two properties: a key this client cannot +// compose declines out loud (improvise-rmvx), and the one text-entry room the +// browser could enter but never showed you typing in (improvise-fdt6). + +import { test, expect } from "@playwright/test"; + +/// The shipped demo, focused, with a grid on screen. `#grid` must have focus +/// or the page's keydown listener never sees a key and every assertion below +/// would pass or fail for a reason that has nothing to do with resolution. +async function theDemo(page) { + await page.goto("/app/index.html"); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await page.locator("#grid").focus(); + await expect(page.locator("#grid td.editable").first()).toBeVisible(); +} + +test.describe("an advertised key is never silent", () => { + // Each of these is named in the mode's own hint line, so silence read as + // broken rather than as unavailable (gotcha 47). The subjects must differ: + // one message reached by three keys would tell the user nothing. + test("the three Normal-mode keys this client cannot compose each say their own subject", async ({ + page, + }) => { + await theDemo(page); + + for (const [key, said] of [ + ["P", "hiding empty rows and columns is not available in the browser yet"], + ["R", "records mode is not available in the browser yet"], + [":", "the command line is not available in the browser yet"], + ]) { + // A stale status would let the NEXT key pass on the previous key's + // message, so each assertion is preceded by a state it is not. + await expect(page.locator("#status")).not.toContainText(said); + await page.keyboard.press(key); + await expect(page.locator("#status")).toContainText(said); + // A refusal that changed the mode would be a door opened anyway. + await expect(page.locator("#mode")).toHaveText("NORMAL"); + } + }); +}); + +test.describe("a formula can be composed in the browser", () => { + // improvise-fdt6. `commit-buffer formula` resolved here already; what did + // not exist was any way to SEE the typing, so the room swallowed it. + test("the formula minibuffer shows what is typed and the commit reaches the panel", async ({ + page, + }) => { + await theDemo(page); + + await expect(page.locator("#minibuffer")).toBeHidden(); + + await page.keyboard.press("F"); + await expect(page.locator("#mode")).toHaveText("FORMULAS"); + await page.keyboard.press("n"); + await expect(page.locator("#mode")).toHaveText("FORMULA"); + + // The property: every character is visible while it is being typed. + await expect(page.locator("#minibuffer")).toBeVisible(); + await page.keyboard.type("Margin = Profit"); + await expect(page.locator("#minibuffer")).toContainText("Margin = Profit"); + + await page.keyboard.press("Enter"); + await expect(page.locator("#minibuffer")).toBeHidden(); + // And it reached the DOCUMENT, not just the buffer: the panel is a + // projection the host sends back. + await expect(page.locator("#formulas")).toContainText("Margin = Profit"); + }); +}); diff --git a/web/app.js b/web/app.js index cf4fac63..9c496209 100644 --- a/web/app.js +++ b/web/app.js @@ -30,6 +30,7 @@ const el = { views: document.getElementById("views"), tiles: document.getElementById("tiles"), mode: document.getElementById("mode"), + minibuffer: document.getElementById("minibuffer"), hint: document.getElementById("hint"), help: document.getElementById("help"), status: document.getElementById("status"), @@ -158,6 +159,7 @@ function repaint() { // itself; see `RenderModel::hint`. el.hint.textContent = model.hint; paintHelp(model); + paintMinibuffer(model); shell.setModel(model.status); el.viewName.textContent = model.view_name; el.grid.replaceChildren(gridTable(model)); @@ -179,6 +181,16 @@ function repaint() { if (changed) sendViewport(); } +/** The bottom-bar prompt. `null` is "no text-entry mode is open", decided in + * `render.rs`, so the page asks nothing about the mode (improvise-fdt6). */ +function paintMinibuffer(model) { + const mb = model.minibuffer; + el.minibuffer.hidden = mb === null || mb === undefined; + if (el.minibuffer.hidden) return; + el.minibuffer.className = mb.color.toLowerCase(); + el.minibuffer.textContent = mb.prompt + mb.value; +} + /** The help overlay. An empty `help` is what says it is closed -- the page * asks no question about the mode, and writes none of the content: the rows, * their keys and the page titles are `improvise_protocol::help`'s, the same diff --git a/web/index.html b/web/index.html index bf2f40fa..8e3dd905 100644 --- a/web/index.html +++ b/web/index.html @@ -83,6 +83,13 @@
+ + + diff --git a/web/style.css b/web/style.css index f3de89d9..03908006 100644 --- a/web/style.css +++ b/web/style.css @@ -272,3 +272,21 @@ td input { .help-dim { opacity: 0.7; } + +/* The bottom-bar prompt while a text-entry mode is open. The colour is the + mode's own answer (`MinibufColor`); this is the last translation, exactly + as `draw.rs::minibuf_fg` is the TUI's. */ +#minibuffer { + padding: 0.25rem 0.5rem; + font-family: inherit; + border-top: 1px solid var(--accent); +} +#minibuffer.green { + color: #7ec699; +} +#minibuffer.cyan { + color: #6fd0d8; +} +#minibuffer.yellow { + color: #d8c96f; +} From 4aa4e88ce49001f4f1105bee3ccef0659b53b78d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 16:55:04 -0700 Subject: [PATCH 2/3] fix(protocol): Ignored is a black hole too, and a decline cannot shadow working code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #71 review, blocking finding — and it was demonstrated rather than argued. `KeyOutcome` has TWO silent variants and the new guard watched one. It asserted `resolve(..) == Upstream(event)`, but `KeyOutcome::of(vec![])` returns `Ignored`, which puts nothing on the screen either. Degrading only the three declines no browser spec presses — add-record-row, delete-formula-at-cursor, view-back — to `Step::Declined(Vec::new())` left 1397 Rust, 19 harness and 26 node tests passing. Three of the six commands this change exists to make audible could go completely silent with the whole evidence base green. The generalisation is the part to keep. `KeyOutcome::of` turning an empty `Local` into an honest `Ignored` is good design and also a LAUNDERING step: every wrong answer that composes nothing arrives wearing the variant that is usually innocent. A guard's discriminator has to be STATE — did anything the user can see change — never the shape of the return value. That is why the `+cat` injection failed in `both_ends`, which compares documents and cursors, and the decline injection passed everywhere, where nothing compared a screen. Widening it costs ten entries, all one boundary case: `move-panel-cursor` mirrors a `Cmd` that returns nothing when the cursor does not move. `furnished` now carries three of everything with all three panel cursors seated at index 1, so the residue is zero rather than parked in the named list. Seating them at 0 and calling the `Up` results expected would have hidden the real defect in the same list as the boundary case. `nothing_declined_is_also_composed` closes the other direction, which was a doc comment. `step` reaches `declined` first, so an arm in `composed` for a declined name is unreachable — a future ticket implementing `view-back` would leave the browser saying "not available yet" over working code with nothing noticing. It is derived from `declined` itself over every `(name, args)` bound in every mode, parents and prefixes and sequence steps included, so a seventh decline needs no edit here. It found a real overlap on its first run: `enter-mode` is answered by both. Rather than move the check, both halves now hold the rule — `composed` returns `None` for a stranded mode — so removing one fails towards `Upstream` rather than towards a dead room. `Keymap::parent()` added so a test can walk the fallback chain. 1397 → 1398 Rust; harness 19 + 3 skipped; node 26. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/clients.md | 18 ++++ context/test-evidence/clients.md | 62 +++++++++++- crates/improvise-protocol/src/keymap.rs | 5 + crates/improvise-protocol/src/resolve.rs | 118 +++++++++++++++++++---- 4 files changed, 184 insertions(+), 19 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index df92f0b2..23eb013c 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -470,6 +470,24 @@ why `add-record-row` and `delete-formula-at-cursor` were outside its reach entirely; `view-back` in RecordsNormal was a seventh instance nobody had noticed. +**`Ignored` is a black hole too**, and the guard watches both variants. +`KeyOutcome::of` turning an empty `Local` into an honest `Ignored` is good +design and also a **laundering** step — every wrong answer that composes nothing +arrives wearing the variant that is usually innocent. A guard's discriminator +must therefore be *state* (did anything the user can see change) and never the +shape of the return value. Watching only `Upstream` let three of the six +declines go completely silent with 1397 Rust, 19 harness and 26 node tests +green, because no browser spec presses those three keys (PR #71 review). + +`nothing_declined_is_also_composed` closes the other direction: `step` reaches +`declined` first, so an arm in `composed` for a declined name is **unreachable**, +and the browser would go on saying "not available yet" over working code. It is +derived from `declined` itself over every bound `(name, args)` in every mode, so +a seventh decline is covered without editing it. `enter-mode` was a real overlap +on its first run; rather than move the check, **both halves hold the rule** — +`composed` returns `None` for a stranded mode — so removing one fails towards +`Upstream` rather than towards a dead room. + `declined` is the answer, and it is one list rather than an arm apiece: six command names, each with **its own true subject**. Wording matters here more than it looks — the `>` fix was nearly routed through a message about diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index f77e7b86..a38a80bc 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1735,10 +1735,43 @@ harder problem than exists. The keys come off `Keymap::keys_running`, which `labels_for` now shares so "which keys" and "how are they spelled" cannot give two answers. +**It watches BOTH silent variants, and the first version did not.** `KeyOutcome` +has two ways of saying nothing: `Upstream`, and `Ignored` — which +`KeyOutcome::of(vec![])` produces whenever a resolver composes an empty vector. +To a user they are the same black hole. The first version asserted +`resolve(..) == Upstream(event)`, so **three of the six commands this whole +change exists to make audible could go completely silent with the entire +evidence base green**: degrading only `add-record-row`, `delete-formula-at-cursor` +and `view-back` to `Step::Declined(Vec::new())` left 1397 Rust tests, 19 harness +tests and 26 node tests passing, because no browser spec presses those three +keys. Found on review of PR #71. The check is now `!matches!(.., Upstream(_) | +Ignored)`. + +**The generalisation is worth more than the fix.** `KeyOutcome::of` turning an +empty `Local` into an honest `Ignored` is good design and it is also a +**laundering** step: every wrong answer that composes nothing arrives wearing +the variant that is usually innocent. So a guard's discriminator has to be +*state* — did anything the user can see change — and never the shape of the +return value. That is exactly why the `+cat` injection in +`a_locally_committed_creating_column_agrees_with_the_host` failed (it compares +documents and cursors) and the decline injection passed everywhere (nothing +compared a screen). + +Widening the check costs ten more entries, all one legitimate boundary case: +`move-panel-cursor` mirrors `MovePanelCursor`, which returns nothing when the +cursor does not move, so a one-row panel or a cursor on an edge answers +`Ignored` for a reason that is not this test's subject. `furnished` therefore +carries **three** of everything with all three panel cursors seated at index 1. +Note what that rules out: seating them at 0 and calling the `Up` results +expected would have hidden the real defect in the same list as the boundary +case. + **The injection**: delete the `"toggle-prune-empty"` arm from `declined`. The test fails naming `NORMAL: Char('P')` and `RECORDS: Char('P')` — both modes, which is the widening the ticket was about, and a build that ran rather than a -compile error. +compile error. The review's own injection is also kept as a check: degrading the +three unpressed declines to empty now fails naming `FORMULAS: Char('d')`, +`FORMULAS: Delete`, `RECORDS: Char('<')` and `RECORDS: Char('o')`. What it found that the ticket did not predict: `view-back` is advertised in RecordsNormal (`<`, captioned "back"), so it was a seventh instance of the same @@ -1804,3 +1837,30 @@ the page was still broken: the string `model.minibuffer` was inside was never looked up. A field can be *mentioned* by a function nothing invokes. `harness/tests/keys.spec.js` caught it on the first run, in the browser, which is the whole argument for that directory existing (gotcha 48). + +### nothing_declined_is_also_composed + +`step` is `declined` then `composed`, and until this test the claim that the two +lists cannot overlap was a **doc comment**. The dangerous direction is not a +typo: it is a future ticket implementing `view-back` (or `toggle-prune-empty`, +which is one projection field away) in `composed` and not thinking to delete its +`declined` arm. The browser then goes on saying "not available in the browser +yet" while the working code sits unreachable, and nothing notices — the classic +shape of a check passing while the thing it names is broken. Raised on review of +PR #71. + +It is **derived from `declined` itself**, not from a list beside it: it walks +every `(name, args)` bound in every mode's keymap — parents and prefix +sub-keymaps included, sequence steps included — and for each one `declined` +answers, asserts `composed` does not. A seventh decline is covered without +coming here, which is the property a hand-written list could not have. + +**The injection**: add `"view-back" => Some(vec![Command::SwitchView(..)])` to +`composed`. It fails naming `NORMAL: view-back []` and `RECORDS: view-back []`. + +It also found a real overlap on its first run. `enter-mode` is answered by +`declined` for the three stranding modes *and* by `composed` for every mode, so +`composed`'s arm would have opened `SavePrompt`, `ExportPrompt` or `CommandMode` +if `declined` were ever bypassed. Rather than moving the check, **both halves now +hold the rule** — `composed` returns `None` for a stranded mode — so the failure +direction if one is removed is the pre-PR `Upstream`, not a dead room. diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 1345933e..23bda05b 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -208,6 +208,11 @@ impl Keymap { &self.bindings } + /// The keymap this one falls through to, if any. + pub fn parent(&self) -> Option<&Arc> { + self.parent.as_ref() + } + /// Every key here or in a parent that runs `command`. `args` of `None` /// accepts any; §8 says why that is the useful granularity. pub fn keys_running( diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 2dcb03c0..b05c8539 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -4,7 +4,7 @@ use improvise_core::view::Axis; use crate::client::ClientState; -use crate::keymap::{Binding, Keymaps, ModeKey}; +use crate::keymap::{Binding, Keymap, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, flatten_cat_tree}; use crate::{ AppMode, CellId, CellValue, Command, CreateKind, GridExtent, HelpPage, KeyCode, KeyEvent, @@ -284,7 +284,12 @@ fn composed( "clear-buffer" => Some(vec![write_buffer(args.first()?, String::new())]), // ── mode entry ─────────────────────────────────────────────── - "enter-mode" => Some(entered(AppMode::from_name(args.first()?)?)), + "enter-mode" => { + let mode = AppMode::from_name(args.first()?)?; + // `declined` answers this first, and both halves hold the rule: + // no path opens a room this client cannot commit. + stranding(&mode).is_none().then(|| entered(mode)) + } // ── panels ─────────────────────────────────────────────────── "cycle-panel-focus" => Some(cycle_panel_focus(&state.view)), @@ -1663,29 +1668,43 @@ mod tests { selected: "Q1".into(), items: Some(vec!["Q1".into(), "Q2".into()]), }]; + // THREE of everything, every panel cursor seated in the MIDDLE: an edge + // makes `move-panel-cursor` answer `Ignored` for the wrong reason. crate::WidgetCache::apply( &mut state.categories, - &crate::CategoryTreeUpdate::Categories(vec![crate::CategoryRow { - name: "Region".into(), - items: vec!["North".into(), "South".into()], - axis: Axis::Row, - }]), + &crate::CategoryTreeUpdate::Categories( + ["Region", "Product", "Quarter"] + .into_iter() + .map(|name| crate::CategoryRow { + name: name.into(), + items: vec!["North".into(), "South".into()], + axis: Axis::Row, + }) + .collect(), + ), ); - state.formulas.formulas = vec!["Profit = Revenue - Cost".into()]; - state.views.views = vec![ - crate::ViewSummary { - name: "Default".into(), + state.formulas.formulas = [ + "Profit = Revenue - Cost", + "Tax = Revenue * 0.1", + "Net = Profit", + ] + .into_iter() + .map(str::to_string) + .collect(); + state.views.views = ["Default", "Second", "Third"] + .into_iter() + .map(|name| crate::ViewSummary { + name: name.into(), axes: Vec::new(), - }, - crate::ViewSummary { - name: "Second".into(), - axes: Vec::new(), - }, - ]; + }) + .collect(); state.tiles.tiles = vec![crate::TileEntry { category: "Region".into(), axis: Axis::Row, }]; + state.view.cat_panel_cursor = 1; + state.view.formula_cursor = 1; + state.view.view_panel_cursor = 1; state .view .buffers @@ -1715,7 +1734,13 @@ mod tests { for (command, args, caption) in crate::help::advertised(&mode) { for (code, mods) in keymap.keys_running(command, args.as_deref()) { let event = KeyEvent::new(code, mods); - if resolve(&state, event) == KeyOutcome::Upstream(event) { + // BOTH silent variants. To a user `Ignored` is the same + // black hole as `Upstream` — nothing is on the screen. + let answered = !matches!( + resolve(&state, event), + KeyOutcome::Upstream(_) | KeyOutcome::Ignored + ); + if !answered { unanswered.push(format!( "{}: {:?} → `{command} {args:?}` ({caption})", crate::mode_label(&mode), @@ -1749,6 +1774,63 @@ mod tests { ); } + /// improvise-rmvx. `step` checks `declined` first, so a name answered + /// there makes an arm in `composed` UNREACHABLE. Evidence file. + #[test] + fn nothing_declined_is_also_composed() { + let mut shadowed = Vec::new(); + for mode in crate::help::all_modes() { + let Some(mode_key) = ModeKey::from_app_mode(&mode, false) else { + continue; + }; + let Some(keymap) = Keymaps::shared().get(&mode_key) else { + continue; + }; + let state = furnished(mode.clone()); + // Derived from `declined` itself rather than from a list beside + // it: a seventh decline is covered without coming here. + let mut vocabulary = Vec::new(); + bound_commands(keymap, &mut vocabulary); + for (name, args) in vocabulary { + if declined(&name, &args).is_none() { + continue; + } + if composed(&state, ev(KeyCode::Char('x')), &name, &args).is_some() { + shadowed.push(format!("{}: {name} {args:?}", crate::mode_label(&mode))); + } + } + } + shadowed.sort(); + shadowed.dedup(); + assert!( + shadowed.is_empty(), + "these are answered by BOTH `declined` and `composed`, and `step` \ + reaches `declined` first — so the browser keeps saying \"not \ + available yet\" while the working code sits unreachable:\n{}", + shadowed.join("\n") + ); + } + + /// Every `(name, args)` a keymap, its parents and its prefixes bind — + /// the whole vocabulary a keypress in that mode can reach. + fn bound_commands(keymap: &Keymap, out: &mut Vec<(String, Vec)>) { + for binding in keymap.bindings().values() { + match binding { + Binding::Cmd { name, args } => out.push((name.to_string(), args.clone())), + Binding::Sequence(steps) => out.extend( + steps + .iter() + .map(|(name, args)| (name.to_string(), args.clone())), + ), + // Prefixes are flat (gotcha 13), so this terminates. + Binding::Prefix(sub) => bound_commands(sub, out), + } + } + if let Some(parent) = keymap.parent() { + bound_commands(parent, out); + } + } + /// The command **names** this client cannot execute, one by one, so that /// closing one of these gaps has to come here and say so. Evidence file. #[test] From df8ee0d135be8420dc9e6e5d94eb459813467c66 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 17:13:38 -0700 Subject: [PATCH 3/3] fix(protocol): Keymap is a test-only import, and my clippy check was not CI's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's `cargo clippy --workspace --all-targets -- -D warnings` failed on an unused `Keymap` import: `bound_commands` takes a `&Keymap` and lives in the test module, so the non-test lib build has no use for it. Moved into `mod tests`. Worth recording WHY I did not catch it locally: I ran clippy without `-D warnings` and grepped its output for `^(error|warning)\[`, which matches a lint with a NAME in brackets and misses a bare `warning:` — which is exactly what `unused_imports` emits. A check whose filter cannot see the failure reads as a clean run, which is the shape this repo keeps finding. Run the gate CI runs, not one that looks like it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-protocol/src/resolve.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 54e448cd..c42644d4 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -4,7 +4,7 @@ use improvise_core::view::Axis; use crate::client::ClientState; -use crate::keymap::{Binding, Keymap, Keymaps, ModeKey}; +use crate::keymap::{Binding, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, flatten_cat_tree}; use crate::{ AppMode, CellId, CellValue, Command, CreateKind, GridExtent, HelpPage, KeyCode, KeyEvent, @@ -1109,6 +1109,7 @@ fn commit_buffer(state: &ClientState, kind: CommitKind) -> Option> #[cfg(test)] mod tests { use super::*; + use crate::keymap::Keymap; use crate::{AxisFrame, CellDisplay, GridUpdate, KeyModifiers, Projection, Snapshot}; fn ev(code: KeyCode) -> KeyEvent {