Repository navigation
feat(deck): add table dataset transforms - #778
Conversation
📝 WalkthroughWalkthroughAdds structured table-backed dataset inputs with optional ChangesTable dataset input flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant resolveDeckMapDashboardDatasetSource
participant createDeckMapDashboardDatasetQuery
participant createDeckTableDatasetSql
participant DuckDB
Dashboard->>resolveDeckMapDashboardDatasetSource: resolve dataset source
resolveDeckMapDashboardDatasetSource-->>Dashboard: SQL source or table source
Dashboard->>createDeckMapDashboardDatasetQuery: build query from resolved source
alt table source with transformSql
createDeckMapDashboardDatasetQuery->>createDeckTableDatasetSql: compile tableName + transformSql
createDeckTableDatasetSql-->>createDeckMapDashboardDatasetQuery: SQL with __sqlrooms_source
else SQL source
createDeckMapDashboardDatasetQuery->>createDeckMapDashboardDatasetQuery: use sqlQuery as-is
end
createDeckMapDashboardDatasetQuery->>DuckDB: execute compiled SQL
DuckDB-->>Dashboard: result rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e490509b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return [ | ||
| `WITH ${DECK_TABLE_DATASET_SOURCE_RELATION} AS (`, | ||
| ` SELECT * FROM ${tableReference}`, | ||
| ')', | ||
| transformSql, |
There was a problem hiding this comment.
When transformSql itself starts with WITH (for example, a transform using a helper CTE before selecting from __sqlrooms_source), this concatenates it after SQLRooms' own WITH and emits WITH __sqlrooms_source AS (...) WITH nested AS (...) SELECT ..., which is not valid DuckDB SQL. The new API describes transformSql as a complete SELECT and the added test even keeps a nested WITH, so those transformed datasets will fail at runtime instead of loading.
Useful? React with 👍 / 👎.
| const baseTableName = | ||
| dashboardTable || | ||
| stripCatalogPrefix( | ||
| datasetSource && 'tableName' in datasetSource | ||
| ? datasetSource.tableName | ||
| : undefined, | ||
| ); |
There was a problem hiding this comment.
When there is no dashboard.selectedTable, this fallback now passes the dataset's own source.tableName through stripCatalogPrefix, which drops the database/catalog from a three-part table identity. For a saved map whose dataset points at an attached table such as "remote"."main"."events", the resolver returns "main"."events", so the generated query reads from the default database (or fails) instead of the authored source. Only dashboard-selected default-database identities should be normalized; the dataset source fallback should remain intact.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/deck/src/MapSettings.tsx (1)
201-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse existing dashboard dataset-source guards instead of re-deriving the discriminant.
activeLayerDatasetSourceis typed asDeckMapDashboardDatasetSource, which already hasisDeckMapDashboardSqlDatasetSource/isDeckMapDashboardTableDatasetSourceguards exported from./dashboardConfig(and re-exported from the package index). Reimplementing the same'tableName' in/'sqlQuery' inchecks here duplicates that logic and risks drifting if the union shape changes.♻️ Proposed refactor
- const activeLayerDatasetSource = activeLayerDataset?.source; - const fallbackTableName = - activeLayerDatasetSource && 'tableName' in activeLayerDatasetSource - ? activeLayerDatasetSource.tableName - : activeLayerDatasetSource && 'sqlQuery' in activeLayerDatasetSource - ? extractTableFromSqlQuery(activeLayerDatasetSource.sqlQuery) - : undefined; + const activeLayerDatasetSource = activeLayerDataset?.source; + const fallbackTableName = isDeckMapDashboardTableDatasetSource( + activeLayerDatasetSource, + ) + ? activeLayerDatasetSource.tableName + : isDeckMapDashboardSqlDatasetSource(activeLayerDatasetSource) + ? extractTableFromSqlQuery(activeLayerDatasetSource.sqlQuery) + : undefined;Please confirm whether these guards are already imported in this file (not shown in the provided context) before applying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/MapSettings.tsx` around lines 201 - 207, The fallback table-name logic in MapSettings.tsx is re-deriving the dataset-source discriminant with inline `'tableName' in` / `'sqlQuery' in` checks; replace that with the existing `isDeckMapDashboardTableDatasetSource` and `isDeckMapDashboardSqlDatasetSource` guards. First confirm those guards are already imported in this file, and if not, import them from `./dashboardConfig` or the package index, then update the `activeLayerDatasetSource` handling to use the guards before calling `extractTableFromSqlQuery`.packages/deck/src/dashboardConfig.ts (1)
34-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing tsdoc on new public exports.
DeckMapDashboardDatasetSource,isDeckMapDashboardSqlDatasetSource, andisDeckMapDashboardTableDatasetSourceare new exported symbols without tsdoc comments.As per coding guidelines, "Add tsdoc to exported types and functions in
@sqlrooms/*packages."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/dashboardConfig.ts` around lines 34 - 52, Add tsdoc comments for the new public exports in dashboardConfig: DeckMapDashboardDatasetSource, isDeckMapDashboardSqlDatasetSource, and isDeckMapDashboardTableDatasetSource. Update the exported type and both type-guard functions with brief tsdoc describing what each represents and what the guards narrow to, following the `@sqlrooms/`* documentation guideline.Source: Coding guidelines
packages/deck/src/useDeckMapFitToBounds.ts (2)
217-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new bounds-SQL branching and error translation.
This function's branching (sql source vs table source, plus the tableName-error remap) has no accompanying tests in the reviewed files, and no
useDeckMapFitToBounds.test.tsappears in the PR outline. As per path instructions,**/*.{js,ts,jsx,tsx}: "Add or update tests when changing behavior or public API, when practical."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/useDeckMapFitToBounds.ts` around lines 217 - 231, Add unit tests around createDeckMapBoundsTableSourceSql and the useDeckMapFitToBounds behavior to cover the new branching logic. Verify the sqlQuery path uses the SQL source branch, the table source path delegates through createDeckTableDatasetSql, and the specific tableName error is translated to “Deck map fit-to-data requires a valid table source.” Also add coverage in the useDeckMapFitToBounds test suite for the bounds-SQL selection and error remapping.Source: Path instructions
217-231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile cross-file error matching.
createDeckMapBoundsTableSourceSqlre-maps errors by comparing the rawerror.messagestring literal ('Deck table dataset input requires a valid tableName.') against the exact text thrown indatasets/tableDatasetSql.ts. If that message is ever reworded, this silently falls back to the genericthrow error, losing the fit-to-data-specific message with no compile-time signal.♻️ Suggested fix: use an error code/type instead of message text
- } catch (error) { - if ( - error instanceof Error && - error.message === 'Deck table dataset input requires a valid tableName.' - ) { - throw new Error('Deck map fit-to-data requires a valid table source.'); - } - throw error; - } + } catch (error) { + if (error instanceof InvalidTableNameError) { + throw new Error('Deck map fit-to-data requires a valid table source.'); + } + throw error; + }Export a dedicated
InvalidTableNameErrorclass (or an errorcodeproperty) fromtableDatasetSql.tsand throw/check against that instead of matching the message string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/useDeckMapFitToBounds.ts` around lines 217 - 231, `createDeckMapBoundsTableSourceSql` is relying on a hard-coded `error.message` string from `createDeckTableDatasetSql`, which is fragile across files. Replace the message comparison with a stable shared signal from `datasets/tableDatasetSql.ts`, such as an exported `InvalidTableNameError` class or an error `code`, then update `createDeckMapBoundsTableSourceSql` to check that type/code before rethrowing the fit-to-data-specific error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/deck/src/dashboard.tsx`:
- Around line 282-289: The Mosaic client cache key in dashboard.tsx is using
dashboard.selectedTable for SQL dataset sources, which causes pinned sqlQuery
sources to remount when the table selector changes. Update the sourceKey logic
in the dashboard source key computation so it branches on
isDeckMapDashboardSqlDatasetSource(source) and uses source.sqlQuery for SQL
sources, while keeping table sources keyed by source.tableName and the fallback
only for non-dataset sources. Ensure the sourceQueryKey logic remains aligned
with the same source-type checks.
In `@packages/deck/src/datasets/helpers.ts`:
- Around line 85-102: The cache-key path in resolvePreparedDatasetCacheKey() can
throw when createDeckTableDatasetSql() compiles a table dataset, which bypasses
ensureEntry()’s per-dataset handling and can fail syncDatasetsForConsumer() for
the whole batch. Update the SQL-backed branch in
resolvePreparedDatasetCacheKey() to guard the createDeckTableDatasetSql(input)
call for isTableDatasetInput(input), either by catching the error and returning
undefined or otherwise isolating the failure to that dataset, while keeping the
existing sqlSourceIdentity and buildGeometryKey behavior intact.
In `@packages/deck/src/mapConfigUtils.ts`:
- Around line 271-289: The coordinates + transformSql path in mapConfigUtils
drops the caller-provided tableReference and falls back to tableName only.
Update the branch that builds the table-based dataset source to preserve
options.tableReference when present, using the same reference used in the
sqlQuery path and threading it through the table dataset construction logic if
needed. Check createDeckMapDashboardConfigForTable and related helpers like
createDeckTableDatasetSql to ensure tableReference is not discarded when
coordinates are enabled without sourceSqlQuery.
---
Nitpick comments:
In `@packages/deck/src/dashboardConfig.ts`:
- Around line 34-52: Add tsdoc comments for the new public exports in
dashboardConfig: DeckMapDashboardDatasetSource,
isDeckMapDashboardSqlDatasetSource, and isDeckMapDashboardTableDatasetSource.
Update the exported type and both type-guard functions with brief tsdoc
describing what each represents and what the guards narrow to, following the
`@sqlrooms/`* documentation guideline.
In `@packages/deck/src/MapSettings.tsx`:
- Around line 201-207: The fallback table-name logic in MapSettings.tsx is
re-deriving the dataset-source discriminant with inline `'tableName' in` /
`'sqlQuery' in` checks; replace that with the existing
`isDeckMapDashboardTableDatasetSource` and `isDeckMapDashboardSqlDatasetSource`
guards. First confirm those guards are already imported in this file, and if
not, import them from `./dashboardConfig` or the package index, then update the
`activeLayerDatasetSource` handling to use the guards before calling
`extractTableFromSqlQuery`.
In `@packages/deck/src/useDeckMapFitToBounds.ts`:
- Around line 217-231: Add unit tests around createDeckMapBoundsTableSourceSql
and the useDeckMapFitToBounds behavior to cover the new branching logic. Verify
the sqlQuery path uses the SQL source branch, the table source path delegates
through createDeckTableDatasetSql, and the specific tableName error is
translated to “Deck map fit-to-data requires a valid table source.” Also add
coverage in the useDeckMapFitToBounds test suite for the bounds-SQL selection
and error remapping.
- Around line 217-231: `createDeckMapBoundsTableSourceSql` is relying on a
hard-coded `error.message` string from `createDeckTableDatasetSql`, which is
fragile across files. Replace the message comparison with a stable shared signal
from `datasets/tableDatasetSql.ts`, such as an exported `InvalidTableNameError`
class or an error `code`, then update `createDeckMapBoundsTableSourceSql` to
check that type/code before rethrowing the fit-to-data-specific error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19e090f9-a4d5-489d-ae3f-6c78a3d9af1e
📒 Files selected for processing (18)
packages/deck/README.mdpackages/deck/__tests__/dashboard-ai-map.test.tspackages/deck/__tests__/dashboard.test.tspackages/deck/__tests__/normalizeDatasets.test.tspackages/deck/__tests__/preparedDatasetStore.test.tspackages/deck/__tests__/tableDatasetSql.test.tspackages/deck/src/MapSettings.tsxpackages/deck/src/ai.tspackages/deck/src/dashboard.tsxpackages/deck/src/dashboardConfig.tspackages/deck/src/datasets/PreparedDatasetStore.tspackages/deck/src/datasets/helpers.tspackages/deck/src/datasets/normalizeDatasets.tspackages/deck/src/datasets/tableDatasetSql.tspackages/deck/src/index.tspackages/deck/src/mapConfigUtils.tspackages/deck/src/types.tspackages/deck/src/useDeckMapFitToBounds.ts
| return `SELECT * FROM ${tableReference}`; | ||
| } | ||
|
|
||
| const transformSql = cleanTransformSql(input.transformSql); |
There was a problem hiding this comment.
reateDeckTableDatasetSql produces invalid SQL when transformSql contains CTEs
When transformSql starts with WITH (e.g. path aggregation queries the AI instructions teach), the output is:
WITH __sqlrooms_source AS (...)
WITH nested AS (...) SELECT ...
Two WITH keywords — invalid SQL. The fix is to detect the leading WITH and merge it as a comma-separated CTE:
const leadingWithRegex = /^\s*WITH\s+/i;
const mergedTransformSql = leadingWithRegex.test(transformSql)
? transformSql.replace(leadingWithRegex, ', ')
: transformSql;
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab264d7895
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isDeckMapDashboardSqlDatasetSource(datasetSource)) { | ||
| return datasetSource; |
There was a problem hiding this comment.
When loading dashboard maps saved before this change, generated lon/lat panels stored their data source as source.sqlQuery (and some configs also carried source.tableName) and relied on this resolver to swap the dashboard-selected table at runtime. This early return now pins every SQL source, so those existing panels keep querying the originally saved table when the user changes dashboard.selectedTable until they are manually regenerated. Please add a migration or compatibility path for the old generated shape before treating arbitrary SQL as pinned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/deck/src/block.tsx (1)
130-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for new settings-panel toggle behavior.
This adds a new prop (
selected), a derived toggle state, a click handler, and header UI — with no accompanying test updates in this cohort's files. As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."Want me to draft a test covering: button hidden/disabled semantics when unselected,
aria-pressedtoggling, andrequestOpenSettingsPanel/requestCloseSettingsPanelbeing called correctly on click?Also applies to: 228-235, 274-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/block.tsx` around lines 130 - 139, Add or update tests for the new settings-panel toggle behavior in the Deck block component: cover the new selected-driven state in Block/BlockHeader, verify the settings button is hidden or disabled when unselected, assert aria-pressed reflects the open/closed state, and confirm the click handler calls requestOpenSettingsPanel and requestCloseSettingsPanel correctly based on current state. Locate the behavior via Block, BlockHeader, and the requestOpenSettingsPanel/requestCloseSettingsPanel hooks in block.tsx, and extend the relevant component tests accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/deck/src/block.tsx`:
- Around line 130-139: Add or update tests for the new settings-panel toggle
behavior in the Deck block component: cover the new selected-driven state in
Block/BlockHeader, verify the settings button is hidden or disabled when
unselected, assert aria-pressed reflects the open/closed state, and confirm the
click handler calls requestOpenSettingsPanel and requestCloseSettingsPanel
correctly based on current state. Locate the behavior via Block, BlockHeader,
and the requestOpenSettingsPanel/requestCloseSettingsPanel hooks in block.tsx,
and extend the relevant component tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e4b8b6b-fa25-422d-b478-548181c88637
📒 Files selected for processing (4)
apps/sqlrooms-cli-ui/src/workspace/WorksheetMapBlockRenderer.tsxpackages/deck/src/block.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsxpackages/documents/src/BlockDocumentStatefulBlockRendererContext.tsx
Summary
Adds a structured Deck table dataset input for
@sqlrooms/deckso maps can express direct table sources and table-relative transforms without rewriting authored SQL strings.What changed
DeckTableDatasetInput,isTableDatasetInput, andcreateDeckTableDatasetSqlwith the reserved__sqlrooms_sourcerelation.sqlQueryFROMrewrites; literalsqlQuerysources now stay pinned while structuredtableNamesources follow dashboard table selection.tableName + transformSql.FROMclauses and structured source swapping.Validation
pnpm buildpnpm --filter @sqlrooms/deck buildpnpm --filter @sqlrooms/deck test -- --runTestsByPath packages/deck/__tests__/normalizeDatasets.test.ts packages/deck/__tests__/tableDatasetSql.test.ts packages/deck/__tests__/dashboard.test.ts packages/deck/__tests__/preparedDatasetStore.test.ts packages/deck/__tests__/dashboard-ai-map.test.tsknip,turbo typecheck,npx madge --exclude "dist" --circular packages/*/src/index.ts,turbo testNotes
Created the follow-up Obsidian plan
SQLRooms Generated Column Aware Map Selector Cleanup Planfor the generated-column-aware settings cleanup that the implementation plan intentionally scoped out.Summary by CodeRabbit
transformSql, including map point geometry generation from table coordinates.tableName/transformSqlrequirements.