feat: Support dashboard variables in MCP server - #2951
Conversation
🦋 Changeset detectedLatest commit: 7a41daf The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThis PR extends MCP dashboard tools with variable-aware filter schemas, validation, substitution, and tile-query inputs.
Confidence Score: 4/5The PR is not yet safe to merge because omitted aggregate-condition languages can still generate malformed SQL for variable-driven builder tiles. Variable substitution defaults an omitted aggregate-condition language to Lucene, while the all-select-condition WHERE pushdown defaults it to SQL and passes the expanded expression through unchanged, so reachable MCP tile queries can fail in ClickHouse. Files Needing Attention: packages/common-utils/src/variables.ts and packages/common-utils/src/core/renderChartConfig.ts
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/variables.ts | Adds dashboard-variable extraction and substitution, but omitted aggregate-condition languages remain inconsistent with the renderer's WHERE pushdown. |
| packages/api/src/mcp/tools/dashboards/variables.ts | Resolves declared dashboard variables and validates caller-supplied selections before tile execution. |
| packages/api/src/mcp/tools/dashboards/queryTile.ts | Threads resolved variable values into individual dashboard tile queries. |
| packages/api/src/mcp/tools/dashboards/queryTiles.ts | Applies the same variable-resolution contract to bounded batch tile execution. |
| packages/api/src/mcp/tools/dashboards/validation.ts | Adds non-blocking warnings for unknown, bare, and dependent variable references. |
| packages/common-utils/src/filters.ts | Extends shared dashboard-filter behavior and variable-name handling used by MCP validation and rendering. |
Sequence Diagram
sequenceDiagram
participant Agent as MCP agent
participant Tool as Dashboard query tool
participant Vars as Variable resolver
participant Render as Chart renderer
participant CH as ClickHouse
Agent->>Tool: query tile + variableValues
Tool->>Vars: resolve dashboard variables
Vars-->>Tool: names, expressions, selected values
Tool->>Render: chart config + variables
Render->>Render: substitute variable macros
Render->>CH: rendered ClickHouse query
CH-->>Tool: tile result
Tool-->>Agent: result and warnings
Comments Outside Diff (1)
-
packages/common-utils/src/variables.ts, line 544 (link)Conflicting aggregate language defaults
When every builder select item has a variable-bearing aggregate condition and omits
aggConditionLanguage, substitution expands the condition as Lucene while the WHERE pushdown consumes it as raw SQL, producing malformed ClickHouse syntax such asServiceName:("api" OR "web")and causing the tile query to fail.Knowledge Base Used:
Reviews (2): Last reviewed commit: "feat: Support dashboard variables in MCP..." | Re-trigger Greptile
E2E Test Results✅ All tests passed • 307 passed • 1 skipped • 1186s
Tests ran across 4 shards in parallel. |
Deep Review✅ No P0/P1 issues found. The change is well-tested (large additions across 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (10): correctness, testing, maintainability, project-standards, api-contract, security, adversarial, kieran-typescript, agent-native, learnings-researcher. Coverage note: The dispatched persona-reviewer sub-agents had not returned their findings at synthesis time, so the findings above come from direct analysis of the diff and the surrounding schema/expansion code; they were verified against the code paths cited. The new behavior is otherwise accompanied by substantial test coverage, and the CSV-format non-escaping in Testing gaps: No test asserts the documented "Rejected when isVariableEnabled is not true" / "rejected when isBroadcastEnabled is false" filter constraints — consistent with those constraints not being enforced. |
70b5f03 to
e283e7d
Compare
e283e7d to
7a41daf
Compare
|
Are we planning to merge this only after we remove the feature flags? I am concerned agents might start using this feature before its allowed. |
Correct, we will merge after enabling the feature. Sorry, should have written that in the description. I'll put this in draft mode but would appreciate your review still, so I can get started on addressing any feedback you may have. I expect we can remove the toggle early next week. |
|
PR LGTM otherwise! Thanks for clarifying |
Summary
This PR updates the MCP tools and prompts to include support for the new dashboard variables feature.
This is to be merged only after removing the toggles guarding the dashboard variables feature.
Screenshots or video
How to test locally
Fire up your agent and ask it to build a dashboard that makes use of dashboard variables.
References