Skip to content

Settings: a read-only window still draws the write controls on seven pages - #2479

Merged
Birfy merged 3 commits into
ThinkFlowLab:mainfrom
HugoXK:dev/3-settings-pages-readonly
Nov 5, 2026
Merged

Birfy merged 3 commits into
ThinkFlowLab:mainfrom
HugoXK:dev/3-settings-pages-readonly

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

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/** or core/client.js; risk-gate reports T1 with why: [] (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) that details-drawer.js, create-bot.js, schedules.js and the two Projects pages already use (schedules.js:9, imported at settings.js:20), and draws a plain-text view instead: no select, no input, 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-only with role="status" — is untouched and still appears once per dialog header; the fix does not copy it into any section.

The seven pages

page what was drawn what the locked view draws now
General (settings.js:49, GeneralPage) Theme select, accent swatches and the custom input, four notification SwitchRows — every one writes set-prefs Appearance rows naming the theme and the accent in use, Notifications rows saying On/Off per kind
Bots list (settings.js:127, BotsPage) MainBotsSection (:91): the Main Bot select (setMain(…, {replace})), Move to first, Remove Main Bot role, Add a Main Bot; MotionSection (:117): Quiet/Normal/Lively Segmented (setPrefs({motion})) Main Bot names the Bot that holds the role and every additional one (Also a Main Bot.); Animation names the step in use. New Bot stays: create-bot.js has been locked since #2465, so the entry opens a form that is already read-only
Bots detail (bot-settings.js) InlineText fields, ModelMenu, FolderMenu, SwitchRows, capability/connector/schedule cards, standing approvals name, label, main-Bot tag, model, working folder and instructions as text; no control
Group chats detail (group-settings.js) name and notice InlineText, Segmented who-answers, FolderMenu, per-member buttons, add/remove member, standing approvals, switches, delete name and subtitle, the member list (each Bot with Admin/Task lead tag, humans, friends' Bots) and the notice as text
Connectors (connectors.js) one ConnectorRow per connector: connect, disconnect, retry, send, allow, block, apps each connector's name and its status sentence as text
Capabilities (capability-settings.js) CapabilityRow per saved capability: save, delete, approve name and description (pack items, local command or HTTP server) as text
Secrets (secret-card.js) SecretRow per key: set, replace, delete, cancel, allow the about paragraph plus each stored name, purpose and scope label as text

Why these are dead controls, not merely inconvenient ones

core/src/host/channel.js:237-239 lists the endpoints a read-only window may still call in READS; neither set-prefs nor set-main is in it, and :307 answers every other endpoint with fail(readOnly, 'bot/read-only'). The same holds for update-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:

# PROBE_BOTS_PAGE_WRITES {"locked":{"selects":["Main Bot","Add a Main Bot"],"roleButtons":[],"motion":["Quiet","Normal","Lively"],"createEntry":["New Bot"]},"writable":{"selects":["Main Bot","Add a Main Bot"],"roleButtons":[],"motion":["Quiet","Normal","Lively"],"createEntry":["New Bot"]}}
# PROBE_BOTS_PAGE_CALLS [["setMain","b",true,{"replace":"a"}],["setPrefs",{"motion":"quiet"}]]

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:

"bots:chief": { "locked": "ok",
  "unlocked": "threw: Rendered more hooks than during the previous render.",
  "html_len_after": 0 }

html_len_after: 0 means the throw unmounted the whole Settings dialog, not only that page. The locked view of BotSettingsForm returned before its refreshModels effect, so a locked render ran four hooks and a writable one five — while settings.js keeps 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 2eca1c3222ea3b58b5feaa617868af03d9e27ac4 with only core/test/settings-readonly-pages.test.js added, core/:

TZ=UTC node --import ./test/harness-resolve.mjs --test test/settings-readonly-pages.test.js

# tests 4 / # pass 2 / # fail 2, and the two failures are the defect:

not ok 1 - a read-only window draws no write control on any of the seven Settings pages (#2474)
  error: a read-only window draws no control
  actual: ['GeneralPage: select', 'GeneralPage: input', 'GeneralPage: SwitchRow' ×4,
           'BotsPage: select' ×2, 'BotsPage: Segmented',
           'BotSettingsForm: InlineText' ×3, 'ModelMenu', 'FolderMenu', 'FlyBotDetails',
           'BotCapabilitiesCard', 'BotConnectorsCard', 'BotSchedules', 'StandingApprovalsCard', 'SwitchRow',
           'GroupSettingsForm: InlineText' ×2, 'Segmented', 'FolderMenu', 'FlyBotRoomMembers',
           'StandingApprovalsCard', 'SwitchRow' ×2,
           'ConnectorsPage: ConnectorRow', 'CapabilitiesPage: CapabilityRow', 'SecretsPage: SecretRow']
  expected: []
not ok 2 - a read-only Bots page draws no Main Bot or animation control, and sends neither write (#2474)
  error: no Main Bot picker
  2 !== 0

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 BotSettingsForm fails by throwing, # tests 5 / # pass 4 / # fail 1:

not ok 5 - every Settings page re-renders when the window unlocks under it (#2474)
  error: 'Rendered more hooks than during the previous render.'
  stack: hook -> useEffect -> Object.BotSettingsForm [as type]
# 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 (tree ba0e699ef4aad69721ee02317ef23856018bd655, base 2eca1c3222ea3b58b5feaa617868af03d9e27ac4). 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 --porcelain empty) at that tree:

TZ=UTC ARCH_DOCS_BASE=2eca1c3222ea3b58b5feaa617868af03d9e27ac4 npm run verify
node scripts/ds-bot-patches.mjs generate    # second run: git status --porcelain empty

verify exit 0:

  • core 2895 pass / 0 fail; plugins/social 577; plugins/tree 137; plugins/timeline 141; plugins/workflow 427; .github/scripts + policy 671; interop 37; live-e2e 26 — 4911 tests, 0 fail.
  • arch-docs: OK (18 changed file(s)) (docs/subsystems/core-client.md +1).
  • build-client --check ok; 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).
  • lint 0 errors (core 78 warnings, plugins 36/9/2/16 — all pre-existing).

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:

file lines
core/src/client/settings.js +25
core/src/client/bot-settings.js +23/−3
core/src/client/group-settings.js +26/−1
core/src/client/connectors.js +12
core/src/client/capability-settings.js +15
core/src/client/secret-card.js +17
core/src/client/i18n.js +2 (On, None)
core/test/settings-readonly-pages.test.js (new) +273
docs/subsystems/core-client.md +1
changelog.d/2474-settings-pages-readonly.fixed.md (new) +1

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 through remoteBotName (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"
}

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
@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Test Self-test: FAIL at dd66b71 (tree 24c9b32103ded69df5dfcb151575a8f88240233e, base 2eca1c32) — one must-fix, everything else green.

Dual-path: git ls-remote → dd66b71e…; REST (Date 01:06:46Z) → same sha, draft, mcm true, 2 commits, 18 files +4933/−3508; pulls/2479/files (Date 01:05:49Z) matches the body line by line. Risk recomputed with base's risk-rules.json: T1 (10 non-generated files, 353 lines).

TZ=UTC ARCH_DOCS_BASE=2eca1c32… npm run verify → exit 0, 4910 tests / 0 fail (core 2894, workflow 427); arch-docs: OK (18 changed file(s)); build-client --check passes; patches clean + equivalence + no-op; lint 0 errors.

Red-first (clean base + only the new test): 4 tests / 2 pass / 2 fail — a read-only window draws no control (31 controls listed) and no Main Bot picker 2 !== 0; 4/4 on the tip.

The seven pages were also mounted with a different roster shape (real components, real SettingsDialog, not the repo's test data): locked → 0 controls, 0 write actions, exactly one bt-read-only sentence with the host's own wording, no render errors (same for an empty roster); writable and readOnly: ' ' (whitespace only) keep their controls (General 6 / Bots 3 / Bot 10 / Group 8 / Connectors 1 / Capabilities 1 / Secrets 1); the New Bot entry stays on the Bots page (intentional per #2465). Static scan of the 8 lockedWindow branches: no select/input/textarea/button, no InlineText/SwitchRow/Segmented/ModelMenu/FolderMenu.

Must-fix (1, core/src/client/bot-settings.js): BotSettingsForm's read-only branch is an early return that crosses hooks — lines 199/200/201/202 call useState, useState, useCallback, useEffect before the if (lockedWindow(roster)) return … at 213, while line 233 has useEffect(() => { void actions.refreshModels?.() }, [bot.id]) after it. The same instance renders 4 hooks locked and 5 writable, violating React's hook order. Concretely: open a Bot's Settings page in the locked window, close the other window → channel.js:309 awaits rt.refresh() on every request, refresh() calls claimLock() and gets the empty lock → readOnly becomes null in the same poll's reply (channel.js:200) → lockedWindow flips false → settings.js:267 reuses the same instance under key: bot.id (which does not include readOnly) → React throws "Rendered more hooks than during the previous render." and the whole Settings tree (possibly the page) goes blank. This is the PR's own target scenario (read-only window → other window closes). Fix: move the line-233 useEffect above the locked branch (or split the locked view into a child component); the other five components have all their hooks before the early return and none after, so their count is constant.

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 ok/ok on the same flip with html_len_after 6318–10150. html_len_after: 0 means the error unmounts the whole Settings tree (blank), not just that page.

Repro recipe: esbuild-bundle core/src/client/settings.js with react/react-dom external; jsdom with document/window/rAF, IS_REACT_ACT_ENVIRONMENT=true, createRoot + React.act; render SettingsDialog({page:'bots', id:'chief'}) with roster.readOnly = <host sentence>, then re-render on the same root with readOnly absent.

Everything else passes, so this is the only blocker. New Bot being kept is intentional per #2465, not a miss.

…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.
@HugoXK
HugoXK marked this pull request as ready for review November 5, 2026 01:33
@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request — tip 01fdf867365ea12c84452d042f2462454ab2c171, base 2eca1c32.

State read at this tip (Date 2026-11-05T02:03:07Z on the check-runs response): 17 success + 0 cancelled, including enqueue-guard (success, 01:18:46Z), merge-gate, tests-ok, policy-sign, and risk tier (report only) = T1. PR is non-draft, mergeable: true / clean, maintainer_can_modify: true, head branch HugoXK:dev/3-settings-pages-readonly (ours).

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 dd66b71e, which is the parent commit. Test re-ran at 01fdf867 and reported PASS (verify 4911/0) to the Lead directly; nothing was pushed after that.

@Birfy
Birfy added this pull request to the merge queue Nov 5, 2026
Merged via the queue into ThinkFlowLab:main with commit 924f623 Nov 5, 2026
17 checks passed
@Birfy
Birfy deleted the dev/3-settings-pages-readonly branch November 5, 2026 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Settings: a read-only window still draws the write controls on seven pages

2 participants