Merge pull request #130 from fiddlerwoaroof/fix/vw47-view-name

fix: a new view takes an unused name, derived once for both front ends

context/code/core.md

@@ -580,6 +580,43 @@ builds on demand. Both put the virtual `_Index`/`_Dim` categories on axes, and
 those categories are never persisted — so these views are rebuilt from scratch
 each time rather than saved (improvise-60z).
 
+### `create_view` replaces, and `unused_view_name` is why it may keep doing so
+
+`Workbook::create_view` inserts over whatever holds the name. That is not an
+oversight: "rebuilt from scratch each time" **is** that replacement, and both
+rebuilders reach it through `effect::CreateView` — `DrillIntoCell` and the
+records-mode command each emit a `CreateView` of their fixed name on every
+entry, so an entry into records mode would otherwise stack views or refuse.
+The third production caller, `parse.rs::build_view`, guards with
+`contains_key` and so cannot observe the difference.
+
+So the two callers that need a **new** name are the user-facing ones, and they
+are the ones that must not name a view that exists. Before improvise-vw47 both
+spelled `format!("View {}", views.len() + 1)`, which is the count and not the
+name: delete `View 2` from a workbook holding `Default`, `View 2`, `View 3` and
+the count says `View 3`, which is taken. `create_view` then did exactly what
+its other callers need and replaced it — the user's configured view silently
+reset to defaults, and no view added. **A symptom that looks like nothing
+happened.**
+
+Two fixes were rejected in favour of the helper, and both are worth naming
+because each turns a quiet wrong answer into a loud one at a caller that was
+never wrong. Making `create_view` **refuse** a name it holds breaks records
+mode outright — the key stops working, which reads as a better error and is a
+worse bug. Making it **rename** silently gives the rebuilders a `_Records 2`
+that `RECORDS_VIEW` does not name and nothing switches to.
+
+`view::unused_view_name` therefore answers only the question the two user-facing
+resolvers ask, and `create_view` keeps its semantics untouched. It takes an
+iterator of names rather than a `&Workbook` because the browser resolver holds
+no workbook — it has `ViewListCache`, a `Vec<ViewSummary>` that
+`project::view_summaries` maps one-for-one off `workbook.views`, so the two
+front ends see the same list and derive the same name **by calling the same
+function**, which is what design-principles §5 asks for and what
+`tests/lockstep.rs` witnesses. It starts its search at the count rather than at
+1 so that the common no-gap case keeps its existing answer; starting at 1 would
+rename the first view a fresh workbook creates from `View 2` to `View 1`.
+
 ## 12. `axis_of` panics, `try_axis_of` does not
 
 Registration is guaranteed for every model category: `Workbook::new`,

context/test-evidence/clients.md

@@ -3712,3 +3712,35 @@ and the reason this test is that test's twin rather than a new shape.
 It asserts on `Command::ChangeMode(m) if m.is_editing()` rather than on the
 status string, because the fault is *an editor opening*, and a fix that speaks
 and opens anyway is a distinct rival already covered at the route level.
