Repository navigation
fix: Standardize table name qualification across SQLRooms - #734
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesQualifiedTableName Canonicalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
…le-name-qualification
ilyabo
changed the title
![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: 2226fec544
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/ai/__tests__/tableSchemaContext.test.ts (1)
8-31: ⚡ Quick winExercise default-database canonicalization in this fixture path.
Because
makeQualifiedTableNameis created withoutdefaultDatabase,.toString()here never validates the “omit default database” branch. On Line 89, the expectation then locks in a fully-qualified local ID and misses the new canonical behavior coverage.Proposed test-fixture update
function makeTable( tableName: string, options: { database?: string; schema?: string; + defaultDatabase?: string; columns?: {name: string; type?: string}[]; rowCount?: number; comment?: string; } = {}, ): DataTable { return { tableName, table: makeQualifiedTableName({ database: options.database, schema: options.schema ?? 'main', table: tableName, + defaultDatabase: options.defaultDatabase, }), isView: false, @@ - expect(output).toContain('tableId: "local"."main"."events"'); + expect(output).toContain('tableId: "main"."events"');As per coding guidelines
**/*.test.{ts,tsx,js,jsx}: Add or update tests when changing behavior or public API, when practical.Also applies to: 89-89
🤖 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/__tests__/tableSchemaContext.test.ts` around lines 8 - 31, The makeTable fixture function does not pass a defaultDatabase parameter when calling makeQualifiedTableName, which means the test never exercises the code path that omits the default database from the canonical representation. Update the makeTable function to include a defaultDatabase option (defaulted appropriately, such as to options.database or a test default value) and pass it to the makeQualifiedTableName call. Additionally, update the expectation at line 89 to verify the canonical behavior where the database name is properly omitted when it matches the default database, ensuring full coverage of the canonicalization logic.Source: Coding guidelines
packages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.ts (1)
42-51: ⚡ Quick winDocument exported BoxPlot query/client APIs with TSDoc.
BoxPlotClientOptions,buildBoxPlotQuery, andBoxPlotClientwere updated as public exports; please add concise TSDoc so consumers understand theQualifiedTableNamerequirements and SQL-reference behavior.As per coding guidelines,
packages/**/*.{ts,tsx}: Add tsdoc to exported types and functions.Also applies to: 132-213, 215-381
🤖 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/chart-types/box-plot/renderer/BoxPlotClient.ts` around lines 42 - 51, Add comprehensive TSDoc documentation to the three public exports in BoxPlotClient.ts: the BoxPlotClientOptions type, the buildBoxPlotQuery function, and the BoxPlotClient class. For BoxPlotClientOptions, document each property and explain the QualifiedTableName requirements. For buildBoxPlotQuery and BoxPlotClient, provide clear descriptions of their purpose, parameters, return values, and document the SQL-reference behavior as applicable. Follow the existing TSDoc conventions in the codebase to ensure consistency.Source: Coding guidelines
packages/mosaic/src/charts/chart-types/base-types.ts (1)
153-170: ⚡ Quick winAdd TSDoc to the exported
ChartRendererPropscontract.This interface’s public API changed (
tablenow carries canonical qualified-name semantics), so a short TSDoc block should document expectations for downstream renderers.As per coding guidelines,
packages/**/*.{ts,tsx}: Add tsdoc to exported types and functions.🤖 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/chart-types/base-types.ts` around lines 153 - 170, The exported interface ChartRendererProps lacks a TSDoc documentation block above its declaration. Add a TSDoc comment block before the interface definition that documents the purpose and contract of this interface for downstream renderers. Include a note that the table property now carries canonical qualified-name semantics as part of the public API contract. Follow the coding guideline requiring TSDoc documentation for all exported types in the packages directory.Source: Coding guidelines
packages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts (1)
13-21: ⚡ Quick winAdd TSDoc for the exported
useBoxPlotClienthook.Since the hook’s public input changed to
QualifiedTableName, a brief TSDoc contract here will prevent misuse and make integration expectations explicit.As per coding guidelines,
packages/**/*.{ts,tsx}: Add tsdoc to exported types and functions.🤖 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/chart-types/box-plot/renderer/useBoxPlotClient.ts` around lines 13 - 21, The exported function useBoxPlotClient is missing TSDoc documentation. Add a TSDoc comment block above the function definition that describes its purpose as a hook and documents the input parameters object, with particular attention to explaining the table parameter which now uses the QualifiedTableName type to make the contract explicit and prevent misuse during integration.Source: Coding guidelines
packages/deck/src/dashboard.tsx (1)
192-195: ⚡ Quick winAdd TSDoc for the exported
createDeckMapBoundsQueryAPI.This export is part of
packages/and should carry TSDoc for contract clarity.📝 Suggested doc block
+/** + * Builds the SQL used to compute map bounds from either a table source + * or an arbitrary SQL source query. + */ export function createDeckMapBoundsQuery(options: { source: DeckMapBoundsQuerySource; fitToData: DeckMapDashboardFitToDataConfig; }) {As per coding guidelines,
packages/**/*.{ts,tsx}: Add tsdoc to exported types and functions.🤖 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 192 - 195, The exported function `createDeckMapBoundsQuery` in the dashboard module is missing TSDoc documentation. Add a TSDoc comment block above the function definition that documents the purpose of the function, describes the options parameter object and its properties (source and fitToData), and documents the return type. This will provide clear API contract documentation for consumers of this exported function from the packages directory.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 `@packages/deck/src/dashboard.tsx`:
- Around line 502-516: In the fitToDataBoundsSource useMemo hook, the current
logic returns undefined when findTable(fitToDataSource.tableName) fails to
locate the table in the cache, which causes auto-fit to be skipped. Modify the
logic at the end of the useMemo where table is resolved to add a fallback path:
instead of immediately returning undefined when the table is not found, create a
qualified table reference directly from the fitToDataSource.tableName string
(the configured name itself) and use that as a fallback return value, ensuring
that bounds queries can still execute even when the schema-cache lookup misses.
In `@packages/duckdb/src/DuckDbSlice.ts`:
- Around line 792-794: The guard condition on line 792 in the
findUniqueMatchingTable fallback block is too permissive - it triggers for any
`database !== currentDatabase` check, even when the caller intentionally
specifies a different database, which can return the wrong table and cause
incorrect operations. Add an additional guard condition to only trigger this
fallback when dealing with a stale database reference scenario (not an
intentional explicit database reference). The condition should verify that the
database parameter was not explicitly provided by the caller before falling back
to findUniqueMatchingTable.
---
Nitpick comments:
In `@packages/ai/__tests__/tableSchemaContext.test.ts`:
- Around line 8-31: The makeTable fixture function does not pass a
defaultDatabase parameter when calling makeQualifiedTableName, which means the
test never exercises the code path that omits the default database from the
canonical representation. Update the makeTable function to include a
defaultDatabase option (defaulted appropriately, such as to options.database or
a test default value) and pass it to the makeQualifiedTableName call.
Additionally, update the expectation at line 89 to verify the canonical behavior
where the database name is properly omitted when it matches the default
database, ensuring full coverage of the canonicalization logic.
In `@packages/deck/src/dashboard.tsx`:
- Around line 192-195: The exported function `createDeckMapBoundsQuery` in the
dashboard module is missing TSDoc documentation. Add a TSDoc comment block above
the function definition that documents the purpose of the function, describes
the options parameter object and its properties (source and fitToData), and
documents the return type. This will provide clear API contract documentation
for consumers of this exported function from the packages directory.
In `@packages/mosaic/src/charts/chart-types/base-types.ts`:
- Around line 153-170: The exported interface ChartRendererProps lacks a TSDoc
documentation block above its declaration. Add a TSDoc comment block before the
interface definition that documents the purpose and contract of this interface
for downstream renderers. Include a note that the table property now carries
canonical qualified-name semantics as part of the public API contract. Follow
the coding guideline requiring TSDoc documentation for all exported types in the
packages directory.
In `@packages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.ts`:
- Around line 42-51: Add comprehensive TSDoc documentation to the three public
exports in BoxPlotClient.ts: the BoxPlotClientOptions type, the
buildBoxPlotQuery function, and the BoxPlotClient class. For
BoxPlotClientOptions, document each property and explain the QualifiedTableName
requirements. For buildBoxPlotQuery and BoxPlotClient, provide clear
descriptions of their purpose, parameters, return values, and document the
SQL-reference behavior as applicable. Follow the existing TSDoc conventions in
the codebase to ensure consistency.
In
`@packages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts`:
- Around line 13-21: The exported function useBoxPlotClient is missing TSDoc
documentation. Add a TSDoc comment block above the function definition that
describes its purpose as a hook and documents the input parameters object, with
particular attention to explaining the table parameter which now uses the
QualifiedTableName type to make the contract explicit and prevent misuse during
integration.
🪄 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: 836412cb-fea6-4f9a-acb0-0a3c5d1e5eea
📒 Files selected for processing (32)
examples/code-editor/src/SqlEditor.tsxpackages/ai/__tests__/tableSchemaContext.test.tspackages/ai/src/index.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/cells/src/defaultCellRegistry.tsxpackages/cells/src/execution.tspackages/db/src/DbSlice.tspackages/deck/__tests__/dashboard-ai-map.test.tspackages/deck/src/dashboard.tsxpackages/duckdb-core/README.mdpackages/duckdb-core/__tests__/getUnqualifiedSqlIdentifier.test.tspackages/duckdb-core/__tests__/schemaTreeUtils.test.tspackages/duckdb-core/src/duckdb-utils.tspackages/duckdb-core/src/schema-tree/schemaTreeUtils.tspackages/duckdb/README.mdpackages/duckdb/__tests__/DuckDbSlice.test.tspackages/duckdb/src/DuckDbSlice.tspackages/duckdb/src/loadTableSchemas.tspackages/mosaic/__tests__/BoxPlotClient.test.tspackages/mosaic/src/charts/MosaicChartView.tsxpackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotPanelRenderer.tsxpackages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.tspackages/pivot/__tests__/pivot-sql.test.tspackages/schema-tree/src/nodes/TableTreeNode.tsxpackages/sql-editor/src/components/CreateTableModal.tsxpackages/sql-editor/src/utils/qualified-name-parser.ts
💤 Files with no reviewable changes (1)
- packages/ai/src/index.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2c650c55d
ℹ️ 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".
Comment on lines
+527
to
+530
| const table = findTable(fitToDataSource.tableName); | ||
| return { | ||
| table: table?.table ?? qualifyTableName(fitToDataSource.tableName), | ||
| }; |
There was a problem hiding this comment.
When a map with fitToData renders before the schema cache contains fitToDataSource.tableName, this fallback passes the persisted dashboard string into qualifyTableName. That helper treats string inputs as literal table names, so a canonical id like "main"."events" becomes a relation literally named "main"."events" in the current schema instead of main.events; because this component only subscribes to the action functions, a later schema refresh does not necessarily rebuild the source, and the bounds query can fail while marking fit as handled. Parse the table reference or subscribe to the resolved table instead of qualifying the raw string.
Useful? React with 👍 / 👎.
Contributor
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/duckdb/__tests__/DuckDbSlice.test.ts`:
- Around line 350-353: The test for the dropTable negative path is currently
swallowing the rejection error with .catch(() => undefined), which allows the
test to pass even if the database-miss guard stops throwing correctly. Remove
the .catch() handler and instead assert the rejection directly using
expect().rejects to validate that the dropTable call on the non-existent table
actually throws the expected error. This ensures the test properly validates
that the guard is triggered for the intended reason.
🪄 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: d9bce405-a8d9-41fa-b6e1-00475a323ddf
📒 Files selected for processing (8)
packages/ai/__tests__/tableSchemaContext.test.tspackages/deck/src/dashboard.tsxpackages/duckdb-core/src/duckdb-utils.tspackages/duckdb/__tests__/DuckDbSlice.test.tspackages/duckdb/src/DuckDbSlice.tspackages/mosaic/src/charts/chart-types/base-types.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.tspackages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/mosaic/src/charts/chart-types/box-plot/renderer/useBoxPlotClient.ts
- packages/mosaic/src/charts/chart-types/base-types.ts
- packages/deck/src/dashboard.tsx
- packages/duckdb-core/src/duckdb-utils.ts
- packages/mosaic/src/charts/chart-types/box-plot/renderer/BoxPlotClient.ts
- packages/duckdb/src/DuckDbSlice.ts
Comment on lines
+350
to
+353
| await store | ||
| .getState() | ||
| .db.dropTable('"remote"."main"."drop_database_miss"') | ||
| .catch(() => undefined); |
Contributor
There was a problem hiding this comment.
Assert the rejection instead of swallowing it in this negative-path test.
Catching and discarding the dropTable error makes this test pass even if the explicit database-miss guard stops throwing for the intended reason. Assert rejection directly so this test validates both “no drop” and “guard triggered”.
Suggested test adjustment
- await store
- .getState()
- .db.dropTable('"remote"."main"."drop_database_miss"')
- .catch(() => undefined);
+ await expect(
+ store.getState().db.dropTable('"remote"."main"."drop_database_miss"'),
+ ).rejects.toThrow();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await store | |
| .getState() | |
| .db.dropTable('"remote"."main"."drop_database_miss"') | |
| .catch(() => undefined); | |
| await expect( | |
| store.getState().db.dropTable('"remote"."main"."drop_database_miss"'), | |
| ).rejects.toThrow(); |
🤖 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/duckdb/__tests__/DuckDbSlice.test.ts` around lines 350 - 353, The
test for the dropTable negative path is currently swallowing the rejection error
with .catch(() => undefined), which allows the test to pass even if the
database-miss guard stops throwing correctly. Remove the .catch() handler and
instead assert the rejection directly using expect().rejects to validate that
the dropTable call on the non-existent table actually throws the expected error.
This ensures the test properly validates that the guard is triggered for the
intended reason.
This was referenced Jun 30, 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
Testing
Summary by CodeRabbit
tableIdformat (including when the default database may be omitted).