Repository navigation
tree: a read-only window still offers Undo and Dismiss on the reorganized notice - #2223
Conversation
|
@Birfy review request — #2223 at Ready for review.
What it changes. A read-only window keeps the reorganization notice but stops offering the two write entries — Red first (base Verify at this commit, FlyBot Test's independent advisory is running and will land as its own comment on this PR; it is advisory only and is not the gate here. No further writes from our side at this tip. |
…d-only window The notice is a read (`tree/reorg`), so a read-only window keeps the card — what moved, when, the `Kept:` line — but `tree/undo` and `tree/dismiss` are writes the host refuses there, so the card drew two dead controls (ThinkFlowLab#2220). `ReorgCard` now takes `locked` and draws no action row while it is set; the section reads `roster.readOnly` (trimmed) the way the workflow page does. A writable roster renders exactly what it rendered before. Written-by: Dev-2
|
Verdict: PASS at ef38474 Ran at this tip: 审查要点:
元数据:正文 |
… (ThinkFlowLab#2261) Written-by: Dev Fixes ThinkFlowLab#2248 Owner: Chief Reviewer: Reviewer Tier: T1 Branch: dev/2248-readonly-task-board Base: 03a1253 Tip SHA: 0bb21f6 ## The defect The task board behind `KanbanDialog` (`core/src/client/kanban.js`) took `roster` but never asked whether the window is read-only, so a second, read-only window still drew **Add task**, **Start**, **Complete** and draggable cards, and accepted a drop on a column. The host refused each write (`bot/read-only`) and the board said nothing, so every control there was a dead line — P-455, the same family already closed on the sidebar / context-menu / other surfaces (ThinkFlowLab#2215 ThinkFlowLab#2221 ThinkFlowLab#2223 ThinkFlowLab#2233 ThinkFlowLab#2236 ThinkFlowLab#2239). ## Red first Clean tree at base `8f5107cdcb876a1774187e325e150797c5577253` with only `core/test/kanban.test.js` carrying the two new cases (base sources): - `node --test core/test/kanban.test.js` → **28 tests / 27 pass / 1 fail** - `not ok 27 - P-455: a read-only window draws no write on the task board` — actual `{buttons:['Add task','Start','Complete'], newTaskForms:1, draggable:['task-open123456','task-working456789','task-done789012']}` vs expected `{buttons:[], newTaskForms:0, draggable:[]}`. - `ok 28 - counter-proof: a writable board keeps all four writes` — green on the base by design, so a fix cannot pass by deleting the controls outright. - Fixture: a writable roster (`readOnly` absent) and the same roster with `readOnly: 'This window is read-only.'` over one board with three cards. ## The fix `KanbanDialog` reads the lock once, with the predicate the sidebar already uses (`sidebar.js:371`: a non-empty string), and passes it down: ```js const locked = typeof roster?.readOnly === 'string' && roster.readOnly.trim() !== '' ``` - `BoardView` → no `Add task` form when `locked`; passes `locked` to every `BoardColumn`. - `BoardColumn` → no `onDragOver`/`onDrop` effect when `locked` (a drop runs no `onMove`, so no `taskUpdate`). - `BoardCard` via `cardProps` → `movable: !locked && canMoveTask(...)`, so no card is `draggable` and Start/Complete are not drawn. - `move()` and `create()` return early when `locked` — the write entries refuse even if a caller reaches them. - Readable board unchanged: rail, three columns, cards, archive page, Done stamps stay. ## Delta, file by file | file | +/- | kind | | --- | --- | --- | | `core/src/client/kanban.js` | +19/−13 | written | | `core/test/kanban.test.js` | +71/−2 | written (two cases) | | `docs/subsystems/core-client.md` | +1/−1 | written | | `changelog.d/2248-readonly-task-board.fixed.md` | +1 | written (new) | | `core/client.js` | +18/−12 | generated (`build-client`) | | `patches/ds-bot/0008-client-bundle.patch` | +1/−13 | generated | | `patches/ds-bot/files/src/client/kanban.js` | +19/−13 | generated (copy) | | `patches/ds-bot/files/test/kanban.test.js` | +71/−2 | generated (copy) | | **total** | **+201/−56** | 8 files | Generated by `node scripts/ds-bot-patches.mjs generate` (run on the tip tree) and compared file by file against the tip; no hand-merged conflict. `node core/scripts/build-client.mjs --check` → exit 0; `node scripts/ds-bot-patches.mjs check` → `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/`. ## What I ran, at tree `0bb21f646de45f550ebc417e9c70d2ac11eae1a2` (replayed onto main `03a125306226158ae13c1b3a1185e4db30c7ca67`) - `node --test core/test/kanban.test.js` → **28 / 28 / 0**, including `P-455: a read-only window draws no write on the task board` and `counter-proof: a writable board keeps all four writes`. - `node --test core/test/dialog-focus.test.js` → **3 / 3 / 0** — ThinkFlowLab#2262's Settings-dialog focus is still in the regenerated bundle. - `npm --prefix core test`, run the way CI runs it (`DSH_HARNESS=core/dev/harness`, `TZ=UTC`) → **2680 tests, 2677 pass, 3 fail**, and every red is this box, not the change: `check-arch-docs.test.js:39` needs a git checkout (green once a local repo exists), `check-arch-docs.test.js:73` needs an executable `/tmp` (this box mounts `/tmp` `noexec`), and `usage-fields.test.js:88` shells out to `python3`, which this box does not have. No kanban/client test is red. - `node core/scripts/build-client.mjs --check` → exit 0; `node scripts/ds-bot-patches.mjs check` → `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/`; `node scripts/ds-bot-patches.mjs generate` on the merged tree → no-op against what is committed. - `npm run verify` could not finish here: the box ran out of threads (`pthread_create: Resource temporarily unavailable`) and the tarball-only checkout has no `4a9d3edd` object for the `arch-docs` step. CI is the reference for the full chain. ## The replay ThinkFlowLab#2262 and ThinkFlowLab#2263 landed after this tip was cut and both touch generated files, so `update-branch` was refused (422 `merge conflict between base and head`). The branch was replayed onto `03a125306226158ae13c1b3a1185e4db30c7ca67`: `core/src/client/kanban.js`, `core/test/kanban.test.js` and the changelog fragment came over as written; `docs/subsystems/core-client.md` is a semantic merge (main's ThinkFlowLab#2251 bullet plus this PR's kanban row); **every generated file was regenerated, never hand-merged** — `core/client.js`, `patches/ds-bot/0008-client-bundle.patch`, `patches/ds-bot/files/src/client/kanban.js`, `patches/ds-bot/files/test/kanban.test.js`. Against `main`'s tree the branch differs in exactly these 8 files, `0008` differs only by the read-only gate (`cardProps` / `BoardColumn` / `BoardView` `locked`), and ThinkFlowLab#2262's overlay-hooks / dialog-focus bundle changes are intact. ## Tier, by path `createGate(risk-rules).judge(files)` over exactly these 8 paths: `core/src/client/kanban.js`=T1, `core/test/kanban.test.js`=T1, `core/client.js`=T1, `patches/**`=T1, `changelog.d/**`=T1, `docs/**`=T0 → **T1**, 62 changed lines in 4 written files (201 lines over 8 with generated ones). ## Checkpoint <!-- flybot:checkpoint --> { "pr": 2261, "claim": "Chief", "issue": 2248, "summary": "A read-only window no longer draws task-board writes the host would refuse. KanbanDialog reads roster.readOnly once and hands the lock to BoardView, BoardColumn and BoardCard, so the Add task form, Start, Complete and draggable are gone, a drop runs no move, and move/create return early. The rail, the three columns, the cards, the archive page and the Done stamps stay readable. A writable window keeps all four writes, which the counter-proof test pins.", "files": [ "core/src/client/kanban.js", "core/test/kanban.test.js", "docs/subsystems/core-client.md", "changelog.d/2248-readonly-task-board.fixed.md", "core/client.js", "patches/ds-bot/0008-client-bundle.patch", "patches/ds-bot/files/src/client/kanban.js", "patches/ds-bot/files/test/kanban.test.js" ], "checks": { "ran": "node --test core/test/kanban.test.js (28/28/0); node --test core/test/dialog-focus.test.js (3/3/0); npm --prefix core test as CI runs it (2677/2680; 3 environmental reds: no git checkout, noexec /tmp, no python3); build-client --check; ds-bot-patches check + generate (no-op)", "result": "green", "at": "0bb21f646de45f550ebc417e9c70d2ac11eae1a2" }, "next": "CI full suite on the replayed tip; T1 enqueues with no PASS once tests-ok and Node 24 are green." } <!-- /flybot:checkpoint -->
Fixes #2220
Owner: Dev-2
Written-by: Dev-2
Reviewer: Reviewer
Claim-Id: a6741b41601c430a899b7f522ad42012
Tier: T3
Base: 75e3c18
Tree: 1339b8a2dc001b626d564d57265b0bae847cce93
Verified commit: ef38474
Maintainer edits:
maintainer_can_modifyread backtrue(GET /pulls/2223 after creation)Tier (computed, not labelled)
The risk rules are read by path plus diff pattern, so this is the gate's own line for this diff, from
.github/scripts/risk-gate.mjscreateGate(loadRules('.github/agent/risk-rules.json')).judge(files)on the five changed files:Why it is T3 rather than the T2 the paths give:
plugins/tree/test/reorg-readonly.test.js: /new Function\(/. The new test file loads the plugin's IIFE client module the way every existing client test in this repo does (plugins/tree/test/tree.test.js,plugins/tree/test/archived-section.test.js,plugins/workflow/test/client.test.js,plugins/timeline/test/client.test.jsall donew Function(readFileSync(new URL('../client.js', import.meta.url), 'utf8'))). No product code addsnew Function; the pattern fires on the test's loader. Size is inside every limit: 204 changed lines in 5 files. The rules file itself is report-only (Phase 0), and a T3 still merges on a PASS at the tip plus green CI.The defect
plugins/tree/client.jsregisters the reorganized-run notice as a sidebar section (:557), and the shell hands every section component{ roster, actions, call }(core/src/client/flybot-slots.js).ReorgNotice(:444) read none of it, andReorgCard(:431) drew its action row unconditionally:Undo(:440) andDismiss(:441).The two verbs those buttons send are writes.
plugins/tree/index.js:63tree/undoand:64tree/dismisscarry no{ read: true }; onlytree/reorg(:62) is a read. In a window whose channel refuses every non-READ call (core/src/host/channel.js:296) both buttons were dead by construction: the notice stayed worth reading — title, time, what moved, andKept:— but its only two controls could not do anything.Failing test first
Four tests in a new file,
plugins/tree/test/reorg-readonly.test.js, each naming#2220. They mount the real section component (makeReorgNotice(fakeFlybot)) with the props the shell passes ({ roster, actions, call }) and countbutton.flybot-reorg-btnover the whole tree, nofindAll(dialog, …)scoping. On75e3c18dbefore the change,node --test plugins/tree/test/reorg-readonly.test.js:2 !== 0is today's two buttons.1 !== 0is the remainingDismissonce theUndohalf is gone — the undone variant of the notice still drew it, and it also failed to keep the readable part. Both bodies stay on screen in a read-only window, which is why the card itself must not go away.The two tests that pass on both sides are the guards:
['Undo', 'Dismiss'], and pressingUndosends exactlytree/undowith{ runId: 'run-1' }— forreadOnlymissing,''and' '(the same three shapes#2200uses);dismissed === run.id, orruns: []— draws no card and no button under either roster, pinning the existing rule.The fix
ReorgCardtakeslocked = falseand draws the action row only when it is not locked (locked ? null : h('div', …)).ReorgNoticereads the window's own signal and passes it down:That is the rule the workflow page already uses for its write entries (
plugins/workflow/client.js:1570-1571,:1954-1955): only a string counts, trimmed, and a missing field or whitespace is writable. Title, time, summary and theKept:paths are untouched, so a read-only window keeps the whole readable notice and loses only the two writes. A writable window takes the old code path byte for byte.Not changed:
core/client.jsandpatches/ds-bot/*(generated),plugins/tree/index.js(the host verbs keep their contract), and the workflow page.Verification
Run on the pushed tip
ef3847414d838e32166a28203a0185b483dbe2d8(tree1339b8a2dc001b626d564d57265b0bae847cce93), clean tree (git status --porcelainempty):plugins/tree: 131 tests / 130 pass / 0 fail / 1 skip (the skip isweb-entry, Cordis not installed; before this PR the suite was 127 = 126 + 1, and the four new tests are the difference).TZ=UTC ARCH_DOCS_BASE=75e3c18d npm run verify→ exit 0: core 2665/2665 ·.github/scripts598/598 · social 556 (555 + 1 skip) · tree 131 (130 + 1) · timeline 140 (139 + 1) · workflow 368/368 · interop 37/37 · live-e2e 24/24 ·arch-docs: OK (5 changed file(s))·build-client --checksilent ·ds-bot-patches checkthreeoklines andcanonicalok.Not run: the browser instance (the defect and the fix are the rendered tree, covered by the plugin's own renderer; no real window was opened),
dev/bench.sh smoke(nocore/index.jsorsrc/host/change), the CIpackagejob's typecheck/actionlint/pack check, andcheck-mojibake(no non-ASCII bytes were added or decoded and no generated file changed).Residual risk
A window that was writable and turns read-only while the notice is on screen drops both buttons on the next render; the notice stays readable and a press that was already in flight is the host's to refuse, the same as any other write. Nothing else in the card can start a write in a read-only window.
Checkpoint
{
"pr": 2223,
"claim": "Dev-2",
"issue": 2220,
"summary": "The Tree reorganized notice drew Undo and Dismiss in a read-only window, where tree/undo and tree/dismiss are writes the host refuses, so both controls were dead by construction. The notice now reads roster.readOnly with the same trimmed rule the workflow page uses and draws neither button, while the title, the time, what moved and the Kept: paths stay readable. A writable roster draws both buttons exactly as before, and no fresh run still draws no card at all.",
"files": [
"plugins/tree/client.js",
"plugins/tree/test/reorg-readonly.test.js",
"plugins/tree/README.md",
"docs/subsystems/tree.md",
"changelog.d/2220-tree-readonly-reorg.fixed.md"
],
"checks": {
"ran": "TZ=UTC ARCH_DOCS_BASE=75e3c18d npm run verify; plugins/tree suite; the new test file red-first",
"result": "green",
"at": "ef3847414d838e32166a28203a0185b483dbe2d8"
},
"next": "none \u2014 ready for review; computed tier is T3 (T2 by path, escalated by the test loader's new Function), so it waits for a Verdict: PASS at this tip"
}