fix(codemirror): stop debounced edits from being dropped/reverted

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)
@@ -239,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

packages/insomnia/src/common/misc.ts

@@ -93,6 +93,10 @@ export const debounce = <F extends (...args: Parameters<F>) => ReturnType<F>>(
     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;
 };

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

packages/insomnia/src/ui/components/.client/codemirror/one-line-editor.tsx

@@ -451,26 +451,45 @@ export const OneLineEditor = forwardRef<OneLineEditorHandle, OneLineEditorProps>
       };
     }, [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);
-      return () => codeMirror.current?.off('changes', fn);
-    }, [editorVersion, onChange, type, updateTooltipValue]);
+      return () => {
+        fn.cancel();
+        debouncedChangeRef.current = null;
+        codeMirror.current?.off('changes', fn);
+      };
+    }, [editorVersion, type]);
 
     useEffect(() => {
       const flushOnBlur = (doc: CodeMirror.Editor) => {
-        if (onChange) {
-          onChange(doc.getValue() || '');
-        }
+        // 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();
+        onChangeRef.current?.(doc.getValue() || '');
       };
       codeMirror.current?.on('blur', flushOnBlur);
       return () => codeMirror.current?.off('blur', flushOnBlur);
-    }, [editorVersion, onChange]);
+    }, [editorVersion]);
 
     useEffect(() => {
       const unsubscribe = window.main.on(

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,
@@ -74,13 +79,39 @@ 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<string, EnvironmentKvPairData>();
-    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<EnvironmentKvPairData[]>(() => 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);
+  };
   const blankNameEditorRef = useRef<OneLineEditorHandle | null>(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 +212,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 +258,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;
+    }
+    // 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({});
     }
-    onChange(persistedPairs);
+    const next = [...persistedPairs];
+    next[changedItemIdx] = updatedItem;
+    commitPairs(next);
   };
 
   const handleItemTypeChange = async (id: string, newType: EnvironmentKvPairDataType) => {
@@ -300,14 +334,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 +582,7 @@ export const EnvironmentKVEditor = ({
         <PromptButton
           disabled={disabled || persistedPairs.length === 0}
           onClick={() => {
-            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)"

packages/insomnia/src/ui/components/modals/workspace-environments-edit-modal.tsx

@@ -533,6 +533,7 @@ export const WorkspaceEnvironmentsEditModal = ({ onClose }: { onClose: () => voi
                     <EnvironmentKVEditor
                       key={selectedEnvironment._id}
                       data={selectedEnvironment.kvPairData || []}
+                      dataRevision={selectedEnvironment.modified}
                       onChange={handleKVPairChange}
                     />
                   )}