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

Merge pull request #48 from fiddlerwoaroof/wave/e-active-view
CI / Test and lint (host) (push) Has been cancelled
Details
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
Details
CI / Knowledge base consistency (push) Has been cancelled
Details
CI / Browser harness (OPFS contract + shell) (push) Has been cancelled
Details
CI / Browser bundle + static site (push) Has been cancelled
Details
CI / Deploy to GitHub Pages (push) Has been cancelled
Details

Browse Source 
Split active_view: the document's default vs each client's (jytm, 6c6q, k69y)
This commit is contained in:
Ed Langley
2026-09-20 11:49:41 -07:00
committed by GitHub
parent 2c92e1ce6f f096659b62
commit c59df9f406
12 changed files with 961 additions and 436 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/command.md
+168 -46
View File
@@ -275,29 +275,29 @@ nobody had to obtain.
### `apply_command`
`Command::SwitchView` does not go through `reduce_view` alone: the document's
"which view is active" and the client's must move together, and
`App::switch_to_view` is one of the only two writers allowed to touch either
(gotcha 29). It reaches that writer the way every other mutation here does —
through its effect — so the view history a switch records is the same on both
doors. Everything else is pure view slice.
`Command::SwitchView` does not go through `reduce_view` alone, but not for the
reason it once did. Since improvise-jytm the document's copy is the *default* — what
a fresh client opens — and the client's slice is authoritative, so a switch moves no
document state at all. What it still has that `reduce_view` has not is the **view
history**: `effect::SwitchView` pushes the frame `effect::ViewBack` pops, and that
stack is this client's. Everything else is pure view slice.
The `SwitchView` arm specifically: the mutation is `effect::SwitchView`'s, not a
second copy of it. The effect records the view history the key path leaves
behind, and `ModelCommand::ViewBack` pops exactly that stack. Switching by
behind, and `effect::ViewBack` pops exactly that stack. Switching by
calling `App::switch_to_view` here instead left the stack empty, so a wire client
could not go back where a keyboard one could — see
`a_wire_view_switch_records_history_the_way_a_key_switch_does`. `apply_effects`
rebuilds the layout.
The `Dirty` that arm returns is deliberately **narrower** than the
`Dirty::of(Model(LayoutGeometry))` the call returns. That table is a per-*kind*
over-approximation — sound for the kind because `CreateView` and `DeleteView`
wear it too — and a view switch is the one member that leaves the view list
alone: its names and axes belong to the document, and which view is active is not
in `ViewListCache` at all (improvise-pca). Over-projecting costs bandwidth, never
staleness, so narrowing here is safe; widening `Dirty::of` for this one member
would not be.
The `Dirty` that arm returns is **not** `Dirty::of` of anything, and since
improvise-jytm that is the point rather than a narrowing. `effect::SwitchView` is
`ViewOnly`, so its `Dirty` is `NOTHING`; what this arm returns is what the *host's
own* widgets must repaint, which is a different question from what the document
changed. The view list is left out for the old reason (improvise-pca): its names and
axes belong to the document, and which view is active is not in `ViewListCache` at
all. A connected client never reaches this arm — `Sessions::apply_switch` moves that
client's mirror and `ClientSession::project` observes the move.
An inbound `Command::Project` is ignored. Projections travel server→client; a
client that sends one is describing state it does not own.
@@ -906,7 +906,7 @@ Read off the effects that carry each kind, not guessed:
- `ModelChange::CellData` is worn by `AddItem`, `RemoveItem`, `AddFormula` and
`RemoveFormula` as well as `SetCell`, so it reaches the category panel and the
formula panel, but no view's axes move under it.
- `ModelChange::LayoutGeometry` is worn by `SetAxis`, `CreateView`, `SwitchView` —
- `ModelChange::LayoutGeometry` is worn by `SetAxis`, `CreateView`, `DeleteView` —
and by `AddCategory`/`RemoveCategory`, which rewrite every view's axis map
(gotcha 3) — so it reaches every axis-bearing widget *and* the formula list,
because `Model::remove_category` ends by dropping every formula that targets the
@@ -1139,9 +1139,12 @@ Making it a field of the thing being sent is the same move as `Applied` being an
enum: a host must name the audience to get at the command
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`).
- `ToOthers` — every client **except** the one that asked. It is a *correction* of
state the asker already folded optimistically, so sending it back would
double-apply a move that client has already made.
- `To(SessionId, _)` — one named client. A *correction* of something only that
client's UI holds, which no other client is affected by. It has two cases, and
they are why it names a session rather than "the asker": a switch to a view the
document has not got, which is the asker (improvise-6c6q), and a view deleted out
from under whoever was on it, which is not. `ToAll` would double-apply the first
on the accepted path and move views nobody asked to move in the second.
- `ToAll` — every client, the asker included. Not a correction but a
**consequence**: the document did something that invalidates a fact each client
holds privately, and the asker holds that fact too. A `Command::Model` is never
@@ -1247,12 +1250,28 @@ happened, if it happened, before the batch was built.
### `apply_switch`
The document half — and it is addressed to no one session, which is why it takes no
`SessionId`. The workbook refuses a switch to a view it does not have, so what every
mirror takes is what was *accepted*, not what was asked, and the broadcast names
the accepted view for the same reason: a client that optimistically folded a name
the document turned down is corrected by the same message that tells the others to
follow.
Which view a client is on is **that client's** (improvise-jytm, improvise-g0f): the
model is shared and the UI is not. So this takes a `SessionId` — it is addressed to
exactly one session, which is the opposite of what it once was — moves only that
session's mirror, and dirties nothing.
The one thing it still asks the document is whether the view exists. A slice keys
`ViewState::cursors` and is read by `grid_in`, so a mirror naming a view the
document lacks would index nothing; what is adopted is therefore what was
*accepted*, which on refusal is the view the session was already on.
The audience follows from that, and it is conditional. On the accepted path the
asker folded the right thing and nobody else is affected, so **nothing is relayed**.
On the refused path the asker — and only the asker — folded a name the document
turned down, so it gets `Broadcast::To(id, …)`. `ToOthers` was the pre-jytm shape and
had the mirror-image hole: it told every client but the one that was wrong. A blanket
`ToAll` is equally wrong here, and for a reason that survives jytm: the asker really
did fold, so `ToAll` double-applies on the common path.
Note what is **not** here any more. `adopt_active_view` pushed the accepted view into
every mirror; under a per-client active view that is precisely the bug, so it is
gone along with the `Broadcast::ToOthers` variant it was the only producer of. A
mechanism whose reason has gone is deleted rather than left.
### `apply_model` (improvise-kmgs)
@@ -1260,19 +1279,39 @@ It goes through the same `apply_command`, which runs the effect the command was
derived from and reads the `Dirty` off that effect's own `EffectKind` — there is no
second classification here that could disagree with gotcha 35's table.
It takes no `SessionId` for the same reason `apply_switch` does not: a document
change is addressed to no one session. It also owes the same lockstep, because some
model mutations move the active view — `ViewBack`, `ViewForward` and a `LoadModel`
that installs a different document all end in `App::switch_to_view` (gotchas 29,
36). The broadcast is conditional where `apply_switch`'s is not: no client
optimistically folded an active view here, so there is nothing to correct unless
the name actually moved.
It takes no `SessionId`, and since improvise-jytm that is the *whole* distinction
from `apply_switch`: a document change is addressed to no one session, a view switch
to exactly one. No model mutation moves anybody's active view any more —
`ViewBack`/`ViewForward` left `ModelCommand` with jytm, because the stack they pop is
one client's history (the `ApplyAndClearDrill` argument, gotcha 38), and a
`LoadModel` that installs a different document bumps the generation and so lands in
the rebase arm, where every client gets a whole fresh slice.
Its *audience* differs for the same reason, and the two facts are one fact: because
nothing was folded optimistically, the asker is not a client being corrected but
one being told, exactly like the others — so it is `Broadcast::ToAll`, as
`adopt_cursor_epoch`'s reset is. `ToOthers` here left the asker's own slice naming
the outgoing view while the call had already moved its mirror to the accepted one.
Two things are left to relay. `adopt_cursor_epoch`'s reset is `Broadcast::ToAll`:
nothing was folded optimistically, so the asker is not a client being corrected but
one being told, exactly like the others. And `adopt_deleted_views` is the case a
per-client active view creates — see below.
### `adopt_deleted_views` (improvise-jytm)
A `DeleteView` can take away the view a *different* client is sitting on. Its mirror
then names nothing, `App::resolve_view` falls back to the document's default, and
that client is projected the default's grid while its own slice still names the
view that is gone — the server and the client disagreeing about which view keys the
cursor map, which is gotcha 24's mismatch arriving through somebody else's keystroke.
Before the split this could not happen: `Workbook::delete_view` moved the document's
active view and `adopt_active_view` pushed the result into every mirror. The split
removed both, so the case has to be answered on its own.
It is **observed, not predicted** — the check is "does this mirror still name a view
the document has", not "was this command a `DeleteView`" — for the reason
`cursor_epoch` beats matching on `CycleAxis` (gotcha 24) and `document_generation`
beats matching on `LoadModel` (gotcha 39): a table of which commands can remove a
view would be a second description of `Workbook`, free to disagree with it.
Each affected client is told with `Broadcast::To`, and the others are not: a client
on another view lost nothing.
The basis was answered for by `Checked::against` before the dispatch that reached
here. That it is a *type* and not a line at the top of the function is what stops a
@@ -1290,13 +1329,6 @@ all. So the question asked is the decisive one — did the generation move
instruction, for the same reason `apply_switch` adopts the accepted view itself:
**the only enforceable version of "you must also do X" is doing X.**
### `adopt_active_view`
Point every mirror at the view the document *accepted*, and say which that is. Not
the one that was asked for: the workbook refuses a switch to a view it does not
have, and a mirror told the asked-for name would be read under a key the document
never adopted (`doctrine(lockstep_binds_every_session_not_the_asker)`).
### `adopt_cursor_epoch` (improvise-gvnb)
If the batch renumbered the grid, invalidate every mirror's cursor and say so on the
@@ -1353,8 +1385,9 @@ running the binary.
asked for one `App` per *connection*, "no shared authoritative state across sessions
yet". That premise no longer holds (`doctrine(check_the_ticket_premise)`):
`Sessions` exists precisely to hold many `ClientSession`s against one authoritative
`App`, `apply_switch`/`apply_model` move every mirror in lockstep rather than the
asker's (`doctrine(lockstep_binds_every_session_not_the_asker)`), and
`App`, `apply_model` invalidates every mirror's cursor rather than the asker's
(`doctrine(lockstep_binds_every_session_not_the_asker)`, now scoped to the *document*
half since improvise-jytm made the active view per-client), and
`Applied::Diff::broadcast` exists only because other clients must hear what this one
did. An `App` per connection would make all of that dead code and give two browser
tabs on the same document two documents.
@@ -1992,3 +2025,92 @@ trip fail on the advertised value.
The seed is an argument rather than a clock read for the reason the CLI fixes its own
default: two fits of the same document should agree, and a user who wants a different
search asks for one (gotcha 37b).
## The active view is split (improvise-jytm)
`Workbook::active_view` is the document's **default** — what a freshly connected
client opens, and what `.improv` writes as `Initial View`. `ViewState::active_view`
is authoritative for the client that holds it. Two clients may sit on different
views of one document: the model is shared and the UI is not (improvise-g0f, user
ruling 2026-09-19).
The old guard, `the_two_active_view_names_never_diverge`, is *replaced* rather than
deleted, by `the_documents_default_view_outlives_this_clients_switches` and by
`a_fresh_session_opens_the_documents_default_view` — a deliberate split with no
guard is how this silently regresses.
### What the split moved
`App::switch_to_view` writes only `view_state.active_view`, and only for a view the
document has. **`App::resolve_view` is the one answer** to "what does this name name
in this document", falling back to the default so a slice naming a deleted view
cannot panic in `rebuild_layout`; `view()`, `view_mut()`, `view_name()` and
`project::grid_in` all go through it rather than each testing `views.contains_key`.
Two copies of that fallback would be a derivation done twice — they would agree
until one of them learned about `_Drill`. Every effect that used to write `workbook.active_view_mut()`
now writes `app.view_mut()` — the *View* is still document state shared by everyone
on it; what changed is which one this client means.
`effect::SwitchView`, `effect::ViewBack` and `effect::ViewForward` are therefore
`EffectKind::ViewOnly`. Gotcha 3 says a mis-declared kind is silent, so this was
re-derived rather than assumed: after the split each of the three writes
`view_state` and nothing else, and a `ViewOnly` effect is exactly one that reaches
no other client. **A sound derivation is not a guard**, though, and review found that
only `SwitchView` had one — putting either sibling back to `Model(LayoutGeometry)` was
invisible across the whole suite. `walking_the_view_history_moves_no_document_state`
closes that, and it asserts the *consequences* rather than the declaration because
those are what a reader can price: a mis-declared view-back raises `model_state.dirty`
(so the browser worker rewrites the whole OPFS document, gotchas 41 and 45) and bumps
`layout_version` (so every other client's next `Command::Model` is refused
`LayoutMoved`, ADR 001) for a move only one client made. That re-classification is also what takes `ViewBack`/`ViewForward`
off the wire, since `ModelCommand` is derived from the `Model(..)` effects — and it
is the right answer for a second reason, the one that removed `ApplyAndClearDrill`:
their operand is `ViewState::view_back_stack`, session-local residue belonging to
whoever navigated (gotchas 30, 38).
### What projects per client
Every projection is view-dependent except the formula list and the view list, so
`grid_in`, `category_tree_in` and `tile_bar_in` take the view and
`ClientSession::project_diff` passes its own. **All three need guarding, not just the
grid**: review found that only `grid_in` had a check, and pointing
`ClientSession::view_in` at the host's view left the whole suite green. The one that
matters most is the tile bar — a category's axis is what that widget *is*, so a client
on another view would be shown the wrong axis in the control it reassigns axes with,
beside a grid that was right. `grid_in` reuses `App::layout` when
the session names the view the host is on — which is the common case and the only
one that can carry a drill snapshot — and derives a `GridLayout` otherwise.
A session on a view that is *deleted* is the other half, and it is answered in
`adopt_deleted_views` rather than here: `project` can only describe a view, it
cannot move a client off one.
**Testing any of this needs a bystander that can be moved.** Both the refused-switch
and the deleted-view tests first sat their bystander on the document default, where a
relay addressed to everybody folds to a no-op — so the assertion read as coverage and
caught nothing, which review demonstrated by injecting `ToAll` twice. Both now put the
bystander on a view it can be moved *off*, and both read the audience off the
`Broadcast` itself rather than inferring it from folded state. The audience is the
message; that is what makes it the thing to assert.
A switch is `ViewOnly`, so `Dirty` cannot carry it. `ClientSession` therefore
**observes** its own move: `projected_view` records the view the mirror's grid was
built from, and a mismatch projects `Dirty::EVERYTHING` for that session alone.
This is the `cursor_epoch` shape (gotcha 24) — watch the state move rather than keep
a second table of which commands move it.
### Two gaps this opens, both filed rather than smuggled
**A view-scoped `ModelCommand` names no view** (improvise-lltt), and it is deferred
on a *condition*, not a judgement: it is unreachable only because no shipped client
emits `Command::SwitchView`, while the browser already emits `TransposeAxes`. `SetAxis`, `CycleAxis`,
`TogglePruneEmpty`, `SetNumberFormat` and their siblings act on "the active view",
which is now ambiguous: `apply_model` runs them against the `App`'s own slice, so a
*connected* client on another view moves the host's view rather than its own. That
was consistent before the split, because every client was on the same view. The
honest fix is for those variants to name their view, which is a protocol change.
**`Initial View` no longer follows the TUI user** (improvise-azhb). A switch moves no document state,
so opening a document, switching view and saving writes the view it was loaded with.
Whether a host's own switch should also set the document's default is a product
question, not something to decide inside this ticket.
context/test-evidence/clients.md
+12 -5
View File
@@ -91,13 +91,20 @@ an `open_all` that failed on every second file would pass the collision test.
on the *same* document and the assertion must flip. Without it, an isolation
test that passed because nobody is ever projected anything looks identical.
### the_other_document_has_the_same_view_name_to_fold
### the_other_document_would_also_have_a_cursor_to_invalidate
`doctrine(check_the_checker)` for `a_broadcast_does_not_cross_a_document_boundary`.
The fault to rule out is the *other* document not having a view of that name —
then a server-wide broadcast would be rejected over there for the wrong reason
and the isolation test would pass without proving isolation. Both fixtures carry
`Second`, so `beta`'s silence is the routing and nothing else.
The fault to rule out is the other document being *incapable* of producing the
broadcast at all — then a server-wide relay would be silent over there for the
wrong reason and the isolation test would pass without proving isolation. So the
checker drives the same `TransposeAxes` against `beta` and asserts it does produce
a `ResetCursor` there; `beta`'s silence in the isolation test is the routing and
nothing else.
Both tests moved from `Command::SwitchView` to `ModelCommand::TransposeAxes` with
improvise-jytm: a view switch is now addressed to one session and relays nothing,
so it can no longer carry an isolation test. `ResetCursor` is the surviving
`Broadcast::ToAll`.
### a_load_that_fails_rebases_nobody
context/test-evidence/command.md
+236 -85
View File
@@ -26,17 +26,23 @@ whose subject *is* staleness call `Checked::against` themselves and assert on th
`Rejection`, so neither kind can pass by accident through the other's
convenience.
### switch_view_moves_the_document_and_the_client_together
### switch_view_moves_the_client_and_leaves_the_documents_default
The two copies of "which view is active" must move together (gotcha 29). Folding
`SwitchView` through `reduce_view` alone would move the client's and leave the
document's behind, and the cursor map would then be read under the wrong key.
The successor to `switch_view_moves_the_document_and_the_client_together`, which
asserted the pre-improvise-jytm lockstep. Which view a client is on is now that
client's; `Workbook::active_view` is the *default* a fresh client opens and what
`.improv` writes. The fault it rules out is the old behaviour returning by inertia
— any writer that moves the workbook's copy from a client's switch fails it.
Fault injected 2026-09-20: restoring `App::switch_to_view`'s
`workbook.switch_view(name)` call fails this and
`the_documents_default_view_outlives_this_clients_switches`.
### a_wire_view_switch_records_history_the_way_a_key_switch_does
A wire-driven view switch owes the same history a key-driven one leaves behind.
`ModelCommand::ViewBack` pops `view_state.view_back_stack`, and the only thing
`effect::ViewBack` pops `view_state.view_back_stack`, and the only thing
that ever pushes onto it is `effect::SwitchView`. While `apply_command` performed
the switch itself — calling `App::switch_to_view` directly rather than running
the effect — the stack stayed empty, so a client that switched over the wire
@@ -44,11 +50,14 @@ could not go back, though the same two keypresses could. Wave 4 is what made tha
reachable: before `ModelCommand` there was no wire way to ask.
Nothing else in the suite could see it.
`switch_view_moves_the_document_and_the_client_together` checks where a switch
*landed*; this checks what it *recorded*.
`switch_view_moves_the_client_and_leaves_the_documents_default` checks where a
switch *landed*; this checks what it *recorded*.
Bug demonstrated 2026-09-19 (Copilot, PR #11): the `ViewBack` left the document
on "Second".
on "Second". Since improvise-jytm the step back is driven through `effect::ViewBack`
rather than `ModelCommand::ViewBack`, which left the wire with that ticket — the
stack it pops is one client's history, so a nullary wire command could not address
it (gotcha 38). What the test asserts is unchanged.
### switching_views_projects_the_axis_widgets_only
@@ -234,18 +243,22 @@ and it has to live there: a view-slice `Command` never reaches `apply_effects` a
all (`apply_command` folds it straight into `ViewState`), so a negative written
against this door would pass however unconditional the raise was.
### a_wire_view_switch_marks_the_document_dirty
### a_wire_view_switch_leaves_the_document_clean
`SwitchView` is the one command with a foot in both halves, and it is the reason
this matters beyond cell edits: `Workbook::active_view` persists as the
`Initial View` line, so a switch really does put the document out of step with
the file.
The inversion of `a_wire_view_switch_marks_the_document_dirty`, which asserted the
pre-improvise-jytm behaviour. A switch used to move `Workbook::active_view`, which
persists as the `Initial View` line, so it really did put the document out of step
with the file. It no longer moves any document state, so there is nothing to
persist and the browser worker's persist trigger (improvise-gv9r) must not fire.
It is tested on its own because the browser worker's persist trigger now depends
on it (improvise-gv9r): the worker used to name `SwitchView` explicitly in a
table of variants and now asks the document. If this effect were ever
reclassified `ViewOnly`, the worker would silently stop writing view switches
back.
The direction matters: the worker asks the document's `dirty` flag rather than a
table of variants, so a switch that dirtied spuriously would cost an OPFS rewrite
of the whole document per keystroke (gotcha 41). This is the negative arm — the
positive one, that a real model command *does* dirty, is the loop above it.
The consequence to keep in view is a product one, filed as improvise-azhb rather
than decided here: a TUI user who switches view and saves now writes the
`Initial View` the document was loaded with.
---
@@ -422,12 +435,24 @@ Populated with non-default values in every field family — a mode carrying a
prefix — because a roundtrip of `ViewState::default()` would pass while proving
nothing.
### the_two_active_view_names_never_diverge
### the_documents_default_view_outlives_this_clients_switches
improvise-cqi: "which view is active" is stored twice — on the `Workbook` (the
document's) and on `ViewState` (the client's, which keys the cursor map). Every
path that changes one must change the other, or the cursor map is read under the
wrong key. `App` owns both writers: `switch_to_view` and `install_workbook`.
The replacement for `the_two_active_view_names_never_diverge`, and it is a
replacement rather than a deletion on purpose: improvise-jytm makes the two copies
diverge deliberately, and a deliberate split with no guard is how one silently
regresses into the other.
What it pins, all four of which have a silent failure mode. A switch moves the
client's copy and **not** the document's default. `App::view()` — what
`rebuild_layout` and `cmd_context` read — follows the client, so the split is
observable in the layout and not merely in a string. A switch to a view the
document does not have moves nothing, because the slice keys `ViewState::cursors`
and `grid_in` indexes the workbook by it. And `install_workbook` still seats this
client on the incoming document's default and clears the cursor map, since the
outgoing document's view names mean nothing in the new one (gotcha 24).
Fault injected 2026-09-20: `App::switch_to_view` calling
`workbook.switch_view(name)` again fails the second assertion.
### moving_the_cursor_does_not_touch_the_shared_workbook
@@ -987,12 +1012,22 @@ and the `CellId`s **valid and pointing at the other record**.
### a_session_that_switches_views_is_projected_the_view_it_switched_to
`ClientSession::apply` folded straight into the session's mirror, so a
`Command::SwitchView` moved the mirrored `active_view` while the workbook's stayed
put (gotcha 29). `viewport()` then read the cursor under the new view's key and the
next `project()` built the grid from the *other* view — the switching client was
sent rows it had not asked for and could never be corrected, because nothing
re-synced the pair.
The original bug: `ClientSession::apply` folded straight into the session's mirror,
so a `Command::SwitchView` moved the mirrored `active_view` while the workbook's
stayed put (gotcha 29). `viewport()` then read the cursor under the new view's key
and the next `project()` built the grid from the *other* view.
improvise-jytm answers it the other way round — the mirror is now authoritative and
`grid_in` is built from it — so the assertions moved with it: the asker's mirror
names "Second", the document's default is still "Default", and the rows it is sent
are the ones "Second" has. The fixture's two views are transposes of each other, so
"was I sent the right grid" cannot be answered by accident.
Two faults injected 2026-09-20, both of which fail it. Pointing `grid_in` at
`app.view_name()` regardless of the session reproduces the original defect exactly.
Removing `ClientSession`'s `projected_view` observation is the *new* way to
reproduce it: a switch is `ViewOnly`, so nothing in `Dirty` carries it and the
session must notice its own move or never rebuild its grid at all.
#### Helper — `app_with_two_views`
@@ -1001,17 +1036,151 @@ Not a test — the fixture for every lockstep and rebase test in this file.
A second view whose axes are the first one's transposed, so the two grids cannot be
confused for each other: "Default" rows are regions, "Second" rows are measures.
### a_view_switch_moves_every_sessions_mirror_not_just_the_switchers
### a_view_switch_moves_only_the_switchers_mirror
Lockstep is a property of *every* session, not of the one that asked.
`Sessions::apply` moved the workbook's active view and then moved the mirror of the
addressed session alone, so a bystander went on naming the view the document had
left. Two things followed. `viewport()` read that bystander's cursor under the
stale key while `grid_of` built the grid from the workbook's *new* active view, so
the bystander was projected the new view's rows positioned by the old view's scroll
— the cursor-vs-geometry mismatch of gotcha 24, arriving through another client.
And its own client-side slice, which folds the very same commands so that the two
"cannot drift" (`ClientSession::fold`), was never handed the switch to fold.
The successor to `a_view_switch_moves_every_sessions_mirror_not_just_the_switchers`,
which pinned the lockstep improvise-jytm removes. The model is shared and the UI is
not, so a bystander keeps its view, its scroll and its grid.
Three assertions, because the old bug can come back through three different doors:
the bystander's mirrored `active_view`, its `row_offset` (nonzero, so a mirror reset
shows as a move rather than coinciding with the origin), and whether `project_all`
names it at all. That third one is the sharpest — a bystander repainted for
somebody else's switch is invisible in the first two, since it would be repainted
with its *own* view's content and look right.
The scroll is drained through `project_all` before the switch, so the fan-out the
test reads names only what the switch caused.
Fault injected 2026-09-20: restoring the `adopt_active_view` loop (push the accepted
view into every mirror) fails it, as does removing the `projected_view` observation.
### a_refused_switch_corrects_the_asker_and_nobody_else
improvise-6c6q. `apply_switch` relayed `Broadcast::ToOthers`, so on the refused path
every client *but* the one that was wrong was told the accepted name. The asker's
own slice kept the name it had folded optimistically while its server-side mirror
held the accepted one — gotcha 24's mismatch, arriving through the asker's own
keystroke. `apply_switch`'s doc comment claimed the hole was closed, which is worse
than the hole.
**The test drives the asker's own `ViewState`**, folding the command the way a real
client would before handing it to the door. That is the whole cost of the ticket:
the sibling bug in `apply_model` shipped green because its test read a
"what do the others hear" helper, which looks away from the asker by construction.
There is deliberately no such helper in this test.
**The bystander is asserted from a view it can be moved OFF.** A first version left it
on the default, where a wrongly-addressed relay folds to a no-op — so the assertion
read as coverage and was vacuous, which review demonstrated by injecting `ToAll` and
watching this test pass. It now sits on "Second", and the audience itself is asserted
(`Broadcast::To(asker, …)`) rather than inferred from what somebody's slice folded
to: the audience **is** the message (gotcha 36), so it is the thing to read.
Two faults injected 2026-09-20, both of which fail it: relaying `ToOthers` (the
original improvise-6c6q bug) and relaying `ToAll` (the over-correction). The second
also fails `folding_the_relay_puts_a_refused_asker_back`, which reads frames rather
than folded state.
### deleting_the_view_this_client_is_on_moves_it_to_the_default
The host-side half of `deleting_a_view_moves_the_client_that_was_on_it_and_tells_it`,
and a desync that predates improvise-jytm: `Workbook::delete_view` moved the
document's active view and nothing moved `ViewState::active_view`, so the TUI's
cursor was read under a key naming a view that was gone. It was invisible while
`draw_status` read the workbook's copy; improvise-k69y makes it read the client's,
which is what turns it into a wrong badge.
The negative arm is the discriminating one: deleting a *different* view must leave
this client where it is. Without it, unconditionally seating the client on the
default would pass the positive assertion. The fixture therefore carries three
views, and the first delete is of the one nobody is on.
Fault injected 2026-09-20: dropping the `view_state.active_view` line from
`effect::DeleteView` fails it.
### deleting_a_view_moves_the_client_that_was_on_it_and_tells_it
A hole improvise-jytm opens and must therefore close. Before the split,
`Workbook::delete_view` moved the document's active view and `adopt_active_view`
pushed the result into every mirror, so deleting the view a client was on moved that
client. The split removes both, and without `adopt_deleted_views` the client is left
naming a view the document has not got: `App::resolve_view` falls back to the
default, so it is *projected* the default's grid while its slice says otherwise, and
`viewport()` reads its cursor under a key nothing will ever write again.
The fixture carries **three** views and the bystander sits on the one the delete does
not remove. That is the whole discrimination: a first version left the bystander on
the default, where a relay addressed to everybody folds to a no-op, and review
demonstrated the vacuity by replacing `Broadcast::To` with `ToAll` and watching the
test pass. The evidence entry had even *stated* the risk in one sentence and claimed
the assertion covered it in the next — the exact shape this file exists to stop.
So four assertions, over three different surfaces. The **audience** is read off the
`Broadcast` itself, because the audience is the message (gotcha 36). The client that
lost its view moved, in its mirror *and* in its own slice, the latter driven by
folding what the host relayed. And the bystander is still on "Third" — in its slice
and in its mirror — which only means something because there is somewhere for a
wrong relay to have moved it from.
The deletion itself is asserted, because `Workbook::delete_view` refuses to remove
the last view: a fixture with one view would pass every other line here while
deleting nothing.
Two faults injected 2026-09-20, both of which fail it: dropping the
`adopt_deleted_views` call from `apply_model`, and addressing its relay `ToAll`.
### a_session_on_another_view_is_projected_that_views_axes
The hole review found in improvise-jytm's first cut: only the **grid** half of
"every projection follows the session's view" was checked. `category_tree_in` and
`tile_bar_in` are reached through `ClientSession::view_in`, and replacing that
function's body with `app.view()` — the host's view — left the whole suite green.
The harm is worse than a wrong pixel. A category's axis is what the **tile bar** is
for: it is the control a user reassigns axes with. Under the fault a client on
another view sees the wrong axis for every category, in the widget it would use to
change them, while the grid beside it is correct — nothing to notice and nothing
that fails.
`app_with_two_views` is what makes two assertions enough: "Second" is "Default"
transposed, so reading the wrong view **swaps** Row and Column rather than blurring
them, and the test names both categories rather than one.
Fault injected 2026-09-20: `ClientSession::view_in` returning `app.view()` fails it.
### walking_the_view_history_moves_no_document_state
The other hole review found: of the three effects improvise-jytm reclassified to
`ViewOnly`, only `SwitchView` had a guard (`a_wire_view_switch_leaves_the_document_clean`).
Putting `ViewBack` or `ViewForward` back to `Model(LayoutGeometry)` left all 1313
tests green — which is gotcha 3's whole point: a mis-declared kind is silent.
It asserts the **consequences** rather than the declaration, because those are what a
reader can price. `App::apply_effects` raises `model_state.dirty` for any
`Model(_)`, and the browser worker persists on that flag — so a mis-declared
view-back rewrites the entire OPFS document (gotcha 41: `Storage` is whole-document)
for a move that changed nothing. And it bumps `layout_version`, so every *other*
client's next `Command::Model` is refused `LayoutMoved` (ADR 001) because somebody
pressed a navigation key.
The `assert_ne!` on the active view before each pair of assertions is the
check-the-checker: an empty history stack makes `ViewBack` a no-op, and a no-op
dirties nothing whatever its kind.
Both effects are driven, because they are separate `kind()` implementations and
review injected them separately.
### a_fresh_session_opens_the_documents_default_view
The other half of improvise-jytm's contract, and the reason `Workbook::active_view`
survives at all: it is what a freshly connected client opens. The fixture switches
an *existing* session first, so a `connect` that read any client's view rather than
the document's would be visible.
Fault injected 2026-09-20: pushing the accepted view into every mirror in
`apply_switch` moves the document's default nowhere, but making `apply_switch` write
`workbook.active_view` fails it.
#### Helper — `diff`
@@ -1025,43 +1194,15 @@ document is a loud failure here and not a silent `Dirty::NOTHING`.
orphaned paragraph ("One connected client, as its host really drives it…")
describing a type that no longer exists at that site. Dropped.
### a_model_command_that_moves_the_active_view_binds_every_mirror
The same lockstep, reached through the model half of the vocabulary
(improvise-kmgs). `ViewBack` names no view, but it ends in `App::switch_to_view`
just as `SwitchView` does, so every mirror owes the accepted name —
`doctrine(lockstep_binds_every_session_not_the_asker)` is about the value moving,
not about which command moved it.
The fixture walks forward through the *effect*, which is what fills the history
stack, and only then connects — so both mirrors start out naming "Second" and the
step back is something they have to be told about.
### a_model_commands_view_switch_reaches_the_asker_too
The asker's own slice owes the switch too, and `ToOthers` never gave it one.
`apply_switch` is right to relay `ToOthers`: a client that typed
`Command::SwitchView` folded it optimistically, so sending it back would re-apply a
move it has already made. A `Command::Model` is the one kind a client **cannot**
fold optimistically — it does not know what the document will accept, which is the
very argument `Broadcast::ToAll` is written on for `ResetCursor` — so the asker is
owed the accepted view on exactly the same terms as everybody else.
With `ToOthers` the asker's mirror was moved to the accepted view by
`adopt_active_view` while its own slice stayed on the outgoing one: the server and
the client disagree about which view keys the cursor map, which is gotcha 24's
mismatch arriving through the asker's own keystroke. Nothing else in the suite
could see it — `a_model_command_that_moves_the_active_view_binds_every_mirror`
reads `to_others`, which by construction looks away from the asker.
#### Helper — `to_others`
Not a test — and its blind spot is exactly why that test was needed.
`diff().1` as the tests that predate `Broadcast` asked it: what a bystander is
told, and nothing about the asker. That is exactly why it cannot see
`a_model_commands_view_switch_reaches_the_asker_too`'s bug.
*Both `a_model_command_that_moves_the_active_view_binds_every_mirror` and
`a_model_commands_view_switch_reaches_the_asker_too` were retired by
improvise-jytm, along with the `to_others` helper whose blind spot the second one
existed to cover. No `ModelCommand` moves anybody's active view any more:
`ViewBack`/`ViewForward` left the wire because the stack they pop is one client's
history, and a document swap lands in the rebase arm. The property the second test
defended — a correction must reach the client that folded it — is now
`a_refused_switch_corrects_the_asker_and_nobody_else`, which drives the asker's own
slice rather than a helper.*
### a_geometry_command_invalidates_every_clients_cursor_including_the_askers
@@ -1076,7 +1217,7 @@ scroll (improvise-gvnb).
The asker is checked alongside the bystander, and that is not symmetry for its own
sake. A `Command::Model` is the one kind a client cannot fold optimistically, so
the asker is in exactly the same position as everyone else, which is why the reset
is `Broadcast::ToAll` and not the `ToOthers` a `SwitchView` correction is.
is `Broadcast::ToAll` and not the `To(id, …)` a refused `SwitchView` correction is.
### a_model_command_that_does_not_reshape_the_grid_leaves_cursors_alone
@@ -1634,11 +1775,15 @@ it fabricates `(hash, 0)` — a claim no real client could compose, and precisel
input that flips the create exemption in `CheckedBatch`. For a record the frame is
*creating*, leave it out, which is what a client would do.
### a_bystander_is_sent_the_broadcast_and_the_asker_is_not
### an_accepted_switch_is_relayed_to_nobody
The bug Copilot found on PR #10: a bystander must fold the accepted command itself,
because its own view slice lives on the client and only `reduce_view` over the same
command keeps it in step with the server's mirror (gotcha 36).
The successor to `a_bystander_is_sent_the_broadcast_and_the_asker_is_not`, which
pinned the pre-improvise-jytm relay (Copilot, PR #10). An accepted switch is now
owed to nobody: the asker folded it, and no other client's view moves.
It asserts the bystander gets **no frame at all**, not merely no `SwitchView`. A
relay that vanished while the projection fan-out still repainted every client would
pass the narrower check and still be pushing one client's view change at the rest.
### a_geometry_command_relays_the_cursor_reset_to_the_asker_too
@@ -1654,11 +1799,17 @@ A refused batch is answered to the **asker and nobody else** (ADR 001). Nothing
happened, so there is nothing to tell the others — and the asker is told only which
class of staleness it was, because it already holds what is current.
### folding_the_broadcast_moves_the_bystanders_active_view
### folding_the_relay_puts_a_refused_asker_back
And what the broadcast is *for*: a bystander that folds what it is sent ends up
naming the view the document accepted, so the cursor its next grid is positioned by
is read under the right key (gotcha 24).
What `Broadcast::To` is *for*, asserted end to end through `Host` rather than
through `Sessions`: a client that folded a view name the document refused is put
back by the frame the host sends it, so the cursor its next grid is positioned by is
read under a key the document has (gotcha 24).
The client slice is folded **before** the command is handed to the host, which is
what makes the assertion mean anything: a test that never folded would see
"Default" whatever the relay did. The bystander is asserted silent in the same test,
because a `ToAll` correction would also put the asker back and would be wrong.
### a_document_swap_sends_every_client_a_fresh_snapshot
crates/improvise-command/src/ui/app.rs
+72 -34
View File
@@ -24,7 +24,7 @@ use crate::persistence;
use crate::ui::effect::{EffectKind, ModelChange};
use crate::ui::project::Dirty;
use crate::ui::viewport::{compute_col_widths, compute_row_header_width, compute_visible_cols};
use crate::view::GridLayout;
use crate::view::{GridLayout, View};
use crate::workbook::Workbook;
use improvise_core::model::record::{AggregateHash, RecordHash};
use improvise_protocol::RecordCell;
@@ -295,14 +295,44 @@ impl App {
.flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c)))
}
/// Point the view slice at `name` and switch the workbook to it. The two
/// copies must move together, or the cursor map is miskeyed (improvise-cqi).
/// Point **this client's** slice at `name`, if the document has such a view.
/// The document's own default stays put (improvise-jytm, gotcha 29).
pub fn switch_to_view(&mut self, name: &str) {
if self.model_state.workbook.switch_view(name).is_ok() {
if self.model_state.workbook.views.contains_key(name) {
self.view_state.active_view = name.to_string();
}
}
/// What `name` names in this document, falling back to the default for a name
/// the document has not got. The **one** answer — see `context/code/command.md`.
pub fn resolve_view<'a>(&'a self, name: &str) -> (&'a str, &'a View) {
let workbook = &self.model_state.workbook;
match workbook.views.get_key_value(name) {
Some((key, view)) => (key.as_str(), view),
None => {
let default = workbook.active_view.as_str();
(default, &workbook.views[default])
}
}
}
/// Which view this client is on.
pub fn view_name(&self) -> &str {
self.resolve_view(&self.view_state.active_view).0
}
/// The view this client is on.
pub fn view(&self) -> &View {
self.resolve_view(&self.view_state.active_view).1
}
/// The same view, to mutate. View configuration is document state shared by
/// every client on that view, so this is where a geometry effect writes.
pub fn view_mut(&mut self) -> &mut View {
let name = self.view_name().to_string();
&mut self.model_state.workbook.views[&name]
}
/// Send the active view's cursor to the origin after a shape change. Clients
/// hear about it through [`cursor_epoch`](Self::cursor_epoch) (improvise-gvnb).
pub fn reset_cursor(&mut self) {
@@ -319,12 +349,13 @@ impl App {
/// Rebuild the grid layout from current workbook, active view, and drill
/// state. Note: `with_frozen_records` already handles pruning internally.
pub fn rebuild_layout(&mut self) {
let none_cats = self.model_state.workbook.active_view().none_cats();
let name = self.view_name().to_string();
let none_cats = self.model_state.workbook.views[&name].none_cats();
self.model_state
.workbook
.model
.recompute_formulas(&none_cats);
let view = self.model_state.workbook.active_view();
let view = &self.model_state.workbook.views[&name];
let frozen = self.drill_state.as_ref().map(|s| Rc::clone(&s.records));
self.layout =
GridLayout::with_frozen_records(&self.model_state.workbook.model, view, frozen);
@@ -343,16 +374,20 @@ impl App {
/// The text an editor opened on `key` starts with — unformatted, so **not**
/// `CellDisplay::text` (design-principles §4). Crate-visible for the wire.
pub(crate) fn display_value_of(&self, key: &CellKey) -> String {
self.display_value_in(&self.layout, key)
}
/// The same text against a layout that may be another client's (improvise-jytm).
pub(crate) fn display_value_in(&self, layout: &GridLayout, key: &CellKey) -> String {
match crate::view::synthetic_record_info(key) {
Some((idx, col)) => self
.layout
Some((idx, col)) => layout
.record_col(col)
.and_then(|rc| {
self.drill_state
.as_ref()
.and_then(|s| s.pending_edits.get(&(idx, rc.clone())).cloned())
})
.or_else(|| self.layout.resolve_display(key))
.or_else(|| layout.resolve_display(key))
.unwrap_or_default(),
None => self
.model_state
@@ -365,7 +400,7 @@ impl App {
}
pub fn cmd_context(&self, key: KeyCode, _mods: KeyModifiers) -> CmdContext<'_> {
let view = self.model_state.workbook.active_view();
let view = self.view();
let layout = &self.layout;
let cursor = self.cursor();
CmdContext {
@@ -720,34 +755,42 @@ mod tests {
);
}
/// improvise-cqi: the document's copy and the client's must move together, or
/// the cursor map is read under the wrong key. `App` owns both writers.
/// improvise-jytm replaces the old lockstep guard: the document keeps the
/// *default*, the client's slice is authoritative. See `context/code/command.md`.
#[test]
fn the_two_active_view_names_never_diverge() {
fn the_documents_default_view_outlives_this_clients_switches() {
let mut app = two_col_model();
app.model_state.workbook.create_view("Second");
fn agree(app: &App) {
assert_eq!(
app.view_state.active_view, app.model_state.workbook.active_view,
"view slice and document disagree about the active view"
);
}
agree(&app);
assert_eq!(app.view_state.active_view, "Default");
assert_eq!(app.model_state.workbook.active_view, "Default");
app.switch_to_view("Second");
agree(&app);
// A switch to a view that does not exist must move neither.
app.switch_to_view("Nonexistent");
agree(&app);
assert_eq!(app.view_state.active_view, "Second");
assert_eq!(
app.model_state.workbook.active_view, "Default",
"a client's switch moved the document's default"
);
assert_eq!(
app.view().axis_of("_Measure"),
app.model_state.workbook.views["Second"].axis_of("_Measure"),
"the layout is built from the document's default, not this client's view"
);
// A switch to a view that does not exist moves nothing: the slice keys
// the cursor map and must never name a view the document lacks.
app.switch_to_view("Nonexistent");
assert_eq!(app.view_state.active_view, "Second");
assert_eq!(app.model_state.workbook.active_view, "Default");
let mut incoming = Workbook::new("Other");
incoming.create_view("Elsewhere");
incoming.switch_view("Elsewhere").unwrap();
app.install_workbook(incoming);
agree(&app);
assert_eq!(app.view_state.active_view, "Elsewhere");
assert_eq!(
app.view_state.active_view, "Elsewhere",
"a replaced document does not seat this client on its default"
);
assert_eq!(app.model_state.workbook.active_view, "Elsewhere");
assert!(
app.view_state.cursors.is_empty(),
"a new document does not inherit the old one's cursors"
@@ -1417,15 +1460,10 @@ mod tests {
assert!(
app.layout.is_records_mode(),
"naming a category must not leave records mode; axes are {:?}",
app.model_state
.workbook
.active_view()
.category_axes
.iter()
.collect::<Vec<_>>()
app.view().category_axes.iter().collect::<Vec<_>>()
);
assert_eq!(
app.model_state.workbook.active_view().axis_of("Product"),
app.view().axis_of("Product"),
crate::view::Axis::None,
"records mode keeps every regular category on None, and \
`records_columns` picks it up as a column from `is_regular` \
crates/improvise-command/src/ui/effect.rs
+85 -38
View File
@@ -241,7 +241,7 @@ impl Effect for EnterEditAtCursor {
pub struct TogglePruneEmpty;
impl Effect for TogglePruneEmpty {
fn apply(&self, app: &mut App) {
let v = app.model_state.workbook.active_view_mut();
let v = app.view_mut();
v.prune_empty = !v.prune_empty;
}
fn kind(&self) -> EffectKind {
@@ -307,6 +307,9 @@ pub struct DeleteView(pub String);
impl Effect for DeleteView {
fn apply(&self, app: &mut App) {
let _ = app.model_state.workbook.delete_view(&self.0);
// This client may have been the one on it. Idempotent for every other
// view; the mirrors owe the same move (`Sessions::adopt_deleted_views`).
app.view_state.active_view = app.view_name().to_string();
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -317,7 +320,7 @@ impl Effect for DeleteView {
pub struct SwitchView(pub String);
impl Effect for SwitchView {
fn apply(&self, app: &mut App) {
let current = app.model_state.workbook.active_view.clone();
let current = app.view_state.active_view.clone();
if current != self.0 {
app.view_state.view_back_stack.push(ViewFrame {
view_name: current,
@@ -327,8 +330,10 @@ impl Effect for SwitchView {
}
app.switch_to_view(&self.0);
}
// Which view this client is looking at is UI, not document (improvise-jytm).
// `Dirty` is therefore not the door: see `ClientSession::project`.
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
EffectKind::ViewOnly
}
}
@@ -338,7 +343,7 @@ pub struct ViewBack;
impl Effect for ViewBack {
fn apply(&self, app: &mut App) {
if let Some(frame) = app.view_state.view_back_stack.pop() {
let current = app.model_state.workbook.active_view.clone();
let current = app.view_state.active_view.clone();
app.view_state.view_forward_stack.push(ViewFrame {
view_name: current,
mode: app.view_state.mode.clone(),
@@ -348,7 +353,7 @@ impl Effect for ViewBack {
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
EffectKind::ViewOnly
}
}
@@ -358,7 +363,7 @@ pub struct ViewForward;
impl Effect for ViewForward {
fn apply(&self, app: &mut App) {
if let Some(frame) = app.view_state.view_forward_stack.pop() {
let current = app.model_state.workbook.active_view.clone();
let current = app.view_state.active_view.clone();
app.view_state.view_back_stack.push(ViewFrame {
view_name: current,
mode: app.view_state.mode.clone(),
@@ -368,7 +373,7 @@ impl Effect for ViewForward {
}
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
EffectKind::ViewOnly
}
}
@@ -379,10 +384,7 @@ pub struct SetAxis {
}
impl Effect for SetAxis {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
.set_axis(&self.category, self.axis);
app.view_mut().set_axis(&self.category, self.axis);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -396,9 +398,7 @@ pub struct SetPageSelection {
}
impl Effect for SetPageSelection {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
app.view_mut()
.set_page_selection(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
@@ -413,9 +413,7 @@ pub struct ToggleGroup {
}
impl Effect for ToggleGroup {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
app.view_mut()
.toggle_group_collapse(&self.category, &self.group);
}
fn kind(&self) -> EffectKind {
@@ -430,10 +428,7 @@ pub struct HideItem {
}
impl Effect for HideItem {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
.hide_item(&self.category, &self.item);
app.view_mut().hide_item(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -447,10 +442,7 @@ pub struct ShowItem {
}
impl Effect for ShowItem {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
.show_item(&self.category, &self.item);
app.view_mut().show_item(&self.category, &self.item);
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -461,7 +453,7 @@ impl Effect for ShowItem {
pub struct TransposeAxes;
impl Effect for TransposeAxes {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().transpose_axes();
app.view_mut().transpose_axes();
// Swapping the axes renumbers every row and column, so the old indices
// name different cells (improvise-8uz, gotcha 24).
app.reset_cursor();
@@ -475,10 +467,7 @@ impl Effect for TransposeAxes {
pub struct CycleAxis(pub String);
impl Effect for CycleAxis {
fn apply(&self, app: &mut App) {
app.model_state
.workbook
.active_view_mut()
.cycle_axis(&self.0);
app.view_mut().cycle_axis(&self.0);
// Same reasoning as TransposeAxes: the grid changes shape underneath
// the cursor, so it goes back to the origin (improvise-8uz).
app.reset_cursor();
@@ -492,7 +481,7 @@ impl Effect for CycleAxis {
pub struct SetNumberFormat(pub String);
impl Effect for SetNumberFormat {
fn apply(&self, app: &mut App) {
app.model_state.workbook.active_view_mut().number_format = self.0.clone();
app.view_mut().number_format = self.0.clone();
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -873,7 +862,7 @@ impl Effect for StartImportWizard {
pub struct ExportCsv(pub PathBuf);
impl Effect for ExportCsv {
fn apply(&self, app: &mut App) {
let view_name = app.model_state.workbook.active_view.clone();
let view_name = app.view_name().to_string();
match crate::persistence::export_csv_to(
&*app.storage,
&app.model_state.workbook,
@@ -1667,7 +1656,7 @@ mod tests {
assert!(app.view_state.view_back_stack.is_empty());
SwitchView("View 2".to_string()).apply(&mut app);
assert_eq!(app.model_state.workbook.active_view.as_str(), "View 2");
assert_eq!(app.view_state.active_view.as_str(), "View 2");
assert_eq!(app.view_state.view_back_stack.len(), 1);
assert_eq!(app.view_state.view_back_stack[0].view_name, "Default");
// Forward stack should be cleared
@@ -1686,18 +1675,18 @@ mod tests {
let mut app = test_app();
app.model_state.workbook.create_view("View 2");
SwitchView("View 2".to_string()).apply(&mut app);
assert_eq!(app.model_state.workbook.active_view.as_str(), "View 2");
assert_eq!(app.view_state.active_view.as_str(), "View 2");
// Go back
ViewBack.apply(&mut app);
assert_eq!(app.model_state.workbook.active_view.as_str(), "Default");
assert_eq!(app.view_state.active_view.as_str(), "Default");
assert_eq!(app.view_state.view_forward_stack.len(), 1);
assert_eq!(app.view_state.view_forward_stack[0].view_name, "View 2");
assert!(app.view_state.view_back_stack.is_empty());
// Go forward
ViewForward.apply(&mut app);
assert_eq!(app.model_state.workbook.active_view.as_str(), "View 2");
assert_eq!(app.view_state.active_view.as_str(), "View 2");
assert_eq!(app.view_state.view_back_stack.len(), 1);
assert_eq!(app.view_state.view_back_stack[0].view_name, "Default");
assert!(app.view_state.view_forward_stack.is_empty());
@@ -1706,9 +1695,9 @@ mod tests {
#[test]
fn view_back_with_empty_stack_is_noop() {
let mut app = test_app();
let before = app.model_state.workbook.active_view.clone();
let before = app.view_state.active_view.clone();
ViewBack.apply(&mut app);
assert_eq!(app.model_state.workbook.active_view, before);
assert_eq!(app.view_state.active_view, before);
}
#[test]
@@ -1721,6 +1710,64 @@ mod tests {
assert!(!app.model_state.workbook.views.contains_key("View 2"));
}
/// improvise-jytm: view history is UI, so it must move no document state.
/// See `context/test-evidence/command.md`.
#[test]
fn walking_the_view_history_moves_no_document_state() {
let mut app = test_app();
app.model_state.workbook.create_view("View 2");
app.apply_effects(vec![Box::new(SwitchView("View 2".to_string()))]);
app.model_state.dirty = false;
let layout_version = app.layout_version();
for (name, effect) in [
("view-back", Box::new(ViewBack) as Box<dyn Effect>),
("view-forward", Box::new(ViewForward)),
] {
let before = app.view_state.active_view.clone();
app.apply_effects(vec![effect]);
assert_ne!(
app.view_state.active_view, before,
"{name} moved nothing, so the assertions below prove nothing"
);
assert!(
!app.model_state.dirty,
"{name} dirtied a document it did not change, so a browser worker \
rewrites the whole OPFS entry for a UI-only move"
);
assert_eq!(
app.layout_version(),
layout_version,
"{name} bumped the basis, so every other client's next edit is \
refused as LayoutMoved for a move only this client made"
);
}
assert_eq!(app.model_state.workbook.active_view, "Default");
}
/// improvise-jytm: deleting the view this client is on must move the client.
/// See `context/test-evidence/command.md`.
#[test]
fn deleting_the_view_this_client_is_on_moves_it_to_the_default() {
let mut app = test_app();
CreateView("View 2".to_string()).apply(&mut app);
CreateView("View 3".to_string()).apply(&mut app);
SwitchView("View 2".to_string()).apply(&mut app);
DeleteView("View 3".to_string()).apply(&mut app);
assert_eq!(
app.view_state.active_view, "View 2",
"deleting somebody else's view moved this client"
);
DeleteView("View 2".to_string()).apply(&mut app);
assert_eq!(
app.view_state.active_view, "Default",
"this client still names the view it was just deleted out of"
);
assert_eq!(app.view_state.active_view, app.view_name());
}
#[test]
fn set_axis_effect() {
let mut app = test_app();
crates/improvise-command/src/ui/host.rs
+32 -31
View File
@@ -154,7 +154,7 @@ impl Host {
Applied::Diff { dirty, broadcast } => {
// The broadcast precedes the projections, so a client's slice is in
// the state the grid it is about to get was built from (gotcha 36).
let mut out = self.relay(id, broadcast);
let mut out = self.relay(broadcast);
out.extend(self.project(dirty));
out
}
@@ -169,18 +169,18 @@ impl Host {
/// Who hears a relayed command is the relay's own business: [`Broadcast`] names
/// its audience and the match is what makes the host honour it (improvise-gvnb).
fn relay(&self, from: SessionId, broadcast: Vec<Broadcast>) -> Vec<Addressed> {
fn relay(&self, broadcast: Vec<Broadcast>) -> Vec<Addressed> {
broadcast
.into_iter()
.flat_map(|item| {
let (cmd, skip) = match item {
Broadcast::ToOthers(cmd) => (cmd, Some(from)),
let (cmd, only) = match item {
Broadcast::ToAll(cmd) => (cmd, None),
Broadcast::To(id, cmd) => (cmd, Some(id)),
};
self.sessions
.ids()
.into_iter()
.filter(move |id| Some(*id) != skip)
.filter(move |id| only.is_none_or(|only| only == *id))
.map(move |to| Addressed {
to,
msg: ServerFrame::Commands(vec![cmd.clone()]),
@@ -845,10 +845,10 @@ mod tests {
// ── the broadcast ───────────────────────────────────────────────────
/// The bug Copilot found on PR #10: a bystander must fold the accepted command
/// itself, because its own view slice lives on the client (gotcha 36).
/// Which view a client is on is that client's (improvise-jytm): an accepted
/// switch reaches nobody, not even back to the asker that folded it.
#[test]
fn a_bystander_is_sent_the_broadcast_and_the_asker_is_not() {
fn an_accepted_switch_is_relayed_to_nobody() {
let mut host = host();
host.app.model_state.workbook.create_view("Second");
let (asker, _) = host.connect(10, 10);
@@ -859,18 +859,16 @@ mod tests {
ClientFrame::command(Command::SwitchView("Second".into())),
);
let broadcast = Command::SwitchView("Second".into());
let relay = ServerFrame::Commands(vec![Command::SwitchView("Second".into())]);
for (who, id) in [("asker", asker), ("bystander", bystander)] {
assert!(
!to(&frames, id).contains(&relay),
"the {who} was relayed a switch it is not owed: {frames:?}"
);
}
assert!(
to(&frames, bystander)
.iter()
.any(|m| *m == ServerFrame::Commands(vec![broadcast.clone()])),
"the bystander was never told the view moved: {frames:?}"
);
assert!(
!to(&frames, asker)
.iter()
.any(|m| *m == ServerFrame::Commands(vec![broadcast.clone()])),
"the asker was sent back the command it already folded: {frames:?}"
to(&frames, bystander).is_empty(),
"a bystander was repainted for somebody else's switch: {frames:?}"
);
}
@@ -930,33 +928,36 @@ mod tests {
);
}
/// And what the broadcast is *for*: a bystander that folds it names the view the
/// document accepted, so its next grid is positioned under the right key.
/// What the [`Broadcast::To`] audience is *for*: a client that folded a
/// name the document refused is put back, through the host (improvise-6c6q).
#[test]
fn folding_the_broadcast_moves_the_bystanders_active_view() {
fn folding_the_relay_puts_a_refused_asker_back() {
let mut host = host();
host.app.model_state.workbook.create_view("Second");
let (asker, _) = host.connect(10, 10);
let (bystander, bootstrap) = host.connect(10, 10);
let (asker, bootstrap) = host.connect(10, 10);
let (bystander, _) = host.connect(10, 10);
let ServerFrame::Snapshot(snapshot) = bootstrap.msg else {
panic!("connect did not send a Snapshot");
};
let mut client: ViewState = snapshot.view;
assert_eq!(client.active_view, "Default");
let asked = Command::SwitchView("Nonexistent".into());
reduce_view(&mut client, &asked);
assert_eq!(client.active_view, "Nonexistent", "the asker folded it");
let frames = host.handle(
asker,
ClientFrame::command(Command::SwitchView("Second".into())),
);
for msg in to(&frames, bystander) {
let frames = host.handle(asker, ClientFrame::command(asked));
for msg in to(&frames, asker) {
if let ServerFrame::Commands(cmds) = msg {
for cmd in &cmds {
reduce_view(&mut client, cmd);
}
}
}
assert_eq!(client.active_view, "Second");
assert_eq!(client.active_view, "Default");
assert!(
to(&frames, bystander).is_empty(),
"a bystander heard about somebody else's refused switch: {frames:?}"
);
}
// ── the document swap ───────────────────────────────────────────────
crates/improvise-command/src/ui/project.rs
+325 -144
View File
@@ -8,8 +8,7 @@ use improvise_core::model::Model;
use improvise_core::model::category::MEASURE_CATEGORY;
use improvise_core::model::cell::CellValue;
use improvise_core::model::record::{AggregateHash, RecordHash};
use improvise_core::view::layout::AxisEntry;
use improvise_core::view::layout::RecordCol;
use improvise_core::view::layout::{AxisEntry, GridLayout, RecordCol};
use improvise_core::view::{Axis, View};
use improvise_core::workbook::Workbook;
use improvise_io::import::analyzer::{DateComponent, FieldKind};
@@ -125,26 +124,24 @@ impl Viewport {
/// The grid as `viewport` sees it. Indexed by position *within the viewport*,
/// which is what `GridCache` means by row and column.
pub fn grid_of(app: &App, viewport: Viewport) -> GridCache {
grid_in(app, app.view_name(), viewport)
}
/// The same grid, for a client that may be on another view (improvise-jytm). The
/// host's own layout is reused where the names agree; see `context/code/command.md`.
pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache {
let workbook = &app.model_state.workbook;
let layout = &app.layout;
let (comma, decimals) = parse_number_format(&workbook.active_view().number_format);
let (name, view) = app.resolve_view(view_name);
let elsewhere = (name != app.view_name()).then(|| GridLayout::new(&workbook.model, view));
let layout = elsewhere.as_ref().unwrap_or(&app.layout);
let (comma, decimals) = parse_number_format(&view.number_format);
let rows = viewport.rows_in(layout.row_count());
let cols = viewport.cols_in(layout.col_count());
let widths = compute_col_widths(&workbook.model, layout, comma, decimals);
GridCache {
row_frame: axis_frame(
&layout.row_cats,
&layout.row_items,
workbook.active_view(),
rows.clone(),
),
col_frame: axis_frame(
&layout.col_cats,
&layout.col_items,
workbook.active_view(),
cols.clone(),
),
row_frame: axis_frame(&layout.row_cats, &layout.row_items, view, rows.clone()),
col_frame: axis_frame(&layout.col_cats, &layout.col_items, view, cols.clone()),
col_widths: cols
.clone()
.map(|c| widths.get(c).copied().unwrap_or(0))
@@ -153,11 +150,11 @@ pub fn grid_of(app: &App, viewport: Viewport) -> GridCache {
.clone()
.map(|r| {
cols.clone()
.map(|c| cell_display(app, r, c, comma, decimals))
.map(|c| cell_display(app, layout, r, c, comma, decimals))
.collect()
})
.collect(),
totals: column_totals(app, cols, comma, decimals),
totals: column_totals(app, layout, cols, comma, decimals),
page_coords: layout.page_coords.clone(),
// The whole grid, not the window: this is what a client clamps its own
// cursor against (improvise-bpq2). Off the layout, not the clipped ranges.
@@ -223,11 +220,11 @@ fn axis_frame(
/// viewport's — which is why it is projected. Empty means "no totals" (gotcha 33).
fn column_totals(
app: &App,
layout: &GridLayout,
cols: std::ops::Range<usize>,
comma: bool,
decimals: u8,
) -> Vec<CellDisplay> {
let layout = &app.layout;
let model = &app.model_state.workbook.model;
let drawn = !layout.is_records_mode() && layout.row_count() > 0 && layout.col_count() > 0;
match drawn {
@@ -246,8 +243,14 @@ fn column_totals(
/// One cell as the client draws it. The server knows whether a value was a number
/// and whether the cell is settable, so it says both (ADR 002, [`cell_id`]).
fn cell_display(app: &App, row: usize, col: usize, comma: bool, decimals: u8) -> CellDisplay {
let layout = &app.layout;
fn cell_display(
app: &App,
layout: &GridLayout,
row: usize,
col: usize,
comma: bool,
decimals: u8,
) -> CellDisplay {
if layout.is_records_mode() {
// Records mode resolves through the frozen snapshot, which yields text only:
// the one place this module infers numeric-ness rather than knowing it.
@@ -258,7 +261,7 @@ fn cell_display(app: &App, row: usize, col: usize, comma: bool, decimals: u8) ->
text,
numeric,
error: false,
id: cell_id(app, row, col),
id: cell_id(app, layout, row, col),
};
}
let value = layout.cell_key(row, col).and_then(|key| {
@@ -271,19 +274,18 @@ fn cell_display(app: &App, row: usize, col: usize, comma: bool, decimals: u8) ->
text: format_value(value.as_ref(), comma, decimals),
numeric: matches!(value, Some(CellValue::Number(_))),
error: matches!(value, Some(CellValue::Error(_))),
id: cell_id(app, row, col),
id: cell_id(app, layout, row, col),
// The *unformatted* value the TUI seeds its edit buffer from. Projecting it
// beats a client un-formatting `text` (gotcha 47).
edit_text: layout
.cell_key(row, col)
.map(|key| app.display_value_of(&key)),
.map(|key| app.display_value_in(layout, &key)),
}
}
/// What the cell at `(row, col)` *is*, as the surrogate the client echoes back. The
/// fork is `EditOrDrill`'s, asked the same way — see `context/code/command.md`.
fn cell_id(app: &App, row: usize, col: usize) -> Option<CellId> {
let layout = &app.layout;
fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option<CellId> {
let model = &app.model_state.workbook.model;
if layout.is_records_mode() {
// The row's *coordinates*, never the synthetic `(_Index, _Dim)` key: that
@@ -364,9 +366,8 @@ pub fn view_summaries(app: &App) -> Vec<ViewSummary> {
/// The tiles the tile bar offers — the same set the TUI's tile bar draws,
/// including the virtual categories, minus the ones a drill has fixed.
pub fn tile_entries(app: &App) -> Vec<TileEntry> {
pub fn tile_entries(app: &App, view: &View) -> Vec<TileEntry> {
let workbook = &app.model_state.workbook;
let view = workbook.active_view();
view.tileable_cats(&workbook.model.category_names())
.into_iter()
.map(|cat| TileEntry {
@@ -382,9 +383,14 @@ pub fn tile_entries(app: &App) -> Vec<TileEntry> {
/// The category panel's cache: every category, its items and its axis in the
/// active view.
pub fn category_tree_of(app: &App) -> CategoryTreeCache {
let workbook = &app.model_state.workbook;
category_tree_in(app, app.view())
}
/// The same cache against a view that may be another client's (improvise-jytm):
/// a category's axis is a fact about the view, so it is per-client.
pub fn category_tree_in(app: &App, view: &View) -> CategoryTreeCache {
CategoryTreeCache {
categories: category_rows(&workbook.model, workbook.active_view()),
categories: category_rows(&app.model_state.workbook.model, view),
}
}
@@ -404,18 +410,29 @@ pub fn view_list_of(app: &App) -> ViewListCache {
/// The tile bar's cache: the tileable categories and their axes.
pub fn tile_bar_of(app: &App) -> TileBarCache {
tile_bar_in(app, app.view())
}
/// The same cache against a view that may be another client's (improvise-jytm).
pub fn tile_bar_in(app: &App, view: &View) -> TileBarCache {
TileBarCache {
tiles: tile_entries(app),
tiles: tile_entries(app, view),
}
}
/// The projections for every widget whose content does not depend on a client's
/// viewport. Each replaces its cache wholesale, so `dirty` is the whole decision.
pub fn document_projections(app: &App, dirty: Dirty) -> Vec<Projection> {
projections_in(app, app.view(), dirty)
}
/// The same projections for a client on `view`. Two of the four are view-dependent
/// and so are per-client since improvise-jytm; the formula list and view list are not.
pub fn projections_in(app: &App, view: &View, dirty: Dirty) -> Vec<Projection> {
let mut out = Vec::new();
if dirty.category_tree {
out.push(Projection::CategoryTree(CategoryTreeUpdate::Categories(
category_tree_of(app).categories,
category_tree_in(app, view).categories,
)));
}
if dirty.formulas {
@@ -425,7 +442,7 @@ pub fn document_projections(app: &App, dirty: Dirty) -> Vec<Projection> {
out.push(Projection::ViewList(view_list_of(app).views));
}
if dirty.tile_bar {
out.push(Projection::TileBar(tile_bar_of(app).tiles));
out.push(Projection::TileBar(tile_bar_in(app, view).tiles));
}
out
}
@@ -593,6 +610,9 @@ pub struct ClientSession {
/// publication the check would be satisfiable only by forging.
basis: BasisCache,
projected: Option<Viewport>,
/// The view the mirror's grid was last built from. A switch is `ViewOnly`, so
/// `Dirty` cannot carry it and this client's own move is **observed** instead.
projected_view: Option<String>,
/// The document generation this mirror was last seeded at. When the app's
/// has moved past it, the whole mirror belongs to a document that is gone.
generation: u64,
@@ -606,6 +626,7 @@ impl ClientSession {
grid: GridCache::default(),
basis: BasisCache::default(),
projected: None,
projected_view: None,
generation,
}
}
@@ -647,10 +668,18 @@ impl ClientSession {
}
fn project_diff(&mut self, app: &App, dirty: Dirty) -> Vec<Projection> {
let mut out = document_projections(app, dirty);
// Every projection is view-dependent, so a client that has moved is owed
// its whole widget set, not the batch's `Dirty` (improvise-jytm).
let moved = self.projected_view.as_deref() != Some(self.view.active_view.as_str());
let dirty = match moved {
true => Dirty::EVERYTHING,
false => dirty,
};
let view = self.view_in(app);
let mut out = projections_in(app, view, dirty);
let viewport = self.viewport();
if dirty.grid || self.projected != Some(viewport) {
let desired = grid_of(app, viewport);
let desired = grid_in(app, &self.view.active_view, viewport);
out.extend(
grid_updates(&self.grid, &desired)
.into_iter()
@@ -658,11 +687,18 @@ impl ClientSession {
);
self.grid = desired;
self.projected = Some(viewport);
self.projected_view = Some(self.view.active_view.clone());
}
out.extend(self.basis_update(app).map(Projection::Basis));
out
}
/// The document's view this client is on, falling back to the default for a
/// slice naming a view the document no longer has.
fn view_in<'a>(&self, app: &'a App) -> &'a View {
app.resolve_view(&self.view.active_view).1
}
/// The basis to publish, if it moved. Emitted **after** the grid updates it
/// describes, and only for the records this client's grid names (ADR 002).
fn basis_update(&mut self, app: &App) -> Option<Basis> {
@@ -694,6 +730,7 @@ impl ClientSession {
self.grid = GridCache::default();
self.basis = BasisCache::default();
self.projected = None;
self.projected_view = None;
self.project_diff(app, Dirty::EVERYTHING)
}
}
@@ -714,12 +751,12 @@ pub struct Sessions {
/// the message because a host cannot infer it from the command.
#[derive(Debug, Clone, PartialEq)]
pub enum Broadcast {
/// Every client **except** the asker: a *correction* of state it already folded
/// optimistically, so sending it back would double-apply the move.
ToOthers(Command),
/// Every client, the asker included: a **consequence**, not a correction. A
/// `Command::Model` is never folded optimistically (improvise-gvnb).
ToAll(Command),
/// One named client: a correction of what only that client's UI holds. Its two
/// cases are a refused switch and a deleted view (improvise-6c6q, improvise-jytm).
To(SessionId, Command),
}
/// What one client's command did, as the host needs to hear it. An enum, so a host
@@ -782,7 +819,7 @@ impl Sessions {
/// a whole frame with a [`CheckedBatch`] first (improvise-955n).
pub fn apply_checked(&mut self, app: &mut App, id: SessionId, cmd: Checked<'_>) -> Applied {
match cmd.command() {
Command::SwitchView(_) => self.apply_switch(app, cmd),
Command::SwitchView(asked) => self.apply_switch(app, id, asked),
Command::Model { .. } => self.apply_model(app, cmd),
other => {
if let Some(session) = self.sessions.get_mut(&id) {
@@ -796,31 +833,46 @@ impl Sessions {
}
}
/// The document half, addressed to no one session — hence no [`SessionId`].
/// Every mirror takes what was *accepted*, never what was asked.
fn apply_switch(&mut self, app: &mut App, cmd: Checked<'_>) -> Applied {
let epoch = app.cursor_epoch();
let dirty = apply_command(app, cmd);
let accepted = self.adopt_active_view(app);
let mut broadcast = vec![Broadcast::ToOthers(Command::SwitchView(accepted))];
broadcast.extend(self.adopt_cursor_epoch(app, epoch));
Applied::Diff { dirty, broadcast }
/// Which view a client is on is **that client's** (improvise-jytm): only the
/// asker's mirror moves, and only to a view the document has.
fn apply_switch(&mut self, app: &App, id: SessionId, asked: &str) -> Applied {
let accepted = match self.sessions.get_mut(&id) {
None => {
return Applied::Diff {
dirty: Dirty::NOTHING,
broadcast: Vec::new(),
};
}
Some(session) => {
session.view.active_view = match app.model_state.workbook.views.contains_key(asked)
{
true => asked.to_string(),
false => session.view.active_view.clone(),
};
session.view.active_view.clone()
}
};
// The asker folded optimistically, so it hears only when the document
// refused; nobody else is affected at all (improvise-6c6q).
let broadcast = (accepted != asked)
.then_some(Broadcast::To(id, Command::SwitchView(accepted)))
.into_iter()
.collect();
Applied::Diff {
dirty: Dirty::NOTHING,
broadcast,
}
}
/// The document half of a [`Command::Model`] (improvise-kmgs). Same lockstep as
/// `apply_switch`, but `ToAll` — see `context/code/command.md`.
fn apply_model(&mut self, app: &mut App, cmd: Checked<'_>) -> Applied {
let before = app.model_state.workbook.active_view.clone();
let generation = app.document_generation();
let epoch = app.cursor_epoch();
let dirty = apply_command(app, cmd);
match app.document_generation() == generation {
true => {
let accepted = self.adopt_active_view(app);
let mut broadcast: Vec<Broadcast> = (accepted != before)
.then_some(Broadcast::ToAll(Command::SwitchView(accepted)))
.into_iter()
.collect();
let mut broadcast = self.adopt_deleted_views(app);
broadcast.extend(self.adopt_cursor_epoch(app, epoch));
Applied::Diff { dirty, broadcast }
}
@@ -832,14 +884,18 @@ impl Sessions {
}
}
/// Point every mirror at the view the document *accepted*, and say which that
/// is (`doctrine(lockstep_binds_every_session_not_the_asker)`).
fn adopt_active_view(&mut self, app: &App) -> String {
let accepted = app.model_state.workbook.active_view.clone();
for session in self.sessions.values_mut() {
session.view.active_view = accepted.clone();
}
accepted
/// A mirror naming a view the batch deleted is moved to the document's default
/// and told. Observed, never predicted — see `context/code/command.md`.
fn adopt_deleted_views(&mut self, app: &App) -> Vec<Broadcast> {
let workbook = &app.model_state.workbook;
self.sessions
.iter_mut()
.filter(|(_, session)| !workbook.views.contains_key(&session.view.active_view))
.map(|(id, session)| {
session.view.active_view = workbook.active_view.clone();
Broadcast::To(*id, Command::SwitchView(workbook.active_view.clone()))
})
.collect()
}
/// If the batch renumbered the grid, invalidate every mirror's cursor **and**
@@ -1590,8 +1646,8 @@ mod tests {
app
}
/// Folding `SwitchView` into the mirror alone moved the client's `active_view`
/// while the workbook's stayed put, so it was projected the other view.
/// A client's grid is built from *its own* view, never the document's default
/// (improvise-jytm). See `context/test-evidence/command.md`.
#[test]
fn a_session_that_switches_views_is_projected_the_view_it_switched_to() {
let mut app = app_with_two_views();
@@ -1603,8 +1659,12 @@ mod tests {
assert_eq!(
sessions.get_mut(id).unwrap().view().active_view,
app.model_state.workbook.active_view,
"the mirror and the state the projection is built from disagree"
"Second",
"the switch did not reach the asker's mirror"
);
assert_eq!(
app.model_state.workbook.active_view, "Default",
"one client's switch moved the document's default"
);
let labels: Vec<_> = sessions
.project_all(&app, dirty)
@@ -1622,25 +1682,28 @@ mod tests {
);
}
/// Lockstep is a property of *every* session, not of the one that asked: a
/// bystander left naming the outgoing view is read under a stale cursor key.
/// The model is shared and the UI is not (improvise-jytm): a bystander keeps
/// its view, its scroll and its grid when somebody else switches.
#[test]
fn a_view_switch_moves_every_sessions_mirror_not_just_the_switchers() {
fn a_view_switch_moves_only_the_switchers_mirror() {
let mut app = app_with_two_views();
let mut sessions = Sessions::default();
let (switcher, _) = sessions.connect(&app, 100, 100);
let (bystander, _) = sessions.connect(&app, 100, 100);
// The bystander is scrolled inside the view the document is about to
// leave, so a stale key is visible rather than coincidentally right.
// The bystander is scrolled, so a mirror reset shows up as a moved
// offset rather than coincidentally matching the origin.
sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
// Drain what the scroll itself owes, so the fan-out below names only
// what the switch caused.
sessions.project_all(&app, Dirty::NOTHING);
let applied = sessions.apply(&mut app, switcher, &Command::SwitchView("Second".into()));
assert_eq!(
sessions.get_mut(bystander).unwrap().view().active_view,
app.model_state.workbook.active_view,
"a bystander's mirror still names the view the document has left"
"Default",
"somebody else's switch moved this client's view"
);
assert_eq!(
sessions
@@ -1649,79 +1712,206 @@ mod tests {
.view()
.cursor()
.row_offset,
0,
"the bystander is positioned in the new view by the old view's scroll"
);
assert_eq!(
to_others(applied),
Some(Command::SwitchView("Second".into())),
"nothing tells the other clients' own slices to follow the switch"
);
}
/// The same lockstep through the model half (improvise-kmgs): `ViewBack` names
/// no view but ends in `App::switch_to_view` just as `SwitchView` does.
#[test]
fn a_model_command_that_moves_the_active_view_binds_every_mirror() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_views();
// Walk forward through the *effect*, which fills the history stack, then
// connect — so the step back is something both mirrors must be told.
apply(
&mut app,
vec![Box::new(crate::ui::effect::SwitchView("Second".into()))],
);
let mut sessions = Sessions::default();
let (walker, _) = sessions.connect(&app, 100, 100);
let (bystander, _) = sessions.connect(&app, 100, 100);
let applied = sessions.apply_caught_up(&mut app, walker, ModelCommand::ViewBack);
assert_eq!(
app.model_state.workbook.active_view, "Default",
"the history step did not reach the document"
);
assert_eq!(
sessions.get_mut(bystander).unwrap().view().active_view,
"Default",
"a bystander's mirror still names the view the document has left"
1,
"somebody else's switch moved this client's scroll"
);
assert_eq!(
diff(applied).1,
vec![Broadcast::ToAll(Command::SwitchView("Default".into()))],
"nothing tells the clients' own slices to follow"
Vec::new(),
"an accepted switch is the asker's own business and relays nothing"
);
let heard: Vec<SessionId> = sessions
.project_all(&app, Dirty::NOTHING)
.into_iter()
.map(|(id, _)| id)
.collect();
assert_eq!(
heard,
vec![switcher],
"a bystander was repainted for a view it is not on"
);
}
/// The asker's own slice owes the switch too, and `ToOthers` never gave it one.
/// `to_others` looks away from the asker by construction — see the evidence.
/// A refused switch is a correction owed to the **asker alone** — the one
/// client that folded it (improvise-6c6q). Driven through the asker's slice.
#[test]
fn a_model_commands_view_switch_reaches_the_asker_too() {
use improvise_protocol::ModelCommand;
fn a_refused_switch_corrects_the_asker_and_nobody_else() {
let mut app = app_with_two_views();
apply(
&mut app,
vec![Box::new(crate::ui::effect::SwitchView("Second".into()))],
);
let mut sessions = Sessions::default();
let (asker, snapshot) = sessions.connect(&app, 100, 100);
let mut client = snapshot.view.clone();
assert_eq!(client.active_view, "Second");
let (bystander, bystander_snapshot) = sessions.connect(&app, 100, 100);
let applied = sessions.apply_caught_up(&mut app, asker, ModelCommand::ViewBack);
host_dispatch(&mut client, &applied, asker, asker);
let mut client = snapshot.view.clone();
// The bystander sits on the OTHER view, so a relay addressed to everyone
// moves it; on the default it would fold to a no-op and prove nothing.
let mut other = bystander_snapshot.view.clone();
let elsewhere = Command::SwitchView("Second".into());
reduce_view(&mut other, &elsewhere);
sessions.apply(&mut app, bystander, &elsewhere);
let asked = Command::SwitchView("Nonexistent".into());
reduce_view(&mut client, &asked);
assert_eq!(client.active_view, "Nonexistent", "the asker folded it");
let applied = sessions.apply(&mut app, asker, &asked);
assert_eq!(
diff(applied.clone()).1,
vec![Broadcast::To(asker, Command::SwitchView("Default".into()))],
"the correction is addressed to somebody other than the asker"
);
host_dispatch(&mut client, &applied, asker);
host_dispatch(&mut other, &applied, bystander);
assert_eq!(
client.active_view, "Default",
"the asker's own slice still names the view the document has left"
"the asker was left naming a view the document does not have"
);
assert_eq!(
client.active_view,
sessions.get_mut(asker).unwrap().view().active_view,
"the asker and the server's mirror of it have drifted"
);
assert_eq!(
other.active_view, "Second",
"a bystander was told about somebody else's refused switch"
);
}
/// The category panel and the tile bar carry a category's **axis**, which is a
/// fact about the view — so they follow the client too (improvise-jytm).
#[test]
fn a_session_on_another_view_is_projected_that_views_axes() {
let mut app = app_with_two_views();
let mut sessions = Sessions::default();
let (id, _) = sessions.connect(&app, 100, 100);
let applied = sessions.apply(&mut app, id, &Command::SwitchView("Second".into()));
let (dirty, _) = diff(applied);
let projections: Vec<Projection> = sessions
.project_all(&app, dirty)
.into_iter()
.flat_map(|(_, p)| p)
.collect();
let axes = |name: &str| {
projections.iter().find_map(|p| match p {
Projection::CategoryTree(CategoryTreeUpdate::Categories(rows)) => Some(
rows.iter()
.find(|row| row.name == name)
.unwrap_or_else(|| panic!("{name} is not in the category tree"))
.axis,
),
_ => None,
})
};
let tile = |name: &str| {
projections.iter().find_map(|p| match p {
Projection::TileBar(tiles) => Some(
tiles
.iter()
.find(|t| t.category == name)
.unwrap_or_else(|| panic!("{name} is not on the tile bar"))
.axis,
),
_ => None,
})
};
// "Second" is "Default" transposed, so reading the host's view instead of
// the session's swaps both of these rather than merely blurring them.
assert_eq!(
(axes("Region"), axes("_Measure")),
(Some(Axis::Column), Some(Axis::Row)),
"the category panel was built from the view this client is not on"
);
assert_eq!(
(tile("Region"), tile("_Measure")),
(Some(Axis::Column), Some(Axis::Row)),
"the tile bar was built from the view this client is not on"
);
}
/// A per-client view can be deleted out from under the client that is on it.
/// The mirror is moved and the client told — see `context/test-evidence/command.md`.
#[test]
fn deleting_a_view_moves_the_client_that_was_on_it_and_tells_it() {
use improvise_protocol::ModelCommand;
let mut app = app_with_two_views();
app.model_state.workbook.create_view("Third");
let mut sessions = Sessions::default();
let (switcher, switcher_snapshot) = sessions.connect(&app, 100, 100);
let (bystander, bystander_snapshot) = sessions.connect(&app, 100, 100);
let mut moved = switcher_snapshot.view.clone();
let switch = Command::SwitchView("Second".into());
reduce_view(&mut moved, &switch);
sessions.apply(&mut app, switcher, &switch);
// The bystander sits on a view the delete does NOT remove, and not on the
// default: a `ToAll` relay would move it, where being on the default hides that.
let mut stayed = bystander_snapshot.view.clone();
let elsewhere = Command::SwitchView("Third".into());
reduce_view(&mut stayed, &elsewhere);
sessions.apply(&mut app, bystander, &elsewhere);
let applied = sessions.apply_caught_up(
&mut app,
bystander,
ModelCommand::DeleteView("Second".into()),
);
assert_eq!(
diff(applied.clone()).1,
vec![Broadcast::To(
switcher,
Command::SwitchView("Default".into())
)],
"the move is addressed to somebody other than the client that lost its view"
);
host_dispatch(&mut moved, &applied, switcher);
host_dispatch(&mut stayed, &applied, bystander);
assert!(
!app.model_state.workbook.views.contains_key("Second"),
"the fixture must actually delete the view, or this proves nothing"
);
assert_eq!(
sessions.get_mut(switcher).unwrap().view().active_view,
"Default",
"a mirror still names a view the document has not got"
);
assert_eq!(
moved.active_view, "Default",
"the client was projected the default view while its own slice named a deleted one"
);
assert_eq!(
stayed.active_view, "Third",
"a client on a surviving view was yanked off it by somebody else's delete"
);
assert_eq!(
sessions.get_mut(bystander).unwrap().view().active_view,
"Third",
"the bystander's mirror was moved off a view the document still has"
);
}
/// A client connecting after somebody else has switched opens the document's
/// default, not that client's view (improvise-jytm).
#[test]
fn a_fresh_session_opens_the_documents_default_view() {
let mut app = app_with_two_views();
let mut sessions = Sessions::default();
let (first, _) = sessions.connect(&app, 100, 100);
sessions.apply(&mut app, first, &Command::SwitchView("Second".into()));
let (_, snapshot) = sessions.connect(&app, 100, 100);
assert_eq!(
snapshot.view.active_view, app.model_state.workbook.active_view,
"a fresh client did not open the document's default view"
);
assert_eq!(snapshot.view.active_view, "Default");
}
/// `reset_cursor` reached only the authoritative `App`'s slice, so every client
@@ -1814,11 +2004,11 @@ mod tests {
let scroll = Command::SetRowOffset(2);
let applied = sessions.apply(&mut app, bystander, &scroll);
reduce_view(&mut client, &scroll);
host_dispatch(&mut client, &applied, bystander, bystander);
host_dispatch(&mut client, &applied, bystander);
assert_eq!(client.cursor().row_offset, 2);
let applied = sessions.apply_caught_up(&mut app, asker, ModelCommand::TransposeAxes);
host_dispatch(&mut client, &applied, bystander, asker);
host_dispatch(&mut client, &applied, bystander);
assert_eq!(
client.cursor().row_offset,
@@ -2010,25 +2200,16 @@ mod tests {
}
}
/// `diff().1` as the tests that predate [`Broadcast`] asked it: what a
/// bystander is told, and nothing about the asker.
fn to_others(applied: Applied) -> Option<Command> {
diff(applied).1.into_iter().find_map(|b| match b {
Broadcast::ToOthers(cmd) => Some(cmd),
Broadcast::ToAll(_) => None,
})
}
/// A stand-in for `Host::command`'s relay. `to` is the client being driven and
/// `from` the one that asked — exactly what [`Broadcast`]'s audience is about.
fn host_dispatch(client: &mut ViewState, applied: &Applied, to: SessionId, from: SessionId) {
/// A stand-in for `Host::command`'s relay. `to` is the client being driven; the
/// audience is read off the [`Broadcast`] and nowhere else.
fn host_dispatch(client: &mut ViewState, applied: &Applied, to: SessionId) {
match applied {
Applied::Diff { broadcast, .. } => {
for item in broadcast {
match item {
Broadcast::ToOthers(cmd) if to != from => reduce_view(client, cmd),
Broadcast::ToOthers(_) => {}
Broadcast::ToAll(cmd) => reduce_view(client, cmd),
Broadcast::To(id, cmd) if *id == to => reduce_view(client, cmd),
Broadcast::To(..) => {}
}
}
}
@@ -2059,7 +2240,7 @@ mod tests {
let scroll = Command::SetRowOffset(3);
let applied = sessions.apply(&mut app, bystander, &scroll);
reduce_view(&mut client, &scroll);
host_dispatch(&mut client, &applied, bystander, bystander);
host_dispatch(&mut client, &applied, bystander);
assert_eq!(client.cursor().row_offset, 3);
assert_eq!(client.active_view, "Default");
@@ -2073,7 +2254,7 @@ mod tests {
app.model_state.workbook.model.name, "Other",
"the load did not reach the document"
);
host_dispatch(&mut client, &applied, bystander, asker);
host_dispatch(&mut client, &applied, bystander);
// The server's mirror self-heals off the generation bump; this is the other
// half, the client's slice, which only a `Snapshot` resets.
crates/improvise-command/src/ui/reduce.rs
+17 -24
View File
@@ -193,8 +193,6 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec<Box<dyn Effect>> {
ModelCommand::SetNumberFormat(fmt) => Box::new(effect::SetNumberFormat(fmt.clone())),
ModelCommand::CreateView(name) => Box::new(effect::CreateView(name.clone())),
ModelCommand::DeleteView(name) => Box::new(effect::DeleteView(name.clone())),
ModelCommand::ViewBack => Box::new(effect::ViewBack),
ModelCommand::ViewForward => Box::new(effect::ViewForward),
ModelCommand::LoadModel(path) => Box::new(effect::LoadModel(path.clone())),
ModelCommand::ImportJsonHeadless {
@@ -236,8 +234,6 @@ pub fn records_named(cmd: &ModelCommand) -> Vec<RecordHash> {
| ModelCommand::SetNumberFormat(_)
| ModelCommand::CreateView(_)
| ModelCommand::DeleteView(_)
| ModelCommand::ViewBack
| ModelCommand::ViewForward
| ModelCommand::LoadModel(_)
| ModelCommand::ImportJsonHeadless { .. } => Vec::new(),
}
@@ -370,14 +366,14 @@ mod tests {
assert_eq!(app.view_state.yanked, Some(CellValue::Number(1.0)));
}
/// The two copies of "which view is active" must move together
/// (gotcha 29), or the cursor map is read under the wrong key.
/// Which view is active is the client's; the document keeps the default a
/// fresh client opens (improvise-jytm, gotcha 29).
#[test]
fn switch_view_moves_the_document_and_the_client_together() {
fn switch_view_moves_the_client_and_leaves_the_documents_default() {
let mut app = app();
reduce_checked(&mut app, &Command::SwitchView("Second".into()));
assert_eq!(app.view_state.active_view, "Second");
assert_eq!(app.model_state.workbook.active_view, "Second");
assert_eq!(app.model_state.workbook.active_view, "Default");
}
/// A wire-driven view switch owes the same `view_back_stack` history a
@@ -386,17 +382,15 @@ mod tests {
fn a_wire_view_switch_records_history_the_way_a_key_switch_does() {
let mut app = app();
reduce_checked(&mut app, &Command::SwitchView("Second".into()));
assert_eq!(app.model_state.workbook.active_view, "Second");
assert_eq!(app.view_state.active_view, "Second");
let back = caught_up(&app, ModelCommand::ViewBack);
reduce_checked(&mut app, &back);
// `ViewBack` is off the wire since improvise-jytm: it pops *this*
// client's history, which is session-local (gotcha 38).
app.apply_effects(vec![Box::new(effect::ViewBack)]);
assert_eq!(
app.model_state.workbook.active_view, "Default",
app.view_state.active_view, "Default",
"the switch pushed nothing onto the back stack"
);
// The two copies move together or the cursor map is read under the
// wrong key (gotcha 29).
assert_eq!(app.view_state.active_view, "Default");
}
/// Switching views changes what the axis-bearing widgets read, so they
@@ -589,8 +583,6 @@ mod tests {
ModelCommand::SetNumberFormat(",.2f".into()),
ModelCommand::CreateView("Third".into()),
ModelCommand::DeleteView("Second".into()),
ModelCommand::ViewBack,
ModelCommand::ViewForward,
ModelCommand::LoadModel(PathBuf::from("doc.improv")),
ModelCommand::ImportJsonHeadless {
path: PathBuf::from("rows.json"),
@@ -625,8 +617,6 @@ mod tests {
ModelCommand::SetNumberFormat(_) => "SetNumberFormat",
ModelCommand::CreateView(_) => "CreateView",
ModelCommand::DeleteView(_) => "DeleteView",
ModelCommand::ViewBack => "ViewBack",
ModelCommand::ViewForward => "ViewForward",
ModelCommand::LoadModel(_) => "LoadModel",
ModelCommand::ImportJsonHeadless { .. } => "ImportJsonHeadless",
}
@@ -887,17 +877,20 @@ mod tests {
}
}
/// `SwitchView` persists as the `Initial View` line, so it dirties the
/// document too — and the worker's persist trigger depends on that.
/// Since improvise-jytm a switch moves no document state, so there is
/// nothing to persist and the worker must not be woken.
#[test]
fn a_wire_view_switch_marks_the_document_dirty() {
fn a_wire_view_switch_leaves_the_document_clean() {
let mut app = app();
assert!(!app.model_state.dirty, "the fixture starts clean");
apply_checked(&mut app, &Command::SwitchView("Second".into()));
assert_eq!(
app.model_state.workbook.active_view, "Second",
app.view_state.active_view, "Second",
"the fixture must actually switch, or this proves nothing"
);
assert!(app.model_state.dirty);
assert!(
!app.model_state.dirty,
"a view switch dirtied a document it did not change"
);
}
}
crates/improvise-protocol/src/reduce.rs
+2 -6
View File
@@ -66,8 +66,8 @@ pub enum Command {
Project(Projection),
}
/// A mutation of the shared document: the 29 `EffectKind::Model(..)` effects
/// minus three, leaving **26**. Derived, not invented — see §2.
/// A mutation of the shared document: the 26 `EffectKind::Model(..)` effects
/// minus two, leaving **24**. Derived, not invented — see §2.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum ModelCommand {
// ── ModelChange::CellData ───────────────────────────────────────
@@ -130,10 +130,6 @@ pub enum ModelCommand {
SetNumberFormat(String),
CreateView(String),
DeleteView(String),
/// Pop the view history. Both move the active view, so both carry a
/// document half exactly as `Command::SwitchView` does (gotchas 29, 36).
ViewBack,
ViewForward,
// ── ModelChange::WholeWorkbook ──────────────────────────────────
/// Paths name documents in the host's `Storage`, never the client's (gotcha 2).
crates/improvise-wasm-client/tests/lockstep.rs
+1 -1
View File
@@ -355,7 +355,7 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() {
let mut client = ClientState {
view: app.view_state.clone(),
tiles: improvise_protocol::TileBarCache {
tiles: tile_entries(&app),
tiles: tile_entries(&app, app.view()),
},
..ClientState::default()
};
crates/improvise-ws-server/src/registry.rs
+9 -17
View File
@@ -444,8 +444,8 @@ mod tests {
// ── the broadcast stays inside its document ─────────────────────────
/// A `SwitchView` is broadcast to the other clients *of that document*; a
/// client elsewhere would fold a view name from a document it is not on.
/// A cursor invalidation reaches the other clients *of that document*; a
/// client elsewhere would blank a cursor into a grid nothing reshaped.
#[test]
fn a_broadcast_does_not_cross_a_document_boundary() {
let mut registry = registry();
@@ -453,11 +453,8 @@ mod tests {
let (same_doc, _) = registry.connect(Some("alpha"), 10, 10).unwrap();
let (other_doc, _) = registry.connect(Some("beta"), 10, 10).unwrap();
let frames = registry.handle(
&asker,
ClientFrame::command(Command::SwitchView("Second".into())),
);
let broadcast = ServerFrame::Commands(vec![Command::SwitchView("Second".into())]);
let frames = registry.model(&asker, ModelCommand::TransposeAxes);
let broadcast = ServerFrame::Commands(vec![Command::ResetCursor]);
assert!(
to(&frames, &same_doc).contains(&&broadcast),
@@ -472,17 +469,12 @@ mod tests {
/// `doctrine(check_the_checker)` for the broadcast test above — see
/// `context/test-evidence/clients.md`.
#[test]
fn the_other_document_has_the_same_view_name_to_fold() {
let registry = registry();
fn the_other_document_would_also_have_a_cursor_to_invalidate() {
let mut registry = registry();
let (elsewhere, _) = registry.connect(Some("beta"), 10, 10).unwrap();
let frames = registry.model(&elsewhere, ModelCommand::TransposeAxes);
assert!(
registry
.document(&DocumentId::new("beta"))
.unwrap()
.app()
.model_state
.workbook
.views
.contains_key("Second"),
to(&frames, &elsewhere).contains(&&ServerFrame::Commands(vec![Command::ResetCursor])),
"the isolation test would pass for the wrong reason"
);
}
src/draw.rs
+2 -5
View File
@@ -365,7 +365,7 @@ fn draw_content(f: &mut Frame, area: Rect, app: &App) {
GridWidget::new(
&grid_cache,
app.cursor(),
&app.model_state.workbook.active_view,
&app.view_state.active_view,
&app.layout,
&app.view_state.mode,
&app.view_state.search_query,
@@ -425,10 +425,7 @@ fn draw_status(f: &mut Frame, area: Rect, app: &App) {
} else {
""
};
let view_badge = format!(
" {}{} ",
app.model_state.workbook.active_view, yank_indicator
);
let view_badge = format!(" {}{} ", app.view_state.active_view, yank_indicator);
let left = format!(" {}{search_part} {msg}", app.view_state.mode.label());
let line = fill_line(left, &view_badge, area.width);