From 2e30c0e09da48115ca380d0d7ff7a6c0c19df944 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Fri, 13 Mar 2026 11:26:09 -0400 Subject: [PATCH] feat: Improve chart editor validations (#1905) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR improves chart validation. Previously, when the chart configuration was incorrect, the save and run buttons would do nothing, or would run an invalid chart config. Now, we surface some known errors to the user via a notification with helpful messages. Additionally, this PR 1. Removes the last remaining `any` and `as` casts from the `DBEditTimeChartForm` component and its utils 2. Adds error and loading states to the SQL Preview component ### Validations The following validations are performed: 1. A metric name is selected when a metric source is selected (pre-existing) 3. A source is selected when using the chart builder format (except for markdown charts) 4. A connection is selected when using the raw sql format 5. Each series with a non-`count` aggregation has a non-empty valueExpression 6. Number and Pie charts have only a single series 7. Raw SQL charts have a non-empty sqlTemplate When errors are found 1. The run and save button do not run or save 2. A notification is shown with user-friendly error messages Screenshot 2026-03-13 at 9 04 43 AM ### SQL Preview Error and Loading States Screenshot 2026-03-13 at 9 06 03 AM Screenshot 2026-03-13 at 9 05 53 AM ## How to test locally or on Vercel This can be tested in the preview environment. ## References - Linear Issue: Closes HDX-3598 - Related PRs: --- .changeset/mighty-gorillas-ring.md | 5 + .../ChartEditor/__tests__/utils.test.ts | 557 +++++++++++++++--- .../app/src/components/ChartEditor/utils.ts | 114 +++- .../app/src/components/ChartSQLPreview.tsx | 16 +- .../src/components/DBEditTimeChartForm.tsx | 76 ++- .../__tests__/DBEditTimeChartForm.test.tsx | 6 +- .../app/src/hooks/useFetchMetricMetadata.tsx | 6 +- .../src/hooks/useFetchMetricResourceAttrs.tsx | 7 +- 8 files changed, 629 insertions(+), 158 deletions(-) create mode 100644 .changeset/mighty-gorillas-ring.md 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 []; }