Elgato_dark/hyperdx
1
0
Fork 0
mirror of https://github.com/hyperdxio/hyperdx synced 2026-04-21 13:37:15 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

feat: Improve chart editor validations (#1905)

Browse source 
## 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

<img width="472" height="89" alt="Screenshot 2026-03-13 at 9 04 43 AM" src="https://github.com/user-attachments/assets/18e57a4f-5f95-4984-b2cb-39fe9d441136" />

### SQL Preview Error and Loading States

<img width="1216" height="115" alt="Screenshot 2026-03-13 at 9 06 03 AM" src="https://github.com/user-attachments/assets/8394bea4-c1b6-4661-bb4e-1ef0e3078815" />
<img width="1203" height="123" alt="Screenshot 2026-03-13 at 9 05 53 AM" src="https://github.com/user-attachments/assets/3e0e103b-36a4-488a-bdf8-00e91353c50a" />

## How to test locally or on Vercel

This can be tested in the preview environment.

## References



- Linear Issue: Closes HDX-3598
- Related PRs:
This commit is contained in:
Drew Davis 2026-03-13 11:26:09 -04:00 committed by GitHub
parent 60d1bbaf58
commit 2e30c0e09d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 629 additions and 158 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.changeset/mighty-gorillas-ring.md Normal file
View file

@ -0,0 +1,5 @@
---
"@hyperdx/app": patch
---
feat: Improve chart editor validations

557
packages/app/src/components/ChartEditor/__tests__/utils.test.ts
View file

@ -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>,
): 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' }),
);
});
});

114
packages/app/src/components/ChartEditor/utils.ts
View file

@ -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<ChartEditorFormState> => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
return `${namePrefix}${fieldName}` as FieldPath<ChartEditorFormState>;
};
export const validateChartForm = (
form: ChartEditorFormState,
source: TSource | undefined,
setError: UseFormSetError<ChartEditorFormState>,
) => {
const errors: { path: Path<ChartEditorFormState>; 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<ChartEditorFormState>,
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;
};

16
packages/app/src/components/ChartSQLPreview.tsx
View file

@ -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 (
<Paper
@ -106,7 +106,17 @@ export default function ChartSQLPreview({
style={{ overflow: 'hidden' }}
p="xs"
>
<SQLPreview data={data} formatData={false} enableCopy={enableCopy} />
{isLoading ? (
<Text className="text-muted" size="xs">
Loading query preview...
</Text>
) : error ? (
<Text className="text-danger" size="xs">
Unable to format query. {error.message}
</Text>
) : (
<SQLPreview data={data} formatData={false} enableCopy={enableCopy} />
)}
</Paper>
);
}

76
packages/app/src/components/DBEditTimeChartForm.tsx
View file

@ -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<any>;
control: Control<ChartEditorFormState>;
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<any>;
setValue: UseFormSetValue<ChartEditorFormState>;
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' && (
<Flex justify="end">
@ -481,7 +482,7 @@ function ChartSeriesEditorComponent({
</div>
)}
</Flex>
{tableSource?.kind === SourceKind.Metric && metricName && (
{tableSource?.kind === SourceKind.Metric && metricName && metricType && (
<MetricAttributeHelperPanel
databaseName={databaseName}
metricType={metricType}
@ -500,6 +501,23 @@ function ChartSeriesEditorComponent({
}
const ChartSeriesEditor = ChartSeriesEditorComponent;
export const ErrorNotificationMessage = ({
errors,
}: {
errors: { path: Path<ChartEditorFormState>; message: string }[];
}) => {
return (
<List
size="sm"
icon={<IconX size={14} style={{ verticalAlign: 'middle' }} />}
>
{errors.map(({ message }, index) => (
<List.Item key={index}>{message}</List.Item>
))}
</List>
);
};
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: <ErrorNotificationMessage errors={errors} />,
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: <ErrorNotificationMessage errors={errors} />,
color: 'red',
});
return;
}
@ -1722,7 +1734,7 @@ function seriesToFilters(select: SelectList): Filter[] {
return null;
}
})
.filter(Boolean) as Filter[];
.filter(f => f != null);
return filters;
}

6
packages/app/src/components/__tests__/DBEditTimeChartForm.test.tsx
View file

@ -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

6
packages/app/src/hooks/useFetchMetricMetadata.tsx
View file

@ -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;
}

7
packages/app/src/hooks/useFetchMetricResourceAttrs.tsx
View file

@ -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 [];
}