Add JSON editor indentation setting and auto-format JSON on paste#191
Add JSON editor indentation setting and auto-format JSON on paste#191Pranav-0440 wants to merge 2 commits intoeclipse-editdor:masterfrom
Conversation
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
|
Hi @TejInaco 👋 Thanks! |
There was a problem hiding this comment.
Pull request overview
Implements a user-configurable JSON editor indentation setting and attempts to auto-format JSON content based on that setting, addressing Issue #185.
Changes:
- Adds a new “JSON Editor” section to Settings with a 2/4 space indentation selector.
- Updates the JSON editor to apply indentation settings to Monaco.
- Adds JSON auto-formatting logic (currently triggered on any valid change).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/components/Editor/JsonEditor.tsx | Reads indentation setting and updates Monaco options; adds JSON auto-formatting logic and refactors linked-TD dropdown generation. |
| src/components/App/Settings.tsx | Extends SettingsData with jsonIndentation and adds UI for configuring indentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/App/Settings.tsx
Outdated
|
|
||
| const handleJsonIndentationChange = useCallback( | ||
| (e: React.ChangeEvent<HTMLSelectElement>) => { | ||
| const value = Number(e.target.value) as 2 | 4; |
There was a problem hiding this comment.
handleJsonIndentationChange trusts/casts the DOM value to 2 | 4. At runtime this can still become another number (e.g. if the DOM is modified), which would violate the intended constraint. Add a small runtime guard to coerce/validate the value (fallback to 2) before updating state.
| const value = Number(e.target.value) as 2 | 4; | |
| const parsed = Number(e.target.value); | |
| const value: 2 | 4 = parsed === 2 || parsed === 4 ? (parsed as 2 | 4) : 2; |
src/components/App/Settings.tsx
Outdated
| <label className="block text-sm text-gray-300 mb-1"> | ||
| Space indentation | ||
| </label> | ||
|
|
||
| <select | ||
| value={data.jsonIndentation} | ||
| onChange={handleJsonIndentationChange} | ||
| className="w-full rounded-md border-2 border-gray-600 bg-gray-600 p-2 text-white focus:border-blue-500 focus:outline-none sm:text-sm" | ||
| > | ||
| <option value={2}>2 spaces</option> | ||
| <option value={4}>4 spaces</option> | ||
| </select> |
There was a problem hiding this comment.
The label "Space indentation" is not programmatically associated with the <select> (no htmlFor on the label and no id on the select). This reduces accessibility for screen readers. Add an id to the select and htmlFor on the label (or wrap the select inside the label) so they are correctly linked.
src/components/Editor/JsonEditor.tsx
Outdated
| // Read indentation from settings (default = 2) | ||
| const jsonIndentation = context.settings?.jsonIndentation ?? 2; |
There was a problem hiding this comment.
context.settings?.jsonIndentation is read here, but IEdiTDorContext (and the GlobalState provider value) does not expose a settings object. This will not compile and the editor will never pick up the setting. Consider sourcing the indentation from the same place Settings are persisted (currently localStorage via getLocalStorage/setLocalStorage) or extend the context/provider to include the persisted settings and update it on save.
| // Read indentation from settings (default = 2) | |
| const jsonIndentation = context.settings?.jsonIndentation ?? 2; | |
| // Indentation used for JSON formatting (default = 2) | |
| const jsonIndentation = 2; |
| const updateMonacoSchemas = (schemaMap: SchemaMapMessage) => { | ||
| proxy.splice(0, proxy.length); | ||
|
|
||
| schemaMap.forEach((schema, schemaUri) => { | ||
| proxy.push({ schemaUri, schema }); | ||
| }); |
There was a problem hiding this comment.
updateMonacoSchemas calls proxy.splice(...) but proxy state is initialized as undefined and the effect registers worker onmessage handlers before setProxy(proxy) runs in editorDidMount. If a worker message arrives before proxy is set, this will throw. Add a guard (e.g. return early when !proxy) or initialize proxy to an empty array/proxy so the handler is always safe.
src/components/Editor/JsonEditor.tsx
Outdated
| const parsed = JSON.parse(editorText); | ||
|
|
||
| // Auto-format JSON using selected indentation | ||
| const formatted = JSON.stringify(parsed, null, jsonIndentation); | ||
|
|
||
| // Prevent infinite update loop | ||
| if (formatted !== editorText) { | ||
| editorInstance.current?.setValue(formatted); | ||
| } | ||
|
|
||
| context.updateOfflineTD(formatted); |
There was a problem hiding this comment.
The onChange handler reformats the entire document (JSON.parse + JSON.stringify + setValue) whenever the JSON is valid. This is expensive for large documents and will reset cursor/selection while the user edits (it also goes beyond the stated requirement of formatting on paste). Prefer formatting only on paste (e.g. via Monaco paste event / editor.onDidPaste) and rely on tabSize/insertSpaces to keep indentation for normal edits, or use Monaco’s built-in editor.action.formatDocument in response to paste.
| const parsed = JSON.parse(editorText); | |
| // Auto-format JSON using selected indentation | |
| const formatted = JSON.stringify(parsed, null, jsonIndentation); | |
| // Prevent infinite update loop | |
| if (formatted !== editorText) { | |
| editorInstance.current?.setValue(formatted); | |
| } | |
| context.updateOfflineTD(formatted); | |
| // Parse to validate JSON, but avoid reformatting on every change | |
| JSON.parse(editorText); | |
| // Store the current editor text as-is to avoid cursor resets and | |
| // expensive full-document reformatting on each change. | |
| context.updateOfflineTD(editorText); |
src/components/Editor/JsonEditor.tsx
Outdated
| const tabs = Object.keys(context.linkedTd).map((key, index) => ( | ||
| <option value={key} key={index}> | ||
| {key} | ||
| </option> | ||
| )); |
There was a problem hiding this comment.
The linked-TD dropdown options previously filtered out entries that were neither a file handle (kind === "file") nor a non-empty object. This change now lists all keys, including empty placeholder entries (which can be created when a link is added but the linked TD couldn't be fetched). Selecting such an entry can result in changeBetweenTd doing nothing. Reintroduce the previous filter condition when building tabs (and consider using key={key} instead of the index).
| const tabs = Object.keys(context.linkedTd).map((key, index) => ( | |
| <option value={key} key={index}> | |
| {key} | |
| </option> | |
| )); | |
| const tabs = Object.entries(context.linkedTd) | |
| .filter(([_, value]) => { | |
| if (!value || typeof value !== "object") { | |
| return false; | |
| } | |
| const typedValue = value as { kind?: string } & Record<string, unknown>; | |
| if (typedValue.kind === "file") { | |
| return true; | |
| } | |
| // Only include non-empty objects | |
| return Object.keys(typedValue).length > 0; | |
| }) | |
| .map(([key]) => ( | |
| <option value={key} key={key}> | |
| {key} | |
| </option> | |
| )); |
| export interface SettingsData { | ||
| northboundUrl: string; | ||
| southboundUrl: string; | ||
| pathToValue: string; | ||
| jsonIndentation: 2 | 4; | ||
| } |
There was a problem hiding this comment.
Adding jsonIndentation to SettingsData makes it a required field for all existing callers (e.g. the Settings dialog constructs SettingsData objects and persists settings). Those call sites currently only provide northboundUrl/southboundUrl/pathToValue, so this will break compilation and the new setting won’t be persisted unless the save logic/localStorage key set is extended. Update the settings persistence and all SettingsData initializations to include a default jsonIndentation (2) and store/load it alongside the other settings.
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
|
Cannot accept your solution. Reason: 1) Solution out of the issue scope 2) The commits modified the entire file instead of only the code necessary for the proposed solution. Note to @egekorkan |
fixes - #185