From 32e25d8bb7ed7d11ffd23a81aa167b3f1dc77df9 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 25 Jul 2026 22:56:05 -0700 Subject: [PATCH] fix(network): harden custom address picker --- .../components/network/AddressPicker.test.tsx | 187 ++++++++++++++++++ .../src/components/network/AddressPicker.tsx | 81 ++++---- .../network/CustomAddressDialog.tsx | 6 +- 3 files changed, 233 insertions(+), 41 deletions(-) create mode 100644 src/renderer/src/components/network/AddressPicker.test.tsx diff --git a/src/renderer/src/components/network/AddressPicker.test.tsx b/src/renderer/src/components/network/AddressPicker.test.tsx new file mode 100644 index 00000000000..ad9abf2a464 --- /dev/null +++ b/src/renderer/src/components/network/AddressPicker.test.tsx @@ -0,0 +1,187 @@ +// @vitest-environment happy-dom + +import '@testing-library/jest-dom/vitest' + +import type { ComponentProps } from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { cleanup, render, screen, waitFor, within } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { AddressPicker } from './AddressPicker' + +afterEach(() => { + cleanup() + vi.restoreAllMocks() +}) + +const LAN_OPTION = { value: '192.168.1.24', label: '192.168.1.24 (en0)' } +const CUSTOM_VALUE = 'orca.example.com' + +function pickerProps( + overrides: Partial> = {} +): ComponentProps { + return { + options: [LAN_OPTION], + value: LAN_OPTION.value, + onValueChange: vi.fn(), + formatCustomLabel: (value) => `${value} (custom)`, + addCustomLabel: 'Add custom address…', + customDialogCopy: { + title: 'Custom network address', + description: 'Use an address another device can reach.', + inputLabel: 'Address', + placeholder: 'orca.example.com', + hint: 'Enter a hostname or IP address.', + cancel: 'Cancel', + confirm: 'Use address' + }, + validateCustom: (input) => + input.trim() === '' ? { ok: false } : { ok: true, value: input.trim() }, + customInputId: 'custom-address', + placeholder: 'No addresses found', + triggerAriaLabel: 'Network address', + ...overrides + } +} + +describe('AddressPicker', () => { + it('preserves the active item when a custom address becomes discovered', async () => { + const user = userEvent.setup() + const onValueChange = vi.fn() + const { rerender } = render( + + ) + + await user.click(screen.getByRole('combobox')) + const customItem = screen.getByRole('option', { name: `${CUSTOM_VALUE} (custom)` }) + + rerender( + + ) + + expect(screen.getByRole('option', { name: `${CUSTOM_VALUE} (tailscale0)` })).toBe(customItem) + }) + + it('renders the first label for duplicate address values', async () => { + const user = userEvent.setup() + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + render( + + ) + + await user.click(screen.getByRole('combobox')) + const listbox = screen.getByRole('listbox') + const renderedOptions = within(listbox).getAllByRole('option') + + expect(renderedOptions.map((option) => option.textContent)).toEqual([ + LAN_OPTION.label, + '100.64.1.20 (tailscale0)' + ]) + expect(within(listbox).getByRole('option', { name: LAN_OPTION.label })).toBeVisible() + expect(within(listbox).queryByRole('option', { name: /bridge0/ })).toBeNull() + expect(consoleError.mock.calls.flat().join(' ')).not.toContain('same key') + }) + + it('uses stable address values for typeahead', async () => { + const user = userEvent.setup() + render( + + ) + + await user.click(screen.getByRole('combobox')) + await user.keyboard('o') + + expect(screen.getByRole('option', { name: `Tailnet host (${CUSTOM_VALUE})` })).toHaveFocus() + }) + + it('keeps the custom action outside the address listbox', async () => { + const user = userEvent.setup() + render() + + const addButton = screen.getByRole('button', { name: 'Add custom address…' }) + await user.click(screen.getByRole('combobox')) + + const listbox = screen.getByRole('listbox') + expect(within(listbox).queryByRole('button', { name: 'Add custom address…' })).toBeNull() + expect(within(listbox).getAllByRole('option')).toHaveLength(1) + expect(addButton).toHaveAttribute('aria-haspopup', 'dialog') + }) + + it('restores focus to the custom action after every dialog close path', async () => { + const user = userEvent.setup() + const onValueChange = vi.fn() + render() + + const addButton = screen.getByRole('button', { name: 'Add custom address…' }) + await user.click(addButton) + expect(screen.getByLabelText('Address')).toHaveFocus() + expect(onValueChange).not.toHaveBeenCalled() + + await user.keyboard('{Escape}') + await waitFor(() => expect(addButton).toHaveFocus()) + + await user.keyboard('{Enter}') + await user.click(screen.getByRole('button', { name: 'Cancel' })) + await waitFor(() => expect(addButton).toHaveFocus()) + + await user.keyboard('{Enter}') + await user.type(screen.getByLabelText('Address'), CUSTOM_VALUE) + await user.click(screen.getByRole('button', { name: 'Use address' })) + + expect(onValueChange).toHaveBeenCalledWith(CUSTOM_VALUE) + await waitFor(() => expect(addButton).toHaveFocus()) + }) + + it('does not reseed an open custom dialog when options refresh', async () => { + const user = userEvent.setup() + const onValueChange = vi.fn() + const { rerender } = render( + + ) + + await user.click(screen.getByRole('button', { name: 'Add custom address…' })) + const input = screen.getByLabelText('Address') + await user.clear(input) + await user.type(input, 'edited.example.com') + + rerender( + + ) + + expect(input).toHaveValue('edited.example.com') + }) + + it('disables both address controls', () => { + render() + + expect(screen.getByRole('combobox')).toBeDisabled() + expect(screen.getByRole('button', { name: 'Add custom address…' })).toBeDisabled() + }) +}) diff --git a/src/renderer/src/components/network/AddressPicker.tsx b/src/renderer/src/components/network/AddressPicker.tsx index 8d6603861b9..001423172d1 100644 --- a/src/renderer/src/components/network/AddressPicker.tsx +++ b/src/renderer/src/components/network/AddressPicker.tsx @@ -1,13 +1,7 @@ -import React, { useState } from 'react' +import React, { useRef, useState } from 'react' import { Plus } from 'lucide-react' -import { - Select, - SelectContent, - SelectItem, - SelectSeparator, - SelectTrigger, - SelectValue -} from '../ui/select' +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '../ui/select' +import { Button } from '../ui/button' import { CustomAddressDialog, type CustomAddressDialogCopy, @@ -19,9 +13,16 @@ export type AddressOption = { label: string } -// Why: a sentinel Select value for the footer action. It is never committed as -// a real address; selecting it opens the custom-address dialog instead. -const ADD_CUSTOM_VALUE = '__add_custom_address__' +function getUniqueAddressOptions(options: readonly AddressOption[]): AddressOption[] { + const seenValues = new Set() + return options.filter((option) => { + if (seenValues.has(option.value)) { + return false + } + seenValues.add(option.value) + return true + }) +} export type AddressPickerProps = { options: readonly AddressOption[] @@ -42,10 +43,6 @@ export type AddressPickerProps = { id?: string } -// Note (crash cluster C6, React #185 in settings): the throwing dispatch is in -// @radix-ui/react-select's SelectItem unmount cleanup, not in our code — this -// file holds no unmount-phase setState, so there is nothing here to guard. -// Item churn here is derived from props only; re-audit if that stops holding. export function AddressPicker({ options, value, @@ -62,47 +59,51 @@ export function AddressPicker({ id }: AddressPickerProps): React.JSX.Element { const [dialogOpen, setDialogOpen] = useState(false) + const dialogInitialValueRef = useRef(undefined) - const isCustomSelection = - value !== undefined && value !== '' && !options.some((option) => option.value === value) - - const handleValueChange = (next: string): void => { - if (next === ADD_CUSTOM_VALUE) { - setDialogOpen(true) - return + const uniqueOptions = getUniqueAddressOptions(options) + const customOption = + value !== undefined && value !== '' && !uniqueOptions.some((option) => option.value === value) + ? { value, label: formatCustomLabel(value) } + : undefined + const selectOptions = customOption ? [...uniqueOptions, customOption] : uniqueOptions + const handleDialogOpenChange = (open: boolean): void => { + if (open) { + dialogInitialValueRef.current = customOption?.value } - onValueChange(next) + setDialogOpen(open) } return ( <> - - {options.map((option) => ( - + {selectOptions.map((option) => ( + {option.label} ))} - {isCustomSelection ? ( - {formatCustomLabel(value)} - ) : null} - {options.length > 0 || isCustomSelection ? : null} - - - {addCustomLabel} - + + {addCustomLabel} + + } + initialValue={dialogInitialValueRef.current} validate={validateCustom} copy={customDialogCopy} inputId={customInputId} diff --git a/src/renderer/src/components/network/CustomAddressDialog.tsx b/src/renderer/src/components/network/CustomAddressDialog.tsx index de6a90ddc92..fda9b9a2949 100644 --- a/src/renderer/src/components/network/CustomAddressDialog.tsx +++ b/src/renderer/src/components/network/CustomAddressDialog.tsx @@ -5,7 +5,8 @@ import { DialogDescription, DialogFooter, DialogHeader, - DialogTitle + DialogTitle, + DialogTrigger } from '@/components/ui/dialog' import { Button } from '@/components/ui/button' import { Input } from '@/components/ui/input' @@ -26,6 +27,7 @@ export type CustomAddressDialogCopy = { type CustomAddressDialogProps = { open: boolean onOpenChange: (open: boolean) => void + trigger?: React.ReactElement // Why: prefill from the current selection when it is already a custom value // so reopening to edit shows what is in use rather than a blank field. initialValue?: string @@ -42,6 +44,7 @@ type CustomAddressDialogProps = { export function CustomAddressDialog({ open, onOpenChange, + trigger, initialValue, validate, copy, @@ -73,6 +76,7 @@ export function CustomAddressDialog({ return ( + {trigger ? {trigger} : null} {copy.title}