Repository navigation
feat: move settings to side panel - #771
Conversation
Design for migrating stateful blocks (dashboards, charts, data tables) from custom selection to TipTap's built-in node selection. Preserves panel-level custom selection inside dashboards. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…ts and add dashboard block settings
…PanelWrapper and unused props
…ksheet artifact UI
|
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:
📝 WalkthroughWalkthroughIntroduces a block settings selection system: a Zustand slice tracks selected blocks/panels, TipTap node views gain click-to-select, a ChangesBlock settings migration
Circular dependency pre-push check
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeView as BlockDocumentStatefulBlockNodeView
participant TipTap as TipTap Editor
participant useSelectedBlockOrPanel
participant BlockSettingsSlice
participant BlockSettingsPanel
participant SettingsComponent
User->>NodeView: click
NodeView->>TipTap: setNodeSelection(pos)
TipTap->>useSelectedBlockOrPanel: selectionUpdate event
useSelectedBlockOrPanel->>BlockSettingsSlice: clearSelection() (panel)
useSelectedBlockOrPanel-->>BlockSettingsPanel: returns {type:'block', blockType, blockId}
BlockSettingsPanel->>BlockSettingsSlice: getSettings(registryKey)
BlockSettingsSlice-->>BlockSettingsPanel: SettingsComponent
BlockSettingsPanel->>SettingsComponent: createElement({blockId, dashboardId})
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 |
![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: 286c21b6e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (16)
packages/deck/src/dashboard.tsx (1)
576-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWarning indicator is now a non-functional button.
The
<button>retains an emptyonClickand is still focusable/clickable but does nothing, which is confusing for keyboard and screen-reader users (an actionable control with no action). Since the tooltip is the only purpose now, use a non-interactive element (e.g. a<span>/icon as theTooltipTrigger) so it isn't announced as a button.♿ Suggested change
- <button - className={`absolute left-2 z-10 flex h-5 w-5 items-center justify-center ${ + <span + className={`absolute left-2 z-10 flex h-5 w-5 items-center justify-center ${ Object.values(datasetStates).some((s) => s.isSampled) && !sampledDismissed ? 'top-9' : 'top-2' }`} - onClick={() => { - // Settings now shown in side panel via block selection - }} > <AlertTriangleIcon className="h-4 w-4 text-amber-500 drop-shadow-sm" /> - </button> + </span>🤖 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 576 - 598, The warning indicator in the missingColumns tooltip block is still rendered as a button even though its onClick is empty, so it should no longer be interactive. Update the TooltipTrigger usage in dashboard.tsx so the alert icon is wrapped with a non-interactive element instead of a <button>, and remove the empty click handler to keep the indicator accessible and not announced as an action.packages/mosaic/src/charts/MosaicChartSettingsPanel.tsx (1)
19-34: 📐 Maintainability & Code Quality | 🔵 TrivialDead code: unreachable spec viewer branch
The
viewModestate can no longer switch to'spec'. The previous "view spec" toggle was removed, and the only remaining call tosetViewMode(inhandleBackToSettings) resets the state to'settings'. Verified by grep: no code inpackages/mosaic/srcever setsviewModeto'spec'.Consequently, the block checking
renderContext.type === 'spec' && viewMode === 'spec'and theMosaicChartSpecViewerPanelrender logic are permanently unreachable. Remove theviewModestate,setViewMode,handleBackToSettings, and the dead conditional block.Dead code block
The following lines can be removed: ```tsx const [viewMode, setViewMode] = useState<'settings' | 'spec'>('settings'); const handleBackToSettings = useCallback(() => { setViewMode('settings'); }, []);if (renderContext.type === 'spec' && viewMode === 'spec') {
return (
);
}</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/mosaic/src/charts/MosaicChartSettingsPanel.tsxaround lines 19 -
34, The spec viewer path in MosaicChartSettingsPanel is unreachable because
viewMode can no longer be set to 'spec'. Remove the unused viewMode state,
setViewMode, handleBackToSettings callback, and the conditional branch that
renders MosaicChartSpecViewerPanel; keep the component focused on the settings
view and preserve the existing renderContext logic only if it is still needed
elsewhere.</details> <!-- cr-comment:v1:4c3c72cfdc3ca2de538224df --> </blockquote></details> <details> <summary>packages/mosaic/src/charts/worksheet/ChartBlockHeader.tsx (1)</summary><blockquote> `5-19`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Remove now-dead `chartConfig` / `onSettingsOpenChange` props.** Both are still declared as required on `ChartBlockHeaderProps` but are no longer destructured or used in the body after dropping the settings toggle. `ChartBlockRenderer` still computes `handleSettingsOpenChange` and passes `chartConfig`/`onSettingsOpenChange` solely to satisfy these required props. Dropping them here lets you also remove that dead wiring upstream. <details> <summary>♻️ Proposed prop cleanup</summary> ```diff export type ChartBlockHeaderProps = { caption?: string; - chartConfig: ChartConfig; onCaptionChange?: (caption: string | undefined) => void; - onSettingsOpenChange: (open: boolean) => void; readOnly?: boolean; tableName: 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/mosaic/src/charts/worksheet/ChartBlockHeader.tsx` around lines 5 - 19, ChartBlockHeader still declares unused required props, so remove the dead chartConfig and onSettingsOpenChange entries from ChartBlockHeaderProps and from the ChartBlockHeader component signature. Then update ChartBlockRenderer to stop computing handleSettingsOpenChange and to stop passing those props into ChartBlockHeader, since they are no longer used after the settings toggle was removed.packages/mosaic/src/components/MultiFieldSelector.tsx (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the dropped minimum width is intended.
Changing the first track from
minmax(120px, 1fr)to1frremoves the 120px floor, so theColumnSelectorcan now shrink arbitrarily in narrow containers (e.g. the new side panel). If column labels truncating in tight layouts is acceptable this is fine; otherwise keep a smallminmax(...)floor.🤖 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/mosaic/src/components/MultiFieldSelector.tsx` around lines 74 - 78, The grid in MultiFieldSelector’s field row now uses a plain 1fr first column, which removes the previous minimum width floor. Review the style on the row container and, if the ColumnSelector should not shrink too far in narrow layouts, restore a minmax(...) minimum for the first track; otherwise leave it as-is only if label truncation is intended.packages/deck/src/MapSettings.tsx (2)
159-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
useDataTable(dashboardSelectedTable)subscription.
selectedDataTable(Line 159) andprimaryTable(Line 187) calluseDataTable(dashboardSelectedTable)with the identical argument, creating two redundant store subscriptions for the same value. Reuse one variable.♻️ Proposed consolidation
const tables = useTablesWithColumns(); const selectedDataTable = useDataTable(dashboardSelectedTable); @@ - const primaryTable = useDataTable(dashboardSelectedTable); const fallbackTable = useDataTable(fallbackTableName); - const dataTable = primaryTable ?? fallbackTable; + const dataTable = selectedDataTable ?? fallbackTable;Also applies to: 187-189
🤖 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` at line 159, There is a redundant duplicate subscription to the same dashboard-selected table in MapSettings; reuse a single useDataTable(dashboardSelectedTable) result instead of calling it twice. Update the selectedDataTable and primaryTable logic so the existing selectedDataTable value is shared wherever the identical argument is needed, and remove the extra useDataTable(dashboardSelectedTable) call in the MapSettings component.
116-121: 📐 Maintainability & Code Quality | 🔵 TrivialRemove unused
onCloseprop fromMapSettingsPanelPropsThe
onClosecallback is no longer used by the component (not destructured) and is not passed by any callers (BlockMapSettings,DashboardMapSettings). It remains in the interface only as a leftover from the removed close-button UI.Original diff context
interface MapSettingsPanelProps { dashboardId: string; panel: MosaicDashboardPanelConfigType; onClose?: () => void; // <-- Remove this line onTableChange?: (table: DataTable) => void; }Callers in
BlockMapSettings.tsxandDashboardMapSettings.tsxalready omitonClose.🤖 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 116 - 121, Remove the unused onClose prop from MapSettingsPanelProps in MapSettingsPanel because it is no longer consumed by the component and is not passed by BlockMapSettings or DashboardMapSettings. Update the MapSettingsPanelProps interface to only include the active props, and make sure any related destructuring or type references in MapSettings.tsx stay consistent after removing onClose.packages/mosaic/src/charts/MosaicChart.tsx (1)
6-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onConfigChangeis now an unused prop.The component no longer destructures or forwards
onConfigChange, but it's still declared onMosaicChartPropsandChartBlockRenderercontinues to passonConfigChange={handleConfigChange}(which is therefore dead). Consider dropping the prop here and thehandleConfigChangewiring inChartBlockRendererto avoid a misleading API surface.🤖 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/mosaic/src/charts/MosaicChart.tsx` around lines 6 - 14, Remove the now-dead onConfigChange API from MosaicChartProps and clean up the unused wiring in ChartBlockRenderer. Update MosaicChart to no longer declare onConfigChange, and remove handleConfigChange plus the onConfigChange={handleConfigChange} prop passing where ChartBlockRenderer renders MosaicChart. Keep the remaining props and rendering logic intact so the public surface matches actual usage.packages/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsx (1)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tsdoc to the exported props type and component.
DataTableSettingsPanelPropsandDataTableSettingsPanelare public exports of an@sqlrooms/*package but lack tsdoc.As per coding guidelines: "Add tsdoc to exported types and functions in
@sqlrooms/*packages".📝 Proposed doc additions
+/** + * Props for {`@link` DataTableSettingsPanel}. + */ export type DataTableSettingsPanelProps = { value: DataTable | undefined; onChange: (table: DataTable) => void; }; +/** + * Settings panel for selecting the dataset (table) backing a data-table block. + */ export const DataTableSettingsPanel: FC<DataTableSettingsPanelProps> = ({ value, onChange, }) => {🤖 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/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsx` around lines 7 - 15, Add tsdoc comments to the public exports in DataTableSettingsPanel.tsx: document DataTableSettingsPanelProps and DataTableSettingsPanel since they are exported from an `@sqlrooms/`* package. Keep the docs brief but explicit about what the props type represents and what the component does, and place them directly above the DataTableSettingsPanelProps type and the DataTableSettingsPanel component declaration.Source: Coding guidelines
apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsx (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the
dashboardId!non-null assertion.
updateBlock(dashboardId!, ...)relies on the assumption thatdataTableBlockonly resolves whendashboardIdis defined. WhileuseDataTableBlockreturnsundefinedfor an undefineddashboardId, the!silently breaks if that contract changes. Guard explicitly instead.🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsx` at line 30, Avoid using the dashboardId! non-null assertion in BlockDataTableSettings; the updateBlock call should explicitly guard that dashboardId is defined before attempting the update. Locate the logic in BlockDataTableSettings where updateBlock is invoked, and restructure it so the code exits early or conditionally calls updateBlock only when dashboardId is present, instead of relying on the useDataTableBlock contract and a silent assertion.apps/sqlrooms-cli-ui/src/workspace/block-settings/useConfirmDatasetChange.ts (1)
4-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused test for the confirmation flow.
This hook now gates dataset switches and has enough branching (
request,confirm,cancel) that a small hook test is worth adding. Without it, regressions here can silently bypass confirmation or leave the dialog stuck open. As per coding guidelines,**/*.{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 `@apps/sqlrooms-cli-ui/src/workspace/block-settings/useConfirmDatasetChange.ts` around lines 4 - 31, Add a focused unit test for useConfirmDatasetChange covering the request/confirm/cancel flow. Verify that handleTableChangeRequest opens the dialog by setting pendingTable and isDialogOpen, handleConfirm calls onConfirmed with the pending DataTable and then clears state, and handleCancel clears pendingTable without calling onConfirmed. Use the hook’s returned methods and state from useConfirmDatasetChange to locate the behavior and keep the test aligned with the branching logic.Source: Coding guidelines
packages/documents/src/block-settings/useSelectedBlockOrPanel.ts (1)
74-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for selection precedence and editor lifecycle.
This hook now owns panel-vs-block precedence, panel clearing, and TipTap subscription behavior, but the change set doesn't include tests for those branches. A small hook/component test around panel priority, initial editor selection, and clearing panel selection on block selection would make this migration much safer.
As per coding guidelines,
**/*.{js,ts,jsx,tsx}should "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/documents/src/block-settings/useSelectedBlockOrPanel.ts` around lines 74 - 139, Add tests for useSelectedBlockOrPanel to cover the new selection precedence and lifecycle behavior: verify that panelSelection from useBlockSettingsStore wins over getEditorSelection(editor), that the hook returns the initial editor selection when no panel selection exists, and that a TipTap selectionUpdate causing a block selection triggers clearSelection and unsubscribes correctly on cleanup. Use the hook name useSelectedBlockOrPanel plus the editor.on/editor.off and clearSelection interactions to locate the behavior.Source: Coding guidelines
packages/documents/src/block-settings/useBlockSettingsStore.ts (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd tsdoc to the exported hook.
useBlockSettingsStoreis a public package export and should document its selector contract. The inline(state) => selector(state)wrapper is also redundant —selectorcan be passed directly touseBaseRoomStore.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/documents/src/block-settings/useBlockSettingsStore.ts` around lines 4 - 10, Add TSDoc to the exported hook `useBlockSettingsStore` so its selector contract is documented for public package consumers. Also simplify the implementation by passing the incoming `selector` directly to `useBaseRoomStore` instead of wrapping it in `(state) => selector(state)`.Source: Coding guidelines
packages/documents/src/block-settings/SelectablePanelWrapper.tsx (1)
83-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelection is mouse-only.
The wrapper selects via
onClickon a plaindivwith norole/tabIndex/keyboard handler, so panels can't be selected via keyboard. Consider adding keyboard affordances if panel selection is meant to be operable without a pointer.🤖 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/documents/src/block-settings/SelectablePanelWrapper.tsx` around lines 83 - 95, The panel selection in SelectablePanelWrapper is mouse-only because the clickable div uses onClick without keyboard accessibility support. Update the wrapper to expose keyboard affordances by adding the appropriate role and tabIndex, and wire a keyboard handler alongside handleClick so users can select the panel with the keyboard as well. Keep the changes centered on SelectablePanelWrapper and its existing selection logic.packages/documents/src/index.ts (1)
161-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-exporting
@tiptap/react'sEditorcouples the public API to TipTap.Re-exporting the third-party
Editortype ties this package's public surface to the@tiptap/reactversion. It's reasonable if consumers must pass anEditorto exported hooks (e.g.useSelectedBlockOrPanel), just be aware it leaks a dependency type. No change required if intentional.As per coding guidelines: "Treat public exports in
packages/as library APIs: keep them generic, composable, documented, and free of app/example-specific assumptions".🤖 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/documents/src/index.ts` around lines 161 - 162, The public API in index.ts is leaking the third-party `@tiptap/react` Editor type, which couples this package to TipTap; review whether the export is actually required by consumers of hooks like useSelectedBlockOrPanel, and if not, remove the re-export or replace it with a package-local, generic type/alias that keeps the surface independent of TipTap. Keep the block-settings export as-is, but avoid exposing external dependency types from the package entrypoint unless they are truly part of the intended library contract.Source: Coding guidelines
packages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx (2)
363-367: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
onEditorReadymay fire more than once.If a consumer passes an inline (non-memoized)
onEditorReady, this effect re-runs on every identity change and re-invokes the callback with the same editor. Consider gating on a ref so it fires once per editor instance, or document that consumers must memoize the callback.🤖 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/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx` around lines 363 - 367, The `useEffect` in `BlockDocumentEditorRoot` can call `onEditorReady` multiple times for the same editor when the callback identity changes. Update the effect so it only fires once per editor instance by tracking the last notified editor with a ref, or otherwise guard against repeated invocations; use the `editor` and `onEditorReady` symbols in `BlockDocumentEditorRoot` to implement the fix.
75-89: 📐 Maintainability & Code Quality | 🔵 TrivialPrefer
Nodefrom@tiptap/pm/modeloveranyand structural types.Replace the loose structural interface and
anywith the actual ProseMirrorNodetype for type safety and consistency with the rest of the editor codebase.
- Import
Nodefrom@tiptap/pm/model:import {type Node} from '`@tiptap/pm/model`';- Update
findNodePositionByIdsignature:function findNodePositionById( doc: Node, nodeId: string, ): number | null { let foundPos: number | null = null; doc.descendants((node: Node, pos: number) => { if (node.attrs?.id === nodeId) { foundPos = pos; return false; } }); return foundPos; }Since
tr.doc(used at the call site) is already aNode, this change removes unnecessary abstraction andanycasts.🤖 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/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx` around lines 75 - 89, `findNodePositionById` is using a loose structural doc type and `any`, but the editor already works with ProseMirror `Node` values. Update the helper in `BlockDocumentEditorRoot` to import `Node` from `@tiptap/pm/model`, change the `doc` parameter to `Node`, and type the `descendants` callback node as `Node` as well. This keeps the helper aligned with the `tr.doc` call site and removes the unnecessary abstraction and unsafe `any`.
🤖 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 `@apps/sqlrooms-cli-ui/src/store.ts`:
- Around line 1048-1056: The timeout callback is calling selectBlock through the
wrong store slice key, which will be undefined at runtime. Update the getState()
access in the block selection flow to use blockSettings.selectBlock instead of
blockSelection.selectBlock, and keep the existing standalone-block arguments
unchanged so the callback resolves the method from the correct slice.
In `@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockChartSettings.tsx`:
- Around line 16-29: Move the `useDataTable` hook out of the conditional render
path in `BlockChartSettings` so hooks are called in a stable order on every
render. The current early return before `useDataTable(tableName)` can cause a
hooks mismatch when `chartBlock` changes from null to present. Refactor by
extracting the chart-specific JSX into a child component that receives the
resolved `chartBlock`, and call `useDataTable` inside that child alongside
`ChartConfig`/`tableName` usage.
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDashboardSettings.tsx`:
- Around line 30-40: The hooks in BlockDashboardSettings are being called after
an early return, which breaks the Rules of Hooks when dashboard is missing. Move
useDataTable(tableName) and useTablesWithColumns() to the top of
BlockDashboardSettings so they always run in the same order before the dashboard
null check, then keep the existing conditional return for the missing-dashboard
UI.
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsx`:
- Around line 16-34: The hook order in BlockDataTableSettings is inconsistent
because useDataTable(tableName) is called after the early return for a missing
dataTableBlock. Move the useDataTable call above the conditional guard so it
runs on every render, and keep the existing dataTableBlock null check only for
the returned UI; use the BlockDataTableSettings component, dataTableBlock, and
useDataTable symbol locations to make the change.
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardChartSettings.tsx`:
- Around line 56-80: DashboardChartSettings is calling hooks conditionally
because useMemo and useDataTable run after early returns, which breaks the Rules
of Hooks. Move the hook calls to the top of the component before any conditional
return, or split the loaded chart UI into a child component so
DashboardChartSettings always invokes the same hooks in the same order. Keep the
existing dashboard/panel checks, but ensure useMemo, panel lookup, and
useDataTable are not skipped on any render.
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardDataTableSettings.tsx`:
- Around line 32-41: Move all hook calls in the affected settings components to
the top of each component so they run unconditionally, including useDataTable
and useTablesWithColumns in DashboardDataTableSettings, BlockDashboardSettings,
BlockDataTableSettings, BlockChartSettings, and DashboardChartSettings. Keep the
early returns for missing dashboard/dataTableBlock/chartBlock, but compute hook
inputs with optional chaining like dashboard?.selectedTable or similar so the
hook order never changes between renders.
In
`@docs/superpowers/specs/2026-06-25-tiptap-block-selection-migration-design.md`:
- Around line 141-176: Update the migration spec so the referenced
implementation symbols point to the new block-settings location instead of the
stale block-selection paths. In the BlockSettingsPanel section, keep the
guidance centered on useSelectedBlockOrPanel(editor) and BlockSettingsPanel, but
replace any package/path references that still point at block-selection with the
actual block-settings modules used in this PR. Also scan the related
BlockSelectionSlice references in the later section and align those doc
references to the new location so the guide matches the implementation.
In `@packages/documents/src/block-settings/BlockSettingsPanel.tsx`:
- Around line 9-18: The BlockSettingsPanelProps type currently allows an
explicit editor without the required documentId, which leads to silent null
settings in useBlockSettings when TipTap blocks are selected. Update
BlockSettingsPanelProps to a discriminated union so the editor-present branch
requires documentId, while the context-based branch can omit it; keep the change
localized to the BlockSettingsPanel props definition and ensure any consumers of
BlockSettingsPanel satisfy the new contract.
In `@packages/documents/src/block-settings/BlockSettingsSlice.ts`:
- Around line 30-33: `selectedBlock` should not live in
`BlockSettingsSliceConfig` because it is transient UI state; move it out of the
persisted config shape into the runtime/non-persisted state used by the block
settings store, and update any related types/usages in `BlockSettingsSlice` and
`createBlockSettingsSlice` accordingly. Also correct the TSDoc example to
reference the actual exported function name `createBlockSettingsSlice` instead
of `createBlockSelectionSlice`, and keep the config slice limited to
serializable document data.
- Around line 112-122: The TSDoc example for the exported block settings slice
references the wrong helper name, so update the example to use
createBlockSettingsSlice instead of createBlockSelectionSlice in
BlockSettingsSlice’s documentation. Keep the snippet aligned with the actual
exported API and ensure the example still shows the settingsRegistry usage with
createRoomStore and ChartSettings.
In `@packages/documents/src/block-settings/types.ts`:
- Around line 3-15: Document the exported block-settings contract by adding
TSDoc to each exported type in types.ts: SelectedBlock,
BlockSettingsComponentProps, and BlockSettingsComponent. Describe what each type
represents, the meaning of fields like type, id, dashboardId, and panelType, and
note that BlockSettingsComponent is a component type for block settings UI so
other `@sqlrooms` packages can consume the API clearly.
In `@packages/documents/src/block-settings/useSelectedBlockOrPanel.ts`:
- Around line 98-120: The selection sync in useSelectedBlockOrPanel is only
happening on the selectionUpdate subscription, so editorSelection can stay stale
when editor changes or starts out unavailable. Update the useEffect in
useSelectedBlockOrPanel to clear the local selection state when editor is
missing, and call updateSelection immediately after confirming editor exists
before registering the selectionUpdate listener. Keep the fix scoped to the
existing updateSelection logic and the editor lifecycle in the effect so the
panel state always reflects the current editor selection.
In
`@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsx`:
- Around line 76-98: The NodeViewWrapper click handler in
BlockDocumentChartNodeView is intercepting clicks from interactive chart
controls and also has an unused mouse event parameter. Update handleClick so
wrapper clicks do not interfere with child interactions by stopping propagation
or limiting setNodeSelection to background-only clicks, and remove the unused e
parameter if it is no longer needed.
In `@packages/mosaic/src/charts/ChartSettingsPanel.tsx`:
- Around line 10-20: Add tsdoc comments to the public exports
`ChartSettingsPanelProps` and `ChartSettingsPanel` in `ChartSettingsPanel.tsx`.
Document the purpose of the props type, the component, and any key
props/behavior so the exported API follows the `@sqlrooms/*` tsdoc requirement.
Keep the comments directly above the `ChartSettingsPanelProps` type and the
`ChartSettingsPanel` component definition.
---
Nitpick comments:
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsx`:
- Line 30: Avoid using the dashboardId! non-null assertion in
BlockDataTableSettings; the updateBlock call should explicitly guard that
dashboardId is defined before attempting the update. Locate the logic in
BlockDataTableSettings where updateBlock is invoked, and restructure it so the
code exits early or conditionally calls updateBlock only when dashboardId is
present, instead of relying on the useDataTableBlock contract and a silent
assertion.
In
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/useConfirmDatasetChange.ts`:
- Around line 4-31: Add a focused unit test for useConfirmDatasetChange covering
the request/confirm/cancel flow. Verify that handleTableChangeRequest opens the
dialog by setting pendingTable and isDialogOpen, handleConfirm calls onConfirmed
with the pending DataTable and then clears state, and handleCancel clears
pendingTable without calling onConfirmed. Use the hook’s returned methods and
state from useConfirmDatasetChange to locate the behavior and keep the test
aligned with the branching logic.
In `@packages/deck/src/dashboard.tsx`:
- Around line 576-598: The warning indicator in the missingColumns tooltip block
is still rendered as a button even though its onClick is empty, so it should no
longer be interactive. Update the TooltipTrigger usage in dashboard.tsx so the
alert icon is wrapped with a non-interactive element instead of a <button>, and
remove the empty click handler to keep the indicator accessible and not
announced as an action.
In `@packages/deck/src/MapSettings.tsx`:
- Line 159: There is a redundant duplicate subscription to the same
dashboard-selected table in MapSettings; reuse a single
useDataTable(dashboardSelectedTable) result instead of calling it twice. Update
the selectedDataTable and primaryTable logic so the existing selectedDataTable
value is shared wherever the identical argument is needed, and remove the extra
useDataTable(dashboardSelectedTable) call in the MapSettings component.
- Around line 116-121: Remove the unused onClose prop from MapSettingsPanelProps
in MapSettingsPanel because it is no longer consumed by the component and is not
passed by BlockMapSettings or DashboardMapSettings. Update the
MapSettingsPanelProps interface to only include the active props, and make sure
any related destructuring or type references in MapSettings.tsx stay consistent
after removing onClose.
In `@packages/documents/src/block-settings/SelectablePanelWrapper.tsx`:
- Around line 83-95: The panel selection in SelectablePanelWrapper is mouse-only
because the clickable div uses onClick without keyboard accessibility support.
Update the wrapper to expose keyboard affordances by adding the appropriate role
and tabIndex, and wire a keyboard handler alongside handleClick so users can
select the panel with the keyboard as well. Keep the changes centered on
SelectablePanelWrapper and its existing selection logic.
In `@packages/documents/src/block-settings/useBlockSettingsStore.ts`:
- Around line 4-10: Add TSDoc to the exported hook `useBlockSettingsStore` so
its selector contract is documented for public package consumers. Also simplify
the implementation by passing the incoming `selector` directly to
`useBaseRoomStore` instead of wrapping it in `(state) => selector(state)`.
In `@packages/documents/src/block-settings/useSelectedBlockOrPanel.ts`:
- Around line 74-139: Add tests for useSelectedBlockOrPanel to cover the new
selection precedence and lifecycle behavior: verify that panelSelection from
useBlockSettingsStore wins over getEditorSelection(editor), that the hook
returns the initial editor selection when no panel selection exists, and that a
TipTap selectionUpdate causing a block selection triggers clearSelection and
unsubscribes correctly on cleanup. Use the hook name useSelectedBlockOrPanel
plus the editor.on/editor.off and clearSelection interactions to locate the
behavior.
In `@packages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx`:
- Around line 363-367: The `useEffect` in `BlockDocumentEditorRoot` can call
`onEditorReady` multiple times for the same editor when the callback identity
changes. Update the effect so it only fires once per editor instance by tracking
the last notified editor with a ref, or otherwise guard against repeated
invocations; use the `editor` and `onEditorReady` symbols in
`BlockDocumentEditorRoot` to implement the fix.
- Around line 75-89: `findNodePositionById` is using a loose structural doc type
and `any`, but the editor already works with ProseMirror `Node` values. Update
the helper in `BlockDocumentEditorRoot` to import `Node` from
`@tiptap/pm/model`, change the `doc` parameter to `Node`, and type the
`descendants` callback node as `Node` as well. This keeps the helper aligned
with the `tr.doc` call site and removes the unnecessary abstraction and unsafe
`any`.
In `@packages/documents/src/index.ts`:
- Around line 161-162: The public API in index.ts is leaking the third-party
`@tiptap/react` Editor type, which couples this package to TipTap; review whether
the export is actually required by consumers of hooks like
useSelectedBlockOrPanel, and if not, remove the re-export or replace it with a
package-local, generic type/alias that keeps the surface independent of TipTap.
Keep the block-settings export as-is, but avoid exposing external dependency
types from the package entrypoint unless they are truly part of the intended
library contract.
In `@packages/mosaic/src/charts/MosaicChart.tsx`:
- Around line 6-14: Remove the now-dead onConfigChange API from MosaicChartProps
and clean up the unused wiring in ChartBlockRenderer. Update MosaicChart to no
longer declare onConfigChange, and remove handleConfigChange plus the
onConfigChange={handleConfigChange} prop passing where ChartBlockRenderer
renders MosaicChart. Keep the remaining props and rendering logic intact so the
public surface matches actual usage.
In `@packages/mosaic/src/charts/MosaicChartSettingsPanel.tsx`:
- Around line 19-34: The spec viewer path in MosaicChartSettingsPanel is
unreachable because viewMode can no longer be set to 'spec'. Remove the unused
viewMode state, setViewMode, handleBackToSettings callback, and the conditional
branch that renders MosaicChartSpecViewerPanel; keep the component focused on
the settings view and preserve the existing renderContext logic only if it is
still needed elsewhere.
In `@packages/mosaic/src/charts/worksheet/ChartBlockHeader.tsx`:
- Around line 5-19: ChartBlockHeader still declares unused required props, so
remove the dead chartConfig and onSettingsOpenChange entries from
ChartBlockHeaderProps and from the ChartBlockHeader component signature. Then
update ChartBlockRenderer to stop computing handleSettingsOpenChange and to stop
passing those props into ChartBlockHeader, since they are no longer used after
the settings toggle was removed.
In `@packages/mosaic/src/components/MultiFieldSelector.tsx`:
- Around line 74-78: The grid in MultiFieldSelector’s field row now uses a plain
1fr first column, which removes the previous minimum width floor. Review the
style on the row container and, if the ColumnSelector should not shrink too far
in narrow layouts, restore a minmax(...) minimum for the first track; otherwise
leave it as-is only if label truncation is intended.
In `@packages/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsx`:
- Around line 7-15: Add tsdoc comments to the public exports in
DataTableSettingsPanel.tsx: document DataTableSettingsPanelProps and
DataTableSettingsPanel since they are exported from an `@sqlrooms/`* package. Keep
the docs brief but explicit about what the props type represents and what the
component does, and place them directly above the DataTableSettingsPanelProps
type and the DataTableSettingsPanel component declaration.
🪄 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: 9a7a6d00-db01-494d-b76a-ffd3917f9992
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.husky/pre-pushapps/sqlrooms-cli-ui/src/store-types.tsapps/sqlrooms-cli-ui/src/store.tsapps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/ResizableSettingsPanelLayout.tsxapps/sqlrooms-cli-ui/src/workspace/WorksheetArtifact.tsxapps/sqlrooms-cli-ui/src/workspace/WorksheetDashboardBlockRenderer.tsxapps/sqlrooms-cli-ui/src/workspace/WorksheetMapBlockRenderer.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockChartSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDashboardSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockMapSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/ConfirmDatasetChangeDialog.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardChartSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardDataTableSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardMapSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/registry.tsapps/sqlrooms-cli-ui/src/workspace/block-settings/useChartBlock.tsapps/sqlrooms-cli-ui/src/workspace/block-settings/useConfirmDatasetChange.tsapps/sqlrooms-cli-ui/src/workspace/block-settings/useDataTableBlock.tsapps/sqlrooms-cli-ui/src/workspace/dashboard/DashboardArtifact.tsxdocs/superpowers/specs/2026-06-25-tiptap-block-selection-migration-design.mdpackage.jsonpackages/deck/src/MapSettings.tsxpackages/deck/src/dashboard.tsxpackages/deck/src/index.tspackages/documents/package.jsonpackages/documents/src/BlockDocumentArtifact.tsxpackages/documents/src/BlockDocumentEditor/BlockDocumentEditorContent.tsxpackages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsxpackages/documents/src/BlockDocumentEditor/extensions/BlockDocumentChartNode.tsxpackages/documents/src/BlockDocumentEditor/extensions/BlockDocumentStatefulBlockNode.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsxpackages/documents/src/block-settings/BlockSettingsPanel.tsxpackages/documents/src/block-settings/BlockSettingsSlice.tspackages/documents/src/block-settings/SelectablePanelWrapper.tsxpackages/documents/src/block-settings/SettingsErrorBoundary.tsxpackages/documents/src/block-settings/index.tspackages/documents/src/block-settings/types.tspackages/documents/src/block-settings/useBlockSettings.tspackages/documents/src/block-settings/useBlockSettingsStore.tspackages/documents/src/block-settings/useSelectedBlockOrPanel.tspackages/documents/src/index.tspackages/mosaic/src/charts/ChartSettingsPanel.tsxpackages/mosaic/src/charts/MosaicChart.tsxpackages/mosaic/src/charts/MosaicChartSettingsPanel.tsxpackages/mosaic/src/charts/chart-settings/MosaicChartSettings.tsxpackages/mosaic/src/charts/dashboard/MosaicDashboardChartHeaderActions.tsxpackages/mosaic/src/charts/worksheet/ChartBlockHeader.tsxpackages/mosaic/src/charts/worksheet/ChartBlockRenderer.tsxpackages/mosaic/src/components/AggregationSelector.tsxpackages/mosaic/src/components/MultiFieldSelector.tsxpackages/mosaic/src/dashboard/MosaicDashboard.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanel.tsxpackages/mosaic/src/dashboard/panel/MosaicDashboardPanelLayout.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardDataTableSelector.tsxpackages/mosaic/src/dashboard/toolbar/MosaicDashboardToolbar.tsxpackages/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsxpackages/mosaic/src/data-table-explorer/dashboard/MosaicDashboardDataTableExplorerPanelRenderer.tsxpackages/mosaic/src/data-table-explorer/worksheet/DataTableBlockHeader.tsxpackages/mosaic/src/data-table-explorer/worksheet/DataTableBlockRenderer.tsxpackages/mosaic/src/index.tspython/sqlrooms/sqlrooms/web/launcher.py
💤 Files with no reviewable changes (2)
- packages/mosaic/src/dashboard/toolbar/MosaicDashboardDataTableSelector.tsx
- packages/mosaic/src/data-table-explorer/worksheet/DataTableBlockHeader.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f023927aaf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/sqlrooms-cli-ui/src/workspace/block-settings/useMapBlock.ts (1)
17-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the raw block state instead of calling
getState()during render.This hook subscribes to
artifact, but derivesmapBlockfrom a separateuseRoomStore.getState()snapshot. That can drift from the subscribed value and makes the rerender contract depend onartifactchanging whenevergetBlocks(documentId)changes. Prefer selecting stable raw document/block state withuseRoomStore(...), then derive the matching block inuseMemo.As per coding guidelines, "When deriving values from Zustand store state in React, select stable raw state first, then derive values with React.useMemo inside the component".
🤖 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 `@apps/sqlrooms-cli-ui/src/workspace/block-settings/useMapBlock.ts` around lines 17 - 36, The map block lookup in useMapBlock is reading a separate snapshot via useRoomStore.getState() inside useMemo, which can drift from the subscribed store value. Refactor the hook to subscribe to the stable raw document/block state with useRoomStore first, then derive the matching map block from that selected state inside useMemo. Keep the existing artifact/documentId/blockId flow, but remove the render-time getState() access and derive from the subscribed state consistently.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
`@apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsx`:
- Around line 43-49: The DataTable settings UI is binding the panel’s “Title”
field to `dataTableBlock.caption`, which mixes up caption and title. Update
`BlockDataTableSettings` so `DataTableSettingsPanel` uses the real block title
for `title`/`onTitleChange`, or introduce a caption-specific prop and label if
caption editing is intended. Make sure the handler names and props in
`handleCaptionChange` and the `DataTableSettingsPanel` call reflect the correct
field being edited.
In `@packages/mosaic/src/charts/ChartSettingsPanel.tsx`:
- Around line 36-43: The Title Input in ChartSettingsPanel is receiving an
optional title value, which can make it start uncontrolled and later become
controlled; update the ChartSettingsPanel component so the Input always gets a
defined string value even when title is omitted. Use the existing title prop and
the Input onChange handler to keep behavior unchanged, but default the value
passed to Input to an empty string when title is undefined so the public
`@sqlrooms/mosaic` export is self-contained and avoids the React warning.
In `@packages/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsx`:
- Around line 8-21: Add tsdoc to the public exports in DataTableSettingsPanel by
documenting both DataTableSettingsPanelProps and DataTableSettingsPanel. The
props type should explain the purpose of value, onChange, title, and
onTitleChange, and the component tsdoc should briefly describe what the panel
renders and how it is used. Keep the existing inline field comment if useful,
but ensure the exported type and function themselves have proper tsdoc per the
`@sqlrooms/`* guideline.
- Around line 30-37: The Title Input in DataTableSettingsPanel is passing an
optional title directly to value, which can make the field switch from
uncontrolled to controlled. Update the Input in DataTableSettingsPanel to always
receive a string value, using a safe fallback when title is undefined, and keep
onTitleChange behavior unchanged. Use the DataTableSettingsPanel component and
its title/Input props to locate the fix.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/workspace/block-settings/useMapBlock.ts`:
- Around line 17-36: The map block lookup in useMapBlock is reading a separate
snapshot via useRoomStore.getState() inside useMemo, which can drift from the
subscribed store value. Refactor the hook to subscribe to the stable raw
document/block state with useRoomStore first, then derive the matching map block
from that selected state inside useMemo. Keep the existing
artifact/documentId/blockId flow, but remove the render-time getState() access
and derive from the subscribed state consistently.
🪄 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: 4159fdf8-d69a-4cd1-b0e9-724c85d5e66e
📒 Files selected for processing (13)
apps/sqlrooms-cli-ui/src/store.tsapps/sqlrooms-cli-ui/src/workspace/WorksheetMapBlockRenderer.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockChartSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockDataTableSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/BlockMapSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardDataTableSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardMapSettings.tsxapps/sqlrooms-cli-ui/src/workspace/block-settings/useMapBlock.tspackages/deck/src/MapSettings.tsxpackages/deck/src/block.tsxpackages/mosaic/src/charts/ChartSettingsPanel.tsxpackages/mosaic/src/data-table-explorer/DataTableSettingsPanel.tsxpackages/mosaic/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/mosaic/src/index.ts
- apps/sqlrooms-cli-ui/src/workspace/block-settings/DashboardMapSettings.tsx
- apps/sqlrooms-cli-ui/src/store.ts
- apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockMapSettings.tsx
- apps/sqlrooms-cli-ui/src/workspace/block-settings/BlockChartSettings.tsx
- apps/sqlrooms-cli-ui/src/workspace/WorksheetMapBlockRenderer.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: 01e22bb82f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const dashboard = useRoomStore((state) => | ||
| state.mosaicDashboard.getDashboard(blockId), |
There was a problem hiding this comment.
When a dashboard stateful block is auto-selected after creation or selected via TipTap node selection, blockId is the block-document node id, while the Mosaic dashboard state is created under the block's blockInstanceId. Looking up getDashboard(blockId) in that scenario makes the new/selected dashboard block show “Dashboard not found” until the user happens to click inside the embedded dashboard wrapper; pass the stateful block instance id through to these settings instead of using the document block id.
Useful? React with 👍 / 👎.
| <MosaicChartSettings.TypeSelector /> | ||
| <MosaicChartSettings.Fields /> |
There was a problem hiding this comment.
For spec-backed charts (renderContext.type === 'spec'), this component still has a branch that renders MosaicChartSpecViewerPanel, but this change removed the only ViewSpecButton path that set viewMode to 'spec'. In the new side settings panel, users can no longer open the spec viewer for those charts; add the view-spec control or another handler back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1706e1d22
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const handleTableChange = useCallback( | ||
| (table: DataTable) => { | ||
| if (dashboardId) { | ||
| setSelectedTable(dashboardId, table.table.table); |
There was a problem hiding this comment.
When an empty map panel is selected (the map add action creates one whenever no geospatial table is available), the new Dataset control reaches this handler, but updating only dashboard.selectedTable does not populate panel.config.datasets or layers. DeckMapDashboardRenderer therefore continues to render its “Map panels require at least one dataset” branch, and MapSettingsPanel still has no layer controls because layers.length is 0; use the same createDeckMapDashboardPanelConfigForTable/updatePanel path as the map block empty-state selector when confirming a dataset.
Useful? React with 👍 / 👎.
| isSelected && 'outline-primary outline outline-2 outline-offset-[-2px]', | ||
| className, | ||
| )} | ||
| onClick={handleClick} |
There was a problem hiding this comment.
When a dashboard panel header action such as Remove is clicked, the button handler removes the panel and then the click bubbles to this wrapper, which selects the now-deleted panelId. The settings pane is left resolving a stale selection and shows a not-found state after deletion instead of clearing; ignore clicks from header/action controls here or clear the selection as part of panel removal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 277f66d249
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| updateBlock(dashboardId!, blockId, { | ||
| ...dataTableBlock, | ||
| title: table.table.toString(), | ||
| }); |
There was a problem hiding this comment.
When a worksheet data-table block already has an active brush/filter, changing its dataset through the new side panel updates the block title but leaves the block-document:...:brush Mosaic selection alive. The removed header selector reset that selection before changing tables in DataTableBlockRenderer, so this new path can reopen the replacement table with predicates from the previous table or invalid filters applied; reset the same selection before calling updateBlock.
Useful? React with 👍 / 👎.
| const handleTableChange = useCallback( | ||
| (table: DataTable) => { | ||
| if (mapId) { | ||
| setSelectedTable(mapId, table.table.table); |
There was a problem hiding this comment.
If a standalone map block was initialized with the empty map config because no geospatial table existed, this side-panel handler only changes selectedTable; it never populates panel.config.datasets or layers. The embedded map therefore keeps rendering the empty-datasets state and the settings panel still has no layer controls after the user picks a dataset here; use the same createDeckMapDashboardPanelConfigForTable/updatePanel path as the map block's empty-state selector when the selected table can produce a map config.
Useful? React with 👍 / 👎.
…arts # Conflicts: # apps/sqlrooms-cli-ui/src/store.ts # packages/mosaic/src/dashboard/toolbar/MosaicDashboardDataTableSelector.tsx # packages/mosaic/src/dashboard/toolbar/MosaicDashboardToolbar.tsx # packages/mosaic/src/data-table-explorer/dashboard/MosaicDashboardDataTableExplorerPanelRenderer.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5145176e16
ℹ️ 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".
| onClick={() => { | ||
| // Settings now shown in side panel via block selection | ||
| }} |
There was a problem hiding this comment.
When a map has missing columns after a dataset/schema change, this warning icon is still rendered as a button, but its click handler is now a no-op; because SelectablePanelWrapper intentionally ignores button clicks, clicking the warning also will not select the panel and show the side settings. This leaves the previous “open settings to fix the broken columns” recovery path unavailable from the warning.
Useful? React with 👍 / 👎.
| updateDeckMapLayer(mapConfig, activeLayerIndex, (layer) => ({ | ||
| ...layer, | ||
| radiusMinPixels: value, | ||
| radiusMaxPixels: Math.max( |
There was a problem hiding this comment.
For point maps created by createDeckMapDashboardConfigForTable, the visible point size is stored as getRadius with radiusUnits: 'pixels' (default 4), but this slider now only writes radiusMinPixels. In that common case, choosing a smaller radius does not shrink the rendered points and the UI can report a radius like 2px while Deck still draws using the old getRadius; update getRadius for pixel-radius point layers as the previous handler did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60393609f6
ℹ️ 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 (selection && panelSelection) { | ||
| clearSelection(); |
There was a problem hiding this comment.
When a dashboard panel or other custom-selected item is active, clicking into normal document text changes TipTap to a TextSelection, so getEditorSelection() returns null and this branch leaves panelSelection intact. The settings pane therefore continues to edit the previously selected panel/block while the user has moved the cursor into a paragraph; clear the custom selection on non-node editor selections as well, not only when a new NodeSelection appears.
Useful? React with 👍 / 👎.
| setTimeout(() => { | ||
| const state = getState(); | ||
| state.blockSettings.selectBlock({ |
There was a problem hiding this comment.
This create callback is also run when stateful blocks appear through BlockDocumentsSlice.setConfig, setContent, or ensureBlockDocument (for example loading a workspace, syncing external content, or applying an agent update), not just when the user adds a block in the current editor. Scheduling selectBlock here can unexpectedly select whichever existing/remote block is processed last and override the user's current settings selection; move auto-selection to the explicit add-block UI path or gate it on a local editor-origin insert.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c6496c36b
ℹ️ 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".
| state.blockSettings.selectBlock({ | ||
| type: 'standalone-block', | ||
| id: blockId, | ||
| dashboardId: documentId, | ||
| panelType: blockType, |
There was a problem hiding this comment.
When a worksheet stateful block is inserted, this delayed custom selection takes priority over the TipTap node selection in useSelectedBlockOrPanel, but it does not include settingsComponent. The panel-selection path in useBlockSettings only renders selectedBlock.settingsComponent, so newly inserted data-table/map/dashboard blocks can open the settings pane as “No settings available” until the user clicks the block again; include the block type's settings component here or select the TipTap node instead.
Useful? React with 👍 / 👎.
| if (layer.radiusUnits === 'pixels') { | ||
| nextLayer.getRadius = value; |
There was a problem hiding this comment.
For scatterplot layers that already use a data-driven pixel radius, such as getRadius: '@@=Magnitude', dragging the point-radius slider enters this branch because radiusUnits is pixels and replaces the accessor with a numeric constant. That permanently drops the map's size encoding on the first adjustment; guard this assignment to numeric radii or leave string accessors intact while updating only clamp values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a158a32ff
ℹ️ 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 clearSelection = useBlockSettingsStore( | ||
| (state) => state.blockSettings.clearSelection, | ||
| ); |
There was a problem hiding this comment.
This remaining root-level subscription still dereferences state.blockSettings on every BlockDocumentEditorRoot render. Hosts that use @sqlrooms/documents with only createBlockDocumentsSlice—which was valid before this change and is still a public editor path—will have state.blockSettings undefined, so the editor crashes on mount even when no settings panel is rendered; the fresh evidence is that the optional guard added in BlockDocumentEditorContent does not cover this root-level hook. Make this cleanup optional or require/register createBlockSettingsSlice for all editor consumers.
Useful? React with 👍 / 👎.
| settingsProps: { | ||
| blockId: selectedItem.blockId, | ||
| dashboardId: documentId, | ||
| blockInstanceId: |
There was a problem hiding this comment.
When the selected block comes from a BlockDocumentEditor mounted with readOnly, these settings props only carry ids, so adapters such as ChartBlockSettings and DataTableBlockSettings still call updateBlock directly from the side panel. That bypasses the read-only guards already present in the node views, letting a read-only document's dataset/caption/config be mutated through BlockSettingsPanel; include readOnly in the settings props and have settings UIs disable or no-op writes in that context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75740c299e
ℹ️ 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".
| (title: string) => { | ||
| if (!panel) return; | ||
|
|
||
| updatePanel(dashboardId, panel.id, {title: title || undefined}); |
There was a problem hiding this comment.
In the read-only dashboard path, MosaicDashboardPanel already reads readOnly from context and passes it into selections, but the new inline title editor still calls this callback and writes through updatePanel unconditionally. When a dashboard is rendered inside a read-only block document, double-clicking a panel title and blurring the input can still mutate the dashboard state; skip passing onTitleChange or no-op here when readOnly is true.
Useful? React with 👍 / 👎.
| <LatitudeSelector | ||
| dashboardId={dashboardId} | ||
| panel={panel} | ||
| currentTable={dataTable} | ||
| /> |
There was a problem hiding this comment.
For read-only map settings, most controls are routed through applyConfig, which returns early, but these latitude/longitude selector components receive no read-only flag and call updatePanel directly inside their own handlers. On a read-only point-map block or panel, choosing a coordinate column from this settings pane still regenerates and persists the map config; pass a disabled/readOnly prop through these selectors or wrap their change handlers in the same read-only guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8955d8debb
ℹ️ 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".
| <div className="flex items-center gap-2"> | ||
| <MosaicDashboardAddPanelDropdown dashboardId={dashboardId} /> | ||
| <MosaicDashboardDataTableSelector dashboardId={dashboardId} /> | ||
| <MosaicDashboardAddPanelDropdown dashboardId={dashboardId} /> |
There was a problem hiding this comment.
When a dashboard is rendered inside a read-only block document, this dropdown remains enabled and still routes through useAddPanelActions to addPanel, so a viewer can mutate the dashboard by adding panels despite MosaicDashboard receiving readOnly. Use the dashboard context to hide/disable this control (and no-op the add callbacks) when readOnly is true.
Useful? React with 👍 / 👎.
|
|
||
| if (!panelId) return; | ||
|
|
||
| removePanel(dashboardId, panelId); |
There was a problem hiding this comment.
When readOnly is true for an embedded dashboard, title editing is blocked by omitting onTitleChange, but this remove handler still calls removePanel unconditionally. Clicking the trash button in a read-only dashboard panel deletes it from shared state, so the header needs a read-only prop or the remove action should be hidden/disabled in that context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0f967adb
ℹ️ 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".
| <BlockCaptionEditor | ||
| value={dashboardTitle} | ||
| placeholder={tableName || 'Dashboard title'} |
There was a problem hiding this comment.
When MosaicDashboard is rendered with readOnly (for example from a read-only block document), this toolbar still mounts BlockCaptionEditor without isReadOnly, while handleTitleChange writes through setDashboardTitle. In that read-only context, users can still rename the dashboard from the toolbar; read readOnly from useMosaicDashboardContext() and disable or no-op this editor.
Useful? React with 👍 / 👎.
| <SelectablePanelWrapper | ||
| dashboardId={dashboardId} | ||
| panelId={panel.id} | ||
| panelType="data-table-explorer" | ||
| blockType="dashboard-panel" |
There was a problem hiding this comment.
For a MosaicDashboard rendered with readOnly outside a BlockDocumentEditor, this nested SelectablePanelWrapper handles the click before the outer MosaicDashboardPanel wrapper can select the panel with its readOnly flag. Because no readOnly prop is passed here and the wrapper only falls back to BlockDocumentEditorContext, selecting a data-table panel body opens settings that can still update the panel/table; pass the dashboard context readOnly here or remove the redundant inner wrapper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1adae71dc2
ℹ️ 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".
|
|
||
| <div className="flex items-center gap-2"> | ||
| <MosaicDashboardAddPanelDropdown dashboardId={dashboardId} /> | ||
| <MosaicDashboardDataTableSelector dashboardId={dashboardId} /> |
There was a problem hiding this comment.
When MosaicDashboard is rendered with readOnly (for example inside a read-only block document), this toolbar still renders an active MosaicDashboardDataTableSelector; that component calls setSelectedTable unconditionally on selection, so a viewer can still mutate the dashboard's dataset even though title edits and add-panel actions are now guarded. Pass the read-only state through as disabled/a no-op handler or hide the selector in the same context.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
New Features
Bug Fixes
Chores