admin: parsed JSONC settings editor (takes over #7666, closes #7603)#7709
Open
JohnMcLear wants to merge 31 commits intoether:developfrom
Open
admin: parsed JSONC settings editor (takes over #7666, closes #7603)#7709JohnMcLear wants to merge 31 commits intoether:developfrom
JohnMcLear wants to merge 31 commits intoether:developfrom
Conversation
Original work by @AyushiGupta160604 in ether#7666 (squashed).
…ests - IconButton: forward ButtonHTMLAttributes so data-testid/aria-* work, fixing TS build break and unblocking stable Playwright selectors. - SettingsPage: move textarea presentation to .settings CSS so the :focus outline is no longer overridden, surface validation/save/socket errors via i18n toasts (Validate, Prettify, Save), and tag every button with data-testid for selector stability. - Replace positional .settingsButton.nth(1) with getByTestId in adminhelper so Save/Test/Prettify/Restart can be reordered safely. - Add regression specs: comments survive a save round-trip; Validate toasts success/failure; invalid JSON blocks Save with a failure toast. - Switch wiki links to protocol-independent URLs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Review Summary by QodoAdmin settings editor with comment preservation and JSON validation
WalkthroughsDescription• Replace plain textarea with dark monospaced editor preserving /* */ comments • Add Validate JSON button with dry-run toasts; Save rejects invalid JSON • Tab key inserts indentation; focus ring displays on textarea focus • Stabilize Playwright selectors with data-testid attributes on all buttons • Add regression tests for comment preservation and JSON validation • Extend IconButton to forward ButtonHTMLAttributes for proper prop spreading • Switch wiki links to protocol-independent URLs; add i18n toast strings Diagramflowchart LR
A["Settings Textarea"] -->|"Dark theme + monospace"| B["Editor with Comments"]
B -->|"Tab key"| C["Indentation Support"]
B -->|"Focus"| D["Focus Ring"]
E["Save Button"] -->|"Validate JSON"| F["Accept or Reject"]
G["Validate Button"] -->|"Dry-run"| H["Toast Success/Failure"]
I["IconButton"] -->|"Forward HTMLAttributes"| J["data-testid Support"]
J -->|"Stable Selectors"| K["Playwright Tests"]
File Changes1. admin/src/pages/SettingsPage.tsx
|
Code Review by Qodo
1. Form editor enabled by default
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Closes the design phase of ether#7603. Implementation will land in a follow-up commit on this same branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r, round-trip
Five new specs verifying: leading comment renders as help text; string
round-trips through save (compact-JSON spacing preserved); boolean toggle
round-trips; env placeholder renders as read-only pill with no input;
broken raw JSON shows parse-error banner with switch-to-raw CTA.
Also fix existing specs that regressed when form mode became the default
(Tasks 1-9): add waitForSelector('[data-testid="settings-form-view"]')
before switching to raw, and fix adminhelper.restartEtherpad to not
wait for the raw textarea which is hidden in form mode.
CSS bugfix in App.css/index.css: .settings-mode-toggle gained
flex-shrink:0 and .settings-page changed from height:100% to
min-height:100% to prevent flex layout from collapsing the mode toggle
to 2px when the settings tree is taller than the viewport.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
/review |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit 4d61291 |
JohnMcLear
changed the title
Comment on lines
+34
to
+42
| export const FormView = ({ onSwitchToRaw }: Props) => { | ||
| const text = useStore(s => s.settings) ?? ''; | ||
|
|
||
| const errors: ParseError[] = []; | ||
| const tree = parseTree(text, errors, { allowTrailingComma: true }); | ||
|
|
||
| const onEdit = (path: JSONPath, value: unknown) => { | ||
| useStore.getState().setSettings(editJsonc(text, path, value)); | ||
| }; |
There was a problem hiding this comment.
1. Stale text drops edits 🐞 Bug ≡ Correctness
FormView’s onEdit() computes edits against the render-time text, so a subsequent edit fired before re-render can be applied to stale JSONC and overwrite a previous change. This can silently drop settings updates when editing multiple fields quickly in form mode.
Agent Prompt
### Issue description
`FormView`’s `onEdit` closes over `text` from the render, so edits are generated against a potentially stale string. If two different controls emit edits before `FormView` re-renders, the later edit can overwrite the earlier change.
### Issue Context
Edits are applied by `jsonc-parser.modify()` + `applyEdits()` to the provided input text; this must be the latest store value to preserve all prior edits.
### Fix Focus Areas
- admin/src/components/settings/FormView.tsx[34-42]
### Suggested change
Inside `onEdit`, read the current settings text from the store:
- `const current = useStore.getState().settings ?? ''`
- `setSettings(editJsonc(current, path, value))`
(Optionally keep `text` only for rendering/parsing, not as the source for mutation.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
When settings.json is on a single line (the shipped/minified form), findTrailing was matching `//` inside `https://etherpad.org` (in defaultPadText) and rendering everything after it as the trailing comment for whichever key came before. Bail out when the value has no newline after it, since trailing comments can only exist on the same line by definition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The live settings.json is per-developer and often lacks comments (e.g. when it was minified by an earlier admin save or test run). Without a fallback, every key in form view shows up bare. settings.json.template in the repo root is the authoritative source of per-key documentation, so we read it once at admin-build time, build a path → comment map, and use it as the fallback when the live file has no comment of its own for that path. Live-file comments still take precedence so admins who add their own notes see them as written. Trailing comments still come from the live file only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the dark IDE-tree look with a light, sectioned form that matches the rest of /admin. Top-level primitive keys collect into a 'General' section; top-level objects/arrays each become their own labelled section with the first sentence of the doc comment as the heading and the rest as descriptive copy. Each leaf is a label/control row with help text below. Switch labels to template-derived first- sentence + humanized-key fallback. Remove the unused CommentLabel/ObjectGroup/ArrayGroup components — sections and grid layout replace them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two-column form derives the row label from the first sentence of a key's comment and puts the rest in a help paragraph; specs now assert both pieces render. Add a spec covering the template-fallback path (live settings.json has no comment for `port`, but `port` shows a non-empty label sourced from settings.json.template). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
/review |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit 81f7407 |
Comment on lines
+17
to
+67
| const [mode, setMode] = useState<Mode>('form'); | ||
| const [exposeExperimental] = useState(false); | ||
|
|
||
| const handleKeyDown = (e: React.KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key !== 'Tab') return; | ||
| e.preventDefault(); | ||
| const target = e.currentTarget; | ||
| const { selectionStart, selectionEnd, value } = target; | ||
| const next = value.substring(0, selectionStart) + TAB_INDENT + value.substring(selectionEnd); | ||
| useStore.getState().setSettings(next); | ||
| requestAnimationFrame(() => { | ||
| target.selectionStart = target.selectionEnd = selectionStart + TAB_INDENT.length; | ||
| }); | ||
| }; | ||
|
|
||
| const showToast = (titleKey: string, success: boolean) => { | ||
| useStore.getState().setToastState({ open: true, title: t(titleKey), success }); | ||
| }; | ||
|
|
||
| const testJSON = () => { | ||
| if (isJSONClean(settings)) showToast('admin_settings.toast.validation_ok', true); | ||
| else showToast('admin_settings.toast.validation_failed', false); | ||
| }; | ||
|
|
||
| const prettifyJSON = () => { | ||
| try { | ||
| const obj = JSON.parse(cleanComments(settings) ?? ''); | ||
| if (window.confirm(t('admin_settings.prettify_confirm'))) { | ||
| useStore.getState().setSettings(JSON.stringify(obj, null, 2)); | ||
| } | ||
| } catch { | ||
| showToast('admin_settings.toast.prettify_failed', false); | ||
| } | ||
| }; | ||
|
|
||
| const handleSave = () => { | ||
| if (!isJSONClean(settings)) return showToast('admin_settings.toast.json_invalid', false); | ||
| if (!settingsSocket?.connected) return showToast('admin_settings.toast.disconnected', false); | ||
| settingsSocket.emit('saveSettings', settings); | ||
| showToast('admin_settings.toast.saved', true); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="settings-page"> | ||
| <h1><Trans i18nKey="admin_settings.current" /></h1> | ||
|
|
||
| <ModeToggle mode={mode} onChange={setMode} /> | ||
|
|
||
| {mode === 'form' | ||
| ? <FormView onSwitchToRaw={() => setMode('raw')} /> | ||
| : ( |
There was a problem hiding this comment.
1. Form editor enabled by default 📘 Rule violation ⚙ Maintainability
The new parsed form editor is shipped as the default mode (useState<Mode>('form')) and there is no
feature flag to disable it, changing the existing /admin/settings behavior by default. This
violates the requirement that new features must be gated and disabled by default to preserve prior
behavior unless explicitly enabled.
Agent Prompt
## Issue description
The new parsed form settings editor is enabled by default and is not controlled by a feature flag, which changes existing behavior when the PR is deployed.
## Issue Context
Compliance requires new features to be behind a feature flag and disabled by default so existing behavior remains unchanged unless explicitly enabled.
## Fix Focus Areas
- admin/src/pages/SettingsPage.tsx[17-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
+99
| {generalProps.length > 0 && ( | ||
| <Section title="General"> | ||
| {generalProps.map((prop, i) => ( | ||
| <JsoncNode | ||
| key={i} | ||
| node={prop.children![1]} | ||
| property={prop} | ||
| text={text} | ||
| onEdit={onEdit} | ||
| /> | ||
| ))} | ||
| </Section> |
There was a problem hiding this comment.
2. Index keys break widget state 🐞 Bug ≡ Correctness
FormView and JsoncNode use array indices as React keys, so inserting/removing/reordering properties (possible via Raw mode) can cause React to reuse the wrong component instances for different settings paths. This can mis-associate local widget state (notably NumberInput’s draft/invalid state) with the wrong setting.
Agent Prompt
## Issue description
Using array indices as React keys can cause incorrect component reuse after list changes, leading to wrong widget state being displayed/edited.
## Issue Context
Raw mode is explicitly an escape hatch for structural edits, so reordering/inserting keys is realistic.
## Fix Focus Areas
- admin/src/components/settings/FormView.tsx[88-115]
- admin/src/components/settings/JsoncNode.tsx[88-107]
## Suggested fix
- In `FormView`, replace `key={i}` with a stable key derived from the property key (e.g., `propertyKey(prop)`) and/or node offset (`prop.offset`).
- In `JsoncNode`, replace `key={i}` with a stable key per child node:
- For object children: use the property key string (from `child.children[0].value`) or `child.offset`.
- For array children: use `child.offset` (or a combination of `offset/length`) rather than index.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
… keys
Replace index-based key={i} with stable keys: property key string (or
byte offset fallback) for object children, and byte offset for array
elements. Same fix applied to generalProps/sectionProps maps in FormView.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ale closures onEdit previously closed over the render-time text snapshot; rapid edits would clobber each other as each call re-applied its delta to the same stale baseline. Now reads useStore.getState().settings on every call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…x.css Both files defined .settings targeting the same textarea element with conflicting declarations (resize: none vs resize: vertical, different font stacks). Disambiguate App.css to textarea.settings and remove the now-superseded legacy .settings block from index.css. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ding When settings is undefined (not yet loaded from server), render an empty aria-busy placeholder div instead of treating undefined as empty string, which previously caused a spurious parse-error banner flash on load. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ocused Add a useEffect that updates the draft string when the value prop changes (e.g. after a server save round-trip canonicalises the number), but only when the input is not focused so in-progress user typing is never stomped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JsoncNode.tsx defines helpId only when help text exists and uses it solely as the p element's id — no aria-describedby ever points at it. No dangling reference; issue was already clean before this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Server now emits saveprogress:'saved' only on writeFile success and saveprogress:'error' with payload.message on failure. The client saveprogress listener in App.tsx drives the toast; handleSave no longer fires a success toast eagerly. Adds admin_settings.toast.save_failed i18n key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… toast open Radix Toast toggles data-state rather than removing the element, so the old .ToastRootSuccess selector could match a stale open toast from a previous save. Now waits for any existing open toast to close before clicking, then requires data-state="open" on the new toast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… ModeToggle
- 'General' section title → t('admin_settings.section.general')
- ModeToggle aria-label → t('admin_settings.mode.aria_label')
- ParseErrorMessage token labels intentionally kept in English (technical
parser-error tokens, not user-facing prose; documented with comment)
- Adds admin_settings.section.general and admin_settings.mode.aria_label
to src/locales/en.json
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tal form submit Without an explicit type, a <button> inside a <form> defaults to type='submit'. Adding type='button' as the default prevents any wrapping form from being submitted unintentionally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…https:// Protocol-relative //github.com inherits the page protocol; explicit https:// is safer and avoids mixed-content issues. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@qodo addressing review on commits 5bcd3e3..bd84291. 12 of 14 issues fixed (#4 was already resolved by Qodo). Pushing back on
Fixed
/review |
Switching the previous `?raw` import to a Vite `define` removes the need to widen the dev server's filesystem allowlist to the repo root. The previous setting (server.fs.allow: ['..']) let the dev server serve any file under the repo root — including settings.json and credentials.json — to anything that could reach the dev server. The build-time `define` produces an identical bundle without ever exposing the filesystem at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Replaces the textarea on
/admin/settingswith a parsed, sectioned form: each top-level object/array is rendered as its own card with a documentation heading, and primitive keys collect into a 'General' card. Labels and help text come from settings.json comments first, falling back tosettings.json.templatedocumentation when the live file has none. Round-trip is byte-identical for untouched regions: edits go throughjsonc-parser'smodify()so comments, key order, whitespace, and${ENV:default}placeholders all survive.A "Raw" mode toggle keeps the existing textarea editor behind it for power users and structural edits.
Takes over #7666 (original author AWOL). Closes #7603.
What it looks like
${VAR:default}placeholders show as a read-only env pill (raw mode is the escape hatch for editing them).What's in the diff
jsonc-parser@^3.3.1inadmin/.admin/src/components/settings/:envPill.ts,comments.ts(extract leading + trailing comments adjacent to a property node),jsoncEdit.ts(wrapsmodify()+applyEdits),templateComments.ts(build-time path → comment map fromsettings.json.template),labels.ts(first-sentence-as-label, humanized-key fallback).StringInput,NumberInput,BooleanToggle,NullChip,EnvPill.JsoncNodedispatcher,FormView(sectioned layout),ParseErrorBanner,ModeToggle.SettingsPagebecomes a Form/Raw toggle shell. Save / Validate / Restart wiring unchanged.IconButton(earlier on this branch) forwardsButtonHTMLAttributessodata-testid/aria-*compile and render..settingsButton.nth(1)togetByTestId('restart-etherpad-button').react-i18next; new keys insrc/locales/en.json.vite.config.tsallows reading from the repo root sosettings.json.templatecan be imported with?raw.Tests (
src/tests/frontend-new/admin-spec/adminsettings.spec.ts)All 10 specs pass locally (
cd src && npx playwright test admin-spec/adminsettings.spec.ts --reporter=line):titlevia the form input round-trips through save preserving structure;requireAuthenticationboolean toggle round-trips;${SSO_ISSUER:…}renders as a read-only env pill (no<input>for that path); breaking raw JSON and toggling to form shows a parse-error banner with a working "Switch to raw" CTA.Spec / plan
docs/superpowers/specs/2026-05-09-admin-settings-parsed-view-design.mddocs/superpowers/plans/2026-05-09-admin-settings-parsed-view.mdOut of scope (follow-ups)
\${VAR:default}placeholders.Are Settings visible…Playwright test wrecks the dev settings.json on each run by writing then "restoring" from in-memory; worth a separate fix to snapshot from disk instead.Semver
patch — admin UI only, no API or settings-file format changes.
🤖 Generated with Claude Code