Merge remote-tracking branch 'gh/main' into wire2/2wu3
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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 · <doc>`
|
||||
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 · <doc>…`, 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
|
||||
|
||||
+71
-6
@@ -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,
|
||||
|
||||
+43
-13
@@ -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".
|
||||
|
||||
@@ -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 · <doc>`. 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 · <doc>` 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 · <doc>…", "in-page worker · <doc>"]`. 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
|
||||
|
||||
@@ -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<CellId>` 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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<ViewFrame> {
|
||||
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<dyn crate::storage::Storage>) -> 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<dyn Effect>),
|
||||
("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();
|
||||
|
||||
@@ -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<Cel
|
||||
RecordCol::NewCategory => 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<Option<CellId>> = (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]
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
|
||||
|
||||
@@ -756,6 +756,7 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option<Vec<Command>> {
|
||||
// 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<crate::RecordHash> {
|
||||
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<Command> {
|
||||
fn record_cell_at_cursor(state: &ClientState) -> Option<RecordCell> {
|
||||
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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -180,7 +180,10 @@ impl Client {
|
||||
pub fn record_cell_at(&self, row: usize, col: usize) -> Option<RecordCell> {
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -187,6 +187,21 @@ test.describe("the standalone deployment", () => {
|
||||
await expect(page.locator("#grid td.editable").first()).toBeVisible();
|
||||
});
|
||||
|
||||
// improvise-i0v9: "in-page worker · <doc>" 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();
|
||||
|
||||
+3
-1
@@ -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
|
||||
|
||||
+13
-7
@@ -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,
|
||||
|
||||
@@ -351,6 +351,43 @@ test("a notice DISARMS the watchdog rather than being replaced by it", () => {
|
||||
);
|
||||
});
|
||||
|
||||
// improvise-i0v9: "in-page worker · <doc>" 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.
|
||||
|
||||
Reference in New Issue
Block a user