diff --git a/.changeset/one-name-when-alerting-a-new-search.md b/.changeset/one-name-when-alerting-a-new-search.md new file mode 100644 index 0000000000..534b9af516 --- /dev/null +++ b/.changeset/one-name-when-alerting-a-new-search.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +fix: Creating an alert from the search page asks for one name, not two. An unsaved search is named once in the alert modal's header and the alert inherits that name and its tags; on a saved search the alert's own name field is labelled "Alert name" and shows the inherited name as its placeholder instead of pre-filling a duplicate. diff --git a/packages/app/src/DBSearchPageAlertModal.tsx b/packages/app/src/DBSearchPageAlertModal.tsx index ab748a0312..dfef18ced0 100644 --- a/packages/app/src/DBSearchPageAlertModal.tsx +++ b/packages/app/src/DBSearchPageAlertModal.tsx @@ -1,12 +1,14 @@ import React from 'react'; import router from 'next/router'; -import { Controller, useForm, useWatch } from 'react-hook-form'; +import { + Control, + Controller, + useForm, + useFormState, + useWatch, +} from 'react-hook-form'; import { z } from 'zod'; import { zodResolver } from '@hookform/resolvers/zod'; -import { - clampAlertDisplayName, - clampAlertTags, -} from '@hyperdx/common-utils/dist/alerts'; import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; import { type Alert, @@ -38,17 +40,19 @@ import { Stack, Tabs, Text, - TextInput, } from '@mantine/core'; import { notifications } from '@mantine/notifications'; import { IconChartLine, IconInfoCircleFilled, IconPlus, + IconTags, } from '@tabler/icons-react'; import { useQueryClient } from '@tanstack/react-query'; +import { InlineNameInputControlled } from '@/components/InlineNameInput/InlineNameInput'; import { SQLInlineEditorControlled } from '@/components/SQLEditor/SQLInlineEditor'; +import { Tags } from '@/components/Tags'; import { useCreateSavedSearch } from '@/savedSearch'; import { useSavedSearch } from '@/savedSearch'; import { useSource } from '@/source'; @@ -95,6 +99,67 @@ const SavedSearchAlertFormSchema = z .superRefine(validateAlertScheduleOffsetMinutes) .superRefine(validateAlertThresholdMax); +type NewSavedSearchFormValues = { name: string }; + +/** + * Name and tags for the search an alert is being created alongside. The alert + * inherits both, so this is the only naming the flow asks for. + */ +const NewSavedSearchFields = ({ + control, + tags, + onTagsChange, +}: { + control: Control; + tags: string[]; + onTagsChange: (tags: string[]) => void; +}) => { + const { errors } = useFormState({ control, name: 'name' }); + const nameError = errors.name?.message; + + return ( + <> + + + Alerts for + + + value.trim().length > 0 || 'Name the search before saving it.', + }} + size="xs" + placeholder="Untitled search" + aria-label="Saved search name" + data-testid="saved-search-name-input" + /> + + + + + {nameError ? ( + + {nameError} + + ) : ( + + Saving creates this search; the alert takes its name and tags. + + )} + + ); +}; + const AlertForm = ({ sourceId, where, @@ -102,13 +167,13 @@ const AlertForm = ({ filters, select, defaultValues, - prefill, loading, deleteLoading, hasSavedSearch, savedSearchName, onDelete, onSubmit, + onSubmitAttempt, onClose, }: { sourceId?: string | null; @@ -117,14 +182,18 @@ const AlertForm = ({ filters?: Filter[] | null; select?: string | null; defaultValues?: null | AlertWithCreatedBy; - /** Seed values for a brand-new alert */ - prefill?: { displayName?: string; tags?: string[] }; loading?: boolean; deleteLoading?: boolean; hasSavedSearch?: boolean; savedSearchName?: string; onDelete: (id: string) => void; onSubmit: (data: Alert) => void; + /** + * Runs on every submit, before this form validates, so fields the parent + * owns can report their own errors in the same pass rather than only once + * the alert itself is valid. + */ + onSubmitAttempt?: () => void; onClose: () => void; }) => { const { data: source } = useSource({ id: sourceId }); @@ -154,8 +223,6 @@ const AlertForm = ({ source: AlertSource.SAVED_SEARCH, channels: [{ type: 'webhook', webhookId: '' }], note: null, - displayName: prefill?.displayName, - tags: prefill?.tags, }, resolver: zodResolver(SavedSearchAlertFormSchema), }); @@ -183,39 +250,48 @@ const AlertForm = ({ const { data: alertData } = api.useAlert(defaultValues?.id); const alert = alertData?.data; + const submitAlert = handleSubmit(data => + onSubmit( + normalizeNoOpAlertScheduleFields( + // `id` is not a registered form field, so carry it from the alert + // this form was opened with. This binds the submission to the + // originally-edited alert rather than whatever the parent's alerts + // list happens to index at submit time. + // `channel` is cleared (JSON drops undefined): the API derives it + // from channels[0], and echoing a stale value back would conflict + // with an edited list. + { ...data, channel: undefined, id: defaultValues?.id }, + defaultValues, + { + preserveExplicitScheduleOffsetMinutes: + dirtyFields.scheduleOffsetMinutes === true, + preserveExplicitScheduleStartAt: dirtyFields.scheduleStartAt === true, + }, + ), + ), + ); + return (
- onSubmit( - normalizeNoOpAlertScheduleFields( - // `id` is not a registered form field, so carry it from the alert - // this form was opened with. This binds the submission to the - // originally-edited alert rather than whatever the parent's alerts - // list happens to index at submit time. - // `channel` is cleared (JSON drops undefined): the API derives it - // from channels[0], and echoing a stale value back would conflict - // with an edited list. - { ...data, channel: undefined, id: defaultValues?.id }, - defaultValues, - { - preserveExplicitScheduleOffsetMinutes: - dirtyFields.scheduleOffsetMinutes === true, - preserveExplicitScheduleStartAt: - dirtyFields.scheduleStartAt === true, - }, - ), - ), - )} + onSubmit={e => { + onSubmitAttempt?.(); + return submitAlert(e); + }} > - + {/* The alert inherits the name and tags of the search it watches, so + there is nothing to override until that search exists. */} + {hasSavedSearch && ( + + )} Trigger @@ -422,7 +498,7 @@ const AlertForm = ({ onClick={() => onDelete(defaultValues.id!)} loading={deleteLoading} > - Delete Alert + Delete alert )} @@ -432,10 +508,10 @@ const AlertForm = ({ @@ -480,22 +556,22 @@ export const DBSearchPageAlertModal = ({ } }; - const [name, setName] = React.useState(''); + const newSavedSearchForm = useForm({ + defaultValues: { name: '' }, + }); + const [newSavedSearchTags, setNewSavedSearchTags] = React.useState( + [], + ); const onSubmit = async (data: Alert) => { try { // Create new search along with alert if (!id && searchedConfig) { - if (!name) { - notifications.show({ - color: 'red', - message: 'Please provide a name for the saved search.', - autoClose: 5000, - }); + if (!(await newSavedSearchForm.trigger())) { return; } const result = await createSavedSearch.mutateAsync({ - name, + name: newSavedSearchForm.getValues('name').trim(), select: searchedConfig.select ?? '', where: searchedConfig.where ?? '', whereLanguage: @@ -503,7 +579,7 @@ export const DBSearchPageAlertModal = ({ source: searchedConfig.source ?? '', orderBy: searchedConfig.orderBy ?? '', filters: searchedConfig.filters ?? [], - tags: [], + tags: newSavedSearchTags, }); await createAlert.mutateAsync({ ...data, @@ -590,56 +666,49 @@ export const DBSearchPageAlertModal = ({ }} loaderProps={{ type: 'dots' }} /> - - + + {id ? ( Alerts for {savedSearch?.name} - {!id && ( - setName(e.currentTarget.value)} - required - /> - )} - - {savedSearch?.where} + ) : ( + + )} + {savedSearch?.where ?? searchedConfig?.where} - - - {(savedSearch?.alerts || []).map((alert, index) => ( - - - {getWebhookChannelIcon(alert.channel?.type)} - Alert {index + 1} - + {/* An unsaved search has no alerts to switch between, and the one tab + left reads as a control that does something. */} + {id && ( + + + {(savedSearch?.alerts || []).map((alert, index) => ( + + + {getWebhookChannelIcon(alert.channel?.type)} + Alert {index + 1} + + + + ))} + + + + New alert - ))} - - - - New Alert - - - - + + + )} void newSavedSearchForm.trigger() + } onDelete={onDelete} deleteLoading={deleteAlert.isPending} onClose={onClose} diff --git a/packages/app/src/__tests__/DBSearchPageAlertModal.test.tsx b/packages/app/src/__tests__/DBSearchPageAlertModal.test.tsx index 081c75a494..48071d0270 100644 --- a/packages/app/src/__tests__/DBSearchPageAlertModal.test.tsx +++ b/packages/app/src/__tests__/DBSearchPageAlertModal.test.tsx @@ -14,6 +14,10 @@ import { DBSearchPageAlertModal } from '@/DBSearchPageAlertModal'; const createAlertMutateAsync = jest.fn().mockResolvedValue({ data: {} }); const updateAlertMutateAsync = jest.fn().mockResolvedValue({ data: {} }); const deleteAlertMutateAsync = jest.fn().mockResolvedValue(undefined); +const createSavedSearchMutateAsync = jest + .fn() + .mockResolvedValue({ id: 'new-saved-search-id' }); +const routerPush = jest.fn(); // Two alerts so the test can edit the second tab and confirm the PUT targets // the right id. @@ -43,8 +47,8 @@ const savedSearch = { select: '', source: 'source-id', orderBy: '', - // One tag over the alert cap (32): the prefill must clamp it or the form - // rejects on mount and Create Alert silently does nothing. + // One tag over the alert cap (32). The form must not copy a saved search's + // tags into the alert, which its own schema would then reject on submit. tags: ['prod', 'checkout-service-payment-failures'], alerts: [firstAlert, secondAlert], }; @@ -78,10 +82,19 @@ jest.mock('@/api', () => ({ }, })); +jest.mock('next/router', () => ({ + __esModule: true, + default: { push: (...args: unknown[]) => routerPush(...args) }, +})); + jest.mock('@/savedSearch', () => ({ __esModule: true, - useSavedSearch: () => ({ data: savedSearch, isLoading: false }), - useCreateSavedSearch: () => ({ mutateAsync: jest.fn() }), + // Mirrors the hook's `enabled` option: no id, no saved search. + useSavedSearch: ({ id }: { id: string }) => ({ + data: id ? savedSearch : undefined, + isLoading: false, + }), + useCreateSavedSearch: () => ({ mutateAsync: createSavedSearchMutateAsync }), })); jest.mock('@/source', () => ({ @@ -138,35 +151,53 @@ jest.mock('@/components/SQLEditor/SQLInlineEditor', () => ({ SQLInlineEditorControlled: () =>
, })); -const renderModal = () => { +const renderModal = ( + props: Partial> = {}, +) => { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, }); return renderWithMantine( - + , ); }; +const searchedConfig = { + source: 'source-id', + select: '', + where: 'level:error', + whereLanguage: 'lucene' as const, + orderBy: '', + filters: [], +}; + describe('DBSearchPageAlertModal', () => { beforeEach(() => { createAlertMutateAsync.mockClear(); updateAlertMutateAsync.mockClear(); deleteAlertMutateAsync.mockClear(); + createSavedSearchMutateAsync.mockClear(); + routerPush.mockClear(); }); it('dispatches an update (PUT) with the id resolved from the selected alert tab', async () => { renderModal(); - // Modal opens on the "New Alert" tab; select the second existing alert so + // Modal opens on the "New alert" tab; select the second existing alert so // it renders in edit mode. Using the second (not first) tab confirms the // update id is resolved by tab index. const alertTab = await screen.findByRole('tab', { name: /Alert 2/ }); fireEvent.click(alertTab); const saveButton = await screen.findByRole('button', { - name: 'Save Alert', + name: 'Save alert', }); fireEvent.click(saveButton); @@ -195,7 +226,7 @@ describe('DBSearchPageAlertModal', () => { fireEvent.click(alertTab); const saveButton = await screen.findByRole('button', { - name: 'Save Alert', + name: 'Save alert', }); fireEvent.click(saveButton); @@ -210,31 +241,83 @@ describe('DBSearchPageAlertModal', () => { ]); }); - // A new alert opens prefilled from the saved search it will watch, so the - // create payload carries those values rather than relying on the server's - // derivation. - it('prefills the display name and tags from the saved search, clamped to the alert caps', async () => { + // The saved search is already named in the modal's header, so the alert's + // own name field stays empty and shows what it inherits. Submitting without + // touching it leaves the name and tags for the server to derive. + it('leaves a new alert to inherit the name and tags of its saved search', async () => { renderModal(); - expect(await screen.findByTestId('alert-display-name-input')).toHaveValue( - 'My Search', + const nameInput = await screen.findByTestId('alert-display-name-input'); + expect(nameInput).toHaveValue(''); + expect(nameInput).toHaveAttribute('placeholder', 'My Search'); + + fireEvent.change(screen.getByTestId('webhook-id-input'), { + target: { value: 'webhook-id' }, + }); + fireEvent.click( + await screen.findByRole('button', { name: 'Create alert' }), + ); + + await waitFor(() => { + expect(createAlertMutateAsync).toHaveBeenCalledTimes(1); + }); + const payload = createAlertMutateAsync.mock.calls[0][0]; + expect(payload.displayName).toBeUndefined(); + expect(payload.tags).toBeUndefined(); + }); + + // An unsaved search is named once, in the header, and the alert inherits it: + // there is no second name field to disambiguate. + it('asks for one name when the search has not been saved yet', async () => { + renderModal({ id: undefined, searchedConfig }); + + expect(await screen.findByTestId('saved-search-name-input')).toHaveValue( + '', ); + expect( + screen.queryByTestId('alert-display-name-input'), + ).not.toBeInTheDocument(); + fireEvent.change(screen.getByTestId('saved-search-name-input'), { + target: { value: ' Checkout errors ' }, + }); fireEvent.change(screen.getByTestId('webhook-id-input'), { target: { value: 'webhook-id' }, }); fireEvent.click( - await screen.findByRole('button', { name: 'Create Alert' }), + await screen.findByRole('button', { name: 'Save search and alert' }), + ); + + await waitFor(() => { + expect(createSavedSearchMutateAsync).toHaveBeenCalledTimes(1); + }); + expect(createSavedSearchMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Checkout errors', tags: [] }), ); await waitFor(() => { expect(createAlertMutateAsync).toHaveBeenCalledTimes(1); }); - expect(createAlertMutateAsync).toHaveBeenCalledWith( - expect.objectContaining({ - displayName: 'My Search', - tags: ['prod', 'checkout-service-payment-failure'], - }), + const payload = createAlertMutateAsync.mock.calls[0][0]; + expect(payload.savedSearchId).toBe('new-saved-search-id'); + expect(payload.displayName).toBeUndefined(); + expect(payload.tags).toBeUndefined(); + }); + + // The name is validated on the same submit as the alert fields, so an + // unnamed search is reported the first time rather than after the rest of + // the form is fixed. + it('flags an unnamed search on the first submit and saves nothing', async () => { + renderModal({ id: undefined, searchedConfig }); + + fireEvent.click( + await screen.findByRole('button', { name: 'Save search and alert' }), ); + + expect( + await screen.findByTestId('saved-search-name-error'), + ).toBeInTheDocument(); + expect(createSavedSearchMutateAsync).not.toHaveBeenCalled(); + expect(createAlertMutateAsync).not.toHaveBeenCalled(); }); }); diff --git a/packages/app/src/components/AlertDisplayFields.tsx b/packages/app/src/components/AlertDisplayFields.tsx index a91248669c..2305264768 100644 --- a/packages/app/src/components/AlertDisplayFields.tsx +++ b/packages/app/src/components/AlertDisplayFields.tsx @@ -32,6 +32,7 @@ export function AlertDisplayFields({ displayNameName, tagsName, derivedDisplayName, + displayNameLabel = 'Name', displayNameRequired = false, tagsInherit = true, labelMarginTop = 'xs', @@ -41,6 +42,12 @@ export function AlertDisplayFields({ tagsName: Path; /** Name the server will derive when the field is left empty. */ derivedDisplayName?: string | null; + /** + * Label for the name field. Override where the surface already shows the + * name of what the alert watches, and a bare "Name" would read as that + * object's name rather than the alert's. + */ + displayNameLabel?: string; /** * Whether the alert must be named here. Set for alerts with no parent to * inherit a name from, where leaving it blank names the alert after @@ -59,7 +66,7 @@ export function AlertDisplayFields({
- Name + {displayNameLabel} {displayNameRequired && (