mirror of
https://github.com/Kong/insomnia.git
synced 2026-10-07 05:26:45 -04:00
fix(environment-editor): treat empty JSON content as invalid JSON (#10540)
## 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)
This commit is contained in:
1 parent
1b65c24c1a
commit
b371926344
2 files changed
+91
-27
No files matched your search
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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<EnvironmentEditorHandle, Props>(
|
||||
({ environmentInfo, onBlur, onChange, historyKey }, ref) => {
|
||||
const editorRef = useRef<CodeEditorHandle>(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<EnvironmentEditorHandle, Props>(
|
||||
};
|
||||
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<EnvironmentEditorHandle, Props>(
|
||||
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 (
|
||||
<div className="environment-editor">
|
||||
<CodeEditor
|
||||
@@ -86,21 +107,10 @@ export const EnvironmentEditor = forwardRef<EnvironmentEditorHandle, Props>(
|
||||
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}
|
||||
|
||||
Reference in new issue
Block a user