Repository navigation
fix(deck): resolve generated map selector columns - #783
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds dataset schema inspection helpers and hook, wires schema-derived columns into map settings, adds a layer timestamp-column setter, and updates dashboard missing-column handling. ChangesDeck Map Dataset Schema
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 8b792db1ae
ℹ️ 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".
| const dataOutputColumns = options.outputColumns.filter( | ||
| (column) => !isDeckMapGeneratedColumn(column.name), | ||
| ); |
There was a problem hiding this comment.
When the map uses a direct table source, useDeckMapDatasetSchema passes the table's sourceColumns through as outputColumns, so this name-only filter removes legitimate user columns named timestamps, source_geom, or target_geom from dataOutputColumns. Since MapSettings uses dataOutputColumns for the color and elevation selectors, a valid table with a real timestamps metric plus other columns can no longer select that field. The generated-column split should be gated to inspected transformed outputs or compare against sourceColumns so actual source fields remain selectable.
Useful? React with 👍 / 👎.
8b792db to
c781229
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c781229227
ℹ️ 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".
| const isGeneratedOutputColumn = (column: TableColumn) => | ||
| isDeckMapGeneratedColumn(column.name) && | ||
| !sourceColumnNames.has(column.name); |
There was a problem hiding this comment.
For literal SQL dataset sources, MapSettings now inspects the query output but passes either no source columns or the dashboard-selected table's unrelated columns, so this name-based filter treats legitimate SQL aliases such as timestamps, source_geom, or target_geom as generated whenever the selected/source table does not contain the same name. In that case dataOutputColumns drops the alias, and the color/elevation selectors cannot choose it even though it is a real output of the pinned SQL query. The generated-column split should be disabled for opaque SQL sources or based on actual transform context rather than an empty/unrelated sourceColumns set.
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: 1
🧹 Nitpick comments (5)
packages/deck/src/dashboard.tsx (1)
180-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate JSX between generated-column and generic missing-column branches.
The blocks at Lines 184-203 and 206-222 are nearly identical, differing only in the lead sentence and the column list used. Consider extracting a shared render helper to avoid drift between the two messages.
♻️ Proposed refactor
+function MissingColumnsMessage({ + intro, + columns, +}: { + intro: string; + columns: string[]; +}) { + return ( + <div className="flex h-full min-h-[200px] flex-col items-center justify-center p-4"> + <div className="mb-2 text-center font-semibold"> + The visualization can't be displayed + </div> + <div className="text-center text-sm"> + <span>{intro} </span> + {columns.map((col, idx) => ( + <span key={idx}> + <span className="inline-flex items-center rounded-md border border-gray-600 bg-gray-800 px-1 py-0.5 text-xs font-medium text-gray-300"> + {col} + </span>{' '} + </span> + ))} + </div> + </div> + ); +}Then in
DeckMapRuntimeIssuePanel:if (generatedMissingColumns.length > 0) { - return ( - <div className="flex h-full min-h-[200px] flex-col items-center justify-center p-4"> - ... - </div> - ); + return ( + <MissingColumnsMessage + intro="The dataset transform did not produce required map output columns:" + columns={generatedMissingColumns} + /> + ); } - return ( - <div className="flex h-full min-h-[200px] flex-col items-center justify-center p-4"> - ... - </div> - ); + return ( + <MissingColumnsMessage + intro="Selected columns are missing in the dataset:" + columns={missingColumns} + /> + );🤖 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/dashboard.tsx` around lines 180 - 222, The missing-column UI in DeckMapRuntimeIssuePanel duplicates nearly the same JSX in the generated-column and generic branches. Extract a shared render helper or small presentational component inside dashboard.tsx and have the existing missingColumns/generatedMissingColumns branches pass only the message text and column list so the layout stays consistent and avoids future drift.packages/deck/src/mapLayerConfigUtils.ts (1)
326-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLogic is correct; missing tsdoc.
setDeckMapLayerTimestampColumncorrectly mirrors thesetDeckMapLayerArcColumnspattern (merge into existing_sqlroomsBinding). It's missing a tsdoc comment though.As per path instructions, "Add tsdoc to exported types and functions in
@sqlrooms/*packages" forpackages/**/*.{ts,tsx}.🤖 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/mapLayerConfigUtils.ts` around lines 326 - 338, Add a tsdoc comment for the exported function setDeckMapLayerTimestampColumn in mapLayerConfigUtils so it matches the documentation style used by setDeckMapLayerArcColumns and other exported APIs in `@sqlrooms` packages. Keep the comment brief and describe that it updates the selected deck map layer’s _sqlroomsBinding by setting timestampColumn while preserving any existing binding fields.Source: Path instructions
packages/deck/src/useDeckMapDatasetSchema.ts (2)
12-17: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded generated-column names are a fragile heuristic.
GENERATED_COLUMN_NAMEShardcodes 4 names that must exactly match whatever the actual geometry/H3/arc/trips compiler emits. This is only saved from misclassifying real user data by the!sourceColumnNames.has(column.name)guard inresolveDeckMapDatasetSchema, but a custom pinned SQL query (or transform) that legitimately produces a column literally namedtimestamps,source_geom, ortarget_geom(not present in the raw source table) will still be silently treated as "generated" and excluded from data selectors, with no way for the user to override it.Consider sourcing this list from wherever the compiler actually assigns these aliases (single source of truth) instead of duplicating the literal strings here, to avoid drift and false positives.
Also applies to: 61-72
🤖 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/useDeckMapDatasetSchema.ts` around lines 12 - 17, The generated-column detection in useDeckMapDatasetSchema is relying on a hardcoded GENERATED_COLUMN_NAMES set, which can misclassify legitimate columns like timestamps, source_geom, and target_geom. Update resolveDeckMapDatasetSchema to use a single source of truth for compiler-assigned aliases instead of duplicating literals here, and wire the alias list from the same place that defines the geometry/H3/arc/trips compiler output so the selector filtering stays in sync and avoids false positives.
19-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing tsdoc on new public exports.
DeckMapResolvedDatasetSchema,DeckMapDatasetSchemaState,isDeckMapGeneratedColumn,createDeckMapDatasetOutputSchemaSql,resolveDeckMapDatasetSchema, anduseDeckMapDatasetSchemaare all exported but lack tsdoc comments.As per path instructions, "Add tsdoc to exported types and functions in
@sqlrooms/*packages" forpackages/**/*.{ts,tsx}.Also applies to: 47-55, 57-80, 100-103
🤖 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/useDeckMapDatasetSchema.ts` around lines 19 - 33, Add tsdoc comments to every new public export in this module so it follows the `@sqlrooms/*` package rule for exported types and functions. Update the exported symbols `DeckMapResolvedDatasetSchema`, `DeckMapDatasetSchemaState`, `isDeckMapGeneratedColumn`, `createDeckMapDatasetOutputSchemaSql`, `resolveDeckMapDatasetSchema`, and `useDeckMapDatasetSchema` with concise documentation describing what each type/function represents and does, and keep the comments directly above the corresponding declarations so they remain attached even if the code moves.Source: Path instructions
packages/deck/__tests__/useDeckMapDatasetSchema.test.ts (1)
1-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the pure helpers; hook itself is untested.
The SQL-building and schema-resolution helper tests are thorough. However,
useDeckMapDatasetSchemaitself — with its non-trivialuseEffect/AbortController/state-transition logic — has no direct test (e.g. viarenderHook+ a mockeduseDuckDbconnector) to verify loading/error/stale-request behavior.As per coding guidelines, "Add or update tests when changing behavior or public API, when practical" for
**/*.{js,ts,jsx,tsx}.🤖 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/__tests__/useDeckMapDatasetSchema.test.ts` around lines 1 - 92, Add direct coverage for useDeckMapDatasetSchema itself, not just its pure helpers, because its useEffect, AbortController, and state transitions are currently untested. Create a hook test using renderHook with a mocked useDuckDb connector to verify the loading, success, error, and stale-request/abort behavior of useDeckMapDatasetSchema, alongside the existing helper coverage.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.
Inline comments:
In `@packages/deck/src/MapSettings.tsx`:
- Around line 218-226: The `dataOutputColumns` fallback in `MapSettings` should
not reuse `outputColumns`, because that reintroduces generated/geometry-only
columns into selectors that should exclude them. Update the
`useDeckMapDatasetSchema` handling so `dataOutputColumns` stays empty (or
otherwise filtered to non-generated columns) when
`datasetSchema.dataOutputColumns` is empty, and make `firstColumnName` use the
same safe data-only set. Also surface `datasetSchema.isLoading` and
`datasetSchema.error` in this panel near the dependent field sections instead of
only gating on `outputColumns.length`, so schema loading/failure is visible
rather than silently hiding fields.
---
Nitpick comments:
In `@packages/deck/__tests__/useDeckMapDatasetSchema.test.ts`:
- Around line 1-92: Add direct coverage for useDeckMapDatasetSchema itself, not
just its pure helpers, because its useEffect, AbortController, and state
transitions are currently untested. Create a hook test using renderHook with a
mocked useDuckDb connector to verify the loading, success, error, and
stale-request/abort behavior of useDeckMapDatasetSchema, alongside the existing
helper coverage.
In `@packages/deck/src/dashboard.tsx`:
- Around line 180-222: The missing-column UI in DeckMapRuntimeIssuePanel
duplicates nearly the same JSX in the generated-column and generic branches.
Extract a shared render helper or small presentational component inside
dashboard.tsx and have the existing missingColumns/generatedMissingColumns
branches pass only the message text and column list so the layout stays
consistent and avoids future drift.
In `@packages/deck/src/mapLayerConfigUtils.ts`:
- Around line 326-338: Add a tsdoc comment for the exported function
setDeckMapLayerTimestampColumn in mapLayerConfigUtils so it matches the
documentation style used by setDeckMapLayerArcColumns and other exported APIs in
`@sqlrooms` packages. Keep the comment brief and describe that it updates the
selected deck map layer’s _sqlroomsBinding by setting timestampColumn while
preserving any existing binding fields.
In `@packages/deck/src/useDeckMapDatasetSchema.ts`:
- Around line 12-17: The generated-column detection in useDeckMapDatasetSchema
is relying on a hardcoded GENERATED_COLUMN_NAMES set, which can misclassify
legitimate columns like timestamps, source_geom, and target_geom. Update
resolveDeckMapDatasetSchema to use a single source of truth for
compiler-assigned aliases instead of duplicating literals here, and wire the
alias list from the same place that defines the geometry/H3/arc/trips compiler
output so the selector filtering stays in sync and avoids false positives.
- Around line 19-33: Add tsdoc comments to every new public export in this
module so it follows the `@sqlrooms/*` package rule for exported types and
functions. Update the exported symbols `DeckMapResolvedDatasetSchema`,
`DeckMapDatasetSchemaState`, `isDeckMapGeneratedColumn`,
`createDeckMapDatasetOutputSchemaSql`, `resolveDeckMapDatasetSchema`, and
`useDeckMapDatasetSchema` with concise documentation describing what each
type/function represents and does, and keep the comments directly above the
corresponding declarations so they remain attached even if the code moves.
🪄 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: 24a88c14-bf6a-4f4d-acd3-e9f8d636301a
📒 Files selected for processing (5)
packages/deck/__tests__/useDeckMapDatasetSchema.test.tspackages/deck/src/MapSettings.tsxpackages/deck/src/dashboard.tsxpackages/deck/src/mapLayerConfigUtils.tspackages/deck/src/useDeckMapDatasetSchema.ts
c781229 to
9ff7ab4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff7ab47b5
ℹ️ 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".
| function arrowSchemaToTableColumns(table: arrow.Table): TableColumn[] { | ||
| return table.schema.fields.map((field) => ({ | ||
| name: field.name, | ||
| type: String(field.type), |
There was a problem hiding this comment.
For transformed table sources and pinned SQL, the settings panel now uses this inspected schema for dataOutputColumns, but String(field.type) produces Apache Arrow names such as Float64, Int32, and Utf8. The column selectors used for color and elevation filter by exact DuckDB type names (DOUBLE, INTEGER, VARCHAR, etc.), so numeric/text output columns from inspected datasets are filtered out and the selectors show no matching columns even though the query output has valid fields. This affects maps whose dataset source has transformSql or sqlQuery; convert Arrow types to the same DuckDB-style names used by table schemas before passing them to ColumnSelector.*.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/deck/src/useDeckMapDatasetSchema.ts (2)
84-93: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated transformSql-truthiness check.
shouldInspectOutputSchema's table-source branch and theclassifyGeneratedColumnscomputation both independently re-deriveisDeckMapDashboardTableDatasetSource(source) && source.transformSql?.trim(). Consider extracting a sharedhasTableTransformSql(source)helper to avoid drift if the condition changes.
[recommended_refactor]♻️ Proposed refactor
+function hasTableTransformSql( + source: DeckMapDashboardDatasetSource | undefined, +): source is Pick<DeckTableDatasetInput, 'tableName' | 'transformSql'> { + return Boolean( + source && + isDeckMapDashboardTableDatasetSource(source) && + source.transformSql?.trim(), + ); +} function shouldInspectOutputSchema( source: DeckMapDashboardDatasetSource | undefined, ) { return Boolean( source && - (isDeckMapDashboardSqlDatasetSource(source) || - (isDeckMapDashboardTableDatasetSource(source) && - source.transformSql?.trim())), + (isDeckMapDashboardSqlDatasetSource(source) || hasTableTransformSql(source)), ); } ... - classifyGeneratedColumns: - isDeckMapDashboardTableDatasetSource(options.source) && - Boolean(options.source.transformSql?.trim()), + classifyGeneratedColumns: hasTableTransformSql(options.source),Also applies to: 265-274
🤖 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/useDeckMapDatasetSchema.ts` around lines 84 - 93, The transformSql truthiness check is duplicated between shouldInspectOutputSchema and classifyGeneratedColumns, which risks the two branches drifting apart. Extract a shared helper such as hasTableTransformSql(source) and use it in both places, keeping the existing isDeckMapDashboardTableDatasetSource/source.transformSql?.trim() logic in one reusable location. Make sure the helper is referenced anywhere this condition is needed so the table-source handling stays consistent.
95-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Arrow’s typed guards here instead of structural casts.
DataType.isIntandDataType.isTimestampalready narrow to Arrow’sInt_/Timestamp_types, so theseas {bitWidth...}/as {unit...}casts can be replaced with the corresponding Arrow types for safer typing and less brittle code.🤖 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/useDeckMapDatasetSchema.ts` around lines 95 - 125, The Arrow type mapping helpers are using brittle structural casts instead of Arrow’s built-in typed guards. Update the logic in mapArrowIntTypeToDuckDb (and the related timestamp path if present in useDeckMapDatasetSchema) to rely on DataType.isInt/DataType.isTimestamp narrowing so you can access bitWidth/unit from the proper Arrow Int_/Timestamp_ types without manual casts. Keep the conversion behavior the same, but replace the unsafe type assertions with the Arrow-typed values.
🤖 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/useDeckMapDatasetSchema.ts`:
- Around line 19-29: Add TSDoc comments to every exported symbol in
useDeckMapDatasetSchema.ts, including DeckMapResolvedDatasetSchema,
DeckMapDatasetSchemaState, isDeckMapGeneratedColumn,
createDeckMapDatasetOutputSchemaSql, resolveDeckMapDatasetSchema,
arrowTypeToDuckDbColumnType, and useDeckMapDatasetSchema. Keep the comments
concise but descriptive, explaining each type/function’s purpose and behavior so
the exported API in this `@sqlrooms/`* package is documented consistently.
---
Nitpick comments:
In `@packages/deck/src/useDeckMapDatasetSchema.ts`:
- Around line 84-93: The transformSql truthiness check is duplicated between
shouldInspectOutputSchema and classifyGeneratedColumns, which risks the two
branches drifting apart. Extract a shared helper such as
hasTableTransformSql(source) and use it in both places, keeping the existing
isDeckMapDashboardTableDatasetSource/source.transformSql?.trim() logic in one
reusable location. Make sure the helper is referenced anywhere this condition is
needed so the table-source handling stays consistent.
- Around line 95-125: The Arrow type mapping helpers are using brittle
structural casts instead of Arrow’s built-in typed guards. Update the logic in
mapArrowIntTypeToDuckDb (and the related timestamp path if present in
useDeckMapDatasetSchema) to rely on DataType.isInt/DataType.isTimestamp
narrowing so you can access bitWidth/unit from the proper Arrow Int_/Timestamp_
types without manual casts. Keep the conversion behavior the same, but replace
the unsafe type assertions with the Arrow-typed values.
🪄 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: d19415ea-f17d-4ddc-914d-47435098b248
📒 Files selected for processing (5)
packages/deck/__tests__/useDeckMapDatasetSchema.test.tspackages/deck/src/MapSettings.tsxpackages/deck/src/dashboard.tsxpackages/deck/src/mapLayerConfigUtils.tspackages/deck/src/useDeckMapDatasetSchema.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/deck/src/mapLayerConfigUtils.ts
- packages/deck/tests/useDeckMapDatasetSchema.test.ts
- packages/deck/src/dashboard.tsx
- packages/deck/src/MapSettings.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8768133b66
ℹ️ 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 (!activeLayerDatasetSource) return undefined; | ||
| if (!isDeckMapDashboardTableDatasetSource(activeLayerDatasetSource)) { | ||
| return activeLayerDatasetSource; |
There was a problem hiding this comment.
When a layer is bound to a dataset object that omits source, this returns undefined even if the dashboard has a selected table; resolveDeckMapDashboardDatasetSource explicitly supports that fallback, so those maps still render from dashboard.selectedTable. With the new schema hook, outputColumns/dataOutputColumns stay empty in that supported configuration, which hides or disables the geometry/H3/arc/color/elevation selectors even though the runtime dataset has columns. Build a direct table source from selectedDataTable before giving up here.
Useful? React with 👍 / 👎.
…n-map-selector # Conflicts: # packages/deck/src/MapSettings.tsx
Summary
LIMIT 0queryValidation
pnpm --filter @sqlrooms/deck test --runInBandpnpm --filter @sqlrooms/deck typecheckpnpm --filter @sqlrooms/deck lint(passes with pre-existing warnings)pnpm buildknip,turbo typecheck, circular dependency check,turbo testSummary by CodeRabbit
New Features
Bug Fixes
Tests