fix(admin): make the plugin and theme screens keep the panel's promises - #123
Merged
Conversation
The panel's banner says "Anything destructive will ask again", and the user, group and forum screens honour it with `requireFreshAdmin()`. Nothing in themes or plugins did. Theme **reset** and **import** each replace every stored override for a theme in one press, with no undo — the import screen itself says "Take a copy of the export above first" — so both now take the fresh gate. Turning a plugin off takes a live capability away from every member of the board at once, durably and on every instance, so disabling takes it too. The reversible controls beside them keep the plain admin gate: activating a theme, moving the default, saving a palette from the editor, saving plugin settings, and enabling a plugin, which is the undo for the one gate that was added. An env-owned plugin setting was silently overwritten whenever it was a boolean. The guard that skips settings the environment owns read `raw === null && type !== 'boolean'`, exempting exactly the kind that cannot be submitted: an env-owned checkbox renders disabled, so it is absent from the POST, so every save stored a `'0'` override under a box that displayed "on" from the environment — the deferred surprise the guard existed to prevent, since removing the variable later would flip the plugin to a stored value nobody chose. The save now resolves each field's source the same way the screen does and skips `source === 'environment'` whatever the field's type. `pluginRegion`/`boardRegion` rendered plugin contributions and `pluginInventory` read `pluginHost.health()` without first awaiting `syncOperatorDisables()`, unlike their siblings `filterView` and `emitEvent`. A freshly started process defaults its in-memory disabled set to "enabled", so a switched-off plugin's UI could render and "Running on this server" could misreport until some unrelated request happened to sync. All three paths now reconcile before they read; `pluginRegion` and `boardRegion` become async and their four call sites await. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014kP43A5shJWmAKqkBovhaF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects audited in MEI-52, all on the plugin and theme admin surface, so they ship as one PR. Conventions: docs/nextjs-conventions.md.
1. Re-auth stopped at the edge of themes and plugins (finding E)
The panel banner promises "It has been a while since you confirmed your password. Anything destructive will ask again" (
apps/community/app/admin/layout.tsx). Nine destructive user/group/forum actions honour it withrequireFreshAdmin(). Nothing in themes or plugins did — including theme reset and import, which throw away every stored override for a theme in one press while the screen itself warns "Take a copy of the export above first".This follows the reasoning #115 established for API tokens: the fresh gate is for the press that cannot be taken back, not for every write.
What I classified as destructive, and why
resetThemeActionrequireFreshAdmin()importThemeActionrequireFreshAdmin()setPluginEnabledAction, disablingrequireFreshAdmin()/plugins/<key>/…starts answering 404). The highest-blast-radius button on the screen.setPluginEnabledAction, enablingrequireAdmin()saveThemeAction/previewThemeActionrequireAdmin()setThemeEnabledActionrequireAdmin()setDefaultThemeActionrequireAdmin()savePluginSettingsActionrequireAdmin()The gate in
setPluginEnabledActionreads the direction out of theFormDatabefore choosing whichrequire*()to call. Reading form fields is side-effect-free, so nothing happens before authorization.The one e2e press of a now-gated control (
admin-tabs-no-js.spec.ts:721, "Reset to the theme") signs in at the top of the file and resets well inside the 15-minuteREAUTH_MINUTESwindow, so it is unaffected.2. Env-owned boolean plugin settings were silently overwritten (finding F)
savePluginSettingsActionskipped settings the environment owns with:— which exempted exactly the kind of field that can never be submitted. An env-owned boolean renders as a disabled checkbox captioned "…in the environment — this box is inert" (
plugin-forms.tsx). A disabled checkbox posts nothing, soraw === null, so the boolean branch below stored'0'— an override written under a box that was displaying on, from the environment, at the moment of the save.That is precisely the deferred surprise the guard's own comment forbade: the stored
'0'is invisible while the variable is set, and the day the variable comes out of the environment the plugin silently flips to a value nobody chose.The row data already knows each field's provenance —
source === 'environment'is what greys the control — so the action now resolves the same detail withresolvePluginSettingDetails(definition, overrides, readPluginEnv)and skips env-owned fields whatever their type. The existingraw === nullskip is kept underneath it, so nothing else about which absent fields get written changes.3. Region and health reads did not sync the operator disable (finding F)
filterViewandemitEventinplugin-view.tsxawait syncOperatorDisables()before touching the host.pluginRegion/boardRegiondid not, andpluginInventoryinplugin-admin.tsreadpluginHost.health()without it either.PluginHostinitialises every plugin'soperatorDisabledtofalse, so in a freshly started process the in-memory set says "everything is enabled" until something reads the settings table. Until some unrelated request happened to sync first, a plugin the operator had switched off could still render its contributions into a page, and the admin screen could report "Running on this server" for it.All three paths now reconcile before they read.
pluginRegionandboardRegionbecomeasyncas a result, and their four call sitesawaitthem:app/(board)/page.tsx—index.footerapp/(board)/member/[id]/page.tsx—profile.panelapp/(board)/thread/[slug]/page.tsx—postbit.badges,postbit.footersrc/components/shell/page-shell.tsx—header.noticeAll five remain Server Components, and no client component gained a domain-package import.
Tests
New, and each one proved to fail before it passed — the fix reverted, the test watched go red, the fix restored:
theme-admin-actions.test.ts— asks for a fresh password before throwing every override away, destroys nothing when the proof is stalerequireAdmin()→ 3 redtheme-admin-actions.test.ts— lets the reversible writes through on the panel session alonesetDefaultThemeAction→ 2 red (pins the classification in both directions)plugin-admin-actions.test.ts— asks for a fresh password before taking a plugin off the board, switches nothing off when the proof is stale, puts a plugin back, and saves settings, on the panel session alonerequireAdmin()→ 2 redplugin-admin-actions.test.ts— leaves an env-owned boolean unwritten rather than storing the inert box as "0", plus companions for the string case and for a declared-but-unset variabletype !== 'boolean'exemption → 1 redplugin-view.test.ts(new file) — region reads against a realPluginHost, where the sync is the only thing that ever applies the switchawait syncOperatorDisables()→ 4 redplugin-admin.test.ts— reconciles the host before reading health, so a fresh process does not say "running"pluginInventory→ 1 redThe env-owned tests drive the real
readPluginEnvthroughvi.stubEnv, so they exercise the resolution path production uses rather than a stand-in.Documentation
No inline comments — the explanations went into
docs/operating.mdin the same commit: the reset/import gate under What you can change without a deploy, the disable gate and the fresh-process sync under Administering one, and the greyed-field rule beside the existing note on where plugin credentials live.Validation
pnpm verify— exit 0 (5731 passed, 8 skipped; the single remaining lint warning is pre-existing inadmin.test.ts)DATA_SOURCE=fixture pnpm build— exit 0, required because this diff touchesapp/pnpm lint,pnpm typecheck,pnpm typecheck:appand all 53apps/community/src/servertest files (701 tests) re-run against the committed treepnpm test:e2enot run — fixed ports shared with sibling worktrees. No e2e spec was changed.Refs MEI-52.
🤖 Generated with Claude Code
Generated by Claude Code