Repository navigation
refactor(ai): move worksheet tools to block documents - #765
Merged
Merged
Conversation
Contributor
|
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 (10)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughWorksheet-scoped AI tools ( ChangesBlock-Document AI Tool Migration
Sequence Diagram(s)sequenceDiagram
participant Host as createWorksheetAgent
participant AgentTool as createWorksheetAgentTool
participant Adapter as BlockDocumentAiAdapter
participant ToolFactory as createWorksheetBlockDocumentAiTools
participant Agent as ToolLoopAgent
Host->>AgentTool: execute(worksheetId, intent)
AgentTool->>Adapter: ensureBlockDocument(worksheetId)
AgentTool->>Adapter: setCurrentBlockDocument(worksheetId)
AgentTool->>ToolFactory: create tools with blockDocumentAdapter
ToolFactory-->>AgentTool: Record<string, Tool>
AgentTool->>Agent: runSubAgent(tools, instructions)
Agent->>Adapter: addBlock or getBlocks via individual tools
Adapter-->>Agent: blockId or nodes
Agent-->>AgentTool: finalOutput
AgentTool-->>Host: WorksheetAgentResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ilyabo
marked this pull request as ready for review
June 23, 2026 07:25
![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
Point map updates at blockInstanceId
When experimental worksheet maps are enabled, the new generic lister no longer returns a top-level mapId; it returns the resource id as statefulBlock.blockInstanceId. This instruction still tells the agent to pass mapId, so update requests for an existing worksheet map can fail to target the existing block (or create a duplicate) depending on which conflicting prompt line the model follows. Update this text to use statefulBlock.blockInstanceId or restore a mapId alias in the list output.
ℹ️ 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".
| inputSchema: AddMosaicDashboardBlockToolInput, | ||
| execute: async ({dashboardTitle, tableName, intent}) => { | ||
| try { | ||
| const {dashboardId, block} = createDashboardBlock({ |
There was a problem hiding this comment.
When addBlock rejects (for example because the target block document was deleted or an adapter requires ensureBlockDocument before mutation), the host callback has already created durable dashboard state; in the CLI callback that means createDashboard/setSelectedTable have run, so the tool reports failure while leaving an unattached dashboard. Call blockDocumentAdapter.ensureBlockDocument(blockDocumentId) before createDashboardBlock, or roll back the created dashboard if appending the block fails.
Useful? React with 👍 / 👎.
ilyabo
force-pushed
the
codex/block-document-ai-refactor
branch
from
June 23, 2026 07:48
1038639 to
cbf9962
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.ts (1)
362-379: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrevent worksheet tools from shadowing data tools.
createWorksheetBlockDocumentAiToolsguards only against overriding worksheet built-ins. Since its result is spread afterdataTools, an extra tool namedquery,list_tables, orread_table_schemawould silently replace the core data tool and break analysis.Proposed collision check
const dataTools = options.createDataTools?.({store}) ?? {}; + const worksheetTools = createWorksheetBlockDocumentAiTools({ + databaseAdapter, + blockDocumentAdapter, + worksheetId, + chartToolsOptions, + dashboardAgentTool, + extraTools, + htmlAppBlocksEnabled, + createDashboardBlock, + createDataTableExplorerBlock, + }); + const duplicateToolNames = Object.keys(worksheetTools).filter( + (toolName) => toolName in dataTools, + ); + + if (duplicateToolNames.length > 0) { + throw new Error( + `Worksheet AI tools cannot override data tools: ${duplicateToolNames.join( + ', ', + )}`, + ); + } const agent = new ToolLoopAgent({ model: options.getModel({state: store.getState()}), tools: { ...dataTools, - ...createWorksheetBlockDocumentAiTools({ - databaseAdapter, - blockDocumentAdapter, - worksheetId, - chartToolsOptions, - dashboardAgentTool, - extraTools, - htmlAppBlocksEnabled, - createDashboardBlock, - createDataTableExplorerBlock, - }), + ...worksheetTools, },🤖 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/ai/createWorksheetAgentTool.ts` around lines 362 - 379, The tools object spreads dataTools first and then spreads the result of createWorksheetBlockDocumentAiTools second, which means any tools returned from createWorksheetBlockDocumentAiTools with names like query, list_tables, or read_table_schema will override the core data tools. Reorder the spread operators in the tools object so that dataTools is spread after createWorksheetBlockDocumentAiTools, ensuring that the core data tools take precedence and cannot be shadowed by worksheet-specific tools.
🤖 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/ai/createWorksheetBlockDocumentAiTools.ts`:
- Line 99: The `htmlAppBlocksEnabled` field is hardcoded to true, which allows
the `add_html_app_block` tool to be registered and the agent to call
`embedded_html_app_agent`, but this embedded tool is only available if the
optional `extraTools` parameter supplies it, creating a dead-end for default
configuration users. Either set `htmlAppBlocksEnabled` to false by default, or
add validation logic that checks whether `embedded_html_app_agent` is actually
present in `extraTools` when `htmlAppBlocksEnabled` is true, throwing an error
if it is not. Additionally, add tests to verify this behavior works correctly in
both scenarios (with and without the embedded tool available).
In `@packages/documents/src/createAddBlockDocumentTextBlockTool.ts`:
- Around line 117-121: In the execute function, before calling
blockDocumentAdapter.addBlock on line 120, you need to call ensureBlockDocument
with the blockDocumentId to ensure the block document exists. Add a call to
ensureBlockDocument(blockDocumentId) before the createBlockDocumentTextBlock
call or before the addBlock call to satisfy the precondition that the document
must exist before mutation operations are performed.
In
`@packages/mosaic/src/ai/block-document/createBlockDocumentDataTableExplorerTool.ts`:
- Around line 39-53: The addDataTable function calls
createDataTableExplorerBlock before verifying that the target block document
exists, which can cause the host-side block state to be created even if the
subsequent addBlock call fails. To fix this, call ensureBlockDocument to confirm
the block document exists before invoking createDataTableExplorerBlock.
Additionally, add a regression test that mirrors the dashboard missing-document
case to verify that the host callback is not invoked when ensureBlockDocument
fails.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.ts`:
- Around line 362-379: The tools object spreads dataTools first and then spreads
the result of createWorksheetBlockDocumentAiTools second, which means any tools
returned from createWorksheetBlockDocumentAiTools with names like query,
list_tables, or read_table_schema will override the core data tools. Reorder the
spread operators in the tools object so that dataTools is spread after
createWorksheetBlockDocumentAiTools, ensuring that the core data tools take
precedence and cannot be shadowed by worksheet-specific tools.
🪄 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: 74b3ccbf-2ed4-424f-93ec-0e98c2072897
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
apps/sqlrooms-cli-ui/src/ai/createAddHtmlAppBlockDocumentBlockTool.tsapps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.tsapps/sqlrooms-cli-ui/src/ai/createWorksheetBlockDocumentAiTools.tsapps/sqlrooms-cli-ui/src/createWorksheetAgent.tsapps/sqlrooms-cli-ui/src/createWorksheetAiAdapter.tspackages/documents/README.mdpackages/documents/__tests__/BlockDocumentAi.test.tspackages/documents/package.jsonpackages/documents/src/BlockDocumentAi.tspackages/documents/src/createAddBlockDocumentTextBlockTool.tspackages/documents/src/createListBlockDocumentBlocksTool.tspackages/documents/src/index.tspackages/mosaic/README.mdpackages/mosaic/__tests__/block-document-ai.test.tspackages/mosaic/__tests__/worksheet-ai.test.tspackages/mosaic/src/ai.tspackages/mosaic/src/ai/block-document/constants.tspackages/mosaic/src/ai/block-document/createAddMosaicDashboardBlockTool.tspackages/mosaic/src/ai/block-document/createBlockDocumentChartTools.tspackages/mosaic/src/ai/block-document/createBlockDocumentDataTableExplorerTool.tspackages/mosaic/src/ai/types.tspackages/mosaic/src/ai/worksheet/constants.tspackages/mosaic/src/ai/worksheet/createAddDashboardBlockTool.tspackages/mosaic/src/ai/worksheet/createAddHtmlAppBlockTool.tspackages/mosaic/src/ai/worksheet/createAddTextBlockTool.tspackages/mosaic/src/ai/worksheet/createListWorksheetBlocksTool.tspackages/mosaic/src/ai/worksheet/createWorksheetAiTools.tspackages/mosaic/src/ai/worksheet/createWorksheetChartTools.tspackages/mosaic/src/ai/worksheet/createWorksheetDataTableExplorerTool.tspackages/mosaic/src/ai/worksheet/worksheet-types.tspackages/mosaic/src/charts/chart-types/createChartTools.tspackages/mosaic/src/index.ts
💤 Files with no reviewable changes (10)
- packages/mosaic/src/ai/worksheet/createWorksheetChartTools.ts
- packages/mosaic/tests/worksheet-ai.test.ts
- packages/mosaic/src/ai/worksheet/createWorksheetAiTools.ts
- packages/mosaic/src/ai/worksheet/constants.ts
- packages/mosaic/src/ai/worksheet/createAddTextBlockTool.ts
- packages/mosaic/src/ai/worksheet/createAddHtmlAppBlockTool.ts
- packages/mosaic/src/ai/worksheet/createWorksheetDataTableExplorerTool.ts
- packages/mosaic/src/ai/worksheet/createAddDashboardBlockTool.ts
- packages/mosaic/src/ai/worksheet/worksheet-types.ts
- packages/mosaic/src/ai/worksheet/createListWorksheetBlocksTool.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf9962fae
ℹ️ 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".
| mapBlocksEnabled | ||
| ? '✅ For map requests, use the direct worksheet map block tool; call list_worksheet_blocks first when updating an existing map and pass its mapId' | ||
| : `✅ For map requests, use ${KnownWorksheetTools.list_blocks} then ${KnownWorksheetTools.embedded_dashboard_agent} so the map is added as a dashboard panel` | ||
| ? `- For map requests, use the direct worksheet map block tool; call ${KnownWorksheetTools.list_blocks} first when updating an existing map and pass its mapId` |
There was a problem hiding this comment.
When experimental map blocks are enabled, list_block_document_blocks no longer returns the old flat mapId; the new generic summarizer only exposes the stateful resource id as statefulBlock.blockInstanceId. Leaving this instruction as mapId (also repeated in the short tool description below) makes updates to an existing worksheet map likely call create_worksheet_map_block without the existing id, creating a duplicate map block instead of updating the one that was listed. Update the prompt to pass statefulBlock.blockInstanceId as the mapId.
Useful? React with 👍 / 👎.
ilyabo
force-pushed
the
codex/block-document-ai-refactor
branch
from
June 23, 2026 08:54
cbf9962 to
913f175
Compare
dmitriy-kostianetskiy
self-requested a review
June 23, 2026 11:12
dmitriy-kostianetskiy
approved these changes
Jun 23, 2026
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit