Repository navigation
feat: Improving tableSchema AI tools - #683
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces ChangesTable identity and schema context for AI tools
Widespread code formatting and layout adjustments
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI createDashboardAgent
participant Agent as ToolLoopAgent
participant Resolve as Dashboard resolveTable
participant Instr as createDefaultAiInstructions
participant Format as formatTablesForLLM
CLI->>Agent: createQueryTools() → createDefaultAiTools(..., {query:{}, commands:false})
Agent->>Resolve: resolveTable(dashboardId, tableName)
Resolve->>Resolve: resolveDashboardTable(match by id/qname/name)
Resolve->>Resolve: getResolvedTable(with column types)
Resolve-->>Agent: resolved table + column metadata
Agent->>Instr: createDefaultAiInstructions(tables, currentDatabase)
Instr->>Format: formatTablesForLLM(tables, currentDatabase, limits)
Format->>Format: getTablesForAiScope(filter by scope)
Format->>Format: formatTableSchemaForAi / formatTableSummaryForAi
Format->>Format: Budget-aware hybrid builder (full→summary→count)
Format-->>Instr: formatted table context string
Instr-->>Agent: DEFAULT_INSTRUCTIONS + context
Agent->>Agent: tools: {query, list_tables, read_table_schema, ...extra}
Agent-->>CLI: agent ready with all tools
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
# Conflicts: # packages/ai/src/tools/defaultTools.ts # packages/mosaic/src/ai.ts
![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: 10d47f9b83
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/package.json (1)
18-26:⚠️ Potential issue | 🟠 MajorAdd missing
ts-jestdependency to align with monorepo patterns and resolve knip validation.All 20+ packages using
@sqlrooms/preset-jestdeclarets-jestin devDependencies.packages/aiis missing this declaration while still using the preset-jest configuration, which causes knip to flag an unresolved dependency. Addts-jestto devDependencies to match the established pattern:Manifest fix
"scripts": { "build": "tsc", "dev": "tsc -w", "lint": "eslint .", "test": "jest", "test:watch": "jest --watch", "typecheck": "tsc --noEmit", "typedoc": "typedoc" }, + "devDependencies": { + "ts-jest": "^29.4.4" + },🤖 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/ai/package.json` around lines 18 - 26, The packages/ai package.json file is missing the ts-jest devDependency despite using the `@sqlrooms/preset-jest` configuration, which causes knip validation to fail. Add ts-jest to the devDependencies section of packages/ai/package.json to match the established pattern used consistently across 20+ other packages in the monorepo that also use the preset-jest configuration.Source: Pipeline failures
🤖 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/createDashboardToolDeps.ts`:
- Line 34: The tableName assignment on line 34 uses a fallback pattern that can
lose fully-qualified table identity and cause the agent to select the wrong
table when multiple databases have tables with identical names. Remove the
fallback logic and use table.tableName directly as the stable unique identifier
for internal routing. If a user-friendly display label is needed, derive it
separately rather than using it for internal agent selection.
In `@packages/ai/__tests__/tableSchemaContext.test.ts`:
- Around line 33-41: The test "does not filter by database when currentDatabase
is unavailable" is actually testing schema filtering rather than database
filtering behavior. The test creates two tables where one has schema 'analytics'
which causes it to be filtered out - but this proves schema filtering is
happening, not that database filtering is absent. To fix this test and prove
that database filtering is not happening when currentDatabase is unavailable,
create two tables that differ only in their database property (both with the
same or no schema), then assert that both tables are included in the
formatTablesForLLM output, demonstrating that the database values are not used
for filtering.
---
Outside diff comments:
In `@packages/ai/package.json`:
- Around line 18-26: The packages/ai package.json file is missing the ts-jest
devDependency despite using the `@sqlrooms/preset-jest` configuration, which
causes knip validation to fail. Add ts-jest to the devDependencies section of
packages/ai/package.json to match the established pattern used consistently
across 20+ other packages in the monorepo that also use the preset-jest
configuration.
🪄 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: 6445c543-6e4c-4d48-9dd1-ad2b12af75a2
📒 Files selected for processing (32)
apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxapps/sqlrooms-cli-ui/src/components/useLocalFileLoader.tsapps/sqlrooms-cli-ui/src/createDashboardAgent.tsapps/sqlrooms-cli-ui/src/createDashboardToolDeps.tsapps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/CliWorkspaceTopbar.tsxapps/sqlrooms-cli-ui/src/workspace/WorksheetSqlQueryBlockRenderer.tsxpackages/ai-core/src/components/Chat.tsxpackages/ai/README.mdpackages/ai/__tests__/tableSchemaContext.test.tspackages/ai/jest.config.jspackages/ai/package.jsonpackages/ai/src/index.tspackages/ai/src/tools/defaultInstructions.tspackages/ai/src/tools/tableSchemaContext.tspackages/artifacts/src/ai/artifactAiSessionHelpers.tspackages/artifacts/src/ai/artifactContextTools.tspackages/blocks/src/index.tspackages/documents/README.mdpackages/documents/src/BlockDocumentEditor/BlockDocumentBlockControls.tsxpackages/documents/src/MarkdownDocumentEditor/MarkdownDocumentEditorRoot.tsxpackages/mosaic/src/ai.tspackages/mosaic/src/ai/ai.tspackages/pivot/README.mdpackages/pivot/src/PivotBlock.tsxpackages/sql-editor/src/SqlEditorSlice.tsxpackages/sql-editor/src/SqlQueryBlock.tsxpackages/sql-editor/src/codemirror/extensions/completion.tspackages/sql-editor/src/codemirror/utils/schema-converter.tspackages/sql-editor/src/components/QueryEditorPanelActions.tsxpackages/sql-editor/src/components/QueryResultPanel.tsxpackages/ui/src/components/tree.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfc788ccd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3e1d8a47e
ℹ️ 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".
| getTablesForAiScope(state.db.tables, state.db.currentDatabase, { | ||
| scope: 'main', | ||
| }).map((table) => ({ |
There was a problem hiding this comment.
When a room has visible tables outside the current database's main schema (for example an attached DuckDB/MotherDuck database or another schema), this filter removes them from the dashboard adapter even though the dashboard tools validate table names only against adapter.getTables() before creating panels. In that scenario list_tables can still discover the table, but dashboard_agent/chart tools will report it as unknown instead of building the requested dashboard, which regresses the previous behavior that exposed all state.db.tables.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5840ad6e9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94aa86d08a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/tools/tables/tableIdentity.ts`:
- Around line 31-33: The condition in the `getAllTablesFromSchemaTrees` function
currently only includes nodes where `tableNode?.object?.type === 'table'`, but
it should also include nodes where the type is 'view' since both `list_tables`
and `read_table_schema` depend on this helper to access views from the schema
tree. Modify the condition to check for both 'table' and 'view' types by
updating the type comparison to accept either value.
🪄 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: a24f0285-a8aa-4625-91bd-bcabbf756535
📒 Files selected for processing (20)
apps/sqlrooms-cli-ui/src/createDashboardToolDeps.tspackages/ai/__tests__/tableSchemaContext.test.tspackages/ai/__tests__/tableTools.test.tspackages/ai/src/tools/defaultInstructions.tspackages/ai/src/tools/tableSchemaContext.tspackages/ai/src/tools/tables/listTables.tspackages/ai/src/tools/tables/readTableSchema.tspackages/ai/src/tools/tables/tableIdentity.tspackages/mosaic/__tests__/chart-types/line-chart-spec.test.tspackages/mosaic/__tests__/dashboard-ai.test.tspackages/mosaic/src/ai/ai.tspackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/count-plot/spec.tspackages/mosaic/src/charts/chart-types/custom-spec/spec.tspackages/mosaic/src/charts/chart-types/heatmap/spec.tspackages/mosaic/src/charts/chart-types/histogram/spec.tspackages/mosaic/src/charts/chart-types/line-chart/spec.tspackages/mosaic/src/charts/chart-types/scatter-plot/spec.tspackages/mosaic/src/charts/useChartDataPolicy.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ai/tests/tableSchemaContext.test.ts
- apps/sqlrooms-cli-ui/src/createDashboardToolDeps.ts
- packages/ai/src/tools/tableSchemaContext.ts
ilyabo
changed the title
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/src/tools/tables/tableIdentity.ts (1)
21-114: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc documentation for public exports.
The exported functions lack documentation. As per coding guidelines, "Public exports in
packages/should be kept generic, composable, documented, and free of app/example-specific assumptions." Adding JSDoc comments forgetCanonicalTableId,getAllTablesFromSchemaTrees,createTableIdentitySummary,resolveTableFromCatalog, andformatAmbiguousTableMessagewould help consumers understand the API and align with project standards.🤖 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/ai/src/tools/tables/tableIdentity.ts` around lines 21 - 114, Add JSDoc documentation comments to all five exported functions in the tableIdentity file: getCanonicalTableId, getAllTablesFromSchemaTrees, createTableIdentitySummary, resolveTableFromCatalog, and formatAmbiguousTableMessage. Each JSDoc should include a clear description of what the function does, parameter documentation with types, and return value documentation. This will provide API consumers with clear understanding of each function's purpose and usage.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.
Outside diff comments:
In `@packages/ai/src/tools/tables/tableIdentity.ts`:
- Around line 21-114: Add JSDoc documentation comments to all five exported
functions in the tableIdentity file: getCanonicalTableId,
getAllTablesFromSchemaTrees, createTableIdentitySummary,
resolveTableFromCatalog, and formatAmbiguousTableMessage. Each JSDoc should
include a clear description of what the function does, parameter documentation
with types, and return value documentation. This will provide API consumers with
clear understanding of each function's purpose and usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6f2eba6-53e0-42dc-9972-cf4916c2e4c0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/sqlrooms-cli-ui/src/createDashboardToolDeps.tspackages/ai/__tests__/tableTools.test.tspackages/ai/jest.config.jspackages/ai/package.jsonpackages/ai/src/tools/tables/tableIdentity.tspackages/duckdb-core/README.mdpackages/duckdb-core/__tests__/getUnqualifiedSqlIdentifier.test.tspackages/duckdb-core/src/duckdb-utils.tspackages/duckdb-core/src/index.tspackages/duckdb/README.mdpackages/duckdb/src/index.tspackages/mosaic/__tests__/BoxPlotClient.test.tspackages/mosaic/__tests__/dashboard-ai.test.tspackages/mosaic/src/ai/ai.tspackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.ts
💤 Files with no reviewable changes (1)
- apps/sqlrooms-cli-ui/src/createDashboardToolDeps.ts
✅ Files skipped from review due to trivial changes (2)
- packages/duckdb/README.md
- packages/duckdb-core/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/mosaic/src/charts/chart-types/base-types.ts
- packages/ai/jest.config.js
- packages/ai/tests/tableTools.test.ts
- packages/ai/package.json
- packages/mosaic/tests/dashboard-ai.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a31db85e65
ℹ️ 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".
| adapter.setSelectedTable(state, artifactId, explicitTableName); | ||
| return {tableName: explicitTableName, columns}; | ||
| const resolvedTable = getResolvedTable(table); | ||
| adapter.setSelectedTable(state, artifactId, resolvedTable.tableName); |
There was a problem hiding this comment.
When a dashboard tool resolves an explicit table, this now stores the fully quoted tableId in dashboard.selectedTable. Deck map datasets that omit dataset.source fall back to that selected table (packages/deck/src/dashboardConfig.ts:99-106), and map auto-fit still builds bounds SQL by doing source.tableName.split('.').map(escapeId) (packages/deck/src/dashboard.tsx:192-196), so a selected table like "local"."main"."events" becomes """local"""."""main"""."""events""" and fit-to-data queries fail for those AI-created maps. Update the deck map table-reference handling before storing/forwarding quoted ids here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acefdbb9c3
ℹ️ 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 bareMatches = tables.filter( | ||
| (candidate) => candidate.table.table === trimmedTableReference, | ||
| ); |
There was a problem hiding this comment.
When the caller passes a quoted bare SQL identifier such as "events.2026" for a unique table whose name contains a dot, parseQualifiedSqlIdentifier correctly strips it to events.2026, but this branch only uses parsed identifiers when a schema/database is present and then compares against the original quoted string. That makes resolveTableReference return not found for quoted bare names, which breaks read_table_schema/dashboard resolution unless the caller supplies the fully qualified tableId.
Useful? React with 👍 / 👎.
Reworks AI table schema context so large database catalogs stay usable without overloading the model prompt, while preserving reliable table identity across AI tools, dashboard agents, and chart rendering.
This PR keeps the existing
list_tables/read_table_schematool surface frommain, removes the paralleldescribe_table_schemaimplementation, and updates the default AI instructions to guide agents toward the existing table discovery workflow.Core table context improvements
tableIdvalues to schema summaries and full-schema headers.Incorporated from PR feat: Allow maxChars parameter in formatTablesForLLM function #723
maxCharssupport for schema context.currentDatabaseedge case to avoid over-filtering tables.Tooling / agent behavior
read_table_schema.list_tablesand schema summaries expose usable, fully quotedtableIdstrings derived fromQualifiedTableName.toString().read_table_schemaaccepts unique bare table names for compatibility, but reports ambiguity instead of silently choosing when names collide.scopeguidance forlist_tables; prompts now reference only supported filters.Canonical table identity
QualifiedTableNametype as the canonical structured table identity.tableNameas user input / display / legacy compatibility."db"."schema"."table".Dashboard and chart behavior
mainschema.CI / maintenance
ts-jestpackage dependency for@sqlrooms/aitests.Summary by CodeRabbit