diff --git a/.changeset/mighty-gorillas-ring.md b/.changeset/mighty-gorillas-ring.md new file mode 100644 index 00000000..be2cbbb4 --- /dev/null +++ b/.changeset/mighty-gorillas-ring.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/app": patch +--- + +feat: Improve chart editor validations diff --git a/packages/app/src/components/ChartEditor/__tests__/utils.test.ts b/packages/app/src/components/ChartEditor/__tests__/utils.test.ts index 5b5efbf8..57079d06 100644 --- a/packages/app/src/components/ChartEditor/__tests__/utils.test.ts +++ b/packages/app/src/components/ChartEditor/__tests__/utils.test.ts @@ -15,7 +15,7 @@ import { convertFormStateToChartConfig, convertFormStateToSavedChartConfig, convertSavedChartConfigToFormState, - validateMetricNames, + validateChartForm, } from '../utils'; jest.mock('../../SearchInput', () => ({ @@ -424,150 +424,543 @@ describe('convertSavedChartConfigToFormState', () => { }); }); -describe('validateMetricNames', () => { +describe('validateChartForm', () => { const metricSeriesItem = { ...seriesItem, metricType: MetricsDataType.Gauge, metricName: 'cpu.usage', }; - it('returns false when tableSource is undefined', () => { - const setError = jest.fn(); - const result = validateMetricNames( - undefined, - [metricSeriesItem], - DisplayType.Line, - setError, - ); - expect(result).toBe(false); - expect(setError).not.toHaveBeenCalled(); + const makeForm = ( + overrides: Partial, + ): ChartEditorFormState => ({ + displayType: DisplayType.Line, + series: [seriesItem], + ...overrides, }); - it('returns false when tableSource is not a metric source', () => { + // ── Valid forms (no errors) ────────────────────────────────────────── + + it('returns no errors for a valid builder chart with a source', () => { const setError = jest.fn(); - const result = validateMetricNames( + const errors = validateChartForm( + makeForm({ source: 'source-log' }), logSource, - [metricSeriesItem], - DisplayType.Line, setError, ); - expect(result).toBe(false); + expect(errors).toHaveLength(0); expect(setError).not.toHaveBeenCalled(); }); - it('returns false when series is undefined', () => { + it('returns no errors for a valid raw SQL chart', () => { const setError = jest.fn(); - const result = validateMetricNames( - metricSource, + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Table, + sqlTemplate: 'SELECT 1', + connection: 'conn-1', + }), undefined, - DisplayType.Line, setError, ); - expect(result).toBe(false); + expect(errors).toHaveLength(0); expect(setError).not.toHaveBeenCalled(); }); - it('returns false when displayType is Markdown', () => { + it('returns no errors for a Markdown chart without source', () => { const setError = jest.fn(); - const seriesWithoutName = { - ...seriesItem, - metricType: MetricsDataType.Gauge, - }; - const result = validateMetricNames( - metricSource, - [seriesWithoutName], - DisplayType.Markdown, + const errors = validateChartForm( + makeForm({ displayType: DisplayType.Markdown }), + undefined, setError, ); - expect(result).toBe(false); + expect(errors).toHaveLength(0); expect(setError).not.toHaveBeenCalled(); }); - it('returns false when all series items with metricType have a metricName', () => { + it('returns no errors for a Number chart with exactly one series', () => { const setError = jest.fn(); - const result = validateMetricNames( - metricSource, - [metricSeriesItem], - DisplayType.Line, + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Number, + source: 'source-log', + series: [seriesItem], + }), + logSource, setError, ); - expect(result).toBe(false); + expect(errors).toHaveLength(0); expect(setError).not.toHaveBeenCalled(); }); - it('returns false when series items have no metricType', () => { + it('returns no errors for a Pie chart with exactly one series', () => { const setError = jest.fn(); - const result = validateMetricNames( - metricSource, - [seriesItem], - DisplayType.Line, + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Pie, + source: 'source-log', + series: [seriesItem], + }), + logSource, setError, ); - expect(result).toBe(false); + expect(errors).toHaveLength(0); expect(setError).not.toHaveBeenCalled(); }); - it('returns true and calls setError when a series item has metricType but no metricName', () => { + it('returns no errors for a Search chart (skips series validation)', () => { const setError = jest.fn(); - const seriesWithoutName = { - ...seriesItem, - metricType: MetricsDataType.Gauge, - }; - const result = validateMetricNames( - metricSource, - [seriesWithoutName], - DisplayType.Line, + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Search, + source: 'source-log', + series: [], + }), + logSource, setError, ); - expect(result).toBe(true); - expect(setError).toHaveBeenCalledTimes(1); - expect(setError).toHaveBeenCalledWith('series.0.metricName', { + expect(errors).toHaveLength(0); + expect(setError).not.toHaveBeenCalled(); + }); + + it('returns no errors for count aggFn without valueExpression', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + source: 'source-log', + series: [{ ...seriesItem, aggFn: 'count', valueExpression: '' }], + }), + logSource, + setError, + ); + expect(errors).toHaveLength(0); + expect(setError).not.toHaveBeenCalled(); + }); + + it('returns no errors for metric series where all items have metricName', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ source: 'source-metric', series: [metricSeriesItem] }), + metricSource, + setError, + ); + expect(errors).toHaveLength(0); + expect(setError).not.toHaveBeenCalled(); + }); + + // ── Raw SQL validation ─────────────────────────────────────────────── + + it('errors when raw SQL chart is missing connection', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Table, + sqlTemplate: 'SELECT 1', + connection: '', + }), + undefined, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'connection' }), + ); + expect(setError).toHaveBeenCalledWith('connection', { type: 'manual', - message: 'Please select a metric name', + message: 'Connection is required', }); }); - it('calls setError for each series item missing metricName', () => { + it('errors when raw SQL chart is missing sqlTemplate', () => { const setError = jest.fn(); - const seriesWithoutName = { - ...seriesItem, - metricType: MetricsDataType.Gauge, - }; - const result = validateMetricNames( - metricSource, - [seriesWithoutName, seriesWithoutName], - DisplayType.Line, + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Line, + sqlTemplate: '', + connection: 'conn-1', + }), + undefined, setError, ); - expect(result).toBe(true); - expect(setError).toHaveBeenCalledTimes(2); - expect(setError).toHaveBeenCalledWith( - 'series.0.metricName', - expect.any(Object), + expect(errors).toContainEqual( + expect.objectContaining({ path: 'sqlTemplate' }), ); - expect(setError).toHaveBeenCalledWith( - 'series.1.metricName', - expect.any(Object), + expect(setError).toHaveBeenCalledWith('sqlTemplate', { + type: 'manual', + message: 'SQL query is required', + }); + }); + + it('does not check connection/sqlTemplate for non-sql configType', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + configType: 'builder', + displayType: DisplayType.Line, + source: 'source-log', + }), + logSource, + setError, + ); + expect( + errors.filter(e => e.path === 'connection' || e.path === 'sqlTemplate'), + ).toHaveLength(0); + }); + + // ── Source validation ──────────────────────────────────────────────── + + it('errors when builder chart has no source', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ source: undefined }), + logSource, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ + path: 'source', + message: 'Source is required', + }), ); }); - it('only errors on series items that are missing metricName', () => { + it('does not require source for Markdown charts', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ displayType: DisplayType.Markdown, source: undefined }), + undefined, + setError, + ); + expect(errors.filter(e => e.path === 'source')).toHaveLength(0); + }); + + it('does not require source for raw SQL charts', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Table, + sqlTemplate: 'SELECT 1', + connection: 'conn-1', + source: undefined, + }), + undefined, + setError, + ); + expect(errors.filter(e => e.path === 'source')).toHaveLength(0); + }); + + // ── Series valueExpression validation ──────────────────────────────── + + it('errors when a non-count aggFn series is missing valueExpression', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + source: 'source-log', + series: [{ ...seriesItem, aggFn: 'sum', valueExpression: '' }], + }), + logSource, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ + path: 'series.0.valueExpression', + message: 'Expression is required for series 1', + }), + ); + }); + + it('errors for each series missing valueExpression with non-count aggFn', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + source: 'source-log', + series: [ + { ...seriesItem, aggFn: 'avg', valueExpression: '' }, + { ...seriesItem, aggFn: 'max', valueExpression: '' }, + ], + }), + logSource, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.0.valueExpression' }), + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.1.valueExpression' }), + ); + }); + + it('does not error for count aggFn even without valueExpression', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + source: 'source-log', + series: [{ ...seriesItem, aggFn: 'count', valueExpression: '' }], + }), + logSource, + setError, + ); + expect( + errors.filter(e => e.path === 'series.0.valueExpression'), + ).toHaveLength(0); + }); + + it('does not validate valueExpression for Search displayType', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Search, + source: 'source-log', + series: [{ ...seriesItem, aggFn: 'sum', valueExpression: '' }], + }), + logSource, + setError, + ); + expect( + errors.filter( + e => typeof e.path === 'string' && e.path.includes('valueExpression'), + ), + ).toHaveLength(0); + }); + + it('does not validate valueExpression for Markdown displayType', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Markdown, + source: 'source-log', + series: [{ ...seriesItem, aggFn: 'sum', valueExpression: '' }], + }), + logSource, + setError, + ); + expect( + errors.filter( + e => typeof e.path === 'string' && e.path.includes('valueExpression'), + ), + ).toHaveLength(0); + }); + + // ── Metric name validation ─────────────────────────────────────────── + + it('errors for each metric series item missing metricName', () => { const setError = jest.fn(); const seriesWithoutName = { ...seriesItem, metricType: MetricsDataType.Gauge, }; - const result = validateMetricNames( + const errors = validateChartForm( + makeForm({ + source: 'source-metric', + series: [seriesWithoutName, seriesWithoutName], + }), metricSource, - [metricSeriesItem, seriesWithoutName], - DisplayType.Line, setError, ); - expect(result).toBe(true); - expect(setError).toHaveBeenCalledTimes(1); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.0.metricName' }), + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.1.metricName' }), + ); + }); + + it('only errors on metric series items that are missing metricName', () => { + const setError = jest.fn(); + const seriesWithoutName = { + ...seriesItem, + metricType: MetricsDataType.Gauge, + }; + const errors = validateChartForm( + makeForm({ + source: 'source-metric', + series: [metricSeriesItem, seriesWithoutName], + }), + metricSource, + setError, + ); + expect(errors.filter(e => e.path === 'series.0.metricName')).toHaveLength( + 0, + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.1.metricName' }), + ); + }); + + it('skips metric validation for non-metric sources', () => { + const setError = jest.fn(); + const seriesWithoutName = { + ...seriesItem, + metricType: MetricsDataType.Gauge, + }; + const errors = validateChartForm( + makeForm({ source: 'source-log', series: [seriesWithoutName] }), + logSource, + setError, + ); + expect( + errors.filter( + e => typeof e.path === 'string' && e.path.includes('metricName'), + ), + ).toHaveLength(0); + }); + + it('skips metric validation when source is undefined', () => { + const setError = jest.fn(); + const seriesWithoutName = { + ...seriesItem, + metricType: MetricsDataType.Gauge, + }; + const errors = validateChartForm( + makeForm({ series: [seriesWithoutName] }), + undefined, + setError, + ); + expect( + errors.filter( + e => typeof e.path === 'string' && e.path.includes('metricName'), + ), + ).toHaveLength(0); + }); + + // ── Number / Pie single-series validation ──────────────────────────── + + it('errors when Number chart has more than one series', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Number, + source: 'source-log', + series: [seriesItem, seriesItem], + }), + logSource, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ + path: 'series', + message: `Only one series is allowed for ${DisplayType.Number} charts`, + }), + ); + }); + + it('errors when Pie chart has more than one series', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Pie, + source: 'source-log', + series: [seriesItem, seriesItem], + }), + logSource, + setError, + ); + expect(errors).toContainEqual( + expect.objectContaining({ + path: 'series', + message: `Only one series is allowed for ${DisplayType.Pie} charts`, + }), + ); + }); + + it('does not error for Line chart with multiple series', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Line, + source: 'source-log', + series: [seriesItem, seriesItem], + }), + logSource, + setError, + ); + expect(errors.filter(e => e.path === 'series')).toHaveLength(0); + }); + + it('does not error for Table chart with multiple series', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Table, + source: 'source-log', + series: [seriesItem, seriesItem], + }), + logSource, + setError, + ); + expect(errors.filter(e => e.path === 'series')).toHaveLength(0); + }); + + it('does not apply single-series limit for raw SQL Number charts', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Number, + sqlTemplate: 'SELECT 1', + connection: 'conn-1', + series: [seriesItem, seriesItem], + }), + undefined, + setError, + ); + expect(errors.filter(e => e.path === 'series')).toHaveLength(0); + }); + + // ── Multiple validation errors at once ─────────────────────────────── + + it('accumulates multiple errors across different validation rules', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + displayType: DisplayType.Number, + source: undefined, + series: [ + { ...seriesItem, aggFn: 'sum', valueExpression: '' }, + { ...seriesItem, aggFn: 'avg', valueExpression: '' }, + ], + }), + logSource, + setError, + ); + // Should have: source error + 2 valueExpression errors + series count error + expect(errors).toContainEqual(expect.objectContaining({ path: 'source' })); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.0.valueExpression' }), + ); + expect(errors).toContainEqual( + expect.objectContaining({ path: 'series.1.valueExpression' }), + ); + expect(errors).toContainEqual(expect.objectContaining({ path: 'series' })); + expect(errors).toHaveLength(4); + }); + + it('calls setError for every accumulated error', () => { + const setError = jest.fn(); + const errors = validateChartForm( + makeForm({ + configType: 'sql', + displayType: DisplayType.Table, + connection: '', + sqlTemplate: '', + }), + undefined, + setError, + ); + // connection (required type) + sqlTemplate (required type) setError calls, + // then both are also called again with 'manual' type in the final loop + expect(errors).toHaveLength(2); + // Each error in the array triggers setError in the final loop expect(setError).toHaveBeenCalledWith( - 'series.1.metricName', - expect.any(Object), + 'connection', + expect.objectContaining({ type: 'manual' }), + ); + expect(setError).toHaveBeenCalledWith( + 'sqlTemplate', + expect.objectContaining({ type: 'manual' }), ); }); }); diff --git a/packages/app/src/components/ChartEditor/utils.ts b/packages/app/src/components/ChartEditor/utils.ts index 9cdab669..6d9c0831 100644 --- a/packages/app/src/components/ChartEditor/utils.ts +++ b/packages/app/src/components/ChartEditor/utils.ts @@ -1,5 +1,5 @@ import { omit, pick } from 'lodash'; -import { FieldPath } from 'react-hook-form'; +import { Path, UseFormSetError } from 'react-hook-form'; import { isBuilderSavedChartConfig, isRawSqlSavedChartConfig, @@ -17,7 +17,7 @@ import { import { getStoredLanguage } from '../SearchInput'; -import { ChartEditorFormState, SavedChartConfigWithSelectArray } from './types'; +import { ChartEditorFormState } from './types'; function normalizeChartConfig< C extends Pick< @@ -162,41 +162,91 @@ export function convertSavedChartConfigToFormState( }; } -// Helper function to safely construct field paths for series -export const getSeriesFieldPath = ( - namePrefix: string, - fieldName: string, -): FieldPath => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - return `${namePrefix}${fieldName}` as FieldPath; -}; +export const validateChartForm = ( + form: ChartEditorFormState, + source: TSource | undefined, + setError: UseFormSetError, +) => { + const errors: { path: Path; message: string }[] = []; -// Helper function to validate metric names for metric sources -export const validateMetricNames = ( - tableSource: TSource | undefined, - series: SavedChartConfigWithSelectArray['select'] | undefined, - displayType: DisplayType | undefined, - setError: ( - name: FieldPath, - error: { type: string; message: string }, - ) => void, -): boolean => { + const isRawSqlChart = + form.configType === 'sql' && isRawSqlDisplayType(form.displayType); + + // Validate connection is selected for raw SQL charts + if (isRawSqlChart && !form.connection) { + errors.push({ path: `connection`, message: 'Connection is required' }); + } + + // Validate SQL is provided for raw SQL charts + if (isRawSqlChart && !form.sqlTemplate) { + errors.push({ path: `sqlTemplate`, message: 'SQL query is required' }); + } + + // Validate source is selected for builder charts if ( - tableSource?.kind === SourceKind.Metric && - Array.isArray(series) && - displayType !== DisplayType.Markdown + !isRawSqlChart && + form.displayType !== DisplayType.Markdown && + !form.source ) { - let hasValidationError = false; - series.forEach((s, index) => { - if (s.metricType && !s.metricName) { - setError(getSeriesFieldPath(`series.${index}.`, 'metricName'), { - type: 'manual', - message: 'Please select a metric name', + errors.push({ path: `source`, message: 'Source is required' }); + } + + // Validate that valueExpressions are specified for each series + if ( + !isRawSqlChart && + Array.isArray(form.series) && + form.displayType !== DisplayType.Markdown && + form.displayType !== DisplayType.Search + ) { + form.series.forEach((s, index) => { + if (s.aggFn && s.aggFn !== 'count' && !s.valueExpression) { + errors.push({ + path: `series.${index}.valueExpression`, + message: `Expression is required for series ${index + 1}`, }); - hasValidationError = true; } }); - return hasValidationError; } - return false; + + // Validate metric names for metric sources + if ( + source?.kind === SourceKind.Metric && + Array.isArray(form.series) && + form.displayType !== DisplayType.Markdown && + form.displayType !== DisplayType.Search && + !isRawSqlChart + ) { + form.series.forEach((s, index) => { + if (s.metricType && !s.metricName) { + errors.push({ + path: `series.${index}.metricName`, + message: `Metric is required`, + }); + } + }); + } + + // Validate number and pie charts only have one series + if ( + !isRawSqlChart && + Array.isArray(form.series) && + (form.displayType === DisplayType.Number || + form.displayType === DisplayType.Pie) && + form.series.length > 1 + ) { + errors.push({ + path: `series`, + message: `Only one series is allowed for ${form.displayType} charts`, + }); + } + + for (const error of errors) { + console.warn(`Validation error in field ${error.path}: ${error.message}`); + setError(error.path, { + type: 'manual', + message: error.message, + }); + } + + return errors; }; diff --git a/packages/app/src/components/ChartSQLPreview.tsx b/packages/app/src/components/ChartSQLPreview.tsx index 0a4d3495..31f3243c 100644 --- a/packages/app/src/components/ChartSQLPreview.tsx +++ b/packages/app/src/components/ChartSQLPreview.tsx @@ -3,7 +3,7 @@ import CopyToClipboard from 'react-copy-to-clipboard'; import { sql } from '@codemirror/lang-sql'; import { format } from '@hyperdx/common-utils/dist/sqlFormatter'; import { ChartConfigWithOptDateRange } from '@hyperdx/common-utils/dist/types'; -import { Button, Paper } from '@mantine/core'; +import { Button, Paper, Text } from '@mantine/core'; import { IconCheck, IconCopy } from '@tabler/icons-react'; import CodeMirror, { EditorView } from '@uiw/react-codemirror'; @@ -96,7 +96,7 @@ export default function ChartSQLPreview({ config: ChartConfigWithOptDateRange; enableCopy?: boolean; }) { - const { data } = useRenderedSqlChartConfig(config); + const { data, error, isLoading } = useRenderedSqlChartConfig(config); return ( - + {isLoading ? ( + + Loading query preview... + + ) : error ? ( + + Unable to format query. {error.message} + + ) : ( + + )} ); } diff --git a/packages/app/src/components/DBEditTimeChartForm.tsx b/packages/app/src/components/DBEditTimeChartForm.tsx index 14412a6f..283e55fc 100644 --- a/packages/app/src/components/DBEditTimeChartForm.tsx +++ b/packages/app/src/components/DBEditTimeChartForm.tsx @@ -3,6 +3,7 @@ import { Control, Controller, FieldErrors, + Path, useFieldArray, useForm, UseFormClearErrors, @@ -41,6 +42,7 @@ import { Divider, Flex, Group, + List, Menu, Paper, SegmentedControl, @@ -51,6 +53,7 @@ import { Textarea, } from '@mantine/core'; import { useDisclosure } from '@mantine/hooks'; +import { notifications } from '@mantine/notifications'; import { IconArrowDown, IconArrowUp, @@ -67,6 +70,7 @@ import { IconPlayerPlay, IconTable, IconTrash, + IconX, } from '@tabler/icons-react'; import { SortingState } from '@tanstack/react-table'; @@ -124,9 +128,8 @@ import { convertFormStateToChartConfig, convertFormStateToSavedChartConfig, convertSavedChartConfigToFormState, - getSeriesFieldPath, isRawSqlDisplayType, - validateMetricNames, + validateChartForm, } from './ChartEditor/utils'; import { ErrorBoundary } from './Error/ErrorBoundary'; import MVOptimizationIndicator from './MaterializedViews/MVOptimizationIndicator'; @@ -188,17 +191,17 @@ function ChartSeriesEditorComponent({ errors, clearErrors, }: { - control: Control; + control: Control; databaseName: string; dateRange?: DateRange['dateRange']; connectionId?: string; index: number; - namePrefix: string; + namePrefix: `series.${number}.`; parentRef?: HTMLElement | null; onRemoveSeries: (index: number) => void; onSwapSeries: (from: number, to: number) => void; onSubmit: () => void; - setValue: UseFormSetValue; + setValue: UseFormSetValue; showGroupBy: boolean; showHaving: boolean; tableName: string; @@ -375,9 +378,7 @@ function ChartSeriesEditorComponent({ metricSource={tableSource} data-testid="metric-name-selector" error={errors?.metricName?.message} - onFocus={() => - clearErrors(getSeriesFieldPath(namePrefix, 'metricName')) - } + onFocus={() => clearErrors(`${namePrefix}metricName`)} /> {metricType === 'gauge' && ( @@ -481,7 +482,7 @@ function ChartSeriesEditorComponent({ )} - {tableSource?.kind === SourceKind.Metric && metricName && ( + {tableSource?.kind === SourceKind.Metric && metricName && metricType && ( ; message: string }[]; +}) => { + return ( + } + > + {errors.map(({ message }, index) => ( + {message} + ))} + + ); +}; + const zSavedChartConfig = z .object({ // TODO: Chart @@ -718,15 +736,14 @@ export default function EditTimeChartForm({ const isRawSqlChart = form.configType === 'sql' && isRawSqlDisplayType(form.displayType); - if ( - !isRawSqlChart && - validateMetricNames( - tableSource, - form.series, - form.displayType, - setError, - ) - ) { + const errors = validateChartForm(form, tableSource, setError); + if (errors.length > 0) { + notifications.show({ + id: 'chart-error', + title: 'Invalid Chart', + message: , + color: 'red', + }); return; } @@ -799,19 +816,14 @@ export default function EditTimeChartForm({ const handleSave = useCallback( (form: ChartEditorFormState) => { - const isRawSqlChart = - form.configType === 'sql' && isRawSqlDisplayType(form.displayType); - - // Validate metric sources have metric names selected - if ( - !isRawSqlChart && - validateMetricNames( - tableSource, - form.series, - form.displayType, - setError, - ) - ) { + const errors = validateChartForm(form, tableSource, setError); + if (errors.length > 0) { + notifications.show({ + id: 'chart-error', + title: 'Invalid Chart', + message: , + color: 'red', + }); return; } @@ -1722,7 +1734,7 @@ function seriesToFilters(select: SelectList): Filter[] { return null; } }) - .filter(Boolean) as Filter[]; + .filter(f => f != null); return filters; } diff --git a/packages/app/src/components/__tests__/DBEditTimeChartForm.test.tsx b/packages/app/src/components/__tests__/DBEditTimeChartForm.test.tsx index 53834ad9..cbe9ded3 100644 --- a/packages/app/src/components/__tests__/DBEditTimeChartForm.test.tsx +++ b/packages/app/src/components/__tests__/DBEditTimeChartForm.test.tsx @@ -195,7 +195,7 @@ describe('DBEditTimeChartForm - Metric Name Validation', () => { await waitFor(() => { const errorMessage = screen.getByTestId('metric-name-error'); expect(errorMessage).toBeInTheDocument(); - expect(errorMessage).toHaveTextContent('Please select a metric name'); + expect(errorMessage).toHaveTextContent('Metric is required'); }); // Verify that the metric name select has aria-invalid attribute @@ -371,7 +371,7 @@ describe('DBEditTimeChartForm - Metric Name Validation', () => { await waitFor(() => { const errorMessage = screen.getByTestId('metric-name-error'); expect(errorMessage).toBeInTheDocument(); - expect(errorMessage).toHaveTextContent('Please select a metric name'); + expect(errorMessage).toHaveTextContent('Metric is required'); }); }); }); @@ -405,7 +405,7 @@ describe('DBEditTimeChartForm - Save Button Metric Name Validation', () => { await waitFor(() => { const errorMessage = screen.getByTestId('metric-name-error'); expect(errorMessage).toBeInTheDocument(); - expect(errorMessage).toHaveTextContent('Please select a metric name'); + expect(errorMessage).toHaveTextContent('Metric is required'); }); // Verify that onSave was not called diff --git a/packages/app/src/hooks/useFetchMetricMetadata.tsx b/packages/app/src/hooks/useFetchMetricMetadata.tsx index 2b9149c6..73d621db 100644 --- a/packages/app/src/hooks/useFetchMetricMetadata.tsx +++ b/packages/app/src/hooks/useFetchMetricMetadata.tsx @@ -16,8 +16,8 @@ export interface MetricMetadata { interface MetricMetadataProps { databaseName: string; - metricType: string; - metricName: string; + metricType?: string; + metricName?: string; tableSource: TSource | undefined; } @@ -48,7 +48,7 @@ export const useFetchMetricMetadata = ({ return useQuery({ queryKey: ['metric-metadata', databaseName, metricType, metricName], queryFn: async ({ signal }) => { - if (!shouldFetch) { + if (!shouldFetch || !metricName) { return null; } diff --git a/packages/app/src/hooks/useFetchMetricResourceAttrs.tsx b/packages/app/src/hooks/useFetchMetricResourceAttrs.tsx index d591aad7..2d5cb3f3 100644 --- a/packages/app/src/hooks/useFetchMetricResourceAttrs.tsx +++ b/packages/app/src/hooks/useFetchMetricResourceAttrs.tsx @@ -128,8 +128,8 @@ const extractAttributeKeys = ( interface MetricResourceAttrsProps { databaseName: string; - metricType: string; - metricName: string; + metricType?: string; + metricName?: string; tableSource: TSource | undefined; isSql: boolean; } @@ -155,6 +155,7 @@ export const useFetchMetricResourceAttrs = ({ databaseName && tableName && metricType && + metricName && tableSource && tableSource?.kind === SourceKind.Metric, ); @@ -162,7 +163,7 @@ export const useFetchMetricResourceAttrs = ({ return useQuery({ queryKey: ['metric-attributes', metricType, metricName, isSql, tableSource], queryFn: async ({ signal }) => { - if (!shouldFetch) { + if (!shouldFetch || !metricName) { return []; }