diff --git a/context/code/command.md b/context/code/command.md index 30c5a2a2..fbb0769a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -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. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 28a6935d..d4618e75 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -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 diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index f6da010d..b8622108 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -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 diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 42c303c4..efc7327f 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -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::>() + app.view().category_axes.iter().collect::>() ); 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` \ diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 0649253b..487f1227 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -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), + ("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(); diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index a0ab082d..3401e103 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -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) -> Vec { + fn relay(&self, broadcast: Vec) -> Vec { 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 ─────────────────────────────────────────────── diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index b2f5c85e..d6d466f6 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -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, comma: bool, decimals: u8, ) -> Vec { - 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 { - let layout = &app.layout; +fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option { 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 { /// 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 { +pub fn tile_entries(app: &App, view: &View) -> Vec { 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 { /// 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 { + 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 { 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 { 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, + /// 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, /// 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 { - 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 { @@ -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 = (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 { + 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 = 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 = 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 { - 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. diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index a0c02de1..877a1b7f 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -193,8 +193,6 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { 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 { | 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" + ); } } diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 26774711..6b0a314e 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -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). diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index aa9ea413..f50dc4d4 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -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() }; diff --git a/crates/improvise-ws-server/src/registry.rs b/crates/improvise-ws-server/src/registry.rs index 019f93c3..e59c659e 100644 --- a/crates/improvise-ws-server/src/registry.rs +++ b/crates/improvise-ws-server/src/registry.rs @@ -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" ); } diff --git a/src/draw.rs b/src/draw.rs index b60bbe3d..6f0b6c7f 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -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);