diff --git a/.changeset/beige-parents-lick.md b/.changeset/beige-parents-lick.md new file mode 100644 index 0000000000..e2fd26d144 --- /dev/null +++ b/.changeset/beige-parents-lick.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +fix: prevent false "data source not set" error on markdown dashboard tiles diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts new file mode 100644 index 0000000000..46e40ee481 --- /dev/null +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts @@ -0,0 +1,77 @@ +import { isBuilderSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; +import { DisplayType } from '@hyperdx/common-utils/dist/types'; + +import { ConfigTile, convertToInternalTileConfig } from '../dashboards'; + +function makeMarkdownTile( + markdown?: string, + overrides: Partial = {}, +): ConfigTile { + return { + id: 'test-id', + x: 0, + y: 0, + w: 12, + h: 4, + name: 'Text Tile', + config: { displayType: 'markdown', markdown }, + ...overrides, + }; +} + +describe('convertToInternalTileConfig', () => { + describe('markdown tiles', () => { + it('sets displayType to Markdown', () => { + const result = convertToInternalTileConfig(makeMarkdownTile('# Hello')); + expect(result.config).toMatchObject({ + displayType: DisplayType.Markdown, + }); + }); + + it('preserves markdown content', () => { + const content = '## Guide\n\nSome instructions here.'; + const result = convertToInternalTileConfig(makeMarkdownTile(content)); + if (!isBuilderSavedChartConfig(result.config)) { + throw new Error('Expected BuilderSavedChartConfig for markdown tile'); + } + expect(result.config.markdown).toBe(content); + }); + + it('handles missing markdown content', () => { + const result = convertToInternalTileConfig(makeMarkdownTile(undefined)); + expect(result.config).toMatchObject({ + displayType: DisplayType.Markdown, + }); + }); + + it('preserves tile layout fields', () => { + const tile = makeMarkdownTile('Some text', { + x: 4, + y: 8, + w: 24, + h: 3, + name: 'How to Use', + }); + const result = convertToInternalTileConfig(tile); + expect(result.x).toBe(4); + expect(result.y).toBe(8); + expect(result.w).toBe(24); + expect(result.h).toBe(3); + // name is picked by convertToInternalTileConfig but not part of the + // Tile type — verify it round-trips via the runtime object. + expect(result).toHaveProperty('name', 'How to Use'); + }); + + it('does not set a real sourceId on the internal config', () => { + // Markdown tiles have source: '' in the internal format to satisfy the + // BuilderSavedChartConfig type. The frontend uses displayTypeRequiresSource() + // to exclude markdown tiles from "source unset" checks. + // See DBDashboardPage isSourceUnset and common-utils/src/guards.ts. + const result = convertToInternalTileConfig(makeMarkdownTile('# Hello')); + if (!isBuilderSavedChartConfig(result.config)) { + throw new Error('Expected BuilderSavedChartConfig for markdown tile'); + } + expect(result.config.source).not.toMatch(/^[a-f0-9]{24}$/); // not a real ObjectId + }); + }); +}); diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index e955dba243..286a51e133 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -25,6 +25,7 @@ import { displayTypeSupportsRawSqlAlerts, } from '@hyperdx/common-utils/dist/core/utils'; import { + displayTypeRequiresSource, isBuilderChartConfig, isBuilderSavedChartConfig, isRawSqlChartConfig, @@ -397,6 +398,7 @@ const Tile = forwardRef( const isSourceUnset = !!chart.config && isBuilderSavedChartConfig(chart.config) && + displayTypeRequiresSource(chart.config.displayType) && !chart.config.source; useEffect(() => { diff --git a/packages/common-utils/src/__tests__/guards.test.ts b/packages/common-utils/src/__tests__/guards.test.ts new file mode 100644 index 0000000000..166465aee1 --- /dev/null +++ b/packages/common-utils/src/__tests__/guards.test.ts @@ -0,0 +1,170 @@ +import { + displayTypeRequiresSource, + isBuilderSavedChartConfig, + isRawSqlSavedChartConfig, +} from '@/guards'; +import { DisplayType } from '@/types'; + +describe('isRawSqlSavedChartConfig', () => { + it('returns true when configType is "sql"', () => { + expect( + isRawSqlSavedChartConfig({ + configType: 'sql', + displayType: DisplayType.Table, + connection: 'conn-id', + sqlTemplate: 'SELECT 1', + } as any), + ).toBe(true); + }); + + it('returns false for a builder config with no configType', () => { + expect( + isRawSqlSavedChartConfig({ + displayType: DisplayType.Line, + source: 'src-id', + select: [], + where: '', + } as any), + ).toBe(false); + }); + + it('returns false for a markdown config', () => { + expect( + isRawSqlSavedChartConfig({ + displayType: DisplayType.Markdown, + markdown: '# Hello', + source: '', + where: '', + select: [], + } as any), + ).toBe(false); + }); +}); + +describe('isBuilderSavedChartConfig', () => { + it('returns true for a standard builder config', () => { + expect( + isBuilderSavedChartConfig({ + displayType: DisplayType.Line, + source: 'src-id', + select: [], + where: '', + } as any), + ).toBe(true); + }); + + it('returns false for a raw SQL config', () => { + expect( + isBuilderSavedChartConfig({ + configType: 'sql', + displayType: DisplayType.Table, + connection: 'conn-id', + sqlTemplate: 'SELECT 1', + } as any), + ).toBe(false); + }); + + it('returns true for a markdown tile config', () => { + // Markdown tiles are stored as BuilderSavedChartConfig with source: '' + // because the type requires a source field. displayTypeRequiresSource() + // is the canonical guard for excluding sourceless display types from the + // "source unset" check -- see DBDashboardPage isSourceUnset. + expect( + isBuilderSavedChartConfig({ + displayType: DisplayType.Markdown, + markdown: '# Hello', + source: '', + where: '', + select: [], + } as any), + ).toBe(true); + }); +}); + +describe('displayTypeRequiresSource', () => { + it('returns false for Markdown', () => { + expect(displayTypeRequiresSource(DisplayType.Markdown)).toBe(false); + }); + + it('returns true for Line', () => { + expect(displayTypeRequiresSource(DisplayType.Line)).toBe(true); + }); + + it('returns true for Table', () => { + expect(displayTypeRequiresSource(DisplayType.Table)).toBe(true); + }); + + it('returns true for Search', () => { + expect(displayTypeRequiresSource(DisplayType.Search)).toBe(true); + }); + + it('returns true for StackedBar', () => { + expect(displayTypeRequiresSource(DisplayType.StackedBar)).toBe(true); + }); + + it('returns true for Pie', () => { + expect(displayTypeRequiresSource(DisplayType.Pie)).toBe(true); + }); + + it('returns true for undefined', () => { + expect(displayTypeRequiresSource(undefined)).toBe(true); + }); + + describe('isSourceUnset pattern', () => { + // These tests exercise the production isSourceUnset condition used in + // DBDashboardPage via the real displayTypeRequiresSource export. + function isSourceUnset(config: any): boolean { + return ( + !!config && + isBuilderSavedChartConfig(config) && + displayTypeRequiresSource(config.displayType) && + !config.source + ); + } + + it('does not fire for markdown tiles with empty source', () => { + expect( + isSourceUnset({ + displayType: DisplayType.Markdown, + markdown: '# Hello', + source: '', // stored as '' by convertToInternalTileConfig + where: '', + select: [], + }), + ).toBe(false); + }); + + it('fires for builder tiles with empty source', () => { + expect( + isSourceUnset({ + displayType: DisplayType.Line, + source: '', + select: [], + where: '', + }), + ).toBe(true); + }); + + it('does not fire for builder tiles with a real source', () => { + expect( + isSourceUnset({ + displayType: DisplayType.Line, + source: 'abc123', + select: [], + where: '', + }), + ).toBe(false); + }); + + it('does not fire for raw SQL tiles', () => { + expect( + isSourceUnset({ + configType: 'sql', + displayType: DisplayType.Table, + connection: 'conn-id', + sqlTemplate: 'SELECT 1', + }), + ).toBe(false); + }); + }); +}); diff --git a/packages/common-utils/src/guards.ts b/packages/common-utils/src/guards.ts index 62621db5d0..8efc810923 100644 --- a/packages/common-utils/src/guards.ts +++ b/packages/common-utils/src/guards.ts @@ -3,6 +3,7 @@ import { BuilderSavedChartConfig, ChartConfig, ChartConfigWithOptDateRange, + DisplayType, RawSqlChartConfig, RawSqlSavedChartConfig, SavedChartConfig, @@ -31,3 +32,15 @@ export function isBuilderSavedChartConfig( ): chartConfig is BuilderSavedChartConfig { return !isRawSqlSavedChartConfig(chartConfig); } + +/** + * Returns true when a display type semantically requires a data source to be + * configured. Currently Markdown is the only display type that does not need a + * source (it renders static content). Add any future sourceless display types + * here rather than scattering per-type checks across the codebase. + */ +export function displayTypeRequiresSource( + displayType: DisplayType | undefined, +): boolean { + return displayType !== DisplayType.Markdown; +}