Skip to content

Commit 21880e4

Browse files
alex-fedotyevclaude
andcommitted
fix(dashboards): address deep review on #2383 + e2e shard 2 fix
E2E shard 2 was failing on three pre-existing dashboard tests because `visibleFilters` in DashboardFilters.tsx was a new array reference every render, churning useDashboardFilterValues' useQueries and leaving the chip dropdown disabled while the values query never settled. Memoize the array so a stable reference flows into the hook. Deep review fixes: - P0/P1: MCP hyperdx_save_dashboard inputSchema now exposes savedFilterValues, paired with a usage example in the filters param description so constant: true filters can actually lock to a value via MCP. The body schema already accepts it; only the tool input shape was missing. - P2: DashboardFiltersModal snapshots initialDefaultValues via state keyed on filter.id, so a background savedFilterValues refetch no longer wipes in-progress edits in the default-value picker. - P2: Schema-level coherence checks moved to common-utils via two reusable refinements applied at the external API boundary: refineDashboardFilterCoherence rejects readonly/hidden render modes without constant: true; refineDashboardFiltersConstantSiblings rejects two filter definitions sharing an expression that disagree on constant. Internal callers (MCP, future migrations) get the same protection through the shared schemas. - P2: openapi.json + JSDoc no longer declare default: false / default: "editable" on the new constant / renderMode property schemas. The Zod schema is .optional() with no .default() so generated SDKs would otherwise surface a default the wire format never produces; the implied default now lives in the description text instead. - P2: page-level merge logic for "Save default" / URL hydration / savedFilterValues upsert is extracted into dashboardFilterUtils (normalizeExpression, buildConstantExpressionSet, stripConstantsFromUrl, mergeConstantFiltersForSave, upsertSavedDefault, removeSavedDefaultForExpression). The matrix including bracket-notation expressions is covered by a new unit test. useDashboardFilters and DBDashboardPage call the helpers. - P2: bracket-notation constant filter is now covered in the hook unit tests (SpanAttributes['k8s.pod.name'] resolves through filterValues, blocks setFilterValue, and survives via getFilterQueriesForSource). P3 polish: - handleRemoveFilter now strips the matching savedFilterValues entry alongside the filter delete so deleting + recreating a filter on the same expression doesn't silently re-lock to an orphaned value. - applyFilterVisibility('editable') returns {} so the spread on save no longer leaves constant: undefined / renderMode: undefined keys in memory. - defaultsChanged uses set equality so reordering the same values doesn't fire a spurious save write. - NEVER_USED_RANGE hoisted to module scope. - FilterVisibility uses z.infer<typeof DashboardFilterRenderMode> instead of a hand-rolled string union. - as DashboardFilter cast dropped on filtersForQuery. - usePresetDashboardFilters.handleSaveFilter widened to accept the optional defaultValues arg so a future supportsConstantFilters=true flip on presets doesn't silently drop the second arg. - parseQuery import uses the @/searchFilters alias for consistency with sibling files. - dashboard?.filters dropped from the URL hydration effect's deps (the initializedDashboard.current === dashboard.id guard already prevents re-runs). - HDX-4404 API round-trip test uses toEqual instead of toMatchObject so unexpected extra fields fail the assertion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 6946b0a commit 21880e4

16 files changed

Lines changed: 793 additions & 159 deletions

File tree

