feat: Blocks and BlockDocument - #666

apps/sqlrooms-cli-ui/src/store.ts (1)

516-558: ⚡ Quick win

Consider logging when stateful block config is not found.

The onCreateOwnedStatefulBlock, onDeleteOwnedStatefulBlock, and onRenameOwnedStatefulBlock handlers silently skip operations when the blockType is not found in STATEFUL_BLOCK_ARTIFACT_CONFIGS (via optional chaining). This could hide bugs if a block type has a typo or isn't properly registered.

Consider adding debug logging when config is undefined to aid troubleshooting, or validate the blockType earlier in the call chain.

Example with debug logging

 onCreateOwnedStatefulBlock: ({
   blockInstanceId,
   blockType,
   getState,
   title,
 }) => {
   const config =
     STATEFUL_BLOCK_ARTIFACT_CONFIGS[
       blockType as keyof typeof STATEFUL_BLOCK_ARTIFACT_CONFIGS
     ];
+  if (!config) {
+    console.debug(`No stateful block config found for blockType: ${blockType}`);
+    return;
+  }
-  config?.ensureState(
+  config.ensureState(
     getState(),
     blockInstanceId,
     title ?? config.embeddedTitle,
   );
 },

🤖 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/store.ts` around lines 516 - 558, The handlers
onCreateOwnedStatefulBlock, onDeleteOwnedStatefulBlock and
onRenameOwnedStatefulBlock currently silently no-op when
STATEFUL_BLOCK_ARTIFACT_CONFIGS[blockType] is undefined; update each handler to
detect when config is falsy and emit a debug/warn log (e.g., console.warn or an
existing logger) including the blockType and blockInstanceId (and title where
relevant) so missing or misspelled block types are visible, then proceed with
the existing calls to config.ensureState, config.deleteState, or
config.renameState when config is present.

packages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx (1)

290-298: ⚡ Quick win

Memoize the context value to keep the provider stable.

contextValue (and the inline generateBlockId arrow) is allocated on every render, so every BlockDocumentEditorContext consumer re-renders even when nothing it depends on changed. Wrapping it in useMemo keeps the context reference stable.

♻️ Proposed refactor

-  const contextValue: BlockDocumentEditorContextValue = {
-    editor,
-    documentId,
-    value: normalizedValue,
-    assets,
-    onChange,
-    readOnly,
-    generateBlockId: () => generateBlockIdRef.current(),
-  };
+  const contextValue: BlockDocumentEditorContextValue = useMemo(
+    () => ({
+      editor,
+      documentId,
+      value: normalizedValue,
+      assets,
+      onChange,
+      readOnly,
+      generateBlockId: () => generateBlockIdRef.current(),
+    }),
+    [editor, documentId, normalizedValue, assets, onChange, readOnly],
+  );

As per coding guidelines: "Keep React context thin and stable."

🤖 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 290 - 298, The contextValue object (and the inline generateBlockId
arrow) is recreated on every render causing unnecessary re-renders; wrap the
BlockDocumentEditorContextValue creation in useMemo so the provider value stays
referentially stable, e.g. memoize contextValue and inside it keep
generateBlockId: () => generateBlockIdRef.current(), and list stable
dependencies [editor, documentId, normalizedValue, assets, onChange, readOnly]
so consumers only update when those actually change.

packages/documents/src/crdt.ts (1)

82-104: Block-document content stored as an opaque schema.Any() blob.

Storing the full ProseMirror body under schema.Any() means concurrent edits to the same block document body don't merge field-by-field — it's effectively whole-content last-write-wins, so simultaneous edits can clobber each other. This matches the existing markdown-snapshot approach and the in-file TODO, so it's consistent for now; just flagging that block-document bodies are typically richer/longer than a markdown string, which raises the practical likelihood of lost concurrent edits before the planned loro-prosemirror migration lands.

🤖 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/crdt.ts` around lines 82 - 104, The blockDocuments
entry uses content: schema.Any(), which stores the entire ProseMirror body as an
opaque blob and causes whole-document last-write-wins; change the schema to a
small structured wrapper to avoid clobbering and pave the loro-prosemirror
migration: replace content: schema.Any() with content: schema.LoroMap({ type:
schema.String(), body: schema.Any() }) (or another minimal typed map) in the
blockDocuments schema so edits can be scoped/validated and later replaced by the
full loro-prosemirror schema; update any create/read usage of blockDocuments to
access content.body and add a TODO referencing loro-prosemirror for the future
migration.

dev-docs/tasks/root-block-artifacts.md (1)

3-16: ⚡ Quick win

Make superseded status unambiguous by archiving/removing the active checklist.

The file says it is superseded, but the unchecked staged todo list reads like current work. Consider replacing the checklist with a short “Archived plan” note and a pointer to dev-docs/tasks/stateful-blocks-artifact-shells.md to avoid accidental execution of stale tasks.

🤖 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 `@dev-docs/tasks/root-block-artifacts.md` around lines 3 - 16, Remove the
active "Todo Checklist" block from this superseded document and replace it with
a short archival note indicating this doc is superseded; include a clear
one-line pointer to the canonical file stateful-blocks-artifact-shells.md and a
brief sentence like "See stateful-blocks-artifact-shells.md for the current
plan" so readers won't act on the stale checklist; target the "Todo Checklist"
section header to locate and delete the unchecked checklist items.

packages/artifacts/src/artifactTabs.tsx (1)

609-613: 💤 Low value

Minor: Inline callback creates new function on each render.

The onSelect callback creates a new inline function on every render. While this is unlikely to cause performance issues in practice, consider memoizing it with useCallback if the TabStrip component is sensitive to prop changes.

♻️ Optional memoization

+  const handleSelect = useCallback((artifactId: string) => {
+    artifactTabs.selectArtifact(artifactId);
+    onSelectArtifact?.(artifactId);
+  }, [artifactTabs, onSelectArtifact]);
+
   <TabStrip
     {...props}
     tabs={artifactTabs.tabs}
     openTabs={artifactTabs.openTabs}
     selectedTabId={artifactTabs.selectedTabId}
     closeable={closeable}
     preventCloseLastTab={preventCloseLastTab}
     onOpenTabsChange={artifactTabs.handleOpenTabsChange}
     onActivate={onActivateArtifact}
-    onSelect={(artifactId) => {
-      artifactTabs.selectArtifact(artifactId);
-      onSelectArtifact?.(artifactId);
-    }}
+    onSelect={handleSelect}

🤖 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/artifacts/src/artifactTabs.tsx` around lines 609 - 613, The inline
onSelect handler is recreated every render; wrap it in useCallback to
stable-reference the function passed to TabStrip: create a memoized handler
(e.g., const handleSelect = useCallback((artifactId) => {
artifactTabs.selectArtifact(artifactId); onSelectArtifact?.(artifactId); },
[artifactTabs, onSelectArtifact]) ) and pass handleSelect to the TabStrip
onSelect prop so the prop identity remains stable across renders.

packages/documents/src/BlockDocumentStatefulBlockRendererContext.tsx (1)

66-82: ⚡ Quick win

Memoize the context value to keep the provider stable.

supportedBlockTypes and the value object are rebuilt on every render, so all consumers of this context re-render whenever the provider re-renders (e.g. on any store update in WorksheetArtifact), even when renderers/blockTypes are unchanged.

♻️ Memoize derived value

 import {
   createContext,
   useContext,
+  useMemo,
   type FC,
   type PropsWithChildren,
 } from 'react';

 export const BlockDocumentStatefulBlockRendererProvider: FC<
   BlockDocumentStatefulBlockRendererProviderProps
 > = ({renderers = {}, blockTypes, children}) => {
-  const supportedBlockTypes =
-    blockTypes ??
-    Object.keys(renderers).map((blockType) => ({
-      blockType,
-    }));
-
-  return (
-    <BlockDocumentStatefulBlockRendererContext.Provider
-      value={{renderers, blockTypes: supportedBlockTypes}}
-    >
-      {children}
-    </BlockDocumentStatefulBlockRendererContext.Provider>
-  );
+  const value = useMemo(
+    () => ({
+      renderers,
+      blockTypes:
+        blockTypes ??
+        Object.keys(renderers).map((blockType) => ({blockType})),
+    }),
+    [renderers, blockTypes],
+  );
+
+  return (
+    <BlockDocumentStatefulBlockRendererContext.Provider value={value}>
+      {children}
+    </BlockDocumentStatefulBlockRendererContext.Provider>
+  );
 };

As per coding guidelines: "Keep React context thin and stable."

🤖 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/BlockDocumentStatefulBlockRendererContext.tsx` around
lines 66 - 82, BlockDocumentStatefulBlockRendererProvider recreates
supportedBlockTypes and the context value on every render causing unnecessary
consumer re-renders; wrap supportedBlockTypes computation and the Provider value
in useMemo so they only change when their inputs change (memoize
supportedBlockTypes derived from blockTypes or renderers, and memoize the object
passed to BlockDocumentStatefulBlockRendererContext.Provider) referencing the
BlockDocumentStatefulBlockRendererProvider, supportedBlockTypes, renderers,
blockTypes, and BlockDocumentStatefulBlockRendererContext.Provider symbols to
locate and update the code.

apps/sqlrooms-cli-ui/src/workspace/WorksheetChartRenderer.tsx (1)

34-36: 💤 Low value

Misleading helper name. stableStringify is a plain JSON.stringify and does not produce order-stable output (object key order is not normalized). Either implement key sorting or rename to avoid implying stability. Practical impact here is low since it only gates the normalized propagation, but the name is misleading for future use.

🤖 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/WorksheetChartRenderer.tsx` around lines
34 - 36, The helper stableStringify currently just calls JSON.stringify but its
name implies deterministic key-ordering; update the function stableStringify in
WorksheetChartRenderer.tsx to produce a stable deterministic string by
recursively serializing objects with sorted keys (e.g., implement a stable JSON
stringify that sorts Object.keys for each plain object and handles
arrays/primitive types), ensuring nested objects are also normalized so the
output is order-stable for use where normalized propagation is gated by this
value; alternatively if you prefer not to implement sorting, rename
stableStringify to something accurate like jsonStringify or stringifyUnstable
and update call sites to reflect the new name.

packages/documents/src/BlockDocumentEditor/extensions/BlockDocumentChartImageNode.tsx (1)

4-38: ⚡ Quick win

Extract a shared factory for image-like block nodes.

BlockDocumentChartImageNode and BlockDocumentImageNode duplicate the same TipTap node config (attrs, atom flags, parse/render scaffolding). Consider a small helper (e.g., createBlockDocumentAssetFigureNode) parameterized by name, dataType, and optional label to keep behavior aligned and reduce future drift.

🤖 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/extensions/BlockDocumentChartImageNode.tsx`
around lines 4 - 38, BlockDocumentChartImageNode and BlockDocumentImageNode
duplicate the same TipTap node config; extract a shared factory function (e.g.,
createBlockDocumentAssetFigureNode) that accepts parameters like name, dataType
and optional label and returns the Node.create configuration used by both nodes;
replace the explicit Node.create calls for BlockDocumentChartImageNode and
BlockDocumentImageNode with calls to
createBlockDocumentAssetFigureNode('blockDocumentChartImage','block-document-chart-image','Chart
image') (and analogous call for the other image node) so attributes (id,
assetId, caption), parseHTML/renderHTML scaffolding, and addNodeView remain
centralized and consistent.

packages/documents/src/BlockDocumentSliceConfig.ts (1)

19-43: 💤 Low value

Update Zod 4 object schemas to avoid deprecated .passthrough()
Zod 4 treats .passthrough() as legacy; prefer z.looseObject({...}) (and keep .strictObject for strictness) for BlockDocumentMark, BlockDocumentNode, and BlockDocumentContent in packages/documents/src/BlockDocumentSliceConfig.ts (lines 19-43) to prevent future breakage.

🤖 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/BlockDocumentSliceConfig.ts` around lines 19 - 43, The
schemas use the deprecated .passthrough(); replace each
z.object(...).passthrough() with z.looseObject({...}) and preserve the existing
shape — update BlockDocumentMark to z.looseObject({ type: z.string(), attrs:
z.record(z.string(), z.unknown()).optional() }), update BlockDocumentNode inside
its z.lazy to z.looseObject({ type: z.string(), attrs: z.record(z.string(),
z.unknown()).optional(), content: z.array(BlockDocumentNode).optional(), marks:
z.array(BlockDocumentMark).optional(), text: z.string().optional() }), and
update BlockDocumentContent to z.looseObject({ type: z.literal('doc'), content:
z.array(BlockDocumentNode).default([]) }); ensure imports still reference z and
tests/type usage still pass.

packages/mosaic/src/chart/MosaicDashboardChartContent.tsx (1)

30-31: 💤 Low value

Consider updating the props interface.

The connection and spec parameters are explicitly voided, indicating they're no longer used after refactoring to delegate to MosaicChartView. Consider removing them from MosaicDashboardChartContentProps if they're not needed for backward compatibility.

🤖 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/chart/MosaicDashboardChartContent.tsx` around lines 30 -
31, The props interface currently includes connection and spec which are
explicitly voided in MosaicDashboardChartContent; remove these unused properties
from MosaicDashboardChartContentProps and from the MosaicDashboardChartContent
component signature, update any places that construct or type the props
(including tests or parent components) to stop passing connection/spec, and
ensure MosaicChartView is used as the single source for those values; keep any
backward-compatibility typings only if required and update imports/usages of
MosaicDashboardChartContent to match the new prop shape.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes