diff --git a/context/adr/002-naming-a-cell.md b/context/adr/002-naming-a-cell.md index f777136c..80626411 100644 --- a/context/adr/002-naming-a-cell.md +++ b/context/adr/002-naming-a-cell.md @@ -226,3 +226,9 @@ learns no coordinate space, and an invalid operation is unconstructible rather than rejected. What the extension adds is that the enum is now **four** answers to "what may be done here", so `None` finally means only what this ADR always said it meant — no projected cell at all. + +**A fifth kind (2026-09-22, improvise-exdg).** The records-mode coordinate cell +was the last `None` that named a visible cell. It is now `CellId::Coordinate`. +The variant carries no operand, because no client composes a coordinate rename. +The previous paragraph's "four answers" is therefore five. `None` now means +only a padded or blanked slot. Reasoning: `context/code/protocol.md` §3. diff --git a/context/code/clients.md b/context/code/clients.md index f7ef1f19..06e08949 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1746,6 +1746,31 @@ watchdog must be DISARMED and not merely pre-empted**: it fires later and would overwrite the better message with "did not answer" (PR #13 review). The watchdog is for silence only, and `setShellFailure` clears the timer. +**Success is claimed by the first frame, and only if nothing failed first** +(improvise-i0v9). The worker transport used to write `in-page worker · ` +the moment it spawned the worker — 13ms after navigation, before anything had +answered — so a worker that could not start showed a success-looking status +over a blank page. It now writes `starting the worker · …`, and +`createShell().arm(message, { answer })` holds the claim until `sawFrame`. The +rule the claim obeys is **the watchdog's own state**: it is written only if the +watchdog is still armed when the first frame lands. That is not a convenience. +`worker.js` posts a `ReadOnly` (or `SeedMissing`) notice straight after +`start()` and **before** the Snapshot that answers the handshake, so an +unconditional "first frame ⇒ say we are up" would overwrite the precise reason +the notice just wrote. `fail` already disarms the watchdog, so "armed" answers +"has anything been said since arming" without a second flag to keep in step. +`fail` disarms **directly** rather than through `sawFrame`, or it would +briefly flash the success text on its way to the failure. The starting text +deliberately does not contain `in-page worker`, so the older specs asserting +that substring now need the first frame to pass. The socket path passes no +`answer`: `open` is already a real fact, and its status was never premature. + +One consequence changes what the watchdog guards. In the read-only case a +Snapshot always follows the notice, and `sawFrame` disarms the watchdog either +way, so a notice that forgot to disarm would **not** be overwritten eight +seconds later. It is overwritten **at once**, by the claim. The eight-second +overwrite survives only where no frame follows: `BootFailed`. + **Did this socket ever open?** The page has to record that itself, because nothing on the close event carries it: `readyState` is already CLOSED by the time a close listener runs, whichever happened, and a refused connection and a diff --git a/context/code/command.md b/context/code/command.md index 3dd16fd3..a943e8bf 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -793,12 +793,40 @@ is gone rather than joined by a second one. A guard spelled here as copy of `switch_to_view`'s own condition, free to disagree with it the day that condition grows a clause. -Not fixed, and reachable only by construction: `ViewBack` and `ViewForward` call -`switch_to_view` just as unguardedly, pushing the current view onto the opposite -stack before the switch they cannot be sure of. They are safe **because -`ViewState::forget_view` purges both stacks when a view is deleted**, so a popped -frame always names a live view — a guard living one level away from the hazard, -which is the shape that keeps returning here. +`ViewBack` and `ViewForward` have the same shape now (improvise-8nd9). They used to +push the current view onto the opposite stack **before** a switch they could not be +sure of, and were safe only because `ViewState::forget_view` purges both stacks when +a view is deleted — a guard living one level away from the hazard. improvise-lrrx +proved the guard incomplete: `App::install_workbook` (`LoadModel`, +`AcceptWizardModel`) cleared `cursors` and not the history, so `<` after a load +walked the **outgoing** document's frames, landing on a view of the same name in the +incoming one that this client had never visited, or on a refused switch that still +left a phantom frame behind. Both halves are fixed, so neither relies on the other: +`install_workbook` clears both stacks beside `cursors`, for the same reason, and the +two steps go through `stepped_to`, which switches, observes whether the document +**accepted** the view (`App::switch_to_view` now returns that), and only then +restores the frame's mode and hands back the frame just left. A frame the document +refuses is consumed and leaves nothing. + +The observation is *accepted*, not *moved*, and that is the one place the sibling +shapes differ. `SwitchView` asks "did `active_view` change" because asking for the +view you are on is not a move. A history frame is a **place**, meaning a view and a +mode, so a live frame naming the view you are on still restores its mode. Such +frames exist: `forget_view` collapses `[A, B, A]` to `[A, A]`. The client's +`reduce_view::stepped` also steps onto them unconditionally. A "moved" test +would therefore drop a frame the client keeps, and the host and the client +would disagree. The first version of this fix made exactly that mistake. +`colon_view_back_respects_whether_history_exists` caught it, because its +fixture is such a frame. `reduce_view`'s `stepped` is unchanged, because the +client holds no `Workbook` to observe against. That leaves the host and the +client able to disagree wherever a client's stack holds a frame the document +would refuse. An earlier version of this paragraph said such a frame could not +exist, because `ForgetView` purges deleted views and a load arrives as a fresh +`Snapshot`. **That is false.** `project.rs::fresh_view` starts from +`ViewState::default()` and folds `SwitchView(active)`, so every connected or +rebased client gets a phantom `{Default, Normal}` back-frame, and `Default` +need not exist in the document. That is improvise-lxg0, and until it is fixed +the divergence is live rather than theoretical. ### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc) @@ -2846,6 +2874,43 @@ flag: tidy-up goes where an abort cannot reach it. stays in `TileSelect` mode so the user can adjust several tiles. `axis: None` cycles; `axis: Some(a)` sets. +**The tile bar offers `_Index` and `_Dim`, and that is deliberate (improvise-jgzw).** +Every tile call site passes `Model::category_names()`, virtual categories included, +so tile cursor 0 is `_Index`. The ticket asked whether that was a leak. It is not, +for three reasons, each checked rather than argued: + +- **It is a decision the user already made.** improvise-kos: "`_Dim` and `_Index` + should work as pivot axes outside Records/Drill view". A category that works as a + pivot axis is one the axis picker offers. +- **The tile bar is the only way out of a records layout that was saved.** + improvise-jgzw's "check what it does" found that hand-assigning `_Index` → Row and + `_Dim` → Column in a real view (tile bar or `:set-axis`) is **written** to the + `.improv`, because a real view persists every axis, and it reopens as records mode. + (`VIRTUAL_VIEW_NAMES`' "never-persisted `_Index`/`_Dim` on axes" is true only of + `_Records`/`_Drill`.) In that document, `R` (`toggle-records-mode`) emits + `ViewBack` on an empty back stack, so it leaves the layout as it was and sets the + status to "Pivot mode". That is filed as improvise-gj5p. Moving the two axes back through + the tile bar is what gets out. In the browser, which declines + `toggle-records-mode`, the tile bar is also the only way in, and improvise-bd9o + already made that route safe instead of closing it + (`the_tile_bar_route_into_records_mode_does_not_strand_the_user`). +- **What filtering would cost was measured.** With `View::tileable_cats` filtering + on `CategoryKind::is_index_or_dim`, that bd9o test and the TUI's + `renders_each_tile_with_its_axis_label` fail, as well as the two characterization + tests here. + +What the default cursor does cost, stated: one cycle key on tile 0 puts `_Index` +on Row alone, and in a pivot view that empties the grid (0 rows on +`web/demo.improv`). The status line says `_Index → Row`, and three more presses +undo it. If that turns out to matter, the fix is the order the tile bar lists +categories in, not taking entries out of it. + +**`_Measure` must stay tileable.** It is on an axis in every shipped document, so +`regular_category_names()`, the advice for *prompts*, is the wrong filter here. +`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included` +names the whole offered list with `_Measure` on a column, and fails if either +decision changes. + ### `cmd/effect_cmds.rs` The parseable model-mutation commands are thin `Cmd` wrappers around effects, diff --git a/context/code/protocol.md b/context/code/protocol.md index 3c5cc69e..f4cfb754 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -614,14 +614,37 @@ broader than its reason — improvise-gcvb, which also records the two things that make it more than a one-liner: an inserted `+cat` column means the TUI does **not** advance, and `AddCategory` can abort. +### `CellId::Coordinate` — a drawn coordinate is not "no cell" (improvise-exdg) + +A records-mode coordinate cell (the `Region` column holding `North`) used to +project `None`. That was a **third** meaning of `None`, next to the padded slot +and the blanked position: a visible cell that holds text and whose write +belongs to the host. So absence was encoding a permission, the same conflation +`Create` removed for the creating columns (improvise-poia). The pinning test, +`a_coordinate_column_is_projected_with_no_identity`, asserted the bug and is +gone. + +`Coordinate` carries **nothing**. Editing a coordinate renames it across every +measure cell in the row (gotcha 1), and no client composes that operation: it +needs `record_col`, and the browser declines every records-mode edit before +reaching a cell. Giving the variant a `RecordHash` would hand a client a +surrogate with nothing to spend it on. The operand belongs with the command +that consumes it, whenever that command exists. Nothing a client can do has +changed. `render.rs::editable` is now an exhaustive `match` rather than a +`matches!`, so the next variant has to be classified. `typed_cell_command`, +`record_at_cursor` and `record_cell_at_cursor` still answer `None` for it. +So paste onto a coordinate is still a silent `Upstream` in the browser. The +TUI has declined it out loud since PR #184. That divergence is +improvise-kkcy, and a named kind is what lets the browser decline deliberately +now. + ### `CellDisplay::id` — `None` is not "aggregate" That is `Some(CellId::Aggregate(..))`, and the distinction is the whole point of the enum. `None` means the slot holds **no projected cell**: a `GridCache` -position padded by `fit_to_labels`, blanked by a reframing, or a -records-mode column that is not a cell to write into at all (a category column -renames a coordinate across the whole row, gotcha 1, which is a different -intent from `SetCell`). Conflating the two either hides a cell or offers a +position padded by `fit_to_labels`, or one blanked by a reframing. Every +records-mode column is some kind of cell (`Record`, `Derived`, `Create` or +`Coordinate`). Conflating the two either hides a cell or offers a write the type system exists to make unconstructible. ### `CellDisplay::edit_text` — the difference corrupts data @@ -637,8 +660,8 @@ and an editor's seed is exactly a feedback path. It is the projection of `App::display_value_at_cursor`, which is what `EditOrDrill` seeds the TUI's buffer from, so both front ends start from one string rather than each deriving one. `None` where there is nothing to edit: -the same set of positions `id` is `None` for, plus cells whose value is -absent. An error cell carries none, because its text is a diagnostic and not +positions whose `id` is `None`, plus pivot cells with no key. A records-mode +cell always carries one, even an empty one. An error cell carries none, because its text is a diagnostic and not a value. A browser found this: `tests/lockstep.rs` drove `i` through the host and the @@ -785,9 +808,16 @@ one**. `AxisFrame::flat` leaves `cats` empty on purpose, so it is not a way to smuggle a multi-level axis in flattened. `FrameGroup::collapsed` is on the wire rather than left to the client because -it is a fact about the *view*, which only the server holds. `FrameGroup::cat` -travels so a client can echo it back in `ToggleGroup` without ever working out -which category a label came from — which slash-joined labels made impossible. +it is a fact about the *view*, which only the server holds. There is no +`FrameGroup::cat` any more (improvise-7xyx). Its doc comment promised that a +client echoes it back in `ToggleGroup`, but no client ever did. `resolve.rs` +has no `toggle-group-under-cursor` arm (`z` is on the still-needs-the-host +list), and neither renderer read the field. A promise about an interaction no +code performs is the stale-sibling defect, so the field went with it. It is +not a loss for `z`. The TUI's `ToggleGroupAtCursor` also places the cursor on +the layout the toggle *will produce*, which a thin client cannot compute. So +composing `z` in a browser needs its own design, and the operand should come +back with that design rather than wait for it. ### `AxisFrame`: four accessors that are **not for a draw loop** @@ -1294,13 +1324,13 @@ frozen behaviour reads as decided afterwards and nothing says it was only frozen for the duration of the move. **The ticket's own proposed fix does not work, and that is worth more than the -fix.** h0iv suggested comparing the `(cat, name)` pair, `FrameGroup` carrying -`cat` precisely so a client need not work out which category a label came from. +fix.** h0iv suggested comparing the `(cat, name)` pair; `FrameGroup` carried +`cat` then (improvise-7xyx has since removed it). But its own reachability case is *one* category: `Month` grouped into `Q1`, nested under `Region`, giving `North/Q1` and `South/Q1` adjacent — the two headers are `(Month, Q1)` and `(Month, Q1)`, **equal as values**, so the pair -separates nothing. `the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them` -pins that, so the next reader does not re-propose it. What separates them is +separates nothing. `the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them` +pins that as whole-value equality, so the next reader does not re-propose it. What separates them is that they are two different `FrameEntry::Group` entries, and `groups()` already borrows each from `self.entries` — so the comparison is `std::ptr::eq` on the header, which is the literal statement of "the same header opened both". diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 9333ba89..144be8a5 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3054,6 +3054,23 @@ The third is the gotcha 48 property, and it needs `timesArmed() > 0` first or it denies an unreachable event: the precise reason must still be on screen after the 8s window, not replaced by the watchdog. `page.clock` walks past it. +**Strengthened by improvise-i0v9's second pass.** Every assertion above read the +*final* `#status`, so two failures could still pass it. A watchdog that fired +first and was then replaced by a late notice passed. So did a success claim +shown before the notice. The test now records every status with `logStatus` and +asserts, **before** the page clock is walked, that the log ends in `read-only` +and never contained `did not answer` or the exact `in-page worker · `. A +reason that arrived after the watchdog would sit in that log behind "did not +answer"; that is the "well before 8s" property, stated without a wall-clock +bound. Measured failing against `gh/main` at `da2e5ab`, whose log was +`["in-page worker · two-tab-….improv", "read-only — …"]`: the premature claim +the ticket measured at 13ms is still there once 1wft's notice exists. The +brief's rival, `onNotice` calling `setShell` instead of `fail` (the watchdog not +disarmed), fails it at the first `toContainText("read-only")`. The Snapshot's +claim replaced the reason at once, which is why the eight-second form of this +rival is now exercised by the invalid-`?doc=` spec instead (see +`context/code/clients.md`, "The watchdog"). + It runs on Chromium and Firefox — the two engines with OPFS — rather than Chromium alone, because the exclusivity semantics are the engine's and the two word the refusal completely differently. @@ -4473,6 +4490,38 @@ registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on `+`, which is why that filter was removed rather than kept. The evidence entries for the host-side tests are in `command.md`. +### the worker is not called up until its first frame has arrived + +improvise-i0v9. The worker transport wrote `in-page worker · ` when it +spawned the worker, before anything had answered. A worker that then failed +therefore showed a success-looking status over a blank page. The spec records +every `#status` with `logStatus` and requires the whole log to be exactly +`["starting the worker · …", "in-page worker · "]`. It pins the order +as well as the final text, since a final read cannot see a claim made too early. +Measured failing against `gh/main` at `da2e5ab`. The rival, restoring the +immediate `setShell(\`in-page worker · ${DOC}\`)` in `workerTransport`, fails it +on the log's first entry. The final read is `toHaveText`, not `toContainText`. +The older specs' `toContainText("in-page worker")` could not tell the starting +text from the claim if the one contained the other, and the starting text +deliberately does not. + +### only the first frame claims the peer answered, and not before it + +improvise-i0v9, the unit half of the spec above. `arm(message, { answer })` +must write nothing until `sawFrame`, then write `answer` once, however many +frames follow. Fails against `gh/main`'s `createShell`, which has no `answer` +and so never writes the claim at all. + +### a reason given before the first frame is not replaced by the frame's claim + +The order `worker.js` really produces on a locked document: the `ReadOnly` +notice is posted after `start()` and **before** the Snapshot. So `fail` then +`sawFrame` must leave the notice's text as the only thing ever shown. The rival +is the obvious fix, `sawFrame` writing `answer` unconditionally. It fails both +this test and the one above: the first on its exact log, which is then written +twice. This test passes against `gh/main` by construction, since no claim exists +there. It is not evidence of the bug; it guards against the naive fix. + ## Numbers across the serialized wire (improvise-6oy4) No manifest enabled serde_json's `float_roundtrip`, so its default f64 parser diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d86e0500..f085ebb4 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1978,12 +1978,27 @@ Two measure columns of one row are two cells of one record, and the next row is different record — the same shape the pivot case has, checked here because records mode reaches it by a different path. -### a_coordinate_column_is_projected_with_no_identity +### a_coordinate_cell_is_named_rather_than_left_as_no_cell -A coordinate column is not a cell to write into: editing it renames that coordinate -across *every* measure cell in the row (gotcha 1), which is a different intent from -`SetCell`. So it is projected with no id at all, and a client cannot compose a -write against it. +improvise-exdg. This entry replaces `a_coordinate_column_is_projected_with_no_identity`, +a test that **pinned the bug**. It asserted that a records-mode coordinate cell +projects `id: None`. ADR 002 and gotcha 42 define `None` as "no projected cell", +yet the Region column is drawn and holds text. So a client could not tell it +from a padded slot. Absence was being used to encode a permission ("the host's"), +which is the conflation ADR 002's rejected `Option` draft was about. +`improvise-poia` removed the same conflation for the creating columns. + +The cell now projects `CellId::Coordinate`. That keeps the old test's real +property: it is still not a `SetCell` target, because `typed_cell_command`, +`record_cell_at_cursor` and `render.rs::editable` all match `Record` alone. +The test seats the cursor on the **second** row, so the first-row and +first-item cases cannot coincide. It then enumerates every cell of the records +grid and requires each to carry an id, which is the ticket's claim with no +case picked by hand. Before the fix it failed at `a drawn coordinate projected +as no cell`. Rival injected afterwards: `RecordCol::Category(_) => return None` +restored. This test failed at the `Some(CellId::Coordinate)` assertion, and +`a_creating_column_is_named_rather_than_left_as_no_cell` (whose `None`-count +now expects zero) failed too. ### a_write_moves_the_version_under_the_clients_own_surrogate @@ -3298,11 +3313,12 @@ aggregates, arriving a third time. The test walks **every** column of a records grid and counts, rather than asserting the two creating columns alone. That matters in the direction a -weaker test fails: a producer that answered `Create` for everything, or that -made the `Region` coordinate column nameable, would pass a two-position check -and fails this one. The fault injected — returning `None` for both creating -columns, which is the pre-fix code — reports three nameless columns where one -is allowed. +weaker test fails: a producer that answered `Create` for everything would pass +a two-position check and fails this one. The fault injected — returning `None` +for both creating columns, which is the pre-fix code — reported three nameless +columns where one was allowed. Since improvise-exdg the coordinate column is +named too (`CellId::Coordinate`), so the count of nameless columns is now zero. +See `a_coordinate_cell_is_named_rather_than_left_as_no_cell`. ### a_records_coordinate_is_text_however_it_is_spelled @@ -5134,6 +5150,57 @@ the list. Its seed is exact (`"007"`), but `typed()` classifies the committed text as `Number(7.0)`. Probed here: both tests fail on it in both modes. That is the classifier, not the seed, and it is improvise-ixxx. +### loading_a_document_forgets_the_outgoing_documents_view_history + +improvise-lrrx: `App::install_workbook` cleared `cursors` but not +`view_back_stack`/`view_forward_stack`, so `<` after `LoadModel` walked the +outgoing document's views inside the incoming one. The history is two deep +(`Default`, `X`, then on `Z`), and the incoming document **has** a view named `X` +but opens on `Y`. Without that, a stale frame is a refused switch and looks the +same as "nothing happened" (the ticket's own fixture warning). Against the unfixed +code, `<` landed on `X`, a view this client never visited in the new document. +Rival injected: the two `clear()` lines removed from `install_workbook`, with the +improvise-8nd9 fix still in place. This test and its wizard twin failed at the +`view-back walked the outgoing document's history` assertion. + +### accepting_an_import_forgets_the_outgoing_documents_view_history + +improvise-lrrx, through the second door into `install_workbook`. A +wizard-built workbook has only `Default`, so the test creates an `X` in it after +accepting. Without that, the stale `X` frame would be refused and could not be +told apart from a cleared history. Same rival and same failure as the load test. + +### a_step_back_the_document_refuses_leaves_no_frame_behind + +improvise-8nd9: `ViewBack` pushed the current view onto the forward stack before +the guarded `switch_to_view`, so a frame naming a view the document lacks left a +phantom behind, and `,` would then "go forward" to where it already was. The +dead frame is pushed by hand, standing for any path that forgets +`forget_view`, with a live frame under it and the frame's mode set to `Help` so +that a restored mode is observable. Two rivals. First, the original code, which +pushes first and restores the mode unconditionally: it fails at `nothing moved, +so no mode`. Second, a push-first version that restores the mode only on a move, +so the mode assertion cannot hide the stack assertion: it fails at `a refused +step left X on the forward stack`. The last two assertions check that the live +frame beneath still works, so a fix that drops the whole stack cannot pass. + +### a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds + +improvise-8nd9. This test holds the fix to the client's `reduce_view::stepped`. +A live frame naming the view you are on (a `forget_view` collapse leaves one) +is still a step. It restores its mode and leaves a forward frame, exactly as +the client folds it. Rival injected: `stepped_to` observing whether +`active_view` *changed* rather than whether the document *accepted* the view. +That was the first version of this fix. This test failed at the +mode assertion, as did +`command::cmd::text_buffer::tests::colon_view_back_respects_whether_history_exists`. +That test's fixture is such a frame, and it is what caught the first version. + +### a_step_forward_the_document_refuses_leaves_no_frame_behind + +The `ViewForward` half of the test above, with the stacks' roles swapped. It fails +against the unfixed code at `nothing moved, so no mode`. + ### yy_then_p_in_records_mode_copies_between_real_records improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None, @@ -5246,3 +5313,27 @@ refusal is recorded and the mode is not `Quit`. `import-json` failed first, before `refuse_and_stop` was applied to its arms. For `load`, the injected fault is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test then fails with "`load absent.improv . q` quit having loaded nothing". + +## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw) + +### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included + +This test pins a decision, not a bug. The argument is in `context/code/command.md` +under `cmd/tile.rs`. Before this test, no tile assertion named the whole offered +set. The two older ones named only the category at cursor 0, so a filter that +changed what the bar offers past the cursor was invisible. The fixture puts +`_Measure` on **Column**, as every shipped document does. The test then walks the +cursor past the end and asserts the exact list +`[_Index, _Dim, _Measure, Type, Month]`. + +Rivals, injected, full workspace run: + +- **`regular_category_names()` in `TileAxisOp`**, the obvious "no virtual + categories in pickers" fix, which drops `_Measure` too. It fails this test + (`["Type", "Month"]`), the two characterization tests above it, and + `lockstep.rs::a_locally_resolved_tile_axis_change_agrees_with_the_host`. +- **`View::tileable_cats` filtering `is_index_or_dim`**, the ticket's option (b). + With the list asserted this test fails by construction, so the point is what + *else* fails: `renders_each_tile_with_its_axis_label` (TUI) and + `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one + is improvise-bd9o's own test of the route this decision keeps. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index a0e901ef..e1dc17a6 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -557,7 +557,7 @@ the evidence that nothing was relying on the permissive parse. ### render::tests::two_runs_under_two_headers_of_one_name_are_two_runs -### render::tests::the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them +### render::tests::the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them improvise-h0iv. `group_runs` coalesced adjacent positions by group **name**, so two runs under two different headers sharing a name read as one and both front @@ -571,10 +571,12 @@ and nothing ungrouped between the runs. Before the fix it returns at the precondition rather than pass with a single run. The second test exists because the ticket proposed the wrong fix and a reader -will propose it again. h0iv says `FrameGroup` carries `cat` and "comparing the -pair would fix it" — but its own reachability case is a single category nested -under an outer one, so both headers are `(Month, Q1)`, equal as values. The test -asserts that equality directly. What actually separates them is that they are +will propose it again. h0iv said `FrameGroup` carried `cat` and that "comparing +the pair would fix it". But its own reachability case is a single category +nested under an outer one, so both headers are `(Month, Q1)`, equal as values. +The test asserts that equality directly. Since improvise-7xyx removed +`FrameGroup::cat`, it asserts equality of the whole header, which is stronger: +no field-wise comparison can separate two equal values. What actually separates them is that they are two distinct entries in `self.entries`, which `std::ptr::eq` on the borrowed header states exactly. diff --git a/crates/improvise-command/src/command/cmd/tile.rs b/crates/improvise-command/src/command/cmd/tile.rs index 1ff05418..c4a71cf9 100644 --- a/crates/improvise-command/src/command/cmd/tile.rs +++ b/crates/improvise-command/src/command/cmd/tile.rs @@ -59,6 +59,26 @@ mod tests { assert!(effects.is_empty()); } + /// improvise-jgzw: `_Index`/`_Dim` stay tileable on purpose, and `_Measure` on + /// a column must too — `context/test-evidence/command.md`. + #[test] + fn the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included() { + let mut m = two_cat_model(); + m.active_view_mut().set_axis("_Measure", Axis::Column); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + let mut offered = Vec::new(); + for idx in 0..8 { + ctx.tile_cat_idx = idx; + let dbg = effects_debug(&TileAxisOp { axis: None }.execute(&ctx)); + if let Some(rest) = dbg.split("CycleAxis(Active, \"").nth(1) { + offered.push(rest.split('"').next().unwrap_or_default().to_string()); + } + } + assert_eq!(offered, ["_Index", "_Dim", "_Measure", "Type", "Month"]); + } + #[test] fn move_tile_cursor_right() { let m = two_cat_model(); diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 565280a6..4969f2cb 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -444,6 +444,9 @@ impl App { pub fn install_workbook(&mut self, workbook: Workbook) { self.view_state.active_view = workbook.active_view.clone(); self.view_state.cursors.clear(); + // The history names the outgoing document's views too (improvise-lrrx). + self.view_state.view_back_stack.clear(); + self.view_state.view_forward_stack.clear(); self.model_state.workbook = workbook; // Every client's mirror is scoped to the outgoing document too, and an // effect cannot reach the session list. The generation is how it hears. @@ -581,12 +584,14 @@ impl App { .flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c))) } - /// 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.views.contains_key(name) { + /// Point **this client's** slice at `name`, if the document has such a view, + /// and say whether it did. The document's default stays put (gotcha 29). + pub fn switch_to_view(&mut self, name: &str) -> bool { + let accepted = self.model_state.workbook.views.contains_key(name); + if accepted { self.view_state.active_view = name.to_string(); } + accepted } /// What `name` names in this document, falling back to the default for a name diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 9bc78b34..18528e69 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -388,19 +388,30 @@ impl Effect for SwitchView { } } +/// Switch to `frame`'s view and restore its mode, returning the frame just left +/// — or `None`, touching nothing, if the document refused the view. +fn stepped_to(app: &mut App, frame: ViewFrame) -> Option { + let left = ViewFrame { + view_name: app.view_state.active_view.clone(), + mode: app.view_state.mode.clone(), + }; + app.switch_to_view(&frame.view_name).then(|| { + app.view_state.mode = frame.mode; + left + }) +} + /// Go back in view history (pop back stack, push current to forward stack). #[derive(Debug)] 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.view_state.active_view.clone(); - app.view_state.view_forward_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // Observed, as `SwitchView` is: a frame the document refuses is dropped + // and leaves nothing behind (improvise-8nd9, `context/code/command.md`). + if let Some(frame) = app.view_state.view_back_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_forward_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -413,14 +424,11 @@ impl Effect for ViewBack { 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.view_state.active_view.clone(); - app.view_state.view_back_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // The mirror of `ViewBack`, observed the same way (improvise-8nd9). + if let Some(frame) = app.view_state.view_forward_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_back_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -1925,6 +1933,169 @@ mod tests { assert_eq!(app.view_state.active_view, before); } + /// A history two deep through `X` and `Z`: back is `[Default, X]`. + fn app_with_history(storage: Box) -> App { + let mut wb = Workbook::new("Outgoing"); + wb.create_view("X"); + wb.create_view("Z"); + let mut app = App::with_storage(wb, None, storage); + for name in ["X", "Z"] { + SwitchView(name.to_string()).apply(&mut app); + } + assert_eq!(app.view_state.view_back_stack.len(), 2, "setup: two deep"); + app + } + + /// Asserts `<` and `,` leave `view` put and the history empty. + fn assert_history_went_with_the_document(app: &mut App, view: &str) { + assert_eq!(app.view_state.active_view, view, "setup: opened on {view}"); + for (name, step) in [ + ("view-back", Box::new(ViewBack) as Box), + ("view-forward", Box::new(ViewForward)), + ] { + step.apply(app); + assert_eq!( + app.view_state.active_view, view, + "{name} walked the outgoing document's history in the incoming one" + ); + } + assert!(app.view_state.view_back_stack.is_empty()); + assert!(app.view_state.view_forward_stack.is_empty()); + } + + /// improvise-lrrx. Evidence: `context/test-evidence/command.md`. + #[test] + fn loading_a_document_forgets_the_outgoing_documents_view_history() { + use crate::storage::{MemoryFs, Storage}; + let fs = MemoryFs::new(); + // The incoming document HAS a view named `X`, and opens somewhere else. + let mut incoming = Workbook::new("Incoming"); + incoming.create_view("X"); + incoming.create_view("Y"); + incoming.active_view = "Y".to_string(); + let text = crate::persistence::format_md(&incoming); + fs.write(std::path::Path::new("b.improv"), text.as_bytes()) + .unwrap(); + let mut app = app_with_history(Box::new(fs)); + + LoadModel("b.improv".into()).apply(&mut app); + assert_eq!( + app.model_state.workbook.model.name, "Incoming", + "setup: loaded" + ); + assert_history_went_with_the_document(&mut app, "Y"); + } + + /// improvise-lrrx: the wizard is the other door into `install_workbook`. + #[test] + fn accepting_an_import_forgets_the_outgoing_documents_view_history() { + let mut app = app_with_history(Box::new(crate::storage::MemoryFs::new())); + let mut wizard = crate::import::wizard::ImportWizard::new(serde_json::json!([ + {"region": "East", "revenue": 100.0}, + {"region": "West", "revenue": 200.0}, + ])); + wizard.advance(); + wizard.advance(); + wizard.build(); + assert!(wizard.built_summary().is_some(), "setup: a built workbook"); + app.wizard = Some(wizard); + + AcceptWizardModel.apply(&mut app); + // The new document gains an `X` of its own, so a stale frame could land. + app.model_state.workbook.create_view("X"); + let opened = app.view_state.active_view.clone(); + assert_ne!(opened, "X", "setup: the import must not open on X"); + assert_history_went_with_the_document(&mut app, &opened); + } + + /// improvise-8nd9: a frame naming a view the document lacks is refused, and + /// a refused step leaves no frame behind. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + // A path that forgot `forget_view`: the history names a dead view. + app.view_state.view_back_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "X", + "setup: the step is refused" + ); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_forward_stack.is_empty(), + "a refused step left X on the forward stack: `,` would go nowhere" + ); + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "Default", + "the live frame survives" + ); + assert_eq!(app.view_state.view_forward_stack.len(), 1); + } + + /// improvise-8nd9: a live frame naming the view you are on is still a step, as + /// it is in the client's `reduce_view`. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + app.view_state.view_back_stack.push(ViewFrame { + view_name: "X".to_string(), + mode: AppMode::Help, + }); + let mut mirror = app.view_state.clone(); + + ViewBack.apply(&mut app); + improvise_protocol::reduce_view(&mut mirror, &improvise_protocol::Command::ViewBack); + assert_eq!( + app.view_state.mode, + AppMode::Help, + "the frame's mode is restored" + ); + assert_eq!(app.view_state.view_back_stack, mirror.view_back_stack); + assert_eq!(app.view_state.view_forward_stack, mirror.view_forward_stack); + assert_eq!(app.view_state.mode, mirror.mode); + } + + /// improvise-8nd9: the forward half of the step above. + #[test] + fn a_step_forward_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + ViewBack.apply(&mut app); + app.view_state.view_forward_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "Default", "setup: refused"); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_back_stack.is_empty(), + "a refused step left Default on the back stack: `<` would go nowhere" + ); + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "X", "the live frame survives"); + assert_eq!(app.view_state.view_back_stack.len(), 1); + } + #[test] fn create_and_delete_view() { let mut app = test_app(); diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 2a433d94..ff23ca77 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -222,7 +222,6 @@ fn axis_frame( pending.clear(); } pending.push(FrameEntry::Group(FrameGroup { - cat: cat_name.clone(), name: group_name.clone(), collapsed: view.is_group_collapsed(cat_name, group_name), })); @@ -324,7 +323,7 @@ fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option return Some(CellId::Create(CreateKind::Category)), // Renaming a coordinate moves every measure cell in the row, which is a // different intent from `SetCell` and still the host's (gotcha 1). - RecordCol::Category(_) => return None, + RecordCol::Category(_) => return Some(CellId::Coordinate), }; return Some(CellId::Record(RecordCell { record: RecordHash::of(record), @@ -493,7 +492,10 @@ pub fn published_basis(app: &App, grid: &GridCache) -> Basis { .flatten() .filter_map(|cell| match cell.id.as_ref()? { CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, }) .collect(); basis::published(app.document_generation(), app.layout_version(), records) @@ -4853,20 +4855,6 @@ mod cell_identity { ); } - /// A coordinate column is not a cell to write into (gotcha 1), so it is projected - /// with no id and a client cannot compose a write against it. - #[test] - fn a_coordinate_column_is_projected_with_no_identity() { - let app = records_app(); - let region = column_named(&app, "Region"); - assert_eq!( - grid_of(&app, WHOLE) - .cell(0, region) - .and_then(|c| c.id.clone()), - None - ); - } - /// A document where `Profit` is BOTH an explicit `_Measure` item and a /// formula target — which the `+` column lets a user build. fn shadowed_measure_records_app() -> App { @@ -5138,10 +5126,9 @@ mod cell_identity { let ids: Vec> = (0..app.layout.col_count()) .map(|c| grid.cell(0, c).and_then(|cell| cell.id.clone())) .collect(); - assert_eq!( - ids.iter().filter(|id| id.is_none()).count(), - 1, - "only the one Region coordinate column may be nameless: {ids:?}" + assert!( + ids.iter().all(Option::is_some), + "no records column is nameless since improvise-exdg: {ids:?}" ); assert_eq!( ids.iter() @@ -5162,6 +5149,30 @@ mod cell_identity { ); } + /// improvise-exdg: a coordinate cell is drawn and holds text, so it is not + /// "no cell". `context/test-evidence/command.md`. + #[test] + fn a_coordinate_cell_is_named_rather_than_left_as_no_cell() { + let app = records_app(); + let grid = grid_of(&app, WHOLE); + let region = column_named(&app, "Region"); + let north = grid.cell(1, region).expect("the second row's coordinate"); + assert_eq!(north.text, "North", "setup: a visible coordinate cell"); + assert_eq!( + north.id, + Some(CellId::Coordinate), + "a drawn coordinate is named" + ); + let nameless: Vec<(usize, usize)> = (0..app.layout.row_count()) + .flat_map(|r| (0..app.layout.col_count()).map(move |c| (r, c))) + .filter(|&(r, c)| grid.cell(r, c).is_some_and(|cell| cell.id.is_none())) + .collect(); + assert!( + nameless.is_empty(), + "every records cell is something: {nameless:?}" + ); + } + /// The version counter is keyed by the surrogate the client holds, so one /// identity answers ADR 002's question and ADR 001's. #[test] diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 647e9bb1..6397136e 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -55,6 +55,9 @@ pub enum CellId { /// A records-mode creating column: typing a **name** into it adds that /// measure or category and writes no cell at all (gotcha 1, improvise-poia). Create(CreateKind), + /// A records-mode coordinate. Renaming it moves every measure cell in the + /// row (gotcha 1), which no client composes: `protocol.md` §3 (improvise-exdg). + Coordinate, } /// What a creating column creates. The name typed into it is the whole @@ -194,9 +197,6 @@ pub const MAX_ROW_HEADER_W: u16 = 24; /// because it is a fact about the *view*, which only the server holds. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct FrameGroup { - /// The category the group belongs to, echoed back in `ToggleGroup` so a - /// client never works out which category a label came from. - pub cat: String, pub name: String, pub collapsed: bool, } @@ -1137,7 +1137,6 @@ mod tests { FrameEntry::Item(vec!["A".into(), "x".into()]), FrameEntry::Item(vec!["A".into(), "y".into()]), FrameEntry::Group(FrameGroup { - cat: "Type".into(), name: "Late".into(), collapsed: true, }), @@ -1150,7 +1149,6 @@ mod tests { /// ungrouped between them — improvise-h0iv's reachable shape. fn repeated_group_frame() -> AxisFrame { let q1 = || FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }; @@ -1176,7 +1174,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }), @@ -1214,7 +1211,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: true, }), @@ -1266,7 +1262,7 @@ mod tests { /// The pair `FrameGroup` carries cannot separate them — h0iv's own proposed /// fix. See `context/test-evidence/protocol.md`. #[test] - fn the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them() { + fn the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them() { let frame = repeated_group_frame(); let headers: Vec<&FrameGroup> = frame .entries @@ -1277,10 +1273,9 @@ mod tests { }) .collect(); assert_eq!( - (&headers[0].cat, &headers[0].name), - (&headers[1].cat, &headers[1].name), - "both headers name the same category AND the same group, so the \ - ticket's (cat, name) comparison would still merge them" + headers[0], headers[1], + "both headers are the same group in the same state, so no \ + comparison of their fields can split them" ); } @@ -1344,7 +1339,7 @@ mod tests { assert_eq!(frame.item(2), Some(&vec!["B".to_string(), "z".to_string()])); assert!(frame.group_of(0).is_none(), "[A,x] is in no group"); let group = frame.group_of(2).expect("[B,z] sits under Late"); - assert_eq!((group.cat.as_str(), group.name.as_str()), ("Type", "Late")); + assert_eq!(group.name, "Late"); assert!(group.collapsed, "the collapse state travels with the group"); } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 3a86f346..0e5d0c6e 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -756,6 +756,7 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { // Both creating columns are records-only, so the guard above takes // them; declining rather than `None` keeps that a door, not a hole. CellId::Create(_) => Some(vec![Command::SetStatus(not_yet("creating a column"))]), + CellId::Coordinate => Some(vec![Command::SetStatus(not_yet("renaming a coordinate"))]), } } @@ -949,7 +950,9 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option fn record_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.record), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } @@ -1016,7 +1019,9 @@ fn model(state: &ClientState, cmd: ModelCommand) -> Option { fn record_cell_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.clone()), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index 6b696c1f..e02f7cf9 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -180,7 +180,10 @@ impl Client { pub fn record_cell_at(&self, row: usize, col: usize) -> Option { match self.state.grid.cell(row, col)?.id.clone()? { CellId::Record(cell) => Some(cell), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index fa48a6bd..8aa220b4 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -144,7 +144,16 @@ impl RenderCell { text: cell.text.clone(), numeric: cell.numeric, error: cell.error, - editable: matches!(cell.id, Some(CellId::Record(_))), + editable: match cell.id { + Some(CellId::Record(_)) => true, + Some( + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate, + ) + | None => false, + }, selected, edit_text: cell.edit_text.clone().unwrap_or_default(), } @@ -563,9 +572,8 @@ mod tests { /// A 2x3 grid whose cells cover the four id shapes plus a padded slot, so /// one fixture answers every editability question. - fn group(cat: &str, name: &str, collapsed: bool) -> FrameEntry { + fn group(name: &str, collapsed: bool) -> FrameEntry { FrameEntry::Group(improvise_protocol::FrameGroup { - cat: cat.into(), name: name.into(), collapsed, }) @@ -583,11 +591,11 @@ mod tests { first: 0, cats: vec!["Month".into()], entries: vec![ - group("Month", "Q1", false), + group("Q1", false), item("Jan"), - group("Fiscal", "Q1", true), + group("Q1", true), item("Feb"), - group("Month", "Q2", false), + group("Q2", false), item("Apr"), ], }; @@ -628,6 +636,10 @@ mod tests { }), ), ((1, 0), CellDisplay::text("pending")), + ( + (1, 2), + CellDisplay::text("North").with_id(CellId::Coordinate), + ), ])), ], }); @@ -659,6 +671,10 @@ mod tests { !model.rows[1].cells()[1].editable, "and neither is a slot fit_to_labels padded" ); + assert!( + !model.rows[1].cells()[2].editable, + "renaming a coordinate moves the whole row, which is not a cell write" + ); } /// The numeric/error flags are the server's answers, echoed — so the test diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9e393226..73feb294 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -187,6 +187,21 @@ test.describe("the standalone deployment", () => { await expect(page.locator("#grid td.editable").first()).toBeVisible(); }); + // improvise-i0v9: "in-page worker · " was written at 13ms, before the + // worker had answered anything, and read as success over a blank page. + test("the worker is not called up until its first frame has arrived", async ({ page }) => { + const doc = `first-frame-${Date.now()}.improv`; + await page.addInitScript(logStatus); + await page.goto(`/app/index.html?doc=${doc}`); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(status(page)).toHaveText(`in-page worker · ${doc}`); + const log = await page.evaluate(() => window.__statusLog); + expect(log, `#status said: ${json(log)}`).toEqual([ + `starting the worker · ${doc}…`, + `in-page worker · ${doc}`, + ]); + }); + // improvise-9cma: a `?doc=` this backend cannot hold used to reach the // generic watchdog 8s later. The REASON now crosses the port (improvise-1wft). test("an invalid ?doc= is named on screen, and not blamed on the browser", async ({ @@ -235,6 +250,7 @@ test.describe("the standalone deployment", () => { const second = await context.newPage(); await second.clock.install(); + await second.addInitScript(logStatus); await second.goto(`/app/index.html?doc=${doc}`); // It PAINTS. That is the whole point: a locked document is read-only, not @@ -242,6 +258,15 @@ test.describe("the standalone deployment", () => { await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).toContainText("another tab"); + // The page's clock has not been walked, so a reason that arrived AFTER the + // watchdog would have been logged behind "did not answer". Nor was success + // ever claimed: the Snapshot that follows the notice must not overwrite it. + const early = await second.evaluate(() => window.__statusLog); + expect(early.at(-1), `#status said: ${json(early)}`).toContain("read-only"); + expect( + early.filter((t) => t.includes("did not answer") || t === `in-page worker · ${doc}`), + `#status said: ${json(early)}`, + ).toEqual([]); // And the reason STANDS: it is not replaced 8s later by a watchdog // blaming the browser for something the code knew precisely. @@ -252,6 +277,10 @@ test.describe("the standalone deployment", () => { await second.clock.runFor(HANDSHAKE_TIMEOUT_MS + 2_000); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).not.toContainText("did not answer"); + const late = await second.evaluate(() => window.__statusLog); + expect(late.some((t) => t.includes("did not answer")), `#status said: ${json(late)}`).toBe( + false, + ); await second.close(); await first.close(); diff --git a/web/app.js b/web/app.js index 22c487ce..22e94a34 100644 --- a/web/app.js +++ b/web/app.js @@ -181,7 +181,8 @@ function workerTransport() { shell.fail(`worker failed to start: ${event.message ?? "see console"}`); console.error("improvise: worker error", event); }); - shell.setShell(`in-page worker · ${DOC}`); + // Not yet a claim that it works: the first frame makes that (improvise-i0v9). + shell.setShell(`starting the worker · ${DOC}…`); // Frames posted before the worker has finished `start()` are held by // worker.js in arrival order, so the handshake can go out immediately. return { send: (text) => worker.postMessage(text), ready: Promise.resolve() }; @@ -801,6 +802,7 @@ shell.arm( WS_URL ? `${WS_URL} never answered — is improvise-ws-server running, and did it send a document?` : "the in-page worker did not answer (see console)", + { answer: WS_URL ? null : `in-page worker · ${DOC}` }, ); await transport.ready; sendViewport(); // the handshake: the snapshot is built at this extent diff --git a/web/shell.js b/web/shell.js index c2393afc..169373a7 100644 --- a/web/shell.js +++ b/web/shell.js @@ -339,7 +339,12 @@ export function createShell({ clearTimer = clearTimeout, }) { let watchdog = null; + let answered = null; let arms = 0; + const disarm = () => { + clearTimer(watchdog); + watchdog = null; + }; return { setShell(text) { @@ -350,24 +355,25 @@ export function createShell({ }, /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ - arm(message) { + arm(message, { answer = null } = {}) { arms += 1; - clearTimer(watchdog); + answered = answer; + disarm(); watchdog = setTimer(() => { watchdog = null; this.setShell(message); }, timeoutMs); }, - /** A frame arrived, so the peer answered: the watchdog has no question - * left to ask. */ + /** A frame arrived, so the peer answered. Only the FIRST, and only if no + * failure spoke first, may say so (improvise-i0v9, clients.md). */ sawFrame() { - clearTimer(watchdog); - watchdog = null; + if (watchdog !== null && answered !== null) this.setShell(answered); + disarm(); }, /** A peer that failed *loudly* has already said something more precise, so * the watchdog must be DISARMED and not merely pre-empted (PR #13). */ fail(text) { - this.sawFrame(); + disarm(); this.setShell(text); }, armed: () => watchdog !== null, diff --git a/web/tests/shell.test.js b/web/tests/shell.test.js index b2d02999..e8a9570d 100644 --- a/web/tests/shell.test.js +++ b/web/tests/shell.test.js @@ -351,6 +351,43 @@ test("a notice DISARMS the watchdog rather than being replaced by it", () => { ); }); +// improvise-i0v9: "in-page worker · " was written before the worker had +// answered anything, and read as success over a blank page for eight seconds. +test("only the first frame claims the peer answered, and not before it", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + assert.deepEqual(seen, [], "the claim was made before any frame arrived"); + shell.sawFrame(); + shell.sawFrame(); + assert.deepEqual(seen, ["in-page worker · a.improv"]); +}); + +// The order worker.js really produces on a locked document: the ReadOnly +// notice is posted after `start()` and BEFORE the Snapshot answers the handshake. +test("a reason given before the first frame is not replaced by the frame's claim", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + shell.fail(noticeText({ kind: "ReadOnly", document: "a.improv", detail: "" })); + shell.sawFrame(); + clock.fire(); + assert.equal(seen.length, 1, `#status said: ${JSON.stringify(seen)}`); + assert.match(seen[0], /read-only/); +}); + // The notice's remembered state. The value crosses `localStorage`, which is // allowed to forget and allowed to throw, so the interesting inputs are the // ones that are NOT either spelling.