From c8693d99a842cccb343221a15fee385eb5528d36 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 10 Apr 2026 15:42:34 -0500 Subject: [PATCH 1/2] fix teensy tiny race in ssh key add --- app/components/form/fields/SshKeysField.tsx | 12 ++++++- app/util/array.spec.tsx | 20 ++++++++++- app/util/array.ts | 8 +++++ test/e2e/instance-create.e2e.ts | 40 +++++++++++++++++++++ 4 files changed, 78 insertions(+), 2 deletions(-) diff --git a/app/components/form/fields/SshKeysField.tsx b/app/components/form/fields/SshKeysField.tsx index e03d4d2c7..390ee4c39 100644 --- a/app/components/form/fields/SshKeysField.tsx +++ b/app/components/form/fields/SshKeysField.tsx @@ -20,6 +20,7 @@ import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { FieldLabel } from '~/ui/lib/FieldLabel' import { Message } from '~/ui/lib/Message' import { TextInputHint } from '~/ui/lib/TextInput' +import { isSubset } from '~/util/array' import { CheckboxField } from './CheckboxField' import { ErrorMessage } from './ErrorMessage' @@ -73,7 +74,16 @@ export function SshKeysField({ }, }) - const allAreSelected = allKeys.length === selectedKeys.length + // do this with a subset instead of just counting the items because there's a + // weird window right after adding but before the invalidate has gone through + // where you can have the new one selected but it's not in the list of all + // keys, which can cause the counts to match even though the sets don't + const allAreSelected = + allKeys.length > 0 && + isSubset( + allKeys.map((k) => k.id), + new Set(selectedKeys) + ) return (
diff --git a/app/util/array.spec.tsx b/app/util/array.spec.tsx index 34dfcb6c0..3e0dd99ed 100644 --- a/app/util/array.spec.tsx +++ b/app/util/array.spec.tsx @@ -8,7 +8,14 @@ import type { JSX, ReactElement } from 'react' import { expect, test } from 'vitest' -import { groupBy, intersperse, isSetEqual, setDiff, setIntersection } from './array' +import { + groupBy, + intersperse, + isSetEqual, + isSubset, + setDiff, + setIntersection, +} from './array' test('groupBy', () => { expect( @@ -73,6 +80,17 @@ test('isSetEqual', () => { expect(isSetEqual(new Set([{}]), new Set([{}]))).toBe(false) }) +test('isSubset', () => { + expect(isSubset(new Set(), new Set())).toBe(true) + expect(isSubset(new Set(), new Set(['a']))).toBe(true) + expect(isSubset(new Set(['a']), new Set(['a', 'b']))).toBe(true) + expect(isSubset(new Set(['a', 'b']), new Set(['a', 'b']))).toBe(true) + + expect(isSubset(new Set(['a']), new Set())).toBe(false) + expect(isSubset(new Set(['a', 'b']), new Set(['a']))).toBe(false) + expect(isSubset(new Set(['c']), new Set(['a', 'b']))).toBe(false) +}) + test('setDiff', () => { expect(setDiff(new Set(), new Set())).toEqual(new Set()) expect(setDiff(new Set(['a']), new Set())).toEqual(new Set(['a'])) diff --git a/app/util/array.ts b/app/util/array.ts index 004ef00e2..c07c48849 100644 --- a/app/util/array.ts +++ b/app/util/array.ts @@ -49,6 +49,14 @@ export function isSetEqual(a: Set, b: Set): boolean { return true } +/** Is `a ⊆ b`? */ +export function isSubset(a: Iterable, b: ReadonlySet): boolean { + for (const item of a) { + if (!b.has(item)) return false + } + return true +} + /** Set `a - b` */ export function setDiff(a: ReadonlySet, b: ReadonlySet): Set { return new Set([...a].filter((x) => !b.has(x))) diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index e7ed4d785..2054bb339 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -351,6 +351,46 @@ test('add ssh key from instance create form', async ({ page }) => { await expectRowVisible(page.getByRole('table'), { name: newKey, description: 'hi' }) }) +// Regression: creating a key with one existing key unchecked caused Select all +// to appear fully checked during the stale window (length coincidence bug). +test('add ssh key with one unchecked does not falsely show select all', async ({ + page, +}) => { + await page.goto('/projects/mock-project/instances-new') + + const macMini = page.getByRole('checkbox', { name: 'mac-mini' }) + await expect(macMini).toBeChecked() + // uncheck one key so selectedKeys length will match stale allKeys length after create + await macMini.click() + await expect(macMini).not.toBeChecked() + + const selectAll = page.getByRole('checkbox', { name: 'Select all' }) + await expect(selectAll).not.toBeChecked() + + const newKey = 'regression-key' + const dialog = page.getByRole('dialog') + await page.getByRole('button', { name: 'Add SSH Key' }).click() + await dialog.getByRole('textbox', { name: 'Name' }).fill(newKey) + await dialog.getByRole('textbox', { name: 'Description' }).fill('test') + await dialog.getByRole('textbox', { name: 'Public key' }).fill('ssh-ed25519 AAAA') + await dialog.getByRole('button', { name: 'Add SSH Key' }).click() + + // Wait for the modal to close (create succeeded), then immediately check + // Select all before the list refetch lands. During the stale window, + // allKeys has 2 items and selectedKeys has 2 items (one old + one new), + // so a length-based allAreSelected would incorrectly be true. + await expect(dialog).toBeHidden() + // Use isChecked() (no auto-retry) to snapshot the state during the stale window + expect(await selectAll.isChecked()).toBe(false) + + // Wait for refetch to land — does Select all self-correct? + const newCheckbox = page.getByRole('checkbox', { name: newKey }) + await expect(newCheckbox).toBeVisible() + await expect(newCheckbox).toBeChecked() + await expect(macMini).not.toBeChecked() + await expect(selectAll).not.toBeChecked() +}) + test('shows object not found error on no default pool', async ({ page }) => { await page.goto('/projects/mock-project/instances-new') await page.getByRole('textbox', { name: 'Name', exact: true }).fill('no-default-pool') From 966aac13e6baf6aa1882ae815b9d5c367ee808da Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 10 Apr 2026 15:56:07 -0500 Subject: [PATCH 2/2] remove test used to prove the issue --- test/e2e/instance-create.e2e.ts | 40 --------------------------------- 1 file changed, 40 deletions(-) diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 2054bb339..e7ed4d785 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -351,46 +351,6 @@ test('add ssh key from instance create form', async ({ page }) => { await expectRowVisible(page.getByRole('table'), { name: newKey, description: 'hi' }) }) -// Regression: creating a key with one existing key unchecked caused Select all -// to appear fully checked during the stale window (length coincidence bug). -test('add ssh key with one unchecked does not falsely show select all', async ({ - page, -}) => { - await page.goto('/projects/mock-project/instances-new') - - const macMini = page.getByRole('checkbox', { name: 'mac-mini' }) - await expect(macMini).toBeChecked() - // uncheck one key so selectedKeys length will match stale allKeys length after create - await macMini.click() - await expect(macMini).not.toBeChecked() - - const selectAll = page.getByRole('checkbox', { name: 'Select all' }) - await expect(selectAll).not.toBeChecked() - - const newKey = 'regression-key' - const dialog = page.getByRole('dialog') - await page.getByRole('button', { name: 'Add SSH Key' }).click() - await dialog.getByRole('textbox', { name: 'Name' }).fill(newKey) - await dialog.getByRole('textbox', { name: 'Description' }).fill('test') - await dialog.getByRole('textbox', { name: 'Public key' }).fill('ssh-ed25519 AAAA') - await dialog.getByRole('button', { name: 'Add SSH Key' }).click() - - // Wait for the modal to close (create succeeded), then immediately check - // Select all before the list refetch lands. During the stale window, - // allKeys has 2 items and selectedKeys has 2 items (one old + one new), - // so a length-based allAreSelected would incorrectly be true. - await expect(dialog).toBeHidden() - // Use isChecked() (no auto-retry) to snapshot the state during the stale window - expect(await selectAll.isChecked()).toBe(false) - - // Wait for refetch to land — does Select all self-correct? - const newCheckbox = page.getByRole('checkbox', { name: newKey }) - await expect(newCheckbox).toBeVisible() - await expect(newCheckbox).toBeChecked() - await expect(macMini).not.toBeChecked() - await expect(selectAll).not.toBeChecked() -}) - test('shows object not found error on no default pool', async ({ page }) => { await page.goto('/projects/mock-project/instances-new') await page.getByRole('textbox', { name: 'Name', exact: true }).fill('no-default-pool')