diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 618d30c2..6512d580 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -28,6 +28,7 @@ them here would let the copy drift: | `context/repo-map.md` | The "where", plus **numbered gotchas** — read the ones your diff touches | | `prolog-sessions/kb.pl` | Executable knowledge: `invariant/2`, `doctrine/2`, `forbidden/2`, `decision/3`, and prior `work_done/3` | | `AGENTS.md` | Working rules: bug-first tests, `Option`/`Result`/`Box` at the edges, merges over rebases | +| `.agents/skills/wave/agent-rules.md`, "GitHub" | The reviewer's rule: what to read before your first comment, how to answer each existing review, and what every review you post must state. Read it before reviewing a PR | `kb.pl` is the highest-value and least-obvious of these. Grep it before reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`. diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index f52ce28a..43787a3b 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -80,7 +80,8 @@ defect is a test that passes. "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, and a test-merge, so it reviews the tree that will exist. Point it at `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. -- An ADR PR merges on the user's approval, not on a review: +- Only the user's explicit approval accepts an ADR, and lets its PR merge. The + review gates still apply, and none of them is approval: `AGENTS.md`, "ADR pull requests". - A re-review is of the **delta**, at a named SHA. A review of a superseded commit reads as a pass. diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 7a4d3f52..7358f704 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -32,9 +32,17 @@ not in a second copy. 6. **The Beads "Session Completion" block near the end of `AGENTS.md` does not set your git policy.** This repository does not opt in to the block's "team-maintainer" profile. On a wave branch that profile's pull and bare - `git push` would be refused (rule 4), or worse. Rules 1–5 are your git - policy. The block is bd's and bd regenerates it whole, so it is not edited - by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). + `git push` would be refused (rule 4), or worse. Rules 1–5 and 7 are your + git policy. The block is bd's and bd regenerates it whole, so it is not + edited by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). +7. **Pin `gh/main` to a literal SHA when the task starts.** Run + `git rev-parse gh/main` once. Use that SHA, not the ref name, for every + reset, merge-base and diff. Remote refs are shared by every worktree of the + repository, so `gh/main` moves whenever another session fetches. An agent + ran `git reset --soft gh/main` mid-task and staged a reverse diff of #196. + It noticed only because it read `git status` (improvise-diho). Re-resolve + only on purpose, for example when you mean to merge a newer main. Rule 3's + check before a PR is such a time. ## Commit early: a worktree is not durable storage @@ -113,6 +121,12 @@ out on purpose: check that no test *disappeared*. Diff test **names**, not counts: a textually clean merge can delete a test module with no conflict marker, and the other side's new tests mask the loss in a count. +- **Build each side's test list in a fresh `CARGO_TARGET_DIR`, every time.** If + a parent's tree comes from `git archive`, a reused target dir gives a wrong + answer, not just a slow one. Archive stamps every file with its commit time, + so cargo judges the sources older than the last build and lists the previous + tree's tests. On #188 that made three of the branch's own tests look + merge-only. - **The shell guard refusing a compound command means NEITHER part ran.** Split the command. After a refusal, re-check the state it was meant to produce. - **Re-run the full suite after reverting an injected fault.** A revert applied @@ -200,6 +214,8 @@ text for any of these in your hand-back. `./scripts/pr-reviews `, unpiped. Reply on its thread to every Codex review: fixed (name the commit), declined (say why), or already one of our tickets (name it). Every review you post states the head SHA it reviewed. +- **Answer a review even if the PR has already merged.** Reviews arrive late, + and the thread is still where the next reader looks. - **Do not merge, and do not close beads.** Your part ends at an open PR plus your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over the Beads blocks". Avoid stacked PRs. If you must stack, never let the base diff --git a/AGENTS.md b/AGENTS.md index 8a11780c..6e66e82c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -131,9 +131,10 @@ and watch the check fail. **Do not push `main` to `gh/main`.** Each agent's work reaches GitHub as its own branch and its own pull request, and lands on `main` by being merged there, once -CI is green and an adversarial review agent has passed the PR's head SHA. The -orchestrator does not integrate agent branches locally: its `main` only -fast-forwards to `gh/main` after a merge. The procedure is +CI is green and an adversarial review agent has passed the PR's head SHA. An ADR +PR also needs the user's explicit approval, and nothing else stands in for it: +"ADR pull requests" below. The orchestrator does not integrate agent branches +locally: its `main` only fast-forwards to `gh/main` after a merge. The procedure is `.agents/skills/wave/SKILL.md`, "Shepherding". `origin` and `gitea` are mirrors and take `main` directly. @@ -184,8 +185,8 @@ earlier head, and the finding may already be fixed — verify rather than assume in either direction. Reviews arrive **as the owner's quota allows**, so absence is not evidence. -Wait where a review is plausible, and when one is found, answer it on the -thread even if the PR has already merged: that is where the next reader looks. +Wait where a review is plausible. How a review is answered once found, by the +author and by a reviewer, is `.agents/skills/wave/agent-rules.md`, "GitHub". **Copilot is not reviewing this repository, and the request is a silent no-op.** Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61, @@ -231,8 +232,8 @@ normally.) Across four PRs Copilot has found genuine bugs and no false positives, so its findings are worth the loop. **But judge each one against the repo's doctrine -rather than applying it reflexively**, and when you decline, reply on the thread -with the reason. The distinction that has mattered: a finding can be *correct* +rather than applying it reflexively**. How a declined finding is answered is +agent-rules, "GitHub". The distinction that has mattered: a finding can be *correct* and still be wrong to fix in that PR. On #12, a non-ASCII `truncate` panic was applied (byte-identical on ASCII, so it cost no evidence) while a hint-line padding bug in the same file was declined — fixing it would have rewritten the @@ -251,21 +252,30 @@ is to branch from `gh/main`, cherry-pick, open a follow-up PR, and delete the stray re-creation. Check `gh pr view --json state` *before* pushing a fix to a PR you have not looked at in a few minutes, rather than after. -Merge only once a review has passed — an adversarial agent's while Copilot is -absent, and it must name the head SHA it reviewed. Prefer a **merge commit** — +Merge only once a review of the PR's current head has passed — an adversarial +agent's while Copilot is absent. What a review must state is agent-rules, +"GitHub". Prefer a **merge commit** — `AGENTS.md` says merges over rebases, and a PR's commits are individually meaningful. ### ADR pull requests -An ADR (`context/adr/`) merges on the user's approval, not on a review. The -steps, modelled on PR #122 (ADR 004): +**Only the user's explicit approval accepts an ADR (`context/adr/`), and only +it lets an ADR PR merge.** "An ADR isn't approved until I say it is" (the +user). Green CI, every review answered and a passing adversarial review +("Shipping a wave") are still required. They are gates, never approval: an ADR +PR that passes all of them is still unapproved and does not merge. The steps, +modelled on PR #122 (ADR 004): 1. The PR proposes the ADR with its **open questions numbered**. 2. The user answers them on the PR. 3. The answers are folded into the ADR, in a section headed **"What the user decided"**. -4. The PR merges only after the user comments **"Approved"**. +4. The PR merges only after the user comments **"Approved"**, and only while + the gates above still hold. Nobody else's comment or review stands in for + it. "Read the PR's reviews before merging it" still + applies after the approval. On #122 the user's own review landed as the PR + merged. ### Never hand-edit `.beads/issues.jsonl` @@ -296,7 +306,7 @@ bd show $(cat /tmp/bd-audit-ids.txt) 2>&1 \ `git ls-files` picks the files, and grep only reads them. The file list is the tracked files plus the untracked ones, minus anything git ignores. That leaves -out three things a `grep -r` over the same roots reads: +out two things a `grep -r` over the same roots reads: - **Build output.** After `./harness/build.sh`, `harness/serve/` holds rendered copies of `context/`, and `grep -r` reported three dead ids from them @@ -304,9 +314,16 @@ out three things a `grep -r` over the same roots reads: - **Other agents' worktrees.** In the orchestrator's checkout `.claude/worktrees/` holds every agent's worktree, dozens of copies of the repository with their unpushed work. -- **The skills twice.** `.claude/skills/*` are symlinks to directories under - `.agents/skills/`. `-d skip` passes over them, so the skills are read once, - through `.agents`. + +`-d skip` is there for the skills' symlinks, and it changes no answer. The +`.claude/skills/*` entries are symlinks to directories under `.agents/skills/`, +and `git ls-files` lists each one, so each reaches grep as a command-line +operand. The grep has no `-r`, so it never reads a directory operand. It prints +`grep: : Is a directory` on stderr instead, and `-d skip` only silences +those lines. The output goes through `sort -u`, so a duplicate could not change +the answer anyway. Measured with BSD grep 2.6.0 and GNU grep 3.12: 3092 matches +with or without the flag, 462 distinct ids. Do not add `-r`: GNU grep then +follows those symlinks and reads the skills a second time (3107 matches). Plain `git grep` is not a substitute: on macOS its `-E` silently ignores `\b` and finds a fraction of the ids. @@ -346,8 +363,10 @@ real defect through: added to the exclusion list in the same commit**, or it arrives as a dead id that no amount of `bd` archaeology explains. -Note the recipe's own prose deliberately spells no live or dead id: a document -that lists ids as examples makes itself a source of audit noise. +The recipe's own prose spells no id as an *example*, because a document that +lists example ids makes itself a source of audit noise. The ids it does cite, +improvise-8haj and improvise-oujb, are the tickets behind two of the rules +above. They resolve, and the audit checks them like any other. One environment note: in a worktree-isolated agent the shell guard refuses a loop that passes a runtime-computed argument to `bd`, so the recipe above @@ -376,9 +395,10 @@ regeneration. Repository policy lives here, outside them: - **Work is not done until it is in a pull request and that pull request has merged.** A pushed branch is not done, and neither is a green one. In a wave, the merge is the orchestrator's, after an adversarial review passes the head - SHA ("Shipping a wave"). So a wave agent's part ends at an open PR plus its - hand-back, and the ticket is done, and its bead closed, only when the merge - lands. + SHA ("Shipping a wave"). An ADR PR merges only on the user's explicit + approval as well ("ADR pull requests"). So a wave agent's part ends at an + open PR plus its hand-back, and the ticket is done, and its bead closed, only + when the merge lands. - **This repository does not opt in to the Beads "team-maintainer" profile.** So the pull (`git pull`, or `git pull --rebase` in bd's "full" profile) and the bare `git push` that the block lists for that profile are not diff --git a/context/code/command.md b/context/code/command.md index a1eef6cd..294c6d8e 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -592,10 +592,23 @@ the client was told nothing at all. key-path batch assumes the failed one happened. `refuse_and_stop` does it for `LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and `import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its - own inline abort (gotcha 21). The item, formula and view effects only refuse: - the frame is rolled back by the refusal alone. On the key path, a failed - `AddItem`, `AddFormula` or view edit no longer stops the rest of its batch, - which is what it did before bok4. + own inline abort (gotcha 21). The item and view effects, and `AddFormula`'s + missing-category arm, only refuse: the frame is rolled back by the refusal + alone. On the key path, a failed `AddItem` or view edit no longer stops the + rest of its batch, which is what it did before bok4. +- **`AddFormula`'s parse-error arm also stops (improvise-v4hg).** It is the one + item-or-formula failure a key-path batch was found to depend on. Enter in + `FormulaEdit` is `[SetStatus, AddFormula, ChangeMode(FormulaPanel), + ClearBuffer("formula")]`, and the last two assume the commit worked. Without + the stop, `Total = (` was reported and then the editor closed and the typed + text was cleared. With it, the user stays in `FormulaEdit` with the text, and + the error is drawn beside the prompt. `fit-measure` emits `AddFormula` last, so + there is nothing after it to stop. The missing-category arm is reachable only + from a wire frame or a `:` line naming another category, and nothing in either + depends on it, so it does not stop. PR #181's review searched for an + `AddItem` case the same way and found none, so `AddItem` does not stop either. + An abort is added where a later effect depends on the failed one, never for + symmetry. In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame is undone whole, and the client is told, which is the channel the narrated @@ -615,9 +628,24 @@ Consequences: refusal, not by `status_msg`. The frame's rollback also rolls the host's status line back. - **Every refusal site has a test that fails when it is reverted** to a - status-only write (improvise-9fpl). Most are one table, - `every_failing_model_command_refuses_its_whole_frame`. That matters because - these are exactly the lines a merge conflict could silently drop. + status-only write (improvise-9fpl, improvise-0v8l). Two tables carry most of + them. `every_failing_model_command_refuses_its_whole_frame` covers the wire. + `every_refusal_site_refuses_its_failure_and_only_its_failure` covers the key + path and the `:` line, which is what the headless exit code reads. The wire + table cannot see the refusal half of a `refuse_and_stop` site, because + `Host::mutating_frame` rejects on `refused || aborted`, and the abort alone + suffices. So each `refuse_and_stop` site needs a key-path assertion for its + refusal as well as one for its abort. The abort is + `a_failed_load_or_import_does_not_run_the_quit_after_it` (improvise-514o). + Three sites are not in the key-path table (improvise-rtud). The quit test's + "setup:" assertion holds the refusal of `load` and `import-json absent.json`. + `a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text` + holds `AddFormula`'s parse arm, which has no case in the quit test. The other + five import arms are in + `every_refusal_site_refuses_its_failure_and_only_its_failure`. + Each case in that table has a succeeding twin, so a site that refused + unconditionally would fail too. That matters because these are exactly the + lines a merge conflict could silently drop. - **Not covered:** a no-op *inside* a view that does resolve. Examples are `SetAxis` on a category the view does not have, `HideItem` of an absent item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already @@ -3357,18 +3385,41 @@ one run in eight (measured: 46 of 400). So the arguments now include **every item of every regular category as a one-coordinate drill**, read off the parsed document. A one-coordinate drill spans every other category whole, so it matches the whole slice of records under that item, not one record picked by name. In -the demo that is four records and eight cells for each of the eight drills. The -test -also asserts that the widest candidate it actually passes matches at least four -records. That guard catches a demo with too little data, and it catches the -derived drills being dropped from the argument list. It does **not** catch the -comparison losing its wide drills some other way, because it measures what the -argument lists match, not what the built commands ordered (improvise-b9f2). The -review showed this with a rival: a `drill-into-cell` parser that refuses fewer -than two arguments sends every one-coordinate drill to the one-record cursor -fallback. The guard still reports four records, and with the store's sort -removed the law passed 38 of 300 runs. With the store's sort removed and the -parser intact, the law fails 400 of 400 runs. +the demo that is four records and eight cells for each of the eight drills. With +the store's sort removed, the law fails 400 of 400 runs. + +**The width guard measures what the comparison compared (improvise-b9f2, +improvise-btyw).** The test asserts that some `drill-into-cell` the comparison +actually built and compared froze at least four records. For each one it applies +those very effects to one scratch `App` and counts the records in its +`drill_state`. It counts them only when their `Debug` text, which carries their +order, appears in the text the comparison compared. The comparison reads `Debug` +while the scratch `App` reads the applied state, and those are two +representations. The review showed they can disagree: give `StartDrill` a +`Debug` that prints only a row count, and the frozen records are still four +while the compared text no longer holds their order. With the store's sort +removed the law then passed 300 of 300 runs, with the guard green. Requiring +the ordered records in the compared text ties the guard to what the comparison +could actually see. A first +version measured what the *argument lists* match in the store instead. That was +a parallel derivation, and the review beat it with a `drill-into-cell` parser +that refuses fewer than two arguments. Every one-coordinate drill then went to +the one-record cursor fallback, the argument lists still matched four records, +and with the store's sort removed the law passed 38 of 300 runs. + +Counting frozen records is not enough by itself. A drill over no coordinates +(any candidate with no `Cat/Item` argument, such as `["1", "1"]`) freezes +**every** record, and it reads them through `iter_cells` in insertion order, +never through the narrowing index where gotcha 18 lives. Under the parser rival +that whole-store drill still froze 16 records, and a guard counting it stayed +silent. So only a drill that **narrowed** the store counts: one that froze fewer +records than the store holds. With the parser rival in place, the guard fires on +every run ("froze 1 records"), and it fires on every run under the `Debug` rival +too ("froze 0 records"). + +What the guard still cannot see is a narrowing drill whose records are frozen in +an order that does not come from the store. It checks that the law had a wide +drill to compare, not why the drill's order would differ. **What it cannot see.** Only commands and arguments the fixture reaches. A command whose nondeterminism needs some other state, such as records mode, a diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 15ff6714..ebe02a28 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4749,3 +4749,58 @@ claims at commit. The commit was then *accepted*. With the refusal assertion disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the cell B cleared. Under that rival, the 0j0f test above failed as well. Both files were restored. + +### a_locally_resolved_page_scroll_lands_where_the_host_would_put_it + +**What was missing (improvise-5xbq).** `tests/lockstep.rs` compared only `j` +and `G` against the host. The browser's page rule (`resolve::page_rows`) and +the TUI's (`Move`'s `Page` arm, `visible_rows * 3 / 4`) were two copies, and +two faults injected into the browser's copy left the whole workspace green: +`page_rows` returning `window_rows / 2`, and `grid_move` paging by +`window_cols`. The second was invisible for a further reason: the only window +the page-rule test used was square, `(8, 8)`. + +**The fixture.** 20 rows by 3 columns; the host sees 8 rows and the client +announces `(8, 3)`. The three candidate rules then give three different +steps: 6 (three quarters of the rows), 4 (half the rows) and 2 (three quarters +of the columns). The walk starts off the edge (two `j`), then presses `PgDn`, +`PgUp`, `Ctrl-d` and `Ctrl-u`, including a `PgDn` that clamps at row 19. + +**Why it pins the rows as well as comparing the ends.** `page_rows` is now the +one home (the TUI's `Move` calls it), so a wrong rule moves host and client +together, and a comparison alone passes. Each press therefore also asserts the +row the TUI's rule names. + +**Injected, before and after the move to one home.** + +| Fault | Two copies | One home | +|---|---|---| +| `page_rows` returns `window_rows / 2` | fails at press 2 (`PgDn`), "client and host disagree" | fails at press 2, "not the row the TUI's page rule names"; `app.rs`'s `page_down_scrolls_by_three_quarters_visible` and `page_up_scrolls_backward` fail too | +| `grid_move` pages by `window_cols` | fails at press 2, "client and host disagree" | fails at press 2, "client and host disagree" | + +`Ctrl-d`/`Ctrl-u` are `scroll-rows ±5`, not the page rule. They are in the walk +because the ticket named them, and they move the start point of the next page +turn to a row the fixed rules alone would not reach. + +### a narrow window still shows how to page through help — `harness/tests/help.spec.js` + +**What broke (improvise-1c5m).** Below the width at which the app bar wraps, +`#help` (fixed, `inset: 2rem`, `z-index: 30`) covers `#hint`, and since PR #180 +Welcome carries no paging instruction of its own. So a new user on Welcome at +phone width could not see how to reach page two. + +**The fix is option (b) from the ticket.** The overlay draws `model.hint` in a +footer. In Help mode that is already `hint_line(Help)`, the same string `#hint` +carries, so it adds no derivation, and it sits outside `.help-body`, so +"Welcome is prose" still holds. The footer is `position: sticky` because the +overlay itself scrolls and Welcome is longer than a phone-height overlay. + +**What the case asserts.** At 600x800 on Welcome, it reads the hint string from +`#hint`, so the spec spells no keys. It then requires some element showing +exactly that string to be the topmost element at its own centre, which is the +`elementFromPoint` measurement the ticket made by hand. + +**Red, then a rival.** At base it failed with "the paging hint is under the +help overlay". With the fix and `position: sticky` changed to `static`, it +fails the same way: the footer exists but sits below the fold of a scrolling +overlay, so presence alone cannot satisfy the case. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d9409ff9..c61ad115 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5316,6 +5316,81 @@ 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". +#### improvise-514o: every `refuse_and_stop` arm + +The case list above covered `load` and the JSON cannot-read arm only. PR #181's +review reverted each `refuse_and_stop` to `app.refuse` one at a time, which +keeps the refusal and drops the abort. Five `ImportJsonHeadless` arms failed +nothing: CSV cannot-read, CSV parse, JSON parse, array path and build. The wire +table `every_failing_model_command_refuses_its_whole_frame` cannot see the +abort, because `Host::mutating_frame` rejects on `refused || aborted`, and the +refusal alone suffices. The list now has one case per arm, using the fixtures +the wire table uses, which now come from `ui::host::import_fixture_storage`. +Each case also asserts the arm's own status text, so a fixture that failed in a +different arm cannot stand in for the one it names. The same scripted sweep, +rerun after the change, fails this test at all seven sites. Before, it failed +at two. + +### ui::app::tests::a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text + +improvise-v4hg. In `FormulaEdit`, Enter on `Total = (` reported the parse error +and then did what a successful commit does: `CommitBuffer`'s +`ChangeMode(FormulaPanel)` closed the editor and the keymap's welded +`ClearBuffer("formula")` cleared the text. The user saw the error with their +formula gone. Driven through `App::handle_key` from `FormulaPanel`: `a`, the +characters, then Enter. The test failed first with "got FormulaPanel". The fix is +`refuse_and_stop` on the parse-error arm, and a `Binding::Sequence` is one +effect vec, so the stop also covers the `ClearBuffer` step. + +The mode assertion alone would be vacuous against a fix that stops the mode +change but still loses the text. That rival (clear the `formula` buffer, then +`refuse_and_stop`) was run and fails "and must keep what they typed" with +`left: None`. The control, `Total = 1`, asserts the ordinary path still stores +the formula and closes the editor, so a stop on every arm would fail it. After +the fix, the abort sweep (`refuse_and_stop` back to `app.refuse`) fails this +test at that site. + +### ui::app::tests::every_refusal_site_refuses_its_failure_and_only_its_failure + +improvise-0v8l. PR #185 split refusals (`App::refuse`, which the headless exit +code reads) from reports (`set_status`), and most of the refusal sites were +unpinned. The ticket counted 23 of 27. At gh/main `608ba6a` (after #181's wire +table), a script (`/tmp/refuse-fu/sweep.py`, not committed) reverted each of +the **32** production refusal sites in `ui/effect.rs` and `command/cmd/*.rs` +alone to a status-only write, and ran `cargo test --workspace --no-fail-fast`. +For `effect::refuse(x)` the revert is `effect::set_status(x)`. For +`app.refuse(x)` it is `app.view_state.status_msg = x.into()`. For +`refuse_and_stop` it is the status write with the abort kept. **13 of 32 failed +a test. 19 survived:** `AddCategory` at the limit, `apply_coordinate_rename`, +both `StartImportWizard` arms, the five `ImportJsonHeadless` arms the wire +table cannot see (the abort alone rejects the frame), `rename_record_coordinate`, +`CommitExport`, both `ViewNavigate` arms, both `AddRecordRow` arms, both +`EditOrDrill` arms, `SearchNavigate` and `ExecuteCommand`. + +This table has one case per survivor. Each case drives the failure through a +`:` line (`run_line`, which does what `improvise cmd` does, one batch per +segment) or through Enter in the site's own minibuffer mode. It asserts that the +batch refused and that the status names the site's own reason. Each case also +has a **succeeding twin** that must not refuse, so a site that refused +unconditionally fails too. That rival (`AddCategory` refusing on `Ok` as well) +was run and fails "its succeeding twin was refused". Rerun after the change, +the sweep catches **32 of 32**. The abort half, 10 sites before v4hg's eleventh, +went from 5 caught to 11 of 11. + +Three fixture choices. The second was found when the first run failed: the input meant to be refused was accepted. + +- **`AddCategory` past the limit** needs eleven categories and a two-segment + line. The first `add-category` fills the twelfth slot and the second is + refused, so the succeeding twin is the same fixture with one segment. +- **`add-record-row` onto an existing row** must page the category on the + view the client is actually on. Records mode switches to `_Records`, so + writing `Workbook::active_view_mut` (the document's default) changed a view + nobody was looking at, and the case "succeeded". It goes through + `App::view_mut_of(&ViewTarget::Active)` (gotcha 29). +- **The drill rename** edits record 1, not record 0, following the + cursor-at-index-0 rule in the wave agent rules. Nothing failed here: this is + a precaution, not a finding. + ### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and @@ -5434,14 +5509,41 @@ rivals it catches: still pin the store directly. - **The derived drills dropped from the argument list** (the `all.extend(one_coordinate_drills(model))` line commented out): the test - fails before comparing anything, with "the widest drill orders 1 records". - That is the only way of losing the wide drills the guard can see. It measures - what the argument lists match, not what the built commands ordered, so it can - pass while the comparison has lost its wide drills (improvise-b9f2). Review - rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, - which sends every one-coordinate drill to the one-record cursor fallback. The - guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 - runs. + fails with "the widest narrowing drill compared froze 1 records". +- **`drill-into-cell`'s parser changed to refuse fewer than two arguments** + (improvise-b9f2): every one-coordinate drill then goes to the one-record cursor + fallback. The first width guard measured what the argument lists match, so it + stayed silent. With gotcha 18 re-injected the law passed 38 of 300 runs in + review, and 31 of 300 here at `cfcc9bff`. The guard now applies the compared + drills' own effects to a scratch `App` and counts the records they froze. + Against this rival it fails on every run with "froze 1 records". + - A first version of the fix counted all frozen records and **still passed**. + A candidate with no `Cat/Item` argument drills the whole store (16 records), + and it reads them in insertion order through `iter_cells`, which the + injection cannot reach. So the guard counts only drills that froze fewer + records than the store holds. + - Measured at `7dfa90f1`, test binary in a loop: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + parser rival | **0 of 300** | all 300 | + | gotcha 18 alone | **0 of 300** | 0 (every failure a real divergence) | + | neither | 300 of 300 | 0 | +- **`StartDrill`'s `Debug` changed to print only its row count** + (`StartDrill(4 rows)`, improvise-btyw): the law compares effects by their + `Debug` text, and this rival takes the record order out of that text, while + the frozen `drill_state` still holds four records. At `7dfa90f1`, with gotcha + 18 re-injected, the law **passed 200 of 200** runs and the guard stayed green. + The guard now counts a drill only when its frozen records, as `Debug` prints + them, appear in the text the comparison compared. The same measurements after + the repair, 200 runs each: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + `Debug` rival | **0 of 200** | all 200 ("froze 0 records") | + | gotcha 18 + parser rival | **0 of 200** | all 200 | + | gotcha 18 alone | **0 of 200** | 0 (every failure a real divergence) | + | neither | 200 of 200 | 0 | - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". @@ -5625,3 +5727,27 @@ unsorted. This test fails. `view_only_effects_leave_shared_state_alone` now runs over `formula_app` too, so every `ViewOnly` case is checked against a populated cache. The two earlier injections (a `SetStatus` that writes the model, and one that writes a view's `number_format`) still fail it. + +### a_document_opening_on_a_records_view_opens_the_host_in_records_normal + +improvise-y5eo. `App::with_storage` set `view_state.mode` to `Normal` and never +settled it against the layout. The host settles only at the end of a batch +(`settle_normal_mode` in `apply_effects`). A session's mirror settles in +`project_diff`, so `Sessions::connect` handed a client `RecordsNormal` while +the host sat in `Normal` over the same records grid. The host's first `i` then +resolved through `Normal`'s keymap and entered plain `Editing`. The fixture's +active view puts `_Index` on rows and `_Dim` on columns, which the `.improv` +writer does persist. The test asserts the starting mode, then presses `i` and +asserts the host is still in a records mode. The fix calls +`settle_normal_mode` once at the end of construction, so the startup mode +comes from the same rule every batch ends with, and so from the same +`improvise_protocol::settled_mode` a client uses. + +Rival injected: that one call removed. The whole workspace had exactly two +failures. This test failed at the mode assertion (`left: Normal, right: +RecordsNormal`), and `a_fresh_sessions_slice_is_the_hosts_startup_slice` +failed on its new records case: the client's `RecordsNormal` against the +host's `Normal`, and nothing else different. `install_workbook` needs no +change of its own, because both of its callers (`LoadModel`, +`AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. + diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index b0517374..c6845bd5 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -764,3 +764,50 @@ to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html` is served, naming the fix. Injected by running `web/assemble.sh harness/serve` alone: 4 chromium failures, each carrying that message, where it used to fail with no hint of the cause. + +### Polling past the second poll — `scripts/pr-reviews-test.sh` (improvise-7epn) + +**What was unguarded.** Every multi-poll fixture resolved on poll 2, and the +timeout case expects exit 1 whether the loop polls twice or twenty times. So +replacing the post-poll deadline check with an unconditional `break` left the +suite at 23 passed, 0 failed, and `--wait N` would have polled at most twice. + +**Two cases, both on the one-tick-per-read clock.** "a review arriving on the +third poll is seen" empties polls 1 and 2 and puts an inline comment at the head +in `last`, with `--wait 10`, and asserts exit 0 *and* exactly 3 polls. "--wait +keeps polling until its deadline" runs the empty fixture with `--wait 5` and +asserts exit 1 after at least 5 polls. The second one is the timeout path. Its +exit code alone cannot tell a loop that stopped early from one that reached the +deadline, so it counts polls. + +**Rival, injected and watched.** `break` in place of +`[ "$(date +%s)" -lt "$deadline" ] || break`: 23 passed, 2 failed. The failures +were exactly the two new cases, each "exit 1 after 2 polls". The first attempt +to inject used `sed -i ''`, which the GNU sed on this PATH read as a filename. +Nothing changed and the suite passed 25/25, which looks like a vacuous rival. +The second attempt asserted the replacement landed before running. + +**Still not covered.** Deleting the check outright makes the timeout case loop +forever rather than fail. The suite has no wall-clock guard, so that fault shows +as a hang, not a FAIL. + +### A stale harness lock names its remedy — `harness/build.sh` (improvise-lnsh) + +**What broke.** With a stale `harness/opfs-contract/Cargo.lock`, `build.sh` +exited 101 and printed only cargo's text, whose `help:` line says to remove +`--locked`. That is the one change that brings improvise-mkbp back. The real +remedy was only in a source comment. + +**Reproduced, then fixed, by hand.** Changing `pest`'s locked version from +`2.9.2` to `2.9.1` in the lock reproduces the stale state. Before the fix: +exit 101 and cargo's hint only. After: exit 1, and the script's own two lines +follow cargo's, telling the reader not to remove `--locked` and naming +`cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. The lock +was untouched both times. Running that command on the stale lock restored it +byte for byte (`cmp` against the committed file). With the committed lock, +`build.sh` still runs to "harness/serve ready". + +**Not automated.** There is no suite for `harness/build.sh`. A test would have +to corrupt a tracked file and call cargo, which needs the crates.io index, from +`checkers.sh`, whose environment is scrubbed. The by-hand run above is the +evidence. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index dd6462b9..080a27b0 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -836,3 +836,37 @@ tests pass — including both of the ones *aimed* at the suppression — and onl this one fails. That is the shape design-principles §7 names: the assertion next door is satisfied by the wrong fix, and the discriminating input is the mode the change was not supposed to touch. + +### the_help_footer_names_the_keys_the_keymap_binds + +**What was wrong (improvise-17ak).** `src/ui/help.rs::render_footer` +hand-wrote the popup's paging keys: `l/Tab: next`, `h/S-Tab: prev`, +`q/Esc: close`. No check read them, since `every_hint_key_resolves_in_its_mode` +cannot see a string literal in a widget (the blind spot improvise-rr8n and +improvise-wk18 name). The literal was also already incomplete: the keymap binds +`help-page-next` to `n` and `→` as well, and `?` closes help, and the footer +advertised none of them. + +**Derive rather than delete.** rr8n's rule is to keep whichever table is +derived from the thing it describes, and the derived one here is the status +row's `hint_line(Help)`. The popup's margins always leave that row uncovered, +which argues for deleting the footer's keys. But `draw_status` prints +`status_msg` *instead of* `hint_text()` whenever the message is non-empty, and +nothing clears the message on entering Help. So after any message ("Saved to +…", an error), a user who presses `?` would see no paging keys at all. The +footer therefore now draws the page indicator followed by `hint_line(Help)` +itself, truncated in display cells (gotcha 50). + +**The test.** It renders the popup at 140×30, wide enough that the whole hint +fits (the rr8n lesson: an 80-column render can hide the very thing asserted), +on every page. It requires the footer row to contain `hint_line(&AppMode::Help)` +exactly, and first asserts that hint is non-empty and pages. + +**Red, and the rival that matters.** +- At base it fails on page 0: `"page 1 of 5 l/Tab: next q/Esc: close"` is + not `"Tab/l/n/→:next-page S-Tab/h/p/←:prev-page ?/Esc/q:close"`. +- A footer holding a literal copy of today's derived string passes today, so it + was injected together with a rebinding (`n` removed from `help-page-next` in + the protocol keymap). The test then fails: the copy still advertises `n`, + which the keymap no longer binds. That rebinding case is exactly the one the + ticket said the old footer would lie through. diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 8f0a71e5..06135353 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -1,14 +1,14 @@ //! A command is a function of its context: two contexts built independently from //! equal data give equal effects — `context/code/command.md`, improvise-hzjo. -use std::collections::{BTreeSet, HashSet}; +use std::collections::BTreeSet; use super::core::{Cmd, CmdContext}; +use super::grid::DrillIntoCell; use super::registry::tests::candidate_args; use crate::command::key::{KeyCode, KeyModifiers}; use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; -use crate::model::record::record_of; use crate::ui::app::App; use crate::ui::effect::{self, Effect}; @@ -61,27 +61,23 @@ fn one_coordinate_drills(model: &Model) -> Vec> { drills } -/// How many distinct records the widest all-`Cat/Item` candidate matches. -fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { - let coordinates = |args: &Vec| -> Option> { - args.iter() - .map(|a| { - a.split_once('/') - .map(|(c, i)| (c.to_string(), i.to_string())) - }) - .collect() - }; - candidates - .iter() - .filter(|args| !args.is_empty()) - .filter_map(coordinates) - .map(|partial| { - let cells = model.data.matching_cells(&partial); - let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); - records.len() - }) - .max() - .unwrap_or(0) +/// Records a compared drill froze, counted only if it narrowed the store and its +/// ordered records are in `compared`, the text the law compared (improvise-btyw). +fn records_narrowed(scratch: &mut App, effects: Vec>, compared: &str) -> usize { + let store = scratch + .model_state + .workbook + .model + .data + .live_records() + .count(); + scratch.apply_effects(effects); + let frozen = scratch + .drill_state + .as_ref() + .filter(|drill| compared.contains(&format!("{:?}", drill.records))) + .map_or(0, |drill| drill.records.len()); + if frozen < store { frozen } else { 0 } } /// The registry's candidates, plus names the demo really has: a drill or a fit @@ -108,9 +104,9 @@ fn args_the_document_names(model: &Model) -> Vec> { all } -fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option>> { let cmd = ctx.registry.interactive(name, args, ctx).ok()?; - Some(format!("{:?}", cmd.execute(ctx))) + Some(cmd.execute(ctx)) } /// What running every registered command under `a` and under each of `others` found. @@ -120,20 +116,34 @@ struct Comparison { shown: Vec, never_built: Vec<&'static str>, compared: usize, + widest_drill: usize, } fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let mut found = Comparison::default(); let candidates = args_the_document_names(a.model); + let drill = DrillIntoCell { + key: CellKey::new(vec![]), + }; + let mut scratch = fixture(); for name in a.registry.names().collect::>() { let before = found.compared; for args in &candidates { - let Some(left) = effects_of(name, args, a) else { + let Some(effects) = effects_of(name, args, a) else { continue; }; + let left = format!("{effects:?}"); + if name == drill.name() { + found.widest_drill = + found + .widest_drill + .max(records_narrowed(&mut scratch, effects, &left)); + } found.compared += 1; for b in others { - let right = effects_of(name, args, b).unwrap_or_default(); + let right = effects_of(name, args, b) + .map(|e| format!("{e:?}")) + .unwrap_or_default(); if left != right { found.diverged.insert(name); found @@ -157,13 +167,12 @@ fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { .iter() .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) .collect(); - let model = contexts[0].model; - let widest = widest_drill(model, &args_the_document_names(model)); - assert!( - widest >= 4, - "the widest drill orders {widest} records, so the store's order is nearly a coin toss" - ); let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.widest_drill >= 4, + "the widest narrowing drill compared froze {} records, so the store's order is nearly a coin toss", + found.widest_drill + ); assert!( found.never_built.is_empty(), "no candidate args build these in the fixture, so the law never ran them: {:?}", diff --git a/crates/improvise-command/src/command/cmd/navigation.rs b/crates/improvise-command/src/command/cmd/navigation.rs index 5f13f2cb..14523b5c 100644 --- a/crates/improvise-command/src/command/cmd/navigation.rs +++ b/crates/improvise-command/src/command/cmd/navigation.rs @@ -118,7 +118,7 @@ impl Cmd for Move { } } MoveKind::Page(dir) => { - let delta = (self.cursor.visible_rows as i32 * 3 / 4).max(1) * dir; + let delta = improvise_protocol::resolve::page_rows(self.cursor.visible_rows) * dir; let nr = (self.cursor.row as i32 + delta).clamp(0, row_max) as usize; (nr, self.cursor.col) } diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 4969f2cb..df25400b 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -386,7 +386,9 @@ impl App { let view = workbook.active_view(); GridLayout::with_frozen_records(&workbook.model, view, None) }; - Self { + // A document may open on a records grid, so the first mode is settled + // by the rule every batch ends with (improvise-y5eo). + let mut app = Self { model_state: ModelState { workbook, file_path, @@ -416,7 +418,9 @@ impl App { last_reframed_elsewhere: std::collections::BTreeSet::new(), keymap_set: KeymapSet::default_keymaps(), storage, - } + }; + app.settle_normal_mode(); + app } /// This client's cursor in the active view. Absent means the origin, so @@ -2758,6 +2762,29 @@ mod tests { } } + /// improvise-y5eo: a document that opens on a records grid opens the host in + /// `RecordsNormal`. See `context/test-evidence/command.md`. + #[test] + fn a_document_opening_on_a_records_view_opens_the_host_in_records_normal() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + let mut app = App::new(wb, None); + assert!(app.layout.is_records_mode(), "setup: the grid is records"); + assert_eq!(app.view_state.mode, AppMode::RecordsNormal); + + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + assert!( + app.view_state.mode.is_records(), + "the host's first `i` left records mode: {:?}", + app.view_state.mode + ); + } + /// The settle's `RecordsNormal` arm, driven from the mode it names. The /// `:` crossing above enters the settle in `Normal` both ways — the evidence. #[test] @@ -3001,16 +3028,34 @@ mod tests { fn a_failed_load_or_import_does_not_run_the_quit_after_it() { use improvise_protocol::keymap::Keymap; - for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] { + // One case per `refuse_and_stop` site (improvise-514o). + let cases: [(&str, &[&str], &str); 7] = [ + ("load", &["absent.improv"], "Load error"), + ("import-json", &["absent.json"], "Cannot read 'absent.json'"), + ("import-json", &["absent.csv"], "Cannot read 'absent.csv'"), + ("import-json", &["bad.csv"], "CSV error"), + ("import-json", &["bad.json"], "JSON parse error"), + ( + "import-json", + &["nested.json", "M", "nope"], + "No array at path", + ), + ("import-json", &["measures.json"], "Import error"), + ]; + for (command, args, arm) in cases { + let path = args.join(" "); let mut app = App::new(Workbook::new("T"), None); - app.storage = Box::new(crate::storage::MemoryFs::new()); + app.storage = Box::new(crate::ui::host::import_fixture_storage()); let key = KeyCode::Char('x'); let mods = KeyModifiers::NONE; let mut km = Keymap::new(); km.bind_seq( key, mods, - vec![(command, vec![path.to_string()]), ("force-quit", vec![])], + vec![ + (command, args.iter().map(|a| a.to_string()).collect()), + ("force-quit", vec![]), + ], ); let effects = { let ctx = app.cmd_context(key, mods); @@ -3020,8 +3065,8 @@ mod tests { app.apply_effects(effects); assert!( - app.last_batch_refused(), - "setup: `{command}` must actually have failed, got {:?}", + app.last_batch_refused() && app.view_state.status_msg.contains(arm), + "setup: `{command} {path}` must fail in the {arm:?} arm, got {:?}", app.view_state.status_msg ); assert_ne!( @@ -3032,6 +3077,392 @@ mod tests { } } + /// improvise-v4hg: Enter on a formula that does not parse threw the typed text + /// away. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text() { + let type_and_commit = |text: &str| { + let mut app = two_col_model(); + app.view_state.mode = AppMode::FormulaPanel; + let press = |app: &mut App, code| { + app.handle_key(KeyEvent::new(code, KeyModifiers::NONE)) + .unwrap() + }; + press(&mut app, KeyCode::Char('a')); + assert!( + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }), + "setup: `a` in the formula panel opens the editor" + ); + for c in text.chars() { + press(&mut app, KeyCode::Char(c)); + } + press(&mut app, KeyCode::Enter); + app + }; + + // The control: a formula that parses is stored and the editor closes. + let ok = type_and_commit("Total = 1"); + assert_eq!(ok.view_state.mode, AppMode::FormulaPanel); + assert!( + ok.model_state + .workbook + .model + .formulas() + .iter() + .any(|f| f.target == "Total"), + "the control formula must land" + ); + + let bad = type_and_commit("Total = ("); + assert!( + bad.last_batch_refused() && bad.view_state.status_msg.contains("Formula error"), + "the parse error must be reported, got {:?}", + bad.view_state.status_msg + ); + assert!( + matches!(bad.view_state.mode, AppMode::FormulaEdit { .. }), + "a refused formula must leave the user in the editor, got {:?}", + bad.view_state.mode + ); + assert_eq!( + bad.view_state.buffers.get("formula").map(String::as_str), + Some("Total = ("), + "and must keep what they typed" + ); + } + + /// Run a `:` line the way `improvise cmd` does, one batch per segment; true + /// when any segment refused, which is what its exit code reads. + fn run_line(app: &mut App, line: &str) -> bool { + let mut refused = false; + for segment in crate::command::parse::segments(line) { + let effects = { + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + crate::command::parse::parse_segment_interactive(segment, &ctx) + .unwrap_or_else(|e| panic!("`{segment}` must parse: {e}")) + .map(|cmd| cmd.execute(&ctx)) + .unwrap_or_default() + }; + app.apply_effects(effects); + refused |= app.last_batch_refused(); + } + refused + } + + /// Commit `text` in a minibuffer mode with Enter; true when that batch refused. + fn commit_in(app: &mut App, mode: AppMode, buffer: &str, text: &str) -> bool { + app.view_state.mode = mode; + app.view_state + .buffers + .insert(buffer.to_string(), text.to_string()); + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + app.last_batch_refused() + } + + fn with_import_fixtures(mut app: App) -> App { + app.storage = Box::new(crate::ui::host::import_fixture_storage()); + app + } + + /// Eleven regular categories: one short of the limit. + fn one_category_short_of_the_limit() -> App { + let mut wb = Workbook::new("T"); + for n in 0..11 { + wb.add_category(format!("C{n}")).unwrap(); + } + App::new(wb, None) + } + + /// Stage one coordinate rename in a drill over the records fixture, then commit it. + fn commit_drill_rename(app: &mut App, to: &str) -> bool { + let records = app.layout.records.clone().expect("records mode"); + let mut pending_edits = HashMap::new(); + pending_edits.insert( + (1, crate::view::RecordCol::Category("Region".into())), + to.into(), + ); + app.drill_state = Some(DrillState { + records, + pending_edits, + }); + app.apply_effects(vec![Box::new(crate::ui::effect::ApplyAndClearDrill)]); + app.last_batch_refused() + } + + /// Records mode with `Region` paged to `page`: `add-record-row` then names + /// the row whose coordinates are exactly that page. + fn records_paged_to(app: &mut App, page: &str) { + app.model_state.workbook.add_item("Region", "West").unwrap(); + let view = app + .view_mut_of(&ViewTarget::Active) + .expect("the active view"); + view.set_axis("Region", crate::view::Axis::Page); + view.set_page_selection("Region", page); + app.rebuild_layout(); + } + + /// A grid with a regular category on None, so every cell is an aggregate. + fn aggregated_grid(with_items: bool) -> App { + let mut wb = Workbook::new("T"); + for cat in ["Row", "Col", "Hidden"] { + wb.add_category(cat).unwrap(); + if with_items { + wb.add_item(cat, "A").unwrap(); + } + } + wb.active_view_mut() + .set_axis("Hidden", crate::view::Axis::None); + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + /// `Region` down, `_Measure` across: `Revenue` at column 0, then `Total`, + /// which a formula owns, at column 1. + fn a_formula_column() -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut() + .set_axis("_Measure", crate::view::Axis::Column); + let mut app = App::new(wb, None); + app.apply_effects(vec![Box::new(crate::ui::effect::AddFormula { + raw: "Total = Revenue * 2".into(), + target_category: "_Measure".into(), + })]); + app + } + + fn with_a_cell(mut app: App) -> App { + app.model_state.workbook.model.set_cell( + CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]), + CellValue::Number(42.0), + ); + app.rebuild_layout(); + app + } + + fn with_a_second_view(mut app: App) -> App { + app.model_state.workbook.create_view("Second"); + app + } + + fn switch_to_second(app: &mut App) { + app.apply_effects(vec![Box::new(crate::ui::effect::SwitchView( + "Second".into(), + ))]); + } + + /// improvise-0v8l: one case per refusal site no other test pinned, each with a + /// succeeding twin. Evidence: `context/test-evidence/command.md`. + #[test] + fn every_refusal_site_refuses_its_failure_and_only_its_failure() { + type Setup = fn() -> App; + type Drive = fn(&mut App) -> bool; + let cases: Vec<(&str, Setup, Drive, &str, Setup, Drive)> = vec![ + ( + "add-category past the limit", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A . add-category B"), + "Maximum of 12 categories", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A"), + ), + ( + "a drill commit that empties a coordinate", + records_app_for_drill, + |app| commit_drill_rename(app, ""), + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_app_for_drill, + |app| commit_drill_rename(app, "West"), + ), + ( + "import of a JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import of an absent file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import absent.json"), + "Cannot read file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import-json of an absent CSV", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json absent.csv"), + "Cannot read 'absent.csv'", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of a CSV that is not text", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.csv"), + "CSV error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json at a path with no array", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json nested.json M nope"), + "No array at path", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json that builds no model", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json measures.json"), + "Import error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "a records coordinate edited to nothing", + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "") + }, + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "West") + }, + ), + ( + "an export with no path", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", ""), + "not exported", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", "out.csv"), + ), + ( + "view-forward with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-forward"), + "No forward view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back"); + run_line(app, "view-forward") + }, + ), + ( + "view-back with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-back"), + "No previous view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back") + }, + ), + ( + "add-record-row outside records mode", + two_col_model, + |app| run_line(app, "add-record-row"), + "only works in records mode", + records_model_with_two_rows, + |app| run_line(app, "add-record-row"), + ), + ( + "add-record-row onto a row that is already there", + records_model_with_two_rows, + |app| { + records_paged_to(app, "South"); + run_line(app, "add-record-row") + }, + "already here", + records_model_with_two_rows, + |app| { + records_paged_to(app, "West"); + run_line(app, "add-record-row") + }, + ), + ( + "edit-or-drill on an aggregate grid with no cell", + || aggregated_grid(false), + |app| run_line(app, "edit-or-drill editing"), + "No cell at cursor", + || aggregated_grid(true), + |app| run_line(app, "edit-or-drill editing"), + ), + ( + "edit-or-drill on a cell a formula owns", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 1); + run_line(app, "edit-or-drill editing") + }, + "computed by a formula", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 0); + run_line(app, "edit-or-drill editing") + }, + ), + ( + "a search with no matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "zzz".into(); + run_line(app, "search-navigate forward") + }, + "No matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "42".into(); + run_line(app, "search-navigate forward") + }, + ), + ( + "a : line that does not parse", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "no-such-command"), + ":no-such-command", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "add-category Z"), + ), + ]; + for (what, fail_setup, fails, says, ok_setup, succeeds) in cases { + let mut app = fail_setup(); + let refused = fails(&mut app); + assert!( + refused && app.view_state.status_msg.contains(says), + "{what}: expected a refusal saying {says:?}, got refused={refused} {:?}", + app.view_state.status_msg + ); + let mut app = ok_setup(); + assert!( + !succeeds(&mut app), + "{what}: its succeeding twin was refused: {:?}", + app.view_state.status_msg + ); + } + } + /// A prefix is consumed whether or not the next key matches, so a stray key /// after `g` does nothing rather than acting as a fresh Normal binding. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index d9ac1d24..5e7ac188 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -45,8 +45,8 @@ pub enum ViewTarget { Named(String), } -/// Refuse AND stop the batch, for a failed document swap: what follows it in -/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d). +/// Refuse AND stop the batch, where what follows assumes the change happened: +/// `load x . q` must not quit, a bad formula must not close its editor (cz8d). fn refuse_and_stop(app: &mut App, why: impl Into) { app.refuse(why); app.abort_effects = true; @@ -219,9 +219,9 @@ impl Effect for AddFormula { } app.model_state.workbook.model.add_formula(formula); } - Err(e) => { - app.refuse(format!("Formula error: {e}")); - } + // Stop too: what follows it on the key path closes the editor and + // clears the buffer, which would throw the user's text away (v4hg). + Err(e) => refuse_and_stop(app, format!("Formula error: {e}")), } } fn kind(&self) -> EffectKind { diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index f31b1d08..78aac189 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -266,6 +266,27 @@ impl Host { } } +/// One file per way `ImportJsonHeadless` can fail, plus one that imports; the +/// `absent.*` paths are left unwritten. Shared by the frame and key-path tests. +#[cfg(test)] +pub(crate) fn import_fixture_storage() -> crate::storage::MemoryFs { + let storage = crate::storage::MemoryFs::new(); + let files: [(&str, &[u8]); 5] = [ + // Every field numeric, so no category is accepted and the build fails. + ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), + ("bad.json", b"{not json"), + ("nested.json", br#"{"data": [{"Region": "North"}]}"#), + // A record that is not UTF-8 is a CSV read error, not a warning. + ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), + ("good.json", br#"[{"Region": "North", "Revenue": 1}]"#), + ]; + for (name, bytes) in files { + crate::storage::Storage::write(&storage, std::path::Path::new(name), bytes) + .expect("fixture writes"); + } + storage +} + #[cfg(test)] mod tests { use std::path::{Path, PathBuf}; @@ -1946,19 +1967,7 @@ mod tests { /// A host whose storage holds one file per way `ImportJsonHeadless` can /// fail, plus nothing at the two paths that must fail to read. fn host_with_import_fixtures() -> Host { - let storage = MemoryFs::new(); - let files: [(&str, &[u8]); 4] = [ - // Every field numeric, so no category is accepted and the build fails. - ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), - ("bad.json", b"{not json"), - ("nested.json", br#"{"data": [{"Region": "North"}]}"#), - // A record that is not UTF-8 is a CSV read error, not a warning. - ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), - ]; - for (name, bytes) in files { - crate::storage::Storage::write(&storage, Path::new(name), bytes) - .expect("fixture writes"); - } + let storage = super::import_fixture_storage(); // A second view, so deleting an absent one is not refused as "the last". let mut wb = workbook(); wb.create_view("Scratch"); diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 73bd7c14..e2d3c088 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -1606,11 +1606,25 @@ mod tests { /// See `context/test-evidence/command.md`. #[test] fn a_fresh_sessions_slice_is_the_hosts_startup_slice() { - for names in [&["Default", "Second"][..], &["Main", "Second"][..]] { - let app = App::new(opening_on(names, "Second"), None); + for (names, records) in [ + (&["Default", "Second"][..], false), + (&["Main", "Second"][..], false), + (&["Main", "Second"][..], true), + ] { + let mut wb = opening_on(names, "Second"); + if records { + // `Second` opens on a records grid (improvise-y5eo). + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + } + let app = App::new(wb, None); assert_eq!(app.view_state.active_view, "Second", "setup: {names:?}"); + assert_eq!(app.layout.is_records_mode(), records, "setup: {names:?}"); let (_, snapshot) = Sessions::default().connect(&app, 10, 10); - assert_eq!(snapshot.view, app.view_state, "{names:?}"); + assert_eq!( + snapshot.view, app.view_state, + "{names:?}, records {records}" + ); } } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index d88b4e23..fa22ee24 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -620,7 +620,8 @@ enum GridMove { } /// Rows one `page-scroll` step moves: three quarters of the window, at least one. -fn page_rows(window_rows: usize) -> i32 { +/// The one home: the TUI's `Move` calls it too (improvise-5xbq). +pub fn page_rows(window_rows: usize) -> i32 { (window_rows as i32 * 3 / 4).max(1) } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 294b3fc0..07e51ceb 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -266,6 +266,70 @@ fn a_locally_resolved_cursor_move_lands_where_the_host_would_put_it() { ); } +/// Twenty rows by three columns, the host's window eight rows by three: not +/// square, so a rule keyed off the wrong axis lands somewhere else. +fn paging_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for i in 0..20 { + wb.add_item("Region", format!("R{i:02}")).unwrap(); + } + for m in ["A", "B", "C"] { + wb.add_item("_Measure", m).unwrap(); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + let mut app = App::new(wb, None); + app.term_height = 8 + 8; + app.rebuild_layout(); + app +} + +/// Page and half-page scrolls land in the same place on both ends, and in the +/// place the TUI's rule names. Why the fixture and the pinned rows: the evidence file. +#[test] +fn a_locally_resolved_page_scroll_lands_where_the_host_would_put_it() { + let window = (8, 3); + let mut app = paging_host(); + let mut client = client_of(&app, window); + let key = |code, mods| KeyEvent::new(code, mods); + let none = KeyModifiers::NONE; + let ctrl = KeyModifiers::CONTROL; + let presses = [ + (key(KeyCode::Char('j'), none), 1), + (key(KeyCode::Char('j'), none), 2), + (key(KeyCode::PageDown, none), 8), + (key(KeyCode::PageDown, none), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::Char('d'), ctrl), 13), + (key(KeyCode::PageDown, none), 19), + (key(KeyCode::Char('u'), ctrl), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::PageUp, none), 2), + ]; + for (press, (event, want_row)) in presses.into_iter().enumerate() { + match resolve(&client, event) { + KeyOutcome::Local(cmds) => { + for cmd in &cmds { + client.fold(cmd); + } + } + other => panic!("press {press}: the client must answer {event:?}, got {other:?}"), + } + reduce_key(&mut app, event); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "press {press} ({event:?}): client and host disagree" + ); + assert_eq!( + client.view.cursor().selected, + (want_row, 0), + "press {press} ({event:?}): not the row the TUI's page rule names" + ); + } +} + /// The same for `G`, the key `GridExtent` exists for: it is *defined* as "the /// last row", so a window-only client would have nothing to jump to. #[test] diff --git a/harness/build.sh b/harness/build.sh index ed73ca64..666b1165 100755 --- a/harness/build.sh +++ b/harness/build.sh @@ -22,7 +22,11 @@ serve="$root/harness/serve" # re-resolves the lock itself before any `-- --locked` reaches cargo build. # Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it. cargo metadata --locked --format-version 1 \ - --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null + --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null || { + echo "harness/build.sh: harness/opfs-contract/Cargo.lock is stale. Do not remove --locked." >&2 + echo " Run: cargo update -w --manifest-path harness/opfs-contract/Cargo.toml, and commit the lock." >&2 + exit 1 +} "$root/web/build.sh" >/dev/null "$root/web/assemble.sh" "$serve" harness >/dev/null diff --git a/harness/tests/help.spec.js b/harness/tests/help.spec.js index 54b9c961..6a2893e6 100644 --- a/harness/tests/help.spec.js +++ b/harness/tests/help.spec.js @@ -132,4 +132,30 @@ test.describe("the help overlay draws the shared content", () => { await expect(rows_.last().locator("td.selected")).toHaveCount(1); await expect(page.locator("#grid tbody td.selected")).toHaveCount(1); }); + + // improvise-1c5m. Below the width at which the app bar wraps, `#help` covers + // `#hint`, and Welcome itself says nothing about paging. The contract: the + // mode's own hint is UNCOVERED somewhere while help is open. + test("a narrow window still shows how to page through help", async ({ page }) => { + await page.setViewportSize({ width: 600, height: 800 }); + await openHelp(page); + + // The string is the page's own `#hint`, i.e. `hint_line(Help)`; this spec + // spells no keys. Asserting it names paging keeps an empty hint out. + const hint = (await page.locator("#hint").textContent()).trim(); + expect(hint).toContain("next-page"); + + // An element showing exactly that string, topmost at its own centre: the + // overlay covering `#hint` is precisely what `elementFromPoint` sees. + const uncovered = await page.evaluate((text) => + [...document.querySelectorAll("body *")].some((node) => { + if (node.children.length > 0 || node.textContent.trim() !== text) return false; + const r = node.getBoundingClientRect(); + if (r.width === 0 || r.height === 0) return false; + const top = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2); + return top !== null && (top === node || node.contains(top)); + }), + hint); + expect(uncovered, "the paging hint is under the help overlay").toBe(true); + }); }); diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 520315fe..0bda68f8 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -217,6 +217,39 @@ got=$? [ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \ || bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0" +# Every fixture above resolves by poll 2, so a loop that stops after one +# re-poll passed them all (improvise-7epn). This one resolves on poll 3. +d3="$work/arrives-third" +for n in 1 2; do + mk "$d3" "$n.pr.json" < "$work/out" 2>&1 +got=$? +polls=$(cat "$d3/.poll" 2>/dev/null || echo 0) +[ "$got" = 0 ] && [ "$polls" = 3 ] && ok "a review arriving on the third poll is seen" \ + || bad "a review arriving on the third poll is seen" "exit $got after $polls polls, wanted 0 after 3" + +# The timeout path: nothing ever arrives, so --wait 5 on a clock that ticks +# once per read must keep polling until the deadline, then report exit 1. +rm -f "$work/empty/.poll" "$work/ticks" +CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$work/empty" \ + "$REVIEWS" 1 --wait 5 --interval 1 > "$work/out" 2>&1 +got=$? +polls=$(cat "$work/empty/.poll" 2>/dev/null || echo 0) +[ "$got" = 1 ] && [ "$polls" -ge 5 ] && ok "--wait keeps polling until its deadline" \ + || bad "--wait keeps polling until its deadline" "exit $got after $polls polls, wanted 1 after 5 or more" + # --------------------------------------------------------------------------- # `gh api --paginate` emits one array PER PAGE, so a thread past the page size # arrives as several concatenated documents. Passing that straight to diff --git a/src/ui/help.rs b/src/ui/help.rs index 30fffcfd..86e5863b 100644 --- a/src/ui/help.rs +++ b/src/ui/help.rs @@ -6,6 +6,7 @@ use ratatui::{ }; use crate::protocol::help::{self, HelpLine}; +use crate::ui::text::truncate; /// Number of help pages available (defined alongside the help-page state). pub use crate::ui::app::HELP_PAGE_COUNT; @@ -228,66 +229,49 @@ fn render_tab_bar(buf: &mut Buffer, x: u16, y: u16, width: u16, active_page: usi } } -/// Renders the footer with page navigation hints. +/// The page indicator, then Help mode's own hint off the keymap: a footer that +/// spelled its keys was a second table nothing checked (improvise-17ak). fn render_footer(buf: &mut Buffer, x: u16, y: u16, width: u16, page: usize) { let dim = Style::default().fg(Color::DarkGray); - let key_style = Style::default().fg(Color::Cyan); + let hint = crate::protocol::hint_line(&crate::ui::app::AppMode::Help); + let line = format!(" page {} of {} {hint}", page + 1, HELP_PAGE_COUNT); + buf.set_string(x, y, truncate(&line, width as usize), dim); +} - let page_indicator = format!(" page {} of {} ", page + 1, HELP_PAGE_COUNT); +#[cfg(test)] +mod tests { + use super::*; + use crate::ui::app::AppMode; - let nav_parts: Vec<(&str, Style)> = if page == 0 && HELP_PAGE_COUNT > 1 { - vec![ - (" ", dim), - ("l", key_style), - ("/", dim), - ("Tab", key_style), - (": next", dim), - ] - } else if page >= HELP_PAGE_COUNT - 1 { - vec![ - (" ", dim), - ("h", key_style), - ("/", dim), - ("S-Tab", key_style), - (": prev", dim), - ] - } else { - vec![ - (" ", dim), - ("h", key_style), - (": prev ", dim), - ("l", key_style), - (": next", dim), - ] - }; - - let close_parts: Vec<(&str, Style)> = vec![ - (" ", dim), - ("q", key_style), - ("/", dim), - ("Esc", key_style), - (": close", dim), - ]; - - // Render page indicator on the left - buf.set_string(x, y, &page_indicator, dim); - - // Render navigation hints after the page indicator - let mut col = x + page_indicator.len() as u16; - for (text, style) in &nav_parts { - if col >= x + width { - break; - } - buf.set_string(col, y, text, *style); - col += text.len() as u16; + fn rows(buf: &Buffer) -> Vec { + let w = buf.area().width as usize; + buf.content() + .chunks(w) + .map(|row| row.iter().map(|c| c.symbol()).collect::()) + .collect() } - // Render close hint - for (text, style) in &close_parts { - if col >= x + width { - break; + /// The footer's keys are `hint_line(Help)`'s, not a table of its own + /// (improvise-17ak). Why the width and every page: `context/test-evidence/tui.md`. + #[test] + fn the_help_footer_names_the_keys_the_keymap_binds() { + let hint = crate::protocol::hint_line(&AppMode::Help); + assert!( + hint.contains("next-page"), + "the derived hint pages: {hint:?}" + ); + let area = Rect::new(0, 0, 140, 30); + for page in 0..HELP_PAGE_COUNT { + let mut buf = Buffer::empty(area); + HelpWidget::new(page).render(area, &mut buf); + let rows = rows(&buf); + let marker = format!("page {} of", page + 1); + let footer = rows.iter().find(|row| row.contains(&marker)); + let footer = footer.unwrap_or_else(|| panic!("page {page}: no footer row")); + assert!( + footer.contains(&hint), + "page {page}: the footer is not the keymap's hint {hint:?}: {footer:?}" + ); } - buf.set_string(col, y, text, *style); - col += text.len() as u16; } } diff --git a/web/app.js b/web/app.js index 22e94a34..31289ce1 100644 --- a/web/app.js +++ b/web/app.js @@ -348,7 +348,13 @@ function paintHelp(model) { } } - el.help.replaceChildren(tabs, body); + // The mode's own hint again, as the TUI popup's footer: once the app bar + // wraps, the overlay covers `#hint` (improvise-1c5m). + const footer = document.createElement("p"); + footer.className = "help-footer"; + footer.textContent = model.hint; + + el.help.replaceChildren(tabs, body, footer); } /** The page bar: which item each fixed category is fixed to. Without it a diff --git a/web/style.css b/web/style.css index 88ed64cc..6d02e7d1 100644 --- a/web/style.css +++ b/web/style.css @@ -736,6 +736,17 @@ td.total { font-weight: 700; } +/* Sticky, so the paging keys stay in view however long the page scrolls. */ +.help-footer { + position: sticky; + bottom: -1.25rem; + margin: 0.75rem 0 0; + padding: 0.5rem 0; + border-top: 1px solid var(--line); + background: var(--bg); + color: var(--dim); +} + .help-body { display: grid; grid-template-columns: 16rem 1fr;