Skip to content

feat(api): expose seriesLimit on line/stacked_bar tiles in external API - #2772

Merged
kodiakhq[bot] merged 5 commits into
mainfrom
cursor/expose-series-limit-external-api-b271
Aug 4, 2026
Merged

feat(api): expose seriesLimit on line/stacked_bar tiles in external API#2772
kodiakhq[bot] merged 5 commits into
mainfrom
cursor/expose-series-limit-external-api-b271

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds the seriesLimit display setting to the line and stacked-bar chart schemas in the external API and MCP. Previously, this field was stripped out by the schemas, and thus could not round-trip in a dashboard modified by MCP or the external API.

Testing

Connect via the API or MCP and validate that the property round-trips for line and stacked bar charts.

Screenshots or video

N/A — non-UI change (External API only).

How to test on Vercel preview

N/A — non-UI change

References

  • Linear Issue: Closes HDX-4988

@changeset-bot

changeset-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 48c49ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 4, 2026 12:11pm
hyperdx-storybook Ready Ready Preview Aug 4, 2026 12:11pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds positive-integer seriesLimit support to line and stacked-bar dashboard tiles across the External API and MCP, preserving the setting through validation, conversion, persistence, and retrieval.

  • Extends External API, MCP, and OpenAPI schemas with the optional field.
  • Updates internal/external dashboard conversion in both directions.
  • Adds create, update, patch, and retrieval round-trip integration coverage.
  • Includes an API patch changeset.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/api/src/utils/zod.ts Extends the external line and stacked-bar chart schemas with an optional positive-integer series limit.
packages/api/src/routers/external-api/v2/utils/dashboards.ts Preserves seriesLimit while converting line and stacked-bar configurations between external and internal representations.
packages/api/src/mcp/tools/dashboards/schemas.ts Exposes seriesLimit in MCP dashboard schemas with matching validation and agent-facing documentation.
packages/api/src/routers/external-api/v2/dashboards.ts Documents seriesLimit for line and stacked-bar configurations in the source OpenAPI annotations.
packages/api/openapi.json Publishes the new optional field in the generated External API contract.
packages/api/src/routers/external-api/tests/dashboards.int.test.ts Covers create, update, omission, persistence, and retrieval behavior through External API v2.
packages/api/src/mcp/tests/dashboards/patchDashboard.int.test.ts Verifies that MCP patching preserves seriesLimit when changing a line tile to stacked-bar.
packages/api/src/mcp/tests/dashboards/saveDashboard.int.test.ts Extends MCP save-dashboard coverage to include grouped line and stacked-bar tiles with series limits.

Reviews (9): Last reviewed commit: "Merge branch 'main' into cursor/expose-s..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 264 passed • 1 skipped • 905s

Status Count
✅ Passed 264
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

Co-authored-by: Drew Davis <pulpdrew@gmail.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (2):
    • packages/api/src/routers/external-api/v2/dashboards.ts
    • packages/api/src/routers/external-api/v2/utils/dashboards.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 5
  • Production lines changed: 38 (+ 229 in test files, excluded from tier calculation)
  • Branch: cursor/expose-series-limit-external-api-b271
  • Author: pulpdrew

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

✅ No critical issues found. The five-line production change is correct on its happy path: seriesLimit exists on the internal builder config as z.number().int().positive().nullish() (packages/common-utils/src/types.ts:455), ?? undefined correctly narrows internal null to the external .optional() type, satisfies BuilderSavedChartConfig genuinely type-checks the new field, and the value reaches ClickHouse as a bound Int32 parameter rather than interpolated SQL. Authz and injection surfaces were reviewed and are clean.

