diff --git a/frontend/src/components/Initializers/AdditionalInitializers.test.tsx b/frontend/src/components/Initializers/AdditionalInitializers.test.tsx index c05d401cd8..74a909d626 100644 --- a/frontend/src/components/Initializers/AdditionalInitializers.test.tsx +++ b/frontend/src/components/Initializers/AdditionalInitializers.test.tsx @@ -1,6 +1,7 @@ import { fireEvent, render, screen, within } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' +import { useState } from 'react' import type { AdditionalInitializerSetting, RegisteredInitializer } from '@/types' @@ -138,7 +139,8 @@ describe('AdditionalInitializers', () => { registeredInitializers: [targetInitializer, scorerInitializer], creating: false, onAdd: jest.fn().mockResolvedValue(true), - onSave: jest.fn().mockResolvedValue(undefined), + onSave: jest.fn().mockResolvedValue(true), + onClearSaveError: jest.fn(), onApply: jest.fn().mockResolvedValue(undefined), onRemove: jest.fn().mockResolvedValue(undefined), } @@ -238,11 +240,11 @@ describe('AdditionalInitializers', () => { await user.click(within(screen.getByTestId('initializer-row-additional-1')).getByRole('button', { name: 'Remove' })) - const dialog = await screen.findByRole('dialog') + const dialog = await screen.findByRole('dialog', { hidden: true }) expect(within(dialog).getByText(/remove the/i)).toBeInTheDocument() expect(within(dialog).getByText('target')).toBeInTheDocument() - await user.click(within(dialog).getByRole('button', { name: 'Remove' })) + await user.click(within(dialog).getByRole('button', { name: 'Remove', hidden: true })) expect(defaultProps.onRemove).toHaveBeenCalledWith('additional-1') }) @@ -258,8 +260,8 @@ describe('AdditionalInitializers', () => { await user.click(within(screen.getByTestId('initializer-row-additional-1')).getByRole('button', { name: 'Remove' })) - const dialog = await screen.findByRole('dialog') - await user.click(within(dialog).getByRole('button', { name: 'Cancel' })) + const dialog = await screen.findByRole('dialog', { hidden: true }) + await user.click(within(dialog).getByRole('button', { name: 'Cancel', hidden: true })) expect(defaultProps.onRemove).not.toHaveBeenCalled() }) @@ -337,6 +339,55 @@ describe('AdditionalInitializers', () => { expect(defaultProps.onAdd).toHaveBeenCalledWith('tagged_target', { tags: ['default', 'scorer'] }) }) + it('should keep the edit dialog open and show an inline error when save fails', async () => { + const user = userEvent.setup() + const onSave = jest.fn() + const onClearSaveError = jest.fn() + + function TestComponent() { + const [saveErrors, setSaveErrors] = useState>({}) + + return ( + { + onSave(id, request) + setSaveErrors({ [id]: 'Mock save failure' }) + return false + }} + onClearSaveError={(id) => { + onClearSaveError(id) + setSaveErrors({}) + }} + /> + ) + } + + render( + + + , + ) + + const row = screen.getByTestId('initializer-row-additional-1') + fireEvent.click(within(row).getByRole('button', { name: 'Edit' })) + + const dialog = await screen.findByRole('dialog', {}, { timeout: 3000 }) + await within(dialog).findByText('Edit target initializer') + const editor = within(dialog).getByTestId('param-tags') + fireEvent.change(editor, { target: { value: 'modified' } }) + await user.click(await within(dialog).findByRole('button', { name: 'Save', hidden: true })) + + expect(screen.getByRole('dialog', { hidden: true })).toBeInTheDocument() + expect(await within(dialog).findByRole('alert', { hidden: true })).toHaveTextContent('Mock save failure') + expect(editor).toHaveValue('modified') + + await user.click(within(dialog).getByRole('button', { name: 'Cancel', hidden: true })) + + expect(onClearSaveError).toHaveBeenCalledWith('additional-1') + }) + it('should hide the parameters editor and submit null for a no-parameter initializer', async () => { const user = userEvent.setup() diff --git a/frontend/src/components/Initializers/AdditionalInitializers.tsx b/frontend/src/components/Initializers/AdditionalInitializers.tsx index 6d9a71d75f..992292ca3c 100644 --- a/frontend/src/components/Initializers/AdditionalInitializers.tsx +++ b/frontend/src/components/Initializers/AdditionalInitializers.tsx @@ -27,10 +27,12 @@ interface AdditionalInitializersProps { registeredInitializers: RegisteredInitializer[] creating: boolean savingInitializerId?: string | null + saveErrors?: Record applyingInitializerId?: string | null deletingInitializerId?: string | null onAdd: (initializerName: string, parameters: Record | null) => Promise - onSave: (id: string, request: UpdateAdditionalInitializerRequest) => Promise + onSave: (id: string, request: UpdateAdditionalInitializerRequest) => Promise + onClearSaveError: (id: string) => void onApply: (id: string, initializerName: string, parameters?: Record | null) => Promise onRemove: (id: string) => Promise } @@ -41,7 +43,9 @@ interface AdditionalInitializerCardProps { isSaving: boolean isApplying: boolean isDeleting: boolean - onSave: (id: string, request: UpdateAdditionalInitializerRequest) => Promise + saveError?: string | null + onSave: (id: string, request: UpdateAdditionalInitializerRequest) => Promise + onClearSaveError: (id: string) => void onApply: (id: string, initializerName: string, parameters?: Record | null) => Promise onRemove: (id: string) => Promise } @@ -52,7 +56,9 @@ function AdditionalInitializerCard({ isSaving, isApplying, isDeleting, + saveError, onSave, + onClearSaveError, onApply, onRemove, }: AdditionalInitializerCardProps) { @@ -62,8 +68,17 @@ function AdditionalInitializerCard({ const isBusy = isSaving || isApplying || isDeleting const handleEditSubmit = async (parameters: Record | null): Promise => { - await onSave(item.id, { parameters, order_index: item.order_index ?? null }) - setEditOpen(false) + const saved = await onSave(item.id, { parameters, order_index: item.order_index ?? null }) + if (saved) { + setEditOpen(false) + } + } + + const handleEditOpenChange = (open: boolean): void => { + setEditOpen(open) + if (!open) { + onClearSaveError(item.id) + } } return ( @@ -130,8 +145,9 @@ function AdditionalInitializerCard({ initializer={initializer} initialParameters={item.parameters} submitting={isSaving} + externalError={saveError} onSubmit={handleEditSubmit} - onOpenChange={setEditOpen} + onOpenChange={handleEditOpenChange} /> )} @@ -143,10 +159,12 @@ export default function AdditionalInitializers({ registeredInitializers, creating, savingInitializerId = null, + saveErrors = {}, applyingInitializerId = null, deletingInitializerId = null, onAdd, onSave, + onClearSaveError, onApply, onRemove, }: AdditionalInitializersProps) { @@ -223,7 +241,9 @@ export default function AdditionalInitializers({ isSaving={savingInitializerId === item.id} isApplying={applyingInitializerId === item.id} isDeleting={deletingInitializerId === item.id} + saveError={saveErrors[item.id] ?? null} onSave={onSave} + onClearSaveError={onClearSaveError} onApply={onApply} onRemove={onRemove} /> diff --git a/frontend/src/components/Initializers/AvailableInitializersDialog.test.tsx b/frontend/src/components/Initializers/AvailableInitializersDialog.test.tsx index cafe49c3e2..8c741d78b6 100644 --- a/frontend/src/components/Initializers/AvailableInitializersDialog.test.tsx +++ b/frontend/src/components/Initializers/AvailableInitializersDialog.test.tsx @@ -50,7 +50,7 @@ describe('AvailableInitializersDialog', () => { await user.click(screen.getByRole('button', { name: /browse available initializers/i })) - const dialog = await screen.findByRole('dialog') + const dialog = await screen.findByRole('dialog', { hidden: true }) const refreshRow = within(dialog).getByTestId('available-initializer-row-refresh_datasets') expect(within(refreshRow).getByText('Refreshes datasets.')).toBeInTheDocument() expect(within(refreshRow).getByText(/HF_TOKEN/)).toBeInTheDocument() @@ -71,7 +71,7 @@ describe('AvailableInitializersDialog', () => { await user.click(screen.getByRole('button', { name: /browse available initializers/i })) - const dialog = await screen.findByRole('dialog') + const dialog = await screen.findByRole('dialog', { hidden: true }) expect(within(dialog).getByText('No registered initializers were found.')).toBeInTheDocument() }) }) diff --git a/frontend/src/components/Initializers/InitializerParametersDialog.test.tsx b/frontend/src/components/Initializers/InitializerParametersDialog.test.tsx index 5fa0d84c73..6b5d43ec94 100644 --- a/frontend/src/components/Initializers/InitializerParametersDialog.test.tsx +++ b/frontend/src/components/Initializers/InitializerParametersDialog.test.tsx @@ -95,7 +95,7 @@ describe('InitializerParametersDialog', () => { expect(screen.getByText('This initializer takes no parameters.')).toBeInTheDocument() - await user.click(screen.getByRole('button', { name: 'Add' })) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) expect(onSubmit).toHaveBeenCalledWith(null) }) @@ -109,7 +109,7 @@ describe('InitializerParametersDialog', () => { , ) - await user.click(screen.getByRole('button', { name: 'Add' })) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) expect(await screen.findByRole('alert')).toHaveTextContent('label is required.') expect(onSubmit).not.toHaveBeenCalled() @@ -126,7 +126,7 @@ describe('InitializerParametersDialog', () => { fireEvent.change(screen.getByTestId('param-days'), { target: { value: '7' } }) fireEvent.change(screen.getByTestId('param-names'), { target: { value: 'x, y' } }) - await user.click(screen.getByRole('button', { name: 'Add' })) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) expect(onSubmit).toHaveBeenCalledWith({ days: 7, names: ['x', 'y'] }) }) @@ -142,11 +142,26 @@ describe('InitializerParametersDialog', () => { await user.click(screen.getByTestId('param-flag')) await user.click(screen.getByTestId('param-tags-a')) - await user.click(screen.getByRole('button', { name: 'Add' })) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) expect(onSubmit).toHaveBeenCalledWith(expect.objectContaining({ flag: true, tags: ['a'] })) }) + it('toggles the intended multiselect option when clicking checkbox label text', async () => { + const user = userEvent.setup() + const onSubmit = jest.fn().mockResolvedValue(undefined) + render( + + + , + ) + + await user.click(screen.getByText('b')) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) + + expect(onSubmit).toHaveBeenCalledWith(expect.objectContaining({ tags: ['b'] })) + }) + it('unchecks a multiselect choice and picks a select value', async () => { const user = userEvent.setup() const onSubmit = jest.fn().mockResolvedValue(undefined) @@ -159,7 +174,7 @@ describe('InitializerParametersDialog', () => { await user.click(screen.getByTestId('param-tags-a')) await user.click(screen.getByTestId('param-tags-a')) fireEvent.change(screen.getByTestId('param-level'), { target: { value: 'high' } }) - await user.click(screen.getByRole('button', { name: 'Add' })) + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) expect(onSubmit).toHaveBeenCalledWith(expect.objectContaining({ level: 'high' })) expect(onSubmit.mock.calls[0][0]).not.toHaveProperty('tags') @@ -221,4 +236,23 @@ describe('InitializerParametersDialog', () => { expect(screen.getByRole('alert')).toHaveTextContent('Server rejected the request.') }) + + it('prefers validation error over externalError', async () => { + const user = userEvent.setup() + const onSubmit = jest.fn().mockResolvedValue(undefined) + render( + + + , + ) + + await user.click(screen.getByRole('button', { name: 'Add', hidden: true })) + + expect(await screen.findByRole('alert')).toHaveTextContent('label is required.') + }) }) diff --git a/frontend/src/components/Initializers/InitializerParametersDialog.tsx b/frontend/src/components/Initializers/InitializerParametersDialog.tsx index 1d2c56dd8f..8315a94fc2 100644 --- a/frontend/src/components/Initializers/InitializerParametersDialog.tsx +++ b/frontend/src/components/Initializers/InitializerParametersDialog.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react' +import { useRef, useState } from 'react' import { Button, Checkbox, @@ -52,6 +52,7 @@ export default function InitializerParametersDialog({ getInitialFormValues(parameters, initialParameters), ) const [error, setError] = useState(null) + const submitInProgressRef = useRef(false) const acceptsParameters = parameters.length > 0 @@ -61,6 +62,15 @@ export default function InitializerParametersDialog({ } const handleSubmit = async (): Promise => { + submitInProgressRef.current = true + try { + await submitForm() + } finally { + submitInProgressRef.current = false + } + } + + const submitForm = async (): Promise => { if (!acceptsParameters) { setError(null) await onSubmit(null) @@ -82,7 +92,15 @@ export default function InitializerParametersDialog({ const submitLabel = mode === 'add' ? 'Add' : 'Save' return ( - onOpenChange(data.open)}> + { + if (!data.open && submitInProgressRef.current) { + return + } + onOpenChange(data.open) + }} + > {title} @@ -173,6 +191,7 @@ function ParameterField({ parameter, value, disabled, onChange }: ParameterField {(parameter.choices ?? []).map((choice) => ( { }) }) + it('should show save errors in the edit dialog and preserve edits', async () => { + const user = userEvent.setup() + mockedInitializersApi.updateAdditional.mockRejectedValue(new Error('Mock save failure')) + renderInitializers() + + await screen.findByTestId('initializer-row-additional-1') + const dialog = await openDialogByButton(user, 'Edit', 'Edit scorer initializer') + const editor = within(dialog).getByTestId('param-tags') + fireEvent.change(editor, { target: { value: 'relaxed' } }) + await user.click(await within(dialog).findByRole('button', { name: 'Save', hidden: true })) + + expect(screen.getByRole('dialog', { hidden: true })).toBeInTheDocument() + expect(await within(dialog).findByRole('alert', { hidden: true })).toHaveTextContent('Mock save failure') + expect(editor).toHaveValue('relaxed') + }) + it('should apply an additional initializer', async () => { const user = userEvent.setup() renderInitializers() @@ -276,8 +292,8 @@ describe('Initializers', () => { const row = await screen.findByTestId('initializer-row-additional-1') await user.click(within(row).getByRole('button', { name: 'Remove' })) - const dialog = await screen.findByRole('dialog') - await user.click(within(dialog).getByRole('button', { name: 'Remove' })) + const dialog = await screen.findByRole('dialog', { hidden: true }) + await user.click(within(dialog).getByRole('button', { name: 'Remove', hidden: true })) await waitFor(() => { expect(mockedInitializersApi.deleteAdditional).toHaveBeenCalledWith('additional-1') diff --git a/frontend/src/components/Initializers/Initializers.tsx b/frontend/src/components/Initializers/Initializers.tsx index f4540e8f3e..55c7547d7f 100644 --- a/frontend/src/components/Initializers/Initializers.tsx +++ b/frontend/src/components/Initializers/Initializers.tsx @@ -31,6 +31,7 @@ export default function Initializers() { const [refetchCount, setRefetchCount] = useState(0) const [creating, setCreating] = useState(false) const [savingInitializerId, setSavingInitializerId] = useState(null) + const [saveErrors, setSaveErrors] = useState>({}) const [applyingInitializerId, setApplyingInitializerId] = useState(null) const [deletingInitializerId, setDeletingInitializerId] = useState(null) @@ -105,19 +106,36 @@ export default function Initializers() { const handleSave = async ( id: string, request: UpdateAdditionalInitializerRequest, - ): Promise => { + ): Promise => { setSavingInitializerId(id) + setSaveErrors((currentErrors) => { + const remainingErrors = { ...currentErrors } + delete remainingErrors[id] + return remainingErrors + }) try { await initializersApi.updateAdditional(id, request) setStatusMessage({ intent: 'success', text: 'Saved additional initializer.' }) await refetchSettingsOnly() + return true } catch (error) { - setStatusMessage({ intent: 'error', text: toApiError(error).detail }) + const detail = toApiError(error).detail + setStatusMessage({ intent: 'error', text: detail }) + setSaveErrors((currentErrors) => ({ ...currentErrors, [id]: detail })) + return false } finally { setSavingInitializerId(null) } } + const clearSaveError = (id: string): void => { + setSaveErrors((currentErrors) => { + const remainingErrors = { ...currentErrors } + delete remainingErrors[id] + return remainingErrors + }) + } + const handleApply = async ( id: string, initializerName: string, @@ -195,10 +213,12 @@ export default function Initializers() { registeredInitializers={registeredInitializers} creating={creating} savingInitializerId={savingInitializerId} + saveErrors={saveErrors} applyingInitializerId={applyingInitializerId} deletingInitializerId={deletingInitializerId} onAdd={handleAdd} onSave={handleSave} + onClearSaveError={clearSaveError} onApply={handleApply} onRemove={handleRemove} />