Skip to content

Commit a5294f8

Browse files
vinzeebrandon-pereira
vinzee
and
brandon-pereira
authored
fix: prevent false "data source not set" error on markdown dashboard tiles (#2202)
## Problem Markdown tiles created via the external API or MCP always show the following error: > The data source for this tile is not set. Edit the tile to select a data source. ...even though markdown tiles do not require a data source. Tiles created through the UI do not exhibit this problem. ## Root Cause `convertToInternalTileConfig` stores `source: ''` on the internal config for markdown tiles to satisfy the `BuilderSavedChartConfig` TypeScript type, which requires a `source` field: ```ts case 'markdown': internalConfig = { displayType: DisplayType.Markdown, markdown: externalConfig.markdown, source: '', // <-- required by type, but semantically empty ... } satisfies BuilderSavedChartConfig; ``` The frontend's `isSourceUnset` check in `DBDashboardPage` used `!chart.config.source`, which evaluates to `true` for an empty string, incorrectly triggering the error banner for every API-created markdown tile: ```ts const isSourceUnset = !!chart.config && isBuilderSavedChartConfig(chart.config) && !chart.config.source; // true for '', fires on all markdown tiles ``` ## Solution Add a `displayType !== DisplayType.Markdown` guard to `isSourceUnset` so markdown tiles are never considered to have an unset source: ```ts const isSourceUnset = !!chart.config && isBuilderSavedChartConfig(chart.config) && chart.config.displayType !== DisplayType.Markdown && !chart.config.source; ``` ## Tests - `packages/common-utils`: unit tests for `isBuilderSavedChartConfig` and the `isSourceUnset` pattern, verifying that markdown tiles evaluate to `false` (no error shown) even when `source` is `''` - `packages/api`: unit tests for `convertToInternalTileConfig` covering the markdown tile conversion path and tile layout field preservation Co-authored-by: Brandon Pereira <7552738+brandon-pereira@users.noreply.github.com>
1 parent 576373f commit a5294f8

5 files changed

Lines changed: 268 additions & 0 deletions

File tree

‎.changeset/beige-parents-lick.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@hyperdx/app': patch
3+
'@hyperdx/common-utils': patch
4+
---
5+
6+
fix: prevent false "data source not set" error on markdown dashboard tiles

‎packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import { isBuilderSavedChartConfig } from '@hyperdx/common-utils/dist/guards';
2+
import { DisplayType } from '@hyperdx/common-utils/dist/types';
3+
4+
import { ConfigTile, convertToInternalTileConfig } from '../dashboards';
5+
6+
function makeMarkdownTile(
7+
markdown?: string,
8+
overrides: Partial<ConfigTile> = {},
9+
): ConfigTile {
10+
return {
11+
id: 'test-id',
12+
x: 0,
13+
y: 0,
14+
w: 12,
15+
h: 4,
16+
name: 'Text Tile',
17+
config: { displayType: 'markdown', markdown },
18+
...overrides,
19+
};
20+
}
21+
22+
describe('convertToInternalTileConfig', () => {
23+
describe('markdown tiles', () => {
24+
it('sets displayType to Markdown', () => {
25+
const result = convertToInternalTileConfig(makeMarkdownTile('# Hello'));
26+
expect(result.config).toMatchObject({
27+
displayType: DisplayType.Markdown,
28+
});
29+
});
30+
31+
it('preserves markdown content', () => {
32+
const content = '## Guide\n\nSome instructions here.';
33+
const result = convertToInternalTileConfig(makeMarkdownTile(content));
34+
if (!isBuilderSavedChartConfig(result.config)) {
35+
throw new Error('Expected BuilderSavedChartConfig for markdown tile');
36+
}
37+
expect(result.config.markdown).toBe(content);
38+
});
39+
40+
it('handles missing markdown content', () => {
41+
const result = convertToInternalTileConfig(makeMarkdownTile(undefined));
42+
expect(result.config).toMatchObject({
43+
displayType: DisplayType.Markdown,
44+
});
45+
});
46+
47+
it('preserves tile layout fields', () => {
48+
const tile = makeMarkdownTile('Some text', {
49+
x: 4,
50+
y: 8,
51+
w: 24,
52+
h: 3,
53+
name: 'How to Use',
54+
});
55+
const result = convertToInternalTileConfig(tile);
56+
expect(result.x).toBe(4);
57+
expect(result.y).toBe(8);
58+
expect(result.w).toBe(24);
59+
expect(result.h).toBe(3);
60+
// name is picked by convertToInternalTileConfig but not part of the
61+
// Tile type — verify it round-trips via the runtime object.
62+
expect(result).toHaveProperty('name', 'How to Use');
63+
});
64+
65+
it('does not set a real sourceId on the internal config', () => {
66+
// Markdown tiles have source: '' in the internal format to satisfy the
67+
// BuilderSavedChartConfig type. The frontend uses displayTypeRequiresSource()
68+
// to exclude markdown tiles from "source unset" checks.
69+
// See DBDashboardPage isSourceUnset and common-utils/src/guards.ts.
70+
const result = convertToInternalTileConfig(makeMarkdownTile('# Hello'));
71+
if (!isBuilderSavedChartConfig(result.config)) {
72+
throw new Error('Expected BuilderSavedChartConfig for markdown tile');
73+
}
74+
expect(result.config.source).not.toMatch(/^[a-f0-9]{24}$/); // not a real ObjectId
75+
});
76+
});
77+
});

‎packages/app/src/DBDashboardPage.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
displayTypeSupportsRawSqlAlerts,
2626
} from '@hyperdx/common-utils/dist/core/utils';
2727
import {
28+
displayTypeRequiresSource,
2829
isBuilderChartConfig,
2930
isBuilderSavedChartConfig,
3031
isRawSqlChartConfig,
@@ -397,6 +398,7 @@ const Tile = forwardRef(
397398
const isSourceUnset =
398399
!!chart.config &&
399400
isBuilderSavedChartConfig(chart.config) &&
401+
displayTypeRequiresSource(chart.config.displayType) &&
400402
!chart.config.source;
401403

402404
useEffect(() => {

‎packages/common-utils/src/__tests__/guards.test.ts‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
import {
2+
displayTypeRequiresSource,
3+
isBuilderSavedChartConfig,
4+
isRawSqlSavedChartConfig,
5+
} from '@/guards';
6+
import { DisplayType } from '@/types';
7+
8+
describe('isRawSqlSavedChartConfig', () => {
9+
it('returns true when configType is "sql"', () => {
10+
expect(
11+
isRawSqlSavedChartConfig({
12+
configType: 'sql',
13+
displayType: DisplayType.Table,
14+
connection: 'conn-id',
15+
sqlTemplate: 'SELECT 1',
16+
} as any),
17+
).toBe(true);
18+
});
19+
20+
it('returns false for a builder config with no configType', () => {
21+
expect(
22+
isRawSqlSavedChartConfig({
23+
displayType: DisplayType.Line,
24+
source: 'src-id',
25+
select: [],
26+
where: '',
27+
} as any),
28+
).toBe(false);
29+
});
30+
31+
it('returns false for a markdown config', () => {
32+
expect(
33+
isRawSqlSavedChartConfig({
34+
displayType: DisplayType.Markdown,
35+
markdown: '# Hello',
36+
source: '',
37+
where: '',
38+
select: [],
39+
} as any),
40+
).toBe(false);
41+
});
42+
});
43+
44+
describe('isBuilderSavedChartConfig', () => {
45+
it('returns true for a standard builder config', () => {
46+
expect(
47+
isBuilderSavedChartConfig({
48+
displayType: DisplayType.Line,
49+
source: 'src-id',
50+
select: [],
51+
where: '',
52+
} as any),
53+
).toBe(true);
54+
});
55+
56+
it('returns false for a raw SQL config', () => {
57+
expect(
58+
isBuilderSavedChartConfig({
59+
configType: 'sql',
60+
displayType: DisplayType.Table,
61+
connection: 'conn-id',
62+
sqlTemplate: 'SELECT 1',
63+
} as any),
64+
).toBe(false);
65+
});
66+
67+
it('returns true for a markdown tile config', () => {
68+
// Markdown tiles are stored as BuilderSavedChartConfig with source: ''
69+
// because the type requires a source field. displayTypeRequiresSource()
70+
// is the canonical guard for excluding sourceless display types from the
71+
// "source unset" check -- see DBDashboardPage isSourceUnset.
72+
expect(
73+
isBuilderSavedChartConfig({
74+
displayType: DisplayType.Markdown,
75+
markdown: '# Hello',
76+
source: '',
77+
where: '',
78+
select: [],
79+
} as any),
80+
).toBe(true);
81+
});
82+
});
83+
84+
describe('displayTypeRequiresSource', () => {
85+
it('returns false for Markdown', () => {
86+
expect(displayTypeRequiresSource(DisplayType.Markdown)).toBe(false);
87+
});
88+
89+
it('returns true for Line', () => {
90+
expect(displayTypeRequiresSource(DisplayType.Line)).toBe(true);
91+
});
92+
93+
it('returns true for Table', () => {
94+
expect(displayTypeRequiresSource(DisplayType.Table)).toBe(true);
95+
});
96+
97+
it('returns true for Search', () => {
98+
expect(displayTypeRequiresSource(DisplayType.Search)).toBe(true);
99+
});
100+
101+
it('returns true for StackedBar', () => {
102+
expect(displayTypeRequiresSource(DisplayType.StackedBar)).toBe(true);
103+
});
104+
105+
it('returns true for Pie', () => {
106+
expect(displayTypeRequiresSource(DisplayType.Pie)).toBe(true);
107+
});
108+
109+
it('returns true for undefined', () => {
110+
expect(displayTypeRequiresSource(undefined)).toBe(true);
111+
});
112+
113+
describe('isSourceUnset pattern', () => {
114+
// These tests exercise the production isSourceUnset condition used in
115+
// DBDashboardPage via the real displayTypeRequiresSource export.
116+
function isSourceUnset(config: any): boolean {
117+
return (
118+
!!config &&
119+
isBuilderSavedChartConfig(config) &&
120+
displayTypeRequiresSource(config.displayType) &&
121+
!config.source
122+
);
123+
}
124+
125+
it('does not fire for markdown tiles with empty source', () => {
126+
expect(
127+
isSourceUnset({
128+
displayType: DisplayType.Markdown,
129+
markdown: '# Hello',
130+
source: '', // stored as '' by convertToInternalTileConfig
131+
where: '',
132+
select: [],
133+
}),
134+
).toBe(false);
135+
});
136+
137+
it('fires for builder tiles with empty source', () => {
138+
expect(
139+
isSourceUnset({
140+
displayType: DisplayType.Line,
141+
source: '',
142+
select: [],
143+
where: '',
144+
}),
145+
).toBe(true);
146+
});
147+
148+
it('does not fire for builder tiles with a real source', () => {
149+
expect(
150+
isSourceUnset({
151+
displayType: DisplayType.Line,
152+
source: 'abc123',
153+
select: [],
154+
where: '',
155+
}),
156+
).toBe(false);
157+
});
158+
159+
it('does not fire for raw SQL tiles', () => {
160+
expect(
161+
isSourceUnset({
162+
configType: 'sql',
163+
displayType: DisplayType.Table,
164+
connection: 'conn-id',
165+
sqlTemplate: 'SELECT 1',
166+
}),
167+
).toBe(false);
168+
});
169+
});
170+
});

‎packages/common-utils/src/guards.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
BuilderSavedChartConfig,
44
ChartConfig,
55
ChartConfigWithOptDateRange,
6+
DisplayType,
67
RawSqlChartConfig,
78
RawSqlSavedChartConfig,
89
SavedChartConfig,
@@ -31,3 +32,15 @@ export function isBuilderSavedChartConfig(
3132
): chartConfig is BuilderSavedChartConfig {
3233
return !isRawSqlSavedChartConfig(chartConfig);
3334
}
35+
36+
/**
37+
* Returns true when a display type semantically requires a data source to be
38+
* configured. Currently Markdown is the only display type that does not need a
39+
* source (it renders static content). Add any future sourceless display types
40+
* here rather than scattering per-type checks across the codebase.
41+
*/
42+
export function displayTypeRequiresSource(
43+
displayType: DisplayType | undefined,
44+
): boolean {
45+
return displayType !== DisplayType.Markdown;
46+
}

0 commit comments

Comments
 (0)