🟡 P2 -- recommended

  • packages/api/src/mcp/tools/dashboards/schemas.ts:498 -- mcpLineTileSchema and mcpBarTileSchema still omit seriesLimit, so the MCP write surface now diverges from REST v2: clickstack_get_dashboard_tile emits the field, Zod strips it on the way back in, and patchDashboard replaces the tile config wholesale, clearing any persisted value.
    • Fix: Add seriesLimit: z.number().int().positive().optional() to both MCP tile config schemas, mirroring the limit field already present on the pie and categorical-bar MCP schemas.
    • correctness, agent-native, adversarial
  • packages/api/src/utils/zod.ts:461 -- Because the builder tile union strips unknown keys instead of rejecting them, a PUT that flips a tile's displayType between line/stacked_bar and pie/bar silently discards the limit, since the same internal field is spelled seriesLimit on one family and limit on the other.
    • Fix: Accept each name as a deprecated alias on the other family and normalize in the converter, or reject a known-sibling key that does not belong to the selected displayType.
    • api-contract, correctness, adversarial, maintainability
  • packages/api/src/utils/zod.ts:271 -- z.number().int().positive() has no upper bound, so a value at or above 2³¹ passes validation and persists with a 200, then fails on every subsequent render when renderSeriesLimitCte binds it as a ClickHouse Int32 at packages/common-utils/src/core/renderChartConfig.ts:1380.
    • Fix: Add a .max() bound to the two new seriesLimit fields, and to the pie/bar limit fields at lines 378 and 388 which share the identical hole.
    • adversarial, api-contract, performance, security
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:2596 -- The added tests exercise only valid positive integers, so none of the three constraints the new schema actually enforces are covered and a refactor that loosened them to z.number().optional() would pass.
    • Fix: Add rejection tests asserting a 400 for seriesLimit values of 0, -1, 1.5, and a string, following the negative-case pattern already used for number-tile color rules.
    • testing, maintainability, kieran-typescript, api-contract, correctness
  • packages/api/src/utils/zod.ts:271 -- No .changeset/ entry accompanies this change, though AGENTS.md requires one for any behavior-affecting change to a published package and @hyperdx/api's external API behavior changes here.
    • Fix: Add a changeset for @hyperdx/api describing the newly honored seriesLimit field with an appropriate semver bump.
    • project-standards
🔵 P3 nitpicks (7)
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:302 -- config.seriesLimit ?? undefined filters only nullish values, so a stored 0, -5, 2.5, or "10" would be echoed on GET and then rejected by the new input schema on the next PUT, failing the whole dashboard update; every validated write path already enforces the constraint, so this needs a direct DB write or legacy data to reach.
    • Fix: Narrow on read with a positive-integer type guard, matching the defensive safeParse and stringValueOrDefault treatment its sibling fields already get in this function.
  • packages/api/src/routers/external-api/v2/dashboards.ts:632 -- The OpenAPI JSDoc documents seriesLimit as type: number with no minimum, while the schema enforces a positive integer, so generated clients will accept values the server rejects.
    • Fix: Change both seriesLimit doc blocks to type: integer with minimum: 1, matching how the pie and bar limit fields are already documented in the same file.
  • packages/api/src/utils/zod.ts:271 -- The two new fields carry no comment, though every other non-obvious field on these external schemas documents why it is exposed and why sibling variants omit it.
    • Fix: Add a comment recording why seriesLimit is absent from the raw-SQL line and stacked-bar schemas and why the external name differs from the pie/bar spelling of the same internal field.
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:707 -- The field is accepted and echoed on metric-source tiles and on tiles with no groupBy, but renderSeriesLimitCte bails when the translated config has an empty from.databaseName, so it silently does nothing in both cases.
    • Fix: Document the no-op caveat on the OpenAPI seriesLimit descriptions, or gate it in validateDashboardTiles the way heatmap source kinds are gated.
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:2665 -- The omission test is titled for line and stacked_bar tiles but posts only a line tile, leaving the separate StackedBar arm of the converter uncovered for the absent-field case.
    • Fix: Add a stacked_bar tile to the same test and assert its config also lacks the property.
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:4427 -- Nothing asserts that a configType: 'sql' line or stacked_bar tile has seriesLimit stripped, which is the exact boundary the deliberate raw-SQL omission rests on.
    • Fix: Extend the raw-SQL round-trip test with a tile that submits seriesLimit and assert it is absent from the persisted config.
  • packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts:34 -- The dedicated unit-test file for these converters was not updated, so the only coverage of the three new converter lines runs through the slow DB-backed integration suite.
    • Fix: Add a direct convertToInternalTileConfig case asserting seriesLimit is carried through and stays absent when omitted.