+
+---
+
+### a_new_view_named_over_a_gap_agrees_with_the_host_and_spares_the_survivor
+
+improvise-vw47's cross-front-end half. The new view's name is derived in two
+places — `cmd/panel.rs::CreateAndSwitchView` and `resolve.rs`'s
+`"create-and-switch-view"` arm — and design-principles §5 says a derivation
+done twice is the defect. The previous agent on this ticket refused to fix one
+site for exactly that reason and implemented nothing, which was the right call.
+
+**What this test proves that `both_ends` alone cannot.** Against the unfixed
+source `both_ends` *passes*: both resolvers computed `format!("View {}", len +
+1)` and were wrong **identically**, so the two documents agreed and the
+agreement check saw nothing. The bug is therefore caught by the two assertions
+after it — the view count must rise, and the surviving `View 4`'s `,.4` format
+must be untouched — and it fails with `left: 3 right: 4`. So the test carries
+two separate claims and needs both: the ends agree, *and* what they agree on is
+right.
+
+The fixture is `furnished_host` plus a gap: create `View 3`, create `View 4`
+with `,.4`, delete `View 3`. That leaves three views, so `len + 1` names
+`View 4`, which exists. `gapped_host` is built twice rather than cloned because
+`App` is not `Clone`, the same reason `twin_of` exists.
+
+The injected rival is the one the ticket was blocked on: fix `panel.rs` and
+leave `resolve.rs` spelling the count. `both_ends` then fails on the document
+text before either later assertion is reached — the host writes `View 4` with
+`,.4` intact plus a new `View 5`, the client asks to replace `View 4` and
+resets it to `,.2`. That failure is the evidence that this file is where the
+one-derivation claim is witnessed, and that a test per front end would have
+passed on both sides of the divergence.

context/test-evidence/command.md

@@ -4373,3 +4373,28 @@ Fault injected: `plan.at(layout.cell_key(0, ci).as_ref())` in place of
 `layout.column_key(ci)` — the rejected version, restored. Fails with
 `left: ["14.5000", "16.5000"] right: ["14.50", "16.50"]`, which is the bug in
 one line: row order decided the spelling.
+
+---
+
+### command::cmd::panel::tests::create_and_switch_view_skips_a_name_a_surviving_view_holds
+
+improvise-vw47, the `Cmd` half. Its sibling
+`create_and_switch_view_names_incrementally` already asserted that the name
+follows the workbook rather than being a keymap literal — but every fixture it
+uses has the derived name **free**, so it could not tell `len + 1` from "the
+first unused name" and the bug lived underneath it.
+
+The fixture is the ticket's own scenario: create `View 2`, create `View 3` with
+a `,.4` format, delete `View 2`. It asserts the count is 2 *before* asking
+anything, because the whole discriminating property is that the count has
+fallen below the highest name — a fixture where `len + 1` happens to be free
+proves nothing, and that is precisely how the sibling test passed for months.
+
+Fails against the unfixed source with
+`[CreateView("View 3"), SwitchView("View 3"), ChangeMode(Normal)]` — the name
+of a view that already exists. The consequence is one level further on, in
+`effect::CreateView` → `Workbook::create_view`, which replaces: the user's
+`,.4` becomes `,.2` and no view is added. Asserting on the emitted effect
+rather than on the applied workbook keeps the test in the `Cmd` layer where a
+command is a pure function of its context; the applied consequence is asserted
+in `lockstep.rs`, where it is also held against the browser's answer.

context/test-evidence/core.md

@@ -1186,3 +1186,34 @@ pre-existing ones (`add_label_category_sets_none_axis_on_all_views`,
 `label_category_defaults_to_none_axis`) that used to be carried by the deleted
 `set_axis` correction — which is the evidence that the consolidation did not
 cost coverage.
+
+---
+
+## `view/types.rs`
+
+### view::types::tests::an_unused_view_name_steps_past_every_name_already_held
+
+improvise-vw47. `unused_view_name` is the one derivation both user-facing
+"create a view" resolvers call, and the whole of what it must get right is that
+the *count* of views is not a free *name*. Deleting `View 2` from `Default`,
+`View 2`, `View 3` leaves two views and the name `View 3` taken, so the old
+`format!("View {}", len + 1)` named a view that already existed and
+`Workbook::create_view` — which replaces, deliberately, for the `_Records` and
+`_Drill` rebuilders (`context/code/core.md` §11) — reset it to defaults.
+
+Four cases, and only the last three discriminate. `["Default"] -> "View 2"` and
+`["Default", "View 2"] -> "View 3"` pin the no-gap behaviour so the fix cannot
+be paid for by renaming the ordinary case; `["Default", "View 3"] -> "View 4"`
+is the bug; the run `["Default", "View 3", "View 4", "View 5"] -> "View 6"`
+separates "searches" from "bumps once", which a single-gap fixture cannot. The
+last case, `["Default", "Sales"] -> "View 3"`, pins that a name outside the
+`View <n>` family still counts toward the starting guess — the helper counts
+views, it does not parse names.
+
+Two rivals were injected and both fail here or one level up. Starting the
+search at `1` rather than at the count fails this test's first case **and** the
+pre-existing `create_and_switch_view_names_incrementally`: it renames the first
+view a fresh workbook creates. Making `Workbook::create_view` refuse or rename
+a name it already holds passes this test and breaks records mode, because both
+rebuilders depend on the replacement — which is why the fix is a helper beside
+`create_view` and not a change to it.

