admin: parsed JSONC settings editor (takes over #7666, closes #7603)#7709
Open
JohnMcLear wants to merge 1 commit into
Open
admin: parsed JSONC settings editor (takes over #7666, closes #7603)#7709JohnMcLear wants to merge 1 commit into
JohnMcLear wants to merge 1 commit into
Conversation
ⓘ 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}
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
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
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 |
Member
So this is just for visualisation not for editing? I'm a bit puzzle how I would edit this. |
Member
Author
|
Looks like maybe your browser cache wasn't refreshed? Do you see an input for site name? |
Member
Author
|
ahhh it because you are using docker, good point.. On it :) Thanks for spotting, adding tests and rules too |
Member
Author
|
@SamTV12345 not a cache issue — diagnosed with your screenshot. Your Pushed 845440e on top of the merge from develop (424a067):
Why Docker CI didn't catch this: Could you retry on your Docker setup once CI is green? |
…#7666) Takes over ether#7666 / closes ether#7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework ether#7716 and admin i18n fixes ether#7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in ether#7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <[email protected]> Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
JohnMcLear
force-pushed
the
takeover/7666-admin-settings-editor
branch
from
845440e to
b2007d1
Compare
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