Reviewers (11): correctness, testing, maintainability, project-standards, api-contract, kieran-typescript, adversarial, performance, security, agent-native, learnings-researcher.

Testing gaps:

  • No boundary coverage at the upper end, where values above the ClickHouse Int32 range currently persist successfully and break the tile later.
  • No test for the displayType-flip scenario in either direction, which is where the seriesLimit/limit naming split silently drops the value.
  • No test that a seriesLimit set through REST v2 survives an MCP patch of the same tile.
  • No legacy-data test seeding a non-conforming seriesLimit directly into the Mixed tiles field, unlike the equivalent guards for containerId, colorRules, and heatmap valueExpression.

Follow-up, not part of this change: granularity and ratioMode are settable in the chart editor and persisted on builder line/stacked_bar configs but absent from these same external schemas. That is a separate parity gap and should not widen this PR.

Environment caveat: bash and git were unavailable in this run (bwrap sandbox init failure), so the diff was reconstructed from the PR's stated file list plus direct file reads rather than git diff. Line numbers and code claims were verified against file contents; pre-existing-versus-new attribution rests on code reading rather than a base-SHA diff.

Co-authored-by: Drew Davis <pulpdrew@gmail.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. The change is additive and backward compatible: an optional field added to two builder tile schemas, wired symmetrically through both converters, with _.omitBy(_.isNil) at packages/api/src/routers/external-api/v2/utils/dashboards.ts:866 correctly keeping absent values out of Mongo. The outbound ?? undefined is required (internal is .nullish(), the annotated external return type is number | undefined), and the checked-in generated spec packages/api/openapi.json was regenerated. Findings below are contract-accuracy, coverage, and release-hygiene gaps.

🟡 P2 -- recommended

  • packages/api/src/routers/external-api/v2/dashboards.ts:633 -- The OpenAPI block documents seriesLimit as type: number with no minimum, but the Zod schema enforces .int().positive(), so a spec-conforming client sending 0 or 2.5 gets a 400 the published spec says is valid.
    • Fix: Change both new blocks (lines 633 and 684) to type: integer with minimum: 1, matching the limit property for the same internal field at lines 832-833 and 885-886, then re-run yarn docgen so packages/api/openapi.json:1646 and :1709 pick it up.
    • correctness, api-contract, maintainability
  • packages/api/src/mcp/tools/dashboards/schemas.ts:545 -- The new MCP seriesLimit field has no test coverage, and mcpLineTileSchema/mcpBarTileSchema are the sole validation gate for clickstack_patch_dashboard arguments, so the REST integration tests do not cover this path at all.
    • Fix: Add a test that drives the save and patch dashboard tool handlers with a line and a stacked_bar tile carrying seriesLimit and asserts the value reaches the persisted tile.
    • testing, kieran-typescript, agent-native
  • packages/api/src/utils/zod.ts:271 -- No test exercises the new .int().positive() rejection branches on either the line or stacked_bar schema, so a future loosening of the constraint would go unnoticed.
    • Fix: Add an it.each rejection test covering seriesLimit values of 0, a negative number, and a non-integer, following the existing rejection-test block in packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts.
    • testing, api-contract
  • packages/api/src/routers/external-api/v2/dashboards.ts:634 -- The REST description does not mention that seriesLimit is silently ignored without a groupBy, so a REST consumer sets it on an ungrouped tile, gets a 200, and no limiting is applied.
    • Fix: State the groupBy and granularity precondition in both descriptions (lines 634 and 685), matching the behavior gated in renderSeriesLimitCte at packages/common-utils/src/core/renderChartConfig.ts:1288-1301.
    • correctness, api-contract, maintainability, agent-native
  • packages/api/src/mcp/tools/dashboards/schemas.ts:50 -- The new description states only that seriesLimit requires groupBy, but the renderer also requires granularity, selectGroupBy !== false, an array select, and a real backing database and table, so an agent setting it on a metric-source tile gets silence rather than a cap.
    • Fix: Extend timeChartSeriesLimitDescription to name every precondition enforced at packages/common-utils/src/core/renderChartConfig.ts:1288-1301, including that metric and CTE sources have no backing table and ignore the field.
    • agent-native, correctness
  • .changeset/ -- No changeset accompanies this user-facing addition to the published @hyperdx/api package, which AGENTS.md:205-211 requires before pushing a PR unless the change does not warrant a release.
    • Fix: Run yarn changeset and select a patch bump for @hyperdx/api describing the newly exposed field.
    • project-standards
