feat(admin): explain env-var substitution in /settings, surface auth errors (#7819)#7826
Open
JohnMcLear wants to merge 2 commits into
Open
feat(admin): explain env-var substitution in /settings, surface auth errors (#7819)#7826JohnMcLear wants to merge 2 commits into
JohnMcLear wants to merge 2 commits into
Conversation
…errors (#7819) Three small, env-var-only UX improvements driven by issue #7819, where a Docker operator saved an ep_oauth block in the admin /settings raw view and reported it "disappeared" — but the underlying confusion was that settings.json on disk is a *template*, not the effective config. None of these changes is visible to installs that don't use ${VAR} placeholders. * Banner above the editor explaining the template/env-substitution model, only rendered when the loaded file contains a ${VAR} placeholder. Tells the operator that the file is not env-substituted in place and that the Effective tab shows the live values. * Effective tab in the mode toggle, read-only, also gated on ${VAR}. The backend was already emitting redacted runtime settings as `resolved` alongside every `load`; the SPA now exposes them so an operator can verify what Etherpad is actually using. * admin_auth_error event from the /settings socket handler. The handler previously silently returned when the connecting session wasn't admin, which made misrouted Traefik+SSO auth look like "save did nothing" with no error path in the UI. Emit a dedicated event before dropping the socket so the SPA can show a clear toast. Tests: - src/tests/backend/specs/admin/adminSettingsAuthError.ts — new spec for the auth_error/disconnect contract. - src/tests/frontend-new/admin-spec/adminsettings.spec.ts — new Playwright test asserting the banner + Effective tab only appear after a ${VAR} is added to settings.json, and that the Effective view is read-only + shows [REDACTED] for secrets. No behaviour change for installs without ${VAR} placeholders — banner, Effective tab, and auth-error contract are all the same as before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoAdd env-var substitution UX improvements and auth error handling to admin /settings
WalkthroughsDescription• Add explanatory banner and Effective tab for env-var substitution in /settings
• Emit admin_auth_error event when non-admin socket connects to /settings
• Gate Docker-aware UX features on presence of ${VAR} placeholders in settings.json
• Add comprehensive tests for auth error handling and env-var banner/Effective tab behavior
Diagramflowchart LR
A["Non-admin socket connects"] -->|Previously| B["Silent disconnect"]
A -->|Now| C["Emit admin_auth_error event"]
C --> D["SPA shows toast"]
E["Settings file with \${VAR}"] -->|Detected| F["Show env-var banner"]
E -->|Detected| G["Enable Effective tab"]
G --> H["Display redacted runtime config"]
I["Settings file without \${VAR}"] -->|No change| J["Original UI unchanged"]
File Changes1. src/node/hooks/express/adminsettings.ts
|
Code Review by Qodo
1. Effective tab lacks feature-flag
|
This was referenced May 20, 2026
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+12
to
42
| // Heuristic: `${VAR}` or `${VAR:default}` in the file means the operator is | ||
| // running with env-var substitution (overwhelmingly Docker / Kubernetes). | ||
| // We use this to gate the Docker-aware UX (the explanatory banner and the | ||
| // Effective-config tab) so non-container installs see the existing UI | ||
| // unchanged. Conservative on purpose — false negatives just keep the old | ||
| // behaviour. | ||
| const ENV_VAR_PATTERN = /\$\{[A-Za-z_][A-Za-z0-9_]*(?::[^}]*)?\}/; | ||
|
|
||
| export const SettingsPage = () => { | ||
| const { t } = useTranslation(); | ||
| const settingsSocket = useStore(state => state.settingsSocket); | ||
| const settings = useStore(state => state.settings) ?? ''; | ||
| const resolved = useStore(state => state.resolved); | ||
|
|
||
| const usesEnvVars = useMemo(() => ENV_VAR_PATTERN.test(settings), [settings]); | ||
|
|
||
| const [mode, setMode] = useState<Mode>('form'); | ||
| const [exposeExperimental] = useState(false); | ||
|
|
||
| // The Effective tab is only meaningful when there is a `resolved` | ||
| // payload AND the file uses substitution. Falling back to Raw on | ||
| // either condition keeps the toggle honest if the user opens this | ||
| // page against an older server. | ||
| const canShowEffective = usesEnvVars && resolved != null; | ||
| useEffect(() => { | ||
| if (mode === 'effective' && !canShowEffective) setMode('raw'); | ||
| }, [mode, canShowEffective]); | ||
|
|
||
| // Tab in textarea inserts two spaces instead of moving focus; rAF restores caret position after React re-renders. | ||
| const handleKeyDown = (e: React.KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key !== 'Tab') return; |
There was a problem hiding this comment.
1. effective tab lacks feature-flag 📘 Rule violation ⚙ Maintainability
The new env-var banner and Effective settings view are enabled automatically based on ${VAR}
placeholders rather than a dedicated feature flag that is disabled by default. This violates the
requirement to gate new functionality behind an explicit enable/disable mechanism to preserve
default behavior control.
Agent Prompt
## Issue description
New Admin Settings functionality (env-var banner + `Effective` tab/view) is gated by detecting `${VAR}` placeholders in `settings.json`, not by a dedicated feature flag that is disabled by default.
## Issue Context
Compliance requires new features to be behind a feature flag with an explicit enable/disable mechanism and default-off behavior.
## Fix Focus Areas
- admin/src/pages/SettingsPage.tsx[12-116]
- admin/src/components/settings/ModeToggle.tsx[3-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
) CI's admin-UI workflow seeds settings.json by copying settings.json.template verbatim, which contains ~30 \${VAR} placeholders. The new Playwright test asserted "banner not present before adding placeholder" — true on a fresh dev machine, false in CI. Drop that assertion: the negative path is covered by the SettingsPage ENV_VAR_PATTERN regex itself; what matters at the UI level is the positive path (banner + Effective tab render correctly when placeholders are present), which this test still exercises. Also: the server's admin_auth_error path calls socket.disconnect(), which the SPA's existing disconnect handler interprets as "io server disconnect" and immediately reconnects — creating a reject/reconnect loop. Track an authErrored flag and suppress the reconnect once an auth_error has been received. Reset on successful connect, so a legitimate re-auth path still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Thanks Qodo — first one (#1) is intentional, second (#2) is already addressed; quick notes on each: #1 — Effective tab / banner without dedicated feature flag These two additions are read-only informational UX that don't change Etherpad's runtime behaviour at all — no save semantics changed, no load semantics changed, no API surface added that wasn't already there. The backend already emitted The activation heuristic — render only when That said, happy to add a #2 — Auth error reconnect loop Already fixed in 5c6b164 (commit pushed before this review landed) — the SPA now tracks an |
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
1 participant
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.
Summary
Three small admin /settings UX improvements driven by #7819, where a Docker operator saved a plugin block in the raw view and reported it "disappeared." After diagnosing the issue, root cause turned out to be a mix of environmental factors (no volume mount on settings.json, env-var substitution misunderstanding) — but the Etherpad UI was confusing enough that somebody will hit this again. These changes close the gaps without touching the UX of non-container installs.
\${VAR}placeholder, so non-container installs see no change.resolvedalongside everyload; the SPA now exposes it. Same gate: only appears when\${VAR}is in the file.No behaviour change for users who don't use env-var placeholders in their settings.json.
Screenshots / behaviour
With
\${VAR}in the file (Docker default):Without any `${VAR}` placeholders:
Test plan
Backend mocha output:
```
18 passing (2s)
```
🤖 Generated with Claude Code