packages/api/openapi.json

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2362,8 +2362,7 @@
23622362
},
23632363
"constant": {
23642364
"type": "boolean",
2365-
"description": "When true, the value from the dashboard's savedFilterValues matched\nby this filter's expression is applied automatically on every tile\nthis filter scopes, and viewers cannot change it. Use this to lock\na dashboard template to a single scope (clone the dashboard, save a\ndifferent default per copy). Pairs with renderMode to control how\nthe locked filter shows in the filter bar.\n",
2366-
"default": false,
2365+
"description": "When true, the value from the dashboard's savedFilterValues matched\nby this filter's expression is applied automatically on every tile\nthis filter scopes, and viewers cannot change it. Use this to lock\na dashboard template to a single scope (clone the dashboard, save a\ndifferent default per copy). Pairs with renderMode to control how\nthe locked filter shows in the filter bar. Omit (or send false)\nfor an ordinary editable filter (the implicit default behavior).\n",
23672366
"example": true
23682367
},
23692368
"renderMode": {
@@ -2373,8 +2372,7 @@
23732372
"readonly",
23742373
"hidden"
23752374
],
2376-
"description": "Controls how this filter renders in the dashboard filter bar.\n\"editable\" (default) shows a normal dropdown the viewer can change.\n\"readonly\" shows a disabled chip with a lock icon; the viewer\nsees the locked value but cannot edit it. \"hidden\" omits the chip\nentirely; the locked value still scopes every matching tile.\n",
2377-
"default": "editable",
2375+
"description": "Controls how this filter renders in the dashboard filter bar.\nOmit for the implicit \"editable\" behavior (normal dropdown the\nviewer can change). \"readonly\" shows a disabled chip with a lock\nicon; the viewer sees the locked value but cannot edit it.\n\"hidden\" omits the chip entirely; the locked value still scopes\nevery matching tile. \"readonly\" and \"hidden\" require constant: true.\n",
23782376
"example": "readonly"
23792377
}
23802378
}