🔵 P3 nitpicks (4)
  • packages/api/src/utils/zod.ts:271 -- The z.number().int().positive() shape is now hand-maintained in four places (here, line 286, packages/api/src/mcp/tools/dashboards/schemas.ts:56, and twice in the OpenAPI JSDoc), while sibling fields in the same file import NumberFormatSchema, ChartPaletteTokenSchema, and BackgroundChartSchema from common-utils specifically to prevent this drift — which the type: number mismatch above shows already happened.
    • Fix: Derive all declarations from SelectSQLStatementSchema.shape.seriesLimit in packages/common-utils/src/types.ts:455 or export a single shared schema const, since neither convertToExternalTileChartConfig's return annotation nor the MCP schemas constrain an optional field added to only one surface.
  • packages/api/src/utils/zod.ts:286 -- seriesLimit has no upper bound, so a value above 2^31-1 validates and persists, then fails at render time as a ClickHouse parameter error where LIMIT ${{ Int32: seriesLimit }} binds it at packages/common-utils/src/core/renderChartConfig.ts:1380.
    • Fix: Add a .max() bound to both new declarations and the MCP schema so an out-of-range value is rejected with a 400 at the API boundary.
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:302 -- The public API now exposes one internal field under two names, seriesLimit on line and stacked_bar tiles and the pre-existing limit on pie and bar tiles, with no comment at either conversion site noting they map to the same value.
    • Fix: Add a cross-reference comment at both conversion arms and in both public descriptions so a caller switching a tile's displayType knows which field name carries the value.
  • packages/api/src/mcp/tools/dashboards/schemas.ts:56 -- The external field is .optional() while the internal field is .nullish() because the chart editor clears the value to null, so a caller echoing a config sourced from internal or exported JSON gets a 400 rather than "limit disabled".
    • Fix: Accept .nullish() on the external and MCP declarations, since _.omitBy(_.isNil) already normalizes a null to absent before the write.

Reviewers (8): correctness, testing, api-contract, maintainability, project-standards, kieran-typescript, agent-native, learnings-researcher.

Testing gaps:

  • The three new tests assert only config.seriesLimit, whereas sibling round-trip tests in the same file compare the whole tile object, so a regression corrupting groupBy or select alongside seriesLimit would pass.
  • No coverage for a raw-SQL (configType: 'sql') line or stacked_bar tile sending seriesLimit, which is silently stripped by the tile-config reparse at packages/api/src/utils/zod.ts:532-543 rather than rejected.
  • No coverage for a tile whose stored seriesLimit is explicitly null, which is the cleared state the outbound ?? undefined exists to handle.
  • No coverage for the accepted-but-never-applied case of seriesLimit set with groupBy omitted.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The diff is a small, additive pass-through of an existing internal SavedChartConfig.seriesLimit onto two external surfaces. I traced the full round-trip and it is implemented correctly:

  • Read path handles the web editor's cleared state — the UI writes seriesLimit: null (packages/app/src/components/ChartDisplaySettingsDrawer.tsx:279-281), and ?? undefined at packages/api/src/routers/external-api/v2/utils/dashboards.ts:302,319 normalizes it to an absent key so the GET body stays re-PUTtable.
  • Write path clears correctly — seriesLimit: externalConfig.seriesLimit at utils/dashboards.ts:707, then _.omitBy(_.isNil) at utils/dashboards.ts:866 with config: strippedConfig doing a full replace, so omitting the field removes the stored value.
  • Excluding the field from the raw-SQL line/stacked_bar schemas (packages/api/src/utils/zod.ts:274-281,289-294) and from mcpSqlTileSchema correctly mirrors the UI's own gate, showSeriesLimit = isTimeChart && configType !== 'sql' && configType !== 'promql' (ChartDisplaySettingsDrawer.tsx:186-187).
  • MCP persistence works because saveDashboard.ts:161-174,257-270 re-parses through createDashboardBodySchema/updateDashboardBodySchema, which resolve to the changed zod.ts schemas — the schemas.ts edit is the LLM-facing description layer.

