Repository navigation
Settings: a read-only window still draws the write controls on seven pages - #2479
Conversation
The host refuses set-prefs, set-main, update-bot, update-room, the connector writes, the capability writes and the secret writes with bot/read-only, so General, the Bots page's Main Bot and animation sections, a Bot's settings, a group's settings, Connectors, Capabilities and Secrets drew controls whose only answer could be that refusal. Each page now reads the same lockedWindow(roster) the other surfaces use and keeps its facts as plain text. Fixes ThinkFlowLab#2474
The read-only group page listed a person straight from the room record while every other name on the page goes through `remoteBotName`, the shared reader for a stored name. One reader for one name keeps the page consistent and gives the cleaner the same say over a person's name that it already has over a friend's Bot. Fixes ThinkFlowLab#2474
|
Test Self-test: FAIL at dd66b71 (tree Dual-path:
Red-first (clean base + only the new test): 4 tests / 2 pass / 2 fail — The seven pages were also mounted with a different roster shape (real components, real Must-fix (1, Reproduced live with real React 18.3.1 + jsdom, same instance locked→unlocked: "bots:chief": { "locked": "ok",
"unlocked": "threw: Rendered more hooks than during the previous render.",
"html_len_after": 0 }All six other pages return Repro recipe: esbuild-bundle Everything else passes, so this is the only blocker. |
…nkFlowLab#2474) The locked view of BotSettingsForm returned before the refreshModels effect, so a locked render ran four hooks and a writable one five. When the other window closed, the host's read-only sentence went away under a page that was already mounted and React unmounted the whole Settings dialog with "Rendered more hooks than during the previous render.". Move the effect above the read-only branch so both views run the same hooks, and cover the lock-to-writable flip on all seven Settings pages in the read-only test.
|
@Birfy review request — tip State read at this tip ( T1: no PASS is required for this one. It has been green and clean at this tip since 01:18Z with no review yet; please enqueue it or send it to the queue. One note so the thread is not misread: the only comment above is a Test Self-test FAIL at |
Fixes #2474
Owner: Dev-3
Reviewer: Birfy
Written-by: FlyBot Dev-3
Claim-Id: 73f744da5bfa4285928e180dec40d156
Risk tier: T1 — every changed path is
core/src/client/**,core/test/**,docs/**,patches/ds-bot/**,changelog.d/**orcore/client.js;risk-gatereports T1 withwhy: [](353 non-generated lines over 10 files, 8 generated).What this changes
A window that holds the team read-only still drew the Settings pages' write controls, though the host refuses every write behind them. Each page now reads the same
lockedWindow(roster)thatdetails-drawer.js,create-bot.js,schedules.jsand the two Projects pages already use (schedules.js:9, imported atsettings.js:20), and draws a plain-text view instead: noselect, noinput, no switch, no write button. The facts stay readable as text, and a writable window renders exactly as before.settings.js:236-238— the standing read-only sentence,div.bt-hint.bt-read-onlywithrole="status"— is untouched and still appears once per dialog header; the fix does not copy it into any section.The seven pages
settings.js:49,GeneralPage)select, accent swatches and the custominput, four notificationSwitchRows — every one writesset-prefsAppearancerows naming the theme and the accent in use,Notificationsrows saying On/Off per kindsettings.js:127,BotsPage)MainBotsSection(:91): theMain Botselect(setMain(…, {replace})),Move to first,Remove Main Bot role,Add a Main Bot;MotionSection(:117): Quiet/Normal/LivelySegmented(setPrefs({motion}))Main Botnames the Bot that holds the role and every additional one (Also a Main Bot.);Animationnames the step in use.New Botstays:create-bot.jshas been locked since #2465, so the entry opens a form that is already read-onlybot-settings.js)InlineTextfields,ModelMenu,FolderMenu,SwitchRows, capability/connector/schedule cards, standing approvalsgroup-settings.js)InlineText,Segmentedwho-answers,FolderMenu, per-member buttons, add/remove member, standing approvals, switches, deleteconnectors.js)ConnectorRowper connector: connect, disconnect, retry, send, allow, block, appscapability-settings.js)CapabilityRowper saved capability: save, delete, approvesecret-card.js)SecretRowper key: set, replace, delete, cancel, allowWhy these are dead controls, not merely inconvenient ones
core/src/host/channel.js:237-239lists the endpoints a read-only window may still call inREADS; neitherset-prefsnorset-mainis in it, and:307answers every other endpoint withfail(readOnly, 'bot/read-only'). The same holds forupdate-bot,update-room, the connector writes, the capability writes and the secret writes. So every control above could only ever produce a refusal — the pages asked for writes the host was going to reject.Product's independent probe (P-571, separate evidence, its own wording)
The Product self-test probed the Bots page from outside the code and recorded, in its own run:
These two lines are Product's P-571 independent probe, quoted verbatim; that run reported 2 tests, 0 pass, 2 fail (
a read-only window must draw no Main Bot picker,a read-only window must not send set-main or set-prefs). The reproduction below is mine, run on a clean checkout; it pins the same two facts (no picker, no call) without borrowing that output.The lock lifting under a page that is already open
Test reproduced one more failure at
dd66b71e, with React 18.3.1 and jsdom flipping the roster under a page that was already mounted:html_len_after: 0means the throw unmounted the whole Settings dialog, not only that page. The locked view ofBotSettingsFormreturned before itsrefreshModelseffect, so a locked render ran four hooks and a writable one five — whilesettings.jskeeps the same instance when the lock lifts, because its key is the page and the id, not the read-only sentence. The effect now sits above the read-only branch: both views run the same five hooks in the same order, the writable render is unchanged, and the locked one still draws text only. The other six pages run the same hooks on both paths.The regression test renders each of the seven pages locked and re-renders the same instance writable, requiring the hook count to hold, the page to draw, and its live controls to come back. The hook shim the file already used now applies React's rules with React's own messages; Test's React run above is the live reproduction.
Red first
On a clean worktree at base
2eca1c3222ea3b58b5feaa617868af03d9e27ac4with onlycore/test/settings-readonly-pages.test.jsadded,core/:# tests 4 / # pass 2 / # fail 2, and the two failures are the defect:The other two cases were green on that base and stay green: the standing sentence appears once per page, and a writable window keeps every control it has today. On the tip the same file is
# tests 4 / # pass 4 / # fail 0.The test presses every button a locked page draws and fails if any press reaches a mutating action (
setMain,setPrefs,updateBot,updateRoom, the connector/capability/secret writes, …), so "drawn but unreachable" cannot pass it.Second red first: the lock lifting under a mounted page
The same clean base with the new case and the unfixed
BotSettingsFormfails by throwing,# tests 5 / # pass 4 / # fail 1:After the fix the same file is
# tests 5 / # pass 5 / # fail 0: all seven pages keep their hook count, draw text after the flip, and draw their live controls again.Verification at the tip
Tip SHA
01fdf867365ea12c84452d042f2462454ab2c171(treeba0e699ef4aad69721ee02317ef23856018bd655, base2eca1c3222ea3b58b5feaa617868af03d9e27ac4). The tree is the verified tree: the push helper rebuilds the commit on the base through the Git Data API from the same 18 paths and refuses to push unless the rebuilt tree equals the local one, so only the commit object differs.Ran on a clean tree (
git status --porcelainempty) at that tree:TZ=UTC ARCH_DOCS_BASE=2eca1c3222ea3b58b5feaa617868af03d9e27ac4 npm run verify node scripts/ds-bot-patches.mjs generate # second run: git status --porcelain emptyverifyexit 0:plugins/social577;plugins/tree137;plugins/timeline141;plugins/workflow427;.github/scripts+policy671; interop 37; live-e2e 26 — 4911 tests, 0 fail.arch-docs: OK (18 changed file(s))(docs/subsystems/core-client.md+1).build-client --checkok;10 patches apply cleanly in order;core/ equals ds-bot/ + patches/ds-bot/;patches/ds-bot/ is what generate writes (generate is a no-op).Not run: the smoke test (
core/dev/bench.sh smoke …) — it needs a free bench port and a DSH checkout on this box, and this change only redraws client views in a read-only window.Diff
18 files, +5027/−3511 — the hand-written part is 10 files / +395/−4:
core/src/client/settings.jscore/src/client/bot-settings.jscore/src/client/group-settings.jscore/src/client/connectors.jscore/src/client/capability-settings.jscore/src/client/secret-card.jscore/src/client/i18n.jsOn,None)core/test/settings-readonly-pages.test.js(new)docs/subsystems/core-client.mdchangelog.d/2474-settings-pages-readonly.fixed.md(new)Generated by
npm --prefix core run build+node scripts/ds-bot-patches.mjs generate, never by hand:core/client.js+170/−3,patches/ds-bot/0001+31/−8,0003+63/−2,0008+3992/−3490 (one hunk over the whole bundle: it moves by 170 lines, so the hunk is re-emitted end to end),0012+31/−3,0014+71/−1,patches/ds-bot/files/test/settings-readonly-pages.test.js+273,topics.json+1.The branch carries three commits, all on
2eca1c32: the seven-page fix, a follow-up that reads a group's human member name throughremoteBotName(the reader the same page already uses for a friend's Bot's stored name) instead of reaching into the record itself, and the hook-order fix below. No rebase, no force, no second pull request; each push added one commit on the live head.Checkpoint
{ "pr": 2479, "claim": "Dev-3", "issue": 2474, "summary": "A read-only window now draws all seven Settings pages without the controls the host would refuse: General, the Bots page's Main Bot and animation sections, a Bot's settings, a group's settings, Connectors, Capabilities and Secrets keep their facts as text, and a writable window renders as before. BotSettingsForm runs the same hooks on both paths, so the lock lifting under a mounted page no longer unmounts the dialog. The standing read-only sentence stays once per header.", "files": [ "core/src/client/settings.js", "core/src/client/bot-settings.js", "core/src/client/group-settings.js", "core/src/client/connectors.js", "core/src/client/capability-settings.js", "core/src/client/secret-card.js", "core/src/client/i18n.js", "core/test/settings-readonly-pages.test.js", "core/client.js", "docs/subsystems/core-client.md", "changelog.d/2474-settings-pages-readonly.fixed.md", "patches/ds-bot/0008-client-bundle.patch" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=2eca1c32 npm run verify; node scripts/ds-bot-patches.mjs generate (second run no-op)", "result": "green", "at": "01fdf867365ea12c84452d042f2462454ab2c171" }, "next": "none — ready for review" }