From 406f53c55d23d7131dc8d6d165c0027cb2f608d5 Mon Sep 17 00:00:00 2001 From: Kyle Date: Sun, 26 Jul 2026 22:02:32 -0400 Subject: [PATCH 1/4] fix(env-editor): cancel pending debounced onChange on blur to stop stale-write races one-line-editor.tsx fired two independent onChange triggers per edit: an immediate blur-flush and a separately-scheduled 100ms-debounced call from the CodeMirror 'changes' event. Blur never cancelled the pending debounced timer, so if another edit (e.g. adding a new environment KV row) landed within that 100ms window, the stale debounced call fired afterward with a closure over the pre-edit state and silently reverted/re-persisted it - the root cause of the flaky "manage environment" smoke test (row added via Add Row would vanish, environment values would fall back to the base environment). misc.debounce() now exposes cancel(), and the blur handler calls it before flushing. Also retains the persistedPairs/commitPairs local-state fix in key-value-editor.tsx (avoids building the next array from a stale `data` prop snapshot) - both fixes were needed to fully close the race. Verified with 8 consecutive full runs of environment-editor-interactions.test.ts (previously ~50% flake rate, now 0/8 failures) and a clean full npm test run (149 files, 2428 passed). --- .../environment-editor-interactions.test.ts | 4 +- packages/insomnia/src/common/misc.ts | 4 ++ .../.client/codemirror/one-line-editor.tsx | 14 +++- .../key-value-editor.tsx | 72 ++++++++++++------- 4 files changed, 65 insertions(+), 29 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 a3fecfe4464c..1af5fd992d23 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 @@ -143,7 +143,9 @@ test.describe('Environment Editor', () => { // (inserting the blank row) - if that click blurred the value cell too, the blur-flush // and the insert would be two separate writes fired by one gesture with no way to wait // between them, and the older one landing after the newer one silently drops the value. - await page.keyboard.press('Tab'); + // Click away (rather than pressing Tab) to blur - Tab's target inside a CodeMirror + // instance isn't guaranteed to actually move focus out of the editor. + await page.locator('body').click(); await waitForSync(); // add second row: exampleObject (JSON type) diff --git a/packages/insomnia/src/common/misc.ts b/packages/insomnia/src/common/misc.ts index 1f7c42ce9465..0144b499815d 100644 --- a/packages/insomnia/src/common/misc.ts +++ b/packages/insomnia/src/common/misc.ts @@ -93,6 +93,10 @@ export const debounce = ) => ReturnType>( clearTimeout(timeout); timeout = setTimeout(() => func(...args), waitFor); }; + // Lets a caller drop an already-scheduled call - e.g. a blur handler that flushes the + // latest value immediately shouldn't leave the pending debounced call to fire again + // afterwards with a now-stale closure over whatever triggered it. + debounced.cancel = () => clearTimeout(timeout); return debounced; }; diff --git a/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx b/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx index 60a4f193523d..d69ef5f22c3a 100644 --- a/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx +++ b/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx @@ -451,6 +451,7 @@ export const OneLineEditor = forwardRef }; }, [editorVersion, type]); + const debouncedChangeRef = useRef<{ cancel: () => void } | null>(null); useEffect(() => { const fn = misc.debounce((doc: CodeMirror.Editor) => { if (onChange) { @@ -458,12 +459,23 @@ export const OneLineEditor = forwardRef } updateTooltipValue(doc.getValue() || ''); }, DEBOUNCE_MILLIS); + debouncedChangeRef.current = fn; codeMirror.current?.on('changes', fn); - return () => codeMirror.current?.off('changes', fn); + return () => { + fn.cancel(); + debouncedChangeRef.current = null; + codeMirror.current?.off('changes', fn); + }; }, [editorVersion, onChange, type, updateTooltipValue]); useEffect(() => { const flushOnBlur = (doc: CodeMirror.Editor) => { + // Drop the pending debounced call from the 'changes' listener above - it would + // otherwise still fire ~DEBOUNCE_MILLIS after this, calling onChange again with a + // closure over whatever was current when the last keystroke happened. If other + // actions (e.g. adding a new row elsewhere in the same form) landed in that window, + // that stale call can silently clobber state newer than what it captured. + debouncedChangeRef.current?.cancel(); if (onChange) { onChange(doc.getValue() || ''); } diff --git a/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx b/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx index c164933126f5..65908c5560a7 100644 --- a/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx +++ b/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx @@ -74,13 +74,28 @@ export const EnvironmentKVEditor = ({ // Deduped by id - react-aria-components' ListBox keys rows by id, and two rows sharing // an id corrupts its internal collection (can hang the tab). Duplicate ids shouldn't occur, // but have been observed with corrupted/legacy persisted data, so guard against it here. - const persistedPairs: EnvironmentKvPairData[] = useMemo(() => { + const dedupe = (pairs: EnvironmentKvPairData[]) => { const byId = new Map(); - data.forEach(pair => byId.set(pair.id, pair)); + pairs.forEach(pair => byId.set(pair.id, pair)); return [...byId.values()]; - // Ensure same array data will not generate different kvPairs to avoid flash issue + }; + // Held as local state (updated optimistically the instant we call onChange below) rather + // than derived fresh from the `data` prop on every render. Deriving straight from `data` + // meant two edits fired back-to-back (e.g. typing a row's value, then immediately adding + // another row) could race: the second edit would build its outgoing array from a `data` + // snapshot that hadn't yet round-tripped the parent's persistence of the first, silently + // dropping it. Local state is always at least as current as the last edit we made + // ourselves; the effect below still re-syncs from `data` for changes that didn't originate + // here (e.g. switching to a different environment). + const [persistedPairs, setPersistedPairs] = useState(() => dedupe(data)); + useEffect(() => { + setPersistedPairs(dedupe(data)); // eslint-disable-next-line react-hooks/exhaustive-deps }, [JSON.stringify(data)]); + const commitPairs = (next: EnvironmentKvPairData[]) => { + setPersistedPairs(next); + onChange(next); + }; const blankNameEditorRef = useRef(null); // The id for the trailing blank row is derived from the persisted pairs (rather than // held in state) so it only changes when the data actually changes. This keeps it in @@ -181,7 +196,7 @@ export const EnvironmentKVEditor = ({ return; } const targetIndex = persistedPairs.findIndex(pair => pair.id === e.target.key.toString()); - onChange(repositionInArray(moveItems, targetIndex === -1 ? persistedPairs.length : targetIndex)); + commitPairs(repositionInArray(moveItems, targetIndex === -1 ? persistedPairs.length : targetIndex)); }, renderDragPreview(items) { const pair = kvPairs.find(pair => pair.id === items[0]['text/plain']) || createNewPair(); @@ -227,30 +242,33 @@ export const EnvironmentKVEditor = ({ if (isNameOrValueChange && !newPair.name && !newPair.value) { return; } - onChange([...persistedPairs, newPair]); + commitPairs([...persistedPairs, newPair]); return; } - // Mutate the persisted working copy in place so sequential calls (e.g. the secret - // type switch, which changes value then type) accumulate onto the same item. + // Build a new array with the changed item replaced by a new object (rather than mutating + // persistedPairs in place) - persistedPairs is React state now, and setState bails out of + // re-rendering when given back the same array reference it was passed. const changedItemIdx = persistedPairs.findIndex(p => p.id === id); - if (changedItemIdx !== -1) { - const changedItem = persistedPairs[changedItemIdx]; - // A blur-flush (from clicking into a disabled/read-only row's name or value editor and - // then clicking away, e.g. into another row) re-fires onChange with the same, unedited - // value. Treat that as a no-op rather than force-enabling a row the user never touched. - const isNameOrValueChange = changedPropertyName === 'name' || changedPropertyName === 'value'; - if (isNameOrValueChange && changedItem[changedPropertyName] === newValue) { - return; - } - // enable item since user modifies the item unless manual disable it - changedItem['enabled'] = true; - changedItem[changedPropertyName] = newValue; - // update value to empty object json string when switch to json type and current value is empty string - if (newValue === EnvironmentKvPairDataType.JSON && changedItem.value.trim() === '') { - changedItem.value = JSON.stringify({}); - } + if (changedItemIdx === -1) { + return; + } + const changedItem = persistedPairs[changedItemIdx]; + // A blur-flush (from clicking into a disabled/read-only row's name or value editor and + // then clicking away, e.g. into another row) re-fires onChange with the same, unedited + // value. Treat that as a no-op rather than force-enabling a row the user never touched. + const isNameOrValueChange = changedPropertyName === 'name' || changedPropertyName === 'value'; + if (isNameOrValueChange && changedItem[changedPropertyName] === newValue) { + return; } - onChange(persistedPairs); + // enable item since user modifies the item unless manual disable it + const updatedItem = { ...changedItem, enabled: true, [changedPropertyName]: newValue }; + // update value to empty object json string when switch to json type and current value is empty string + if (newValue === EnvironmentKvPairDataType.JSON && updatedItem.value.trim() === '') { + updatedItem.value = JSON.stringify({}); + } + const next = [...persistedPairs]; + next[changedItemIdx] = updatedItem; + commitPairs(next); }; const handleItemTypeChange = async (id: string, newType: EnvironmentKvPairDataType) => { @@ -300,14 +318,14 @@ export const EnvironmentKVEditor = ({ const insertIdx = id ? persistedPairs.findIndex(d => d.id === id) : persistedPairs.length - 1; const next = [...persistedPairs]; next.splice(insertIdx === -1 ? next.length : insertIdx + 1, 0, newPair); - onChange(next); + commitPairs(next); }; const handleDeleteItem = (id: string) => { // Drop the deleted row's cached undo history (keyed on the pair id) so its // ephemeral entries don't linger in the shared cache. purgeCachedEditorStates(key => key.includes(id)); - onChange(persistedPairs.filter(d => d.id !== id)); + commitPairs(persistedPairs.filter(d => d.id !== id)); }; const checkValidJSONString = (input: string) => { @@ -548,7 +566,7 @@ export const EnvironmentKVEditor = ({ { - onChange([]); + commitPairs([]); }} ariaLabel="Delete All" className="flex h-full items-center justify-center gap-2 px-4 py-1 text-xs text-(--color-font) ring-1 ring-transparent transition-all hover:bg-(--hl-xs) focus:ring-(--hl-md) focus:ring-inset aria-pressed:bg-(--hl-sm)" From e37e8417ecf6da959ded3c6627a64d69f965499c Mon Sep 17 00:00:00 2001 From: Kyle Date: Tue, 4 Aug 2026 17:41:42 -0400 Subject: [PATCH 2/4] fix(codemirror): stop debounced edits from being dropped on parent re-render OneLineEditor's debounced change/blur listeners depended on onChange and updateTooltipValue by reference, so any parent re-render (a fresh inline onChange closure, or handleRender's loader data getting a new reference after a fetcher revalidation) tore down the listener and cancelled a pending debounced call before it could fire, silently dropping the edit. Track both in refs so the listeners are stable across re-renders. Also close a related hint-dropdown leak: completeAfter() only checked focus before its async lookups, not after, so a slow-resolving autocomplete could pop a hint over an editor the user had already left, and since it was orphaned it never got a blur event to close it. --- .../codemirror/extensions/autocomplete.ts | 11 ++++++++ .../.client/codemirror/one-line-editor.tsx | 25 ++++++++++++------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/insomnia/src/ui/components/.client/codemirror/extensions/autocomplete.ts b/packages/insomnia/src/ui/components/.client/codemirror/extensions/autocomplete.ts index 8ef4842221b9..870337ea2c5b 100644 --- a/packages/insomnia/src/ui/components/.client/codemirror/extensions/autocomplete.ts +++ b/packages/insomnia/src/ui/components/.client/codemirror/extensions/autocomplete.ts @@ -91,6 +91,17 @@ CodeMirror.defineOption( const variables = options.getVariables ? await options.getVariables() : null; const snippets = options.getSnippets ? await options.getSnippets() : null; const tags = options.getTags ? await options.getTags() : null; + + // The awaits above can take a while (e.g. a slow render-context round trip). If the + // user has since moved focus elsewhere, showing the hint now would create a popup + // with no editor to blur it closed - closeOnUnfocus only starts listening for blur + // once the widget exists, so a blur that already happened is missed entirely and the + // popup is orphaned, sitting open (and blocking clicks) until some unrelated focus + // change happens to close it. + if (!cm.hasFocus()) { + return; + } + // Actually show the hint cm.showHint({ // Insomnia-specific options diff --git a/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx b/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx index d69ef5f22c3a..c6d74fe31025 100644 --- a/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx +++ b/packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx @@ -451,13 +451,22 @@ export const OneLineEditor = forwardRef }; }, [editorVersion, type]); + // Keep the latest onChange/updateTooltipValue in refs so the listener effects below + // don't need to depend on their identity. Parents commonly pass a fresh inline closure + // on every render, and updateTooltipValue itself is recreated whenever handleRender's + // upstream loader data gets a new reference (e.g. on every route revalidation) - if the + // 'changes' listener effect re-ran on either alone, it would cancel any pending debounced + // call scheduled by keystrokes that haven't fired yet, silently dropping them. + const onChangeRef = useRef(onChange); + onChangeRef.current = onChange; + const updateTooltipValueRef = useRef(updateTooltipValue); + updateTooltipValueRef.current = updateTooltipValue; + const debouncedChangeRef = useRef<{ cancel: () => void } | null>(null); useEffect(() => { const fn = misc.debounce((doc: CodeMirror.Editor) => { - if (onChange) { - onChange(doc.getValue() || ''); - } - updateTooltipValue(doc.getValue() || ''); + onChangeRef.current?.(doc.getValue() || ''); + updateTooltipValueRef.current(doc.getValue() || ''); }, DEBOUNCE_MILLIS); debouncedChangeRef.current = fn; codeMirror.current?.on('changes', fn); @@ -466,7 +475,7 @@ export const OneLineEditor = forwardRef debouncedChangeRef.current = null; codeMirror.current?.off('changes', fn); }; - }, [editorVersion, onChange, type, updateTooltipValue]); + }, [editorVersion, type]); useEffect(() => { const flushOnBlur = (doc: CodeMirror.Editor) => { @@ -476,13 +485,11 @@ export const OneLineEditor = forwardRef // actions (e.g. adding a new row elsewhere in the same form) landed in that window, // that stale call can silently clobber state newer than what it captured. debouncedChangeRef.current?.cancel(); - if (onChange) { - onChange(doc.getValue() || ''); - } + onChangeRef.current?.(doc.getValue() || ''); }; codeMirror.current?.on('blur', flushOnBlur); return () => codeMirror.current?.off('blur', flushOnBlur); - }, [editorVersion, onChange]); + }, [editorVersion]); useEffect(() => { const unsubscribe = window.main.on( From 419501083684d885c6065913c3c20d089476ea47 Mon Sep 17 00:00:00 2001 From: Kyle Date: Sun, 16 Aug 2026 17:21:12 -0400 Subject: [PATCH 3/4] fix(env-editor): stop stale data revalidation from clobbering newer local edits EnvironmentKVEditor's persistedPairs resync effect overwrote local optimistic state on every `data` prop change with no ordering check. Because environment updates round-trip through an async fetcher/loader that doesn't resolve in submission order, a slower earlier write's revalidation (e.g. Delete All) could land after a faster later edit and silently clobber it - reintroducing deleted rows or dropping a value that had already committed. Thread the owning environment's `modified` timestamp through as a `dataRevision` prop and only accept a resync when it's at least as new as the last one applied, so an out-of-order/stale delivery is ignored instead of overwriting fresher local state. --- .../key-value-editor.tsx | 18 +++++++++++++++++- .../workspace-environments-edit-modal.tsx | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx b/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx index 65908c5560a7..ba1aa5125516 100644 --- a/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx +++ b/packages/insomnia/src/ui/components/editors/environment-key-value-editor/key-value-editor.tsx @@ -32,6 +32,10 @@ import { PasswordInput } from './password-input'; interface EditorProps { data: EnvironmentKvPairData[]; + // Monotonic revision (e.g. the owning model's `modified` timestamp) tied to `data`. Lets + // the resync effect below tell a genuinely newer update apart from a stale one that was + // merely slow to arrive - see the effect for why that distinction matters. + dataRevision?: number; onChange: (newPair: EnvironmentKvPairData[]) => void; vaultKey?: string; isPrivate?: boolean; @@ -64,6 +68,7 @@ const ItemButton = (props: ButtonProps & { tabIndex?: number }) => { export const EnvironmentKVEditor = ({ data, + dataRevision, onChange, vaultKey = '', isPrivate = false, @@ -88,10 +93,21 @@ export const EnvironmentKVEditor = ({ // ourselves; the effect below still re-syncs from `data` for changes that didn't originate // here (e.g. switching to a different environment). const [persistedPairs, setPersistedPairs] = useState(() => dedupe(data)); + // Tracks the newest dataRevision this component has actually applied. `data` updates land + // via async fetcher/loader round-trips that don't resolve in submission order - an edit + // committed here can trigger a revalidation that overtakes an earlier, still-in-flight one + // (e.g. Delete All's own revalidation arriving after a faster edit that landed on top of + // it). Without this guard, that late/stale delivery would resync persistedPairs backwards, + // silently reintroducing rows the user already replaced or removed. + const lastAppliedRevisionRef = useRef(dataRevision); useEffect(() => { + if (dataRevision !== undefined && lastAppliedRevisionRef.current !== undefined && dataRevision < lastAppliedRevisionRef.current) { + return; + } + lastAppliedRevisionRef.current = dataRevision; setPersistedPairs(dedupe(data)); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [JSON.stringify(data)]); + }, [JSON.stringify(data), dataRevision]); const commitPairs = (next: EnvironmentKvPairData[]) => { setPersistedPairs(next); onChange(next); diff --git a/packages/insomnia/src/ui/components/modals/workspace-environments-edit-modal.tsx b/packages/insomnia/src/ui/components/modals/workspace-environments-edit-modal.tsx index a63806a62c81..86f032050285 100644 --- a/packages/insomnia/src/ui/components/modals/workspace-environments-edit-modal.tsx +++ b/packages/insomnia/src/ui/components/modals/workspace-environments-edit-modal.tsx @@ -533,6 +533,7 @@ export const WorkspaceEnvironmentsEditModal = ({ onClose }: { onClose: () => voi )} From faa52a3a98e0ccc39e282e4f062e60e16506260a Mon Sep 17 00:00:00 2001 From: Kyle Date: Sun, 16 Aug 2026 18:47:39 -0400 Subject: [PATCH 4/4] test(env-editor): wait for Close to re-enable before closing after Disable Row The Disable Row edit persists asynchronously through the same fetcher that gates the Close button (isDisabled while a change is in-flight), but this test clicked Close immediately after asserting the row's opacity instead of waiting for that persistence to round-trip first - unlike every other edit->action transition in this file, which does wait. On a slow/contended CI runner this let Close be clicked while still disabled, a no-op click that left the Manage Environments dialog open and the next waitFor hanging for the full 30s timeout. --- .../tests/smoke/environment-editor-interactions.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 1af5fd992d23..a76ac24f6635 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 @@ -241,8 +241,13 @@ test.describe('Environment Editor', () => { await exampleStringRow.getByRole('button', { name: 'Disable Row' }).click(); await expect.soft(exampleStringRow).toHaveCSS('opacity', '0.4'); + // wait for the disable-row edit's persistence to round-trip (Close is disabled while it's + // in-flight) before closing - clicking Close while it's still disabled is a no-op click + const closeButton = page.getByRole('button', { name: 'Close', exact: true }); + await expect.soft(closeButton).toBeEnabled(); + // close the editor and wait for it to fully disappear - await page.getByRole('button', { name: 'Close', exact: true }).click(); + await closeButton.click(); await page.getByRole('heading', { name: 'Manage Environments' }).waitFor({ state: 'hidden' }); // dismiss the environment picker dropdown if it appeared