🟡 P2 — recommended

  • packages/api/src/routers/external-api/v2/dashboards.ts:635 — the published OpenAPI description for seriesLimit states no precondition and says series are "rendered", but the field only takes effect when groupBy is set and it drops series at fetch time, so a REST caller who sets it on an ungrouped line tile gets a silent no-op with nothing in the contract explaining why.
    • Fix: Amend both seriesLimit descriptions (dashboards.ts:635 and dashboards.ts:687) to state that the field only applies when groupBy is set and is ignored otherwise, and describe it as limiting series fetched rather than rendered.

The same PR documents this correctly on the MCP surface — packages/api/src/mcp/tools/dashboards/schemas.ts:50-54 says "Maximum number of series to fetch … Requires groupBy; ignored on a chart without one" — and the UI copy agrees (ChartDisplaySettingsDrawer.tsx:274, "Maximum number of series fetched for a group-by chart"), as does the internal comment at ChartDisplaySettingsDrawer.tsx:49-51 ("On group-by time charts it drives the __hdx_series_limit CTE"). renderChartConfig.ts:88-94 carries the corresponding isUsingGroupBy guard. So the two agent/API-facing surfaces added in one PR describe one field inconsistently, and the REST one is the incomplete of the two.

🔵 P3 nitpicks (1)
  • packages/api/src/routers/external-api/v2/dashboards.ts:632LineBuilderChartConfig.seriesLimit and BarBuilderChartConfig.seriesLimit expose the same internal field that PieBuilderChartConfig.limit and CategoricalBarBuilderChartConfig.limit (dashboards.ts:833-841,886-894) expose under the name limit, so a consumer reading across displayTypes sees two names for one concept.
    • Fix: Add a one-line cross-reference in each description noting the equivalent field on the other displayType family; do not rename a published field.

Reviewers (6): correctness, testing, maintainability, project-standards, api-contract, agent-native