packages/api/src/mcp/tools/dashboards/saveDashboard.ts

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,12 @@ import {
2323
resolveSavedQueryLanguage,
2424
updateDashboardBodySchema,
2525
} from '@/routers/external-api/v2/utils/dashboards';
26-
import type {
27-
ExternalDashboardFilter,
28-
ExternalDashboardFilterWithId,
29-
ExternalDashboardTileWithId,
26+
import {
27+
type ExternalDashboardFilter,
28+
type ExternalDashboardFilterWithId,
29+
type ExternalDashboardSavedFilterValue,
30+
externalDashboardSavedFilterValueSchema,
31+
type ExternalDashboardTileWithId,
3032
} from '@/utils/zod';
3133

3234
import { withToolTracing } from '../../utils/tracing';
@@ -62,6 +64,20 @@ export function registerSaveDashboard(
6264
tags: z.array(z.string()).optional().describe('Dashboard tags'),
6365
containers: mcpContainersParam.optional(),
6466
filters: mcpFiltersParam.optional(),
67+
savedFilterValues: z
68+
.array(externalDashboardSavedFilterValueSchema)
69+
.optional()
70+
.describe(
71+
'Optional saved default values for the dashboard filters. Each ' +
72+
'entry is a Lucene or SQL `condition` string keyed by a filter ' +
73+
'expression (e.g. `ServiceName:"hdx-private-api"`). ' +
74+
'Pair this with `constant: true` on a filter in the `filters` ' +
75+
'array to lock that filter to a specific value: the matching ' +
76+
'savedFilterValues entry is applied automatically on every ' +
77+
'tile and the viewer cannot override it. ' +
78+
'If you set `constant: true` without a corresponding ' +
79+
'savedFilterValues entry, the filter has no effect.',
80+
),
6581
}),
6682
},
6783
withToolTracing(
@@ -74,6 +90,7 @@ export function registerSaveDashboard(
7490
tags,
7591
containers,
7692
filters: inputFilters,
93+
savedFilterValues: inputSavedFilterValues,
7794
}) => {
7895
if (!dashboardId) {
7996
return createDashboard({
@@ -84,6 +101,7 @@ export function registerSaveDashboard(
84101
tags,
85102
containers,
86103
inputFilters,
104+
inputSavedFilterValues,
87105
});
88106
}
89107
return updateDashboard({
@@ -95,6 +113,7 @@ export function registerSaveDashboard(
95113
tags,
96114
containers,
97115
inputFilters,
116+
inputSavedFilterValues,
98117
});
99118
},
100119
),
@@ -147,6 +166,7 @@ async function createDashboard({
147166
tags,
148167
containers,
149168
inputFilters,
169+
inputSavedFilterValues,
150170
}: {
151171
teamId: string;
152172
frontendUrl: string | undefined;
@@ -157,13 +177,15 @@ async function createDashboard({
157177
inputFilters:
158178
| (ExternalDashboardFilter | ExternalDashboardFilterWithId)[]
159179
| undefined;
180+
inputSavedFilterValues: ExternalDashboardSavedFilterValue[] | undefined;
160181
}) {
161182
const parsed = createDashboardBodySchema.safeParse({
162183
name,
163184
tiles: inputTiles,
164185
tags,
165186
containers,
166187
filters: stripFilterIds(inputFilters),
188+
savedFilterValues: inputSavedFilterValues,
167189
});
168190
if (!parsed.success) {
169191
return {
@@ -332,6 +354,7 @@ async function updateDashboard({
332354
tags,
333355
containers,
334356
inputFilters,
357+
inputSavedFilterValues,
335358
}: {
336359
teamId: string;
337360
frontendUrl: string | undefined;
@@ -343,6 +366,7 @@ async function updateDashboard({
343366
inputFilters:
344367
| (ExternalDashboardFilter | ExternalDashboardFilterWithId)[]
345368
| undefined;
369+
inputSavedFilterValues: ExternalDashboardSavedFilterValue[] | undefined;
346370
}) {
347371
if (!mongoose.Types.ObjectId.isValid(dashboardId)) {
348372
return {
@@ -357,6 +381,7 @@ async function updateDashboard({
357381
tags,
358382
containers,
359383
filters: assignFilterIds(inputFilters),
384+
savedFilterValues: inputSavedFilterValues,
360385
});
361386
if (!parsed.success) {
362387
return {

packages/api/src/mcp/tools/dashboards/schemas.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -745,7 +745,9 @@ export const mcpFiltersParam = z
745745
"its value to the dashboard's saved default (matched by `expression`); pair with " +
746746
'`renderMode: "readonly"` to show a disabled chip or `"hidden"` to drop the chip ' +
747747
'entirely while keeping the WHERE clause active. Locked filters cannot be cleared by ' +
748-
'the viewer.\n\n' +
748+
'the viewer. The locked value comes from the dashboard-level `savedFilterValues` ' +
749+
"array (matched by this filter's `expression`); set both together in the same " +
750+
'`hyperdx_save_dashboard` call.\n\n' +
749751
'Example (broadcast to every tile):\n' +
750752
'[\n' +
751753
' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "ServiceName",\n' +
@@ -756,7 +758,18 @@ export const mcpFiltersParam = z
756758
' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "SpanName",\n' +
757759
' "sourceId": "<trace-source-id>", "whereLanguage": "sql",\n' +
758760
' "appliesToSourceIds": ["<trace-source-id>"] }\n' +
759-
']',
761+
']\n\n' +
762+
'Example (locked scope-filter template) - pair with the top-level ' +
763+
'savedFilterValues array on the dashboard call so the constant filter ' +
764+
'has a value to apply:\n' +
765+
'[\n' +
766+
' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "ServiceName",\n' +
767+
' "sourceId": "<trace-source-id>", "whereLanguage": "sql",\n' +
768+
' "constant": true, "renderMode": "readonly" }\n' +
769+
']\n' +
770+
'plus on the dashboard call body:\n' +
771+
' "savedFilterValues": [ { "type": "lucene",\n' +
772+
' "condition": "ServiceName:\\"hdx-private-api\\"" } ]',
760773
);
761774

762775
export const mcpContainersParam = z

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

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -958,21 +958,41 @@ describe('External API v2 Dashboards - old format', () => {
958958
.expect(200);
959959

960960
expect(response.body.data.filters).toHaveLength(3);
961-
expect(response.body.data.filters[0]).toMatchObject({
961+
// Use toEqual on the full filter object so extra fields (which would
962+
// indicate schema drift) fail the assertion. The bot deep-review at
963+
// dashboards.test.ts:961 flagged toMatchObject for letting unexpected
964+
// fields slip through silently.
965+
expect(response.body.data.filters[0]).toEqual({
966+
id: response.body.data.filters[0].id,
967+
type: 'QUERY_EXPRESSION',
962968
name: 'Service (locked, read-only)',
969+
expression: 'ServiceName',
970+
sourceId: traceSource._id.toString(),
971+
whereLanguage: 'sql',
963972
constant: true,
964973
renderMode: 'readonly',
965974
});
966-
expect(response.body.data.filters[1]).toMatchObject({
975+
expect(response.body.data.filters[1]).toEqual({
976+
id: response.body.data.filters[1].id,
977+
type: 'QUERY_EXPRESSION',
967978
name: 'Environment (hidden)',
979+
expression: 'environment',
980+
sourceId: traceSource._id.toString(),
981+
whereLanguage: 'sql',
968982
constant: true,
969983
renderMode: 'hidden',
970984
});
971985
// Filter 2 omitted the new fields. They must NOT be materialized as
972986
// defaults on read; the absence is meaningful (default behavior
973987
// matches today's editable, non-locked filter).
974-
expect(response.body.data.filters[2].constant).toBeUndefined();
975-
expect(response.body.data.filters[2].renderMode).toBeUndefined();
988+
expect(response.body.data.filters[2]).toEqual({
989+
id: response.body.data.filters[2].id,
990+
type: 'QUERY_EXPRESSION',
991+
name: 'Region (default editable)',
992+
expression: 'region',
993+
sourceId: traceSource._id.toString(),
994+
whereLanguage: 'sql',
995+
});
976996

977997
// GET round-trip.
978998
const getResponse = await authRequest(
@@ -1015,9 +1035,13 @@ describe('External API v2 Dashboards - old format', () => {
10151035
.expect(200);
10161036

10171037
expect(updateResponse.body.data.filters).toHaveLength(2);
1018-
expect(updateResponse.body.data.filters[1]).toMatchObject({
1038+
expect(updateResponse.body.data.filters[1]).toEqual({
10191039
id: response.body.data.filters[2].id,
1040+
type: 'QUERY_EXPRESSION',
10201041
name: 'Region (now read-only)',
1042+
expression: 'region',
1043+
sourceId: traceSource._id.toString(),
1044+
whereLanguage: 'sql',
10211045
constant: true,
10221046
renderMode: 'readonly',
10231047
});

packages/api/src/routers/external-api/v2/dashboards.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1378,19 +1378,19 @@ function getSourceConnectionMismatches(
13781378
* this filter scopes, and viewers cannot change it. Use this to lock
13791379
* a dashboard template to a single scope (clone the dashboard, save a
13801380
* different default per copy). Pairs with renderMode to control how
1381-
* the locked filter shows in the filter bar.
1382-
* default: false
1381+
* the locked filter shows in the filter bar. Omit (or send false)
1382+
* for an ordinary editable filter (the implicit default behavior).
13831383
* example: true
13841384
* renderMode:
13851385
* type: string
13861386
* enum: [editable, readonly, hidden]
13871387
* description: |
13881388
* Controls how this filter renders in the dashboard filter bar.
1389-
* "editable" (default) shows a normal dropdown the viewer can change.
1390-
* "readonly" shows a disabled chip with a lock icon; the viewer
1391-
* sees the locked value but cannot edit it. "hidden" omits the chip
1392-
* entirely; the locked value still scopes every matching tile.
1393-
* default: "editable"
1389+
* Omit for the implicit "editable" behavior (normal dropdown the
1390+
* viewer can change). "readonly" shows a disabled chip with a lock
1391+
* icon; the viewer sees the locked value but cannot edit it.
1392+
* "hidden" omits the chip entirely; the locked value still scopes
1393+
* every matching tile. "readonly" and "hidden" require constant: true.
13941394
* example: "readonly"
13951395
*
13961396
* Filter:

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@ import {
1414
DASHBOARD_MAX_CONTAINERS,
1515
DashboardContainer,
1616
DashboardContainerSchema,
17+
DashboardFilter,
1718
DisplayType,
1819
isLogSource,
1920
isOnClickDashboardById,
2021
isOnClickSearchById,
2122
isTraceSource,
2223
RawSqlSavedChartConfig,
24+
refineDashboardFiltersConstantSiblings,
2325
SavedChartConfig,
2426
} from '@hyperdx/common-utils/dist/types';
2527
import { SearchConditionLanguageSchema as whereLanguageSchema } from '@hyperdx/common-utils/dist/types';
@@ -1149,6 +1151,18 @@ function buildDashboardBodySchema(filterSchema: z.ZodTypeAny): z.ZodEffects<
11491151
// (otherwise a tile that references a real preserved container
11501152
// would be rejected against an empty `data.containers ?? []`).
11511153
validateDashboardContainersStructure(data.containers ?? [], ctx);
1154+
1155+
// Cross-filter check: reject mixing constant: true with editable
1156+
// siblings on the same expression. The filter-array element type
1157+
// is `unknown` from Zod's perspective here, so cast through the
1158+
// shared shape. Filters are already individually validated by the
1159+
// `filterSchema` arg, so the cast is safe.
1160+
if (data.filters && data.filters.length > 0) {
1161+
refineDashboardFiltersConstantSiblings(
1162+
data.filters as unknown as DashboardFilter[],
1163+
ctx,
1164+
);
1165+
}
11521166
});
11531167
}
11541168

packages/api/src/utils/zod.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
NumberFormatSchema,
1111
OnClickDashboardSchema,
1212
OnClickSearchSchema,
13+
refineDashboardFilterCoherence,
1314
scheduleStartAtSchema,
1415
SearchConditionLanguageSchema as whereLanguageSchema,
1516
validateAlertScheduleOffsetMinutes,
@@ -128,18 +129,28 @@ type ChartSeries = z.infer<typeof chartSeriesSchema>;
128129

129130
export const tagsSchema = z.array(z.string().max(32)).max(50).optional();
130131

131-
export const externalDashboardFilterSchemaWithId = DashboardFilterSchema.omit({
132+
// Bare object schema (no superRefine) so `.omit`/`.extend` chains still
133+
// work on the create variant below. Apply the coherence refinement to
134+
// each shipped schema separately.
135+
const externalDashboardFilterBaseSchemaWithId = DashboardFilterSchema.omit({
132136
source: true,
133137
})
134138
.extend({ sourceId: objectIdSchema })
135139
.strict();
136140

141+
export const externalDashboardFilterSchemaWithId =
142+
externalDashboardFilterBaseSchemaWithId.superRefine(
143+
refineDashboardFilterCoherence,
144+
);
145+
137146
export type ExternalDashboardFilterWithId = z.infer<
138147
typeof externalDashboardFilterSchemaWithId
139148
>;
140149

141150
export const externalDashboardFilterSchema =
142-
externalDashboardFilterSchemaWithId.omit({ id: true });
151+
externalDashboardFilterBaseSchemaWithId
152+
.omit({ id: true })
153+
.superRefine(refineDashboardFilterCoherence);
143154

144155
export type ExternalDashboardFilter = z.infer<
145156
typeof externalDashboardFilterSchema
@@ -150,7 +161,7 @@ export const externalDashboardSavedFilterValueSchema = z.object({
150161
condition: z.string().max(10000),
151162
});
152163

153-
type ExternalDashboardSavedFilterValue = z.infer<
164+
export type ExternalDashboardSavedFilterValue = z.infer<
154165
typeof externalDashboardSavedFilterValueSchema
155166
>;
156167

0 commit comments

Comments
 (0)