crates/improvise-command/src/command/cmd/panel.rs

@@ -480,6 +480,32 @@ mod tests {
         );
     }
 
+    /// improvise-vw47: the count is not the name. Deleting a view leaves the
+    /// obvious next name TAKEN — see `context/test-evidence/command.md`.
+    #[test]
+    fn create_and_switch_view_skips_a_name_a_surviving_view_holds() {
+        let mut m = two_cat_model();
+        m.create_view("View 2");
+        m.create_view("View 3").number_format = ",.4".to_string();
+        m.delete_view("View 2").unwrap();
+        assert_eq!(
+            m.views.len(),
+            2,
+            "the fixture must leave the count BELOW the highest name, or \
+             `len + 1` is free and the test proves nothing"
+        );
+
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let ctx = make_ctx(&m, &layout, &reg);
+        let dbg = effects_debug(&CreateAndSwitchView.execute(&ctx));
+        assert!(
+            dbg.contains("CreateView(\"View 4\")"),
+            "`View 3` is taken and `create_view` REPLACES, so naming it again \
+             resets a view the user configured, got: {dbg}"
+        );
+    }
+
     #[test]
     fn delete_view_at_cursor_zero_does_not_adjust_cursor() {
         let m = two_cat_model();
@@ -842,7 +868,7 @@ impl Cmd for CreateAndSwitchView {
         "create-and-switch-view"
     }
     fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
-        let name = format!("View {}", ctx.workbook.views.len() + 1);
+        let name = crate::view::unused_view_name(ctx.workbook.views.keys().map(String::as_str));
         vec![
             Box::new(effect::CreateView(name.clone())),
             Box::new(effect::SwitchView(name)),

crates/improvise-core/src/view/mod.rs

@@ -7,4 +7,6 @@ pub use layout::{
     AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow,
     drill_constraint, drill_snapshot, synthetic_record_info,
 };
-pub use types::{DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View};
+pub use types::{
+    DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View, unused_view_name,
+};

crates/improvise-core/src/view/types.rs

@@ -22,6 +22,16 @@ pub const RECORDS_VIEW: &str = VIRTUAL_VIEW_NAMES[0];
 /// Name of the view drill-down switches to.
 pub const DRILL_VIEW: &str = VIRTUAL_VIEW_NAMES[1];
 
+/// The `View <n>` a new user-created view takes, skipping any name already
+/// held. Why not a refusal in `create_view`: `context/code/core.md` §11.
+pub fn unused_view_name<'a>(existing: impl IntoIterator<Item = &'a str>) -> String {
+    let taken: HashSet<&str> = existing.into_iter().collect();
+    (taken.len() + 1..)
+        .map(|n| format!("View {n}"))
+        .find(|name| !taken.contains(name.as_str()))
+        .expect("a name is free below usize::MAX, there being fewer views")
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct View {
     pub name: String,
@@ -256,10 +266,28 @@ impl View {
 
 #[cfg(test)]
 mod tests {
-    use super::View;
+    use super::{View, unused_view_name};
     use crate::model::category::CategoryKind;
     use crate::view::Axis;
 
+    /// improvise-vw47: the count is the starting guess, never the answer.
+    /// What a taken name costs: `context/test-evidence/core.md`.
+    #[test]
+    fn an_unused_view_name_steps_past_every_name_already_held() {
+        assert_eq!(unused_view_name(["Default"]), "View 2");
+        assert_eq!(unused_view_name(["Default", "View 2"]), "View 3");
+
+        // The gap a delete leaves: two views, and `View 3` among them.
+        assert_eq!(unused_view_name(["Default", "View 3"]), "View 4");
+        // A run of them, so the step is a search and not a single bump.
+        assert_eq!(
+            unused_view_name(["Default", "View 3", "View 4", "View 5"]),
+            "View 6"
+        );
+        // Names that are not `View <n>` still count toward the guess.
+        assert_eq!(unused_view_name(["Default", "Sales"]), "View 3");
+    }
+
     fn view_with_cats(cats: &[&str]) -> View {
         let mut v = View::new("Test");
         for &c in cats {

crates/improvise-protocol/src/resolve.rs

@@ -398,7 +398,9 @@ fn composed(
             Some(switch_view_at(state, state.view.view_panel_cursor).unwrap_or_default())
         }
         "create-and-switch-view" => {
-            let name = format!("View {}", state.views.views.len() + 1);
+            let name = improvise_core::view::unused_view_name(
+                state.views.views.iter().map(|v| v.name.as_str()),
+            );
             Some(vec![
                 model(state, ModelCommand::CreateView(name.clone()))?,
                 Command::SwitchView(name),

crates/improvise-wasm-client/tests/lockstep.rs

@@ -879,6 +879,54 @@ fn the_view_panel_bindings_agree_with_the_host() {
     );
 }
 
+/// `furnished_host` with a GAP: `View 3` created, configured `View 4` created,
+/// `View 3` deleted. So `views.len() + 1` names a view that already exists.
+fn gapped_host() -> App {
+    let mut app = furnished_host();
+    let wb = &mut app.model_state.workbook;
+    wb.create_view("View 3");
+    wb.create_view("View 4").number_format = ",.4".to_string();
+    wb.delete_view("View 3").unwrap();
+    app.rebuild_layout();
+    app
+}
+
+/// improvise-vw47: the new view's name is derived on BOTH ends, so the two
+/// must agree AND both be right. Why a gap is the fixture: the evidence file.
+#[test]
+fn a_new_view_named_over_a_gap_agrees_with_the_host_and_spares_the_survivor() {
+    let mut app = gapped_host();
+    app.view_state.mode = AppMode::ViewPanel;
+    let mut twin = gapped_host();
+    twin.view_state = app.view_state.clone();
+    twin.rebuild_layout();
+    let mut client = furnished_client(&app, (2, 1));
+
+    let names: Vec<String> = app.model_state.workbook.views.keys().cloned().collect();
+    assert!(
+        app.model_state.workbook.views.len() == 3
+            && app.model_state.workbook.views.contains_key("View 4"),
+        "the fixture must leave `len + 1` TAKEN, or a wrong derivation is \
+         free to be right by accident: {names:?}"
+    );
+
+    // The agreement. A fix applied to one resolver and not the other diverges
+    // here, on the document text, before either assertion below is reached.
+    both_ends(&mut app, &mut twin, &mut client, "n");
+
+    let names: Vec<String> = app.model_state.workbook.views.keys().cloned().collect();
+    assert_eq!(
+        app.model_state.workbook.views.len(),
+        4,
+        "`n` must ADD a view; naming one that exists makes `create_view` \
+         replace it instead: {names:?}"
+    );
+    assert_eq!(
+        app.model_state.workbook.views["View 4"].number_format, ",.4",
+        "and the view the user configured must be untouched"
+    );
+}
+
 /// improvise-cfly: the wave shipped `Command::ViewBack` with no key reaching
 /// it. Both arms of `<`, against the host — see the evidence file.
 #[test]