⚠️ Review completeness caveat. This run was degraded by the environment, and the findings above should be read as partial:

  • Bash, Grep, and Glob were non-functional for both the orchestrator and every sub-agent (bwrap: Can't create file at /home/.mcp.json, unchanged with the sandbox disabled). Read was the only working tool, so no git diff was ever obtained — the diff was reconstructed by reading the post-change files and cross-checking against the commit subjects. Untracked-file scope could not be checked.
  • Six reviewers were dispatched; only api-contract had reported when output was forced. Correctness, testing, maintainability, project-standards, and agent-native were still running, so their findings are not represented here. The P0/P1-clear verdict rests on my own trace of the round-trip, not on a completed reviewer sweep.

Testing gaps:

  • The commit log contains two test(api): focus seriesLimit round-trip assertions on the field commits, so tests exist, but I could not locate the files without Grep/Glob and therefore cannot assess what they assert. Treat the items below as unverified rather than as confirmed gaps.
  • packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts — the natural home for a converter round-trip assertion — contains no seriesLimit coverage across all 403 lines; it covers markdown tiles, container/tab validation, and orphan-ref healing only.
  • Behaviors worth confirming are covered somewhere: omitting seriesLimit on PUT clears a stored value; a stored null normalizes to absent on read; 0/negative/non-integer are rejected; and whether a seriesLimit sent on a raw-SQL line/stacked_bar tile is rejected or silently stripped (zod.ts:497-544 re-parses to strip unknown fields, so the two outcomes differ observably).
  • Unresolved: whether the committed generated artifact packages/api/openapi.json was regenerated with the new properties. The root package.json:56-60 lint-staged hook regenerates it on any packages/api/src/routers/external-api/**/*.ts change, but packages/api/package.json:107 defines lint:openapi as yarn docgen && spectral lint openapi.json — regenerating before linting — so CI would not fail on a stale committed spec. This could not be confirmed either way and is worth a manual check.

@pulpdrew
pulpdrew requested a review from wrn14897 August 3, 2026 21:41
@kodiakhq
kodiakhq Bot merged commit 2d78083 into main Aug 4, 2026
27 checks passed
@kodiakhq
kodiakhq Bot deleted the cursor/expose-series-limit-external-api-b271 branch August 4, 2026 12:15
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

No critical issues found. The change is additive, type-sound, and well-tested. Injection is not reachable (the value is bound as a ClickHouse Int32 query parameter, never interpolated), tenancy is enforced on every handler, alert evaluation does not propagate the field, openapi.json was regenerated, a changeset exists, and integration tests cover create/get, update/get, MCP save, and MCP patch for both line and stacked_bar. The items below are refinements and pre-existing gaps surfaced while tracing this diff.

🟡 P2 -- recommended

  • packages/api/src/utils/zod.ts:271 -- seriesLimit has no .max() bound, so a value above 2^31-1 passes validation, persists, and then fails the tile query at render time as a ClickHouse Int32 parameter error instead of a 400 at the boundary.
    • Fix: Add .max(2147483647) (or a product cap) to seriesLimit on both external builder schemas and to seriesLimitSchema in the MCP schemas, and mirror the bound as maximum in the OpenAPI JSDoc.
    • correctness, api-contract, security
  • packages/api/src/routers/external-api/v2/dashboards.ts:632 -- the new OpenAPI description documents no prerequisites, so a REST client that sets seriesLimit on a tile without groupBy gets a 200 and a silently ignored setting with no way to learn why.
    • Fix: State the groupBy and granularity prerequisites and the silent-no-op behavior in the seriesLimit description for both LineBuilderChartConfig and BarBuilderChartConfig, then regenerate openapi.json.
    • correctness, api-contract
  • packages/api/src/mcp/tools/dashboards/schemas.ts:50 -- the MCP description names only the groupBy requirement, but renderSeriesLimitCte also gates on granularity, selectGroupBy !== false, and a non-empty from.tableName, which is blanked for metric sources at mcp/tools/query/helpers.ts:557.
    • Fix: Extend timeChartSeriesLimitDescription to state that the setting is inert on metric-source tiles and requires a granularity, so an agent can diagnose a no-op it cannot observe in logs.
    • agent-native, api-contract
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:285 -- granularity and per-series color/colorRules are still absent from the external line/stacked_bar schemas, so a GET → mutate → PUT client keeps silently deleting them; this is the same round-trip data-loss class the diff closes for one field only.
    • Fix: Add granularity to the external line/stacked_bar schemas and color/colorRules to externalDashboardSelectItemSchema, or add a round-trip test asserting the stored internal config is deep-equal after GET → PUT so the next unmapped field fails loudly.
    • adversarial
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:3749 -- omitting seriesLimit on update is the only mechanism for clearing a stored value, and no test covers it on any of the three write surfaces.
    • Fix: Add a set-then-omit-then-get assertion for the REST PUT, MCP save, and MCP patch paths confirming the field is cleared rather than retained.
    • testing
  • packages/api/src/mcp/tools/dashboards/patchDashboard.ts:36 -- the tool description promises that unmentioned fields are preserved, but config: incoming.config replaces the tile config wholesale, so a patch touching only groupBy silently drops a previously set seriesLimit.
    • Fix: Scope the description to layout and dashboard-level fields and state that config is a full replacement, directing agents to read the current config and merge before patching.
    • correctness, agent-native
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:611 -- unrelated pre-existing bug found while tracing the converter: the MCP select item advertises isDelta, but convertToInternalSelectItem reads periodAggFn, so every MCP write persists isDelta: false and delta charts silently render raw values.
    • Fix: Normalize isDelta to periodAggFn in the MCP save and patch paths, or accept both spellings in convertToInternalSelectItem.
    • adversarial
🔵 P3 nitpicks (7)
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:2665 -- the test titled omits seriesLimit on line/stacked_bar tiles when it is not provided builds only a line tile, and the read path has separate emit sites per display type.
    • Fix: Add a stacked_bar tile to this test so the body matches its title.
  • packages/api/src/utils/zod.ts:271 -- no negative-path test pins that 0, -1, 1.5, and null are rejected as 400s rather than coerced or surfacing as 500s.
    • Fix: Add a parametrized rejection test following the existing rejects $label pattern in the dashboards integration suite.
  • packages/api/src/utils/zod.ts:532 -- seriesLimit on a raw-SQL line/stacked_bar tile, or the pie/bar spelling limit on a builder line tile, is accepted with a 200 and silently stripped by the normalizing transform.
    • Fix: Make the tile-config sub-schemas .strict(), or add a refinement rejecting seriesLimit on configType: 'sql' and limit on line/stacked_bar.
  • packages/api/src/mcp/tools/dashboards/patchDashboard.ts:167 -- the patch path calls convertToInternalTileConfig directly, bypassing the body schema that carries cross-field rules, so it can persist a tile with asRatio: true and one select item that REST would reject.
    • Fix: Parse the merged tile through externalDashboardTileSchemaWithId before conversion and return a user error on failure.
  • packages/api/src/mcp/tools/dashboards/schemas.ts:913 -- the MCP tile union is z.union rather than z.discriminatedUnion, so a config mixing raw-SQL and builder fields is resolved by declaration order and silently reshaped into a builder line tile.
    • Fix: Route on configType first and use a z.discriminatedUnion over displayType for builder tiles, mirroring the REST side.
  • packages/api/src/mcp/tools/query/timeseries.ts:32 -- clickstack_timeseries has no series-cap parameter, so an agent must persist a dashboard tile before it can preview a top-N series query.
    • Fix: Add an optional series-cap parameter to timeseriesSchema, or document that top-N preview requires the saved-tile path.
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:302 -- GET responses for line/stacked_bar tiles now expose a seriesLimit value that was already stored by the UI control but previously stripped, which a strict generated client may treat as an unexpected field.
    • Fix: Note the newly surfaced response field in the release notes.

Reviewers (10): correctness, testing, maintainability, project-standards, api-contract, security, adversarial, kieran-typescript, agent-native, learnings-researcher.

Testing gaps:

  • No coverage of clear-by-omission semantics for seriesLimit on REST PUT, MCP save, or MCP patch — the only way to unset the field.
  • No negative-value validation coverage (0, -1, 1.5, null, out-of-Int32) at either the REST or MCP schema layer.
  • The omits seriesLimit REST test exercises only line despite its title claiming both display types.
  • No full-fixture GET → PUT deep-equality test, which would catch the remaining unmapped fields (granularity, per-series color/colorRules) and future ones.

Note: Bash and network access were unavailable in this environment, so scope was reconstructed from the working tree via Read/Grep rather than from git diff; findings tied to file contents were verified directly, and two reviewer claims (missing changeset, integration tests not gating CI) were retracted after verification.

brandon-pereira added a commit that referenced this pull request Aug 4, 2026
PR #2772 (merged into this branch via main) added seriesLimit to the
external Line/StackedBar tile schemas as positive-only, and the converter
emitted `config.seriesLimit ?? undefined`. This branch relaxes the internal
schema to allow 0 (unlimited), so a stored 0 would serialize verbatim and be
rejected on a GET->PUT round-trip by the positive-only write schema. Gate both
cases on hasPositiveSeriesLimit so 0/null map to absent, matching the existing
Pie/Bar limit mappings. Adds unit coverage for 0/null/positive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants