Closed
Conversation
…xploration
This commit implements the full integration of DuckLake into the SQL Editor, enabling
users to execute queries, explore schemas, and utilize IntelliSense for DuckLake
instances alongside traditional database connections.
Key features and improvements:
- SQL Editor Integration: Added support for 'ducklake' as a first-class connection type.
- Query Execution: Implemented backend routing to DuckLakeService for SQL execution,
including support for SELECT, DDL, and DML operations with command detection.
- Schema Explorer: Developed a professional MUI TreeView for DuckLake metadata,
displaying the schemas → tables → columns hierarchy with type-specific icons.
- Monaco completions: Created a dedicated IntelliSense provider for DuckLake-specific
system tables, metadata views, functions, and time-travel syntax.
- IPC Layer: Added new handlers for query execution, cancellation, and schema extraction.
- Critical Bug Fixes:
- Resolved metadata qualification issues by automatically prefixing internal
queries with the correct metadata database.
- Fixed a critical data normalization bug in the DuckDB adapter where string
values were being incorrectly converted to undefined.
- Corrected schema extraction queries to align with the DuckLake v0.3 specification.
- Fixed infinite loading states in the SQL screen for DuckLake connections.
- UI/UX Polish: Replaced custom lists with standardized components, added
theme-aware icons, and improved connection grouping in the sidebar.
Files changed:
- types: Updated backend, frontend, duckLake, and ipc definitions.
- services: Enhanced DuckLake main and frontend services with execution/extraction logic.
- adapters: Updated DuckDB, PostgreSQL, and SQLite adapters for data normalization.
- UI: Modified SqlEditor, SqlScreen sidebar, and icon Registries.
- utils: Added Monaco completion generator for DuckLake.
|
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 DuckLake support end-to-end: new connection type and types, IPC handlers for execute/cancel/schema-extract, adapter response standardization (pagination, time-travel, numeric normalization), query cancellation tracking, schema extraction/completions, renderer integrations, asset and diagnostic SQL. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Renderer (SQL Editor)
participant UI as Renderer (QueryResult / UI)
participant Main as Main Process (IPC)
participant Adapter as Catalog Adapter
participant DB as Database
Editor->>UI: user runs query (query + optional queryId)
UI->>Main: 'ducklake:query:execute' (query, queryId, instanceId, snapshotId, limit, offset)
Main->>Main: register activeQueries[queryId] -> cancel callback
Main->>Adapter: executeQuery(request)
Adapter->>DB: run SQL (metadata qualification / FOR SYSTEM_TIME AS OF SNAPSHOT)
DB-->>Adapter: rows + schema
Adapter-->>Main: {success,data,fields,rowCount,duration,snapshotId}
Main-->>UI: IPC response (includes duration, isCommand, commandType)
UI->>Editor: render results, update history (isDuckLake, instanceId)
alt user cancels
Editor->>Main: 'ducklake:query:cancel' (queryId)
Main->>Main: lookup activeQueries[queryId] and invoke cancellation
Main-->>Editor: cancellation acknowledged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/main/services/duckLake/adapters/postgresql.adapter.ts (1)
303-376:⚠️ Potential issue | 🔴 Critical
updateRows,deleteRows, andupsertRowscallSELECT changes()which is unsupported by the DuckLake extension.The DuckLake extension does not implement DuckDB's
changes()function, sochangesResult.getRows()returns an empty set or errors out, makingrowsAffectedalways0. This is the same pattern flagged in the sibling DuckDB adapter. Because the actual mutation queries generated by the UI (CTASCREATE OR REPLACE TABLE … AS SELECT) are DDL statements,changes()would return 0 even in vanilla DuckDB.The recommended alternative is to run a
SELECT COUNT(*) FROM … WHERE <condition>query before/after the mutation or require callers to supply aRETURNING-based query. Based on learnings, apply the same fix that was identified forduckdb.adapter.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/postgresql.adapter.ts` around lines 303 - 376, The three methods updateRows, deleteRows, and upsertRows incorrectly rely on "SELECT changes()" (via connectionInfo.connection.run and changesResult.getRows()) which DuckLake does not implement; replace that logic by deriving the affected row count explicitly—either execute a "SELECT COUNT(*) FROM <table> WHERE <same condition>" before/after the mutation when the adapter can construct the predicate from the supplied query, or require callers to supply a RETURNING-style statement and extract the row count from the returned rows (mirror the approach used in duckdb.adapter.ts); update updateRows, deleteRows, and upsertRows to stop calling "SELECT changes()", instead run/parse a COUNT query or use RETURNING results and return rowsAffected accordingly, and remove the console.error message that references changes() if present.src/renderer/screens/sql/index.tsx (1)
477-674:⚠️ Potential issue | 🟡 MinorUse the DuckLake icon consistently in the dropdown.
The selected value uses the DuckLake icon, but the DuckLake menu items use the DuckDB icon (Line 662). This creates a visual mismatch.🎨 Suggested icon consistency
- <img - src={connectionIcons.images.duckdb || defaultIcon} + <img + src={connectionIcons.images.ducklake || defaultIcon} alt="" style={{ width: 14, height: 14, objectFit: 'contain', }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 477 - 674, The dropdown shows DuckLake with the correct icon when rendering the selected value, but each DuckLake MenuItem uses the DuckDB icon; in the duckLakeInstances.map rendering (the MenuItem with key `ducklake-${instance.id}` and the img inside it) replace the src reference from connectionIcons.images.duckdb to connectionIcons.images.ducklake (falling back to defaultIcon) so the menu items and the selected display use the same DuckLake icon.src/main/services/duckLake/adapters/duckdb.adapter.ts (1)
955-1098:⚠️ Potential issue | 🟠 MajorRow counts for DML are misleading without RETURNING.
Line 1097 derivesrowCountfromdata.length; for UPDATE/DELETE/MERGE without aRETURNINGclause this will report0even when rows were affected. The updateRows, deleteRows, and upsertRows methods already acknowledge this limitation in their implementations. Add detection for DML statements and only setrowCountwhenRETURNINGis present, or leave it undefined to prevent misleading zero counts.Suggested guard for DML row counts
- return { + const isDml = /^\s*(INSERT|UPDATE|DELETE|MERGE)\b/i.test(query); + const hasReturning = /\bRETURNING\b/i.test(query); + return { success: true, data, fields, - rowCount: totalRows ?? data.length, + rowCount: totalRows ?? (isDml && !hasReturning ? undefined : data.length), duration, snapshotId, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 955 - 1098, The returned rowCount currently uses totalRows ?? data.length which can incorrectly report 0 for DML statements without RETURNING; update the code in the method that builds and returns { success, data, fields, rowCount, duration } (the block that references query, totalRows and data) to detect DML statements (e.g., /^\s*(UPDATE|DELETE|MERGE|INSERT)\b/i) and check for a RETURNING clause (/\bRETURNING\b/i) on the executed query; only set rowCount when the statement is not a DML or when a RETURNING clause is present (use totalRows if computed, else data.length), otherwise set rowCount to undefined to avoid misleading zero counts. Ensure you reference the same query variable (modified by snapshot/limit/offset) and preserve existing behavior for SELECT/paginated queries.src/main/services/connectors.service.ts (4)
559-587:⚠️ Potential issue | 🟠 Major
testConnectionthrows forducklaketypeNo
ducklakecase; falls todefault: throw new Error('Unsupported connection type: ...'). If the connection UI exposes a "Test" button for DuckLake connections, it will always error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/connectors.service.ts` around lines 559 - 587, testConnection currently lacks a 'ducklake' branch so DuckLake connections fall through to the default error; add a new case 'ducklake' in the switch inside testConnection to call the DuckLake tester (e.g., return testDuckLakeConnection(connection)) or the appropriate existing function name for DuckLake, ensuring the function is imported/available and follows the same await/return pattern as the other cases.
716-759:⚠️ Potential issue | 🔴 Critical
generateJdbcUrlhas noducklakearm — the newisDuckLakebranches ingenerateRosettaYmlandmapToRosettaConnectionare unreachable
generateJdbcUrlis called unconditionally at line 720 (before any type-guard) and at line 867. BecausegenerateJdbcUrl'sdefaultarm throws'Unsupported connection type!', execution never reaches theisDuckLakelogic at lines 726-728 or theducklakebranch at lines 891-893. Both callers will throw for anyducklakeconnection.🔧 Add a `ducklake` arm to `generateJdbcUrl`
+ case 'ducklake': + // DuckLake uses its own connection management; no JDBC URL needed + return ''; default: throw new Error('Unsupported connection type!');This lets the null/empty URL propagate to
generateRosettaYml/mapToRosettaConnection, where the existingisDuckLakeguards already handle the empty string correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/connectors.service.ts` around lines 716 - 759, generateJdbcUrl currently throws on unknown types so any 'ducklake' connection never reaches the isDuckLake guards in generateRosettaYml and mapToRosettaConnection; add a 'ducklake' arm in the generateJdbcUrl function that returns an empty string (or null if the function convention uses that) for ducklake connections so the empty URL propagates and the existing isDuckLake branches (in generateRosettaYml and mapToRosettaConnection) can handle it; update the switch/if that checks connection.type inside generateJdbcUrl to include the 'ducklake' case and return the appropriate empty value consistent with the callers.
761-795:⚠️ Potential issue | 🔴 Critical
validateConnectionthrows forducklake, blocking the entireconfigureConnectionflow
configureConnectioncallsthis.validateConnection(connection)at line 365 unconditionally. With the missingducklakecase, it falls todefault: throw new Error('Unsupported connection type!'), so any attempt to configure a DuckLake connection for a project fails before any other logic runs.🔧 Add a `ducklake` arm
+ case 'ducklake': + if (!(conn as DuckLakeConnectionConfig).instanceId) + throw new Error('DuckLake instance ID is required'); + break; default: throw new Error('Unsupported connection type!');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/connectors.service.ts` around lines 761 - 795, validateConnection is missing a branch for 'ducklake', causing the default "Unsupported connection type!" error and blocking configureConnection; add a case 'ducklake' in validateConnection (alongside existing cases like 'duckdb') and perform the required DuckLake checks (at minimum validate presence of database_path as done for duckdb, or any other DuckLake-specific required fields) so configureConnection can proceed for DuckLake connections.
909-995:⚠️ Potential issue | 🔴 Critical
mapToDbtConnection(andmapToDbtProfileOutput) throw forducklake, breakingloadConfigurations
loadConfigurationscallsmapToDbtConnectionat line 316 andgenerateProfilesYml→mapToDbtProfileOutputat lines 319/1005. Both switches fall through todefault: throw. This means the full project profile-generation flow fails for any DuckLake connection oncevalidateConnectionis fixed.🔧 Add `ducklake` stubs to both methods
In
mapToDbtConnection:+ case 'ducklake': + // DuckLake manages its own connections; no DBT profile needed + return { type: 'ducklake' } as any; default: throw new Error(`Unsupported connection type: ...`);In
mapToDbtProfileOutput:+ case 'ducklake': + return { type: 'ducklake' }; default: throw new Error('Unsupported connection type!');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/connectors.service.ts` around lines 909 - 995, The switch in mapToDbtConnection and mapToDbtProfileOutput currently throws for unknown types and lacks a branch for DuckLake, causing loadConfigurations (which calls mapToDbtConnection) and generateProfilesYml → mapToDbtProfileOutput to fail for ducklake connections; add a ducklake case in both functions that mirrors the duckdb/duckbase mapping (e.g., type: 'ducklake', map database_path to path or other DuckLake-specific fields, include database and schema) so ducklake connections are returned instead of falling through to the default throw; update mapToDbtConnection and mapToDbtProfileOutput to handle 'ducklake' explicitly and ensure generateProfilesYml/loadConfigurations will succeed for DuckLake.
🧹 Nitpick comments (6)
src/main/utils/yamlPartialUpdate.ts (1)
243-245:updateProfilesYmlshould have a defensive ducklake guard for consistency
updateMainConfcorrectly short-circuits forducklake, butupdateProfilesYmllacks an equivalent guard. Although the current codebase callsupdateProfilesYmlonly throughupdateProjectConfigFiles(which has its own guard at line 348),updateProfilesYmlis exported and may be called directly in future code. Adding a guard prevents an inconsistent error fromgenerateProfileOutputFields.♻️ Add a defensive guard to
updateProfilesYmlexport async function updateProfilesYml( projectPath: string, projectName: string, connection: ConnectionInput, ): Promise<void> { + if (connection.type === 'ducklake') { + return; + } const profilesPath = path.join(projectPath, 'profiles.yml');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/yamlPartialUpdate.ts` around lines 243 - 245, Add the same defensive short-circuit for ducklake to updateProfilesYml: inside the exported function updateProfilesYml, check if connection.type === 'ducklake' and return early to avoid calling generateProfileOutputFields; mirror the behavior already used in updateMainConf and updateProjectConfigFiles so direct calls to updateProfilesYml won’t produce inconsistent errors from generateProfileOutputFields.src/renderer/screens/sql/SchemaTreeViewerWithSchema.tsx (1)
110-131: The conditional rendering logic is correct. Minor:schemaMapis always computed even whenhideSchemaLevelistrue.When
hideSchemaLevelistrue, the flat-list path only needsfilteredTables, butschemaMapstill runs itsreduceviauseMemo. For large schemas this is wasted work.♻️ Optional refactor
- const schemaMap = React.useMemo(() => { + const schemaMap = React.useMemo(() => { + if (hideSchemaLevel) return {}; return filteredTables.reduce<Record<string, Table[]>>((acc, table) => { if (!acc[table.schema]) { acc[table.schema] = []; } acc[table.schema].push(table); return acc; }, {}); - }, [filteredTables]); + }, [filteredTables, hideSchemaLevel]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/SchemaTreeViewerWithSchema.tsx` around lines 110 - 131, Currently schemaMap is computed via useMemo even when hideSchemaLevel is true and only filteredTables is used; move or conditionally compute schemaMap so the reduce only runs when hideSchemaLevel is false. Update the useMemo (or create a new one) that builds schemaMap to depend on hideSchemaLevel and return an empty object (or skip computation) when hideSchemaLevel is true, referencing the existing identifiers schemaMap, hideSchemaLevel, filteredTables and the useMemo wrapper so the expensive reduce is avoided for the flat-list render path.src/renderer/components/sqlEditor/index.tsx (1)
126-208: Propagate DuckLake execution duration for parity with other paths.
Line 147 currently dropsduckLakeResult.duration, so timing isn’t available in the UI/history for DuckLake runs.💡 Suggested tweak
result = { success: duckLakeResult.success, data: duckLakeResult.data, fields: mappedFields, rowCount: duckLakeResult.rowCount, error: duckLakeResult.error, + duration: duckLakeResult.duration, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sqlEditor/index.tsx` around lines 126 - 208, The DuckLake branch drops duckLakeResult.duration so timing isn't propagated; update the DuckLakeService.executeQuery handling to include duration in the unified result object (add duration: duckLakeResult.duration), ensure enrichedResult and historyResults/ newHistoryItem carry that duration (e.g., include duration in result, in the truncated historyResults, and in newHistoryItem), and keep the same field name used by other paths so the UI/history can read the timing consistently.src/main/services/duckLake/adapters/duckdb.adapter.ts (1)
583-719: Gate verboselistTableslogging in production.
The addedconsole.logcalls (Line 583-719) can flood logs in normal use; consider routing these through a debug-level logger or a feature flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 583 - 719, The PR introduces many console.log calls inside listTables which will flood production logs; replace or gate them by using the adapter's logger at debug level (e.g., this.logger.debug) or a verbosity flag (e.g., this.options.verbose or process.env.NODE_ENV !== 'production') and remove direct console.* usage; update every occurrence in the listTables flow (search for console.log blocks around listTables, connectionInfo checks, metadataDatabase, query execution, rows mapping and final mapping logs) so only debug-level messages are emitted when verbose/debug is enabled, leaving functional code (DuckLakeTableInfo mapping and normalizeNumericValue usage) unchanged.src/main/services/duckLake.service.ts (2)
1235-1265: Two diagnostic queries add unnecessary round-trips and are immediately discarded
snapshotCheckResult(lines 1235-1248) andschemaTableCheck(lines 1251-1265) exist purely to produce debug output — their results are logged but never used in the returnedDuckLakeSchemaInfo. This means every call toextractSchemafires two extra queries against the catalog before doing any useful work. Remove them before merging.♻️ Remove unused diagnostic queries
- // First, check what the current snapshot is - const snapshotCheckResult = await adapter.executeQuery({...}); - console.log('[DuckLake Service] Current snapshot check:', snapshotCheckResult); - - // Check what's in the schema table - const schemaTableCheck = await adapter.executeQuery({...}); - console.log('[DuckLake Service] Schema table contents:', schemaTableCheck); - // Query metadata catalog for schemas using DuckLake v0.3 schema const schemasResult = await adapter.executeQuery({...});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake.service.ts` around lines 1235 - 1265, Remove the two unused diagnostic queries and their console.log calls in the extractSchema flow: delete the adapter.executeQuery call that sets snapshotCheckResult (SELECT MAX(snapshot_id) ...) and the adapter.executeQuery call that sets schemaTableCheck (SELECT schema_id, schema_name, ...), along with the corresponding console.log statements; ensure no other code depends on snapshotCheckResult or schemaTableCheck (search for those identifiers) and keep the remaining logic in the DuckLakeService.extractSchema method unchanged so no functional behavior is altered.
1222-1226: Remove or gate the ~12 debugconsole.logcalls inextractSchema
extractSchemalogs intermediate state at virtually every step. These statements should not ship to production as they will flood application logs on every editor schema refresh.Also applies to: 1231-1232, 1244-1248, 1261-1265, 1286-1293, 1298-1302, 1331-1337, 1358-1365, 1373-1377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake.service.ts` around lines 1222 - 1226, The extractSchema function currently emits ~12 console.log statements (e.g., the call that logs '[DuckLake Service] extractSchema called for instance:' with instanceId and other logs at the noted ranges); remove these direct console.log calls or wrap them behind a runtime debug flag/check (for example NODE_ENV !== 'production' or a dedicated DuckLakeService.debug flag) so they only run when debugging is enabled; update all occurrences inside extractSchema (including the calls around instanceId and the ranges listed) to use the gated logger or remove them, and ensure any important errors still use processLogger.error or the existing production logger instead of console.log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1694-1725: detectCommandType currently treats any occurrence of
DDL keywords inside the query as DDL (using normalized.includes), causing false
positives for string literals; update the DDL detection in detectCommandType to
check only the statement verb (use normalized.startsWith(...) for each entry in
ddlKeywords or otherwise ensure you match at the beginning of the statement) so
queries like SELECT ... 'DROP TABLE ...' are not misclassified, and align the
ddlKeywords list with validateSingleStatement's allowedPrefixes (e.g., decide
whether TRUNCATE TABLE should be included in both or removed) so validation and
detection use the same set of prefixes.
- Around line 1296-1370: The SQL builds a query by interpolating schemaName
directly into the string (in the adapter.executeQuery call) which can break with
quotes and is unsafe; change the query to use a parameter placeholder (e.g.,
WHERE s.schema_name = $1) and pass schemaName in the executeQuery
parameters/values array (or use the adapter's parameter API) instead of string
interpolation, keeping the rest of the query and the adapter.executeQuery
invocation (and queryId) intact.
- Around line 1171-1217: The current cancelQuery is a no-op because
activeQueries stores only a console.log; change executeQuery usage so the
adapter can supply a real cancel function (e.g., have adapter.executeQuery
return { result, cancel?: () => void } or expose
adapter.getCancelFn(request.queryId)), store that cancel function in
this.activeQueries under the same request.queryId instead of the console log
stub, call that real cancel function in cancelQuery and then delete the entry,
and add a tracked issue referencing the TODO about implementing interruption in
the adapter before shipping.
- Around line 1390-1401: The systemTables array in duckLake.service.ts is
missing the 'ducklake_schema' entry which extractSchema queries (see
extractSchema references at lines where extractSchema calls/query
ducklake_schema); update the systemTables array defined in the same file
(symbol: systemTables inside the duckLake service) to include 'ducklake_schema'
so autocomplete and completion candidates include that table.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts`:
- Around line 795-804: The mapping over rows in executeQuery currently assumes
each row is an object and uses Object.entries(row), which breaks when DuckDB
returns rows as arrays; update the rows.map callback to branch on
Array.isArray(row): if row is an array, iterate over the adapter's fields array
to assign normalized[fields[i].name] = normalizeNumericValue(row[i]) (or
fields[i] if that symbol differs), otherwise keep the existing
Object.entries-based logic that uses normalizeNumericValue(value); ensure you
reference the existing normalizeNumericValue helper and the fields variable so
column names are preserved when rows are arrays.
- Around line 795-804: executeQuery is calling normalizeNumericValue on every
column which turns non-numeric fields into undefined; update the rows -> data
mapping so you only normalize when appropriate: for scalar values call
normalizeNumericValue and if it returns undefined keep the original value, and
for array values map each element through normalizeNumericValue but retain
non-numeric elements unchanged; use the existing normalizeNumericValue helper
and adjust the mapping inside the executeQuery rows processing (leave listTables
behavior untouched) so strings/booleans/dates remain as-is and DuckDB
numeric/array values are converted.
In `@src/main/services/duckLake/adapters/sqlite.adapter.ts`:
- Around line 744-754: The normalization loop in the rows mapping applies
normalizeNumericValue to every cell (in the block creating normalized inside
rows.map), which converts non-numeric types to undefined; change it so you only
call normalizeNumericValue for actual numeric values (e.g., typeof value ===
'number' or strings that match a numeric pattern) and otherwise assign the
original value (preserving null/boolean/string/object) into normalized[key];
update the entries.forEach in sqlite.adapter.ts to perform that conditional
check while keeping the normalized object creation and return behavior
unchanged.
In `@src/renderer/components/dataLake/tableDetails/TableDataRowsTab.tsx`:
- Around line 219-223: The onSuccess handler in TableDataRowsTab.tsx is setting
actualRowCount from data.rowCount (which is just the page size) causing
TablePagination to think the table has at most one page; remove the
setActualRowCount(data.rowCount) call in the onSuccess of the query (leave
setQueryResult(data) intact) so pagination falls back to
tableDetails?.stats?.recordCount, or alternatively only setActualRowCount when
the response is from a dedicated COUNT(*) query; ensure TablePagination receives
the true total via tableDetails?.stats?.recordCount unless a separate count
query populates actualRowCount.
In `@src/renderer/components/schemaTreeViewer/TreeItems.tsx`:
- Around line 75-77: The branch in TreeItems.tsx that checks
lowerType.includes('uuid') currently returns NumbersIcon which is misleading;
update that branch to return TagIcon (the same icon used for other string-like
types) instead of NumbersIcon so UUID columns are represented as string/tag
icons; locate the conditional that references lowerType and replace the
NumbersIcon usage with TagIcon while preserving the same sx styling.
In `@src/renderer/controllers/duckLake.controller.ts`:
- Around line 646-653: The cancelQuery hook correctly calls
DuckLakeService.cancelQuery but the DuckLakeService.cancelQuery handler is
currently a no-op that only logs "marked for cancellation"; update the
DuckLakeService.cancelQuery implementation to actually interrupt running queries
by wiring an abort/cancellation token: have cancelQuery look up the running
query by ID, signal its AbortController (or invoke the adapter/duckdb connection
interrupt method) so the adapter-level execution checks for and responds to
AbortSignal, ensure the adapter exposes a cancel/interrupt method (or uses
AbortSignal) to stop the in-flight DuckDB query, clean up the stored token after
cancellation, and propagate a clear error/result back so useCancelDuckLakeQuery
consumers observe the cancellation outcome.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 216-285: The loadDuckLakeCompletions effect can show stale
schema/items while awaiting DuckLakeService.extractSchema; before starting the
async fetch clear the visible state and capture the current instanceId to avoid
races. Specifically, inside loadDuckLakeCompletions (and after you verify
connectionInput.type === 'ducklake' and instanceId exists) call
setDuckLakeCompletions([]) and setDuckLakeSchema(null) immediately, store the
instanceId in a local variable, then after awaiting
DuckLakeService.extractSchema only call
setDuckLakeCompletions/generateDuckLakeCompletions and setDuckLakeSchema if the
current connectionInput still matches that saved instanceId (or if the saved
instanceId still equals (connectionInput as any).instanceId) to prevent
overwriting with a stale response.
---
Outside diff comments:
In `@src/main/services/connectors.service.ts`:
- Around line 559-587: testConnection currently lacks a 'ducklake' branch so
DuckLake connections fall through to the default error; add a new case
'ducklake' in the switch inside testConnection to call the DuckLake tester
(e.g., return testDuckLakeConnection(connection)) or the appropriate existing
function name for DuckLake, ensuring the function is imported/available and
follows the same await/return pattern as the other cases.
- Around line 716-759: generateJdbcUrl currently throws on unknown types so any
'ducklake' connection never reaches the isDuckLake guards in generateRosettaYml
and mapToRosettaConnection; add a 'ducklake' arm in the generateJdbcUrl function
that returns an empty string (or null if the function convention uses that) for
ducklake connections so the empty URL propagates and the existing isDuckLake
branches (in generateRosettaYml and mapToRosettaConnection) can handle it;
update the switch/if that checks connection.type inside generateJdbcUrl to
include the 'ducklake' case and return the appropriate empty value consistent
with the callers.
- Around line 761-795: validateConnection is missing a branch for 'ducklake',
causing the default "Unsupported connection type!" error and blocking
configureConnection; add a case 'ducklake' in validateConnection (alongside
existing cases like 'duckdb') and perform the required DuckLake checks (at
minimum validate presence of database_path as done for duckdb, or any other
DuckLake-specific required fields) so configureConnection can proceed for
DuckLake connections.
- Around line 909-995: The switch in mapToDbtConnection and
mapToDbtProfileOutput currently throws for unknown types and lacks a branch for
DuckLake, causing loadConfigurations (which calls mapToDbtConnection) and
generateProfilesYml → mapToDbtProfileOutput to fail for ducklake connections;
add a ducklake case in both functions that mirrors the duckdb/duckbase mapping
(e.g., type: 'ducklake', map database_path to path or other DuckLake-specific
fields, include database and schema) so ducklake connections are returned
instead of falling through to the default throw; update mapToDbtConnection and
mapToDbtProfileOutput to handle 'ducklake' explicitly and ensure
generateProfilesYml/loadConfigurations will succeed for DuckLake.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 955-1098: The returned rowCount currently uses totalRows ??
data.length which can incorrectly report 0 for DML statements without RETURNING;
update the code in the method that builds and returns { success, data, fields,
rowCount, duration } (the block that references query, totalRows and data) to
detect DML statements (e.g., /^\s*(UPDATE|DELETE|MERGE|INSERT)\b/i) and check
for a RETURNING clause (/\bRETURNING\b/i) on the executed query; only set
rowCount when the statement is not a DML or when a RETURNING clause is present
(use totalRows if computed, else data.length), otherwise set rowCount to
undefined to avoid misleading zero counts. Ensure you reference the same query
variable (modified by snapshot/limit/offset) and preserve existing behavior for
SELECT/paginated queries.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts`:
- Around line 303-376: The three methods updateRows, deleteRows, and upsertRows
incorrectly rely on "SELECT changes()" (via connectionInfo.connection.run and
changesResult.getRows()) which DuckLake does not implement; replace that logic
by deriving the affected row count explicitly—either execute a "SELECT COUNT(*)
FROM <table> WHERE <same condition>" before/after the mutation when the adapter
can construct the predicate from the supplied query, or require callers to
supply a RETURNING-style statement and extract the row count from the returned
rows (mirror the approach used in duckdb.adapter.ts); update updateRows,
deleteRows, and upsertRows to stop calling "SELECT changes()", instead run/parse
a COUNT query or use RETURNING results and return rowsAffected accordingly, and
remove the console.error message that references changes() if present.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 477-674: The dropdown shows DuckLake with the correct icon when
rendering the selected value, but each DuckLake MenuItem uses the DuckDB icon;
in the duckLakeInstances.map rendering (the MenuItem with key
`ducklake-${instance.id}` and the img inside it) replace the src reference from
connectionIcons.images.duckdb to connectionIcons.images.ducklake (falling back
to defaultIcon) so the menu items and the selected display use the same DuckLake
icon.
---
Nitpick comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1235-1265: Remove the two unused diagnostic queries and their
console.log calls in the extractSchema flow: delete the adapter.executeQuery
call that sets snapshotCheckResult (SELECT MAX(snapshot_id) ...) and the
adapter.executeQuery call that sets schemaTableCheck (SELECT schema_id,
schema_name, ...), along with the corresponding console.log statements; ensure
no other code depends on snapshotCheckResult or schemaTableCheck (search for
those identifiers) and keep the remaining logic in the
DuckLakeService.extractSchema method unchanged so no functional behavior is
altered.
- Around line 1222-1226: The extractSchema function currently emits ~12
console.log statements (e.g., the call that logs '[DuckLake Service]
extractSchema called for instance:' with instanceId and other logs at the noted
ranges); remove these direct console.log calls or wrap them behind a runtime
debug flag/check (for example NODE_ENV !== 'production' or a dedicated
DuckLakeService.debug flag) so they only run when debugging is enabled; update
all occurrences inside extractSchema (including the calls around instanceId and
the ranges listed) to use the gated logger or remove them, and ensure any
important errors still use processLogger.error or the existing production logger
instead of console.log.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 583-719: The PR introduces many console.log calls inside
listTables which will flood production logs; replace or gate them by using the
adapter's logger at debug level (e.g., this.logger.debug) or a verbosity flag
(e.g., this.options.verbose or process.env.NODE_ENV !== 'production') and remove
direct console.* usage; update every occurrence in the listTables flow (search
for console.log blocks around listTables, connectionInfo checks,
metadataDatabase, query execution, rows mapping and final mapping logs) so only
debug-level messages are emitted when verbose/debug is enabled, leaving
functional code (DuckLakeTableInfo mapping and normalizeNumericValue usage)
unchanged.
In `@src/main/utils/yamlPartialUpdate.ts`:
- Around line 243-245: Add the same defensive short-circuit for ducklake to
updateProfilesYml: inside the exported function updateProfilesYml, check if
connection.type === 'ducklake' and return early to avoid calling
generateProfileOutputFields; mirror the behavior already used in updateMainConf
and updateProjectConfigFiles so direct calls to updateProfilesYml won’t produce
inconsistent errors from generateProfileOutputFields.
In `@src/renderer/components/sqlEditor/index.tsx`:
- Around line 126-208: The DuckLake branch drops duckLakeResult.duration so
timing isn't propagated; update the DuckLakeService.executeQuery handling to
include duration in the unified result object (add duration:
duckLakeResult.duration), ensure enrichedResult and historyResults/
newHistoryItem carry that duration (e.g., include duration in result, in the
truncated historyResults, and in newHistoryItem), and keep the same field name
used by other paths so the UI/history can read the timing consistently.
In `@src/renderer/screens/sql/SchemaTreeViewerWithSchema.tsx`:
- Around line 110-131: Currently schemaMap is computed via useMemo even when
hideSchemaLevel is true and only filteredTables is used; move or conditionally
compute schemaMap so the reduce only runs when hideSchemaLevel is false. Update
the useMemo (or create a new one) that builds schemaMap to depend on
hideSchemaLevel and return an empty object (or skip computation) when
hideSchemaLevel is true, referencing the existing identifiers schemaMap,
hideSchemaLevel, filteredTables and the useMemo wrapper so the expensive reduce
is avoided for the flat-list render path.
Comment on lines
+646
to
+653
| export function useCancelDuckLakeQuery() { | ||
| return useMutation({ | ||
| mutationFn: DuckLakeService.cancelQuery, | ||
| onError: (error: Error) => { | ||
| toast.error(`Query cancellation failed: ${error.message}`); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
useCancelDuckLakeQuery wires correctly but the server-side cancellation is a stub — actual query interruption is not implemented yet.
The cancelQuery handler in DuckLakeService (main process) stores a placeholder lambda that only logs "marked for cancellation" without interrupting the underlying DuckDB connection or adapter execution. The hook is safe to ship as scaffolding, but callers should not rely on it to actually stop an in-flight query until the adapter-level abort is wired in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/controllers/duckLake.controller.ts` around lines 646 - 653, The
cancelQuery hook correctly calls DuckLakeService.cancelQuery but the
DuckLakeService.cancelQuery handler is currently a no-op that only logs "marked
for cancellation"; update the DuckLakeService.cancelQuery implementation to
actually interrupt running queries by wiring an abort/cancellation token: have
cancelQuery look up the running query by ID, signal its AbortController (or
invoke the adapter/duckdb connection interrupt method) so the adapter-level
execution checks for and responds to AbortSignal, ensure the adapter exposes a
cancel/interrupt method (or uses AbortSignal) to stop the in-flight DuckDB
query, clean up the stored token after cancellation, and propagate a clear
error/result back so useCancelDuckLakeQuery consumers observe the cancellation
outcome.
Add export button to query results toolbar with dropdown menu for JSON, CSV, and Parquet formats. JSON and CSV exports are client-side downloads, while Parquet export uses DuckDB/COPY command for DuckDB and DuckLake connections. Export is disabled when there are no results.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/screens/sql/index.tsx (1)
408-418:⚠️ Potential issue | 🟠 Major
handleRefreshSchemasilently no-ops for DuckLake connections.
fetchSchemaForConnectionreturns early forducklake-IDs (line 294-303), only setting an emptytabSchemasentry. The DuckLake schema stored induckLakeSchema/duckLakeTablesis never refreshed becauseloadDuckLakeCompletionsonly runs whenconnectionInputchanges. Clicking the Refresh icon on a DuckLake schema tree produces no visible result.A dedicated refresh path is needed:
🐛 Proposed fix
const handleRefreshSchema = useCallback(() => { if (activeTab) { + if (isDuckLakeConnection) { + // Force re-fetch DuckLake schema by resetting state; + // loadDuckLakeCompletions watches connectionInput but a + // dedicated refresh counter is cleaner. + setDuckLakeSchema(null); + setDuckLakeCompletions([]); + // Re-trigger the effect by bumping a refresh counter: + // setDuckLakeRefreshKey((k) => k + 1); + return; + } setTabSchemas((prev) => { const updated = { ...prev }; delete updated[activeTab.connectionId]; return updated; }); fetchSchemaForConnection(activeTab.connectionId); } - }, [activeTab, fetchSchemaForConnection]); + }, [activeTab, isDuckLakeConnection, fetchSchemaForConnection]);Note: because effects can't be manually triggered, adding a
duckLakeRefreshKeycounter toloadDuckLakeCompletions's dependency array is the idiomatic React approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 408 - 418, handleRefreshSchema currently only clears tabSchemas and calls fetchSchemaForConnection, but fetchSchemaForConnection bails for DuckLake IDs and so DuckLake state (duckLakeSchema / duckLakeTables) never refreshes; add a duckLakeRefreshKey state/counter and include it in the dependency array of the effect/function that runs loadDuckLakeCompletions so that increments re-run it. Update handleRefreshSchema to detect when activeTab.connectionId startsWith("ducklake-") and increment duckLakeRefreshKey (instead of—or in addition to—calling fetchSchemaForConnection), while keeping the existing cache-clear logic for non-DuckLake connections so that loadDuckLakeCompletions (referenced by the duckLake-related effect) re-executes and repopulates duckLakeSchema/duckLakeTables.
🧹 Nitpick comments (2)
src/renderer/screens/sql/index.tsx (2)
642-673:onClickhandler onMenuIteminsideSelectis a fragile workaround.Because
Select.onChange(line 463) only handles IDs found inconnections, and DuckLake IDs are absent from that array, DuckLake item selection silently falls throughonChangeand relies entirely on theMenuItem'sonClick. This pattern is brittle: future MUISelectupdates or event-order changes could break DuckLake selection without obvious symptoms.Consider extending the
onChangehandler to recogniseducklake-IDs directly, removing the need for per-itemonClick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 642 - 673, The current DuckLake MenuItem uses an onClick workaround because Select.onChange only recognises IDs in the connections array; update the Select.onChange handler to detect values prefixed with "ducklake-" (from duckLakeInstances) and call handleConnectionSelect with { id, name: instance.name, type: 'ducklake' } instead of relying on per-MenuItem onClick; locate the Select change handler (Select.onChange) and modify it to parse value strings like "ducklake-<id>" and resolve the matching instance from duckLakeInstances, then invoke handleConnectionSelect so MenuItem no longer needs an onClick.
138-166: Remove debug logging effects before merging.Both effects exist solely to print state to the console. They add unnecessary re-subscriptions and log noise in production. Structured logging or a flag-gated debug mode would be more appropriate if observability is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 138 - 166, Remove the two debug-only useEffect blocks that log state to the console: the effect referencing activeTab/isLoadingSchema/tabSchemas (the "[SQL Screen] Loading state" logger) and the effect that logs duckLakeSchema and duckLakeSchema.schemas (related to duckLakeSchema and duckLakeCompletions/setDuckLakeSchema). Either delete these effects entirely or gate them behind an explicit debug flag (e.g., if (IS_DEBUG) { ... }) so production builds do not subscribe/log; ensure no other logic depends on those effects before removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 927-938: The export currently uses the live editor value
activeTab.query which changes on keystrokes; modify the tab state to store the
last executed SQL (e.g., add activeTab.lastExecutedQuery and set it when
dispatching/running a query), then update where QueryResult is rendered to pass
exportContext.originalSql = activeTab.lastExecutedQuery (fall back to
activeTab.query if needed for safety); ensure the query-run code path that
executes SQL assigns lastExecutedQuery before or after successful execution so
exported SQL matches the result set (update any functions that dispatch/run
queries to set lastExecutedQuery).
- Around line 660-672: The dropdown MenuItem for DuckLake is using the wrong
icon key: replace usage of connectionIcons.images.duckdb with
connectionIcons.images.ducklake in the MenuItem rendering (the block that
renders <MenuItem> showing instance.name) so it matches renderValue which
already uses connectionIcons.images.ducklake; locate the MenuItem that displays
instance.name and update the img src to connectionIcons.images.ducklake with the
existing fallback (defaultIcon).
- Around line 753-763: The SchemaTreeViewer is keyed to isLoading via
!duckLakeSchema which causes permanent loading when
DuckLakeService.extractSchema fails and setDuckLakeSchema(null) is called;
update loadDuckLakeCompletions to maintain a separate boolean state (e.g.,
duckLakeSchemaLoading) that is true when starting extraction and set to false in
both the try and catch blocks, and keep setDuckLakeSchema(null) only for the
schema value; then change the JSX prop isLoading to use duckLakeSchemaLoading
and add a conditional or error/empty state when duckLakeSchema is null and
duckLakeSchemaLoading is false so SchemaTreeViewerWithSchema can show failure or
a retry (references: loadDuckLakeCompletions, DuckLakeService.extractSchema,
setDuckLakeSchema, duckLakeSchema, duckLakeSchemaLoading, isDuckLakeConnection,
SchemaTreeViewerWithSchema, handleRefreshSchema).
In `@src/renderer/screens/sql/queryResult.tsx`:
- Around line 99-106: escapeCsvValue currently misses carriage returns because
its regex only checks /[",\n]/; update the check to include '\r' (e.g.
/[",\r\n]/) so values containing CR (Windows CRLF/legacy data) get quoted, and
keep the existing double-quote escaping logic (String(value).replace(/"/g,
'""')) unchanged; edit the escapeCsvValue function to use the new character
class to prevent malformed CSV rows.
- Around line 82-94: The download handler handleDownloadJson currently calls
JSON.stringify(rows, null, 2) which will throw for BigInt values from
DuckDB/DuckLake; update handleDownloadJson to use a replacer when serializing
rows (or call a small utility that walks values) that detects typeof value ===
'bigint' and returns value.toString() (or another chosen string format) so
JSON.stringify succeeds, then continue creating the Blob, object URL, triggering
the download and revoking the URL as before; reference the rows and hasRows
checks and keep the rest of the logic (Blob creation, a.click(),
revokeObjectURL, handleExportMenuClose) unchanged.
- Around line 170-174: The catch block for the Parquet export currently only
logs to console and closes the menu; update the catch in the function containing
handleExportMenuClose to call toast.error with a friendly failure message (e.g.,
"Failed to export Parquet: <brief reason>") so the user gets visible feedback,
then call handleExportMenuClose(); ensure toast is imported from
'react-toastify' at the top of the file if not present and preserve the existing
console.error logging for debug purposes (reference handleExportMenuClose and
the catch block around the IPC COPY call).
---
Outside diff comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 408-418: handleRefreshSchema currently only clears tabSchemas and
calls fetchSchemaForConnection, but fetchSchemaForConnection bails for DuckLake
IDs and so DuckLake state (duckLakeSchema / duckLakeTables) never refreshes; add
a duckLakeRefreshKey state/counter and include it in the dependency array of the
effect/function that runs loadDuckLakeCompletions so that increments re-run it.
Update handleRefreshSchema to detect when activeTab.connectionId
startsWith("ducklake-") and increment duckLakeRefreshKey (instead of—or in
addition to—calling fetchSchemaForConnection), while keeping the existing
cache-clear logic for non-DuckLake connections so that loadDuckLakeCompletions
(referenced by the duckLake-related effect) re-executes and repopulates
duckLakeSchema/duckLakeTables.
---
Duplicate comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 216-285: The stale state bug: before calling
DuckLakeService.extractSchema in the useEffect/loadDuckLakeCompletions flow,
immediately clear duckLakeCompletions and duckLakeSchema (via
setDuckLakeCompletions([]) and setDuckLakeSchema(null)) and then kick off the
async call; also add a local cancellation guard (e.g., capture the current
instanceId or incrementing requestId inside the useEffect) and, after await
DuckLakeService.extractSchema(instanceId), verify the captured id still matches
connectionInput (and only then call setDuckLakeCompletions and setDuckLakeSchema
with generateDuckLakeCompletions result) so responses from previous TOCTOU calls
are ignored.
---
Nitpick comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 642-673: The current DuckLake MenuItem uses an onClick workaround
because Select.onChange only recognises IDs in the connections array; update the
Select.onChange handler to detect values prefixed with "ducklake-" (from
duckLakeInstances) and call handleConnectionSelect with { id, name:
instance.name, type: 'ducklake' } instead of relying on per-MenuItem onClick;
locate the Select change handler (Select.onChange) and modify it to parse value
strings like "ducklake-<id>" and resolve the matching instance from
duckLakeInstances, then invoke handleConnectionSelect so MenuItem no longer
needs an onClick.
- Around line 138-166: Remove the two debug-only useEffect blocks that log state
to the console: the effect referencing activeTab/isLoadingSchema/tabSchemas (the
"[SQL Screen] Loading state" logger) and the effect that logs duckLakeSchema and
duckLakeSchema.schemas (related to duckLakeSchema and
duckLakeCompletions/setDuckLakeSchema). Either delete these effects entirely or
gate them behind an explicit debug flag (e.g., if (IS_DEBUG) { ... }) so
production builds do not subscribe/log; ensure no other logic depends on those
effects before removal.
Comment on lines
+660
to
+672
| <Box sx={{ display: 'flex', alignItems: 'center', gap: 1 }}> | ||
| <img | ||
| src={connectionIcons.images.duckdb || defaultIcon} | ||
| alt="" | ||
| style={{ | ||
| width: 14, | ||
| height: 14, | ||
| objectFit: 'contain', | ||
| }} | ||
| /> | ||
| {instance.name} | ||
| </Box> | ||
| </MenuItem> |
There was a problem hiding this comment.
DuckLake MenuItem uses the duckdb icon instead of ducklake.
renderValue (line 495) correctly uses connectionIcons.images.ducklake, but the MenuItem in the dropdown (line 663) uses connectionIcons.images.duckdb. These should be consistent.
🎨 Proposed fix
- src={connectionIcons.images.duckdb || defaultIcon}
+ src={connectionIcons.images.ducklake || defaultIcon}📝 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
| <Box sx={{ display: 'flex', alignItems: 'center', gap: 1 }}> | |
| <img | |
| src={connectionIcons.images.duckdb || defaultIcon} | |
| alt="" | |
| style={{ | |
| width: 14, | |
| height: 14, | |
| objectFit: 'contain', | |
| }} | |
| /> | |
| {instance.name} | |
| </Box> | |
| </MenuItem> | |
| <Box sx={{ display: 'flex', alignItems: 'center', gap: 1 }}> | |
| <img | |
| src={connectionIcons.images.ducklake || defaultIcon} | |
| alt="" | |
| style={{ | |
| width: 14, | |
| height: 14, | |
| objectFit: 'contain', | |
| }} | |
| /> | |
| {instance.name} | |
| </Box> | |
| </MenuItem> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/index.tsx` around lines 660 - 672, The dropdown
MenuItem for DuckLake is using the wrong icon key: replace usage of
connectionIcons.images.duckdb with connectionIcons.images.ducklake in the
MenuItem rendering (the block that renders <MenuItem> showing instance.name) so
it matches renderValue which already uses connectionIcons.images.ducklake;
locate the MenuItem that displays instance.name and update the img src to
connectionIcons.images.ducklake with the existing fallback (defaultIcon).
src/renderer/screens/sql/index.tsx
Outdated
Comment on lines
+753
to
+763
| {activeTab && connectionInput && isDuckLakeConnection && ( | ||
| <SchemaTreeViewerWithSchema | ||
| databaseName={connectionInput.name || 'DuckLake Instance'} | ||
| type="ducklake" | ||
| schema={duckLakeTables} | ||
| isLoading={!duckLakeSchema} | ||
| onRefresh={handleRefreshSchema} | ||
| filter={filter} | ||
| hideSchemaLevel | ||
| /> | ||
| )} |
There was a problem hiding this comment.
isLoading={!duckLakeSchema} causes an unrecoverable infinite loading state on schema extraction failure.
When DuckLakeService.extractSchema throws, the catch block (line 280) calls setDuckLakeSchema(null), which keeps !duckLakeSchema === true → the SchemaTreeViewerWithSchema stays in its loading state indefinitely with no error indicator and no way for the user to recover.
Track loading independently:
🐛 Proposed fix
+ const [isDuckLakeSchemaLoading, setIsDuckLakeSchemaLoading] = useState(false);
const [duckLakeCompletions, setDuckLakeCompletions] = useState<any[]>([]);
const [duckLakeSchema, setDuckLakeSchema] = useState<any>(null);In loadDuckLakeCompletions:
try {
+ setIsDuckLakeSchemaLoading(true);
const schema = await DuckLakeService.extractSchema(instanceId);
...
setDuckLakeSchema(schema);
+ } catch ...
+ } finally {
+ setIsDuckLakeSchemaLoading(false);
}In the JSX:
- isLoading={!duckLakeSchema}
+ isLoading={isDuckLakeSchemaLoading}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/index.tsx` around lines 753 - 763, The
SchemaTreeViewer is keyed to isLoading via !duckLakeSchema which causes
permanent loading when DuckLakeService.extractSchema fails and
setDuckLakeSchema(null) is called; update loadDuckLakeCompletions to maintain a
separate boolean state (e.g., duckLakeSchemaLoading) that is true when starting
extraction and set to false in both the try and catch blocks, and keep
setDuckLakeSchema(null) only for the schema value; then change the JSX prop
isLoading to use duckLakeSchemaLoading and add a conditional or error/empty
state when duckLakeSchema is null and duckLakeSchemaLoading is false so
SchemaTreeViewerWithSchema can show failure or a retry (references:
loadDuckLakeCompletions, DuckLakeService.extractSchema, setDuckLakeSchema,
duckLakeSchema, duckLakeSchemaLoading, isDuckLakeConnection,
SchemaTreeViewerWithSchema, handleRefreshSchema).
Comment on lines
+927
to
+938
| <QueryResult | ||
| results={activeTab.results} | ||
| exportContext={{ | ||
| connectionType: connectionInput.type, | ||
| connectionId: activeTab.connectionId, | ||
| duckLakeInstanceId: | ||
| connectionInput.type === 'ducklake' | ||
| ? (connectionInput as any).instanceId | ||
| : undefined, | ||
| originalSql: activeTab.query, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
originalSql: activeTab.query reflects current editor content, not the last-executed query.
activeTab.query is updated on every keystroke. If a user edits the query after executing it (without re-running), clicking "Export Parquet" issues the COPY command against the new, unexecuted SQL — not the query that produced the visible result set. The exported file will not match the displayed data.
The fix requires persisting the last-executed query in the tab state (e.g., activeTab.lastExecutedQuery) and setting it at query dispatch time instead of deriving it from the live editor content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/index.tsx` around lines 927 - 938, The export
currently uses the live editor value activeTab.query which changes on
keystrokes; modify the tab state to store the last executed SQL (e.g., add
activeTab.lastExecutedQuery and set it when dispatching/running a query), then
update where QueryResult is rendered to pass exportContext.originalSql =
activeTab.lastExecutedQuery (fall back to activeTab.query if needed for safety);
ensure the query-run code path that executes SQL assigns lastExecutedQuery
before or after successful execution so exported SQL matches the result set
(update any functions that dispatch/run queries to set lastExecutedQuery).
Comment on lines
+82
to
+94
| const handleDownloadJson = () => { | ||
| if (!hasRows) return; | ||
| const blob = new Blob([JSON.stringify(rows, null, 2)], { | ||
| type: 'application/json', | ||
| }); | ||
| const url = window.URL.createObjectURL(blob); | ||
| const a = document.createElement('a'); | ||
| a.href = url; | ||
| a.download = 'query_results.json'; | ||
| a.click(); | ||
| window.URL.revokeObjectURL(url); | ||
| handleExportMenuClose(); | ||
| }; |
There was a problem hiding this comment.
JSON.stringify will throw on BigInt values from DuckDB/DuckLake BIGINT columns.
Electron IPC uses the structured clone algorithm, which preserves BigInt across the main/renderer boundary. Calling JSON.stringify(rows, ...) without a replacer will throw a TypeError: Do not know how to serialize a BigInt for any result set containing a BIGINT column. Wrap the call or use a replacer:
🐛 Proposed fix
- const blob = new Blob([JSON.stringify(rows, null, 2)], {
+ const bigIntReplacer = (_: string, v: unknown) =>
+ typeof v === 'bigint' ? v.toString() : v;
+ const blob = new Blob([JSON.stringify(rows, bigIntReplacer, 2)], {
type: 'application/json',
});📝 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
| const handleDownloadJson = () => { | |
| if (!hasRows) return; | |
| const blob = new Blob([JSON.stringify(rows, null, 2)], { | |
| type: 'application/json', | |
| }); | |
| const url = window.URL.createObjectURL(blob); | |
| const a = document.createElement('a'); | |
| a.href = url; | |
| a.download = 'query_results.json'; | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| handleExportMenuClose(); | |
| }; | |
| const handleDownloadJson = () => { | |
| if (!hasRows) return; | |
| const bigIntReplacer = (_: string, v: unknown) => | |
| typeof v === 'bigint' ? v.toString() : v; | |
| const blob = new Blob([JSON.stringify(rows, bigIntReplacer, 2)], { | |
| type: 'application/json', | |
| }); | |
| const url = window.URL.createObjectURL(blob); | |
| const a = document.createElement('a'); | |
| a.href = url; | |
| a.download = 'query_results.json'; | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| handleExportMenuClose(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/queryResult.tsx` around lines 82 - 94, The download
handler handleDownloadJson currently calls JSON.stringify(rows, null, 2) which
will throw for BigInt values from DuckDB/DuckLake; update handleDownloadJson to
use a replacer when serializing rows (or call a small utility that walks values)
that detects typeof value === 'bigint' and returns value.toString() (or another
chosen string format) so JSON.stringify succeeds, then continue creating the
Blob, object URL, triggering the download and revoking the URL as before;
reference the rows and hasRows checks and keep the rest of the logic (Blob
creation, a.click(), revokeObjectURL, handleExportMenuClose) unchanged.
Comment on lines
+99
to
+106
| const escapeCsvValue = (value: unknown): string => { | ||
| if (value === null || value === undefined) return ''; | ||
| const str = String(value); | ||
| if (/[",\n]/.test(str)) { | ||
| return `"${str.replace(/"/g, '""')}"`; | ||
| } | ||
| return str; | ||
| }; |
There was a problem hiding this comment.
\r (carriage return) not handled in CSV escaping.
The regex /[",\n]/ misses bare \r. Data containing \r (e.g., Windows CRLF in TEXT columns or legacy data) will produce malformed rows that split incorrectly in parsers.
🐛 Proposed fix
- if (/[",\n]/.test(str)) {
+ if (/[",\n\r]/.test(str)) {📝 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
| const escapeCsvValue = (value: unknown): string => { | |
| if (value === null || value === undefined) return ''; | |
| const str = String(value); | |
| if (/[",\n]/.test(str)) { | |
| return `"${str.replace(/"/g, '""')}"`; | |
| } | |
| return str; | |
| }; | |
| const escapeCsvValue = (value: unknown): string => { | |
| if (value === null || value === undefined) return ''; | |
| const str = String(value); | |
| if (/[",\n\r]/.test(str)) { | |
| return `"${str.replace(/"/g, '""')}"`; | |
| } | |
| return str; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/queryResult.tsx` around lines 99 - 106,
escapeCsvValue currently misses carriage returns because its regex only checks
/[",\n]/; update the check to include '\r' (e.g. /[",\r\n]/) so values
containing CR (Windows CRLF/legacy data) get quoted, and keep the existing
double-quote escaping logic (String(value).replace(/"/g, '""')) unchanged; edit
the escapeCsvValue function to use the new character class to prevent malformed
CSV rows.
Comment on lines
+170
to
+174
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Parquet export error:', error); | ||
| handleExportMenuClose(); | ||
| } |
There was a problem hiding this comment.
Silent Parquet export failure — no user feedback.
When the IPC COPY command throws, only console.error is called and the menu closes silently. The user has no way to know the export failed. Given that react-toastify is already imported and used elsewhere in index.tsx, adding an error toast here is straightforward:
🐛 Proposed fix
} catch (error) {
// eslint-disable-next-line no-console
console.error('Parquet export error:', error);
+ // Surface the error to the user
+ // e.g.: toast.error(`Parquet export failed: ${(error as Error).message}`);
handleExportMenuClose();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/screens/sql/queryResult.tsx` around lines 170 - 174, The catch
block for the Parquet export currently only logs to console and closes the menu;
update the catch in the function containing handleExportMenuClose to call
toast.error with a friendly failure message (e.g., "Failed to export Parquet:
<brief reason>") so the user gets visible feedback, then call
handleExportMenuClose(); ensure toast is imported from 'react-toastify' at the
top of the file if not present and preserve the existing console.error logging
for debug purposes (reference handleExportMenuClose and the catch block around
the IPC COPY call).
- Implement paginated query execution with limit/offset for DuckLake connections - Add server-side data fetching with custom pagination controls in query results table - Support query cancellation through adapter's getCancelFn method - Preserve original SQL across query execution and result display - Enhance table pagination with formatted number display and extended rows per page options - Adjust SQL screen layout to accommodate pagination controls
…and result display limits, and correct CSV value escaping.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/services/duckLake/adapters/sqlite.adapter.ts (1)
274-356:⚠️ Potential issue | 🟠 Major
changes()is unsupported by the DuckLake extension —rowsAffectedwill be incorrect in all three DML methods.
updateRows(line 284),deleteRows(line 312), andupsertRows(line 340) all issueSELECT changes() as changesto report affected-row counts. Per prior learnings, the DuckLake extension does not support thechanges()function, so these calls will fail or return incorrect counts.The recommended fix (consistent with the DuckDB adapter learning) is to add a
RETURNING *clause to the DML query and useresult.getRows().length, or switch to a method that the extension does support.Based on learnings: "DuckLake extension does not support DuckDB's
changes()function for retrieving affected row counts… This guidance should apply to all TypeScript files in thesrc/main/services/duckLake/adaptersdirectory that perform DML and infer counts from returned rows."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/sqlite.adapter.ts` around lines 274 - 356, The DML methods updateRows, deleteRows and upsertRows currently call 'SELECT changes()' which DuckLake doesn't support; change each method to execute the provided DML with a 'RETURNING *' clause (or detect/append RETURNING if already absent) via this.connectionInfo.connection.run(upsertQuery|updateQuery|deleteQuery), then call result.getRows() and use its length as rowsAffected (i.e. rowsAffected = rows.length), removing the SELECT changes() logic and related hugeint parsing and fallbacks; ensure you still throw when !this.connectionInfo and preserve existing error logging.src/main/services/duckLake/adapters/postgresql.adapter.ts (1)
294-376:⚠️ Potential issue | 🟠 Major
SELECT changes()is not supported by DuckLake —updateRows,deleteRows, andupsertRowswill always return incorrect row counts.All three DML methods issue
SELECT changes() as changesto measure affected rows. Per the DuckLake adapter learning for this project, DuckLake does not support DuckDB'schanges()function. In practice the call may return 0 or throw silently, making therowsAffectedvalue unreliable for any caller that relies on it (e.g., confirmation dialogs, audit logs).The fix is to either:
- Include a
RETURNING *clause in the DML query and useresult.getRows().length, or- Check whether the DuckDB Node.js driver exposes an
affectedRows/rowCountproperty on the result object.Based on learnings: DuckLake extension does not support DuckDB's
changes()function for retrieving affected row counts. This guidance should apply to all TypeScript files in thesrc/main/services/duckLake/adaptersdirectory that perform DML and infer counts from returned rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/postgresql.adapter.ts` around lines 294 - 376, The updateRows, deleteRows, and upsertRows implementations currently call SELECT changes() to compute rowsAffected, but DuckLake does not support DuckDB's changes() so the count is unreliable; update each method (updateRows, deleteRows, upsertRows) to obtain affected row counts from the DML result instead: either require/append a RETURNING * clause to the provided upsertQuery/updateQuery/deleteQuery and use the returned result.getRows().length, or read a driver-provided affectedRows/rowCount property from the object returned by connectionInfo.connection.run (and fall back to 0 if neither is available); ensure you replace the SELECT changes() call and related parsing of rows with this new logic in each method.src/main/services/duckLake/adapters/duckdb.adapter.ts (1)
27-27:⚠️ Potential issue | 🟠 Major
src/rendererutility imported into the main process — architectural violation
normalizeNumericValuelives undersrc/renderer/utils/fileUtils. Importing renderer code from the main process:
- Couples the main process bundle to renderer code paths.
- Silently breaks if
fileUtils.tsever adopts a browser-only API (window,document, etc.).- Violates Electron's recommended process-isolation design where each process is independently bundled.
Extract the function to a shared location (e.g.,
src/shared/utils/numericUtils.ts) and import from there in both processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` at line 27, Currently the main process adapter (duckdb.adapter.ts) imports normalizeNumericValue from src/renderer, which breaks process isolation; move the normalizeNumericValue implementation out of src/renderer/utils/fileUtils.ts into a new shared module (e.g., src/shared/utils/numericUtils.ts) and export it, then update duckdb.adapter.ts to import normalizeNumericValue from the new shared path; also update the renderer code to import the function from src/shared/utils/numericUtils.ts and remove the renderer-only import to ensure no renderer APIs are referenced from the main process.src/renderer/screens/sql/index.tsx (1)
408-418:⚠️ Potential issue | 🟠 MajorRefresh button does not reload DuckLake schema
handleRefreshSchemacallsfetchSchemaForConnection, which for DuckLake connections (line 294–303) immediately sets an empty schema and returns without re-fetching.duckLakeSchemaandduckLakeCompletionsremain stale becauseloadDuckLakeCompletionsonly re-runs whenconnectionInputchanges (line 285), which clicking Refresh does not trigger.🐛 Proposed fix
+ // Expose loadDuckLakeCompletions as a stable callback to allow manual refresh + const reloadDuckLakeSchema = React.useCallback(() => { + setDuckLakeSchema(null); + setDuckLakeCompletions([]); + // trigger re-run of the effect by bumping a counter or calling directly + }, []); const handleRefreshSchema = useCallback(() => { if (activeTab) { + if (isDuckLakeConnection) { + setDuckLakeSchema(null); + setDuckLakeCompletions([]); + // Re-trigger loadDuckLakeCompletions (e.g., via a refresh counter dep) + return; + } setTabSchemas((prev) => { const updated = { ...prev }; delete updated[activeTab.connectionId]; return updated; }); fetchSchemaForConnection(activeTab.connectionId); } - }, [activeTab, fetchSchemaForConnection]); + }, [activeTab, isDuckLakeConnection, fetchSchemaForConnection]);One clean approach: add a
refreshKeycounter state toloadDuckLakeCompletions's dependency array and increment it fromhandleRefreshSchemawhenisDuckLakeConnectionis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 408 - 418, handleRefreshSchema currently calls fetchSchemaForConnection but that path early-returns for DuckLake connections so duckLakeSchema/duckLakeCompletions stay stale; add a refreshKey state (e.g., const [duckLakeRefreshKey, setDuckLakeRefreshKey] = useState(0)) and include it in loadDuckLakeCompletions' dependency array alongside connectionInput so it re-runs when refreshKey changes, then in handleRefreshSchema increment this key (setDuckLakeRefreshKey(k=>k+1)) whenever isDuckLakeConnection is true; this ensures loadDuckLakeCompletions will reload duckLakeSchema and duckLakeCompletions on Refresh.
🧹 Nitpick comments (3)
src/main/services/duckLake/adapters/sqlite.adapter.ts (1)
735-754:COUNT(*)subquery runs on every paginated request, doubling I/O.Wrapping the user query in
SELECT COUNT(*) as total FROM (${query})forces a full scan before each paginated fetch. For large DuckLake tables this can double execution time. Consider making the count opt-in (e.g. only when the caller passes aincludeTotalCountflag), or cache the total for subsequentoffsetfetches on the same query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/sqlite.adapter.ts` around lines 735 - 754, The current logic inside the limit handling block (when limit && !hasExistingLimit) runs a costly COUNT(*) by building countQuery and executing this.connectionInfo.connection.run(countQuery) for every paginated fetch; change this so the COUNT is optional and/or cached: add and check an includeTotalCount boolean flag on the request/caller context before constructing/executing countQuery (only run the COUNT when includeTotalCount is true), and/or implement a simple cache keyed by the normalized query + params to store totalRows after the first COUNT and reuse it for subsequent offset/limit requests to avoid re-running the COUNT; update references around isSelectQuery, totalRows, and the countQuery execution accordingly.src/renderer/screens/sql/index.tsx (1)
139-166: Remove debug-onlyuseEffecthooks before shippingTwo
useEffectblocks (lines 139–149 and 156–166) exist solely toconsole.logloading and schema state. They produce noise in production and run on every render cycle. Both should be removed prior to the release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 139 - 166, Remove the two debug-only useEffect hooks that only console.log runtime state: the effect that logs loading state for activeTab (references activeTab, isLoadingSchema, tabSchemas) and the effect that logs duckLakeSchema changes (references duckLakeSchema). Delete these useEffect blocks entirely from src/renderer/screens/sql/index.tsx and ensure you keep the related state declarations (duckLakeCompletions, setDuckLakeCompletions, duckLakeSchema, setDuckLakeSchema) intact so runtime logic is unchanged.src/main/services/duckLake/adapters/duckdb.adapter.ts (1)
874-904:getMetadataPrefixissues an extra DB roundtrip on every query execution
qualifyMetadataTablesis called insideexecuteQuery, and it callsgetMetadataPrefix()which runsSELECT database_name FROM duckdb_databases() WHERE …on every invocation. ForextractSchema, which makes 3 + Nadapter.executeQuerycalls (N = number of schemas), this adds 3 + N redundant metadata lookups. The prefix is stable for the life of a connection.♻️ Proposed cache
+ private metadataPrefixCache: string | null = null; private async getMetadataPrefix(): Promise<string> { + if (this.metadataPrefixCache !== null) return this.metadataPrefixCache; try { // … existing query … - return `"${metadataDatabase}".main.`; + const prefix = `"${metadataDatabase}".main.`; + this.metadataPrefixCache = prefix; + return prefix; } catch { return ''; } }Reset
this.metadataPrefixCache = nullinsidecleanup()/disconnect().Similarly, the two diagnostic
adapter.executeQuerycalls at the top ofDuckLakeService.extractSchema(snapshot-check and schema-table-check) are purely for debug logging and add two extra roundtrips on every schema load. They should be removed or gated behind a debug flag before shipping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 874 - 904, getMetadataPrefix currently queries duckdb_databases() on every call causing extra roundtrips because qualifyMetadataTables is invoked inside executeQuery; cache the stable prefix in a field (e.g. this.metadataPrefixCache) and have getMetadataPrefix return the cached value if present, setting the cache the first time you compute it; ensure the cache is invalidated by assigning null inside cleanup() and/or disconnect(). Also remove or gate the two diagnostic adapter.executeQuery calls at the start of DuckLakeService.extractSchema (the snapshot-check and schema-table-check) behind a debug flag or eliminate them to avoid the two extra roundtrips on every schema load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 973-975: The query builder is vuln to SQL injection because
snapshotId from DuckLakeQueryRequest is interpolated into query after
validateSingleStatement; before appending the FOR SYSTEM_TIME AS OF SNAPSHOT
clause, validate and sanitize snapshotId (the snapshotId variable) to ensure it
is a positive integer (e.g., reject non-digit chars or parse and check
Number.isInteger and >=0) and only then append it to the query string, or else
omit the clause/throw; do not directly interpolate unvalidated snapshotId into
query variable (used in the adapter method that mutates query).
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts`:
- Around line 769-778: The bug is that you append the time-travel clause to
baseQuery before removing trailing semicolons, leaving an embedded ';' that
breaks the clause; fix it by stripping trailing semicolons from baseQuery first
(use baseQuery.replace(/;\s*$/, '')) and assign that result to query, then, if
snapshotId is present, append " FOR SYSTEM_TIME AS OF SNAPSHOT '<snapshotId>'"
to query; update usage of the existing variables (baseQuery, query, snapshotId,
and the existing replace call) so the replace happens before building the
time-travel clause.
- Around line 785-818: The code currently appends LIMIT/OFFSET even for
non-SELECT statements (when limit && !hasExistingLimit), which causes invalid
SQL for DDL/DML; inside the same block where isSelectQuery is checked (and where
you run the count query using this.connectionInfo.connection.run and set
totalRows), move the lines that append `query += \` LIMIT ${limit}\`` and `query
+= \` OFFSET ${offset}\`` so they execute only when isSelectQuery is true; keep
the count query and totalRows logic intact and remove the LIMIT/OFFSET appends
from the outer if (limit && !hasExistingLimit) scope.
- Around line 770-773: The executeQuery() flow currently interpolates
DuckLakeQueryRequest.snapshotId (a string) directly into SQL; mirror
restoreSnapshot()'s validation: check snapshotId is non-empty, parse with
Number.parseInt(snapshotId, 10), and ensure Number.isNaN(parsed) is false before
appending FOR SYSTEM_TIME AS OF SNAPSHOT '...'; if validation fails, do not
interpolate the snapshot or throw/return a clear validation error. Apply this
change in the executeQuery implementations (postgresql.adapter.ts and other
adapters) so the snapshotId is numeric and safe before SQL interpolation.
In `@src/main/services/duckLake/adapters/sqlite.adapter.ts`:
- Around line 719-728: The trailing-semicolon removal is happening after
appending the snapshot clause which can leave a stray semicolon in the middle of
the SQL (see variables baseQuery and query in the sqlite adapter), so move the
strip step to run on baseQuery before any suffixes are appended; specifically,
perform query = baseQuery.replace(/;\s*$/, '') (or sanitize baseQuery) first,
then conditionally append the snapshot clause (`FOR SYSTEM_TIME AS OF SNAPSHOT
'${snapshotId}'`) and then proceed to add LIMIT/OFFSET and other suffixes.
In `@src/renderer/components/customTable/CustomTablePagination.tsx`:
- Around line 30-39: Replace the inline formatter closure inside
labelDisplayedRows with a module-scoped formatter that uses the runtime locale
(e.g., navigator.language or the app's active locale) instead of the hardcoded
'de-DE'; move the function named fmt out of labelDisplayedRows to the top-level
of the module and have labelDisplayedRows call that shared fmt, and update the
other occurrence in queryResult.tsx where the same hardcoded 'de-DE' pattern is
used so both use the runtime/app locale.
---
Outside diff comments:
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Line 27: Currently the main process adapter (duckdb.adapter.ts) imports
normalizeNumericValue from src/renderer, which breaks process isolation; move
the normalizeNumericValue implementation out of src/renderer/utils/fileUtils.ts
into a new shared module (e.g., src/shared/utils/numericUtils.ts) and export it,
then update duckdb.adapter.ts to import normalizeNumericValue from the new
shared path; also update the renderer code to import the function from
src/shared/utils/numericUtils.ts and remove the renderer-only import to ensure
no renderer APIs are referenced from the main process.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts`:
- Around line 294-376: The updateRows, deleteRows, and upsertRows
implementations currently call SELECT changes() to compute rowsAffected, but
DuckLake does not support DuckDB's changes() so the count is unreliable; update
each method (updateRows, deleteRows, upsertRows) to obtain affected row counts
from the DML result instead: either require/append a RETURNING * clause to the
provided upsertQuery/updateQuery/deleteQuery and use the returned
result.getRows().length, or read a driver-provided affectedRows/rowCount
property from the object returned by connectionInfo.connection.run (and fall
back to 0 if neither is available); ensure you replace the SELECT changes() call
and related parsing of rows with this new logic in each method.
In `@src/main/services/duckLake/adapters/sqlite.adapter.ts`:
- Around line 274-356: The DML methods updateRows, deleteRows and upsertRows
currently call 'SELECT changes()' which DuckLake doesn't support; change each
method to execute the provided DML with a 'RETURNING *' clause (or detect/append
RETURNING if already absent) via
this.connectionInfo.connection.run(upsertQuery|updateQuery|deleteQuery), then
call result.getRows() and use its length as rowsAffected (i.e. rowsAffected =
rows.length), removing the SELECT changes() logic and related hugeint parsing
and fallbacks; ensure you still throw when !this.connectionInfo and preserve
existing error logging.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 408-418: handleRefreshSchema currently calls
fetchSchemaForConnection but that path early-returns for DuckLake connections so
duckLakeSchema/duckLakeCompletions stay stale; add a refreshKey state (e.g.,
const [duckLakeRefreshKey, setDuckLakeRefreshKey] = useState(0)) and include it
in loadDuckLakeCompletions' dependency array alongside connectionInput so it
re-runs when refreshKey changes, then in handleRefreshSchema increment this key
(setDuckLakeRefreshKey(k=>k+1)) whenever isDuckLakeConnection is true; this
ensures loadDuckLakeCompletions will reload duckLakeSchema and
duckLakeCompletions on Refresh.
---
Duplicate comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1408-1419: The systemTables array returned by the function is
missing "ducklake_schema", but extractSchema queries ducklake_schema in three
places; add "ducklake_schema" to the systemTables array so it is exposed for
IntelliSense completions. Locate the systemTables literal (variable named
systemTables) in duckLake.service.ts and append "ducklake_schema" to the list
alongside the existing entries (e.g., 'ducklake_table', 'ducklake_column', ...),
ensuring the string matches the exact table name used in extractSchema.
- Around line 1314-1388: The query in the schemaNames.map block interpolates
schemaName directly into the SQL (the clause "WHERE s.schema_name =
'${schemaName}'"), which allows malformed SQL or injection; fix it by using a
parameterized query via adapter.executeQuery (pass schemaName as a query
parameter and replace the inline interpolation with the parameter placeholder
your adapter supports) or, if the adapter lacks parameter binding, explicitly
escape single quotes before interpolation (e.g., sanitize schemaName with
schemaName.replace(/'/g, "''")) so that adapter.executeQuery and the query
string in this block no longer inject raw schemaName.
- Around line 1712-1743: The current detectCommandType uses ddlKeywords.some(kw
=> normalized.includes(kw)) which false-positive matches DDL keywords appearing
inside literals; update detectCommandType (and ddlKeywords) to only detect DDL
when the statement actually begins with those keywords (or with allowed leading
modifiers like IF NOT EXISTS) — e.g., replace includes checks with startsWith
checks or a start-anchored regex against normalized (trimmed/uppercased) so only
leading DDL statements (in detectCommandType) return 'DDL' and not queries
containing those phrases elsewhere.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts`:
- Around line 832-841: The mapping of query rows uses Object.entries which
breaks for array-form rows and the normalizeNumericValue helper currently
returns number | undefined which replaces all non-numeric columns with
undefined; to fix, in the rows.map block (the variable data and its mapping)
first guard Array.isArray(row) and handle array rows by mapping using the column
name array (or preserve index-to-column mapping used elsewhere) instead of
Object.entries, and for object rows iterate Object.keys/Object.entries only when
not an array; also change normalizeNumericValue so it returns the original value
for non-numeric inputs (i.e., keep strings, booleans, dates intact) rather than
undefined so VARCHAR/BOOLEAN/TIMESTAMP columns are preserved. Ensure references:
normalizeNumericValue, the rows.map callback that produces normalized, and any
column-name mapping logic are updated accordingly.
In `@src/main/services/duckLake/adapters/sqlite.adapter.ts`:
- Around line 782-791: The mapping currently applies normalizeNumericValue to
every cell and treats all rows as objects; update the rows.map block to handle
both array and object row formats: if Array.isArray(row) build normalized by
iterating the existing fields array (use fields[i].name or fields[i] to get the
column name) and mapping row[i] into that key, otherwise iterate
Object.entries(row) as before. Crucially, only call normalizeNumericValue when
the raw value is numeric-like (e.g., typeof value === 'number' or a string that
parses as a number); for all other types (string, boolean, Date, null/undefined)
keep the original value unchanged. Ensure you reference the same variables used
in this scope (rows, fields, normalizeNumericValue) so the produced normalized
objects keep correct keys and non-numeric data is preserved.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 660-672: The DuckLake MenuItem is using the wrong icon key;
replace the src reference inside the MenuItem (where
connectionIcons.images.duckdb is used) with connectionIcons.images.ducklake and
keep the same fallback (defaultIcon) so it matches the renderValue logic; update
the MenuItem image instantiation (the <img> inside MenuItem) to use
connectionIcons.images.ducklake || defaultIcon.
- Around line 753-763: The current usage of isLoading={!duckLakeSchema} causes
perpetual loading when DuckLakeService.extractSchema fails and duckLakeSchema is
set to null; update the logic to use an explicit loading/error state instead of
relying on duckLakeSchema truthiness: introduce or use an existing boolean like
duckLakeLoading and an error flag duckLakeError (set duckLakeLoading=true before
fetch, false in finally, and set duckLakeError when the catch runs), then pass
isLoading={duckLakeLoading} to SchemaTreeViewerWithSchema and surface
duckLakeError (e.g., render an error message or fallback UI when duckLakeError
is true) so failures don't show infinite loading; target symbols:
duckLakeSchema, duckLakeLoading, duckLakeError, SchemaTreeViewerWithSchema, and
handleRefreshSchema.
- Around line 216-285: loadDuckLakeCompletions leaves previous DuckLake UI state
visible while the new schema loads; before calling DuckLakeService.extractSchema
(inside loadDuckLakeCompletions) clear the UI state by calling
setDuckLakeCompletions([]) and setDuckLakeSchema(null) as soon as you detect a
DuckLake connection (or when connectionInput is falsy/non-ducklake), then
proceed with the async extractSchema and set state from
generateDuckLakeCompletions(schema) on success or clear again on error; update
references: loadDuckLakeCompletions, connectionInput,
DuckLakeService.extractSchema, generateDuckLakeCompletions,
setDuckLakeCompletions, setDuckLakeSchema.
In `@src/renderer/screens/sql/queryResult.tsx`:
- Around line 210-217: The CSV escaping in escapeCsvValue misses carriage
returns, so update the character-class check from /[,"\n]/ to include '\r'
(e.g., /[,"\r\n]/) and ensure the same character-class is used consistently
where CSV detection occurs inside the escapeCsvValue function so values
containing CR or CRLF are quoted and double-quoted correctly via the existing
replace logic in escapeCsvValue.
- Around line 193-205: The download handler handleDownloadJson currently calls
JSON.stringify(rows, null, 2) which will throw on BigInt/HUGEINT values; update
handleDownloadJson to pass a replacer to JSON.stringify that detects BigInt
values (and other non-serializable types) and converts them to a safe
serializable form (e.g., toString() or a typed wrapper) before creating the Blob
and object URL; keep the existing hasRows guard, URL revoke logic and
handleExportMenuClose call, but replace the direct JSON.stringify(rows, null, 2)
with JSON.stringify(rows, replacer, 2) where replacer handles BigInt
serialization.
- Around line 283-287: The catch block that currently only does
console.error('Parquet export error:', error) and calls handleExportMenuClose()
should also surface a user-facing error using react-toastify; update the catch
to call toast.error(...) with a clear message (including error.message or a
friendly fallback) so users see the failure, keep the existing console.error and
handleExportMenuClose() calls, and ensure react-toastify's toast is
imported/available in this module (use toast.error from react-toastify).
---
Nitpick comments:
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 874-904: getMetadataPrefix currently queries duckdb_databases() on
every call causing extra roundtrips because qualifyMetadataTables is invoked
inside executeQuery; cache the stable prefix in a field (e.g.
this.metadataPrefixCache) and have getMetadataPrefix return the cached value if
present, setting the cache the first time you compute it; ensure the cache is
invalidated by assigning null inside cleanup() and/or disconnect(). Also remove
or gate the two diagnostic adapter.executeQuery calls at the start of
DuckLakeService.extractSchema (the snapshot-check and schema-table-check) behind
a debug flag or eliminate them to avoid the two extra roundtrips on every schema
load.
In `@src/main/services/duckLake/adapters/sqlite.adapter.ts`:
- Around line 735-754: The current logic inside the limit handling block (when
limit && !hasExistingLimit) runs a costly COUNT(*) by building countQuery and
executing this.connectionInfo.connection.run(countQuery) for every paginated
fetch; change this so the COUNT is optional and/or cached: add and check an
includeTotalCount boolean flag on the request/caller context before
constructing/executing countQuery (only run the COUNT when includeTotalCount is
true), and/or implement a simple cache keyed by the normalized query + params to
store totalRows after the first COUNT and reuse it for subsequent offset/limit
requests to avoid re-running the COUNT; update references around isSelectQuery,
totalRows, and the countQuery execution accordingly.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 139-166: Remove the two debug-only useEffect hooks that only
console.log runtime state: the effect that logs loading state for activeTab
(references activeTab, isLoadingSchema, tabSchemas) and the effect that logs
duckLakeSchema changes (references duckLakeSchema). Delete these useEffect
blocks entirely from src/renderer/screens/sql/index.tsx and ensure you keep the
related state declarations (duckLakeCompletions, setDuckLakeCompletions,
duckLakeSchema, setDuckLakeSchema) intact so runtime logic is unchanged.
Comment on lines
+973
to
+975
| if (snapshotId) { | ||
| // Modify query to use specific snapshot | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${request.snapshotId}'`; | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; |
There was a problem hiding this comment.
SQL injection via unvalidated snapshotId
snapshotId arrives from DuckLakeQueryRequest (populated via IPC) and is interpolated directly into the query string. The service-level validateSingleStatement check runs on the base query before the adapter appends the snapshot clause, so it provides no protection here.
A malicious snapshotId such as 1'; DROP TABLE t; -- produces:
SELECT * FROM t FOR SYSTEM_TIME AS OF SNAPSHOT '1'; DROP TABLE t; --'DuckLake snapshot IDs are numeric integers; validate before interpolating:
🛡️ Proposed fix
if (snapshotId) {
+ if (!/^\d+$/.test(snapshotId)) {
+ throw new Error('Invalid snapshotId: must be a non-negative integer');
+ }
- query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`;
+ query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT ${snapshotId}`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 973 -
975, The query builder is vuln to SQL injection because snapshotId from
DuckLakeQueryRequest is interpolated into query after validateSingleStatement;
before appending the FOR SYSTEM_TIME AS OF SNAPSHOT clause, validate and
sanitize snapshotId (the snapshotId variable) to ensure it is a positive integer
(e.g., reject non-digit chars or parse and check Number.isInteger and >=0) and
only then append it to the query string, or else omit the clause/throw; do not
directly interpolate unvalidated snapshotId into query variable (used in the
adapter method that mutates query).
Comment on lines
+769
to
+778
| let query = baseQuery; | ||
| if (snapshotId) { | ||
| // Modify query to use specific snapshot | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${request.snapshotId}'`; | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; | ||
| } | ||
|
|
||
| // Add limit and offset if specified | ||
| if (request.limit) { | ||
| query += ` LIMIT ${request.limit}`; | ||
| if (request.offset) { | ||
| query += ` OFFSET ${request.offset}`; | ||
| let totalRows: number | undefined; | ||
|
|
||
| // Strip trailing semicolons so LIMIT/OFFSET can be safely appended. | ||
| query = query.replace(/;\s*$/, ''); |
There was a problem hiding this comment.
Semicolon must be stripped from baseQuery before appending the time-travel clause.
When baseQuery ends with a semicolon (e.g., "SELECT * FROM foo;") and snapshotId is set, the result after line 772 is "SELECT * FROM foo; FOR SYSTEM_TIME AS OF SNAPSHOT '5'". The replace(/;\s*$/, '') at line 778 only strips a trailing semicolon, so the embedded ; before the time-travel clause is left intact. DuckDB will either execute only the first statement (silently ignoring the time-travel instruction) or throw a parse error — either way the snapshot filtering is broken.
🐛 Proposed fix — strip semicolons first
- let query = baseQuery;
- if (snapshotId) {
- // Modify query to use specific snapshot
- query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`;
- }
-
- let totalRows: number | undefined;
-
- // Strip trailing semicolons so LIMIT/OFFSET can be safely appended.
- query = query.replace(/;\s*$/, '');
+ // Strip trailing semicolons first so time-travel and LIMIT/OFFSET can be safely appended.
+ let query = baseQuery.replace(/;\s*$/, '');
+ if (snapshotId) {
+ // Modify query to use specific snapshot
+ query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`;
+ }
+
+ let totalRows: number | undefined;📝 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
| let query = baseQuery; | |
| if (snapshotId) { | |
| // Modify query to use specific snapshot | |
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${request.snapshotId}'`; | |
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; | |
| } | |
| // Add limit and offset if specified | |
| if (request.limit) { | |
| query += ` LIMIT ${request.limit}`; | |
| if (request.offset) { | |
| query += ` OFFSET ${request.offset}`; | |
| let totalRows: number | undefined; | |
| // Strip trailing semicolons so LIMIT/OFFSET can be safely appended. | |
| query = query.replace(/;\s*$/, ''); | |
| // Strip trailing semicolons first so time-travel and LIMIT/OFFSET can be safely appended. | |
| let query = baseQuery.replace(/;\s*$/, ''); | |
| if (snapshotId) { | |
| // Modify query to use specific snapshot | |
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; | |
| } | |
| let totalRows: number | undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts` around lines 769 -
778, The bug is that you append the time-travel clause to baseQuery before
removing trailing semicolons, leaving an embedded ';' that breaks the clause;
fix it by stripping trailing semicolons from baseQuery first (use
baseQuery.replace(/;\s*$/, '')) and assign that result to query, then, if
snapshotId is present, append " FOR SYSTEM_TIME AS OF SNAPSHOT '<snapshotId>'"
to query; update usage of the existing variables (baseQuery, query, snapshotId,
and the existing replace call) so the replace happens before building the
time-travel clause.
Comment on lines
+770
to
773
| if (snapshotId) { | ||
| // Modify query to use specific snapshot | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${request.snapshotId}'`; | ||
| query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the function containing lines 770-773 and understand the snapshotId type
cd src/main/services/duckLake/adapters && sed -n '750,790p' postgresql.adapter.tsRepository: rosettadb/dbt-studio
Length of output: 1422
🏁 Script executed:
# Search for where snapshotId is defined/used in the codebase
rg -n "snapshotId" --type ts -B3 -A3 | head -100Repository: rosettadb/dbt-studio
Length of output: 8366
🏁 Script executed:
# Search for the DuckLakeQueryRequest interface to find snapshotId definition
rg -n "DuckLakeQueryRequest|snapshotId" --type ts -B2 -A2Repository: rosettadb/dbt-studio
Length of output: 28761
Validate snapshotId as numeric before SQL interpolation in executeQuery() methods.
In DuckLakeQueryRequest, snapshotId is typed as a string (optional). The executeQuery() methods in all adapters (postgresql, duckdb, sqlite) directly interpolate this unvalidated string into SQL without casting or validation. Meanwhile, restoreSnapshot() in the same codebase validates and parses snapshotId as a number before use, confirming this pattern is necessary. A non-numeric or malformed value can break the query or alter its semantics. Apply the same validation from restoreSnapshot() (check emptiness, parse with Number.parseInt(), validate with Number.isNaN()) in the executeQuery() method before SQL interpolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/duckLake/adapters/postgresql.adapter.ts` around lines 770 -
773, The executeQuery() flow currently interpolates
DuckLakeQueryRequest.snapshotId (a string) directly into SQL; mirror
restoreSnapshot()'s validation: check snapshotId is non-empty, parse with
Number.parseInt(snapshotId, 10), and ensure Number.isNaN(parsed) is false before
appending FOR SYSTEM_TIME AS OF SNAPSHOT '...'; if validation fails, do not
interpolate the snapshot or throw/return a clear validation error. Apply this
change in the executeQuery implementations (postgresql.adapter.ts and other
adapters) so the snapshotId is numeric and safe before SQL interpolation.
… 10, and enable schema level display in the schema browser.
- Remove debug console logs across duckLake service, adapter, and SQL screen - Escape single quotes in schema names to prevent SQL injection in queries - Add recursive sanitization to convert BigInt values to Number in schema data - Add warnings and errors for presence of BigInt values during DuckDB adapter data processing - Skip regular schema loading for DuckLake connections to rely on custom extraction - Normalize numeric values including BigInt and hugeint objects in file utilities - Remove redundant console logs from SQL screen regarding DuckLake schema and completions state
…endering - Add sanitizeValue function to recursively convert BigInt to Number for IPC serialization - Remove unused snapshot and schema checks in adapter query - Add debug warnings for BigInt detection in raw DuckDB adapter rows - Log errors if BigInt remain after normalization in DuckDB adapter - Update QueryResult component to render null/undefined as italicized "NULL" - Improve cell rendering by stripping quotes from stringified JSON values
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/screens/sql/index.tsx (1)
340-350:⚠️ Potential issue | 🟠 Major
handleRefreshSchemais a complete no-op for DuckLake connections — schema never reloads.
handleRefreshSchemacallsfetchSchemaForConnection(activeTab.connectionId). For anyducklake-*id,fetchSchemaForConnection(lines 229–236) immediately sets an empty schema and returns without fetching anything. TheloadDuckLakeCompletionseffect only re-runs whenconnectionInputchanges, which never happens when Refresh is clicked.The net result: after creating a new DuckLake table, clicking Refresh leaves the schema tree stale with no way to reload it short of switching connections.
🐛 Proposed fix — extract `loadDuckLakeCompletions` into a callable `useCallback`
- // Load DuckLake completions when connection changes - useEffect(() => { - const loadDuckLakeCompletions = async () => { - if (!connectionInput || (connectionInput as any).type !== 'ducklake') { - setDuckLakeCompletions([]); - return; - } - try { - const { instanceId } = connectionInput as any; - if (!instanceId) return; - const schema = await DuckLakeService.extractSchema(instanceId); - setDuckLakeCompletions(generateDuckLakeCompletions(schema)); - setDuckLakeSchema(schema); - } catch (error) { - console.error('[SQL Screen] Failed to load DuckLake completions:', error); - setDuckLakeCompletions([]); - setDuckLakeSchema(null); - } - }; - loadDuckLakeCompletions(); - }, [connectionInput]); + const loadDuckLakeCompletions = useCallback(async () => { + if (!connectionInput || (connectionInput as any).type !== 'ducklake') { + setDuckLakeCompletions([]); + return; + } + try { + const { instanceId } = connectionInput as any; + if (!instanceId) return; + setDuckLakeCompletions([]); + setDuckLakeSchema(null); + const schema = await DuckLakeService.extractSchema(instanceId); + setDuckLakeCompletions(generateDuckLakeCompletions(schema)); + setDuckLakeSchema(schema); + } catch (error) { + console.error('[SQL Screen] Failed to load DuckLake completions:', error); + setDuckLakeCompletions([]); + setDuckLakeSchema(null); + } + }, [connectionInput]); + + useEffect(() => { + loadDuckLakeCompletions(); + }, [loadDuckLakeCompletions]); const handleRefreshSchema = useCallback(() => { if (activeTab) { + if (activeTab.connectionId.startsWith('ducklake-')) { + loadDuckLakeCompletions(); + return; + } setTabSchemas((prev) => { const updated = { ...prev }; delete updated[activeTab.connectionId]; return updated; }); fetchSchemaForConnection(activeTab.connectionId); } - }, [activeTab, fetchSchemaForConnection]); + }, [activeTab, fetchSchemaForConnection, loadDuckLakeCompletions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 340 - 350, The refresh currently does nothing for DuckLake because fetchSchemaForConnection early-returns for "ducklake-*" ids; refactor the effect logic by extracting loadDuckLakeCompletions into a standalone useCallback (preserving its internal behavior and dependencies like connectionInput) and invoke that callback from fetchSchemaForConnection or directly from handleRefreshSchema when activeTab.connectionId startsWith("ducklake-"); update fetchSchemaForConnection to call the new loadDuckLakeCompletions callback instead of early-returning, or have handleRefreshSchema call loadDuckLakeCompletions for ducklake connections so Refresh triggers a real reload of schema/completions.
🧹 Nitpick comments (3)
src/main/services/duckLake/adapters/duckdb.adapter.ts (3)
583-726: Debug logging left active in production code pathsSeveral new
console.logcalls remain acrosslistTables(),qualifyMetadataTables(), andexecuteQuery(). In particular:
- Lines 946–948 log the full qualified SQL on every user query.
- Lines 1050–1073 and 1117–1129 iterate over result rows on every query result to scan for
BigInt.These should be removed or gated behind a debug flag before this lands on
dev.Also applies to: 946-948, 1050-1129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 583 - 726, Remove or gate behind a debug flag all production console logging introduced in listTables(), qualifyMetadataTables(), and executeQuery(): specifically eliminate the per-request full SQL log in executeQuery() (the console.log that prints the qualified SQL), and stop iterating/printing rows on every result (the loops in executeQuery()/qualifyMetadataTables() that scan for BigInt and the console.log blocks in listTables() that dump databases, queries, and mapped tables). Replace them with either no-op production paths or conditional logging using an existing logger/debug flag (e.g., a this.debug or process.env.DEBUG check) so that listTables(), qualifyMetadataTables(), and executeQuery() only emit these detailed logs when debugging is explicitly enabled.
874-912:getMetadataPrefix()issues a DB round-trip on everyexecuteQuery()call — cache it
qualifyMetadataTables(line 912) callsgetMetadataPrefix(), which queriesduckdb_databases()every timeexecuteQuery()runs. The metadata database name is stable for the lifetime of a connection; hitting the DB to resolve it on every user query adds unnecessary latency.♻️ Proposed fix: cache as instance property
+ private _metadataPrefixCache: string | undefined; private async getMetadataPrefix(): Promise<string> { + if (this._metadataPrefixCache !== undefined) { + return this._metadataPrefixCache; + } try { if (!this.connectionInfo) { - return ''; + return (this._metadataPrefixCache = ''); } // ... existing query ... - return `"${metadataDatabase}".main.`; + return (this._metadataPrefixCache = `"${metadataDatabase}".main.`); } catch (error) { console.warn('Could not determine metadata prefix:', error); - return ''; + // Don't cache errors — allow retry on next call + return ''; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 874 - 912, getMetadataPrefix() currently queries duckdb_databases() on every call (via qualifyMetadataTables -> executeQuery) causing unnecessary DB round-trips; make it cache the resolved metadata prefix as an instance property (e.g., this.cachedMetadataPrefix) and return that if present, otherwise compute it once, store it, and return it; ensure you set/clear the cache when the connection changes or is closed (reference methods/properties getMetadataPrefix, qualifyMetadataTables, and connectionInfo) so the cached value stays valid for the lifetime of the connection.
937-944: Static analysis: dynamicRegExpfrom variable (CWE-1333) — false positive, but easy to eliminate
metadataTablesis a static, hardcoded array so there is no real ReDoS risk. The lint warning can be silenced by pre-compiling the patterns at class or module level instead of reconstructing them on every call.♻️ Proposed fix: pre-compile patterns
+ // Pre-compiled at class level — all names are static identifiers + private static readonly METADATA_TABLE_PATTERNS: [RegExp, string][] = [ + 'ducklake_table', + 'ducklake_column', + // ... full list ... + ].map((table) => [ + new RegExp( + `\\b(FROM|JOIN)\\s+(?!"?__ducklake_metadata_)\\b${table}\\b`, + 'gi', + ), + table, + ]); private async qualifyMetadataTables(query: string): Promise<string> { const metadataPrefix = await this.getMetadataPrefix(); if (!metadataPrefix) return query; - const metadataTables = [ ... ]; - const qualifiedQuery = metadataTables.reduce((currentQuery, table) => { - const pattern = new RegExp(...); - return currentQuery.replace(pattern, `$1 ${metadataPrefix}${table}`); - }, query); + const qualifiedQuery = DuckDBCatalogAdapter.METADATA_TABLE_PATTERNS.reduce( + (currentQuery, [pattern, table]) => + currentQuery.replace(pattern, `$1 ${metadataPrefix}${table}`), + query, + ); return qualifiedQuery; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 937 - 944, The code in qualifiedQuery creates a new dynamic RegExp for each table on every call, triggering a lint/analysis warning; precompile the patterns once and reuse them to silence the warning. Create a module- or class-level array of RegExp objects (e.g., METADATA_TABLE_PATTERNS) built from the static metadataTables and the same pattern semantics and flags ('gi'), then update the reduce in qualifiedQuery to iterate over these precompiled RegExp instances (using metadataPrefix and table names as needed) instead of constructing new RegExp objects at runtime. Ensure the precompiled patterns preserve the original word-boundary and (FROM|JOIN) logic and skip already-qualified names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1153-1170: The validation currently blocks COPY commands because
'COPY' is missing from allowedPrefixes in validateSingleStatement; add 'COPY' to
the allowedPrefixes array used in validateSingleStatement so COPY (...) TO ...
(FORMAT PARQUET) is accepted, and also update detectCommandType to recognize
'COPY' and map it to the 'DDL' command type (so responses don't get isCommand:
true or are misclassified as SELECT); update the arrays/branch that lists SQL
verbs in validateSingleStatement and the logic in detectCommandType (referencing
validateSingleStatement and detectCommandType) to include 'COPY'.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 919-933: The metadataTables list in duckdb.adapter.ts (the
metadataTables constant) is missing the 'ducklake_snapshot_changes' entry, so
add 'ducklake_snapshot_changes' to the array of qualified metadata tables in the
metadataTables constant (alongside 'ducklake_snapshot' and others) so
qualifyMetadataTables will properly recognize and qualify queries referencing
ducklake_snapshot_changes.
- Around line 1022-1030: The SQL builder interpolates limit and offset directly
(see the uses of `query += \` LIMIT ${limit}\`` and `query += \` OFFSET
${offset}\``) so validate and coerce those IPC-sourced values to safe integers
before concatenation: parse them (e.g. via Number/parseInt), ensure
Number.isInteger and non-negative (or clamp to sensible defaults), and reject or
fallback on invalid inputs; alternatively use parameterized query placeholders
if supported. Update the code that constructs the query (the branch that appends
LIMIT/OFFSET) to use the validated/coerced values or throw/return an error when
validation fails.
In `@src/renderer/components/sqlEditor/index.tsx`:
- Around line 138-141: mappedFields currently converts any non-numeric duckLake
field.type to 0, losing type information; implement a mapDuckLakeTypeName(name:
string): number (or use existing TypeId enum) that maps known DuckLake type
strings like 'VARCHAR', 'INTEGER', 'TIMESTAMP', 'BOOLEAN', etc. to the app's
numeric TypeId values and use it in the mapping expression for
duckLakeResult.fields (replace the typeof field.type check) so string type names
are translated to the correct numeric type ids instead of defaulting to 0;
ensure mappedFields uses mapDuckLakeTypeName(field.type as string) when
field.type is a string and preserves numeric values when already a number.
In `@src/renderer/screens/sql/queryResult.tsx`:
- Around line 446-479: The current cell renderer in the render function uses
JSON.stringify(cellValue).replace(/"/g, '') which corrupts values containing
quotes and can throw on BigInt; change the conversion for display by avoiding
blanket quote-stripping: for primitives use String(cellValue) (no-op for
strings, natural for numbers/booleans) and for objects/arrays use
JSON.stringify(cellValue) inside a try/catch that falls back to
String(cellValue) if stringify throws (e.g., BigInt or circular refs); update
the stringValue assignment (the variable named stringValue and the cellValue
reference) to implement this safe conversion.
- Around line 108-117: fetchPage currently clears the table on any fetch error
and allows out-of-order concurrent fetches to overwrite newer pages; fix by
marking in-flight requests as stale at the start of fetchPage (set
fetchCancelRef.current = true then create a new token/sequence id e.g.,
increment fetchSeqRef.current) so only responses matching the latest sequence
update state, and in the catch block do not call setRows([]) unconditionally—set
an explicit error state (e.g., setFetchError or setError) so the UI can show an
error and retry button, and ensure each successful response checks the cancel
token/sequence before calling setRows and setLoading(false).
---
Outside diff comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 340-350: The refresh currently does nothing for DuckLake because
fetchSchemaForConnection early-returns for "ducklake-*" ids; refactor the effect
logic by extracting loadDuckLakeCompletions into a standalone useCallback
(preserving its internal behavior and dependencies like connectionInput) and
invoke that callback from fetchSchemaForConnection or directly from
handleRefreshSchema when activeTab.connectionId startsWith("ducklake-"); update
fetchSchemaForConnection to call the new loadDuckLakeCompletions callback
instead of early-returning, or have handleRefreshSchema call
loadDuckLakeCompletions for ducklake connections so Refresh triggers a real
reload of schema/completions.
---
Duplicate comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1680-1711: The detectCommandType function is producing false
positives because it checks ddlKeywords against the raw query string which can
match keywords inside quoted string literals; update detectCommandType to first
remove/neutralize all string literals (single-quoted, double-quoted, and
backtick) and escaped quotes before converting to uppercase and checking
ddlKeywords/DML; operate on the sanitizedQuery in the existing function
(reference detectCommandType and ddlKeywords) so includes/startsWith checks only
consider SQL tokens outside string literals.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 967-969: The current interpolation of snapshotId into query (in
the DuckDB adapter code that mutates the local variable query when handling
DuckLakeQueryRequest) allows SQL injection; validate that snapshotId is a
numeric non-negative integer (parse and check Number.isInteger && >= 0) before
using it, and if valid append the clause without quotes as FOR SYSTEM_TIME AS OF
SNAPSHOT ${snapshotId}; if invalid, omit the clause or throw a controlled error.
Locate the block referencing snapshotId and query in duckdb.adapter.ts and
replace the direct string interpolation with the numeric validation plus
unquoted insertion (or safe rejection) to prevent injection.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 592-604: The MenuItem rendering in the SQL screen is using
connectionIcons.images.duckdb for DuckLake entries, causing the wrong icon to
display; update the icon lookup in the MenuItem that renders instance.name to
select the correct key (e.g., connectionIcons.images.ducklake || defaultIcon) or
derive the key from the instance (e.g., connectionIcons.images[instance.type] ||
defaultIcon) so DuckLake instances use the ducklake icon; adjust the img src
reference in that MenuItem accordingly.
- Around line 190-222: When switching DuckLake instances the async
loadDuckLakeCompletions can finish after a newer request, causing stale
schema/completions to be set; modify loadDuckLakeCompletions inside the
useEffect to capture the current instanceId (const current = (connectionInput as
any)?.instanceId) or create an AbortController/token and check it before calling
setDuckLakeCompletions and setDuckLakeSchema, and ignore results if the
instanceId/token no longer matches the latest connectionInput; keep using
DuckLakeService.extractSchema and generateDuckLakeCompletions but only update
state when the response corresponds to the most recent instanceId.
- Around line 685-694: The current isLoading prop uses {!duckLakeSchema}, which
stays true on extraction failure and causes infinite loading; change the loading
control to a dedicated fetching/error-aware flag (e.g.,
isFetchingDuckLakeSchema) or explicit schema state checks so loading is true
only while a fetch is in progress and false on either success or failure. Update
the code that populates duckLakeSchema (where duckLakeTables/duckLakeSchema are
managed) to set isFetchingDuckLakeSchema = true when starting the request, set
duckLakeSchema on success and isFetchingDuckLakeSchema = false, and set
isFetchingDuckLakeSchema = false plus duckLakeError on failure; then pass
isLoading={isFetchingDuckLakeSchema} (and optionally pass error) into
SchemaTreeViewerWithSchema instead of isLoading={!duckLakeSchema}.
- Around line 858-871: The exportContext currently passes originalSql as
activeTab.query (live editor content) which is incorrect; change QueryResult's
exportContext.originalSql to prefer the executed SQL stored with the results
((activeTab.results as any)?.originalSql) and fall back to a dedicated
last-executed field on the tab (e.g., activeTab.lastExecutedQuery) before using
activeTab.query; also ensure the query execution path (the function that
runs/executes queries, e.g., runQuery/executeQuery/handleRunQuery) sets
activeTab.lastExecutedQuery when a query completes so the fallback has the
correct value.
In `@src/renderer/screens/sql/queryResult.tsx`:
- Around line 210-217: The CSV escaping logic in escapeCsvValue doesn't account
for carriage returns, so values with '\r' won't be quoted; update the regex
check in escapeCsvValue (function escapeCsvValue) to include '\r' (e.g.
/[,"\n\r]/) so strings containing CR are quoted, and keep the existing
double-quote replacement behavior intact (str.replace(/"/g, '""')).
- Around line 193-205: handleDownloadJson currently uses JSON.stringify(rows,
null, 2) which throws if rows contain BigInt; update handleDownloadJson to
serialize BigInt safely by providing a replacer that converts BigInt values to
strings (or another safe representation) before creating the Blob, ensuring rows
(used in handleDownloadJson) are passed through that replacer when calling
JSON.stringify and then continue to createObjectURL, set a.download, click,
revokeObjectURL, and call handleExportMenuClose as before.
- Around line 283-287: The catch block for Parquet export currently only logs to
console and closes the export menu (console.error('Parquet export error:',
error); handleExportMenuClose()), leaving the user uninformed; update this block
to both log the full error and surface a user-facing error notification using
the app's notification mechanism (e.g., enqueueSnackbar/showToast/showError)
including error.message or serialized error details, and then call
handleExportMenuClose() as before so the menu still closes.
---
Nitpick comments:
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 583-726: Remove or gate behind a debug flag all production console
logging introduced in listTables(), qualifyMetadataTables(), and executeQuery():
specifically eliminate the per-request full SQL log in executeQuery() (the
console.log that prints the qualified SQL), and stop iterating/printing rows on
every result (the loops in executeQuery()/qualifyMetadataTables() that scan for
BigInt and the console.log blocks in listTables() that dump databases, queries,
and mapped tables). Replace them with either no-op production paths or
conditional logging using an existing logger/debug flag (e.g., a this.debug or
process.env.DEBUG check) so that listTables(), qualifyMetadataTables(), and
executeQuery() only emit these detailed logs when debugging is explicitly
enabled.
- Around line 874-912: getMetadataPrefix() currently queries duckdb_databases()
on every call (via qualifyMetadataTables -> executeQuery) causing unnecessary DB
round-trips; make it cache the resolved metadata prefix as an instance property
(e.g., this.cachedMetadataPrefix) and return that if present, otherwise compute
it once, store it, and return it; ensure you set/clear the cache when the
connection changes or is closed (reference methods/properties getMetadataPrefix,
qualifyMetadataTables, and connectionInfo) so the cached value stays valid for
the lifetime of the connection.
- Around line 937-944: The code in qualifiedQuery creates a new dynamic RegExp
for each table on every call, triggering a lint/analysis warning; precompile the
patterns once and reuse them to silence the warning. Create a module- or
class-level array of RegExp objects (e.g., METADATA_TABLE_PATTERNS) built from
the static metadataTables and the same pattern semantics and flags ('gi'), then
update the reduce in qualifiedQuery to iterate over these precompiled RegExp
instances (using metadataPrefix and table names as needed) instead of
constructing new RegExp objects at runtime. Ensure the precompiled patterns
preserve the original word-boundary and (FROM|JOIN) logic and skip
already-qualified names.
- Add isLoading state for DuckLake instances to manage loading status - Return a minimal connection object with instanceId when instance is not found - Prevent "Connection is still loading" errors when navigating back to SQL screen - Update dependencies of useMemo hook to include loading state of DuckLake instances
Nuri1977
changed the title
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)
src/renderer/screens/sql/index.tsx (1)
357-367:⚠️ Potential issue | 🟠 MajorRefresh button does nothing for DuckLake connections.
handleRefreshSchemacallsfetchSchemaForConnection, which short-circuits immediately forducklake-IDs (lines 248–252) and just writes an empty schema — it never re-fetches.duckLakeSchemaandduckLakeCompletionsare not cleared, and theloadDuckLakeCompletionseffect won't re-fire becauseconnectionInputis unchanged.The fix requires either triggering the effect dependency to change (e.g., an explicit
refreshKeycounter) or callingDuckLakeService.extractSchemadirectly fromhandleRefreshSchemawhenisDuckLakeConnection.🐛 Proposed fix — add a refresh trigger
+ const [duckLakeSchemaRefreshKey, setDuckLakeSchemaRefreshKey] = useState(0); // Load DuckLake completions when connection changes useEffect(() => { ... - }, [connectionInput]); + }, [connectionInput, duckLakeSchemaRefreshKey]); const handleRefreshSchema = useCallback(() => { if (activeTab) { + if (isDuckLakeConnection) { + setDuckLakeSchema(null); + setDuckLakeCompletions([]); + setDuckLakeSchemaRefreshKey((k) => k + 1); + return; + } setTabSchemas((prev) => { const updated = { ...prev }; delete updated[activeTab.connectionId]; return updated; }); fetchSchemaForConnection(activeTab.connectionId); } - }, [activeTab, fetchSchemaForConnection]); + }, [activeTab, isDuckLakeConnection, fetchSchemaForConnection]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 357 - 367, The refresh button does nothing for DuckLake connections because fetchSchemaForConnection short-circuits for ducklake- IDs and only writes an empty schema; to fix, update handleRefreshSchema to either (a) when isDuckLakeConnection(activeTab.connectionId) call DuckLakeService.extractSchema(activeTab.connectionId, connectionInput) directly and then clear/set duckLakeSchema and duckLakeCompletions accordingly, or (b) add a refreshKey counter state and increment it in handleRefreshSchema so loadDuckLakeCompletions (which depends on connectionInput) also includes refreshKey and will re-run; modify handleRefreshSchema to detect ducklake via isDuckLakeConnection and perform one of these two approaches, and ensure tabSchemas, duckLakeSchema, and duckLakeCompletions are cleared/updated so the UI reflects the refreshed schema.
♻️ Duplicate comments (4)
src/renderer/screens/sql/index.tsx (4)
609-621: DuckLakeMenuItemstill uses theduckdbicon — unresolved from previous review.
connectionIcons.images.duckdbshould beconnectionIcons.images.ducklaketo be consistent withrenderValue(line 444) and to correctly identify DuckLake instances in the dropdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 609 - 621, The dropdown MenuItem for DuckLake is still referencing connectionIcons.images.duckdb; update the JSX inside the MenuItem (the img src usage) to use connectionIcons.images.ducklake instead so it matches the renderValue logic and correctly shows DuckLake icons (search for the MenuItem block that renders instance.name and the img tag to change duckdb → ducklake).
207-239: Stale schema still possible when switching DuckLake instances — race condition unresolved.The async
loadDuckLakeCompletionshas no cancellation token or post-awaitinstanceId guard. A slow response from the previously active instance can overwrite the schema of the newly selected one.This was raised in a previous review; the fix (capture
instanceIdbefore theawait, then verify it still matches(connectionInput as any).instanceIdafter theawait) is still pending.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 207 - 239, The async loader in useEffect (loadDuckLakeCompletions) can race between instance switches; capture the current instanceId into a local variable before calling await DuckLakeService.extractSchema, then after the await verify that (connectionInput as any).instanceId still equals that captured instanceId and bail early if not; only call setDuckLakeCompletions and setDuckLakeSchema when the instanceId matches. Apply this guard around the await-returning call in loadDuckLakeCompletions to prevent stale schema from overwriting the current one.
875-888:originalSqlfallback to live editor content still unresolved.
(activeTab.results as any)?.originalSqlis a useful first-try, but the fallbackactiveTab.queryreflects the current (possibly edited) editor content, not the query that produced the visible result set. The recommended fix of persisting alastExecutedQueryin tab state was raised in a previous review and is still pending.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 875 - 888, The fallback for originalSql must use the query that produced the result set, not the live editor content: add a lastExecutedQuery property to the tab state type and ensure the query runner (e.g., the function that runs/executed queries such as executeQuery/runQuery in the tab logic) saves the executed SQL into activeTab.lastExecutedQuery when the result is produced; then update the QueryResult exportContext to use (activeTab.results?.originalSql) ?? activeTab.lastExecutedQuery ?? activeTab.query so the persisted lastExecutedQuery is preferred over the live editor content.
702-711:isLoading={!duckLakeSchema}still causes an unrecoverable loading state on schema failure.When
DuckLakeService.extractSchemathrows, the catch setsduckLakeSchema(null), keeping!duckLakeSchemapermanentlytrue. A separateisDuckLakeSchemaLoadingboolean (set/cleared in try/finally) is needed to decouple the loading indicator from the data value.This was raised in a previous review and remains unresolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 702 - 711, The SchemaTreeViewerWithSchema is currently driven by isLoading={!duckLakeSchema}, which leaves the UI stuck loading when DuckLakeService.extractSchema fails and duckLakeSchema is set to null; introduce a separate boolean state (e.g. isDuckLakeSchemaLoading) in the component, set it true before calling DuckLakeService.extractSchema in handleRefreshSchema (or wherever extraction happens), clear it in a finally block regardless of success/failure, and change the JSX to pass isLoading={isDuckLakeSchemaLoading} while keeping duckLakeSchema null on error so error UI or retry logic can render; update any related logic that assumed !duckLakeSchema as the loading signal.
🧹 Nitpick comments (1)
src/renderer/screens/sql/index.tsx (1)
142-147:isLoadingDuckLakeInstancesis a redundantuseMemodependency.React Query always updates the
duckLakeInstancesreference when loading finishes, so includingisLoadingDuckLakeInstancesin the deps array triggers an unnecessary extra evaluation without adding correctness value.♻️ Proposed cleanup
- }, [ - activeConnection, - activeTab, - duckLakeInstances, - isLoadingDuckLakeInstances, - ]); + }, [activeConnection, activeTab, duckLakeInstances]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 142 - 147, The dependency array for the useMemo that includes activeConnection, activeTab, duckLakeInstances, and isLoadingDuckLakeInstances is redundant because React Query updates the duckLakeInstances reference when loading finishes; remove isLoadingDuckLakeInstances from the deps array so the memo only depends on activeConnection, activeTab, and duckLakeInstances (locate the useMemo call in src/renderer/screens/sql/index.tsx where those symbols are used and update its dependencies).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 128-134: The fallback loading object currently hardcodes
catalogType: 'duckdb' which can be wrong for non-DuckDB instances; update the
loading return in the render/fallback path (the object creation that returns
type: 'ducklake', name: activeTab.connectionName..., instanceId, status:
'loading') to set catalogType to undefined (or omit it) while the real instance
is loading so consumers won't see an incorrect 'duckdb' value; keep all other
fields (type, name, instanceId, status) unchanged and ensure any downstream
checks handle undefined safely.
- Around line 427-433: The renderValue branch that handles
selected.startsWith('ducklake-') currently returns 'Select Connection' when no
matching instance is found; update it to use the current tab's connection name
instead: when instance is undefined, read activeTab.connectionName (guarding
that activeTab exists) and return that value if present, falling back to 'Select
Connection' only if connectionName is missing; reference variables/functions:
selected, duckLakeInstances, instanceId, instance, and activeTab in the
renderValue logic.
---
Outside diff comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 357-367: The refresh button does nothing for DuckLake connections
because fetchSchemaForConnection short-circuits for ducklake- IDs and only
writes an empty schema; to fix, update handleRefreshSchema to either (a) when
isDuckLakeConnection(activeTab.connectionId) call
DuckLakeService.extractSchema(activeTab.connectionId, connectionInput) directly
and then clear/set duckLakeSchema and duckLakeCompletions accordingly, or (b)
add a refreshKey counter state and increment it in handleRefreshSchema so
loadDuckLakeCompletions (which depends on connectionInput) also includes
refreshKey and will re-run; modify handleRefreshSchema to detect ducklake via
isDuckLakeConnection and perform one of these two approaches, and ensure
tabSchemas, duckLakeSchema, and duckLakeCompletions are cleared/updated so the
UI reflects the refreshed schema.
---
Duplicate comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 609-621: The dropdown MenuItem for DuckLake is still referencing
connectionIcons.images.duckdb; update the JSX inside the MenuItem (the img src
usage) to use connectionIcons.images.ducklake instead so it matches the
renderValue logic and correctly shows DuckLake icons (search for the MenuItem
block that renders instance.name and the img tag to change duckdb → ducklake).
- Around line 207-239: The async loader in useEffect (loadDuckLakeCompletions)
can race between instance switches; capture the current instanceId into a local
variable before calling await DuckLakeService.extractSchema, then after the
await verify that (connectionInput as any).instanceId still equals that captured
instanceId and bail early if not; only call setDuckLakeCompletions and
setDuckLakeSchema when the instanceId matches. Apply this guard around the
await-returning call in loadDuckLakeCompletions to prevent stale schema from
overwriting the current one.
- Around line 875-888: The fallback for originalSql must use the query that
produced the result set, not the live editor content: add a lastExecutedQuery
property to the tab state type and ensure the query runner (e.g., the function
that runs/executed queries such as executeQuery/runQuery in the tab logic) saves
the executed SQL into activeTab.lastExecutedQuery when the result is produced;
then update the QueryResult exportContext to use
(activeTab.results?.originalSql) ?? activeTab.lastExecutedQuery ??
activeTab.query so the persisted lastExecutedQuery is preferred over the live
editor content.
- Around line 702-711: The SchemaTreeViewerWithSchema is currently driven by
isLoading={!duckLakeSchema}, which leaves the UI stuck loading when
DuckLakeService.extractSchema fails and duckLakeSchema is set to null; introduce
a separate boolean state (e.g. isDuckLakeSchemaLoading) in the component, set it
true before calling DuckLakeService.extractSchema in handleRefreshSchema (or
wherever extraction happens), clear it in a finally block regardless of
success/failure, and change the JSX to pass isLoading={isDuckLakeSchemaLoading}
while keeping duckLakeSchema null on error so error UI or retry logic can
render; update any related logic that assumed !duckLakeSchema as the loading
signal.
---
Nitpick comments:
In `@src/renderer/screens/sql/index.tsx`:
- Around line 142-147: The dependency array for the useMemo that includes
activeConnection, activeTab, duckLakeInstances, and isLoadingDuckLakeInstances
is redundant because React Query updates the duckLakeInstances reference when
loading finishes; remove isLoadingDuckLakeInstances from the deps array so the
memo only depends on activeConnection, activeTab, and duckLakeInstances (locate
the useMemo call in src/renderer/screens/sql/index.tsx where those symbols are
used and update its dependencies).
- Added 'TRUNCATE' and 'RENAME' commands to supported DDL operations - Changed detection to check statement start only, avoiding false positives - Simplified DDL keyword list to command verbs only for accurate matching - Removed specific table/view/schema suffixes from keyword checks
- Ensure LIMIT and OFFSET are appended for queries without existing limit - Skip applying LIMIT for queries with existing limits or non-SELECT statements - Clarify handling of LIMIT clause for DML/DDL queries to avoid invalid syntax
…ling - Refactor isDDLOperation to use startsWith for precise DDL detection - Add error toast when no connection is selected in connection mode - Add info toast if DuckLake connection is still loading and block query run - Prevent error toast duplicate by refining loading state checks - Show error when no connection or project is selected outside connection mode - Limit DuckLake query results only for SELECT commands using getCommandType - Clean up redundant code and improve query execution flow for DuckLake connections
- Add refetch function for DuckLake instances in useHook response - Trigger refetch when active tab connection ID changes for DuckLake - Display connection not found message if DuckLake instance is loading but missing - Provide retry button to allow manual refetch of DuckLake instances - Style connection error UI with centered box and descriptive text
There was a problem hiding this comment.
♻️ Duplicate comments (10)
src/main/services/duckLake/adapters/duckdb.adapter.ts (3)
919-933:ducklake_snapshot_changesis still missing from the qualification list.This table is used in
listSnapshots()andlistInstanceSnapshots()methods. With Monaco IntelliSense now surfacing DuckLake metadata tables as completions, users writingSELECT * FROM ducklake_snapshot_changeswill get "table not found" errors because the table reference won't be qualified.🐛 Proposed fix
const metadataTables = [ 'ducklake_table', 'ducklake_column', 'ducklake_schema', 'ducklake_snapshot', + 'ducklake_snapshot_changes', 'ducklake_data_file', 'ducklake_delete_file', 'ducklake_table_stats',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 919 - 933, The metadataTables array in duckdb.adapter.ts is missing 'ducklake_snapshot_changes', causing unqualified references to fail in listSnapshots() and listInstanceSnapshots(); add 'ducklake_snapshot_changes' to the metadataTables array (alongside entries like 'ducklake_snapshot' and 'ducklake_table') so the adapter will recognize and qualify that table name in queries used by listSnapshots() and listInstanceSnapshots().
967-970: SQL injection via unvalidatedsnapshotIdremains unaddressed.
snapshotIdfromDuckLakeQueryRequestis interpolated directly into the query string without validation. A malicioussnapshotIdlike1'; DROP TABLE t; --would produce invalid or dangerous SQL.DuckLake snapshot IDs are numeric integers; validate before interpolating:
🛡️ Proposed fix
- if (snapshotId) { + if (snapshotId) { + if (!/^\d+$/.test(snapshotId)) { + throw new Error('Invalid snapshotId: must be a non-negative integer'); + } // Modify query to use specific snapshot - query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT '${snapshotId}'`; + query = `${query} FOR SYSTEM_TIME AS OF SNAPSHOT ${snapshotId}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 967 - 970, The code in duckdb.adapter.ts interpolates snapshotId from DuckLakeQueryRequest directly into SQL (the block that appends "FOR SYSTEM_TIME AS OF SNAPSHOT '...'" ), which allows SQL injection; validate and sanitize snapshotId before using it: ensure snapshotId is present and is a strict integer (e.g., parse/validate with Number.isInteger or a regex allowing only digits) and only then append it to the query (or better, use a parameterized mechanism if DuckDB client supports params) and avoid embedding any non-numeric characters; if validation fails, throw or ignore the snapshotId so no untrusted input is interpolated.
1022-1025:limitandoffsetare interpolated without runtime validation.Both values arrive via IPC from the renderer. TypeScript types are not enforced at runtime at the IPC boundary. A non-integer or crafted value would be interpolated directly into the query string.
🛡️ Proposed fix
+ // Validate pagination parameters — IPC payloads are not runtime-typed + const safeLimit = Number.isInteger(limit) && limit > 0 ? limit : undefined; + const safeOffset = Number.isInteger(offset) && offset >= 0 ? offset : 0; + if (isSelectQuery && !hasExistingLimit) { // ... count query ... - query += ` LIMIT ${limit}`; - if (offset) { - query += ` OFFSET ${offset}`; + query += ` LIMIT ${safeLimit}`; + if (safeOffset > 0) { + query += ` OFFSET ${safeOffset}`; } }Apply validation at the start of the pagination block before using
limitandoffset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 1022 - 1025, The pagination block in duckdb.adapter.ts interpolates limit and offset directly into the SQL string (variables `limit` and `offset`), so validate/coerce them before use: in the function that builds the query (the query construction / pagination block), parse `limit` and `offset` to integers (e.g., Number/parseInt), verify Number.isInteger and >= 0 (and apply a reasonable max cap), fallback to sensible defaults or reject/throw if invalid, and then use the validated values when appending `LIMIT` and `OFFSET`; alternatively switch to parameterized binding if the query builder/DB client supports it to avoid string interpolation. Ensure you update the same function/method that contains the lines `query += \` LIMIT ${limit}\`;` and `query += \` OFFSET ${offset}\`;`.src/main/services/duckLake.service.ts (1)
1153-1167: AddCOPYtoallowedPrefixesto enable DuckLake Parquet export.The Parquet export feature in
queryResult.tsxconstructsCOPY (SELECT ...) TO '/path/file.parquet' (FORMAT PARQUET)queries and invokes them via this method. WithoutCOPYinallowedPrefixes,validateSingleStatementrejects these queries, making the export feature non-functional for DuckLake connections.🔧 Proposed fix
const allowedPrefixes = [ 'SELECT', 'WITH', 'INSERT', 'UPDATE', 'DELETE', 'CREATE', 'DROP', 'ALTER', 'TRUNCATE', 'RENAME', 'DESCRIBE', 'PRAGMA', 'SHOW', + 'COPY', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake.service.ts` around lines 1153 - 1167, Add the SQL verb "COPY" to the allowedPrefixes array used by validateSingleStatement so DuckLake COPY ... TO ... (FORMAT PARQUET) export queries are accepted; locate the allowedPrefixes declaration in duckLake.service.ts and include 'COPY' alongside the other strings (e.g., 'SELECT','WITH',...), then run unit/manual validation for validateSingleStatement to ensure COPY statements now pass.src/renderer/screens/sql/index.tsx (5)
615-627: Duplicate: DuckLakeMenuItemusesduckdbicon instead ofducklake.Line 617 uses
connectionIcons.images.duckdbbut should useconnectionIcons.images.ducklaketo match therenderValuedisplay.🎨 Proposed fix
<img - src={connectionIcons.images.duckdb || defaultIcon} + src={connectionIcons.images.ducklake || defaultIcon} alt=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 615 - 627, The MenuItem rendering for the DuckLake instance is using the wrong icon key; in the JSX block inside the SQL screen (the MenuItem that displays {instance.name}) replace the use of connectionIcons.images.duckdb with connectionIcons.images.ducklake so the icon matches the renderValue/display for DuckLake; update the img src reference in that MenuItem to use ducklake instead of duckdb.
431-470: Duplicate:renderValueshows "Select Connection" while DuckLake instance is loading.This was flagged in a previous review. Consider using
activeTab?.connectionName || 'Loading...'wheninstanceis undefined instead of the generic fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 431 - 470, In the DuckLake branch inside the renderValue logic (the block that checks selected.startsWith('ducklake-') and resolves instance via duckLakeInstances.find), replace the generic fallback that returns 'Select Connection' when instance is undefined with a contextual fallback such as activeTab?.connectionName || 'Loading...' so the UI shows the current connectionName or a Loading... state while the DuckLake instance is still loading.
208-240: Duplicate: Stale schema/completions can briefly show when switching instances.This was flagged in a previous review. Consider clearing
duckLakeCompletionsandduckLakeSchemaat the start of the async operation to prevent showing stale data from a different instance while the new schema loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 208 - 240, When switching DuckLake instances the old schema/completions can briefly persist; inside the useEffect's async loader (loadDuckLakeCompletions) clear the UI state immediately before any awaits by calling setDuckLakeCompletions([]) and setDuckLakeSchema(null) as soon as you verify the input is a 'ducklake' connection (or even at start of the function), then proceed to call DuckLakeService.extractSchema and generateDuckLakeCompletions; optionally add a simple request token (e.g., local requestId checked before applying setDuckLakeCompletions/setDuckLakeSchema) to avoid applying results from an outdated async call.
129-135: Duplicate: HardcodedcatalogType: 'duckdb'in loading fallback.This was flagged in a previous review. While loading, the fallback incorrectly assumes DuckDB catalog type, which could be wrong for PostgreSQL or SQLite catalogs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 129 - 135, The loading fallback currently hardcodes catalogType: 'duckdb' which can be incorrect; update the fallback object returned in the loading branch (the block that builds the connection summary using activeTab and instanceId) to derive catalogType from the activeTab (e.g., activeTab.catalogType or activeTab.type) or fall back to a neutral value like undefined/'unknown' instead of always 'duckdb', so the UI reflects the actual catalog type while loading.
708-717: Duplicate:isLoading={!duckLakeSchema}causes unrecoverable loading state on failure.This was flagged in a previous review. When
extractSchemafails,duckLakeSchemaremainsnull, keeping the tree viewer in a permanent loading state. Track loading state independently with a boolean likeisDuckLakeSchemaLoading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/screens/sql/index.tsx` around lines 708 - 717, The component currently uses duckLakeSchema to drive loading (isLoading={!duckLakeSchema}), which leaves SchemaTreeViewerWithSchema stuck in loading if extractSchema fails; introduce and manage a separate boolean state (e.g. isDuckLakeSchemaLoading) that is set true when schema extraction starts and set false on both success and failure, update the JSX to pass isLoading={isDuckLakeSchemaLoading} to SchemaTreeViewerWithSchema, and ensure the schema-fetching logic (where extractSchema/duckLakeTables/duckLakeSchema are handled) sets duckLakeSchema on success and clears/reset any error state on failure while also setting isDuckLakeSchemaLoading to false.src/renderer/components/sqlEditor/index.tsx (1)
143-146: Duplicate: Type mapping defaults all string types to0(unknown).This concern was already raised in a previous review. Consider implementing a proper
mapDuckLakeTypeNamefunction to preserve type information for downstream rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sqlEditor/index.tsx` around lines 143 - 146, The current mapping in mappedFields uses typeof field.type === 'number' ? field.type : 0 which collapses all non-numeric (string) types to 0; replace this with a proper mapper function (e.g., mapDuckLakeTypeName) that takes field.type (and optionally field.typeName) and returns a meaningful numeric enum or preserved type info for downstream rendering; update the mappedFields creation to call mapDuckLakeTypeName(field.type) (or mapDuckLakeTypeName(field.typeName)) and implement mapDuckLakeTypeName to handle known DuckLake type strings and fallback to a defined UNKNOWN value rather than always 0.
🧹 Nitpick comments (5)
src/main/services/duckLake/adapters/duckdb.adapter.ts (2)
1044-1067: BigInt debugging logs are verbose; consider gating behind a debug flag.The slice-and-iterate logging for BigInt detection in raw data (lines 1044-1067) and post-normalization verification (lines 1111-1123) are helpful during development but add overhead and noise in production. Consider gating these behind a debug flag or removing them once the feature is stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 1044 - 1067, The BigInt detection console.warn block (inspecting rows, columnNames and raw row values) and the post-normalization BigInt checks should be gated behind a debug flag or replaced with debug-level logging; wrap the checks/log calls so they only run when a debug mode is enabled (e.g. via an existing logger.debug or a new env flag like DEBUG_DUCKDB) and remove direct console.warn usage, referencing the BigInt detection logic around rows/columnNames and the post-normalization verification blocks so they only execute when the debug flag is true.
583-740: Debug logging is verbose but acceptable for development.The extensive
console.logstatements aid in troubleshooting metadata database discovery. Consider wrapping these in a debug flag or using a proper logging library with configurable levels for production builds to reduce noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake/adapters/duckdb.adapter.ts` around lines 583 - 740, The listTables function in duckdb.adapter.ts contains many raw console.log/console.error calls that should be controlled for production; replace these with a configurable logger or conditional debug flag — e.g., use an existing logger (processLogger) or a class-level debug option (this.debugEnabled / this.options.debug) inside the listTables method to only emit these messages when debugging is enabled, and use logger.error for errors (including the error object) instead of console.error; update all occurrences of console.log and console.error in listTables to use the chosen logger or be gated by the debug flag so logs are not noisy in production.src/main/services/duckLake.service.ts (1)
1238-1244: Consider logging when cancellation is requested for an unknown queryId.If
cancelQueryis called with aqueryIdthat doesn't exist inactiveQueries(e.g., query already completed or never registered), the method silently does nothing. Adding a debug log could help diagnose race conditions or mismatched IDs.🔧 Optional enhancement
static async cancelQuery(queryId: string): Promise<void> { const cancelFn = this.activeQueries.get(queryId); if (cancelFn) { cancelFn(); this.activeQueries.delete(queryId); + } else { + // eslint-disable-next-line no-console + console.debug(`[DuckLake Service] cancelQuery: queryId ${queryId} not found (may have already completed)`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/duckLake.service.ts` around lines 1238 - 1244, The cancelQuery method silently ignores unknown queryIds; update DuckLakeService.cancelQuery to log a debug/warn when this.activeQueries.get(queryId) is undefined, including the queryId and context (e.g., "cancel requested for unknown queryId") so race conditions or mismatches are visible; keep existing behavior of calling cancelFn() and deleting when present and only add the log path for missing entries.src/renderer/components/sqlEditor/index.tsx (2)
74-88:TRUNCATEis listed in both DDL and DML checks — minor dead code.
TRUNCATEis inddlKeywords(line 70), andisDDLOperationis called first (line 77). TheTRUNCATEcheck on line 83 is therefore unreachable. Consider removing it from the DML block for clarity.♻️ Proposed cleanup
if ( normalized.startsWith('INSERT') || normalized.startsWith('UPDATE') || normalized.startsWith('DELETE') || - normalized.startsWith('MERGE') || - normalized.startsWith('TRUNCATE') + normalized.startsWith('MERGE') ) { return 'DML'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sqlEditor/index.tsx` around lines 74 - 88, The DML branch in getCommandType contains a redundant TRUNCATE check because isDDLOperation (which uses ddlKeywords including "TRUNCATE") is evaluated first; remove "TRUNCATE" from the normalized.startsWith(...) DML checks (in getCommandType) to eliminate the dead/unreachable code and keep TRUNCATE handled solely by isDDLOperation/ddlKeywords.
130-138: Consider making the DuckLake query limit configurable or larger.A hardcoded limit of 10 rows is quite restrictive for exploratory queries. Users may expect to see more data. Consider increasing the default or making it configurable (e.g., via a constant or user preference).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sqlEditor/index.tsx` around lines 130 - 138, The DuckLake query uses a hardcoded duckLakeQueryLimit = 10 inside the isDuckLakeConnection && instanceId branch which is too small; make this limit configurable or larger by replacing the constant with a configurable value (e.g., read from a module-level constant, settings/preferences, or a new prop) and pass that value into DuckLakeService.executeQuery instead of 10; update any UI or settings plumbing so users can change the default, and ensure getCommandType(selectedQuery) logic still uses limit only for SELECT queries when calling DuckLakeService.executeQuery with instanceId, selectedQuery and queryId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1153-1167: Add the SQL verb "COPY" to the allowedPrefixes array
used by validateSingleStatement so DuckLake COPY ... TO ... (FORMAT PARQUET)
export queries are accepted; locate the allowedPrefixes declaration in
duckLake.service.ts and include 'COPY' alongside the other strings (e.g.,
'SELECT','WITH',...), then run unit/manual validation for
validateSingleStatement to ensure COPY statements now pass.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 919-933: The metadataTables array in duckdb.adapter.ts is missing
'ducklake_snapshot_changes', causing unqualified references to fail in
listSnapshots() and listInstanceSnapshots(); add 'ducklake_snapshot_changes' to
the metadataTables array (alongside entries like 'ducklake_snapshot' and
'ducklake_table') so the adapter will recognize and qualify that table name in
queries used by listSnapshots() and listInstanceSnapshots().
- Around line 967-970: The code in duckdb.adapter.ts interpolates snapshotId
from DuckLakeQueryRequest directly into SQL (the block that appends "FOR
SYSTEM_TIME AS OF SNAPSHOT '...'" ), which allows SQL injection; validate and
sanitize snapshotId before using it: ensure snapshotId is present and is a
strict integer (e.g., parse/validate with Number.isInteger or a regex allowing
only digits) and only then append it to the query (or better, use a
parameterized mechanism if DuckDB client supports params) and avoid embedding
any non-numeric characters; if validation fails, throw or ignore the snapshotId
so no untrusted input is interpolated.
- Around line 1022-1025: The pagination block in duckdb.adapter.ts interpolates
limit and offset directly into the SQL string (variables `limit` and `offset`),
so validate/coerce them before use: in the function that builds the query (the
query construction / pagination block), parse `limit` and `offset` to integers
(e.g., Number/parseInt), verify Number.isInteger and >= 0 (and apply a
reasonable max cap), fallback to sensible defaults or reject/throw if invalid,
and then use the validated values when appending `LIMIT` and `OFFSET`;
alternatively switch to parameterized binding if the query builder/DB client
supports it to avoid string interpolation. Ensure you update the same
function/method that contains the lines `query += \` LIMIT ${limit}\`;` and
`query += \` OFFSET ${offset}\`;`.
In `@src/renderer/components/sqlEditor/index.tsx`:
- Around line 143-146: The current mapping in mappedFields uses typeof
field.type === 'number' ? field.type : 0 which collapses all non-numeric
(string) types to 0; replace this with a proper mapper function (e.g.,
mapDuckLakeTypeName) that takes field.type (and optionally field.typeName) and
returns a meaningful numeric enum or preserved type info for downstream
rendering; update the mappedFields creation to call
mapDuckLakeTypeName(field.type) (or mapDuckLakeTypeName(field.typeName)) and
implement mapDuckLakeTypeName to handle known DuckLake type strings and fallback
to a defined UNKNOWN value rather than always 0.
In `@src/renderer/screens/sql/index.tsx`:
- Around line 615-627: The MenuItem rendering for the DuckLake instance is using
the wrong icon key; in the JSX block inside the SQL screen (the MenuItem that
displays {instance.name}) replace the use of connectionIcons.images.duckdb with
connectionIcons.images.ducklake so the icon matches the renderValue/display for
DuckLake; update the img src reference in that MenuItem to use ducklake instead
of duckdb.
- Around line 431-470: In the DuckLake branch inside the renderValue logic (the
block that checks selected.startsWith('ducklake-') and resolves instance via
duckLakeInstances.find), replace the generic fallback that returns 'Select
Connection' when instance is undefined with a contextual fallback such as
activeTab?.connectionName || 'Loading...' so the UI shows the current
connectionName or a Loading... state while the DuckLake instance is still
loading.
- Around line 208-240: When switching DuckLake instances the old
schema/completions can briefly persist; inside the useEffect's async loader
(loadDuckLakeCompletions) clear the UI state immediately before any awaits by
calling setDuckLakeCompletions([]) and setDuckLakeSchema(null) as soon as you
verify the input is a 'ducklake' connection (or even at start of the function),
then proceed to call DuckLakeService.extractSchema and
generateDuckLakeCompletions; optionally add a simple request token (e.g., local
requestId checked before applying setDuckLakeCompletions/setDuckLakeSchema) to
avoid applying results from an outdated async call.
- Around line 129-135: The loading fallback currently hardcodes catalogType:
'duckdb' which can be incorrect; update the fallback object returned in the
loading branch (the block that builds the connection summary using activeTab and
instanceId) to derive catalogType from the activeTab (e.g.,
activeTab.catalogType or activeTab.type) or fall back to a neutral value like
undefined/'unknown' instead of always 'duckdb', so the UI reflects the actual
catalog type while loading.
- Around line 708-717: The component currently uses duckLakeSchema to drive
loading (isLoading={!duckLakeSchema}), which leaves SchemaTreeViewerWithSchema
stuck in loading if extractSchema fails; introduce and manage a separate boolean
state (e.g. isDuckLakeSchemaLoading) that is set true when schema extraction
starts and set false on both success and failure, update the JSX to pass
isLoading={isDuckLakeSchemaLoading} to SchemaTreeViewerWithSchema, and ensure
the schema-fetching logic (where extractSchema/duckLakeTables/duckLakeSchema are
handled) sets duckLakeSchema on success and clears/reset any error state on
failure while also setting isDuckLakeSchemaLoading to false.
---
Nitpick comments:
In `@src/main/services/duckLake.service.ts`:
- Around line 1238-1244: The cancelQuery method silently ignores unknown
queryIds; update DuckLakeService.cancelQuery to log a debug/warn when
this.activeQueries.get(queryId) is undefined, including the queryId and context
(e.g., "cancel requested for unknown queryId") so race conditions or mismatches
are visible; keep existing behavior of calling cancelFn() and deleting when
present and only add the log path for missing entries.
In `@src/main/services/duckLake/adapters/duckdb.adapter.ts`:
- Around line 1044-1067: The BigInt detection console.warn block (inspecting
rows, columnNames and raw row values) and the post-normalization BigInt checks
should be gated behind a debug flag or replaced with debug-level logging; wrap
the checks/log calls so they only run when a debug mode is enabled (e.g. via an
existing logger.debug or a new env flag like DEBUG_DUCKDB) and remove direct
console.warn usage, referencing the BigInt detection logic around
rows/columnNames and the post-normalization verification blocks so they only
execute when the debug flag is true.
- Around line 583-740: The listTables function in duckdb.adapter.ts contains
many raw console.log/console.error calls that should be controlled for
production; replace these with a configurable logger or conditional debug flag —
e.g., use an existing logger (processLogger) or a class-level debug option
(this.debugEnabled / this.options.debug) inside the listTables method to only
emit these messages when debugging is enabled, and use logger.error for errors
(including the error object) instead of console.error; update all occurrences of
console.log and console.error in listTables to use the chosen logger or be gated
by the debug flag so logs are not noisy in production.
In `@src/renderer/components/sqlEditor/index.tsx`:
- Around line 74-88: The DML branch in getCommandType contains a redundant
TRUNCATE check because isDDLOperation (which uses ddlKeywords including
"TRUNCATE") is evaluated first; remove "TRUNCATE" from the
normalized.startsWith(...) DML checks (in getCommandType) to eliminate the
dead/unreachable code and keep TRUNCATE handled solely by
isDDLOperation/ddlKeywords.
- Around line 130-138: The DuckLake query uses a hardcoded duckLakeQueryLimit =
10 inside the isDuckLakeConnection && instanceId branch which is too small; make
this limit configurable or larger by replacing the constant with a configurable
value (e.g., read from a module-level constant, settings/preferences, or a new
prop) and pass that value into DuckLakeService.executeQuery instead of 10;
update any UI or settings plumbing so users can change the default, and ensure
getCommandType(selectedQuery) logic still uses limit only for SELECT queries
when calling DuckLakeService.executeQuery with instanceId, selectedQuery and
queryId.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/services/duckLake.service.tssrc/main/services/duckLake/adapters/duckdb.adapter.tssrc/renderer/components/sqlEditor/index.tsxsrc/renderer/screens/sql/index.tsx
…ter data processing, and qualify metadata tables for SQLite.
…s and add export progress UI.
… for parquet export operations.
nbesimi
reviewed
Feb 24, 2026
…with JSON.stringify and fall back to String(), and use String() for non-object types.
… result fetching using a sequence ref.
…ing any query suffixes in database adapters.
…it` is specified.
…ctionality and prevent SQL editor cursor jumps by optimizing initial query handling.
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
2 participants
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.
Key features and improvements:
including support for SELECT, DDL, and DML operations with command detection.
displaying the schemas → tables → columns hierarchy with type-specific icons.
system tables, metadata views, functions, and time-travel syntax.
queries with the correct metadata database.
values were being incorrectly converted to undefined.
theme-aware icons, and improved connection grouping in the sidebar.
Files changed:
Summary by CodeRabbit
New Features
Improvements
Documentation