Repository navigation
feat: refreshTableSchemas improvements - #346
Conversation
Signed-off-by: Ilya Boyandin <[email protected]>
|
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:
📝 WalkthroughWalkthroughAdds a new dropRelation API and deduplicates concurrent DuckDB schema refreshes (per-table metadata queries); removes S3 UI/props from Kepler add-data dialog; adds guards and small flow changes in room-shell and kepler syncing; several UI/layout and package.json tweaks across the repo. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![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
🤖 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.
…hTableSchemas-improvements Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 673-719: The current dedup returns an in-flight refreshPromise
even when DB mutations occur after it starts, causing callers to see stale
schemas; fix by tracking a mutation nonce (e.g., db.lastSchemaMutationNonce)
that mutation paths (addRoomFile/removeDataSource/etc.) increment, and change
refreshTableSchemas to capture a startNonce when it begins; if refreshPromise
exists, return a promise that awaits the in-flight promise and then checks if
get().db.lastSchemaMutationNonce !== startNonce—if different, queue/perform one
follow-up refresh (call the same internal refresh logic that uses
get().db.loadTableSchemas) and return that new promise; ensure you still
set/clear draft.db.isRefreshingTableSchemas and only update draft.db.tables and
draft.db.schemaTrees when the returned newTables differ (deepEquals) as before,
and keep refreshPromise single-active-with-at-most-one-queued-followup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06240f27-671a-40bc-9652-b102b7234d49
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/duckdb-node/package.jsonpackages/duckdb/src/DuckDbSlice.ts
Signed-off-by: Ilya Boyandin <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/duckdb/src/DuckDbSlice.ts (1)
491-534: Parallel query execution opportunity.The table and view queries at lines 533-534 are executed sequentially. Since they're independent, they could be parallelized for better performance.
🔧 Suggested parallel execution
- const tableResults = await connector.query(tableSql); - const viewResults = await connector.query(viewSql); + const [tableResults, viewResults] = await Promise.all([ + connector.query(tableSql), + connector.query(viewSql), + ]);🤖 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 491 - 534, The two independent queries built into tableSql and viewSql are executed sequentially via connector.query and can be parallelized: replace the separate awaits that assign tableResults and viewResults with a single concurrent execution using Promise.all (call connector.query(tableSql) and connector.query(viewSql) together and destructure the resulting array into tableResults and viewResults), preserving existing variable names and existing error propagation/handling.
🤖 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/src/DuckDbSlice.ts`:
- Around line 491-534: The two independent queries built into tableSql and
viewSql are executed sequentially via connector.query and can be parallelized:
replace the separate awaits that assign tableResults and viewResults with a
single concurrent execution using Promise.all (call connector.query(tableSql)
and connector.query(viewSql) together and destructure the resulting array into
tableResults and viewResults), preserving existing variable names and existing
error propagation/handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4b75a3b-4cb4-4790-84d4-f17e41c69390
📒 Files selected for processing (1)
packages/duckdb/src/DuckDbSlice.ts
Signed-off-by: Ilya Boyandin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/duckdb/src/DuckDbSlice.ts (1)
743-789:⚠️ Potential issue | 🔴 CriticalStill need a queued follow-up refresh for mid-flight schema mutations.
Returning
refreshPromisehere fixed the immediate stale return, but this single-flight gate still collapses refresh requests that arrive after a relation was created or dropped while the current refresh was already running.packages/room-shell/src/RoomShellSlice.ts, Lines 491-503 and 528-529 both awaitrefreshTableSchemas()after mutating the catalog; if the initialdb.initialize()refresh is still in flight, those callers can still resolve against pre-mutation metadata and then read stale or missing tables. Please queue one follow-up refresh, or track a schema-mutation nonce, whenever a schema-changing operation lands during an active run.🤖 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 743 - 789, refreshTableSchemas currently single-flights concurrent callers via refreshPromise, but schema-mutating callers (e.g., in RoomShellSlice) can trigger changes while a refresh is in flight and then still read stale metadata; add a queued follow-up mechanism: introduce a flag or nonce (e.g., pendingSchemaRefresh or schemaMutationNonce) that schema-mutating paths set when they change the catalog, expose a helper like markSchemaDirty() that sets this flag/increments the nonce, and update refreshTableSchemas (the refreshPromise flow and its finally block) to, after the current run completes, check the flag/nonce and if set clear it and invoke refreshTableSchemas again to perform one guaranteed follow-up (use the nonce to avoid loops if new mutations arrive during the follow-up); keep existing behaviors around setting draft.db.isRefreshingTableSchemas, loading via get().db.loadTableSchemas, and updating draft.db.tables and draft.db.schemaTrees (createDbSchemaTrees).
🧹 Nitpick comments (1)
packages/data-table/src/ColumnTypeBadge.tsx (1)
14-15: Use visual overflow detection instead of a fixed character threshold.At Line 14,
label.length > 10can disagree with actual truncation (variable glyph widths + fixed badge width). Consider showing tooltip only when the rendered text is truly truncated (scrollWidth > clientWidth).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/data-table/src/ColumnTypeBadge.tsx` around lines 14 - 15, The current fixed check (shouldShowTooltip = label.length > 10) can mis-detect visual truncation; change ColumnTypeBadge to compute shouldShowTooltip by measuring the rendered badge element (use a ref to the text/container element and set shouldShowTooltip = element.scrollWidth > element.clientWidth in a useEffect), update the state accordingly, and re-check on mount and on resize (or via ResizeObserver) with proper cleanup so the tooltip only appears when the text is actually visually truncated.
🤖 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/kepler/src/KeplerSlice.ts`:
- Line 297: The current single-flight logic using syncKeplerPromise lets
concurrent callers share the same promise and therefore miss maps/tables added
after the snapshot (e.g., via createMap(), duplicateMap() or a late db.tables
refresh); add a simple follow-up-queued sync: introduce a pendingSync (boolean)
and, when a caller finds syncKeplerPromise non-null, set pendingSync = true and
return the shared promise; when the active promise resolves, if pendingSync is
true clear it and call the sync function again (loop until pendingSync stays
false) so each late mutation triggers one extra pass and a fresh snapshot is
taken each run; apply the same pattern to the other single-flight block
referenced around lines 486-514.
In `@packages/room-shell/src/RoomShellSlice.ts`:
- Around line 320-322: The catch block in RoomShellSlice that does
"captureException(error); throw error;" is double-reporting startup failures
already handled by BaseRoomStore.captureException in room.initialize(); remove
the duplicate capture call and simply rethrow the error (i.e., replace the catch
body to only "throw error;") so initialization failures are reported once by the
existing BaseRoomStore logic.
In `@packages/ui/src/components/accordion.tsx`:
- Line 37: The trigger icon is inverted because the code uses ChevronUp while
the trigger applies a rotation on open; replace ChevronUp with ChevronDown
(i.e., swap the ChevronUp import/usage to ChevronDown) so the existing rotation
class on the trigger shows the expected closed-down / open-up behavior, or
alternatively invert the rotation logic on the trigger (flip the rotate-180 to
rotate-0 and vice versa) if you prefer to keep ChevronUp; update the reference
where ChevronUp is used in accordion.tsx (the trigger icon render) and ensure
the import matches.
---
Duplicate comments:
In `@packages/duckdb/src/DuckDbSlice.ts`:
- Around line 743-789: refreshTableSchemas currently single-flights concurrent
callers via refreshPromise, but schema-mutating callers (e.g., in
RoomShellSlice) can trigger changes while a refresh is in flight and then still
read stale metadata; add a queued follow-up mechanism: introduce a flag or nonce
(e.g., pendingSchemaRefresh or schemaMutationNonce) that schema-mutating paths
set when they change the catalog, expose a helper like markSchemaDirty() that
sets this flag/increments the nonce, and update refreshTableSchemas (the
refreshPromise flow and its finally block) to, after the current run completes,
check the flag/nonce and if set clear it and invoke refreshTableSchemas again to
perform one guaranteed follow-up (use the nonce to avoid loops if new mutations
arrive during the follow-up); keep existing behaviors around setting
draft.db.isRefreshingTableSchemas, loading via get().db.loadTableSchemas, and
updating draft.db.tables and draft.db.schemaTrees (createDbSchemaTrees).
---
Nitpick comments:
In `@packages/data-table/src/ColumnTypeBadge.tsx`:
- Around line 14-15: The current fixed check (shouldShowTooltip = label.length >
10) can mis-detect visual truncation; change ColumnTypeBadge to compute
shouldShowTooltip by measuring the rendered badge element (use a ref to the
text/container element and set shouldShowTooltip = element.scrollWidth >
element.clientWidth in a useEffect), update the state accordingly, and re-check
on mount and on resize (or via ResizeObserver) with proper cleanup so the
tooltip only appears when the text is actually visually truncated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ae8fa8a-6349-492e-8c3f-e0ec1ed7ec49
📒 Files selected for processing (12)
AGENTS.mdpackages/cells/package.jsonpackages/data-table/src/ColumnTypeBadge.tsxpackages/db/package.jsonpackages/duckdb/__tests__/DuckDbSlice.test.tspackages/duckdb/package.jsonpackages/duckdb/src/DuckDbSlice.tspackages/kepler/src/KeplerSlice.tspackages/notebook/package.jsonpackages/room-shell/src/RoomShellSlice.tspackages/ui/src/components/accordion.tsxpackages/ui/src/components/error-pane.tsx
💤 Files with no reviewable changes (3)
- packages/db/package.json
- packages/notebook/package.json
- packages/cells/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/duckdb/package.json
| } catch (error) { | ||
| captureException(error); | ||
| throw error; |
There was a problem hiding this comment.
Avoid double-reporting initialization failures.
packages/room-store/src/BaseRoomStore.ts, Lines 196-207 already catches rejected room.initialize() calls and reports them via room.captureException(error). Capturing again here before rethrowing will emit duplicate telemetry for the same startup failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/room-shell/src/RoomShellSlice.ts` around lines 320 - 322, The catch
block in RoomShellSlice that does "captureException(error); throw error;" is
double-reporting startup failures already handled by
BaseRoomStore.captureException in room.initialize(); remove the duplicate
capture call and simply rethrow the error (i.e., replace the catch body to only
"throw error;") so initialization failures are reported once by the existing
BaseRoomStore logic.
| > | ||
| {children} | ||
| <ChevronDown className="text-muted-foreground h-4 w-4 shrink-0 transition-transform duration-200" /> | ||
| <ChevronUp className="text-muted-foreground h-4 w-4 shrink-0 transition-transform duration-200" /> |
There was a problem hiding this comment.
Icon direction appears inverted after switching to ChevronUp.
Line 31 still rotates the icon on open; with Line 37 now using ChevronUp, the trigger shows up when closed and down when open, which is usually the opposite for accordions.
Suggested fix
-import {ChevronUp} from 'lucide-react';
+import {ChevronDown} from 'lucide-react';
...
- <ChevronUp className="text-muted-foreground h-4 w-4 shrink-0 transition-transform duration-200" />
+ <ChevronDown className="text-muted-foreground h-4 w-4 shrink-0 transition-transform duration-200" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/accordion.tsx` at line 37, The trigger icon is
inverted because the code uses ChevronUp while the trigger applies a rotation on
open; replace ChevronUp with ChevronDown (i.e., swap the ChevronUp import/usage
to ChevronDown) so the existing rotation class on the trigger shows the expected
closed-down / open-up behavior, or alternatively invert the rotation logic on
the trigger (flip the rotate-180 to rotate-0 and vice versa) if you prefer to
keep ChevronUp; update the reference where ChevronUp is used in accordion.tsx
(the trigger icon render) and ensure the import matches.
…--refreshTableSchemas-improvements Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
…hTableSchemas-improvements Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Summary by CodeRabbit