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..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 @@ -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 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/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 60a4f193523d..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,26 +451,45 @@ 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); - 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( 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..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, @@ -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(); - 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)); + // 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(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 = ({ { - 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)" 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 )}