Repository navigation
feat: support rich text generation via AI - #810
Conversation
![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: 9e5c1186c6
ℹ️ 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".
| ...BlockDocumentTextBlockBase, | ||
| type: z.literal('paragraph'), | ||
| text: z.string(), | ||
| text: z.array(BlockDocumentNode), |
There was a problem hiding this comment.
For callers that still use the previously supported block DTO shape, such as appendBlocks(..., [{type: 'paragraph', text: 'Hello'}]), this schema now rejects the block before blockDocumentBlockToNode can convert it because BlockDocumentBlock.parse only accepts arrays. Supporting rich text does not require breaking existing plain-text command/API payloads; keep a string union/transform to preserve current JS callers and serialized command inputs while accepting rich text nodes.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughBlock document text and list items now use rich-text node arrays throughout schemas, conversions, creation/listing tools, and tests. CLI and worksheet AI block eligibility is computed from runtime-enabled types. Block-scoped chat actions now return sessions with content. ChangesRich-text block documents
Runtime AI block eligibility
Block-scoped chat session types
Presentation formatting
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/documents/src/BlockDocumentSliceConfig.ts (2)
82-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGeneric
BlockDocumentNodedoesn't constrain inline node types.
text/itemsfields now accept anyBlockDocumentNode(arbitrarytype: string), the same schema used for block-level nodes. Nothing stops a caller (especially the AI tool) from nesting a block-level node type (e.g.,heading) inside an inlinetextarray, producing a structurally invalid TipTap document that could break rendering downstream.Consider a narrower inline-node schema (e.g., restrict
typeto'text'/'hardBreak') fortext/itemsfields, separate from the generic node type used for document-level content.🤖 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 82 - 129, Introduce a dedicated inline-node schema restricted to supported inline types such as text and hardBreak, then update BlockDocumentHeadingBlock, BlockDocumentParagraphBlock, BlockDocumentTodoBlock, and BlockDocumentListBlock items to use it instead of the generic BlockDocumentNode. Keep BlockDocumentNode for document-level content while ensuring inline text and list item arrays reject block types like heading.
214-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicate text-extraction helpers.
textFromNodeinBlockDocumentSliceConfig.tsis no longer called by the block↔node converters after this migration, whilecreateListBlockDocumentBlocksTool.tsreimplements the same recursive text-flattening logic locally asextractTextFromNodes. One shared, exported helper avoids the duplication and dead code.
packages/documents/src/BlockDocumentSliceConfig.ts#L214-L221: export a single plain-text extraction helper (rename/reusetextFromNode, accepting either a node or node array) and droptextContentif confirmed unused.packages/documents/src/createListBlockDocumentBlocksTool.ts#L10-L24: remove the localextractTextFromNodesand import the shared helper fromBlockDocumentSliceConfig.tsinstead.🤖 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 214 - 221, Consolidate recursive text extraction into one exported helper: in packages/documents/src/BlockDocumentSliceConfig.ts lines 214-221, update textFromNode to accept a node or node array, export it, and remove textContent if unused; in packages/documents/src/createListBlockDocumentBlocksTool.ts lines 10-24, delete extractTextFromNodes and import/use the shared helper instead.packages/documents/src/createAddBlockDocumentTextBlockTool.ts (1)
17-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAI-facing
text/itemsaccept arbitrary nodetypevalues.Since
BlockDocumentNode.typeis an unconstrained string, the LLM tool caller could pass non-inline node types (e.g., a nestedheadingorlisttype) insidetext/items, which are meant to represent inline text runs. This is the primary entry point where malformed rich-text payloads would originate. Tightening the schema here (or in the sharedBlockDocumentNode, seeBlockDocumentSliceConfig.ts) to restrict inline content totext/mark-carrying nodes would prevent invalid documents from being persisted via the AI tool.🤖 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/createAddBlockDocumentTextBlockTool.ts` around lines 17 - 33, Restrict the AI-facing text and items schemas in createAddBlockDocumentTextBlockTool to inline text/mark-carrying node types instead of the unconstrained BlockDocumentNode type. Apply the constraint consistently to both the text array and each nested items array, while preserving supported marks and existing heading-level behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/documents/src/BlockDocumentSliceConfig.ts`:
- Around line 82-129: Introduce a dedicated inline-node schema restricted to
supported inline types such as text and hardBreak, then update
BlockDocumentHeadingBlock, BlockDocumentParagraphBlock, BlockDocumentTodoBlock,
and BlockDocumentListBlock items to use it instead of the generic
BlockDocumentNode. Keep BlockDocumentNode for document-level content while
ensuring inline text and list item arrays reject block types like heading.
- Around line 214-221: Consolidate recursive text extraction into one exported
helper: in packages/documents/src/BlockDocumentSliceConfig.ts lines 214-221,
update textFromNode to accept a node or node array, export it, and remove
textContent if unused; in
packages/documents/src/createListBlockDocumentBlocksTool.ts lines 10-24, delete
extractTextFromNodes and import/use the shared helper instead.
In `@packages/documents/src/createAddBlockDocumentTextBlockTool.ts`:
- Around line 17-33: Restrict the AI-facing text and items schemas in
createAddBlockDocumentTextBlockTool to inline text/mark-carrying node types
instead of the unconstrained BlockDocumentNode type. Apply the constraint
consistently to both the text array and each nested items array, while
preserving supported marks and existing heading-level behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 855c513b-0601-40d0-ab02-88993b04cd65
📒 Files selected for processing (9)
packages/documents/__tests__/BlockDocumentAi.test.tspackages/documents/__tests__/BlockDocumentCommands.test.tspackages/documents/__tests__/BlockDocumentRichText.test.tspackages/documents/__tests__/BlockDocumentsSlice.test.tspackages/documents/__tests__/documentsCrdt.test.tspackages/documents/src/BlockDocumentSliceConfig.tspackages/documents/src/createAddBlockDocumentTextBlockTool.tspackages/documents/src/createListBlockDocumentBlocksTool.tspackages/documents/src/startBlockScopedChat.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07aa64659b
ℹ️ 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.
🧹 Nitpick comments (1)
apps/sqlrooms-cli-ui/src/components/useAssistantContextItems.ts (1)
34-42: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the enabled block types to avoid repeated computations.
isEnabledCliBlockTypeis invoked repeatedly per node during React render cycles (inside hooks likeuseValidatedSelectedIdsanduseContextSelectorItems). SincegetEnabledStatefulBlockArtifactTypestraverses and filters the artifact configurations, computing it on every call adds unnecessary overhead.Consider lazily caching the result to optimize performance for worksheets with many blocks.
♻️ Proposed refactor
+let enabledCliBlockTypesCache: Set<string> | null = null; + function isEnabledCliBlockType(blockType: string): boolean { if (blockType === 'chart') return true; if (!isStatefulBlockArtifactType(blockType)) return false; - return ( - getEnabledStatefulBlockArtifactTypes(experimentalEnabled).includes( - blockType, - ) && CLI_BLOCK_CONTEXT_TYPES.has(blockType) - ); + + if (!enabledCliBlockTypesCache) { + enabledCliBlockTypesCache = new Set( + getEnabledStatefulBlockArtifactTypes(experimentalEnabled) + ); + } + + return enabledCliBlockTypesCache.has(blockType) && CLI_BLOCK_CONTEXT_TYPES.has(blockType); }🤖 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/components/useAssistantContextItems.ts` around lines 34 - 42, Cache the result of getEnabledStatefulBlockArtifactTypes(experimentalEnabled) within the surrounding assistant-context logic, reusing the cached enabled-type collection across repeated isEnabledCliBlockType calls during a render. Ensure the cache is invalidated or recreated when experimentalEnabled changes, while preserving the existing chart, stateful-type, and CLI_BLOCK_CONTEXT_TYPES checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/components/useAssistantContextItems.ts`:
- Around line 34-42: Cache the result of
getEnabledStatefulBlockArtifactTypes(experimentalEnabled) within the surrounding
assistant-context logic, reusing the cached enabled-type collection across
repeated isEnabledCliBlockType calls during a render. Ensure the cache is
invalidated or recreated when experimentalEnabled changes, while preserving the
existing chart, stateful-type, and CLI_BLOCK_CONTEXT_TYPES checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 322efc47-c1ba-44f9-92ef-3086e37b113f
📒 Files selected for processing (6)
apps/sqlrooms-cli-ui/src/__tests__/createCliBlockDocumentAiAdapter.test.tsapps/sqlrooms-cli-ui/src/components/useAssistantContextItems.tsapps/sqlrooms-cli-ui/src/statefulBlockArtifactConfigs.tsapps/sqlrooms-cli-ui/src/workspace/WorksheetArtifact.tsxpackages/documents/src/BlockDocumentSliceConfig.tspackages/documents/src/createAddBlockDocumentTextBlockTool.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/documents/src/createAddBlockDocumentTextBlockTool.ts
- packages/documents/src/BlockDocumentSliceConfig.ts
Summary by CodeRabbit