From 01f289f5a4866327c723cc9113c8c55170eef4f9 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Thu, 13 Aug 2026 20:02:24 +0000 Subject: [PATCH 01/28] FEAT: Pick an operation from existing values in the labels bar Setting the operation label meant retyping the name from memory. The suggestion list that was supposed to help never appeared, because it filtered the known values against the value being replaced -- with the shipped default op_trash_panda that matches nothing. Editing the operation label now opens a combobox listing the operations already in memory, sourced from the labels request the bar already makes. Typing filters the list and offers to create a name that doesn't exist yet. - Existing values are selectable as-is; only new names are validated, so operations created before the current naming rules stay usable. - The editor now renders in the labels popover too. Clicking a label there previously set edit state but rendered no editor, which showed nothing at all when the chip was too narrow to fit inline. - Label rows are reachable by keyboard, and focus moves into the picker. - Escape, clicking away, and Tab all leave without writing a value. The selected operation applies to attacks started afterwards; it does not relabel existing ones. --- doc/gui/0_gui.md | 2 + .../src/components/Labels/LabelsBar.styles.ts | 3 + .../src/components/Labels/LabelsBar.test.tsx | 351 ++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 209 +++++++++-- 4 files changed, 534 insertions(+), 31 deletions(-) diff --git a/doc/gui/0_gui.md b/doc/gui/0_gui.md index 60d96adc2f..97963b539c 100644 --- a/doc/gui/0_gui.md +++ b/doc/gui/0_gui.md @@ -87,6 +87,8 @@ The export runs entirely in your browser and captures exactly what is shown in t The labels bar in the ribbon displays the current attack's labels (e.g., `operator`, `operation`). Labels are key-value pairs that help organize and filter attacks. You can add, edit, and remove labels inline. The `operator` and `operation` labels are required and cannot be removed. +Clicking the `operation` label opens a picker listing the operations already recorded in memory, so you can choose one without typing it from memory. Typing a name that doesn't exist yet offers to create it. The operation you pick is applied to attacks you start from then on; it does not change attacks that already exist. + #### Behavioral Guards CoPyRIT enforces several safety guards: diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index aabb3c3bae..82f821eb5e 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -113,6 +113,9 @@ export const useLabelsBarStyles = makeStyles({ overflowY: 'auto', minWidth: '120px', }, + operationPicker: { + minWidth: '180px', + }, suggestionChip: { cursor: 'pointer', ':hover': { diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index 27fc52269c..edd2618122 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1,4 +1,5 @@ import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' import LabelsBar from './LabelsBar' import { DEFAULT_GLOBAL_LABELS } from './labelDefaults' @@ -640,4 +641,354 @@ describe('LabelsBar', () => { }) expect(screen.getByTestId('popover-label-extra')).toBeInTheDocument() }) + + describe('operation picker', () => { + const OPERATIONS = ['op_2026_07_grok_45', 'op_2026_08_probe', 'validate-button-test'] + + function renderWithOperations(onChange: jest.Mock, operations: string[] = OPERATIONS) { + mockedLabelsApi.getLabels.mockResolvedValue({ + source: 'attacks', + labels: { operation: operations, operator: ['alice'] }, + }) + render( + + + + ) + } + + it('should list every operation without clearing the current value first', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect(await screen.findByRole('option', { name: 'op_2026_08_probe' })).toBeInTheDocument() + expect(screen.getByRole('option', { name: 'op_2026_07_grok_45' })).toBeInTheDocument() + const input = screen.getByTestId('edit-label-operation') as HTMLInputElement + expect(input.placeholder).toBe(DEFAULT_GLOBAL_LABELS.operation) + expect(input.value).toBe('') + }) + + it('should select an existing operation', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + fireEvent.click(await screen.findByRole('option', { name: 'op_2026_08_probe' })) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'op_2026_08_probe', + }) + }) + + it('should select an existing operation that predates the value rules', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + fireEvent.click(await screen.findByRole('option', { name: 'validate-button-test' })) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'validate-button-test', + }) + }) + + it('should filter the options by typed text', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + await screen.findByRole('option', { name: 'op_2026_08_probe' }) + fireEvent.change(screen.getByTestId('edit-label-operation'), { target: { value: 'grok' } }) + + expect(await screen.findByRole('option', { name: 'op_2026_07_grok_45' })).toBeInTheDocument() + expect(screen.queryByRole('option', { name: 'op_2026_08_probe' })).not.toBeInTheDocument() + }) + + it('should create a new operation from typed text', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + await screen.findByRole('option', { name: 'op_2026_08_probe' }) + fireEvent.change(screen.getByTestId('edit-label-operation'), { target: { value: 'op_2026_09_new' } }) + fireEvent.click(await screen.findByRole('option', { name: 'Create "op_2026_09_new"' })) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'op_2026_09_new', + }) + }) + + it('should reject a new operation that breaks the value rules', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + await screen.findByRole('option', { name: 'op_2026_08_probe' }) + fireEvent.change(screen.getByTestId('edit-label-operation'), { target: { value: 'bad name!' } }) + fireEvent.click(await screen.findByRole('option', { name: 'Create "bad name!"' })) + + expect(onChange).not.toHaveBeenCalled() + expect(screen.getByText('Only lowercase letters, numbers, underscores')).toBeInTheDocument() + }) + + it('should commit the highlighted option with the keyboard', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + const input = await screen.findByTestId('edit-label-operation') + // Narrow to a single option so the active option is unambiguous. + fireEvent.change(input, { target: { value: 'grok' } }) + await screen.findByRole('option', { name: 'op_2026_07_grok_45' }) + fireEvent.keyDown(input, { key: 'Enter' }) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'op_2026_07_grok_45', + }) + }) + + it('should dismiss the picker on Escape without committing', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + const input = await screen.findByTestId('edit-label-operation') + fireEvent.keyDown(input, { key: 'Escape' }) + + await waitFor(() => { + expect(screen.queryByTestId('edit-label-operation')).not.toBeInTheDocument() + }) + expect(onChange).not.toHaveBeenCalled() + }) + + it('should offer creation when no operations exist yet', async () => { + const onChange = jest.fn() + renderWithOperations(onChange, []) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + expect(await screen.findByRole('option', { name: /type a name to create one/i })).toBeInTheDocument() + + fireEvent.change(screen.getByTestId('edit-label-operation'), { target: { value: 'op_first' } }) + fireEvent.click(await screen.findByRole('option', { name: 'Create "op_first"' })) + + expect(onChange).toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS, operation: 'op_first' }) + }) + + it('should show a loading option while operations are still being fetched', async () => { + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockImplementation(() => new Promise(() => {})) + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect(await screen.findByRole('option', { name: /loading operations/i })).toBeInTheDocument() + }) + + it('should edit the operation from the popover list', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('labels-icon-btn')) + fireEvent.click(await screen.findByTestId('popover-label-operation')) + + fireEvent.click(await screen.findByRole('option', { name: 'op_2026_08_probe' })) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'op_2026_08_probe', + }) + }) + + it('should dismiss the picker when the user clicks away', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + await user.click(screen.getByTestId('label-operation')) + await screen.findByTestId('edit-label-operation') + await user.click(document.body) + + await waitFor(() => { + expect(screen.queryByTestId('edit-label-operation')).not.toBeInTheDocument() + }) + expect(onChange).not.toHaveBeenCalled() + }) + + it('should move focus into the picker so it can be driven by keyboard', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + // The chip must be a real, focusable control before it can be activated. + const chip = screen.getByTestId('label-operation') + expect(chip).toHaveAttribute('role', 'button') + expect(chip).toHaveAttribute('aria-label', expect.stringContaining(DEFAULT_GLOBAL_LABELS.operation)) + chip.focus() + expect(chip).toHaveFocus() + await user.keyboard('{Enter}') + + const input = await screen.findByTestId('edit-label-operation') + expect(await screen.findByRole('option', { name: 'op_2026_08_probe' })).toBeInTheDocument() + await waitFor(() => expect(input).toHaveFocus()) + }) + + it('should end the edit when the popover is dismissed', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('labels-icon-btn')) + fireEvent.click(await screen.findByTestId('popover-label-operation')) + expect(await screen.findByTestId('edit-label-operation')).toBeInTheDocument() + + // Toggle the popover shut; the edit must not reappear on the inline chip. + fireEvent.click(screen.getByTestId('labels-icon-btn')) + + await waitFor(() => { + expect(screen.queryByTestId('edit-label-operation')).not.toBeInTheDocument() + }) + expect(screen.getByTestId('label-operation')).toBeInTheDocument() + }) + + it('should not commit an operation when the user tabs away', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.keyDown(screen.getByTestId('label-operation'), { key: 'Enter' }) + await screen.findByTestId('edit-label-operation') + await user.tab() + + expect(onChange).not.toHaveBeenCalled() + }) + + it('should let focus advance to the next control when tabbing away', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockResolvedValue({ + source: 'attacks', + labels: { operation: OPERATIONS, operator: ['alice'] }, + }) + render( + + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.keyDown(screen.getByTestId('label-operation'), { key: 'Enter' }) + await screen.findByTestId('edit-label-operation') + await user.tab() + + expect(onChange).not.toHaveBeenCalled() + await waitFor(() => expect(document.activeElement).not.toBe(document.body)) + }) + + it('should match existing operations regardless of their casing', async () => { + const onChange = jest.fn() + renderWithOperations(onChange, ['op_Legacy_Run']) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + const input = await screen.findByTestId('edit-label-operation') + + // A partial match still finds the differently-cased operation. + fireEvent.change(input, { target: { value: 'legacy' } }) + expect(await screen.findByRole('option', { name: 'op_Legacy_Run' })).toBeInTheDocument() + + // Typing its full name must not offer to create a case-duplicate. + fireEvent.change(input, { target: { value: 'op_legacy_run' } }) + expect(await screen.findByRole('option', { name: 'op_Legacy_Run' })).toBeInTheDocument() + expect(screen.queryByRole('option', { name: 'Create "op_legacy_run"' })).not.toBeInTheDocument() + }) + + it('should remove a custom label with the keyboard instead of starting an edit', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockResolvedValue({ source: 'attacks', labels: {} }) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + screen.getByTestId('remove-label-team').focus() + await user.keyboard('{Enter}') + + expect(onChange).toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS }) + expect(screen.queryByTestId('edit-label-team')).not.toBeInTheDocument() + }) + + it('should open the picker from the keyboard inside the popover', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('labels-icon-btn')) + const row = await screen.findByTestId('popover-label-operation') + expect(row).toHaveAttribute('role', 'button') + row.focus() + expect(row).toHaveFocus() + await user.keyboard(' ') + + expect(await screen.findByRole('option', { name: 'op_2026_08_probe' })).toBeInTheDocument() + }) + + it('should remove a custom label with the keyboard from the popover', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockResolvedValue({ source: 'attacks', labels: {} }) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('labels-icon-btn')) + ;(await screen.findByTestId('popover-remove-label-team')).focus() + await user.keyboard('{Enter}') + + expect(onChange).toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS }) + expect(screen.queryByTestId('edit-label-team')).not.toBeInTheDocument() + }) + + it('should keep the plain input for labels other than operation', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operator')) + + expect(await screen.findByTestId('edit-label-operator')).toBeInTheDocument() + expect(screen.queryByRole('option')).not.toBeInTheDocument() + }) + }) + }) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 5e6ba4408a..121b4a9530 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -4,6 +4,8 @@ import { Button, Input, Badge, + Combobox, + Option, Tooltip, Popover, PopoverTrigger, @@ -28,6 +30,79 @@ interface LabelsBarProps { onLabelsChange: (labels: Record) => void } +interface OperationPickerProps { + currentValue: string + options: string[] + isLoading: boolean + onSelect: (operation: string) => void + onDismiss: () => void + inputRef: React.Ref + className?: string +} + +/** + * Picker for the `operation` label. Opens with every known operation listed so + * a value can be chosen without typing, and accepts a new name via freeform entry. + * The search text starts empty — seeding it with the current value would filter + * the list down to nothing. + */ +function OperationPicker({ + currentValue, + options, + isLoading, + onSelect, + onDismiss, + inputRef, + className, +}: OperationPickerProps) { + const [search, setSearch] = useState('') + + const matches = search ? options.filter(option => option.toLowerCase().includes(search)) : options + const canCreate = search.length > 0 && !options.some(option => option.toLowerCase() === search) + + // Deferred so focus lands on whatever the user moved to before this unmounts. + const dismissAfterFocusMoves = () => { setTimeout(onDismiss, 0) } + + return ( + setSearch(e.target.value.toLowerCase())} + onOptionSelect={(_, data) => { if (data.optionValue) onSelect(data.optionValue) }} + onKeyDownCapture={e => { + // Fluent commits the active option on Tab. Block that, but let the key + // through so focus still moves; onBlur then ends the edit. + if (e.key === 'Tab') e.stopPropagation() + }} + onKeyDown={e => { if (e.key === 'Escape') onDismiss() }} + onBlur={dismissAfterFocusMoves} + aria-label="Operation" + data-testid="edit-label-operation" + > + {isLoading && ( + + )} + {!isLoading && matches.length === 0 && !canCreate && ( + + )} + {matches.map(option => ( + + ))} + {canCreate && ( + + )} + + ) +} + export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const styles = useLabelsBarStyles() const [isPopoverOpen, setIsPopoverOpen] = useState(false) @@ -37,6 +112,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const [editValue, setEditValue] = useState('') const [error, setError] = useState('') const [existingLabels, setExistingLabels] = useState>({}) + const [labelsLoading, setLabelsLoading] = useState(true) const editInputRef = useRef(null) // Fetch existing label keys/values for suggestions @@ -44,6 +120,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { labelsApi.getLabels() .then(resp => setExistingLabels(resp.labels)) .catch(() => { /* ignore */ }) + .finally(() => setLabelsLoading(false)) }, []) const isDummyValue = useCallback((key: string, value: string): boolean => { @@ -95,6 +172,15 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { setTimeout(() => editInputRef.current?.focus(), 50) } + const handleStartEditKeyDown = (e: React.KeyboardEvent, key: string) => { + // Let focusable children (the remove button) handle their own keys. + if (e.target !== e.currentTarget) return + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + handleStartEdit(key) + } + } + const handleSaveEdit = () => { if (!editingLabel) return const valueError = validateValue(editValue) @@ -110,6 +196,25 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { if (e.key === 'Escape') { setEditingLabel(null); setError('') } } + const handleCancelEdit = () => { + setEditingLabel(null) + setEditValue('') + setError('') + } + + const handleSelectOperation = (operation: string) => { + // Values already in memory predate the current rules, so they are always + // selectable; only a newly typed name has to satisfy them. + if (!(existingLabels.operation || []).includes(operation)) { + const valueError = validateValue(operation) + if (valueError) { setError(valueError); return } + } + onLabelsChange({ ...labels, operation }) + setEditingLabel(null) + setEditValue('') + setError('') + } + const handleAddKeyDown = (e: React.KeyboardEvent) => { if (e.key === 'Enter') handleAddLabel() if (e.key === 'Escape') setIsPopoverOpen(false) @@ -189,42 +294,69 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { return () => observer.disconnect() }, [labelEntries]) + const renderValueEditor = (key: string, value: string) => { + if (key === 'operation') { + return ( + <> + {key}: + + {error && {error}} + + ) + } + + const filteredSuggestions = suggestedValues + .filter(v => v !== value && v.includes(editValue)) + .slice(0, 8) + return ( + <> + {key}: + { setEditValue(d.value.toLowerCase()); setError('') }} + onKeyDown={handleEditKeyDown} + onBlur={() => { setTimeout(handleSaveEdit, 150) }} + style={{ width: '120px' }} + data-testid={`edit-label-${key}`} + /> + {error && {error}} + {filteredSuggestions.length > 0 && ( +
+ {filteredSuggestions.map(v => ( + { onLabelsChange({ ...labels, [key]: v }); setEditingLabel(null); setEditValue('') }} + >{v} + ))} +
+ )} + + ) + } + const renderLabelBadge = (key: string, value: string, idx: number) => { const isDummy = isDummyValue(key, value) const isRequired = key === 'operator' || key === 'operation' - const isEditing = editingLabel === key + // The popover renders its own editor, so only one is mounted at a time. + const isEditing = editingLabel === key && !isPopoverOpen if (isEditing) { - const filteredSuggestions = suggestedValues - .filter(v => v !== value && v.includes(editValue)) - .slice(0, 8) return (
- {key}: - { setEditValue(d.value.toLowerCase()); setError('') }} - onKeyDown={handleEditKeyDown} - onBlur={() => { setTimeout(handleSaveEdit, 150) }} - style={{ width: '120px' }} - data-testid={`edit-label-${key}`} - /> - {error && {error}} - {filteredSuggestions.length > 0 && ( -
- {filteredSuggestions.map(v => ( - { onLabelsChange({ ...labels, [key]: v }); setEditingLabel(null); setEditValue('') }} - >{v} - ))} -
- )} + {renderValueEditor(key, value)}
) } @@ -239,6 +371,10 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { data-label-idx={idx} className={`${styles.labelBadge} ${isDummy ? styles.labelDummy : styles.labelNormal}`} onClick={() => handleStartEdit(key)} + onKeyDown={e => handleStartEditKeyDown(e, key)} + role="button" + tabIndex={0} + aria-label={`Edit ${key} label, currently ${value}`} data-testid={`label-${key}`} style={{ flexShrink: 0 }} > @@ -264,11 +400,22 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { {labelEntries.map(([key, value]) => { const isDummy = isDummyValue(key, value) const isRequired = key === 'operator' || key === 'operation' + if (editingLabel === key) { + return ( +
+ {renderValueEditor(key, value)} +
+ ) + } return (
handleStartEdit(key)} + onKeyDown={e => handleStartEditKeyDown(e, key)} + role="button" + tabIndex={0} + aria-label={`Edit ${key} label, currently ${value}`} data-testid={`popover-label-${key}`} style={{ flexShrink: 0 }} > @@ -352,7 +499,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) {
)} - {error && {error}} + {error && !editingLabel && {error}} ) @@ -394,7 +541,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { — so even when every chip fits, this is still the canonical entry point for editing/adding labels. */} - { setIsPopoverOpen(d.open); setError('') }}> + { setIsPopoverOpen(d.open); setError(''); if (!d.open) setEditingLabel(null) }}> Date: Fri, 14 Aug 2026 02:39:10 +0000 Subject: [PATCH 02/28] FIX: Size the operation picker dropdown to its contents The dropdown took its width from the input it hangs off, so longer operation names were cut off mid-name with no ellipsis -- op_2026_05_mai_image_2.5 rendered as op_2026_05_mai_image. Names never wrap out of it either: values may only contain letters, digits and underscores, none of which are line break opportunities. Widening the input instead would push it past the labels bar and clip the control itself, so leave the input alone and let the dropdown size to its own content. --- frontend/src/components/Labels/LabelsBar.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 121b4a9530..ea1d998eed 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -82,6 +82,9 @@ function OperationPicker({ }} onKeyDown={e => { if (e.key === 'Escape') onDismiss() }} onBlur={dismissAfterFocusMoves} + // Fluent sizes the dropdown to the input, which cuts off longer + // operation names. Let it size to its own content instead. + positioning={{ matchTargetSize: undefined }} aria-label="Operation" data-testid="edit-label-operation" > From f720def8cd5492e938c2bc31c2c6a1c30bc038d4 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 05:00:41 +0000 Subject: [PATCH 03/28] FIX: State the operation naming rules while the name is being typed Rejecting a bad name after the fact put the reason in a line of text beside the picker, and the labels bar clips anything that overflows it. On a narrow ribbon none of it survived; at full width it read "Only l". The message also stayed on screen while the name was corrected. The rules are now stated in the dropdown as the name is typed, and a name that breaks them is not offered for creation at all. The dropdown sizes to its contents, so the whole message is always readable. Fluent dims disabled options to roughly 1.9:1 against their background, which is too faint for text that has to be read rather than chosen, so the notes carry their own colour. --- .../src/components/Labels/LabelsBar.styles.ts | 8 +++++ .../src/components/Labels/LabelsBar.test.tsx | 27 ++++++++++++-- frontend/src/components/Labels/LabelsBar.tsx | 35 ++++++++++++++++--- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 82f821eb5e..64d62f2a17 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -116,6 +116,14 @@ export const useLabelsBarStyles = makeStyles({ operationPicker: { minWidth: '180px', }, + // Fluent dims disabled options to ~1.9:1 contrast, which is too faint for + // text the user has to read. These are messages, not choices. + operationNote: { + color: tokens.colorNeutralForeground2, + }, + operationNoteError: { + color: tokens.colorPaletteRedForeground1, + }, suggestionChip: { cursor: 'pointer', ':hover': { diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index edd2618122..d5381671a3 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -728,7 +728,7 @@ describe('LabelsBar', () => { }) }) - it('should reject a new operation that breaks the value rules', async () => { + it('should refuse to create a new operation that breaks the value rules', async () => { const onChange = jest.fn() renderWithOperations(onChange) await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) @@ -736,10 +736,31 @@ describe('LabelsBar', () => { fireEvent.click(screen.getByTestId('label-operation')) await screen.findByRole('option', { name: 'op_2026_08_probe' }) fireEvent.change(screen.getByTestId('edit-label-operation'), { target: { value: 'bad name!' } }) - fireEvent.click(await screen.findByRole('option', { name: 'Create "bad name!"' })) + // The rules are stated while typing instead of offering a create that fails. + expect( + await screen.findByRole('option', { name: 'Only lowercase letters, numbers, underscores' }) + ).toBeInTheDocument() + expect(screen.queryByRole('option', { name: 'Create "bad name!"' })).not.toBeInTheDocument() expect(onChange).not.toHaveBeenCalled() - expect(screen.getByText('Only lowercase letters, numbers, underscores')).toBeInTheDocument() + }) + + it('should drop the rules note once the typed name becomes valid', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + const input = await screen.findByTestId('edit-label-operation') + fireEvent.change(input, { target: { value: 'bad name!' } }) + await screen.findByRole('option', { name: 'Only lowercase letters, numbers, underscores' }) + + fireEvent.change(input, { target: { value: 'op_2026_09_ok' } }) + + expect(await screen.findByRole('option', { name: 'Create "op_2026_09_ok"' })).toBeInTheDocument() + expect( + screen.queryByRole('option', { name: 'Only lowercase letters, numbers, underscores' }) + ).not.toBeInTheDocument() }) it('should commit the highlighted option with the keyboard', async () => { diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index ea1d998eed..108f784b2e 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -20,6 +20,13 @@ import { labelsApi } from '../../services/api' import { useLabelsBarStyles } from './LabelsBar.styles' +const validateOperationValue = (value: string): string | null => { + if (!value) return 'Value is required' + if (value !== value.toLowerCase()) return 'Values must be lowercase' + if (!/^[a-z0-9_]+$/.test(value)) return 'Only lowercase letters, numbers, underscores' + return null +} + const DUMMY_VALUES: Record = { operator: 'roakey', operation: 'op_trash_panda', @@ -35,9 +42,12 @@ interface OperationPickerProps { options: string[] isLoading: boolean onSelect: (operation: string) => void + onSearchChange: () => void onDismiss: () => void inputRef: React.Ref className?: string + noteClassName?: string + noteErrorClassName?: string } /** @@ -51,14 +61,21 @@ function OperationPicker({ options, isLoading, onSelect, + onSearchChange, onDismiss, inputRef, className, + noteClassName, + noteErrorClassName, }: OperationPickerProps) { const [search, setSearch] = useState('') const matches = search ? options.filter(option => option.toLowerCase().includes(search)) : options - const canCreate = search.length > 0 && !options.some(option => option.toLowerCase() === search) + const isNewName = search.length > 0 && !options.some(option => option.toLowerCase() === search) + // Say why a name can't be created while it is being typed, rather than + // rejecting it after the fact next to a bar that clips the message. + const searchError = isNewName ? validateOperationValue(search) : null + const canCreate = isNewName && !searchError // Deferred so focus lands on whatever the user moved to before this unmounts. const dismissAfterFocusMoves = () => { setTimeout(onDismiss, 0) } @@ -73,7 +90,7 @@ function OperationPicker({ value={search} placeholder={currentValue} selectedOptions={options.includes(currentValue) ? [currentValue] : []} - onChange={e => setSearch(e.target.value.toLowerCase())} + onChange={e => { setSearch(e.target.value.toLowerCase()); onSearchChange() }} onOptionSelect={(_, data) => { if (data.optionValue) onSelect(data.optionValue) }} onKeyDownCapture={e => { // Fluent commits the active option on Tab. Block that, but let the key @@ -89,10 +106,10 @@ function OperationPicker({ data-testid="edit-label-operation" > {isLoading && ( - + )} - {!isLoading && matches.length === 0 && !canCreate && ( - )} @@ -102,6 +119,11 @@ function OperationPicker({ {canCreate && ( )} + {searchError && ( + + )} ) } @@ -304,10 +326,13 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { {key}: setError('')} onDismiss={handleCancelEdit} inputRef={editInputRef} /> From 84dabc733f8f2d57ed70e3c42ae045d18932d85d Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 17:25:26 +0000 Subject: [PATCH 04/28] FIX: Keep the operation list anchored and remember new names With more operations than fit under the input -- and there are already around sixty in use -- the list stopped hanging off the picker and became a full height column pinned to the top of the window, covering the page. Giving it a ceiling lets it stay where it belongs and scroll instead. A name typed into the picker also disappeared from it. Operations are read once from the labels API, and a name only reaches that API after an attack has been stored under it, so a name created moments earlier was absent when the picker was reopened and was offered for creation again. Newly created names now join the list they came from. --- .../src/components/Labels/LabelsBar.styles.ts | 5 +++++ .../src/components/Labels/LabelsBar.test.tsx | 18 ++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 10 +++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 64d62f2a17..569a2efa4c 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -116,6 +116,11 @@ export const useLabelsBarStyles = makeStyles({ operationPicker: { minWidth: '180px', }, + // Without a ceiling the list grows to the height of the viewport and Fluent + // parks it away from the input it belongs to. + operationListbox: { + maxHeight: '240px', + }, // Fluent dims disabled options to ~1.9:1 contrast, which is too faint for // text the user has to read. These are messages, not choices. operationNote: { diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index d5381671a3..b38da4a8f9 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1000,6 +1000,24 @@ describe('LabelsBar', () => { expect(screen.queryByTestId('edit-label-team')).not.toBeInTheDocument() }) + it('should keep a newly created operation in the list', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + fireEvent.change(await screen.findByTestId('edit-label-operation'), { + target: { value: 'op_2026_09_fresh' }, + }) + fireEvent.click(await screen.findByRole('option', { name: 'Create "op_2026_09_fresh"' })) + + // Reopen: the name it just created has to still be selectable. + fireEvent.click(screen.getByTestId('label-operation')) + + expect(await screen.findByRole('option', { name: 'op_2026_09_fresh' })).toBeInTheDocument() + expect(screen.queryByRole('option', { name: 'Create "op_2026_09_fresh"' })).not.toBeInTheDocument() + }) + it('should keep the plain input for labels other than operation', async () => { const onChange = jest.fn() renderWithOperations(onChange) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 108f784b2e..49621168f1 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -46,6 +46,7 @@ interface OperationPickerProps { onDismiss: () => void inputRef: React.Ref className?: string + listboxClassName?: string noteClassName?: string noteErrorClassName?: string } @@ -65,6 +66,7 @@ function OperationPicker({ onDismiss, inputRef, className, + listboxClassName, noteClassName, noteErrorClassName, }: OperationPickerProps) { @@ -102,6 +104,7 @@ function OperationPicker({ // Fluent sizes the dropdown to the input, which cuts off longer // operation names. Let it size to its own content instead. positioning={{ matchTargetSize: undefined }} + listbox={{ className: listboxClassName }} aria-label="Operation" data-testid="edit-label-operation" > @@ -228,11 +231,15 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { } const handleSelectOperation = (operation: string) => { + const known = existingLabels.operation || [] // Values already in memory predate the current rules, so they are always // selectable; only a newly typed name has to satisfy them. - if (!(existingLabels.operation || []).includes(operation)) { + if (!known.includes(operation)) { const valueError = validateValue(operation) if (valueError) { setError(valueError); return } + // A name only reaches the labels API once an attack has been stored under + // it, so keep it listed here or the picker forgets what it just created. + setExistingLabels(prev => ({ ...prev, operation: [...known, operation] })) } onLabelsChange({ ...labels, operation }) setEditingLabel(null) @@ -326,6 +333,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { {key}: Date: Fri, 14 Aug 2026 17:51:32 +0000 Subject: [PATCH 05/28] FIX: Say when the operations could not be loaded A failed labels request left the picker looking like a working picker with nothing in it, so someone whose backend had hiccuped was told their operations did not exist and invited to type a name that already existed somewhere else. It now says it could not load them, and keeps the "none recorded yet" wording for the case where that is actually true. Also fold the operation naming rules back into the single validator they were copied from, and stop the newly created name from being appended to a list captured before the request that fills it had returned. --- .../src/components/Labels/LabelsBar.styles.ts | 6 ++-- .../src/components/Labels/LabelsBar.test.tsx | 35 +++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 34 ++++++++++-------- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 569a2efa4c..5607f07258 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -116,8 +116,10 @@ export const useLabelsBarStyles = makeStyles({ operationPicker: { minWidth: '180px', }, - // Without a ceiling the list grows to the height of the viewport and Fluent - // parks it away from the input it belongs to. + // Keeps the list small enough that Fluent leaves it under the input rather + // than turning it into a full height column elsewhere on the page. Fluent + // writes its own max-height inline once positioned, so this is a floor on + // that decision rather than the height you end up seeing. operationListbox: { maxHeight: '240px', }, diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index b38da4a8f9..8714d8d3e0 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1000,6 +1000,41 @@ describe('LabelsBar', () => { expect(screen.queryByTestId('edit-label-team')).not.toBeInTheDocument() }) + it('should say so when the operations could not be loaded', async () => { + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockRejectedValue(new Error('boom')) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect( + await screen.findByRole('option', { name: /could not load existing operations/i }) + ).toBeInTheDocument() + expect(screen.queryByRole('option', { name: /no operations yet/i })).not.toBeInTheDocument() + }) + + it('should create a typed name with the keyboard', async () => { + const onChange = jest.fn() + renderWithOperations(onChange) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + const input = await screen.findByTestId('edit-label-operation') + fireEvent.change(input, { target: { value: 'op_2026_09_typed' } }) + await screen.findByRole('option', { name: 'Create "op_2026_09_typed"' }) + fireEvent.keyDown(input, { key: 'Enter' }) + + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'op_2026_09_typed', + }) + }) + it('should keep a newly created operation in the list', async () => { const onChange = jest.fn() renderWithOperations(onChange) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 49621168f1..89cd27c3e3 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -20,7 +20,7 @@ import { labelsApi } from '../../services/api' import { useLabelsBarStyles } from './LabelsBar.styles' -const validateOperationValue = (value: string): string | null => { +const validateValue = (value: string): string | null => { if (!value) return 'Value is required' if (value !== value.toLowerCase()) return 'Values must be lowercase' if (!/^[a-z0-9_]+$/.test(value)) return 'Only lowercase letters, numbers, underscores' @@ -41,6 +41,7 @@ interface OperationPickerProps { currentValue: string options: string[] isLoading: boolean + loadFailed: boolean onSelect: (operation: string) => void onSearchChange: () => void onDismiss: () => void @@ -61,6 +62,7 @@ function OperationPicker({ currentValue, options, isLoading, + loadFailed, onSelect, onSearchChange, onDismiss, @@ -76,7 +78,7 @@ function OperationPicker({ const isNewName = search.length > 0 && !options.some(option => option.toLowerCase() === search) // Say why a name can't be created while it is being typed, rather than // rejecting it after the fact next to a bar that clips the message. - const searchError = isNewName ? validateOperationValue(search) : null + const searchError = isNewName ? validateValue(search) : null const canCreate = isNewName && !searchError // Deferred so focus lands on whatever the user moved to before this unmounts. @@ -112,9 +114,15 @@ function OperationPicker({ )} {!isLoading && matches.length === 0 && !canCreate && !searchError && ( - + loadFailed ? ( + + ) : ( + + ) )} {matches.map(option => ( @@ -141,13 +149,14 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const [error, setError] = useState('') const [existingLabels, setExistingLabels] = useState>({}) const [labelsLoading, setLabelsLoading] = useState(true) + const [labelsFailed, setLabelsFailed] = useState(false) const editInputRef = useRef(null) // Fetch existing label keys/values for suggestions useEffect(() => { labelsApi.getLabels() .then(resp => setExistingLabels(resp.labels)) - .catch(() => { /* ignore */ }) + .catch(() => setLabelsFailed(true)) .finally(() => setLabelsLoading(false)) }, []) @@ -165,13 +174,6 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { return null } - const validateValue = (value: string): string | null => { - if (!value) return 'Value is required' - if (value !== value.toLowerCase()) return 'Values must be lowercase' - if (!/^[a-z0-9_]+$/.test(value)) return 'Only lowercase letters, numbers, underscores' - return null - } - const handleAddLabel = () => { const keyError = validateKey(newKey) if (keyError) { setError(keyError); return } @@ -239,7 +241,10 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { if (valueError) { setError(valueError); return } // A name only reaches the labels API once an attack has been stored under // it, so keep it listed here or the picker forgets what it just created. - setExistingLabels(prev => ({ ...prev, operation: [...known, operation] })) + setExistingLabels(prev => ({ + ...prev, + operation: [...(prev.operation || []), operation], + })) } onLabelsChange({ ...labels, operation }) setEditingLabel(null) @@ -339,6 +344,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { currentValue={value} options={suggestedValues} isLoading={labelsLoading} + loadFailed={labelsFailed} onSelect={handleSelectOperation} onSearchChange={() => setError('')} onDismiss={handleCancelEdit} From 8b4c049fea8ab6c56b09e9c9403f10edbfe96bf1 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 19:22:53 +0000 Subject: [PATCH 06/28] FIX: Keep an operation created while the list was still loading The picker lets a name be typed and created before the request that fills it has come back, and the response then replaced everything that had been collected in the meantime, so a name created during those first moments disappeared again as soon as the list arrived. The response is now merged with what is already there rather than replacing it. --- .../src/components/Labels/LabelsBar.test.tsx | 31 ++++++++++++++++++- frontend/src/components/Labels/LabelsBar.tsx | 7 ++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index 8714d8d3e0..93381d71cd 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' import LabelsBar from './LabelsBar' @@ -1053,6 +1053,35 @@ describe('LabelsBar', () => { expect(screen.queryByRole('option', { name: 'Create "op_2026_09_fresh"' })).not.toBeInTheDocument() }) + it('should keep an operation created while the list was still loading', async () => { + const onChange = jest.fn() + let resolveLabels: (value: { source: string; labels: Record }) => void = () => {} + mockedLabelsApi.getLabels.mockReturnValue( + new Promise(resolve => { resolveLabels = resolve }) + ) + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operation')) + fireEvent.change(await screen.findByTestId('edit-label-operation'), { + target: { value: 'op_made_while_loading' }, + }) + fireEvent.click(await screen.findByRole('option', { name: 'Create "op_made_while_loading"' })) + + // The response was in flight and cannot know about the name just created. + await act(async () => { + resolveLabels({ source: 'attacks', labels: { operation: ['op_from_server'] } }) + }) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect(await screen.findByRole('option', { name: 'op_made_while_loading' })).toBeInTheDocument() + expect(screen.getByRole('option', { name: 'op_from_server' })).toBeInTheDocument() + }) + it('should keep the plain input for labels other than operation', async () => { const onChange = jest.fn() renderWithOperations(onChange) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 89cd27c3e3..45f85bf556 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -155,7 +155,12 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { // Fetch existing label keys/values for suggestions useEffect(() => { labelsApi.getLabels() - .then(resp => setExistingLabels(resp.labels)) + // A name created while this was in flight is not in the response yet, + // so keep anything already collected rather than replacing outright. + .then(resp => setExistingLabels(prev => ({ + ...resp.labels, + operation: [...new Set([...(resp.labels.operation || []), ...(prev.operation || [])])], + }))) .catch(() => setLabelsFailed(true)) .finally(() => setLabelsLoading(false)) }, []) From a339dbb9b679808b9420d8c8788eefb67d34ba6a Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 19:58:52 +0000 Subject: [PATCH 07/28] FIX: Apply the operation list height cap The 240px cap never took effect. Fluent's combobox defaults to autoSize: true, which writes its own max-height inline once positioned, and an inline style beats the class the cap lives in. Overriding only matchTargetSize left autoSize in place, so the list stretched to whatever room it had: 501px below the input at an 800px viewport, and above the input it ran to the top of the window. Asking Fluent to auto-size width alone leaves the height to the class. Measured in Chromium with 60 options: 240px and anchored under the input at 800px, 240px and anchored above it at 500px and 420px, still scrolling internally, and the dropdown width is unchanged. It shrinks below the cap when there are fewer options. No test: jsdom does not position the popup, so the inline max-height that caused this is never written there and a unit assertion would pass either way. --- frontend/src/components/Labels/LabelsBar.styles.ts | 8 ++++---- frontend/src/components/Labels/LabelsBar.tsx | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 5607f07258..8a18df063b 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -116,10 +116,10 @@ export const useLabelsBarStyles = makeStyles({ operationPicker: { minWidth: '180px', }, - // Keeps the list small enough that Fluent leaves it under the input rather - // than turning it into a full height column elsewhere on the page. Fluent - // writes its own max-height inline once positioned, so this is a floor on - // that decision rather than the height you end up seeing. + // Caps the list so it stays under the input instead of stretching to fill + // the window. This only takes effect because the picker asks Fluent to + // auto-size width alone; by default it writes its own max-height inline, + // which beats this rule. operationListbox: { maxHeight: '240px', }, diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 45f85bf556..d2c15017cb 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -104,8 +104,9 @@ function OperationPicker({ onKeyDown={e => { if (e.key === 'Escape') onDismiss() }} onBlur={dismissAfterFocusMoves} // Fluent sizes the dropdown to the input, which cuts off longer - // operation names. Let it size to its own content instead. - positioning={{ matchTargetSize: undefined }} + // operation names, and stretches it to fill the space it has. Size to + // content instead, and leave the height to the listbox class. + positioning={{ matchTargetSize: undefined, autoSize: 'width' }} listbox={{ className: listboxClassName }} aria-label="Operation" data-testid="edit-label-operation" From 7783474f154562dc2bea8d4a39263beb2d0988a6 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 21:34:58 +0000 Subject: [PATCH 08/28] FIX: Let the operation list shrink when the window is too short Sizing the dropdown to width alone hands the height back to the class, which is what makes the 240px cap work, but it also gives up Fluent's vertical fitting. The cap was a flat 240px, so in a window shorter than about 250px the list ran past the viewport edge and the options there were unreachable. Yielding to the viewport keeps both: measured with 60 options, the list is still the full 240px and anchored at every height from 300px up, and at 200px it now renders 168px and stays on screen instead of overflowing by 40px. --- frontend/src/components/Labels/LabelsBar.styles.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 8a18df063b..f75931ccc1 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -119,9 +119,10 @@ export const useLabelsBarStyles = makeStyles({ // Caps the list so it stays under the input instead of stretching to fill // the window. This only takes effect because the picker asks Fluent to // auto-size width alone; by default it writes its own max-height inline, - // which beats this rule. + // which beats this rule. Asking for width alone also gives up Fluent's + // vertical fitting, so the cap yields to the viewport when it has to. operationListbox: { - maxHeight: '240px', + maxHeight: 'min(240px, calc(100vh - 32px))', }, // Fluent dims disabled options to ~1.9:1 contrast, which is too faint for // text the user has to read. These are messages, not choices. From 965586dfd1ca7e82dbf884a68ea42bdc3bbd312a Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 14 Aug 2026 21:35:10 +0000 Subject: [PATCH 09/28] TEST: Measure the operation picker in a real browser Every sizing bug in this feature shipped past a green unit run, because jsdom has no layout engine: reverting the fix that caps the list height leaves all 53 LabelsBar unit tests passing. These run in the existing mock Playwright project, which CI already runs on every PR and which needs only Vite. They assert what jsdom cannot -- the list is capped and anchored to the input, it stays on screen when it opens upwards, and a long operation name is not cut off. Reverting the cap fails the first one with a 501px list. --- frontend/e2e/labels-operation-picker.spec.ts | 112 +++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 frontend/e2e/labels-operation-picker.spec.ts diff --git a/frontend/e2e/labels-operation-picker.spec.ts b/frontend/e2e/labels-operation-picker.spec.ts new file mode 100644 index 0000000000..2de60fce57 --- /dev/null +++ b/frontend/e2e/labels-operation-picker.spec.ts @@ -0,0 +1,112 @@ +import { test, expect, type Page } from "@playwright/test"; + +// --------------------------------------------------------------------------- +// The operation picker's size and placement are decided by Fluent's floating +// positioning at runtime. jsdom has no layout engine, so the unit suite cannot +// see any of it — several sizing regressions shipped past a green Jest run. +// These tests measure the rendered box in a real browser. +// --------------------------------------------------------------------------- + +const LIST_MAX_HEIGHT = 240; +const LONG_OPERATION = "op_2026_08_a_very_long_operation_name_that_would_be_clipped"; + +function operations(count: number): string[] { + return Array.from( + { length: count }, + (_, i) => `op_2026_08_run_${String(i).padStart(3, "0")}`, + ); +} + +async function setupMocks(page: Page, operationLabels: string[]): Promise { + await page.route(/\/api\/labels/, async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + source: "attacks", + labels: { operator: ["roakey"], operation: operationLabels }, + }), + }); + }); + + await page.route(/\/api\/attacks(?:\?|$)/, async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ items: [], total: 0, limit: 5, offset: 0 }), + }); + }); +} + +/** Opens the picker from the labels bar and returns the rendered listbox. */ +async function openOperationPicker(page: Page) { + await page.goto("/"); + const chip = page.getByTestId("label-operation"); + await expect(chip).toBeVisible(); + await chip.click(); + + const listbox = page.getByRole("listbox"); + await expect(listbox).toBeVisible(); + return listbox; +} + +test.describe("operation picker placement", () => { + test("caps the list height and anchors it to the input", async ({ page }) => { + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, operations(60)); + const listbox = await openOperationPicker(page); + + const box = (await listbox.boundingBox())!; + const input = (await page + .getByTestId("edit-label-operation") + .boundingBox())!; + + expect(box.height).toBeLessThanOrEqual(LIST_MAX_HEIGHT); + // Opens below the input and stays attached to it. + expect(box.y).toBeGreaterThanOrEqual(input.y + input.height); + expect(box.y - (input.y + input.height)).toBeLessThan(16); + + // The options that do not fit are reachable by scrolling, not lost. + const scroll = await listbox.evaluate((el) => ({ + scrollHeight: el.scrollHeight, + clientHeight: el.clientHeight, + })); + expect(scroll.scrollHeight).toBeGreaterThan(scroll.clientHeight); + }); + + test("keeps the list on screen when it opens above the input", async ({ + page, + }) => { + // Too little room below the labels bar, so Fluent flips the list upwards. + await page.setViewportSize({ width: 1280, height: 420 }); + await setupMocks(page, operations(60)); + const listbox = await openOperationPicker(page); + + const box = (await listbox.boundingBox())!; + const input = (await page + .getByTestId("edit-label-operation") + .boundingBox())!; + const viewport = page.viewportSize()!; + + expect(box.y).toBeLessThan(input.y); + expect(box.y).toBeGreaterThanOrEqual(0); + expect(box.y + box.height).toBeLessThanOrEqual(viewport.height); + }); + + test("sizes the list to its content so long names are not clipped", async ({ + page, + }) => { + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, [LONG_OPERATION, "op_short"]); + await openOperationPicker(page); + + const option = page.getByRole("option", { name: LONG_OPERATION }); + await expect(option).toBeVisible(); + + const overflow = await option.evaluate((el) => ({ + scrollWidth: el.scrollWidth, + clientWidth: el.clientWidth, + })); + expect(overflow.scrollWidth).toBeLessThanOrEqual(overflow.clientWidth); + }); +}); From b81e47559c56d921933a7d0c1f0f26eebbc9b22b Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Mon, 17 Aug 2026 17:31:44 +0000 Subject: [PATCH 10/28] FIX: Keep the operation editor inside the labels bar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker is 180px where the plain input it replaced was 120px, and the row it sits in has never been allowed to shrink. On Home that row starts further right once the grid splits into two columns, so the control ran past the edge the bar clips at and took its dropdown chevron with it. Measured on Home with the editor open, against main: main clips 0px at every width, this branch clipped 27px at 520, 1000 and 1024 — and a hit test at the chevron's centre returned the card behind it rather than the icon. 1024 is an ordinary laptop width. Letting the operation row give way fixes it: 0px clipped and the chevron hit-testable at 520/560/600/1000/1024/1090/1280/1920. The control keeps its full 180px wherever there is room and shrinks to about 140 where there is not. The dropdown is sized separately, so it still measures 466px and shows a 58-character name in full. --- .../src/components/Labels/LabelsBar.styles.ts | 10 +++++++++- frontend/src/components/Labels/LabelsBar.tsx | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index f75931ccc1..2c09d51a4b 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -113,8 +113,16 @@ export const useLabelsBarStyles = makeStyles({ overflowY: 'auto', minWidth: '120px', }, + // The picker is wider than the plain input it replaces, and the labels bar + // clips what overflows. Let it shrink rather than lose its chevron: Fluent + // puts an intrinsic min-width on both the root and the inner input. operationPicker: { - minWidth: '180px', + width: '180px', + minWidth: 0, + maxWidth: '100%', + '& input': { + minWidth: 0, + }, }, // Caps the list so it stays under the input instead of stretching to fill // the window. This only takes effect because the picker asks Fluent to diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index d2c15017cb..7f0b1bf9cf 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -402,8 +402,21 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const isEditing = editingLabel === key && !isPopoverOpen if (isEditing) { + // The picker is wider than a plain input, so let its row give way rather + // than push the control past the edge the bar clips at. + const canShrink = key === 'operation' return ( -
+
{renderValueEditor(key, value)}
) From c89954d6095676e00d77b08d365ca15d932e4edf Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Mon, 17 Aug 2026 17:32:08 +0000 Subject: [PATCH 11/28] FIX: Show the operation in use in the picker that offers it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each labels bar fetches its own list of known operations, and the one on Home and the one in the chat ribbon are separate mounts — Home is a route, so it is thrown away when you navigate. Pick an operation on Home, go to Chat to run the attack, and the picker there does not list the value the chip is showing: typing it offers to Create the name already in use, which is exactly the confusion the picker was built to remove. It is also gone from Home on the way back. The value in use is now listed wherever it came from, and shows as selected. The placeholder stays off the list — it is not a real operation, and it would otherwise contradict "No operations yet". The empty and could-not-load notes now key off the fetched list rather than what is on screen, so listing the current value cannot suppress them. Without that, a failed load with an operation already set would have shown that operation and said nothing about the failure. --- .../src/components/Labels/LabelsBar.test.tsx | 53 +++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 17 ++++-- 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index 93381d71cd..1c9447e96d 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1053,6 +1053,59 @@ describe('LabelsBar', () => { expect(screen.queryByRole('option', { name: 'Create "op_2026_09_fresh"' })).not.toBeInTheDocument() }) + it('should list the operation in use even when the saved list has not caught up', async () => { + // The labels bar in the ribbon and the one on Home each fetch their own + // list, so a name chosen in the other one is not in this response yet. + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockResolvedValue({ + source: 'attacks', + labels: { operation: OPERATIONS, operator: ['alice'] }, + }) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect(await screen.findByRole('option', { name: 'op_chosen_elsewhere' })).toBeInTheDocument() + + // Typing it must not offer to create the name that is already set. + fireEvent.change(screen.getByTestId('edit-label-operation'), { + target: { value: 'op_chosen_elsewhere' }, + }) + expect( + screen.queryByRole('option', { name: 'Create "op_chosen_elsewhere"' }) + ).not.toBeInTheDocument() + }) + + it('should still say the operations could not be loaded when one is already set', async () => { + // The value in use is listed, but that must not read as a loaded list. + const onChange = jest.fn() + mockedLabelsApi.getLabels.mockRejectedValue(new Error('boom')) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + + expect( + await screen.findByRole('option', { name: /Could not load existing operations/ }) + ).toBeInTheDocument() + expect(screen.getByRole('option', { name: 'op_already_set' })).toBeInTheDocument() + }) + it('should keep an operation created while the list was still loading', async () => { const onChange = jest.fn() let resolveLabels: (value: { source: string; labels: Record }) => void = () => {} diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 7f0b1bf9cf..fd010dfea4 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -74,8 +74,17 @@ function OperationPicker({ }: OperationPickerProps) { const [search, setSearch] = useState('') - const matches = search ? options.filter(option => option.toLowerCase().includes(search)) : options - const isNewName = search.length > 0 && !options.some(option => option.toLowerCase() === search) + // Each labels bar fetches its own list, and the popover and ribbon mount + // separately, so a name created a moment ago may not be in `options` here. + // List it anyway, or the picker offers to create the value already in use. + // The placeholder is not a real operation, so it stays off the list. + const listed = useMemo(() => { + const inUse = currentValue && currentValue !== DUMMY_VALUES.operation + return inUse && !options.includes(currentValue) ? [...options, currentValue] : options + }, [options, currentValue]) + + const matches = search ? listed.filter(option => option.toLowerCase().includes(search)) : listed + const isNewName = search.length > 0 && !listed.some(option => option.toLowerCase() === search) // Say why a name can't be created while it is being typed, rather than // rejecting it after the fact next to a bar that clips the message. const searchError = isNewName ? validateValue(search) : null @@ -93,7 +102,7 @@ function OperationPicker({ defaultOpen value={search} placeholder={currentValue} - selectedOptions={options.includes(currentValue) ? [currentValue] : []} + selectedOptions={listed.includes(currentValue) ? [currentValue] : []} onChange={e => { setSearch(e.target.value.toLowerCase()); onSearchChange() }} onOptionSelect={(_, data) => { if (data.optionValue) onSelect(data.optionValue) }} onKeyDownCapture={e => { @@ -114,7 +123,7 @@ function OperationPicker({ {isLoading && ( )} - {!isLoading && matches.length === 0 && !canCreate && !searchError && ( + {!isLoading && options.length === 0 && !canCreate && !searchError && ( loadFailed ? ( )} - {matches.map(option => ( + {matches.slice(0, MAX_LISTED).map(option => ( ))} + {matches.length > MAX_LISTED && ( + + )} {canCreate && ( )} From 2fe7dde6dafc91e7faab1e49072f5bd421192166 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Wed, 19 Aug 2026 17:31:56 +0000 Subject: [PATCH 23/28] FIX: Do not let the option cap hide the name you asked for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the capped list could drop the very operation you wanted, both only reachable once memory holds more names than the cap shows. The operation in use was only moved to the front of the list when the labels request had not returned it. Once an attack has been stored under it, it usually is in that response, and it stays wherever it sorted — so the cap dropped it and you could no longer see or re-pick the operation you were working in. Typing a name in full had the same problem from the other end. If two hundred other operations contain what you typed, the exact one sorts wherever it sorts, the list never shows it, and no offer to create it appears either, because it does exist. Pressing Enter then committed whichever operation happened to be listed first: a different operation from the one you typed, with nothing on screen to say so. Both are now pinned ahead of the cap. Also says in the GUI docs that long lists show the first two hundred. --- doc/gui/0_gui.md | 2 +- frontend/e2e/labels-operation-picker.spec.ts | 33 +++++++-- .../src/components/Labels/LabelsBar.test.tsx | 69 ++++++++++++++++--- frontend/src/components/Labels/LabelsBar.tsx | 16 ++++- 4 files changed, 101 insertions(+), 19 deletions(-) diff --git a/doc/gui/0_gui.md b/doc/gui/0_gui.md index 97963b539c..22be7a91af 100644 --- a/doc/gui/0_gui.md +++ b/doc/gui/0_gui.md @@ -87,7 +87,7 @@ The export runs entirely in your browser and captures exactly what is shown in t The labels bar in the ribbon displays the current attack's labels (e.g., `operator`, `operation`). Labels are key-value pairs that help organize and filter attacks. You can add, edit, and remove labels inline. The `operator` and `operation` labels are required and cannot be removed. -Clicking the `operation` label opens a picker listing the operations already recorded in memory, so you can choose one without typing it from memory. Typing a name that doesn't exist yet offers to create it. The operation you pick is applied to attacks you start from then on; it does not change attacks that already exist. +Clicking the `operation` label opens a picker listing the operations already recorded in memory, so you can choose one without typing it from memory. Typing a name that doesn't exist yet offers to create it. Very long lists show the first 200 and say how many are left, so type to narrow them. The operation you pick is applied to attacks you start from then on; it does not change attacks that already exist. #### Behavioral Guards diff --git a/frontend/e2e/labels-operation-picker.spec.ts b/frontend/e2e/labels-operation-picker.spec.ts index 17152c9977..91369d467c 100644 --- a/frontend/e2e/labels-operation-picker.spec.ts +++ b/frontend/e2e/labels-operation-picker.spec.ts @@ -208,7 +208,7 @@ test.describe("operation picker placement", () => { // so an uncapped list stalls the tab. Measured before the cap, 50k options // took ~27s for a single keystroke. await page.setViewportSize({ width: 1280, height: 800 }); - await setupMocks(page, operations(5000)); + await setupMocks(page, operations(600)); const listbox = await openOperationPicker(page); await expect(listbox.getByRole("option").first()).toBeVisible(); @@ -216,9 +216,9 @@ test.describe("operation picker placement", () => { // Typing has to stay responsive, which is the thing that was broken. const started = Date.now(); - await page.getByTestId("edit-label-operation").fill("run_004"); + await page.getByTestId("edit-label-operation").fill("run_599"); await expect( - page.getByRole("option", { name: "op_2026_08_run_004", exact: true }), + page.getByRole("option", { name: "op_2026_08_run_599", exact: true }), ).toBeVisible(); expect(Date.now() - started).toBeLessThan(3000); }); @@ -226,8 +226,8 @@ test.describe("operation picker placement", () => { test("keeps the operation in use reachable past the end of a long list", async ({ page, }) => { - // The value in use is added to whatever the API returned. Cap the wrong - // end of that list and it is the first thing to disappear. + // The value in use goes to the front of the list. Cap the wrong end and it + // is the first thing to disappear — whether or not the request returned it. await page.setViewportSize({ width: 1280, height: 800 }); await page.addInitScript(() => { window.localStorage.setItem( @@ -235,7 +235,7 @@ test.describe("operation picker placement", () => { JSON.stringify({ operator: "roakey", operation: "op_chosen_elsewhere" }), ); }); - await setupMocks(page, operations(5000)); + await setupMocks(page, operations(600)); await openOperationPicker(page); const inUse = page.getByRole("option", { @@ -248,6 +248,27 @@ test.describe("operation picker placement", () => { "op_chosen_elsewhere", ); }); + + test("keeps an operation the saved list already holds past the cap", async ({ + page, + }) => { + // The usual case: the operation in use is in the response, just not near + // the front of it. + const inUseName = "op_2026_08_run_400"; + await page.setViewportSize({ width: 1280, height: 800 }); + await page.addInitScript((name) => { + window.localStorage.setItem( + "pyrit.globalLabels", + JSON.stringify({ operator: "roakey", operation: name }), + ); + }, inUseName); + await setupMocks(page, operations(600)); + await openOperationPicker(page); + + await expect( + page.getByRole("option", { name: inUseName, exact: true }), + ).toHaveCount(1); + }); }); test.describe("operation picker persistence", () => { diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index d5e149e936..c9ede4b23e 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1188,10 +1188,10 @@ describe('LabelsBar', () => { }) it('should keep the operation in use on the list when the list is capped', async () => { - // The value in use is appended to whatever the API returned, so a cap - // applied to the end of the list is exactly what would drop it. + // The value in use is put at the front of whatever the API returned, so + // a cap applied to the end of the list is exactly what would drop it. const onChange = jest.fn() - const many = Array.from({ length: 400 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) + const many = Array.from({ length: 250 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) mockedLabelsApi.getLabels.mockResolvedValue({ source: 'attacks', labels: { operation: many, operator: ['alice'] }, @@ -1219,9 +1219,60 @@ describe('LabelsBar', () => { }) }) + it('should keep the operation in use on a capped list that already contains it', async () => { + // The saved list usually does contain the operation in use, and it can + // sit anywhere in it — including past the cap. + const onChange = jest.fn() + const many = Array.from({ length: 250 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) + mockedLabelsApi.getLabels.mockResolvedValue({ + source: 'attacks', + labels: { operation: many, operator: ['alice'] }, + }) + render( + + + + ) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + + // Listed once, not twice, even though it is also in the saved list. + expect(await screen.findAllByRole('option', { name: 'op_2026_08_run_0240' })).toHaveLength(1) + expect(screen.getByText('Showing 200 of 250 — type to narrow')).toBeInTheDocument() + }) + + it('should keep a name typed in full on a capped list', async () => { + // Every decoy contains the typed name, so the exact match sorts last and + // the cap would hide it — leaving Enter to commit a different operation. + const onChange = jest.fn() + const decoys = Array.from({ length: 250 }, (_, i) => `op_2026_08_run_042_${String(i).padStart(3, '0')}`) + renderWithOperations(onChange, [...decoys, 'run_042'].sort()) + await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) + + fireEvent.click(screen.getByTestId('label-operation')) + fireEvent.change(await screen.findByTestId('edit-label-operation'), { + target: { value: 'run_042' }, + }) + + const exact = await screen.findByRole('option', { name: 'run_042' }) + expect(exact).toBeInTheDocument() + // It is not offered for creation, because it already exists. + expect(screen.queryByRole('option', { name: 'Create "run_042"' })).not.toBeInTheDocument() + + fireEvent.click(exact) + expect(onChange).toHaveBeenCalledWith({ + ...DEFAULT_GLOBAL_LABELS, + operation: 'run_042', + }) + }) + it('should show only the first page of a long list and say so', async () => { const onChange = jest.fn() - const many = Array.from({ length: 400 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) + const many = Array.from({ length: 250 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) renderWithOperations(onChange, many) await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) @@ -1229,20 +1280,20 @@ describe('LabelsBar', () => { await screen.findByRole('option', { name: 'op_2026_08_run_0000' }) expect(screen.getAllByRole('option')).toHaveLength(201) - expect(screen.getByText('Showing 200 of 400 — type to narrow')).toBeInTheDocument() - expect(screen.queryByRole('option', { name: 'op_2026_08_run_0399' })).not.toBeInTheDocument() + expect(screen.getByText('Showing 200 of 250 — type to narrow')).toBeInTheDocument() + expect(screen.queryByRole('option', { name: 'op_2026_08_run_0249' })).not.toBeInTheDocument() // Typing narrows it below the cap, and then the note goes away. fireEvent.change(screen.getByTestId('edit-label-operation'), { - target: { value: 'run_039' }, + target: { value: 'run_024' }, }) - expect(await screen.findByRole('option', { name: 'op_2026_08_run_0399' })).toBeInTheDocument() + expect(await screen.findByRole('option', { name: 'op_2026_08_run_0249' })).toBeInTheDocument() expect(screen.queryByText(/type to narrow/)).not.toBeInTheDocument() }) it('should not offer the cap note as something to choose', async () => { const onChange = jest.fn() - const many = Array.from({ length: 400 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) + const many = Array.from({ length: 250 }, (_, i) => `op_2026_08_run_${String(i).padStart(4, '0')}`) renderWithOperations(onChange, many) await waitFor(() => expect(mockedLabelsApi.getLabels).toHaveBeenCalled()) diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 13a33e1fb4..bdcd67aeaf 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -81,11 +81,12 @@ function OperationPicker({ // Each labels bar fetches its own list, and the popover and ribbon mount // separately, so a name created a moment ago may not be in `options` here. // List it anyway, or the picker offers to create the value already in use. - // It goes first so the cap below can never be what drops it. + // It goes first, whether or not the request returned it, so the cap below + // can never be what drops it. // The placeholder is not a real operation, so it stays off the list. const listed = useMemo(() => { const inUse = currentValue && currentValue !== DUMMY_VALUES.operation - return inUse && !options.includes(currentValue) ? [currentValue, ...options] : options + return inUse ? [currentValue, ...options.filter(option => option !== currentValue)] : options }, [options, currentValue]) const matches = search ? listed.filter(option => option.toLowerCase().includes(search)) : listed @@ -95,6 +96,15 @@ function OperationPicker({ const searchError = isNewName ? validateValue(search) : null const canCreate = isNewName && !searchError + // A name typed in full has to survive the cap too. Without this, typing an + // operation whose name is also a substring of two hundred others would leave + // it off the list, and Enter would commit whichever one happened to be first. + const shown = useMemo(() => { + const exact = matches.find(option => option.toLowerCase() === search) + const ordered = exact ? [exact, ...matches.filter(option => option !== exact)] : matches + return ordered.slice(0, MAX_LISTED) + }, [matches, search]) + // Deferred so focus lands on whatever the user moved to before this unmounts. const dismissAfterFocusMoves = () => { setTimeout(onDismiss, 0) } @@ -138,7 +148,7 @@ function OperationPicker({ No operations yet — type a name to create one )} - {matches.slice(0, MAX_LISTED).map(option => ( + {shown.map(option => ( ))} {matches.length > MAX_LISTED && ( From 8cb03c069947fa0097d68b9bbb40401d3f72e081 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Mon, 24 Aug 2026 15:22:19 +0000 Subject: [PATCH 24/28] FIX: Keep the label edit you just started Both label editors finish on blur a turn later, so that focus lands before the editor goes away. By then the click that took the focus has already opened the next editor, and the clean-up ended that one instead: the click looked like it did nothing, and a second click was needed. Leaving an editor now only ends the edit it was started for. The value is still saved either way; only the clean-up is skipped. This was already true of the plain input on main, where the blur waits 150ms and loses the race at an ordinary click speed, so both paths are covered. The clean-up has to leave the new editor's value alone as well as its identity. Guarding only the identity leaves the editor open but empty, which is the same bug wearing a different hat. Also stops the remove button sitting inside the control that edits the label. Screen readers flatten a button nested in another button, and this one had no name of its own to fall back on, so it read as an unlabelled button. It is now a sibling of the edit control and says what it removes. --- frontend/e2e/labels-operation-picker.spec.ts | 41 +++++++++++ .../src/components/Labels/LabelsBar.styles.ts | 13 ++++ .../src/components/Labels/LabelsBar.test.tsx | 70 +++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 67 +++++++++++------- 4 files changed, 165 insertions(+), 26 deletions(-) diff --git a/frontend/e2e/labels-operation-picker.spec.ts b/frontend/e2e/labels-operation-picker.spec.ts index 91369d467c..6646d5bfe9 100644 --- a/frontend/e2e/labels-operation-picker.spec.ts +++ b/frontend/e2e/labels-operation-picker.spec.ts @@ -402,3 +402,44 @@ test.describe("operation picker persistence", () => { await expect(page.getByTestId("label-operation")).toContainText("op_beta"); }); }); + +test.describe("switching between labels", () => { + test("keeps the label you click on next while leaving the picker", async ({ + page, + }) => { + // Both editors finish on blur a turn later, so the click that takes the + // focus has already opened the next editor by then. jsdom does not order + // blur and click the way a browser does, so only this can see it. + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, ["op_alpha", "op_beta"]); + await openOperationPicker(page); + + await page.getByTestId("label-operator").click(); + + const operatorEditor = page.getByTestId("edit-label-operator"); + await expect(operatorEditor).toBeVisible(); + await expect(operatorEditor).toHaveValue("roakey"); + await page.waitForTimeout(500); + await expect(operatorEditor).toBeVisible(); + await expect(operatorEditor).toHaveValue("roakey"); + }); + + test("keeps the picker you open while leaving another label", async ({ + page, + }) => { + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, ["op_alpha", "op_beta"]); + await page.goto("/"); + + await page.getByTestId("label-operator").click(); + await page.getByTestId("edit-label-operator").fill("alice"); + await page.getByTestId("label-operation").click(); + + await expect(page.getByRole("listbox")).toBeVisible(); + await page.waitForTimeout(500); + await expect(page.getByRole("listbox")).toBeVisible(); + // The operator edit still went in; only its clean-up was skipped. + await page.keyboard.press("Escape"); + await expect(page.getByTestId("label-operator")).toContainText("alice"); + }); +}); diff --git a/frontend/src/components/Labels/LabelsBar.styles.ts b/frontend/src/components/Labels/LabelsBar.styles.ts index 2c09d51a4b..d99a8b0394 100644 --- a/frontend/src/components/Labels/LabelsBar.styles.ts +++ b/frontend/src/components/Labels/LabelsBar.styles.ts @@ -61,6 +61,19 @@ export const useLabelsBarStyles = makeStyles({ backgroundColor: tokens.colorPaletteYellowBackground2, border: `1px solid ${tokens.colorPaletteYellowBorder1}`, }, + labelEdit: { + display: 'inline-flex', + alignItems: 'center', + gap: tokens.spacingHorizontalXXS, + background: 'none', + border: 'none', + padding: 0, + margin: 0, + font: 'inherit', + color: 'inherit', + cursor: 'pointer', + userSelect: 'none' as const, + }, removeBtn: { minWidth: '16px', width: '16px', diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index c9ede4b23e..aedf836a7c 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -88,6 +88,26 @@ describe('LabelsBar', () => { }) }) + it('should keep the remove button out of the edit control', async () => { + // A control that removes the label cannot sit inside the control that + // edits it: screen readers flatten the inner one and it loses its name. + const onChange = jest.fn() + render( + + + + ) + + const edit = screen.getByTestId('label-team') + const remove = screen.getByTestId('remove-label-team') + + expect(edit).toHaveAttribute('role', 'button') + expect(edit).not.toContainElement(remove) + expect(remove).toHaveAccessibleName('Remove team label') + // Required labels have nothing to nest in the first place. + expect(screen.getByTestId('label-operator')).toHaveAttribute('role', 'button') + }) + it('should add a new label via popover', async () => { const onChange = jest.fn() render( @@ -286,6 +306,56 @@ describe('LabelsBar', () => { }) }) + it('should keep the edit you just started when leaving another one', async () => { + // Both editors finish on blur a turn later. If that late work is not tied + // to the label it was started for, it ends whichever edit is open by then, + // and the click that opened it looks like it did nothing. + const onChange = jest.fn() + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + fireEvent.change(operatorInput, { target: { value: 'alice' } }) + + // Leaving the operator schedules its save; the click starts the next edit. + fireEvent.blur(operatorInput) + fireEvent.click(screen.getByTestId('label-operation')) + await screen.findByTestId('edit-label-operation') + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + expect(screen.getByTestId('edit-label-operation')).toBeInTheDocument() + expect(screen.queryByTestId('edit-label-operator')).not.toBeInTheDocument() + // The operator edit still went in; only its clean-up was skipped. + expect(onChange).toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS, operator: 'alice' }) + }) + + it('should not clear the value of the edit you just started', async () => { + const onChange = jest.fn() + render( + + + + ) + + // Leaving the operation picker for the operator, the other way round. + fireEvent.click(screen.getByTestId('label-operation')) + const operationInput = await screen.findByTestId('edit-label-operation') + fireEvent.blur(operationInput) + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + // An editor that opens empty is the same bug wearing a different hat. + expect(screen.getByTestId('edit-label-operator')).toBeInTheDocument() + expect(operatorInput).toHaveValue(DEFAULT_GLOBAL_LABELS.operator) + }) + it('should cancel edit on Escape key', async () => { const onChange = jest.fn() render( diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index bdcd67aeaf..4340996a68 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -180,6 +180,11 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const [labelsLoading, setLabelsLoading] = useState(true) const [labelsFailed, setLabelsFailed] = useState(false) const editInputRef = useRef(null) + // Both editors finish their work on blur, one turn later, so that focus lands + // first. By then the click that took the focus may already have opened a + // different label's editor, which this has to be able to notice. + const editingLabelRef = useRef(null) + useEffect(() => { editingLabelRef.current = editingLabel }, [editingLabel]) // Fetch existing label keys/values for suggestions useEffect(() => { @@ -245,14 +250,20 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { } } + /** Ends an edit, unless the user has already moved on to another label. */ + const endEdit = (key: string) => { + if (editingLabelRef.current !== key) return + setEditingLabel(null) + setEditValue('') + setError('') + } + const handleSaveEdit = () => { if (!editingLabel) return const valueError = validateValue(editValue) if (valueError) { setError(valueError); return } onLabelsChange({ ...labels, [editingLabel]: editValue }) - setEditingLabel(null) - setEditValue('') - setError('') + endEdit(editingLabel) } const handleEditKeyDown = (e: React.KeyboardEvent) => { @@ -260,12 +271,6 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { if (e.key === 'Escape') { setEditingLabel(null); setError('') } } - const handleCancelEdit = () => { - setEditingLabel(null) - setEditValue('') - setError('') - } - const handleSelectOperation = (operation: string) => { const known = existingLabels.operation || [] // Values already in memory predate the current rules, and so may the one @@ -383,7 +388,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { loadFailed={labelsFailed} onSelect={handleSelectOperation} onSearchChange={() => setError('')} - onDismiss={handleCancelEdit} + onDismiss={() => endEdit(key)} inputRef={editInputRef} /> {error && {error}} @@ -461,16 +466,20 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) {
handleStartEdit(key)} - onKeyDown={e => handleStartEditKeyDown(e, key)} - role="button" - tabIndex={0} - aria-label={`Edit ${key} label, currently ${value}`} - data-testid={`label-${key}`} style={{ flexShrink: 0 }} > - {key}: - {value} +
handleStartEdit(key)} + onKeyDown={e => handleStartEditKeyDown(e, key)} + role="button" + tabIndex={0} + aria-label={`Edit ${key} label, currently ${value}`} + data-testid={`label-${key}`} + > + {key}: + {value} +
{!isRequired && (
- + + {!isRequired && ( +
) } From c5867a8a94d5b04d36c947bc7973614650b74a25 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Mon, 24 Aug 2026 16:55:22 +0000 Subject: [PATCH 27/28] FIX: Stop a late save undoing a label picked meanwhile The save that runs a moment after blur wrote the whole set of labels as it saw them at that moment. Pick an operation before it gets its turn and the save puts the old one back: the choice appears, then quietly reverts, and only the saved label reaches storage. Reproduced in Chromium three times out of three, with the pick landing 30-50ms after the blur. It now writes onto the labels as they are when it runs, so it only ever changes the one it was editing. --- .../src/components/Labels/LabelsBar.test.tsx | 33 +++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 6 +++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index 8e8ba63489..612a5a50d1 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -414,6 +414,39 @@ describe('LabelsBar', () => { expect(onChange).not.toHaveBeenCalled() }) + it('should not undo a label chosen while another one was still saving', async () => { + // The save runs a moment after blur and used to write the labels it saw + // then, quietly putting back anything picked in between. + const onChange = jest.fn() + const { rerender } = render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + fireEvent.change(operatorInput, { target: { value: 'dana' } }) + fireEvent.blur(operatorInput) + + // Something else changes the labels before the save gets its turn. + rerender( + + + + ) + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + expect(onChange).toHaveBeenCalledWith({ + operator: 'dana', + operation: 'op_2026_08_picked', + }) + }) + it('should cancel edit on Escape key', async () => { const onChange = jest.fn() render( diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 14e30501d1..816fc3482a 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -185,6 +185,10 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { // different edit, which this has to be able to notice. Counting the edits is // what tells them apart: the same label can be picked up again in between. const editSession = useRef(0) + // That late save also has to write onto the labels as they are by then, not + // the ones it was looking at when the blur happened. + const labelsRef = useRef(labels) + useEffect(() => { labelsRef.current = labels }, [labels]) // Fetch existing label keys/values for suggestions useEffect(() => { @@ -267,7 +271,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { if (editSession.current === session) setError(valueError) return } - onLabelsChange({ ...labels, [key]: editValue }) + onLabelsChange({ ...labelsRef.current, [key]: editValue }) endEdit(session) } From f2bf6da01293d85e2711c1560a18a85f319e4339 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Wed, 26 Aug 2026 03:08:39 +0000 Subject: [PATCH 28/28] FIX Call off a label save the edit no longer needs A save waits out the blur so focus can land first. If that edit finished another way in the meantime the save still ran, landing a moment later with the value that was typed rather than the one that was chosen: - picking a suggestion put the half-typed value back over it - removing a label while its own editor was open brought it back Both now cancel the save the blur left behind. Keeping the timers per edit rather than one at a time matters when two edits finish inside the same delay, where remembering only the last one let the first through. The suggestion also holds the focus on mousedown, so the blur that starts all this no longer happens on that path at all. The label the save writes onto is the one this component last left, which is what the pending save has to build on when a removal or another edit landed first. Clicking a chip beside its text did nothing, because the pill's padding sits outside the control that opens the editor while still showing a pointer. The chip now passes those clicks on, which leaves the remove button and the width measuring alone. --- frontend/e2e/labels-operation-picker.spec.ts | 51 +++++- .../src/components/Labels/LabelsBar.test.tsx | 147 ++++++++++++++++++ frontend/src/components/Labels/LabelsBar.tsx | 59 ++++++- 3 files changed, 248 insertions(+), 9 deletions(-) diff --git a/frontend/e2e/labels-operation-picker.spec.ts b/frontend/e2e/labels-operation-picker.spec.ts index 6646d5bfe9..14f4ca5d82 100644 --- a/frontend/e2e/labels-operation-picker.spec.ts +++ b/frontend/e2e/labels-operation-picker.spec.ts @@ -20,7 +20,11 @@ function operations(count: number): string[] { async function setupMocks( page: Page, operationLabels: string[], - options: { versionDelayMs?: number; defaultLabels?: Record } = {}, + options: { + versionDelayMs?: number; + defaultLabels?: Record; + operatorLabels?: string[]; + } = {}, ): Promise { // Everything the app calls while booting, so the run does not depend on a // dev-server proxy with no backend behind it. @@ -46,7 +50,10 @@ async function setupMocks( if (path === "/labels") { return route.fulfill(json({ source: "attacks", - labels: { operator: ["roakey"], operation: operationLabels }, + labels: { + operator: options.operatorLabels ?? ["roakey"], + operation: operationLabels, + }, })); } if (path === "/attacks") { @@ -443,3 +450,43 @@ test.describe("switching between labels", () => { await expect(page.getByTestId("label-operator")).toContainText("alice"); }); }); + +test.describe("finishing an edit another way", () => { + test("keeps a suggestion picked while the typed value was still saving", async ({ + page, + }) => { + // The input's blur schedules a save of what was typed, and only a browser + // orders that blur against the click that picked the suggestion. + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, ["op_alpha"], { operatorLabels: ["roakey", "alice"] }); + await page.goto("/"); + + await page.getByTestId("label-operator").click(); + await page.getByTestId("edit-label-operator").fill("al"); + await page.getByText("alice", { exact: true }).click(); + + await page.waitForTimeout(500); + await expect(page.getByTestId("label-operator")).toContainText("alice"); + }); + + test("starts an edit when the chip is clicked beside the edit control", async ({ + page, + }) => { + // The pill's padding sits outside the control that opens the editor, and + // only a real layout says where that padding actually is. + await page.setViewportSize({ width: 1280, height: 800 }); + await setupMocks(page, ["op_alpha"]); + await page.goto("/"); + + const chip = page.getByTestId("label-operator"); + await expect(chip).toBeVisible(); + const badge = chip.locator("xpath=.."); + const box = await badge.boundingBox(); + if (!box) throw new Error("chip has no layout"); + + // Two pixels in from the pill's left edge is padding, not the control. + await page.mouse.click(box.x + 2, box.y + box.height / 2); + + await expect(page.getByTestId("edit-label-operator")).toBeVisible(); + }); +}); diff --git a/frontend/src/components/Labels/LabelsBar.test.tsx b/frontend/src/components/Labels/LabelsBar.test.tsx index 612a5a50d1..f69dbe0eda 100644 --- a/frontend/src/components/Labels/LabelsBar.test.tsx +++ b/frontend/src/components/Labels/LabelsBar.test.tsx @@ -1,3 +1,4 @@ +import { useState } from 'react' import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' @@ -121,6 +122,22 @@ describe('LabelsBar', () => { expect(screen.getByTestId('label-operator')).toHaveAttribute('role', 'button') }) + it('should start an edit when the chip is clicked beside the edit control', async () => { + // Moving the click onto an inner control left the pill's own padding + // showing a pointer and doing nothing, so the edges of a chip looked + // clickable but were not. + const onChange = jest.fn() + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-team').parentElement as HTMLElement) + + expect(await screen.findByTestId('edit-label-team')).toBeInTheDocument() + }) + it('should add a new label via popover', async () => { const onChange = jest.fn() render( @@ -447,6 +464,136 @@ describe('LabelsBar', () => { }) }) + it('should keep a suggestion you picked while the last value was still saving', async () => { + // Leaving the input schedules a save of what was typed. Picking a + // suggestion is that same edit finishing another way, so the save it left + // behind must not put the half-typed value back. + mockedLabelsApi.getLabels.mockResolvedValueOnce({ + source: 'attacks', + labels: { operator: ['alice'] }, + }) + + const onChange = jest.fn() + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + fireEvent.change(operatorInput, { target: { value: 'al' } }) + + const suggestion = await screen.findByText('alice') + fireEvent.blur(operatorInput) + fireEvent.click(suggestion) + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + expect(onChange).toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS, operator: 'alice' }) + expect(onChange).not.toHaveBeenCalledWith({ ...DEFAULT_GLOBAL_LABELS, operator: 'al' }) + }) + + it('should keep both values when two suggestions are picked in quick succession', async () => { + // Each edit leaves its own save behind, so remembering only the last one + // that finished early lets the one before it through with a stale value. + mockedLabelsApi.getLabels.mockResolvedValueOnce({ + source: 'attacks', + labels: { operator: ['alice'], team: ['blue'] }, + }) + + const onChange = jest.fn() + // The real bar is driven by state in App, so a value it commits is on its + // way back down as a prop while the next edit is already under way. + const Harness = () => { + const [labels, setLabels] = useState({ ...DEFAULT_GLOBAL_LABELS, team: 'bravo' }) + return ( + + { onChange(next); setLabels(next) }} + /> + + ) + } + render() + + // Wait for the suggestions once, then run the sequence without awaiting + // anything: both edits have to finish inside the same save delay. + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + fireEvent.change(operatorInput, { target: { value: 'al' } }) + const alice = await screen.findByText('alice') + fireEvent.blur(operatorInput) + fireEvent.click(alice) + + fireEvent.click(screen.getByTestId('label-team')) + const teamInput = await screen.findByTestId('edit-label-team') + fireEvent.change(teamInput, { target: { value: 'bl' } }) + const blue = await screen.findByText('blue') + fireEvent.blur(teamInput) + fireEvent.click(blue) + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + const [last] = onChange.mock.calls[onChange.mock.calls.length - 1] + expect(last).toEqual({ ...DEFAULT_GLOBAL_LABELS, operator: 'alice', team: 'blue' }) + }) + + it('should not bring back a label removed while another one was still saving', async () => { + const onChange = jest.fn() + render( + + + + ) + + fireEvent.click(screen.getByTestId('label-operator')) + const operatorInput = await screen.findByTestId('edit-label-operator') + fireEvent.change(operatorInput, { target: { value: 'dana' } }) + fireEvent.blur(operatorInput) + fireEvent.click(screen.getByTestId('remove-label-team')) + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + const [last] = onChange.mock.calls[onChange.mock.calls.length - 1] + expect(last).not.toHaveProperty('team') + expect(last).toHaveProperty('operator', 'dana') + }) + + it('should not bring back a label removed while it was the one being edited', async () => { + // The popover editor leaves the label's own chip on the bar, so the label + // can be taken away while its edit is still finishing. + const onChange = jest.fn() + const Harness = () => { + const [labels, setLabels] = useState({ ...DEFAULT_GLOBAL_LABELS, team: 'green' }) + return ( + + { onChange(next); setLabels(next) }} + /> + + ) + } + render() + + fireEvent.click(screen.getByTestId('labels-icon-btn')) + fireEvent.click(await screen.findByTestId('popover-label-team')) + const teamInput = await screen.findByTestId('edit-label-team') + fireEvent.change(teamInput, { target: { value: 'gr' } }) + fireEvent.blur(teamInput) + fireEvent.click(screen.getByTestId('remove-label-team')) + + await act(async () => { await new Promise(r => setTimeout(r, 400)) }) + + const [last] = onChange.mock.calls[onChange.mock.calls.length - 1] + expect(last).not.toHaveProperty('team') + }) + it('should cancel edit on Escape key', async () => { const onChange = jest.fn() render( diff --git a/frontend/src/components/Labels/LabelsBar.tsx b/frontend/src/components/Labels/LabelsBar.tsx index 816fc3482a..e83039b36f 100644 --- a/frontend/src/components/Labels/LabelsBar.tsx +++ b/frontend/src/components/Labels/LabelsBar.tsx @@ -185,11 +185,32 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { // different edit, which this has to be able to notice. Counting the edits is // what tells them apart: the same label can be picked up again in between. const editSession = useRef(0) + // A save waits out the blur so focus can land first. If the edit it belongs + // to finishes another way in the meantime — a suggestion picked, the label + // removed — that save has to be called off, or it lands afterwards with the + // value it was typed with. Starting a different edit is not the same thing: + // that save is still the user's and still has to land. + const pendingSaves = useRef(new Map>()) + + const cancelPendingSave = (session: number) => { + const timer = pendingSaves.current.get(session) + if (timer === undefined) return + clearTimeout(timer) + pendingSaves.current.delete(session) + } // That late save also has to write onto the labels as they are by then, not // the ones it was looking at when the blur happened. const labelsRef = useRef(labels) useEffect(() => { labelsRef.current = labels }, [labels]) + // Writing through the ref as well means a save still waiting its turn works + // off the labels as this component last left them, rather than depending on + // the parent having re-rendered in the meantime. + const commitLabels = (next: Record) => { + labelsRef.current = next + onLabelsChange(next) + } + // Fetch existing label keys/values for suggestions useEffect(() => { labelsApi.getLabels() @@ -223,7 +244,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const valueError = validateValue(newValue) if (valueError) { setError(valueError); return } - onLabelsChange({ ...labels, [newKey]: newValue }) + commitLabels({ ...labelsRef.current, [newKey]: newValue }) setNewKey('') setNewValue('') setError('') @@ -233,9 +254,16 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { const handleRemoveLabel = (key: string) => { // Don't allow removing operator or operation — they're required if (key === 'operator' || key === 'operation') return - const next = { ...labels } + // The label may be open for editing in the popover while its chip is still + // on the bar, and that edit has a save on the way. Taking the label away + // has to take the save with it, or it comes back a moment later. + if (editingLabel === key) { + cancelPendingSave(editSession.current) + endEdit(editSession.current) + } + const next = { ...labelsRef.current } delete next[key] - onLabelsChange(next) + commitLabels(next) } const handleStartEdit = (key: string) => { @@ -271,7 +299,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { if (editSession.current === session) setError(valueError) return } - onLabelsChange({ ...labelsRef.current, [key]: editValue }) + commitLabels({ ...labelsRef.current, [key]: editValue }) endEdit(session) } @@ -296,7 +324,7 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { operation: [...(prev.operation || []), operation], })) } - onLabelsChange({ ...labels, operation }) + commitLabels({ ...labelsRef.current, operation }) setEditingLabel(null) setEditValue('') setError('') @@ -419,7 +447,13 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { value={editValue} onChange={(_, d) => { setEditValue(d.value.toLowerCase()); setError('') }} onKeyDown={e => handleEditKeyDown(e, key, session)} - onBlur={() => { setTimeout(() => saveEdit(key, session), 150) }} + onBlur={() => { + cancelPendingSave(session) + pendingSaves.current.set(session, setTimeout(() => { + pendingSaves.current.delete(session) + saveEdit(key, session) + }, 150)) + }} style={{ width: '120px' }} data-testid={`edit-label-${key}`} /> @@ -432,7 +466,13 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { appearance="outline" size="small" className={styles.suggestionChip} - onClick={() => { onLabelsChange({ ...labels, [key]: v }); setEditingLabel(null); setEditValue('') }} + onMouseDown={e => e.preventDefault()} + onClick={() => { + cancelPendingSave(session) + commitLabels({ ...labelsRef.current, [key]: v }) + setEditingLabel(null) + setEditValue('') + }} >{v} ))} @@ -474,6 +514,10 @@ export default function LabelsBar({ labels, onLabelsChange }: LabelsBarProps) { data-label-idx={idx} className={`${styles.labelBadge} ${isDummy ? styles.labelDummy : styles.labelNormal}`} style={{ flexShrink: 0 }} + // The pill's own padding sits outside the edit control, so a click that + // lands on it reaches nothing. Forward only those: anything on a child + // is that child's to handle. + onClick={e => { if (e.target === e.currentTarget) handleStartEdit(key) }} > { if (e.target === e.currentTarget) handleStartEdit(key) }} >