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

fix(cursor): the frame basis follows active_view, not the next geometry effect (improvise-9c9o)

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-21 03:53:37 -07:00
parent 6782e8555c
commit 8e9842b89f
3 changed files with 156 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/command.md
+36 -4
View File
@@ -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
context/test-evidence/command.md
+48
View File
@@ -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
crates/improvise-command/src/ui/app.rs
+72 -6
View File
@@ -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]