Repository navigation
feat: decouple worksheet Deck map resources - #806
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR migrates worksheet maps from Mosaic dashboard panels to persisted deck map resources, adds resource-backed rendering, settings, fitting, and orchestration, and moves Mosaic-specific APIs behind a dedicated adapter entrypoint. ChangesDeck map resource migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ResourceOrchestrator
participant DeckMapsSlice
participant BlockDocument
CLI->>ResourceOrchestrator: create or update map resource
ResourceOrchestrator->>DeckMapsSlice: ensure and write map
ResourceOrchestrator->>BlockDocument: persist block metadata after map write
ResourceOrchestrator-->>CLI: return map and block identifiers
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…sqlrooms into feat/deck-map-resource
![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: 8122a42a22
ℹ️ 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 (isDeckMapTableDatasetSource(source)) { | ||
| resolved[datasetId] = { | ||
| ...datasetConfig, | ||
| tableName: map.selectedTable ?? source.tableName, |
There was a problem hiding this comment.
When a worksheet map has multiple table-backed datasets and selectedTable is set (for example from the first dataset or top-level tableName during map creation), this rewrites every dataset to that one table. A map with layers over two different tables will query the selected table for both datasets, so the second dataset's transform/columns can fail or silently render the wrong data; preserve source.tableName except for the single dataset that is intentionally being switched.
Useful? React with 👍 / 👎.
| dataAdapter={dataAdapter} | ||
| /> | ||
| </DeckMapResourceErrorBoundary> | ||
| ) : ( |
There was a problem hiding this comment.
If a map previously reported a SQL/render issue and then its config is changed to have no datasets (for example by switching to a non-geospatial table or clearing the config), this branch bypasses DeckMapSurface, so the onClearIssue effect in that component never runs. The stale entry remains in deckMaps.runtime.issuesByMapId and will still be returned to the worksheet agent/runtime issue inspection even though the map is now in the no-dataset picker state.
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 (6)
packages/deck/src/useDeckMapFitToBounds.ts (1)
50-65: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor:
mapConfigisn't memoized, so the downstreamuseMemos recompute every render.
asDeckJsonMapConfig(panel.config)returns a fresh reference each render, so bothuseMemo([mapConfig])(Line 51) and thesourcememo (depmapConfig?.datasets, Line 64) recompute on every render. It's functionally harmless sinceuseDeckMapFitControllerkeys off a JSON-stringified snapshot, but wrappingmapConfiginuseMemo(() => asDeckJsonMapConfig(panel.config), [panel.config])restores the intended memoization.🤖 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 50 - 65, Memoize the mapConfig value in the hook using useMemo around asDeckJsonMapConfig(panel.config), keyed by panel.config, so the downstream fitToData and source useMemo calls retain stable dependencies across renders. Update the mapConfig declaration only; preserve the existing downstream logic.packages/deck/src/DeckMapsSlice.ts (1)
79-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tsdoc to new public exports. These files introduce new
@sqlrooms/deckpublic API without tsdoc on exported functions/types. As per coding guidelines, "Add tsdoc to exported types and functions in@sqlrooms/*packages".
packages/deck/src/DeckMapsSlice.ts#L79-L149: documentcreateDeckMapsSlice,useStoreWithDeckMaps, and the exported slice types (DeckMapsSliceState,DeckMapRuntimeIssue, etc.).packages/deck/src/mapConfig.ts#L72-L121: documentcreateEmptyDeckMapConfig, the dataset-source type guards, andasDeckJsonMapConfig.packages/deck/src/mapAiConfig.ts#L38-L62: document the exportedDeckMapResourceConfigParameterandDeckMapResourceToolParametersschemas.🤖 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/DeckMapsSlice.ts` around lines 79 - 149, Add TSDoc for every exported public API in packages/deck/src/DeckMapsSlice.ts: document createDeckMapsSlice, useStoreWithDeckMaps, and exported slice-related types including DeckMapsSliceState and DeckMapRuntimeIssue. Also document createEmptyDeckMapConfig, dataset-source type guards, and asDeckJsonMapConfig in packages/deck/src/mapConfig.ts, plus the exported DeckMapResourceConfigParameter and DeckMapResourceToolParameters schemas in packages/deck/src/mapAiConfig.ts; make each description accurately state its purpose and usage without changing behavior.Source: Coding guidelines
packages/deck/src/createOrUpdateDeckMapResource.ts (1)
8-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd tsdoc to newly exported types. These are new public exported types in the
@sqlrooms/deckpackage but lack tsdoc; add doc comments to each.
packages/deck/src/createOrUpdateDeckMapResource.ts#L8-L67: documentCreateOrUpdateDeckMapResourceHost,CreateOrUpdateDeckMapResourceParams, andCreateOrUpdateDeckMapResourceResult.packages/deck/src/mapDataPolicy.ts#L7-L11: document theDeckMapDataPolicytype.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/createOrUpdateDeckMapResource.ts` around lines 8 - 67, Add TSDoc comments for the exported types CreateOrUpdateDeckMapResourceHost, CreateOrUpdateDeckMapResourceParams, and CreateOrUpdateDeckMapResourceResult in packages/deck/src/createOrUpdateDeckMapResource.ts (lines 8-67), describing each public API type. Also document the exported DeckMapDataPolicy type in packages/deck/src/mapDataPolicy.ts (lines 7-11), following the `@sqlrooms` package documentation guidelines.Source: Coding guidelines
packages/deck/src/MapSettingsControls.tsx (1)
28-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tsdoc to the new exported components/types.
This is a new set of public exports in
@sqlrooms/deck(DeckMapSettingsField,filterDeckMapColumns,DeckMapColumnsProvider,DeckMapColumnSelector,DeckMapTableSelector,DeckMapCodeViewToggleButton,DeckMapCodeViewerPanel, and their types) with no documentation. Consider adding brief tsdoc, and updating the packageREADME.mdif these are intended as public API.As per coding guidelines: "Add tsdoc to exported types and functions in
@sqlrooms/*packages" and "When changing the public API of an@sqlrooms/*package or adding new public API, update the package'sREADME.md".🤖 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/MapSettingsControls.tsx` around lines 28 - 247, Add brief TSDoc comments to each newly exported component, function, context provider, selector, and related type in this diff, including DeckMapSettingsField, filterDeckMapColumns, DeckMapColumnsProvider, DeckMapColumnSelector and its props/kind types, DeckMapTableSelector, DeckMapCodeViewToggleButton, and DeckMapCodeViewerPanel. Update the package README.md to document these exports as public API.Source: Coding guidelines
packages/deck/src/DeckMapDefaultStylesProvider.tsx (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tsdoc to exported types and functions.
DeckMapStyle,DeckMapDefaultStyles,useDeckMapDefaultStyles, andresolveDeckMapStyleare exported from an@sqlrooms/*package without tsdoc.DeckMapDefaultStylesProvideris the only export with documentation.As per coding guidelines: "Add tsdoc to exported types and functions in
@sqlrooms/*packages"📝 Proposed tsdoc additions
+/** + * Basemap style URL compatible with `react-map-gl`'s `mapStyle` prop. + */ export type DeckMapStyle = NonNullable<MapProps['mapStyle']>; + +/** + * Theme-keyed default basemap styles supplied by the host application. + * Keys are `ResolvedTheme` values; not every theme needs an entry. + */ export type DeckMapDefaultStyles = Partial<Record<ResolvedTheme, DeckMapStyle>>; // ... provider already has tsdoc ... +/** + * Reads host-provided default basemap styles from context. + * Returns `undefined` when no `DeckMapDefaultStylesProvider` ancestor is present. + */ export function useDeckMapDefaultStyles() { return useContext(DeckMapDefaultStylesContext); } +/** + * Resolves the effective basemap style using strict precedence: + * explicit `mapStyle` → `mapPropsMapStyle` → host default for the resolved + * theme → `fallbackStyles` for the resolved theme. + */ export function resolveDeckMapStyle(options: {Also applies to: 29-31, 33-46
🤖 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/DeckMapDefaultStylesProvider.tsx` around lines 10 - 11, Add TSDoc comments to the exported types DeckMapStyle and DeckMapDefaultStyles, and to the exported useDeckMapDefaultStyles and resolveDeckMapStyle functions. Keep the existing DeckMapDefaultStylesProvider documentation and document each symbol’s purpose and usage consistent with the package’s conventions.Source: Coding guidelines
packages/deck/src/block.tsx (1)
228-250: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConfig-keyed remount re-initializes the whole map surface.
key={JSON.stringify(map.config)}onDeckMapResourceErrorBoundaryforces a full unmount/remount of the boundary,DeckMapSurface, andDeckJsonMapon every config edit. Beyond the intended error-boundary reset, this re-initializes the deck.gl map and likely resets the viewport on each settings change. Consider resetting only the boundary (e.g. aresetKey/getDerivedStateFromPropsinDeckMapResourceErrorBoundary) rather than keying the entire subtree.🤖 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 228 - 250, Remove the JSON-stringified config key from the DeckMapResourceErrorBoundary usage so config edits no longer remount DeckMapSurface and DeckJsonMap. Update DeckMapResourceErrorBoundary to reset its error state using a resetKey or getDerivedStateFromProps tied to map.config, preserving boundary recovery without re-initializing the map surface.
🤖 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 1012-1043: Update the latitude selector’s onChange handler in the
fitToData configuration to read the current longitude column and return early
when it is unset, matching the guard in the longitude handler. Only call
regenerateMapConfigForTable when both coordinates are selected, preserving
consistent behavior between the two selectors.
---
Nitpick comments:
In `@packages/deck/src/block.tsx`:
- Around line 228-250: Remove the JSON-stringified config key from the
DeckMapResourceErrorBoundary usage so config edits no longer remount
DeckMapSurface and DeckJsonMap. Update DeckMapResourceErrorBoundary to reset its
error state using a resetKey or getDerivedStateFromProps tied to map.config,
preserving boundary recovery without re-initializing the map surface.
In `@packages/deck/src/createOrUpdateDeckMapResource.ts`:
- Around line 8-67: Add TSDoc comments for the exported types
CreateOrUpdateDeckMapResourceHost, CreateOrUpdateDeckMapResourceParams, and
CreateOrUpdateDeckMapResourceResult in
packages/deck/src/createOrUpdateDeckMapResource.ts (lines 8-67), describing each
public API type. Also document the exported DeckMapDataPolicy type in
packages/deck/src/mapDataPolicy.ts (lines 7-11), following the `@sqlrooms` package
documentation guidelines.
In `@packages/deck/src/DeckMapDefaultStylesProvider.tsx`:
- Around line 10-11: Add TSDoc comments to the exported types DeckMapStyle and
DeckMapDefaultStyles, and to the exported useDeckMapDefaultStyles and
resolveDeckMapStyle functions. Keep the existing DeckMapDefaultStylesProvider
documentation and document each symbol’s purpose and usage consistent with the
package’s conventions.
In `@packages/deck/src/DeckMapsSlice.ts`:
- Around line 79-149: Add TSDoc for every exported public API in
packages/deck/src/DeckMapsSlice.ts: document createDeckMapsSlice,
useStoreWithDeckMaps, and exported slice-related types including
DeckMapsSliceState and DeckMapRuntimeIssue. Also document
createEmptyDeckMapConfig, dataset-source type guards, and asDeckJsonMapConfig in
packages/deck/src/mapConfig.ts, plus the exported DeckMapResourceConfigParameter
and DeckMapResourceToolParameters schemas in packages/deck/src/mapAiConfig.ts;
make each description accurately state its purpose and usage without changing
behavior.
In `@packages/deck/src/MapSettingsControls.tsx`:
- Around line 28-247: Add brief TSDoc comments to each newly exported component,
function, context provider, selector, and related type in this diff, including
DeckMapSettingsField, filterDeckMapColumns, DeckMapColumnsProvider,
DeckMapColumnSelector and its props/kind types, DeckMapTableSelector,
DeckMapCodeViewToggleButton, and DeckMapCodeViewerPanel. Update the package
README.md to document these exports as public API.
In `@packages/deck/src/useDeckMapFitToBounds.ts`:
- Around line 50-65: Memoize the mapConfig value in the hook using useMemo
around asDeckJsonMapConfig(panel.config), keyed by panel.config, so the
downstream fitToData and source useMemo calls retain stable dependencies across
renders. Update the mapConfig declaration only; preserve the existing downstream
logic.
🪄 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: 68ede1c7-39ab-4591-9992-dfe710ef18aa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
apps/sqlrooms-cli-ui/src/__tests__/createCliBlockDocumentCommands.test.tsapps/sqlrooms-cli-ui/src/ai/__tests__/createCliBlockDocumentAgentTool.test.tsapps/sqlrooms-cli-ui/src/ai/createCliBlockDocumentAgentTool.tsapps/sqlrooms-cli-ui/src/ai/getMapBlockRuntimeIssues.tsapps/sqlrooms-cli-ui/src/createBlockDocumentAgent.tsapps/sqlrooms-cli-ui/src/createCliBlockDocumentCommands.tsapps/sqlrooms-cli-ui/src/createDashboardAgent.tsapps/sqlrooms-cli-ui/src/statefulBlockArtifactConfigs.tsapps/sqlrooms-cli-ui/src/store-types.tsapps/sqlrooms-cli-ui/src/store.tspackages/deck/README.mdpackages/deck/__tests__/DeckMapDataAdapter.test.tspackages/deck/__tests__/DeckMapDefaultStylesProvider.test.tspackages/deck/__tests__/DeckMapsSlice.test.tspackages/deck/__tests__/MapSettingsControls.test.tspackages/deck/__tests__/createOrUpdateDeckMapBlock.test.tspackages/deck/__tests__/createOrUpdateDeckMapResource.test.tspackages/deck/__tests__/mapAiConfig.test.tspackages/deck/__tests__/mapFit.test.tspackages/deck/package.jsonpackages/deck/src/BlockMapSettings.tsxpackages/deck/src/DashboardMapSettings.tsxpackages/deck/src/DeckJsonMap.tsxpackages/deck/src/DeckMapDefaultStylesProvider.tsxpackages/deck/src/DeckMapSurface.tsxpackages/deck/src/DeckMapsSlice.tspackages/deck/src/LatitudeSelector.tsxpackages/deck/src/LongitudeSelector.tsxpackages/deck/src/MapSettings.tsxpackages/deck/src/MapSettingsControls.tsxpackages/deck/src/ai.tspackages/deck/src/block.tsxpackages/deck/src/blockDocumentRegistration.tspackages/deck/src/createOrUpdateDeckMapBlock.tspackages/deck/src/createOrUpdateDeckMapResource.tspackages/deck/src/dashboard.tsxpackages/deck/src/dashboardConfig.tspackages/deck/src/index.tspackages/deck/src/mapAiConfig.tspackages/deck/src/mapConfig.tspackages/deck/src/mapConfigUtils.tspackages/deck/src/mapDataPolicy.tspackages/deck/src/mapFit.tspackages/deck/src/mapLayerConfigUtils.tspackages/deck/src/mosaic.tspackages/deck/src/useDeckMapDatasetSchema.tspackages/deck/src/useDeckMapFitToBounds.ts
💤 Files with no reviewable changes (4)
- packages/deck/tests/createOrUpdateDeckMapBlock.test.ts
- packages/deck/src/LatitudeSelector.tsx
- packages/deck/src/LongitudeSelector.tsx
- packages/deck/src/createOrUpdateDeckMapBlock.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1be7d003f
ℹ️ 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".
| ), | ||
| [dashboard?.selectedTable, tablesWithColumns], | ||
| ); | ||
| if (mapId && !map) ensureMap(mapId, {title: title ?? 'Embedded Map'}); |
There was a problem hiding this comment.
For persisted worksheets created before this change, a map block's blockInstanceId points at a Mosaic dashboard that contains the deck-json-map panel, and there is no deckMaps entry yet. This effect creates a brand-new empty map for that id, so the old panel config under mosaicDashboard.dashboardsById[mapId] is ignored and the worksheet renders the table picker instead of the user's existing map. Add a migration/fallback from the old dashboard panel before creating empty state.
Useful? React with 👍 / 👎.
| <DeckJsonMap | ||
| ref={deckMapRef} | ||
| spec={map.config.spec} | ||
| datasets={datasets} |
There was a problem hiding this comment.
For direct worksheet maps with table or SQL datasets over large sources, this passes the resolved datasets straight to DeckJsonMap; the prepared dataset path executes the table/SQL input directly, so neither the default 100,000-row limit nor config.dataPolicy.maxRows is applied. This regresses the old Mosaic-backed worksheet maps, where the dashboard dataset client sampled with maxRows, and a large table can now be loaded entirely into Arrow/WebGL and freeze or crash the tab.
Useful? React with 👍 / 👎.
| <DeckMapSettingsPanel | ||
| title={block?.caption ?? map.title} | ||
| selectedTable={map.selectedTable} | ||
| config={map.config} | ||
| tables={tables.filter((table) => table.columns.length > 0)} |
There was a problem hiding this comment.
When a worksheet map has configMode: 'custom', this unconditional settings panel now exposes the basic Dataset/layer controls; the previous block adapter explicitly short-circuited custom configs because those controls only understand a subset of Deck props. In that context, selecting a new dataset calls the handler above and regenerates datasets, which can remove dataset ids still referenced by custom multi-layer specs and break otherwise valid maps. Preserve the custom-mode guard or make custom maps code-only here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b6eb7fc3
ℹ️ 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".
| writeMap: ({mapId, title, config, selectedTable}) => { | ||
| state.deckMaps.updateMap(mapId, {title, config, selectedTable}); |
There was a problem hiding this comment.
In this command path no prepareConfig callback is supplied to createOrUpdateDeckMapResource, so direct worksheet maps now persist the incoming config verbatim. The previous worksheet-map path went through createDeckMapPanelFromNativeConfig, which repaired the common AI output of source.tableName + fitToData.longitudeColumn/latitudeColumn by adding the WKB transformSql/geometryColumn; without that normalization, those direct maps store a table-only dataset and GeoArrow point layers have no geometry to render. Add the same normalization here before writeMap receives the config.
Useful? React with 👍 / 👎.
| onRenderingError={(error) => | ||
| onReportIssue({ | ||
| kind: 'render-error', | ||
| message: error.message, | ||
| recoverable: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
For direct worksheet maps, passing onRenderingError suppresses DeckJsonMap's built-in error overlay (DeckJsonMap only shows it when no handler is provided), but DeckMapBlockRenderer never reads deckMaps.runtime.issuesByMapId to render a replacement panel. When a custom spec or layer conversion fails, the layer list is cleared and the user just sees a blank map while the issue is only stored for agents. Either render the reported issue in the direct block surface or let DeckJsonMap keep its fallback overlay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6614a6a349
ℹ️ 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 activeFitState = | ||
| fitState.key === fitKey | ||
| ? fitState | ||
| : createInitialFitState(fitKey, requestVersion); |
There was a problem hiding this comment.
When a direct worksheet map's config/source changes, fitState.key no longer matches fitKey; this fallback then initializes handledRequestVersion to the current requestVersion, so each Fit button click is treated as already handled and the effect returns because autoFit is false in DeckMapSurface. In that scenario the map view can no longer be fitted until the component remounts; initialize the new-key state with the previous handled version or commit the new key before comparing manual requests.
Useful? React with 👍 / 👎.
| return ( | ||
| <> | ||
| <MapSettingsPanel | ||
| dashboardId={dashboard.id} | ||
| panel={panel} | ||
| <DeckMapSettingsPanel | ||
| title={panel.title} | ||
| selectedTable={dashboard.selectedTable} | ||
| config={panel.config as DeckMapDashboardPanelConfig} | ||
| tables={tables} |
There was a problem hiding this comment.
For dashboard panels with configMode: 'custom', this now always renders DeckMapSettingsPanel and wires onConfigChange, so the basic layer/dataset controls can rewrite custom AI-authored specs that the UI cannot round-trip safely. The block-map settings path still guards custom configs; add the same guard here or keep custom dashboard maps code-only to avoid breaking valid custom layers when users adjust basic controls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba178e4405
ℹ️ 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".
| } else if ( | ||
| Object.values(states).every((state) => state.status === 'ready') | ||
| ) { | ||
| onClearIssue(); |
There was a problem hiding this comment.
When a direct worksheet map has a render/config error (for example an invalid layer spec) and its async datasets later transition to ready, this branch treats the ready datasets as a successful recovery and calls onClearIssue(). DeckJsonMap reports render errors through a separate path, so this can erase the render-error from deckMaps.runtime.issuesByMapId while the map still shows the error overlay; target agents/runtime issue inspection then see no issue to repair. Clear only dataset-loading SQL issues here, or keep issue kinds separate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f000f8892
ℹ️ 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".
| updateMap(mapId, { | ||
| config: regenerateMapConfigForTable( | ||
| {config: map.config}, | ||
| table, | ||
| ) as typeof map.config, |
There was a problem hiding this comment.
When a direct worksheet map is edited in settings and the user selects a different geospatial table, regenerateMapConfigForTable replaces datasets with a key based on the new table while preserving the existing layer spec. Those layers still have _sqlroomsBinding.dataset pointing at the old dataset id, so DeckJsonMap cannot find a matching dataset state and filters the layer out, leaving the map blank after the switch. Preserve the dataset id or rewrite the layer bindings when regenerating the config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e98bcc0a6
ℹ️ 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 ( | ||
| patch.config && | ||
| Object.keys(patch.config.datasets).length === 0 | ||
| ) { | ||
| delete draft.deckMaps.runtime.issuesByMapId[id]; |
There was a problem hiding this comment.
When a direct map previously reported a render-error and the user or AI replaces it with a valid non-empty config, this branch leaves the old issue in deckMaps.runtime.issuesByMapId because it only clears issues for empty dataset configs. DeckMapSurface only clears sql-error after dataset success, so runtime issue inspection can keep reporting an obsolete render failure even after the map has recovered.
Useful? React with 👍 / 👎.
| const handleTableChange = useCallback( | ||
| (table: DataTable) => { | ||
| if (readOnly || !map) return; | ||
| setSelectedTable(mapId, getTableIdentity(table.table)); |
There was a problem hiding this comment.
When the settings table picker is changed to a table without longitude/latitude or geometry columns, regenerateMapConfigForTable returns the existing config unchanged, but this line has already persisted selectedTable. For a single table-backed map the direct adapter then resolves the old dataset against that non-geospatial table, so the map starts querying the wrong schema and errors instead of keeping the previous selection or moving to the empty/table-picker state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62e4a0f9e4
ℹ️ 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".
| params.caption ?? | ||
| params.title?.trim() ?? | ||
| existingBlock?.caption?.trim() ?? | ||
| existingMap?.title?.trim() ?? |
There was a problem hiding this comment.
When an existing worksheet map block has an empty caption (the default for UI-created map nodes) and the AI/tool updates it without an explicit title, this ?? chain treats existingBlock.caption.trim() as a valid empty string and persists title: '' in writeMap, discarding the existing map resource title. Use a truthy trimmed caption/title fallback here so title-less updates keep the saved map title instead of blanking the label.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20de24b43f
ℹ️ 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".
| (missingBehavior === 'create' | ||
| ? params.createMapId?.() | ||
| : (params.mapId ?? params.createMapId?.())); |
There was a problem hiding this comment.
When a host opts into missingMapBlockBehavior: 'create' for an AI/create flow and passes a desired mapId, this branch discards params.mapId and calls only createMapId. If no generator was supplied the helper throws at the next line; if one was supplied the block is created under a different resource id, so callers cannot create or recover the requested map id. Prefer params.mapId ?? params.createMapId?.() in the create branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d315f110d
ℹ️ 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 nextDataset = Object.values(nextConfig.datasets)[0]; | ||
|
|
||
| if (existingDatasetIds.length === 1 && nextDataset) { |
There was a problem hiding this comment.
When the settings panel is opened for an empty direct map and the user selects a geospatial table, existingDatasetIds.length is 0, so this single-dataset branch is skipped and the function falls through to the "preserve existing layer spec" return. For createEmptyDeckMapConfig() that spec has layers: [], so the map gains a dataset and leaves the picker state but renders a blank map with no layer; return the full nextConfig when there is no existing dataset/layer to preserve.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98fdbb23ea
ℹ️ 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 ( | ||
| existingDatasetIds.length === 0 && | ||
| existingConfig.spec.layers.length === 0 |
There was a problem hiding this comment.
When an otherwise valid empty/basic map stores spec as a JSON string or as an object without a layers array, selecting a geospatial table in settings reaches this branch with existingDatasetIds.length === 0 and then dereferences existingConfig.spec.layers.length. Since the map config schema accepts string specs, this throws instead of seeding the map from the selected table, so the settings panel can crash for those persisted/API-authored maps. Check that spec is an object with an array of layers before reading .length, or fall back to nextConfig for no-dataset maps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37cbb02df9
ℹ️ 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".
| }), | ||
| [onReportIssue], | ||
| ); | ||
| useDeckMapFitController({ |
There was a problem hiding this comment.
When a direct worksheet map is created or updated from a geospatial table, the config has fitToData but the generated initialViewState is still the default world view. This controller call omits autoFit (which defaults to false), so unlike the previous Mosaic-backed worksheet map path and the dashboard adapter, no bounds query runs until the user manually clicks Fit; local datasets can open zoomed far out and look empty. Pass autoFit for direct worksheet surfaces or seed a fitted view when writing the map.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c8c12d12
ℹ️ 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".
| } else if ( | ||
| Object.values(states).every((state) => state.status === 'ready') | ||
| ) { | ||
| onClearIssue('sql-error'); |
There was a problem hiding this comment.
When a map's datasets load successfully but the fit-to-data bounds query fails (for example because fitToData references missing lon/lat columns or H3 loading fails), handleFitError records that failure as kind sql-error. This ready-state branch then clears any sql-error as soon as all dataset states are ready, so the map remains unfitted while runtime issue inspection loses the recoverable error. Fresh evidence relative to the earlier render-error issue is that fit failures now share the same sql-error kind; clear only dataset-load SQL issues or use a distinct fit issue kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
sqlrooms/packages/deck/src/mapConfigUtils.ts
Lines 375 to 376 in 3f97c84
Normalize generated geometry aliases before persisting
When a direct worksheet map arrives with source.tableName and datasets[id].geometryColumn set to the intended generated WKB alias for a lon/lat point map but omits source.transformSql, this guard treats the non-empty geometry column as native and skips adding the ST_AsWKB transform. The saved dataset then queries the raw table, which lacks that generated alias, so the layer and fit-to-data path fail instead of being repaired. Fresh evidence relative to the earlier normalization issue is that this new normalizer now runs, but exits on the alias before consulting the table schema.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed98f9dfd
ℹ️ 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 hasSqlSource = | ||
| isDeckMapSqlDatasetSource(source) && source.sqlQuery.trim().length > 0; | ||
| const hasTableSource = | ||
| isDeckMapTableDatasetSource(source) && source.tableName.trim().length > 0; |
There was a problem hiding this comment.
When a persisted or API-authored map contains a malformed dataset source such as {tableName: null} or {sqlQuery: 1}, these guards still pass because they only check for the key and then .trim() throws. Since DeckMapsSliceConfig persists dataset entries as unknown and the settings/surface path uses this validator to turn bad configs into recoverable config errors, this can crash the map UI instead of reporting an actionable issue; check typeof ... === 'string' before trimming.
Useful? React with 👍 / 👎.
| const longitudeColumn = | ||
| mapConfig.fitToData?.longitudeColumn; | ||
| if (!longitudeColumn) return; | ||
| applyConfig( |
There was a problem hiding this comment.
When a basic point map was authored without fitToData.longitudeColumn/latitudeColumn, both coordinate selectors start empty and each change handler now returns until the other coordinate already exists. That makes the settings panel unable to repair otherwise valid table-backed point maps missing fit metadata; allow the first selected coordinate to regenerate the config while inferring or preserving the counterpart instead of ignoring it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb41a83ee
ℹ️ 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 (!incomingLayers || incomingLayers.length === 0) return existingLayers; | ||
| if (!existingLayers || existingLayers.length === 0) return incomingLayers; | ||
|
|
||
| const nextLayers = [...existingLayers]; |
There was a problem hiding this comment.
When updating an existing map that has multiple layers, this starts the merged layer list from all existing layers and only updates/appends incoming ones, so an update whose spec.layers contains the desired replacement list cannot remove a stale layer. For example, if the worksheet agent is asked to delete a broken second layer, the omitted layer remains persisted and can keep the render/runtime issue alive; the update path needs an explicit replacement/delete mode rather than treating every object layers array as additive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4adcd5311a
ℹ️ 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 fitSource = useMemo( | ||
| () => | ||
| fitToData ? getDeckMapDatasetSource(datasets[fitToData.dataset]) : null, |
There was a problem hiding this comment.
For direct worksheet maps with the default/maxRows data policy, datasets has already had each table/SQL source wrapped in USING SAMPLE, and this line feeds that sampled source into the bounds query used by auto-fit and the Fit button. On large geospatial tables this computes the viewport from a random capped subset instead of the full dataset extent, so maps can open or refit zoomed to only part of the data while valid rendered/available features outside the sample extent are excluded from view; derive the fit source from the authored config or otherwise bypass the row-limit policy for bounds queries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 354ade886a
ℹ️ 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 {source, fitToData} = options; | ||
| if (!fitToData.dataset) return null; | ||
| const baseSourceSql = isDeckMapSqlDatasetSource(source) | ||
| ? `SELECT * FROM (${source.sqlQuery}) AS "__sqlrooms_dashboard_map_source"` |
There was a problem hiding this comment.
When a direct worksheet map uses a source.sqlQuery that ends with a semicolon, the map data can still render because the sampled dataset path strips trailing semicolons, but the fit-to-data path wraps the unsampled raw SQL here as FROM (${source.sqlQuery}). That produces invalid DuckDB SQL like FROM (SELECT ...;), so initial auto-fit and the Fit button report a fit error and leave otherwise valid SQL-backed maps at the default view; clean the SQL the same way before wrapping it for bounds queries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/sqlrooms/sqlrooms/blob/554d8fe0f3d15cd3a6d7bfdc7a16d2440d82ff5c//tmp/sqlrooms-review-818/packages/deck/src/mapResourceAuthoring.ts#L135
When repairing an existing multi-dataset map, for example by calling the map tool with replaceLayers: true and only the remaining layer/dataset, this merge starts from all existing dataset entries and never deletes omitted keys. The direct map surface resolves every entry in config.datasets, so a stale dataset whose layer was removed is still queried and can keep reporting SQL/runtime errors even though the visible layer was deleted; add an explicit dataset replacement/delete path alongside layer replacement.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6507a9d895
ℹ️ 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".
|
|
||
| /** Validates inputs for an AI or command invocation that writes a map resource. */ | ||
| export const DeckMapResourceToolParameters = z.object({ | ||
| title: z.string().optional().default('Map'), |
There was a problem hiding this comment.
When a host uses this exported tool schema for an existing worksheet map update and omits title, Zod materializes it as "Map". createOrUpdateDeckMapResource then treats that as an explicit title and writes it back instead of falling through to the existing block caption/resource title, so title-less AI repairs can clobber saved map labels; keep title optional here and let the orchestration helper apply its preservation logic.
Useful? React with 👍 / 👎.
| const existing = isRecord(existingSpec) ? existingSpec : undefined; | ||
| const incoming = isRecord(incomingSpec) ? incomingSpec : undefined; |
There was a problem hiding this comment.
When an existing map stores spec as a JSON string and the AI sends a sparse object patch such as spec: {} or one patched layer, this line treats the existing spec as absent, so the return below replaces the whole spec with the incoming object and drops the previous view state/layers that sparse updates are supposed to preserve. Parse valid string specs before merging, or keep the original string unless the caller explicitly replaces it.
Useful? React with 👍 / 👎.
Summary
deckMaps.config.mapsByIdresources with ephemeral map runtime issuescreateOrUpdateDeckMapBlockwith resource-nativecreateOrUpdateDeckMapResource, preserving title resolution and metadata-after-map-write ordering from feat: share block-document Ask AI and Deck map helpers across hosts #805deckMaps@sqlrooms/deck/mosaicadapter and remove hard Mosaic dependencies from the core package pathpanelIdresults without migration or dual reads/writesBehavior
Worksheet maps are independent and do not participate in Mosaic cross-filter selection. Actual Mosaic dashboard map panels retain their existing adapter-backed behavior.
Validation
resource-native authoring now rejects unsupported dataset/layer shapes before durable writes and reports malformed saved resources as ephemeral
config-errorissuescommit hooks passed Prettier and SQLRooms lint
pre-push
knippassedSQLRooms builds/tests were intentionally not run locally; CI is the verification source per the implementation plan
repository-wide pre-push typecheck is blocked by an unrelated baseline stale-export error in
@sqlrooms/ai/@sqlrooms/ai-coreLinked FSQ integration
foursquare/fsq-spatial-desktop#730 has merged into
worksheets. Theworksheetsbranch now points its SQLRooms submodule directly at this PR's current head,68c8c12d1, including resource-native authoring validation, shared worksheet-agent instructions, and the latest map fitting fixes.Summary by CodeRabbit