Repository navigation
feat: implement internal resource filtering in createDbSchemaTrees - #477
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:
📝 WalkthroughWalkthroughThis PR adds table schema loading filters to exclude Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (1)
packages/duckdb-core/src/schema-tree/__tests__/schemaTree.test.ts (1)
8-315: Consider a small fixture builder to reduce test duplication.A local
makeTable(...)helper would make this suite easier to extend and maintain.Refactor sketch
+const makeTable = ({ + database, + schema, + tableName, + columns = [{name: 'id', type: 'INTEGER'}], +}: { + database?: string; + schema: string; + tableName: string; + columns?: {name: string; type: string}[]; +}): DataTable => ({ + table: { + database, + schema, + table: tableName, + toString: () => [database, schema, tableName].filter(Boolean).join('.'), + }, + isView: false, + database, + schema, + tableName, + columns, +});-{ - table: { - database: 'main', - schema: 'public', - table: 'users', - toString: () => 'main.public.users', - }, - isView: false, - database: 'main', - schema: 'public', - tableName: 'users', - columns: [{name: 'id', type: 'INTEGER'}, {name: 'name', type: 'VARCHAR'}], -}, +makeTable({ + database: 'main', + schema: 'public', + tableName: 'users', + columns: [{name: 'id', type: 'INTEGER'}, {name: 'name', type: 'VARCHAR'}], +}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/duckdb-core/src/schema-tree/__tests__/schemaTree.test.ts` around lines 8 - 315, Tests repeat identical DataTable fixtures; introduce a local helper (e.g., makeTable) in schemaTree.test.ts to build DataTable objects to reduce duplication. Add a function named makeTable(database?, schema, tableName, columns?, isView?) that returns the same shape used in the tests (including the nested table object with toString), then replace the repeated literal objects in each it(...) case with calls to makeTable('__sqlrooms_ephemeral','main','temp', [{name:'id',type:'INTEGER'}]) etc.; update all tests that construct tables (including cases for default DB where database is omitted) to use makeTable so fixtures are concise and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/duckdb-core/src/schema-tree/__tests__/schemaTree.test.ts`:
- Around line 8-315: Tests repeat identical DataTable fixtures; introduce a
local helper (e.g., makeTable) in schemaTree.test.ts to build DataTable objects
to reduce duplication. Add a function named makeTable(database?, schema,
tableName, columns?, isView?) that returns the same shape used in the tests
(including the nested table object with toString), then replace the repeated
literal objects in each it(...) case with calls to
makeTable('__sqlrooms_ephemeral','main','temp', [{name:'id',type:'INTEGER'}])
etc.; update all tests that construct tables (including cases for default DB
where database is omitted) to use makeTable so fixtures are concise and
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efbc3599-0305-49be-b0b0-7d5a9cdb54ea
📒 Files selected for processing (2)
packages/duckdb-core/src/schema-tree/__tests__/schemaTree.test.tspackages/duckdb-core/src/schema-tree/schemaTree.ts
There was a problem hiding this comment.
I think we should not filter just in schemaTree, but implement it on the level of DuckDbSlice loadTableSchemas. It already accepts a filter prop. What I would suggest to do is adding an optional loadTableSchemasFilter?: (table: QualifiedTableName) => boolean prop to CreateDuckDbSliceProps and applying it in loadTableSchemas. Then we can pass the __sqlrooms filter in the CLI app.
Or maybe the default can filter all __sqlrooms* schemas and databases out.
Agreed, I would filter all |
@copilot can you implement this suggestion |
|
@dmitriy-kostianetskiy I've opened a new pull request, #481, to work on those changes. Once the pull request is ready, I'll request review from you. |
| @@ -487,6 +488,17 @@ export function createDuckDbSlice({ | |||
| filter?: SchemaAndDatabase & {table?: string}, | |||
There was a problem hiding this comment.
looks like Copilot didn't follow the instructions carefully
What I would suggest to do is adding an optional loadTableSchemasFilter?: (table: QualifiedTableName) => boolean prop to CreateDuckDbSliceProps and applying it in loadTableSchemas. Then we can pass the __sqlrooms filter in the CLI app.
Or maybe the default can filter all __sqlrooms* schemas and databases out.
There was a problem hiding this comment.
I guess the idea was to filter out all __sqlrooms* schemas by default, which this change effectively does at the sql level.
do you want me to pass it via filter parameter? If yes I have to two points to clarify:
- Current implementation is very effective sql-level include filter. We can add a function like you described, however it won't be part of SQL query anymore, but predicate for javascript
.filter. If you think this is fine, I can refactor it this way. - More conceptually, why would we need to that in the first place? What are the use cases for that sort of flexible filtering?
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Dmitriy Kostianetskiy <[email protected]>
…se it in DuckDbSlice
264b44b to
f8d88b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/duckdb/src/DuckDbSlice.ts (1)
32-44:⚠️ Potential issue | 🟠 MajorWiden the default hidden-resource prefix to
__sqlrooms*.The review thread on this PR asked for all
__sqlrooms*schemas/databases to be hidden by default.__sqlrooms_only catches the underscored form, so__sqlroomsand__sqlroomsFoostill slip through. Please broaden the prefix check and align the public docstring with that rule.Suggested patch
-const INTERNAL_SQLROOMS_PREFIX = '__sqlrooms_'; +const INTERNAL_SQLROOMS_PREFIX = '__sqlrooms'; /** - * Default filter to exclude internal SQLRooms tables, schemas, and databases + * Default filter to exclude internal SQLRooms schemas and databases */ export const createDefaultLoadTableSchemasFilter = ( table: QualifiedTableName, ): boolean => { @@ /** * Optional filter function to control which tables are included when loading schemas. - * By default, filters out tables/schemas/databases starting with '__sqlrooms_'. + * By default, filters out schemas/databases starting with '__sqlrooms'. * Pass `null` to disable filtering and show all tables.Also applies to: 306-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/duckdb/src/DuckDbSlice.ts` around lines 32 - 44, The default hidden-resource prefix constant INTERNAL_SQLROOMS_PREFIX and the filter createDefaultLoadTableSchemasFilter should be broadened from '__sqlrooms_' to '__sqlrooms' and the docstring updated accordingly; change INTERNAL_SQLROOMS_PREFIX = '__sqlrooms' and ensure createDefaultLoadTableSchemasFilter still uses startsWith on table.database and table.schema (QualifiedTableName) so any name beginning with '__sqlrooms' (e.g. '__sqlrooms', '__sqlroomsFoo', '__sqlrooms_bar') is excluded by default, and update the comment above the function to state it hides all '__sqlrooms*' schemas/databases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/duckdb/src/DuckDbSlice.ts`:
- Around line 600-606: loadTableSchemas' visibility filter
(loadTableSchemasFilter) can hide exact lookups and make addTable and
checkTableExists fail after a successful write; update loadTableSchemas to
accept an optional bypass flag (e.g., bypassFilter: boolean = false) or a
separate internal path (e.g., loadTableSchemaByName) that ignores
loadTableSchemasFilter when performing exact/qualified lookups, and then call
that bypass from addTable and checkTableExists (or pass bypassFilter=true) so
dataTable.table exact matches are not filtered out; ensure default behavior
still applies for normal listing calls.
---
Duplicate comments:
In `@packages/duckdb/src/DuckDbSlice.ts`:
- Around line 32-44: The default hidden-resource prefix constant
INTERNAL_SQLROOMS_PREFIX and the filter createDefaultLoadTableSchemasFilter
should be broadened from '__sqlrooms_' to '__sqlrooms' and the docstring updated
accordingly; change INTERNAL_SQLROOMS_PREFIX = '__sqlrooms' and ensure
createDefaultLoadTableSchemasFilter still uses startsWith on table.database and
table.schema (QualifiedTableName) so any name beginning with '__sqlrooms' (e.g.
'__sqlrooms', '__sqlroomsFoo', '__sqlrooms_bar') is excluded by default, and
update the comment above the function to state it hides all '__sqlrooms*'
schemas/databases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aede73f-32e2-4990-bc9d-edb7408b06d3
📒 Files selected for processing (4)
packages/cells/src/cellsSlice.tspackages/duckdb-core/src/schema-tree/__tests__/schemaTree.test.tspackages/duckdb/src/DuckDbSlice.tspackages/duckdb/src/index.ts
💤 Files with no reviewable changes (1)
- packages/cells/src/cellsSlice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/duckdb-core/src/schema-tree/tests/schemaTree.test.ts
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Issue: #437
This pull request introduces comprehensive filtering logic to hide internal SQLRooms resources from the database schema tree and adds thorough tests to ensure this functionality. The main focus is to ensure that any databases or schemas prefixed with
__sqlrooms_are excluded from the UI, while still allowing user-created resources with other__prefixes. The changes also include a basic test for tree structure creation.Filtering internal resources:
isInternalSqlRoomsResourcefunction inschemaTree.tsto identify internal SQLRooms resources by their name prefix (__sqlrooms_).createDbSchemaTreesinschemaTree.tsto skip any databases or schemas identified as internal SQLRooms resources, ensuring they are not included in the schema tree.Testing improvements:
schemaTree.test.tsto verify filtering of internal SQLRooms databases and schemas, correct handling of user databases with__prefix, mixed resource scenarios, and default database behavior.schemaTree.test.tsto confirm that the schema tree is correctly constructed for standard cases.Summary by CodeRabbit
New Features
Tests
Chores