From 2eca27e7dcdd7c4f24ebd2ca88ca8b4eb1342839 Mon Sep 17 00:00:00 2001 From: Pavlos Koutoglou Date: Thu, 16 Jul 2026 18:58:04 +0300 Subject: [PATCH] fix: stop reusing blank-row ids in key-value editors [INS-2598] (#10260) * fix: ensure unique blank IDs for environment key-value pairs to prevent stale state * test: add validation for blank row behavior after deleting committed rows in environment table editor (cherry picked from commit 4ad32eeb7c37a28b37d800396882fe0d1f88053f) --- .../smoke/key-value-editor-blank-row.test.ts | 48 +++++++++++++++++++ .../key-value-editor.tsx | 13 +++-- .../key-value-editor/key-value-editor.tsx | 13 +++-- 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/packages/insomnia-smoke-test/tests/smoke/key-value-editor-blank-row.test.ts b/packages/insomnia-smoke-test/tests/smoke/key-value-editor-blank-row.test.ts index 76f741a7e5..159199f53a 100644 --- a/packages/insomnia-smoke-test/tests/smoke/key-value-editor-blank-row.test.ts +++ b/packages/insomnia-smoke-test/tests/smoke/key-value-editor-blank-row.test.ts @@ -85,4 +85,52 @@ test.describe('Key-value editor blank row', () => { await expect.soft(kvTable.getByRole('option')).toHaveCount(optionsBefore + 1); await expect.soft(kvTable).toContainText('blankRowKey'); }); + + test('environment table editor: deleting a committed row does not leave stale text on the blank row', 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(); + await page.getByRole('dialog').waitFor({ state: 'hidden' }); + + await page.getByRole('button', { name: 'Manage Environments' }).click(); + await page.getByRole('button', { name: 'Manage collection environments' }).click(); + await page.getByLabel('Environments', { exact: true }).getByText('ExampleA').click(); + await page.getByRole('button', { name: 'Table Edit' }).click(); + + const kvTable = page.getByRole('listbox', { name: 'Environment Key Value Pair' }); + await expect.soft(kvTable).toContainText('exampleString'); + const optionsBefore = await kvTable.getByRole('option').count(); + + // Commit two new pairs, one after another, via the trailing blank row. Each commit is + // awaited (a fresh blank row appears) before typing into the next one, so the second + // keystroke does not land in the still-committing first row. + await kvTable.getByRole('option').last().getByTestId('OneLineEditor').first().locator('.CodeMirror').click(); + await page.keyboard.type('firstNewKey'); + await expect.soft(kvTable.getByRole('option')).toHaveCount(optionsBefore + 1); + + await kvTable.getByRole('option').last().getByTestId('OneLineEditor').first().locator('.CodeMirror').click(); + await page.keyboard.type('secondNewKey'); + await expect.soft(kvTable.getByRole('option')).toHaveCount(optionsBefore + 2); + + // Delete the first of the two new rows (a PromptButton: first click arms it, second confirms). + const firstRow = kvTable.getByRole('option').filter({ hasText: 'firstNewKey' }); + const deleteButton = firstRow.getByRole('button', { name: 'Delete Row' }); + await deleteButton.click(); + await deleteButton.click(); + + // The deleted row is gone, the other committed row remains, and the trailing blank row + // is still present and empty (it must not resurrect the deleted row's text). + await expect.soft(kvTable).not.toContainText('firstNewKey'); + await expect.soft(kvTable).toContainText('secondNewKey'); + + // Typing into the blank row must produce only the new text, confirming its id/editor + // instance was not silently reused from the deleted row. + await kvTable.getByRole('option').last().getByTestId('OneLineEditor').first().locator('.CodeMirror').click(); + await page.keyboard.type('thirdNewKey'); + await expect.soft(kvTable).not.toContainText('firstNewKey'); + await expect.soft(kvTable).toContainText('thirdNewKey'); + }); }); 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 bb50a26e22..824578670f 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 @@ -81,12 +81,17 @@ export const EnvironmentKVEditor = ({ // held in state) so it only changes when the data actually changes. This keeps it in // sync with the async data updates - if it flipped eagerly the row the user just typed // into would briefly belong to no list and flicker. + // A fresh id is minted only once the previous one is committed (found in persistedPairs) - + // never reused, even after that row is later deleted. Reclaiming a freed numeric slot would + // let a deleted row's id land back on the blank row, and since the row's key (and its + // OneLineEditor's key) is derived from that id, React would reuse the deleted row's DOM/editor + // instance instead of remounting it - leaving stale, uncommitted text on screen. + const blankIdRef = useRef(generateId('envPair-blank')); const blankId = useMemo(() => { - let n = 0; - while (persistedPairs.some(p => p.id === `envPair-blank-${n}`)) { - n++; + if (persistedPairs.some(p => p.id === blankIdRef.current)) { + blankIdRef.current = generateId('envPair-blank'); } - return `envPair-blank-${n}`; + return blankIdRef.current; }, [persistedPairs]); const blankPair: EnvironmentKvPairData = useMemo( () => ({ id: blankId, name: '', value: '', type: EnvironmentKvPairDataType.STRING, enabled: true }), diff --git a/packages/insomnia/src/ui/components/key-value-editor/key-value-editor.tsx b/packages/insomnia/src/ui/components/key-value-editor/key-value-editor.tsx index 684439577b..fa26449992 100644 --- a/packages/insomnia/src/ui/components/key-value-editor/key-value-editor.tsx +++ b/packages/insomnia/src/ui/components/key-value-editor/key-value-editor.tsx @@ -109,12 +109,17 @@ export const KeyValueEditor: FC = ({ // held in state) so it only changes when the data actually changes. This keeps it in // sync with the async data updates - if it flipped eagerly the row the user just typed // into would briefly belong to no list and flicker. + // A fresh id is minted only once the previous one is committed (found in persistedItems) - + // never reused, even after that row is later deleted. Reclaiming a freed numeric slot would + // let a deleted row's id land back on the blank row, and since the row's key (and its + // OneLineEditor's key) is derived from that id, React would reuse the deleted row's DOM/editor + // instance instead of remounting it - leaving stale, uncommitted text on screen. + const blankIdRef = useRef(generateId('pair-blank')); const blankId = useMemo(() => { - let n = 0; - while (persistedItems.some(item => item.id === `pair-blank-${n}`)) { - n++; + if (persistedItems.some(item => item.id === blankIdRef.current)) { + blankIdRef.current = generateId('pair-blank'); } - return `pair-blank-${n}`; + return blankIdRef.current; }, [persistedItems]); const blankPair = useMemo( () => ({ id: blankId, name: '', value: '', description: '', disabled: false }),