From 8e9842b89f4edac02e95a346a6681df0a28114b3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 03:53:37 -0700 Subject: [PATCH] fix(cursor): the frame basis follows active_view, not the next geometry effect (improvise-9c9o) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #78 round 2's `was == self.view_name()` guard fixed a spurious invalidation and introduced its mirror: the first geometry effect after a mid-batch `SwitchView` skipped the check entirely, because there was no basis for the new view yet, so the code re-based and skipped. Under-reporting, which gotcha 35 says is the expensive side — a stale cursor has nothing in the stream to correct it. The re-base now happens at the switch, in the loop, as soon as the name moves, so by the time the geometry arm runs the name and the basis agree by construction and the guard is gone as unreachable. `SwitchView` stays `ViewOnly`: the basis is `App`'s own bookkeeping and none of it is on the wire, so this is not the protocol change the ticket feared. Also deletes a fixture line in the sibling test that was a complete no-op — `views["Second"].clone_into(&mut views["Default"].clone())` clones into a temporary that is dropped — and replaces it with the assertion it was pretending to be. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 40 +++++++++++-- context/test-evidence/command.md | 48 ++++++++++++++++ crates/improvise-command/src/ui/app.rs | 78 ++++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 10 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index cc39d37e..6d96b619 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -704,6 +704,32 @@ snapped a successful `:import` straight back out of the wizard, while answering "yes" would have stranded a failed one in `CommandMode`. Reading the mode here, after the batch has run, needs no prediction. That is gotcha 31. +### `App::normal_mode` — what `Normal` means on this grid (improvise-sihr) + +`AppMode::Normal` and `AppMode::RecordsNormal` are the same mode over two +layouts, and every minibuffer mode opened from a records grid used to exit to +the first: the exit was a constant, spelled in `TextEntry::new(.., "normal")` +and again in each commit command, and nothing carried where the user came from. +Three arms — `:`, `Ctrl-E` and `Ctrl-W` — and the class predates the last of +them, which is why Copilot's finding against `SavePrompt` alone was declined on +PR #27. + +**It is resolved, not carried.** `ChangeMode` maps `Normal` through this +function, and `ReturnToNormalUnlessMoved` reads it too; `CloseWizard` writes the +field directly and reads the same function. Those are the only three places a +mode becomes `Normal`, so there is no exit to forget and the keymap spells none. +The ticket proposed the opposite — an exit mode beside `MinibufferConfig` — and +it was declined for a reason beyond doctrine: carried generically it also +returns `CategoryAdd` to `CategoryPanel`, which nobody asked for, and carried +for three modes it is the one-arm shape the ticket itself argues against. + +**It reads `App::layout`, which is the batch's *opening* layout** (gotcha 23), +and that is the property rather than a defect: "the grid the user was looking at +when the batch started" is exactly the entry mode. `:toggle-records-mode` is the +case that proves it — leaving records, the head `ReturnToNormalUnlessMoved` sees +a records layout and sets `RecordsNormal`, then `ViewBack` restores the frame's +own mode and the batch ends in `Normal` on the pivot view, unchanged. + ### `SetBuffer` — "search" is not a buffer (improvise-gdl) Every name goes into the buffers map. The search query is not a buffer — it has @@ -1123,10 +1149,16 @@ and the structural cases are already covered by `view_shapes`; the **`CellData`* arm pays for every view. Measured: **0%** on the held-key path, **+5.7%** on an 18k-cell two-view `SetCell`, scaling linearly in view count. -The frame basis is **keyed by view name**. `SwitchView` is `ViewOnly`, so it does -not re-base, and a batch that switches and then runs a model effect was comparing -the new view's hash against the old view's — a near-certain mismatch, so a -spurious reset. It re-bases rather than compares when the name moves. +The frame basis is **keyed by view name**, and **it follows `active_view`**. A +batch that switches and then runs a model effect was comparing the new view's +hash against the old view's — a near-certain mismatch, so a spurious reset. The +first fix guarded the *comparison* on the names agreeing, which exempted the +first geometry effect after a mid-batch switch from the check altogether +(improvise-9c9o): under-reporting, gotcha 35's expensive side. So the re-base +happens at the **switch** — in the loop, as soon as the name moves — and by the +time the geometry arm runs the name and the basis agree by construction. Note +what this did *not* need: `SwitchView` stays `ViewOnly`, because the basis is +`App`'s own bookkeeping and none of it is on the wire. **And writing the test for all this found the bug that was hiding it.** `project::grid_in` built a non-active view with `GridLayout::new`, which does not diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index cf5296b1..5169b3a1 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3260,6 +3260,54 @@ The basis is now keyed by view name and re-bases rather than comparing when the name moves. Fault injected: dropping the `was == self.view_name()` guard fails this test, with `cursor_epoch` moving for a batch that reframed nothing. +improvise-9c9o moved *where* the re-base happens, and this test was unchanged by +it because it names the property, not the mechanism — the re-base now fires on +the switch rather than on the next geometry effect, and a batch that reframes +nothing still invalidates nothing. + +Its fixture carried a line that did nothing at all: +`views["Second"].clone_into(&mut views["Default"].clone())` clones into a +temporary that is immediately dropped. It read as the thing making the two views +differ, so had `create_view` ever started copying the active view's config this +test would have gone vacuous with that line standing there looking like it +prevented exactly that. It is deleted and replaced by the assertion it was +pretending to be: the two views' `view_frame` hashes really differ. + +### a_geometry_effect_after_a_mid_batch_switch_still_invalidates_the_cursor + +The other side of the guard above, and introduced by it. PR #78 round 1 compared +frame hashes across a view change, which threw a cursor away spuriously; round 2 +added `was == self.view_name()`, which fixed that and also **exempted the first +geometry effect after a mid-batch `SwitchView` from the check altogether** — +there is no basis for the new view yet, so the code re-based and skipped. +Passes on pre-#78 `main` and on round-1 head; fails at the merged head. + +Direction is why this is worth more than the bug it replaced. Round 1 kept a +cursor it should have thrown away; round 2 threw away nothing it should have +kept — which is gotcha 35's expensive side, since a stale cursor has nothing in +the stream to correct it where a spurious invalidation costs one projection. + +The fix is the ticket's: **the basis follows `active_view`**, re-based in the +loop as soon as the name moves, rather than at the next geometry effect. It did +**not** need `SwitchView` to stop being `ViewOnly` and it is not a protocol +change — the basis is `App`'s own bookkeeping, and nothing about it is on the +wire. The `was == self.view_name()` guard is gone from the comparison because it +is now unreachable: by the time the geometry arm runs, the name and the basis +agree by construction. + +What it changes elsewhere, and why that is fine: `DrillIntoCell` and +`EnterRecordsMode` both emit `[CreateView, SwitchView, SetAxis, SetAxis, …]`, so +they now reset the cursor on the *first* `SetAxis` where they used to skip it and +reset on the second. Same outcome, one effect earlier. `DeleteView` is the one +`LayoutGeometry` effect that moves `active_view` itself; the re-base fires for it +too, so the geometry arm compares the new view against itself and resets nothing +— which is what the old skip did. + +The fixture measures the row count the transpose must move rather than spelling +it: a first version asserted `row_count() == 1` and the real answer is 2, which +is the shape of a precondition that would have gone quietly wrong later. It +fails on the unfixed tree at the epoch assertion, `left: 0 right: 0`. + ### a_frozen_drill_does_not_take_its_opening_frame_from_the_held_layout improvise-mcq4's fix reads the batch's opening frame off `App::layout` rather than diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b60013b7..859b4e40 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -775,15 +775,20 @@ impl App { self.model_state.dirty |= now != basis; config_basis = Some(now); } + // The basis follows `active_view`: a switch re-bases HERE, so the next + // geometry effect compares (improvise-9c9o, `context/code/command.md`). + if let Some((was, _)) = frame_basis.as_ref() + && was != self.view_name() + { + frame_basis = Some((self.view_name().to_string(), self.active_view_frame())); + } // Inside the loop, or a batch that reframes and then places the // cursor loses the placement — `context/code/command.md`. - if let Some((was, basis)) = frame_basis.as_ref() + if let Some((_, basis)) = frame_basis.as_ref() && matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) { let now = self.active_view_frame(); - // A `SwitchView` earlier in the batch re-bases rather than compares: - // two views' hashes differ for no reason (improvise-mcq4's sibling). - if was == self.view_name() && now != *basis { + if now != *basis { self.reset_cursor(); self.last_reframed_views .insert(self.view_name().to_string()); @@ -1336,9 +1341,15 @@ mod tests { fn a_switch_then_a_geometry_effect_compares_within_one_view() { let mut app = paged_model(false); app.model_state.workbook.create_view("Second"); - app.model_state.workbook.views["Second"] - .clone_into(&mut app.model_state.workbook.views["Default"].clone()); app.rebuild_layout(); + let wb = &app.model_state.workbook; + assert_ne!( + super::view_frame(&wb.model, &wb.views["Default"]), + super::view_frame(&wb.model, &wb.views["Second"]), + "the fixture only says anything if the two views really do hash \ + differently — a `create_view` that copied the active view's config \ + would make this test vacuous" + ); let epoch = app.cursor_epoch(); // Switch to a view whose frame differs, then a geometry effect that @@ -1361,6 +1372,61 @@ mod tests { ); } + /// BUG (improvise-9c9o): the first geometry effect after a mid-batch switch + /// skipped the check entirely. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_geometry_effect_after_a_mid_batch_switch_still_invalidates_the_cursor() { + use crate::view::Axis; + fn two_view_app() -> App { + let mut app = paged_model(false); + let second = app.model_state.workbook.create_view("Second"); + second.set_axis("Region", Axis::Row); + second.set_axis("_Measure", Axis::Column); + app.rebuild_layout(); + app + } + // What the switch alone leaves, so the row count the transpose must move + // is measured rather than spelled. + let mut probe = two_view_app(); + probe.apply_effects(vec![Box::new(super::super::effect::SwitchView( + "Second".to_string(), + ))]); + let rows_before = probe.layout.row_count(); + + let mut app = two_view_app(); + app.cursor_mut().selected = (2, 0); + let epoch = app.cursor_epoch(); + + app.apply_effects(vec![ + Box::new(super::super::effect::SwitchView("Second".to_string())), + Box::new(super::super::effect::TransposeAxes), + ]); + + assert_eq!( + app.view_name(), + "Second", + "setup: the switch has to land, or the batch is a plain transpose" + ); + assert_ne!( + app.layout.row_count(), + rows_before, + "setup: the transpose has to renumber Second's rows, or there is \ + nothing for the check to catch" + ); + assert_ne!( + app.cursor_epoch(), + epoch, + "the grid renumbered under a cursor at row 2 and nothing was \ + invalidated — the basis was still keyed to the outgoing view, so \ + the check re-based and skipped" + ); + assert_eq!( + app.cursor().selected, + (0, 0), + "the host's own cursor is stale" + ); + } + /// BUG (improvise-7kvw): pruning depends on the page, so a page flip renumbers /// the grid and left both the host's cursor and every client's stale. #[test]