Conversation
Signed-off-by: Ilya Boyandin <ilyabo@gmail.com>
📝 WalkthroughWalkthroughThis pull request refactors DuckDbSlice's table schema loading with deduplication and enhanced metadata handling, deprecates S3 integration in KeplerAddDataDialog, adds a config guard to RoomShellSlice, and relocates an import in ColumnTreeNode. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/duckdb/src/DuckDbSlice.ts`:
- Around line 618-665: refreshTableSchemas currently returns the stale
get().db.tables when refreshPromise exists and also does not await the newly
created IIFE, so callers get pre-refresh data; change the dedup pattern so that
when refreshPromise is already set the function returns that promise, and when
you create refreshPromise = (async () => { ... })() you should return/await that
promise instead of immediately returning get().db.tables; update the function
refreshTableSchemas and its use of refreshPromise so all callers receive the
same Promise<DataTable[]> (resolve value should be the newTables returned from
get().db.loadTableSchemas) while preserving the existing error handling and
final cleanup logic.
In `@packages/room-shell/src/RoomShellSlice.ts`:
- Around line 503-506: Remove the debug console.log in updateReadyDataSources
that prints the full config object; locate the function updateReadyDataSources
(and the store subscription that calls it) and delete the
console.log('updateReadyDataSources', {config}) line so the function simply
guards on if (!config) return and proceeds without logging sensitive or noisy
config details.
🧹 Nitpick comments (4)
packages/kepler/src/components/KeplerAddDataDialog.tsx (1)
68-86: Consider removing the commented-out S3 code and the unusedS3enum member.
AddDataMethods.S3(line 71) is now dead code since the S3 option is commented out everywhere. If S3 support is being deprecated, removing the commented blocks (lines 19, 82–85, 94, 100–105, 117–123, 188–200) and the unused enum member in favor of version control history would reduce noise. If the intent is to re-enable it soon, a tracking issue or TODO comment would make that clearer.packages/duckdb/src/DuckDbSlice.ts (3)
486-535: Variable shadowing and N+1 query pattern in the inner loop.
Shadowing:
database(line 488),schema(line 489), andtable(line 490) shadow the identically named destructured variables from the outerfilterparameter (line 438). This is technically correct (block-scoped) but can mislead readers. Consider renaming to e.g.rowDatabase,rowSchema,rowTable— orentryDatabase, etc.N+1 queries: Each table/view row issues a separate
PRAGMA table_info(...)call (line 503). For schemas with many tables, this will be noticeably slow. DuckDB'sinformation_schema.columnsorduckdb_columns()can return all column metadata in a single query, which could then be joined in-memory.
254-254:refreshPromiseis a module-level mutable variable shared across all slice instances.
refreshPromiseis declared outside the returned slice creator (line 254), which means it lives at module scope. IfcreateDuckDbSliceis ever called more than once (e.g., multiple rooms or tests), all instances share the same deduplication lock. Moving it inside the closure returned bycreateDuckDbSlicewould scope it per instance.Proposed fix — move inside the closure
export function createDuckDbSlice({ connector = createWasmDuckDbConnector(), }: CreateDuckDbSliceProps = {}): StateCreator<DuckDbSliceState> { - let refreshPromise: Promise<DataTable[]> | null = null; return createSlice<DuckDbSliceState, BaseRoomStoreState & DuckDbSliceState>( (set, get) => { + let refreshPromise: Promise<DataTable[]> | null = null; return { db: {
443-480: Duplicated SQL filter logic between table and view queries.The WHERE clause construction is copy-pasted between
tableSql(lines 451–461) andviewSql(lines 470–480). Consider extracting a helper likebuildWhereClause(defaultDatabaseFilter, schema, database, table)to reduce duplication and the risk of the two diverging.
| async refreshTableSchemas(): Promise<DataTable[]> { | ||
| if (refreshPromise) { | ||
| return get().db.tables; | ||
| } | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.isRefreshingTableSchemas = true; | ||
| }), | ||
| ); | ||
| try { | ||
| const connector = await get().db.getConnector(); | ||
| const result = await connector.query( | ||
| `SELECT current_schema() AS schema, current_database() AS database`, | ||
| ); | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.currentSchema = result.getChild('schema')?.get(0); | ||
| draft.db.currentDatabase = result | ||
| .getChild('database') | ||
| ?.get(0); | ||
| }), | ||
| ); | ||
| const newTables = await get().db.loadTableSchemas(); | ||
| // Only update if there's an actual change in the schemas | ||
| if (!deepEquals(newTables, get().db.tables)) { | ||
| refreshPromise = (async () => { | ||
| try { | ||
| const connector = await get().db.getConnector(); | ||
| const result = await connector.query( | ||
| `SELECT current_schema() AS schema, current_database() AS database`, | ||
| ); | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.tables = newTables; | ||
| draft.db.schemaTrees = createDbSchemaTrees(newTables); | ||
| draft.db.currentSchema = result.getChild('schema')?.get(0); | ||
| draft.db.currentDatabase = result | ||
| .getChild('database') | ||
| ?.get(0); | ||
| }), | ||
| ); | ||
| const newTables = await get().db.loadTableSchemas(); | ||
| // Only update if there's an actual change in the schemas | ||
| if (!deepEquals(newTables, get().db.tables)) { | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.tables = newTables; | ||
| draft.db.schemaTrees = createDbSchemaTrees(newTables); | ||
| }), | ||
| ); | ||
| } | ||
| return newTables; | ||
| } catch (err) { | ||
| get().room.captureException(err); | ||
| return []; | ||
| } finally { | ||
| refreshPromise = null; | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.isRefreshingTableSchemas = false; | ||
| }), | ||
| ); | ||
| } | ||
| return newTables; | ||
| } catch (err) { | ||
| get().room.captureException(err); | ||
| return []; | ||
| } finally { | ||
| set((state) => | ||
| produce(state, (draft) => { | ||
| draft.db.isRefreshingTableSchemas = false; | ||
| }), | ||
| ); | ||
| } | ||
| })(); | ||
| return get().db.tables; | ||
| }, |
There was a problem hiding this comment.
refreshTableSchemas deduplication silently breaks the awaiting contract — callers receive stale tables.
When refreshPromise is already in-flight, the function returns get().db.tables (line 620) — the current stale snapshot. And even for the first caller, the IIFE is assigned to refreshPromise but not awaited (line 664 returns get().db.tables immediately, before the refresh completes).
This means every await get().db.refreshTableSchemas() in RoomShellSlice (lines 369, 409, 447, 658, 699) will resolve with pre-refresh data. For example, addRoomFile (line 409) does:
await get().db.refreshTableSchemas();
// ...
return get().db.findTableByName(tableName); // may return undefinedThe standard deduplication pattern returns the same promise to all callers:
Proposed fix
async refreshTableSchemas(): Promise<DataTable[]> {
if (refreshPromise) {
- return get().db.tables;
+ return refreshPromise;
}
set((state) =>
produce(state, (draft) => {
draft.db.isRefreshingTableSchemas = true;
}),
);
refreshPromise = (async () => {
try {
const connector = await get().db.getConnector();
const result = await connector.query(
`SELECT current_schema() AS schema, current_database() AS database`,
);
set((state) =>
produce(state, (draft) => {
draft.db.currentSchema = result.getChild('schema')?.get(0);
draft.db.currentDatabase = result
.getChild('database')
?.get(0);
}),
);
const newTables = await get().db.loadTableSchemas();
// Only update if there's an actual change in the schemas
if (!deepEquals(newTables, get().db.tables)) {
set((state) =>
produce(state, (draft) => {
draft.db.tables = newTables;
draft.db.schemaTrees = createDbSchemaTrees(newTables);
}),
);
}
return newTables;
} catch (err) {
get().room.captureException(err);
return [];
} finally {
refreshPromise = null;
set((state) =>
produce(state, (draft) => {
draft.db.isRefreshingTableSchemas = false;
}),
);
}
})();
- return get().db.tables;
+ return refreshPromise;
},🤖 Prompt for AI Agents
In `@packages/duckdb/src/DuckDbSlice.ts` around lines 618 - 665,
refreshTableSchemas currently returns the stale get().db.tables when
refreshPromise exists and also does not await the newly created IIFE, so callers
get pre-refresh data; change the dedup pattern so that when refreshPromise is
already set the function returns that promise, and when you create
refreshPromise = (async () => { ... })() you should return/await that promise
instead of immediately returning get().db.tables; update the function
refreshTableSchemas and its use of refreshPromise so all callers receive the
same Promise<DataTable[]> (resolve value should be the newTables returned from
get().db.loadTableSchemas) while preserving the existing error handling and
final cleanup logic.
| console.log('updateReadyDataSources', {config}); | ||
| if (!config) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Remove debug console.log before merging.
This looks like a leftover debug statement. It logs the full config object on every updateReadyDataSources call (including from the store subscription at line 491), which will be noisy in production and may expose config details in the console.
Proposed fix
const {config} = get().room;
- console.log('updateReadyDataSources', {config});
if (!config) {
return;
}📝 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.
| console.log('updateReadyDataSources', {config}); | |
| if (!config) { | |
| return; | |
| } | |
| if (!config) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@packages/room-shell/src/RoomShellSlice.ts` around lines 503 - 506, Remove the
debug console.log in updateReadyDataSources that prints the full config object;
locate the function updateReadyDataSources (and the store subscription that
calls it) and delete the console.log('updateReadyDataSources', {config}) line so
the function simply guards on if (!config) return and proceeds without logging
sensitive or noisy config details.
Summary by CodeRabbit
Changes
Improvements