From b371926344abd773303ffb2b0b18328d2d05e25a Mon Sep 17 00:00:00 2001 From: Bingbing Date: Tue, 22 Sep 2026 17:47:07 +0800 Subject: [PATCH] fix(environment-editor): treat empty JSON content as invalid JSON (#10540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Background The environment editor's JSON view had two reproducible problems. **1. Deleting all JSON content left the Table view unchanged** 1. Open an environment and configure several variables in **Table View**. 2. Switch to **JSON View**. 3. Delete all content in the JSON editor. 4. Switch back to **Table View**. Expected: the variable entries are gone, since the JSON content is empty. Actual: every previously configured entry is still listed — the deletion is ignored. **2. Switching routes and coming back dropped the error state** 1. Clear the JSON view: the editor shows `Unexpected end of JSON input` and switching to Table view is blocked by the error dialog. 2. Navigate to another route, then return to this page. Expected: the state stays consistent. Actual: the error is gone and the empty document can be switched to Table view, which again shows the last saved variables. **Root cause** - `EnvironmentEditor.getValue()` short-circuited on an empty document (`if (!editorRef.current || !value) return null`), so `onChange` never fired: no error was recorded, nothing was submitted, and `isValid()` stayed `true`. On the JSON → Table switch, `useToggleEnvironmentType` then derived `kvPairData` from the unchanged `environment.data` — the last saved variables. - Validity was push-only: it was refreshed exclusively from `CodeEditor`'s debounced `onChange`. On remount the `CodeEditor` restores unsaved content from its `historyKey` cache programmatically (cached-state `setValue` inside `initEditor`, before the `changes` listener is attached), so no `onChange` ran and the error silently vanished. ## Changes **`packages/insomnia/src/ui/components/editors/environment-editor.tsx`** - `getValue()` no longer special-cases empty content: an empty (or whitespace-only) document reaches `orderedJSON.parse`, which throws, so it takes exactly the same error path as any other malformed JSON (inline notice + `isValid() === false`). - Validation is one `validate()` helper (parse + `checkNestedKeys`) shared by `onChange` and by a new `useEffect` that re-applies it on mount and whenever the incoming value or `historyKey` changes, so the notice and the validity follow the document instead of the last edit event. - `isValid()` is derived from the document on demand, so it cannot lag the `CodeEditor`'s 100 ms `onChange` debounce. - `getValue()` is documented as throwing on invalid content; every caller gates on `isValid()`, and `request-group-pane` additionally wraps it in `try/catch`. **`packages/insomnia-smoke-test/tests/smoke/environment-editor-interactions.test.ts`** - New regression test: clear the JSON content, assert the parse error surfaces and that switching to Table view is blocked by the existing error dialog; then close and reopen the editor and assert the error state survives the remount and the switch stays blocked. ## Other - **Product decision**: an empty document is treated as invalid JSON and follows the existing malformed-JSON handling (inline notice + the existing dialog that blocks the switch). Interpreting it as `{}` was rejected because it would wipe every variable on a transient select-all + delete. - **Behavior changes**: empty content reports an error and blocks the JSON ↔ Table switch; an invalid document keeps its error across remounts and route changes until it is fixed; `getValue()` may throw for invalid content. - **Unchanged**: a top-level JSON value whose object is falsy (`false`, `0`, `""`) keeps its pre-existing silent no-op behavior — neither widened nor narrowed. - **Verification**: - The new test fails on the pre-fix source (missing error notice, switch not blocked, KV list still visible) and passes with the fix. - `npm run test:smoke:dev -- environment-editor-interactions`: 6 passed. - `npm run type-check` clean, `npm run lint` 0 errors, `npx prettier --check` clean. - `npm test -w packages/insomnia`: only the pre-existing, unrelated `git-service-clone-folder-naming` failure, which also fails on clean `develop`. [INS-3884](https://konghq.atlassian.net/browse/INS-3884) --- .../environment-editor-interactions.test.ts | 54 ++++++++++++++++ .../components/editors/environment-editor.tsx | 64 +++++++++++-------- 2 files changed, 91 insertions(+), 27 deletions(-) diff --git a/packages/insomnia-smoke-test/tests/smoke/environment-editor-interactions.test.ts b/packages/insomnia-smoke-test/tests/smoke/environment-editor-interactions.test.ts index a6635c9851..a22fdf0581 100644 --- a/packages/insomnia-smoke-test/tests/smoke/environment-editor-interactions.test.ts +++ b/packages/insomnia-smoke-test/tests/smoke/environment-editor-interactions.test.ts @@ -330,4 +330,58 @@ test.describe('Environment Editor', () => { await privateRow.waitFor({ state: 'visible' }); await expect.soft(privateRow.locator('.fa-lock')).toBeVisible(); }); + + test('clearing the JSON editor reports a JSON error instead of keeping stale table values', async ({ page, app }) => { + const text = await loadFixture('environments.yaml'); + await app.evaluate(async ({ clipboard }, text) => clipboard.writeText(text), text); + await page.getByLabel('Import').click(); + await page.locator('[data-test-id="import-from-clipboard"]').click(); + await page.getByRole('button', { name: 'Scan' }).click(); + await page.getByRole('dialog').getByRole('button', { name: 'Import' }).click(); + + // wait for import dialog to close before proceeding + await page.getByRole('dialog').waitFor({ state: 'hidden' }); + + // open the JSON environment in the environment editor + await page.getByLabel('Select an API Collection Environment').click(); + await page.getByRole('button', { name: 'Manage API collection environments' }).click(); + const environmentDialog = page.getByRole('dialog', { name: 'Manage Environments' }); + await environmentDialog.getByLabel('Environments', { exact: true }).getByText('ExampleA').click(); + + // delete all JSON content + const jsonEditor = environmentDialog.getByTestId('CodeEditor').getByRole('textbox'); + await jsonEditor.focus(); + await page.keyboard.press('ControlOrMeta+a'); + await page.keyboard.press('Delete'); + + /* Empty content must be reported like any other parse error, not silently dropped. */ + await expect.soft(environmentDialog.getByText('Unexpected end of JSON input')).toBeVisible(); + + /* The existing JSON error dialog blocks the switch, so the table can never show values that no + longer match the editor content. */ + await page.getByRole('button', { name: 'Table Edit' }).click(); + await expect + .soft(page.getByText('Please modify and fix the JSON string error before switch to Table view')) + .toBeVisible(); + await expect.soft(page.getByRole('listbox', { name: 'Environment Key Value Pair' })).toBeHidden(); + await page.getByRole('button', { name: 'Ok', exact: true }).click(); + + /* Reopening remounts the editor and restores the unsaved empty content from its cache: the error + must survive, otherwise the document is empty but still switchable. */ + await page.getByRole('button', { name: 'Close', exact: true }).click(); + await page.getByRole('heading', { name: 'Manage Environments' }).waitFor({ state: 'hidden' }); + // the picker popover can still be open; this click dismisses it, then reopen the editor + await page.locator('body').click(); + await page.getByRole('button', { name: 'Select an API Collection Environment' }).click(); + await page.getByRole('button', { name: 'Manage API collection environments' }).click(); + await page.getByRole('dialog', { name: 'Manage Environments' }).waitFor({ state: 'visible' }); + await environmentDialog.getByLabel('Environments', { exact: true }).getByText('ExampleA').click(); + + await expect.soft(environmentDialog.getByText('Unexpected end of JSON input')).toBeVisible(); + await page.getByRole('button', { name: 'Table Edit' }).click(); + await expect + .soft(page.getByText('Please modify and fix the JSON string error before switch to Table view')) + .toBeVisible(); + await expect.soft(page.getByRole('listbox', { name: 'Environment Key Value Pair' })).toBeHidden(); + }); }); diff --git a/packages/insomnia/src/ui/components/editors/environment-editor.tsx b/packages/insomnia/src/ui/components/editors/environment-editor.tsx index 5d84ab690a..815698f531 100644 --- a/packages/insomnia/src/ui/components/editors/environment-editor.tsx +++ b/packages/insomnia/src/ui/components/editors/environment-editor.tsx @@ -1,6 +1,6 @@ import { isWindows } from 'insomnia-data/common'; import orderedJSON from 'json-order'; -import React, { forwardRef, useCallback, useImperativeHandle, useRef, useState } from 'react'; +import React, { forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState } from 'react'; import { checkNestedKeys } from '~/common/utils/environment-utils'; import { CodeEditor, type CodeEditorHandle } from '~/ui/components/.client/codemirror/code-editor'; @@ -22,20 +22,22 @@ interface Props { export interface EnvironmentEditorHandle { isValid: () => boolean; + /** Throws on invalid content (empty included) - guard with isValid(). */ getValue: () => EnvironmentInfo | null; } export const EnvironmentEditor = forwardRef( ({ environmentInfo, onBlur, onChange, historyKey }, ref) => { const editorRef = useRef(null); - const editorErrorRef = useRef(''); const [error, setError] = useState(''); const getValue = useCallback(() => { - // @ts-expect-error -- current can be null - let value = editorRef.current.getValue(); - if (!editorRef.current || !value) { + const editor = editorRef.current; + if (!editor) { return null; } + /* Empty content is invalid JSON, not "no value": let orderedJSON.parse throw so it follows + the same error path as any other malformed input. */ + let value = editor.getValue(); // On Windows, backslashes are used as directory separators. // The file tag inserted by Nunjucks in JSON uses double backslashes in its path parameter, but in the logic below, orderedJSON.parse unescapes those double backslashes into a single backslash. This causes the file tag to fail when the corresponding environment variable is referenced in a request. @@ -52,20 +54,33 @@ export const EnvironmentEditor = forwardRef( }; return environmentInfo; }, []); + + /** Parses the current document: the value to commit, or the error to show. */ + const validate = useCallback(() => { + try { + const value = getValue(); + if (!value?.object) { + return { value: null, error: '' }; + } + // Check root and nested properties + const err = checkNestedKeys(value.object); + return err ? { value: null, error: err } : { value, error: '' }; + } catch (err) { + return { value: null, error: err.message }; + } + }, [getValue]); + useImperativeHandle( ref, () => ({ - isValid: () => !editorErrorRef.current, + /* Derived from the document, not from the last edit event: onChange is debounced and the + cached content is restored on mount without firing it. */ + isValid: () => !validate().error, getValue, }), - [getValue], + [getValue, validate], ); - const updateEditorError = (message: string) => { - editorErrorRef.current = message; - setError(message); - }; - let defaultValue = orderedJSON.stringify( environmentInfo.object, environmentInfo.propertyOrder || null, @@ -77,6 +92,12 @@ export const EnvironmentEditor = forwardRef( defaultValue = unescapeFileTag(defaultValue); } + /* The editor can hold content that never passed through onChange (the cached document is + restored on mount), so re-validate to keep the notice and isValid() in sync with it. */ + useEffect(() => { + setError(validate().error); + }, [validate, defaultValue, historyKey]); + return (
( autoPrettify enableNunjucks onChange={() => { - updateEditorError(''); - try { - const value = getValue(); - // Check for invalid key names - if (value?.object) { - // Check root and nested properties - const err = checkNestedKeys(value.object); - if (err) { - updateEditorError(err); - } else { - onChange?.(value); - } - } - } catch (err) { - updateEditorError(err.message); + const { value, error: validationError } = validate(); + setError(validationError); + if (value) { + onChange?.(value); } }} defaultValue={defaultValue}