fix(app): hydrate source fields for raw SQL tiles in all chart rendering contexts#2503
fix(app): hydrate source fields for raw SQL tiles in all chart rendering contexts#2503pulpdrew wants to merge 1 commit into
Conversation
…onfig Raw SQL tiles referencing source-dependent macros (, ) failed with 'Macro requires a source to be selected' when rendered outside the Dashboard Tile component (e.g. in notebook tiles), because the saved config only stores the source ID — the runtime fields (from, metricTables, etc.) were not merged before query execution. Add hydrateRawSqlConfigFromSource() to useQueriedChartConfig and useRenderedSqlChartConfig hooks. This mirrors the hydration pattern already established in DBDashboardPage's Tile useEffect, ensuring macros resolve correctly in all rendering contexts. Fixes HDX-4594 Co-authored-by: Drew Davis <pulpdrew@gmail.com>
🦋 Changeset detectedLatest commit: b5ebff2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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.
|
Greptile SummaryThis PR fixes raw SQL tiles that used source-dependent macros (
Confidence Score: 4/5Safe to merge; the change correctly extends source hydration to all chart rendering contexts with no risk to non-raw-SQL paths. The implementation is correct and the helper is a no-op for non-raw-SQL configs. The isSourceLoading gate was already present before this PR. The only gap is a dropped explanatory comment. No files require special attention beyond the minor dropped comment in useChartConfig.tsx. Important Files Changed
Reviews (1): Last reviewed commit: "fix(app): hydrate source fields for raw ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const sql = parameterizedQueryToSql(query); | ||
| // sql-formatter can't handle prometheusQuery() / CTE syntax in PromQL queries | ||
| if (isPromqlChartConfig(config)) { | ||
| if (isPromqlChartConfig(hydratedConfig)) { |
There was a problem hiding this comment.
The comment explaining why PromQL queries skip format() was removed when config was renamed to hydratedConfig. The formatter still cannot handle prometheusQuery() / CTE PromQL syntax, so the guard still needs the explanation. Without it, future readers may not understand why PromQL is returned early.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
E2E Test Results✅ All tests passed • 219 passed • 3 skipped • 1384s
Tests ran across 4 shards in parallel. |
2 participants
Summary
Raw SQL tiles referencing source-dependent macros (
$__sourceTable,$__filters) failed with "Macro '$__sourceTable' requires a source to be selected" when rendered outside the Dashboard Tile component (e.g. in notebook tiles). This happened because the saved config only stores the source ID — the runtime fields (from,metricTables,implicitColumnExpression, etc.) were never merged before query execution in those contexts.The fix adds a
hydrateRawSqlConfigFromSource()helper to theuseQueriedChartConfiganduseRenderedSqlChartConfighooks. This mirrors the hydration pattern already established inDBDashboardPage's TileuseEffect, ensuring macros resolve correctly in all chart rendering contexts (dashboards, notebooks, chart explorer, SQL preview).Key design decisions:
How to test on Vercel preview
Preview routes: /chart
Steps:
$__sourceTable(e.g.SELECT count() FROM $__sourceTable)References
Linear Issue: HDX-4594