Feature: Comprehensive notebook system for SQL queries

[Nuri1977]

Summary by CodeRabbit

• New Features

  • Full notebook system: list, create, edit (SQL ), run cells (per-cell & run-all), reorder via drag-and-drop, tabs, editor, toolbar, output panel with pagination and exports (JSON/CSV/Parquet).
  • Notebooks screen and route added; sidebar entry and icon for quick access.
  • Archival/restore workflows for notebooks tied to deleted connections.

• Chores

  • Added drag-and-drop runtime dependency for notebook reordering.

• Documentation

  • Removed internal GitHub Copilot Instructions guide.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a full notebooks feature: backend file-based NotebooksService with IPC handlers, main-process integration, renderer IPC service, React UI (editor, cells, output, lists, sidebar), hooks/controllers for state and schema, DuckLake/connector support, import/export, and archival workflows.

Changes

Cohort / File(s)	Summary
Removed Documentation \.kiro/steering/github-intructions.md	Removed a 466-line developer guide describing IPC patterns, channel conventions, and development guidelines.
Main process — Notebooks service & IPC src/main/services/notebooks.service.ts, src/main/ipcHandlers/notebooks.ipcHandlers.ts, src/main/ipcHandlers/index.ts, src/main/ipcSetup.ts	New file-based NotebooksService implementing CRUD, import/export, execution, pagination, archival; IPC handlers register notebook channels and are wired into IPC setup. Attention: path-safety, serialization, and IPC surface.
Main process — Integration points src/main/services/connectors.service.ts, src/main/services/duckLake.service.ts	Added calls to archive notebooks when deleting connections/instances (deletion aborts on archival failure). Attention: transactional/failure semantics and ordering.
Renderer — IPC wrappers & types src/renderer/services/notebooks.service.ts, src/types/ipc.ts, src/types/notebooks.ts	Frontend IPC service and new IPC channel/type declarations and notebook domain types. Attention: channel name alignment and payload shapes.
Renderer — Controllers & hooks src/renderer/controllers/notebooks.controller.ts, src/renderer/hooks/*	React Query hooks for notebooks/schema and multiple new hooks (monaco autocomplete, schema loader, tab/connection/sidebar persistence). Attention: cache invalidation, hydration, and localStorage format.
Renderer — Notebook UI components src/renderer/components/notebook/..., src/renderer/components/notebook/index.ts	Large set of new UI components: NotebookEditor, NotebookCell, SQLCell, MarkdownCell, OutputPanel, Toolbar, List, Sidebar, TreeView, TabManager. Attention: Monaco lifecycle, DnD (@hello-pangea/dnd), memoization, accessibility, and resize/drag behavior.
Renderer — QueryResult & table changes src/renderer/components/queryResult/index.tsx, src/renderer/components/customTable/*, src/renderer/components/index.ts	New QueryResult component and visual adjustments to custom table (removed sortable headers, added sx styling). Attention: pagination/export flows and header behavior.
Renderer — Screens, routing & sidebar src/renderer/screens/notebooks/index.tsx, src/renderer/screens/index.ts, src/renderer/App.tsx, src/renderer/components/sidebar/*, src/renderer/layouts/appLayout/styles.ts	Added Notebooks screen and route /app/notebooks, sidebar entry/icon, and adjusted active-index mapping; layout style tweaks. Attention: route integration and sidebar indexing.
Hooks & Tab manager persistence src/renderer/hooks/useNotebookTabManager.ts, src/renderer/hooks/useNotebookConnectionState.ts, src/renderer/hooks/useNotebookSidebarState.ts	Tab persistence and notebook-specific localStorage-backed state with hydration. Attention: migration, concurrency, and storage key formats.
Package package.json	Added dependency @hello-pangea/dnd for drag-and-drop. Attention: dependency install and potential lockfile update.

Sequence Diagram(s)

sequenceDiagram
  participant UI as "Renderer UI"
  participant Hooks as "notebooks.controller / hooks"
  participant IPC as "ipcRenderer"
  participant Main as "Main IPC handler"
  participant Service as "NotebooksService (fs)"
  participant Connector as "DuckLake / Connector"

  UI->>Hooks: user triggers Run Cell
  Hooks->>IPC: invoke 'notebooks:runCell'(connectionId, notebookId, cellId, sql, limit, offset)
  IPC->>Main: IPC invoke 'notebooks:runCell'
  Main->>Service: NotebooksService.runCell(...)
  alt DuckLake-backed
    Service->>Connector: execute query via DuckLake
    Connector-->>Service: rows, total, execTime
  else regular connector
    Service->>Connector: execute query via connector
    Connector-->>Service: rows, total, execTime
  end
  Service-->>Main: CellOutput (sanitized, paginated)
  Main-->>IPC: IPC response
  IPC-->>Hooks: resolve CellOutput
  Hooks-->>UI: update cell output UI

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• Feature/ducklake crud operations and data preview #197 — Overlaps DuckLake/service and backend instance deletion changes; may conflict at service-level logic.
• Feature: end to end testing setup, projects screens and sql editor screen tests. #185 — Sidebar/navigation updates touching the same sidebar elements and indices.
• Feature: Implement DuckLake integration with query execution, schema extraction, and metadata handling #209 — DuckLake integration and IPC/schema extraction changes related to notebook execution and schema flows.

Suggested reviewers

• jasir99
• nbesimi
• ailegion

Poem

🐰
Notebooks bloom where queries sing,
Cells that run and notes that spring,
Archived burrows keep them safe,
Drag and run with nimble grace,
Hop, export, and joyfully bring! 🎉

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding a comprehensive notebook system for SQL queries, which is the primary focus of this large feature PR.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/sql-notebooks-v1

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 16

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main/services/connectors.service.ts (1)

536-551: ⚠️ Potential issue | 🟠 Major

Potential inconsistency if notebook archival fails after credential cleanup.

The credential cleanup (lines 536-546) happens before notebook archival (lines 548-550). If archiveConnectionNotebooks throws, the deletion aborts but the credentials have already been deleted from secure storage. This leaves the connection in an inconsistent state where it exists but cannot authenticate.

Consider reordering to archive notebooks first (which is more likely to fail), or wrapping the credential cleanup in a try-finally that can restore credentials if archival fails.

💡 Suggested reordering

     if (projectsUsingConnection.length > 0) {
       const projectNames = projectsUsingConnection
         .map((p) => p.name)
         .join(', ');
       throw new Error(
         `Cannot delete connection. It is currently being used by the following project(s): ${projectNames}. Please remove the connection from these projects first.`,
       );
     }

+    // Archive notebooks for this connection first
+    // If archival fails, abort the deletion to prevent orphaned notebooks
+    await NotebooksService.archiveConnectionNotebooks(connectionToDelete.id);
+
     // Clean up connection-specific credentials from secure storage
     try {
       await SecureStorageService.cleanupConnectionCredentials(
         connectionToDelete.id,
       );
     } catch (error) {
       // eslint-disable-next-line no-console
       console.error(
         `Failed to cleanup credentials for connection ${connectionToDelete.connection.name}:`,
         error,
       );
     }

-    // Archive notebooks for this connection
-    // If archival fails, abort the deletion to prevent orphaned notebooks
-    await NotebooksService.archiveConnectionNotebooks(connectionToDelete.id);
-
     // Remove the connection from the database

🤖 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 536 - 551, The cleanup
of stored credentials (SecureStorageService.cleanupConnectionCredentials called
with connectionToDelete.id) happens before archiving notebooks
(NotebooksService.archiveConnectionNotebooks) which can leave the system
inconsistent if archiving fails; either reorder so
NotebooksService.archiveConnectionNotebooks(connectionToDelete.id) runs first
and only then call
SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id), or
wrap the current cleanup in a try/finally that captures and preserves the
credentials (so you can restore them) and only permanently remove them after
NotebooksService.archiveConnectionNotebooks succeeds; locate the calls around
connectionToDelete in connectors.service.ts and update the control flow
accordingly.

🟡 Minor comments (7)

src/renderer/components/queryResult/index.tsx-321-325 (1)

321-325: ⚠️ Potential issue | 🟡 Minor

Escape CSV header fields to keep exported CSV valid.

Line 321 builds the header with raw join(','). Column aliases containing commas/quotes/newlines will produce invalid CSV.

🛠️ Header escaping fix

-    const header = columns.join(',');
+    const header = columns.map((col) => escapeCsvValue(col)).join(',');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 321 - 325, The
CSV header is built without escaping (const header = columns.join(',')) which
breaks CSV when column names contain commas/quotes/newlines; change header
construction to map each column through the existing escapeCsvValue helper
(e.g., const header = columns.map(c => escapeCsvValue(c)).join(',')) so header
fields are escaped the same way as row values; update usages of header and
csvContent accordingly to keep consistent CSV quoting/escaping behavior for both
headers and dataRows.

src/renderer/components/queryResult/index.tsx-417-438 (1)

417-438: ⚠️ Potential issue | 🟡 Minor

DuckLake DML “rows affected” text may be inaccurate without RETURNING.

This UI assumes results.rowCount is reliable for DML, which can misreport affected rows for DuckLake unless queries include RETURNING.

Based on learnings: DuckLake extension does not support DuckDB changes() and affected row counts are accurate only when DML uses a RETURNING clause.

src/renderer/components/notebook/MarkdownCell.tsx-136-139 (1)

136-139: ⚠️ Potential issue | 🟡 Minor

List items rendered without parent <ul> element creates invalid HTML.

The <li> elements are rendered directly without a wrapping <ul> or <ol> parent, which produces invalid HTML and may cause inconsistent rendering across browsers.

💡 Suggested approach

Consider grouping consecutive list items into a proper list structure, or wrapping them individually:

                 // Lists
                 if (line.startsWith('- ') || line.startsWith('* ')) {
-                  return <li key={index}>{line.slice(2)}</li>;
+                  return (
+                    <Box component="ul" key={index} sx={{ m: 0, pl: 2 }}>
+                      <li>{line.slice(2)}</li>
+                    </Box>
+                  );
                 }

For proper consecutive list grouping, consider using a markdown parsing library like marked or react-markdown as noted in the TODO comment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/MarkdownCell.tsx` around lines 136 - 139,
The code in the MarkdownCell component returns <li> elements directly (the
conditional that checks line.startsWith('- ') || line.startsWith('* ') and
returns <li key={index}>{...}) which produces invalid HTML; update the rendering
logic in MarkdownCell (the lines-to-JSX mapping/block that generates list items)
to group consecutive list lines into a parent <ul> (or <ol>) wrapper and emit
<li> children, or at minimum wrap isolated list items in a <ul> so no <li>
appears without a parent; alternatively replace the ad-hoc parser with a
markdown parser (e.g., react-markdown/marked) as noted in the TODO to handle
lists and nesting correctly.

src/renderer/components/notebook/NotebookToolbar.tsx-338-344 (1)

338-344: ⚠️ Potential issue | 🟡 Minor

Disable “Clear” when no callback is provided.

Line 343 only checks notebook.cells.length. If onClearOutputs is undefined, the button is enabled but does nothing.

🔧 Proposed fix

         <Button
           startIcon={<ClearIcon sx={{ fontSize: 14 }} />}
           variant="outlined"
           size="small"
           onClick={onClearOutputs}
-          disabled={notebook.cells.length === 0}
+          disabled={notebook.cells.length === 0 || !onClearOutputs}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookToolbar.tsx` around lines 338 - 344,
The Clear button currently only disables based on notebook.cells.length and
remains enabled when no callback exists; update its disabled condition to also
check that onClearOutputs is a valid function (e.g.,
disabled={notebook.cells.length === 0 || typeof onClearOutputs !== 'function' ||
!onClearOutputs}) so the Button (startIcon ClearIcon, props variant/size) is
disabled when onClearOutputs is undefined or not a function and therefore cannot
be invoked.

src/renderer/hooks/useSchemaForConnection.ts-39-45 (1)

39-45: ⚠️ Potential issue | 🟡 Minor

Preserve column ordinal positions in DuckLake table mapping.

Line 44 sets ordinalPosition to 0 for every column, which can break ordering assumptions in consumers.

🔧 Proposed fix

-              columns: table.columns.map((col) => ({
+              columns: table.columns.map((col, index) => ({
                 name: col.name,
                 typeName: col.type,
                 type: col.type,
                 nullable: true,
-                ordinalPosition: 0,
+                ordinalPosition: index + 1,
                 primaryKeySequenceId: 0,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/useSchemaForConnection.ts` around lines 39 - 45, The
column mapping in useSchemaForConnection.ts sets ordinalPosition to a constant 0
which breaks consumers relying on column order; update the mapping inside the
columns: table.columns.map(...) block to use the actual ordinal value (e.g., use
col.ordinalPosition if the source provides it or fallback to the map index + 1)
so each Column object produced by this mapping preserves the original ordering
and replaces the hardcoded 0 in ordinalPosition.

src/renderer/services/notebooks.service.ts-208-217 (1)

208-217: ⚠️ Potential issue | 🟡 Minor

exportData is a public stub that always throws

This creates a guaranteed runtime failure if any caller uses the API.

If helpful, I can draft the IPC channel + renderer/main implementations for JSON/CSV/Parquet export and open a follow-up issue checklist.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/services/notebooks.service.ts` around lines 208 - 217, The
public method exportData in notebooks.service.ts currently logs and throws,
causing runtime failures; replace the stub with an IPC invocation to the main
process (use ipcRenderer.invoke) sending { cellId, format, data } and await the
main's response (file path/URL or error), remove the console.log and the
unconditional throw, and propagate or handle errors from the IPC call; ensure
you use the exportData function name to locate the stub and agree on an IPC
channel (e.g., "export-data") and return type (Promise<string>) so callers
receive the exported file path or a rejected promise on error.

src/renderer/components/notebook/NotebooksList.tsx-171-177 (1)

171-177: ⚠️ Potential issue | 🟡 Minor

Close the notebook menu when delete is canceled

When confirmation is canceled, the context menu stays open. This is a small UX glitch.

💡 Suggested fix

-    if (!window.confirm('Are you sure you want to delete this notebook?'))
-      return;
+    if (!window.confirm('Are you sure you want to delete this notebook?')) {
+      setMenuAnchor(null);
+      return;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebooksList.tsx` around lines 171 - 177,
The context menu remains open when deletion is canceled; inside
handleDeleteNotebook, after the confirm check returns false, close the menu by
clearing the menu anchor (call setMenuAnchor(null) or the component's menu-close
handler) so the menu is dismissed when the user cancels; ensure you reference
menuAnchor and update its setter (e.g., setMenuAnchor) in the canceled branch.

🧹 Nitpick comments (8)

src/renderer/components/customTable/CustomTableHead.tsx (1)

5-20: Avoid required-but-unused sorting props in CustomTableHead API.

Line 7-9 defines sorting props as required while Line 18-19 intentionally ignores them. This makes the contract misleading and requires lint suppression. Consider making sort props optional (compat mode) or splitting static/sortable header variants.

♻️ Minimal contract cleanup

-/* eslint-disable react/no-unused-prop-types */
 type Props<T> = {
-  onRequestSort: (property: keyof T) => void;
+  onRequestSort?: (property: keyof T) => void;
   orderBy?: keyof T;
-  order: 'asc' | 'desc';
+  order?: 'asc' | 'desc';
   indexCell?: boolean;
   columns: Array<ColumnType<T>>;
   rowActions?: Array<TableRowAction<T>>;
 };
-/* eslint-enable react/no-unused-prop-types */

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/customTable/CustomTableHead.tsx` around lines 5 - 20,
The Props<T> type declares sorting props (onRequestSort, orderBy, order) as
required but CustomTableHead intentionally ignores them; update the API by
making these sorting props optional (e.g., onRequestSort?: (property: keyof T)
=> void; orderBy?: keyof T; order?: 'asc' | 'desc') or alternatively create a
separate SortableCustomTableHead variant and remove unused props from the plain
CustomTableHead; change the Props<T> definition accordingly, remove the
eslint-disable for react/no-unused-prop-types, and adjust any callers of
CustomTableHead (or provide the new sortable variant) so consumers pass the
optional sort props only when needed.

src/renderer/components/sidebar/index.tsx (1)

25-25: Prefer path-based active matching over hard-coded indices.

This now depends on element order (4/5/6). A small reorder in getSidebarElements() can silently break active highlighting. Consider returning an active path/key instead of numeric indices.

♻️ Suggested refactor

-  const activeItem = React.useMemo(() => {
+  const activePath = React.useMemo(() => {
     if (location.pathname.includes('cloud-explorer')) {
-      return 5;
+      return '/app/cloud-explorer';
     }
     if (
       location.pathname.includes('data-lake') ||
       location.pathname.includes('datalake')
     ) {
-      return 6;
+      return '/app/data-lake';
     }
     if (location.pathname.includes('connection')) {
-      return 0;
+      return '/app/connections';
     }
     if (location.pathname.includes('select-project')) {
-      return 1;
+      return '/app/select-project';
     }
     if (location.pathname.includes('notebooks')) {
-      return 4;
+      return '/app/notebooks';
     }
     if (location.pathname.includes('sql')) {
-      return 3;
+      return '/app/sql';
     }
     if (
       location.pathname.includes('settings') ||
       location.pathname.includes('add-connection') ||
       location.pathname.includes('edit-connection')
     ) {
-      return -1;
+      return null;
     }
     if (location.pathname === '/app') {
-      return 2;
+      return '/app';
     }
-    return -1;
+    return null;
   }, [location.pathname]);

-  const isActive = activeItem === index;
+  const isActive = activePath === element.path;

Also applies to: 31-31, 39-41

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/sidebar/index.tsx` at line 25, The current
getSidebarElements() implementation returns hard-coded numeric indices (e.g.,
return 5) which breaks active highlighting when element order changes; change it
to return a stable identifier (path or key string) representing the active item
instead of numeric indices, update getSidebarElements() to compute and return
that path/key, and then update all consumers (places currently reading the
numeric result — e.g., the active/highlight logic in the Sidebar component and
any call sites referencing the numeric values) to compare the returned path/key
against each sidebar item's id/path for highlighting; ensure tests and type
signatures (if any) reflect the new return type.

src/main/ipcHandlers/notebooks.ipcHandlers.ts (2)

104-124: Consider using an options object for handlers with many parameters.

The runCell and fetchCellPage handlers accept 6+ positional parameters, which can be error-prone when calling from the renderer. An options object pattern would improve readability and maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/ipcHandlers/notebooks.ipcHandlers.ts` around lines 104 - 124, Wrap
the many positional args for the 'notebooks:runCell' IPC handler into a single
options object and update the call site to pass that object into
NotebooksService.runCell; specifically change the ipcMain.handle callback
signature to accept one param (e.g. args: { connectionId, notebookId, cellId,
sql, limit?, offset? }) and call NotebooksService.runCell(args) (and mirror this
pattern for the 'notebooks:fetchCellPage' handler and
NotebooksService.fetchCellPage) so callers from the renderer use a single named
object instead of many positional parameters.

---

37-42: Consider using a specific type instead of any for updates parameter.

The updates parameter is typed as any, which bypasses TypeScript's type checking. If there's a NotebookUpdate or Partial<Notebook> type available in the notebooks types, using it would improve type safety and IDE support.

💡 Suggested improvement

+import { Notebook } from '../../types/notebooks';
+
   // Update a notebook
   ipcMain.handle(
     'notebooks:update',
-    async (_event, connectionId: string, notebookId: string, updates: any) => {
+    async (_event, connectionId: string, notebookId: string, updates: Partial<Notebook>) => {
       return NotebooksService.updateNotebook(connectionId, notebookId, updates);
     },
   );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/ipcHandlers/notebooks.ipcHandlers.ts` around lines 37 - 42, The IPC
handler for 'notebooks:update' currently types the updates parameter as any;
change it to a concrete type (e.g., NotebookUpdate or Partial<Notebook>) and
import that type from your notebooks types, then update the handler signature
and the call to NotebooksService.updateNotebook(connectionId, notebookId,
updates) to use the new typed parameter; if no suitable type exists, define an
interface NotebookUpdate (or use Partial<Notebook>) that reflects allowed update
fields and use it in the handler and service method signatures for full type
safety.

src/renderer/components/notebook/NotebooksSidebar.tsx (1)

355-360: “Export Selected” is currently unreachable.

Line 355 is hard-disabled, so onExportSelected can never run.

🔧 Minimal fix

-        <MenuItem onClick={handleExportSelected} disabled>
+        <MenuItem onClick={handleExportSelected} disabled={!onExportSelected}>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebooksSidebar.tsx` around lines 355 -
360, The "Export Selected" MenuItem is always disabled so handleExportSelected
never runs; update the MenuItem in NotebooksSidebar (the element using
handleExportSelected) to be enabled conditionally based on the actual selection
state instead of hard-disabled—e.g., replace the static disabled prop with a
boolean expression like disabled={selectedNotebookIds.length === 0} (or
disabled={!selectedNotebooks || selectedNotebooks.length === 0}) so the item is
enabled when there are selected notebooks, and ensure handleExportSelected
remains the click handler.

src/types/notebooks.ts (1)

8-9: Avoid any in exported notebook contracts.

Lines 8, 63, and 64 expose any in shared types, which reduces type safety across the notebooks surface.

♻️ Proposed fix

 export interface CellOutput {
   type: 'table' | 'error' | 'empty';
-  data?: any[];
+  data?: unknown[];
   columns?: string[];
   rowCount?: number;
   totalRows?: number; // Total rows in full dataset (for pagination)
   executionTime?: number;
   error?: string;
 }
@@
     schema_name: string;
     nulls_allowed?: boolean;
-    min_value?: any;
-    max_value?: any;
+    min_value?: unknown;
+    max_value?: unknown;
     parent_column?: string;
     parent_column_name?: string;
   }>;
 }

Also applies to: 63-64

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types/notebooks.ts` around lines 8 - 9, The exported notebook contract
currently exposes data?: any[]; (and the same any usage on lines 63-64), reduce
unsafe any by introducing a specific shape or a generic type: replace data?:
any[] with either data?: Array<Record<string, unknown>> or make the exported
interface generic (e.g. interface Notebook<T = Record<string, unknown>> { data?:
T[]; columns?: string[] }) and update all usages to supply a concrete T; ensure
the properties named data and columns in this file are updated to the chosen
type and any consuming code is adjusted to the new generic or Record-based type.

src/renderer/screens/notebooks/index.tsx (1)

1141-1145: Prefer onKeyDown over onKeyPress for Enter handlers.

The DOM keypress event is deprecated per web standards; React's onKeyPress maps to this legacy behavior. Use onKeyDown instead for better future compatibility and alignment with current best practices.

🔧 Proposed fix pattern

-              onKeyPress={(e) => {
+              onKeyDown={(e) => {
                 if (e.key === 'Enter' && newNotebookName.trim()) {
                   handleCreateNotebook();
                 }
               }}

Also applies to: lines 1232-1236, 1282-1286

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/screens/notebooks/index.tsx` around lines 1141 - 1145, Replace
deprecated onKeyPress handlers with onKeyDown for Enter key handling: update the
JSX where onKeyPress={(e) => { if (e.key === 'Enter' && newNotebookName.trim())
{ handleCreateNotebook(); } }} to use onKeyDown instead, and perform the same
key check and call to handleCreateNotebook; also update the other occurrences
that use onKeyPress (the similar handlers around the
newNotebookName/handleCreateNotebook logic) to maintain consistent behavior
across the component (keep the e.key === 'Enter' check and trimming logic
intact).

src/renderer/controllers/notebooks.controller.ts (1)

170-188: Avoid stringly-typed cancel control flow in import mutations.

Throwing and matching 'No file selected' is brittle and duplicated. A shared sentinel/error type avoids accidental breakage from message changes.

Suggested refactor

+const IMPORT_CANCELLED = 'IMPORT_CANCELLED';
+
 export function useImportNotebook() {
@@
-      if (!filePath) {
-        throw new Error('No file selected');
-      }
+      if (!filePath) throw new Error(IMPORT_CANCELLED);
@@
-      if (error.message !== 'No file selected') {
+      if (error.message !== IMPORT_CANCELLED) {
         toast.error(`Failed to import notebook: ${error.message}`);
       }
@@
 export function useImportAllNotebooks() {
@@
-      if (!filePath) {
-        throw new Error('No file selected');
-      }
+      if (!filePath) throw new Error(IMPORT_CANCELLED);
@@
-      if (error.message !== 'No file selected') {
+      if (error.message !== IMPORT_CANCELLED) {
         toast.error(`Failed to import notebooks: ${error.message}`);
       }

Also applies to: 198-217

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/controllers/notebooks.controller.ts` around lines 170 - 188,
Replace the stringly-typed cancel flow by introducing and checking a dedicated
sentinel error/type from the service instead of matching 'No file selected':
have notebooksService.selectImportFile throw a specific exported error class or
a well-named sentinel (e.g., UserCancelledError or SELECT_FILE_CANCELLED) and
update the mutationFn and onError in notebooks.controller.ts (the mutationFn
that calls notebooksService.selectImportFile and the onError handler that
currently checks error.message) to detect that sentinel/type (using instanceof
or identity) and skip showing the toast only for that case; apply the same
change to the other import mutation block referenced (the 198-217 section) so
both import flows use the shared sentinel error type.

🤖 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/notebooks.service.ts`:
- Around line 1147-1171: The restoreNotebook implementation (and similarly
deleteArchivedNotebook and deleteAllArchivedNotebooks) currently concatenates
archivedConnectionKey / connectionKey / notebookId into paths (ORPHANED_DIR,
getConnectionDir, getNotebookPath) without validating segments; add and call an
assertSafeSegment (or equivalent) on archivedConnectionKey,
targetConnectionId/connectionKey and notebookId before building any paths, then
resolve the final path against the base directory (e.g., ensure the resulting
path startsWith the expected base like ORPHANED_DIR or getConnectionDir result)
to prevent path traversal; update restoreNotebook to validate
archivedConnectionKey and notebookId up front, validate targetConnectionId
(normalizeConnectionKey) as well, then proceed to ensureDirectories(), mkdir,
readFile, writeFile using only the validated segments and resolved base-path
checks.
- Around line 654-657: The current isSelectQuery helper only checks
startsWith('SELECT') and misses row-returning statements like "WITH ... SELECT"
so update isSelectQuery to detect SELECT even when preceded by CTEs or
whitespace/comments: implement a regex-based check that ignores leading CTE
definitions (e.g. /^\s*(?:WITH\b[\s\S]*?\)\s*)*SELECT\b/i or a similar pattern)
and return true for those cases; also update the duplicate detector used later
(the other isSelectQuery-like logic around the same file) to reuse this single
robust function so all row-returning queries (including WITH ... SELECT) trigger
pagination/count behavior.
- Around line 1169-1174: The restore currently writes to targetPath (built via
getNotebookPath(targetConnectionKey, notebookId)) with fs.writeFile without
checking for an existing notebook, which can silently overwrite; modify the
restore flow to check for an existing file at targetPath (e.g.,
fs.stat/fs.access or fs.exists) before calling fs.writeFile and, if the file
exists, abort and surface an error (or generate a new unique notebookId and
targetPath) rather than blindly overwriting; update any surrounding logic that
assumes the write always succeeds to handle the new error/branch.
- Around line 803-870: fetchCellPage uses limit and offset directly when
building the SQL for regular connections (in the branch that calls
ConnectorsService.executeQueryForConnection), which skips the sanitization used
in runCell; before constructing queryToExecute you must validate and/or
normalize limit and offset exactly as runCell does (e.g., coerce to integers,
enforce non-negative values, apply the same max cap or defaults, and reject
non-numeric input), or reuse the runCell sanitization helper if available, and
prefer passing them as query parameters to
ConnectorsService.executeQueryForConnection when supported rather than string
interpolation to prevent injection and ensure correct typing.

In `@src/renderer/components/notebook/NotebookCell.tsx`:
- Around line 27-38: The onRun callback in NotebookCellProps is typed as no-arg
but SQLCell calls onRun(content), causing stale SQL execution; update the
NotebookCellProps interface to declare onRun: (content: string) => void, update
any internal usages in NotebookCell (and related handlers) to pass the current
cell content when invoking onRun, and update the NotebookEditor caller (which
currently passes a no-arg lambda) to accept the content parameter and forward it
into the execution handler so SQLCell receives the correct SQL string; ensure
the same signature change is applied wherever onRun is defined or forwarded
(e.g., the NotebookEditor run handler mentioned in the review).

In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 187-221: The run-all loop iterates over raw localCells which can
be out-of-order versus the UI (cells are rendered by their order property),
causing execution sequence and setRunningCellIndex to mismatch; before
iterating, derive a sorted array (e.g., sort localCells by cell.order) and
iterate that sorted list, using each cell.id with runCell.mutateAsync and
updating setRunningCellIndex based on the sorted index (and map back to original
indices if needed for UI), and replace all other run-all usages (including the
block around runCell.mutateAsync and the other occurrence noted) to use this
sorted sequence.
- Around line 111-155: The code re-registers a global Monaco completion provider
per NotebookEditor instance (via loader.init ->
monaco.languages.registerCompletionItemProvider('sql', ...)) which causes
duplicate providers; instead create a module-level singleton provider registered
once (e.g., a file-scoped variable like sharedCompletionProvider) and only
update its backing suggestions via the existing completionsRef used by
provideCompletionItems; modify NotebookEditor to avoid calling
registerCompletionItemProvider on each mount (call it once at module init or a
top-level effect) and stop disposing the shared provider in the instance cleanup
(only dispose on app teardown) while still keeping provideCompletionItems
reading the latest completionsRef and using the same range logic.

In `@src/renderer/components/notebook/NotebooksList.tsx`:
- Around line 92-168: handleCreateExampleNotebook currently creates a notebook
with createNotebook.mutateAsync and then updates it with
updateNotebook.mutateAsync, which can leave an empty notebook if the second call
fails; either (preferred) add a single backend RPC like
createNotebookWithCells(connectionId, name, description, cells) and call that
atomically from handleCreateExampleNotebook, or (quick fix) implement a
rollback: after createNotebook.mutateAsync returns, wrap
updateNotebook.mutateAsync in try/catch and on failure call
deleteNotebook.mutateAsync({ connectionId: instanceId, notebookId: notebook.id
}) (log/handle delete errors) before rethrowing or showing an error; reference
the functions handleCreateExampleNotebook, createNotebook.mutateAsync,
updateNotebook.mutateAsync, and deleteNotebook.mutateAsync.

In `@src/renderer/components/notebook/OutputPanel.tsx`:
- Around line 76-80: The effect that resets pagination on output change (the
React.useEffect calling setPage(0) and setPaginatedData(output.data || []))
doesn't increment or invalidate latestRequestId, so in-flight async page
requests can still resolve and overwrite the new state; update that effect to
bump/invalidate latestRequestId (the same request id used by your pagination
logic such as in handlePageRequest/handlePaginationRequest) whenever output
changes so any ongoing requests check the id and abort/apply no-ops if it no
longer matches, ensuring stale responses are ignored.

In `@src/renderer/components/notebook/SQLCell.tsx`:
- Around line 510-537: The Monaco keyboard commands in handleEditorDidMount
capture stale closures for isExecuting and handleRun; create refs (e.g.,
isExecutingRef and handleRunRef), update them whenever isExecuting or handleRun
change (inside useEffect or where those values are set), and change the command
callbacks in handleEditorDidMount to read isExecutingRef.current and invoke
handleRunRef.current(content) so the shortcuts always use the latest state and
handler; keep editorRef and monaco usage as-is but replace direct references to
isExecuting/handleRun inside the added commands with the ref-backed values.

In `@src/renderer/components/queryResult/index.tsx`:
- Around line 345-347: The handler handleExportParquet uses
exportContext?.originalSql as a guard but canExportParquet is derived from
originalSql present on either exportContext or results, causing a no-op when SQL
lives on results; change handleExportParquet to resolve a single originalSql
value (e.g., const originalSql = exportContext?.originalSql ??
results?.originalSql) and use that for the early return and export logic, and
apply the same fix to the sibling export handler (the one around lines 366-370)
so both handlers use the same originalSql source as canExportParquet; update
references to canExportParquet if needed to rely on the resolved originalSql
variable.

In `@src/renderer/controllers/notebooks.controller.ts`:
- Around line 399-427: The code currently ignores result.error and returns an
empty schemaInfo, masking failures; update the logic around the result check
(the block that builds schemaInfo from result.tables) so that if result.error is
truthy you propagate the error instead of returning empty arrays—e.g., inside
the function that builds/returns schemaInfo, detect result.error and either
throw result.error (or return a rejected Promise with result.error) before
constructing schemaInfo; ensure references to result, schemaInfo, result.tables,
and the existing return schemaInfo are adjusted so failures are surfaced rather
than returning an empty successful schema.
- Line 6: The file imports from the v3 namespace "react-query" but uses v4+ hook
signatures (useMutation({ mutationFn, ... })); either upgrade to TanStack v4+
(replace imports with '@tanstack/react-query' and adjust peer deps) or, more
simply for v3.39.3, change every useMutation call to the v3 signature
useMutation(mutationFn, options) and pass the mutation function as the first
argument and the config object as the second (update all occurrences referenced
in the review: the useMutation calls at the listed locations), ensuring any
option keys are compatible with v3 and keeping useQuery/useQueryClient imports
as-is.

In `@src/renderer/hooks/useNotebookConnectionState.ts`:
- Around line 18-21: Persisted activeConnectionId from JSON.parse can be
non-string; before calling setActiveConnectionId(parsed.activeConnectionId) in
useNotebookConnectionState validate that parsed.activeConnectionId is a string
(and optionally non-empty) — e.g., check typeof parsed.activeConnectionId ===
'string' (and/or String(parsed.activeConnectionId).length > 0) and only call
setActiveConnectionId when it passes, otherwise ignore or clear the value to
avoid downstream crashes in code that assumes a string (e.g., startsWith).

In `@src/renderer/hooks/useNotebookSidebarState.ts`:
- Around line 14-25: The persisted sidebar state handling (STORAGE_KEY,
PersistedSidebarState) trusts storage shape on read and writes without guards;
update the useEffect that reads (JSON.parse raw) to validate the parsed object
before calling setShowArchived (e.g., check typeof parsed === 'object' and
'showArchived' in parsed and its boolean type) and keep the existing try/catch,
and also wrap any localStorage.setItem calls that write the persisted state in a
try/catch and serialize only a validated object (derive payload from current
showArchived state rather than raw input) so malformed payloads or write
failures are safely handled before calling setShowArchived or setIsHydrated.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 175-176: The cache short-circuit (if (tabSchemas[connectionId])
return;) causes manual refreshes to be skipped because handleRefreshSchema
deletes the cache asynchronously; fix by adding a force-refresh flag to the
schema-loading routine (e.g., loadSchemasForConnection or whatever function
contains the tabSchemas check) and change the check to return only when
!forceRefresh && tabSchemas[connectionId]; then update handleRefreshSchema to
either await the cache deletion before calling the loader or call the loader
with forceRefresh=true. Apply the same change where a similar short-circuit
exists (the other occurrence around the 277-286 logic) so manual refresh
bypasses the cached short-circuit reliably.

---

Outside diff comments:
In `@src/main/services/connectors.service.ts`:
- Around line 536-551: The cleanup of stored credentials
(SecureStorageService.cleanupConnectionCredentials called with
connectionToDelete.id) happens before archiving notebooks
(NotebooksService.archiveConnectionNotebooks) which can leave the system
inconsistent if archiving fails; either reorder so
NotebooksService.archiveConnectionNotebooks(connectionToDelete.id) runs first
and only then call
SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id), or
wrap the current cleanup in a try/finally that captures and preserves the
credentials (so you can restore them) and only permanently remove them after
NotebooksService.archiveConnectionNotebooks succeeds; locate the calls around
connectionToDelete in connectors.service.ts and update the control flow
accordingly.

---

Minor comments:
In `@src/renderer/components/notebook/MarkdownCell.tsx`:
- Around line 136-139: The code in the MarkdownCell component returns <li>
elements directly (the conditional that checks line.startsWith('- ') ||
line.startsWith('* ') and returns <li key={index}>{...}) which produces invalid
HTML; update the rendering logic in MarkdownCell (the lines-to-JSX mapping/block
that generates list items) to group consecutive list lines into a parent <ul>
(or <ol>) wrapper and emit <li> children, or at minimum wrap isolated list items
in a <ul> so no <li> appears without a parent; alternatively replace the ad-hoc
parser with a markdown parser (e.g., react-markdown/marked) as noted in the TODO
to handle lists and nesting correctly.

In `@src/renderer/components/notebook/NotebooksList.tsx`:
- Around line 171-177: The context menu remains open when deletion is canceled;
inside handleDeleteNotebook, after the confirm check returns false, close the
menu by clearing the menu anchor (call setMenuAnchor(null) or the component's
menu-close handler) so the menu is dismissed when the user cancels; ensure you
reference menuAnchor and update its setter (e.g., setMenuAnchor) in the canceled
branch.

In `@src/renderer/components/notebook/NotebookToolbar.tsx`:
- Around line 338-344: The Clear button currently only disables based on
notebook.cells.length and remains enabled when no callback exists; update its
disabled condition to also check that onClearOutputs is a valid function (e.g.,
disabled={notebook.cells.length === 0 || typeof onClearOutputs !== 'function' ||
!onClearOutputs}) so the Button (startIcon ClearIcon, props variant/size) is
disabled when onClearOutputs is undefined or not a function and therefore cannot
be invoked.

In `@src/renderer/components/queryResult/index.tsx`:
- Around line 321-325: The CSV header is built without escaping (const header =
columns.join(',')) which breaks CSV when column names contain
commas/quotes/newlines; change header construction to map each column through
the existing escapeCsvValue helper (e.g., const header = columns.map(c =>
escapeCsvValue(c)).join(',')) so header fields are escaped the same way as row
values; update usages of header and csvContent accordingly to keep consistent
CSV quoting/escaping behavior for both headers and dataRows.

In `@src/renderer/hooks/useSchemaForConnection.ts`:
- Around line 39-45: The column mapping in useSchemaForConnection.ts sets
ordinalPosition to a constant 0 which breaks consumers relying on column order;
update the mapping inside the columns: table.columns.map(...) block to use the
actual ordinal value (e.g., use col.ordinalPosition if the source provides it or
fallback to the map index + 1) so each Column object produced by this mapping
preserves the original ordering and replaces the hardcoded 0 in ordinalPosition.

In `@src/renderer/services/notebooks.service.ts`:
- Around line 208-217: The public method exportData in notebooks.service.ts
currently logs and throws, causing runtime failures; replace the stub with an
IPC invocation to the main process (use ipcRenderer.invoke) sending { cellId,
format, data } and await the main's response (file path/URL or error), remove
the console.log and the unconditional throw, and propagate or handle errors from
the IPC call; ensure you use the exportData function name to locate the stub and
agree on an IPC channel (e.g., "export-data") and return type (Promise<string>)
so callers receive the exported file path or a rejected promise on error.

---

Nitpick comments:
In `@src/main/ipcHandlers/notebooks.ipcHandlers.ts`:
- Around line 104-124: Wrap the many positional args for the 'notebooks:runCell'
IPC handler into a single options object and update the call site to pass that
object into NotebooksService.runCell; specifically change the ipcMain.handle
callback signature to accept one param (e.g. args: { connectionId, notebookId,
cellId, sql, limit?, offset? }) and call NotebooksService.runCell(args) (and
mirror this pattern for the 'notebooks:fetchCellPage' handler and
NotebooksService.fetchCellPage) so callers from the renderer use a single named
object instead of many positional parameters.
- Around line 37-42: The IPC handler for 'notebooks:update' currently types the
updates parameter as any; change it to a concrete type (e.g., NotebookUpdate or
Partial<Notebook>) and import that type from your notebooks types, then update
the handler signature and the call to
NotebooksService.updateNotebook(connectionId, notebookId, updates) to use the
new typed parameter; if no suitable type exists, define an interface
NotebookUpdate (or use Partial<Notebook>) that reflects allowed update fields
and use it in the handler and service method signatures for full type safety.

In `@src/renderer/components/customTable/CustomTableHead.tsx`:
- Around line 5-20: The Props<T> type declares sorting props (onRequestSort,
orderBy, order) as required but CustomTableHead intentionally ignores them;
update the API by making these sorting props optional (e.g., onRequestSort?:
(property: keyof T) => void; orderBy?: keyof T; order?: 'asc' | 'desc') or
alternatively create a separate SortableCustomTableHead variant and remove
unused props from the plain CustomTableHead; change the Props<T> definition
accordingly, remove the eslint-disable for react/no-unused-prop-types, and
adjust any callers of CustomTableHead (or provide the new sortable variant) so
consumers pass the optional sort props only when needed.

In `@src/renderer/components/notebook/NotebooksSidebar.tsx`:
- Around line 355-360: The "Export Selected" MenuItem is always disabled so
handleExportSelected never runs; update the MenuItem in NotebooksSidebar (the
element using handleExportSelected) to be enabled conditionally based on the
actual selection state instead of hard-disabled—e.g., replace the static
disabled prop with a boolean expression like
disabled={selectedNotebookIds.length === 0} (or disabled={!selectedNotebooks ||
selectedNotebooks.length === 0}) so the item is enabled when there are selected
notebooks, and ensure handleExportSelected remains the click handler.

In `@src/renderer/components/sidebar/index.tsx`:
- Line 25: The current getSidebarElements() implementation returns hard-coded
numeric indices (e.g., return 5) which breaks active highlighting when element
order changes; change it to return a stable identifier (path or key string)
representing the active item instead of numeric indices, update
getSidebarElements() to compute and return that path/key, and then update all
consumers (places currently reading the numeric result — e.g., the
active/highlight logic in the Sidebar component and any call sites referencing
the numeric values) to compare the returned path/key against each sidebar item's
id/path for highlighting; ensure tests and type signatures (if any) reflect the
new return type.

In `@src/renderer/controllers/notebooks.controller.ts`:
- Around line 170-188: Replace the stringly-typed cancel flow by introducing and
checking a dedicated sentinel error/type from the service instead of matching
'No file selected': have notebooksService.selectImportFile throw a specific
exported error class or a well-named sentinel (e.g., UserCancelledError or
SELECT_FILE_CANCELLED) and update the mutationFn and onError in
notebooks.controller.ts (the mutationFn that calls
notebooksService.selectImportFile and the onError handler that currently checks
error.message) to detect that sentinel/type (using instanceof or identity) and
skip showing the toast only for that case; apply the same change to the other
import mutation block referenced (the 198-217 section) so both import flows use
the shared sentinel error type.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1141-1145: Replace deprecated onKeyPress handlers with onKeyDown
for Enter key handling: update the JSX where onKeyPress={(e) => { if (e.key ===
'Enter' && newNotebookName.trim()) { handleCreateNotebook(); } }} to use
onKeyDown instead, and perform the same key check and call to
handleCreateNotebook; also update the other occurrences that use onKeyPress (the
similar handlers around the newNotebookName/handleCreateNotebook logic) to
maintain consistent behavior across the component (keep the e.key === 'Enter'
check and trimming logic intact).

In `@src/types/notebooks.ts`:
- Around line 8-9: The exported notebook contract currently exposes data?:
any[]; (and the same any usage on lines 63-64), reduce unsafe any by introducing
a specific shape or a generic type: replace data?: any[] with either data?:
Array<Record<string, unknown>> or make the exported interface generic (e.g.
interface Notebook<T = Record<string, unknown>> { data?: T[]; columns?: string[]
}) and update all usages to supply a concrete T; ensure the properties named
data and columns in this file are updated to the chosen type and any consuming
code is adjusted to the new generic or Record-based type.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f514ac0-d839-4698-811b-22b37c8c202e

📥 Commits

Reviewing files that changed from the base of the PR and between a4702ef and f3b7951.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (43)

• .kiro/steering/github-intructions.md
• package.json
• src/main/ipcHandlers/index.ts
• src/main/ipcHandlers/notebooks.ipcHandlers.ts
• src/main/ipcSetup.ts
• src/main/services/connectors.service.ts
• src/main/services/duckLake.service.ts
• src/main/services/notebooks.service.ts
• src/renderer/App.tsx
• src/renderer/components/customTable/CustomTableHead.tsx
• src/renderer/components/customTable/CustomTablePagination.tsx
• src/renderer/components/customTable/CustomTableRow.tsx
• src/renderer/components/customTable/index.tsx
• src/renderer/components/index.ts
• src/renderer/components/notebook/MarkdownCell.tsx
• src/renderer/components/notebook/NotebookCell.tsx
• src/renderer/components/notebook/NotebookEditor.tsx
• src/renderer/components/notebook/NotebookTabManager.tsx
• src/renderer/components/notebook/NotebookToolbar.tsx
• src/renderer/components/notebook/NotebooksList.tsx
• src/renderer/components/notebook/NotebooksSidebar.tsx
• src/renderer/components/notebook/NotebooksTreeView.tsx
• src/renderer/components/notebook/OutputPanel.tsx
• src/renderer/components/notebook/SQLCell.tsx
• src/renderer/components/notebook/index.ts
• src/renderer/components/queryResult/index.tsx
• src/renderer/components/sidebar/elements.ts
• src/renderer/components/sidebar/icons.tsx
• src/renderer/components/sidebar/index.tsx
• src/renderer/components/sidebar/styles.ts
• src/renderer/controllers/notebooks.controller.ts
• src/renderer/hooks/index.ts
• src/renderer/hooks/useMonacoAutocomplete.ts
• src/renderer/hooks/useNotebookConnectionState.ts
• src/renderer/hooks/useNotebookSidebarState.ts
• src/renderer/hooks/useNotebookTabManager.ts
• src/renderer/hooks/useSchemaForConnection.ts
• src/renderer/layouts/appLayout/styles.ts
• src/renderer/screens/index.ts
• src/renderer/screens/notebooks/index.tsx
• src/renderer/services/notebooks.service.ts
• src/types/ipc.ts
• src/types/notebooks.ts

💤 Files with no reviewable changes (1)

• .kiro/steering/github-intructions.md

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (2)

src/renderer/controllers/notebooks.controller.ts (1)

6-6: ⚠️ Potential issue | 🔴 Critical

React Query API style mismatch with installed package version.

The code imports from 'react-query' (v3 namespace) but uses useMutation({ mutationFn, ... }) object syntax throughout, which is TanStack React Query v4+/v5 style. The v3 API uses useMutation(mutationFn, options) positional arguments.

This mismatch will cause type errors or runtime failures depending on the actual installed version.

#!/bin/bash
# Check installed react-query version and usage pattern
echo "=== Checking package.json for react-query version ==="
rg -n '"react-query"|"@tanstack/react-query"' package.json

echo ""
echo "=== Checking if `@tanstack/react-query` is installed ==="
fd package-lock.json -x sh -c 'rg -n "@tanstack/react-query" "$1" | head -5' sh {}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/controllers/notebooks.controller.ts` at line 6, The file imports
React Query from the old v3 namespace but uses v4+ object-style hooks; update
the import source to the TanStack package to match the object-style API—replace
"react-query" with "@tanstack/react-query" for useQuery, useMutation, and
useQueryClient so the existing useMutation({ mutationFn, ... }) and useQuery({
queryFn, ... }) calls remain valid and types line up with the installed v4+/v5
API.

src/main/services/notebooks.service.ts (1)

1195-1200: ⚠️ Potential issue | 🟠 Major

Restore can silently overwrite an existing notebook in target connection.

fs.writeFile(targetPath, ...) is executed without a collision check. If the target connection already has a notebook with the same ID, data will be replaced without warning.

🔧 Proposed fix

       // Write to target location
       const targetPath = getNotebookPath(targetConnectionKey, notebookId);
+      try {
+        await fs.access(targetPath);
+        throw new Error(`Notebook ${notebookId} already exists in target connection`);
+      } catch (e: any) {
+        if (e.code !== 'ENOENT') throw e;
+      }
       await fs.writeFile(
         targetPath,
         JSON.stringify(notebook, bigIntReplacer, 2),
       );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/services/notebooks.service.ts` around lines 1195 - 1200, The restore
currently calls fs.writeFile(targetPath, ...) and can silently overwrite an
existing notebook; update the restore logic in notebooks.service.ts to detect
collisions by checking for an existing notebook at
getNotebookPath(targetConnectionKey, notebookId) before writing (e.g., use
fs.stat/fs.access or open with exclusive write flag), and if a file exists
return/throw a clear error or prompt/skip per the restore policy instead of
overwriting; ensure the check references targetPath/notebookId and the same
write path used by fs.writeFile so behavior is atomic and consistent.

🧹 Nitpick comments (3)

src/renderer/screens/notebooks/index.tsx (1)

540-600: Debug console.log statements should be removed before merge.

Lines 549-555 contain debug logging (console.log('Exporting notebooks:', ...)) that should be removed for production code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/screens/notebooks/index.tsx` around lines 540 - 600, Remove the
debug console.log that prints notebook cell counts inside
handleExportAllNotebooks (the console.log call that logs 'Exporting notebooks:'
and the preceding eslint-disable-next-line no-console), leaving the rest of the
export logic intact; locate the function handleExportAllNotebooks and delete
that debugging statement (or replace it with a proper production logger if
needed).

src/renderer/components/notebook/NotebookCell.tsx (1)

462-478: Memoization comparator excludes callbacks, risking stale closures.

The areEqual comparator deliberately omits callback props (onRun, onDelete, etc.) to prevent re-renders. However, since NotebookEditor passes inline arrow functions (() => handleRunCell(cell.id, cell.content)), the first-render closures are captured and never updated.

This is partially mitigated by functional state updates (setLocalCells((prev) => ...)) in the handlers, but cell.content captured in onRun could still be stale if the user edits between renders.

Consider either:

1. Including callbacks in the comparator (causes more re-renders), or
2. Making NotebookEditor use stable callback references via useCallback with cell-specific logic inside the callback rather than inline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookCell.tsx` around lines 462 - 478,
The memo comparator for NotebookCell (wrapping NotebookCellComponent) omits
callback props like onRun/onDelete, causing stale closures when NotebookEditor
passes inline arrow functions (e.g., () => handleRunCell(cell.id,
cell.content)); update NotebookEditor to provide stable callback references
using useCallback (e.g., memoize onRun, onDelete, onChange, etc., and compute
cell-specific behavior inside those callbacks using the current cell id/content
or functional state updates) so NotebookCell can safely keep the current
comparator; alternatively, include the callback props in the comparator if you
prefer re-renders instead of stabilizing callbacks.

src/renderer/controllers/notebooks.controller.ts (1)

37-48: Consider implications of disabled caching for notebook details.

cacheTime: 0 and staleTime: 0 disable all caching, meaning every component remount triggers a fresh fetch. This may cause unnecessary network requests when switching tabs or during re-renders.

If the concern is stale data, consider using staleTime: 0 with a non-zero cacheTime to allow background refetches while showing cached data.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/controllers/notebooks.controller.ts` around lines 37 - 48, The
useNotebook hook currently disables all caching (staleTime: 0 and cacheTime: 0)
causing unnecessary fetches on remount; update useNotebook to keep staleTime: 0
but set a non-zero cacheTime (e.g., 300000 ms or another appropriate TTL) so
notebooksService.getNotebook can return cached data between mounts while still
treating it as immediately stale for background refetches—modify the options in
useNotebook (queryKey: notebooksKeys.detail, queryFn:
notebooksService.getNotebook) to use a non-zero cacheTime instead of 0.

🤖 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/components/notebook/NotebookCell.tsx`:
- Around line 126-178: The effect's mousedown handler (handleMouseDown) adds
document-level mousemove/mouseup listeners but the effect cleanup only removes
the mousedown listener, causing mousemove/mouseup to leak if the component
unmounts mid-drag; ensure the effect cleanup also removes any document listeners
and cancels any pending animation frame and resets body styles. Fix by making
handleMouseMove/handleMouseUp removable from the outer scope (e.g., store them
in refs or declare them in the effect scope so cleanup can reference them), and
in the effect's return cleanup remove document.removeEventListener('mousemove',
handleMouseMove) and document.removeEventListener('mouseup', handleMouseUp'),
call window.cancelAnimationFrame(resizeThrottleRef.current) if set, set
resizeThrottleRef.current = null, and restore document.body.style.cursor and
userSelect and call setIsDraggingOutput(false) so no listeners/frames/styles
remain when using outputResizeHandleRef/current, outputHeightRef/current,
resizeThrottleRef, setOutputHeight and setIsDraggingOutput.

In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Line 772: The inline arrow passed to NotebookCell's onRun closes over stale
cell.content and discards the child's content parameter; update the prop so it
forwards the content argument to handleRunCell instead of using cell.content —
e.g., change the onRun callback in NotebookEditor.tsx to accept the content
param and call handleRunCell(cell.id, content) so NotebookCell/SQLCell can pass
editedContent through correctly.
- Around line 898-902: The duplicate-notebook confirm input currently uses the
deprecated onKeyPress handler; replace that with onKeyDown so key events are
handled consistently (like the rename dialog). Update the JSX where onKeyPress
is attached (the handler referencing duplicateNotebookName and calling
handleDuplicateConfirm) to use onKeyDown and keep the same conditional logic
(e.key === 'Enter' && duplicateNotebookName.trim()) invoking
handleDuplicateConfirm.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1136-1140: Replace the deprecated onKeyPress handler with
onKeyDown in the create-notebook input to correctly capture Enter presses:
change the JSX prop usage from onKeyPress to onKeyDown and keep the same logic
that checks e.key === 'Enter' && newNotebookName.trim() to call
handleCreateNotebook(); ensure the handler signature and any referenced
variables (newNotebookName, handleCreateNotebook) remain unchanged so behavior
mirrors the previous implementation.
- Around line 74-91: The hydrated activeConnectionId returned by
useNotebookConnectionState can point at a deleted connection; after hydration
completes (check isConnectionHydrated and notebookTabManager.isHydrated)
validate activeConnectionId against the current connections list and call
setActiveConnectionId(null) (or another safe default) if it is not found;
perform this check in the component initialization/effect where isFullyHydrated
is computed so the UI won’t attempt to load notebooks for a missing connection
and reference useNotebookConnectionState.activeConnectionId and
setActiveConnectionId to implement the clear/reset behavior.

---

Duplicate comments:
In `@src/main/services/notebooks.service.ts`:
- Around line 1195-1200: The restore currently calls fs.writeFile(targetPath,
...) and can silently overwrite an existing notebook; update the restore logic
in notebooks.service.ts to detect collisions by checking for an existing
notebook at getNotebookPath(targetConnectionKey, notebookId) before writing
(e.g., use fs.stat/fs.access or open with exclusive write flag), and if a file
exists return/throw a clear error or prompt/skip per the restore policy instead
of overwriting; ensure the check references targetPath/notebookId and the same
write path used by fs.writeFile so behavior is atomic and consistent.

In `@src/renderer/controllers/notebooks.controller.ts`:
- Line 6: The file imports React Query from the old v3 namespace but uses v4+
object-style hooks; update the import source to the TanStack package to match
the object-style API—replace "react-query" with "@tanstack/react-query" for
useQuery, useMutation, and useQueryClient so the existing useMutation({
mutationFn, ... }) and useQuery({ queryFn, ... }) calls remain valid and types
line up with the installed v4+/v5 API.

---

Nitpick comments:
In `@src/renderer/components/notebook/NotebookCell.tsx`:
- Around line 462-478: The memo comparator for NotebookCell (wrapping
NotebookCellComponent) omits callback props like onRun/onDelete, causing stale
closures when NotebookEditor passes inline arrow functions (e.g., () =>
handleRunCell(cell.id, cell.content)); update NotebookEditor to provide stable
callback references using useCallback (e.g., memoize onRun, onDelete, onChange,
etc., and compute cell-specific behavior inside those callbacks using the
current cell id/content or functional state updates) so NotebookCell can safely
keep the current comparator; alternatively, include the callback props in the
comparator if you prefer re-renders instead of stabilizing callbacks.

In `@src/renderer/controllers/notebooks.controller.ts`:
- Around line 37-48: The useNotebook hook currently disables all caching
(staleTime: 0 and cacheTime: 0) causing unnecessary fetches on remount; update
useNotebook to keep staleTime: 0 but set a non-zero cacheTime (e.g., 300000 ms
or another appropriate TTL) so notebooksService.getNotebook can return cached
data between mounts while still treating it as immediately stale for background
refetches—modify the options in useNotebook (queryKey: notebooksKeys.detail,
queryFn: notebooksService.getNotebook) to use a non-zero cacheTime instead of 0.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 540-600: Remove the debug console.log that prints notebook cell
counts inside handleExportAllNotebooks (the console.log call that logs
'Exporting notebooks:' and the preceding eslint-disable-next-line no-console),
leaving the rest of the export logic intact; locate the function
handleExportAllNotebooks and delete that debugging statement (or replace it with
a proper production logger if needed).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 17e5e327-3d4a-4574-8bfa-50e7bf44a8a2

📥 Commits

Reviewing files that changed from the base of the PR and between f3b7951 and d8e310b.

📒 Files selected for processing (7)

• src/main/services/notebooks.service.ts
• src/renderer/components/notebook/NotebookCell.tsx
• src/renderer/components/notebook/NotebookEditor.tsx
• src/renderer/controllers/notebooks.controller.ts
• src/renderer/hooks/useNotebookConnectionState.ts
• src/renderer/hooks/useNotebookSidebarState.ts
• src/renderer/screens/notebooks/index.tsx

🚧 Files skipped from review as they are similar to previous changes (1)

• src/renderer/hooks/useNotebookSidebarState.ts

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (2)

src/renderer/screens/notebooks/index.tsx (1)

1250-1304: ⚠️ Potential issue | 🟡 Minor

Replace deprecated onKeyPress in rename/duplicate dialogs.

Lines 1250 and 1300 still use onKeyPress; use onKeyDown (with preventDefault) for consistent Enter handling.

Suggested fix

-              onKeyPress={(e) => {
+              onKeyDown={(e) => {
                 if (e.key === 'Enter' && renameNotebookName.trim()) {
+                  e.preventDefault();
                   confirmRenameNotebook();
                 }
               }}
@@
-              onKeyPress={(e) => {
+              onKeyDown={(e) => {
                 if (e.key === 'Enter' && duplicateNotebookName.trim()) {
+                  e.preventDefault();
                   confirmDuplicateNotebook();
                 }
               }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/screens/notebooks/index.tsx` around lines 1250 - 1304, The
dialog TextField handlers use the deprecated onKeyPress; update both to
onKeyDown to handle Enter consistently: replace the onKeyPress handlers for the
rename and duplicate dialogs' TextField components (the ones referencing
renameNotebookName/duplicateNotebookName and calling
confirmRenameNotebook/confirmDuplicateNotebook) with onKeyDown handlers that
check for e.key === 'Enter', call e.preventDefault(), and then invoke
confirmRenameNotebook() or confirmDuplicateNotebook() only if the trimmed name
is non-empty; ensure you keep autoFocus, value, onChange, and required
unchanged.

src/renderer/components/notebook/NotebookCell.tsx (1)

179-183: ⚠️ Potential issue | 🟠 Major

Unmount cleanup still leaves drag-side effects behind.

Line 181 removes listeners via dragCleanupRef, but cleanup does not cancel pending RAF or restore body cursor/selection state in this unmount path.

Suggested fix

     if (resizeHandle) {
       resizeHandle.addEventListener('mousedown', handleMouseDown);
       return () => {
         resizeHandle.removeEventListener('mousedown', handleMouseDown);
         dragCleanupRef.current?.();
+        dragCleanupRef.current = null;
+        if (resizeThrottleRef.current) {
+          window.cancelAnimationFrame(resizeThrottleRef.current);
+          resizeThrottleRef.current = null;
+        }
+        document.body.style.cursor = '';
+        document.body.style.userSelect = '';
       };
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookCell.tsx` around lines 179 - 183, On
unmount the NotebookCell's resize cleanup only removes the mousedown listener
and calls dragCleanupRef.current but doesn't fully undo drag side-effects;
update the cleanup in the effect that references resizeHandle/handleMouseDown to
explicitly cancel any pending requestAnimationFrame (use cancelAnimationFrame on
the stored raf id), restore document.body.style.cursor to its prior value and
clear any selection (document.getSelection().removeAllRanges()), and ensure
dragCleanupRef.current() is invoked and also removes any mousemove/mouseup
listeners it created; reference resizeHandle, handleMouseDown, and
dragCleanupRef to locate and extend the tear-down logic so all RAFs, body cursor
changes, and selection state are cleared on unmount.

🧹 Nitpick comments (7)

src/renderer/components/queryResult/index.tsx (6)

653-653: Type assertion customPagination as any indicates interface mismatch.

This cast suggests the customPagination object shape doesn't match what CustomTable expects. Consider aligning the types or creating a proper type for the pagination interface.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` at line 653, The prop is being
force-cast with customPagination as any because its shape doesn't match
CustomTable's expected pagination type; update the pagination data to conform to
the expected interface or declare a proper type for it and use that instead of
any. Locate the customPagination object and the CustomTable component prop
definition (prop name customPagination) and either (a) adjust the
properties/keys/types on customPagination to match CustomTable's pagination
interface, or (b) export/define a pagination interface/type (e.g.,
PaginationProps) and type the customPagination variable accordingly so you can
pass it without using "as any". Ensure any transform/mapping logic between your
data and CustomTable uses that typed shape.

---

168-173: Unused sorting and keyword state in server-side pagination.

The order, orderBy, and keyword states are exposed in customPagination but fetchPage doesn't include them in the query. This could mislead future developers into expecting server-side sorting/filtering.

If these features aren't planned, consider removing them from the DuckLake pagination path. If planned for later, a TODO comment would clarify intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 168 - 173,
customPagination is exposing order, orderBy and keyword but fetchPage in the
DuckLake pagination flow never uses them; either remove these unused states
(order, orderBy, keyword) from the customPagination return to avoid misleading
API, or wire them into fetchPage's request parameters and backend call so
server-side sorting/filtering works; update the customPagination block
(references: customPagination, fetchPage, order, orderBy, keyword) to either
delete the unused properties or add a clear TODO comment if you intend to
implement server-side sorting/filtering later.

---

71-72: Type assertion (results as any).originalSql indicates missing type definition.

The originalSql property access via type assertion suggests QueryResponseType may be missing this field. Consider extending the type definition in backend.ts to include originalSql?: string for type safety.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 71 - 72, The code
uses (results as any).originalSql because QueryResponseType lacks that property;
add originalSql?: string to the QueryResponseType definition in backend.ts and
update any exported type so the renderer imports the augmented type, then remove
the type assertion in src/renderer/components/queryResult/index.tsx and access
results.originalSql (or use a nullish fallback to exportContext?.originalSql) to
restore type safety; reference the QueryResponseType type in backend.ts and the
originalSql usage in index.tsx to make the change.

---

442-444: Redundant non-null assertions after undefined check.

results.duration! is used even though line 439 already verified results.duration !== undefined. TypeScript should narrow the type, making the ! unnecessary.

-              {results.duration! > 1000
-                ? `${(results.duration! / 1000).toFixed(2)}s`
-                : `${results.duration!}ms`}
+              {results.duration > 1000
+                ? `${(results.duration / 1000).toFixed(2)}s`
+                : `${results.duration}ms`}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 442 - 444, The
conditional rendering uses redundant non-null assertions on results.duration
even though earlier logic already checks results.duration !== undefined; remove
the unnecessary `!` operators so references read `results.duration` (in the JSX
block inside the QueryResult component in
src/renderer/components/queryResult/index.tsx) to rely on TypeScript's type
narrowing and avoid suppressing potential type issues.

---

369-397: Add defensive else clause for unexpected connection types.

While canExportParquet currently guarantees only ducklake or duckdb reaches this point, an else clause would prevent silent failures if the guard logic changes in the future. Without it, isExporting would stay true indefinitely.

🛡️ Defensive else clause

       } else if (
         exportContext.connectionType === 'duckdb' &&
         exportContext.connectionId
       ) {
         // ... duckdb export logic
+      } else {
+        throw new Error(`Unsupported connection type for Parquet export: ${exportContext.connectionType}`);
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 369 - 397, Add a
defensive else branch after the existing ducklake/duckdb branches to handle
unexpected exportContext.connectionType values: detect when neither branch runs
and throw a descriptive Error (including the actual exportContext.connectionType
and any relevant identifiers like exportContext.duckLakeInstanceId/connectionId)
so the caller can catch it and ensure isExporting is cleared; place this else
alongside the existing if/else-if in the export logic that uses
window.electron.ipcRenderer.invoke and variables
duckLakeExportResult/duckDbExportResult so it covers future changes to
canExportParquet guard logic.

---

97-131: Consider adding cleanup to prevent state updates after unmount.

The fetchPage callback doesn't have a cleanup mechanism. If the component unmounts during an in-flight request, the setLoading, setColumns, etc. calls may trigger React warnings about state updates on unmounted components.

♻️ Potential fix using mounted ref

   const fetchSeqRef = React.useRef(0);
+  const mountedRef = React.useRef(true);
+
+  React.useEffect(() => {
+    return () => {
+      mountedRef.current = false;
+    };
+  }, []);

   const fetchPage = React.useCallback(
     async (newPage: number, newPerPage: number) => {
       // ...
       try {
         setLoading(true);
         // ...
-        if (seq !== fetchSeqRef.current) return;
+        if (seq !== fetchSeqRef.current || !mountedRef.current) return;
         setColumns(res.fields?.map((f) => f.name) ?? []);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/queryResult/index.tsx` around lines 97 - 131, The
fetchPage callback can update state after the component unmounts; add a mounted
ref (e.g., mountedRef) that is set to true in useEffect and false in the
cleanup, check mountedRef.current before any setLoading, setFetchError,
setColumns, setRows, setTotalCount calls (and before returning early when seq
mismatches) so in-flight responses don't update unmounted components, and ensure
fetchPage still references mountedRef in its closure (add mountedRef to
dependencies if necessary).

src/renderer/components/notebook/SQLCell.tsx (1)

493-500: Monaco SQL language setup is repeated per cell instance.

setMonarchTokensProvider('sql', ...) is effectively global per language id, but Line 499 can run for every mounted SQLCell. Prefer a module-level one-time registration.

Suggested refactor

+let sqlLanguageConfigured = false;
+
   useEffect(() => {
     loader
       .init()
       .then((monaco) => {
-        if (!monacoConfigured) {
+        if (!sqlLanguageConfigured) {
           defineSQLThemes(monaco);
           configureSQLLanguage(monaco);
-          setMonacoConfigured(true);
+          sqlLanguageConfigured = true;
         }
+        setMonacoConfigured(true);
         return undefined;
       })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/SQLCell.tsx` around lines 493 - 500, The
Monaco SQL setup in useEffect is being run per SQLCell instance; move the
one-time language registration out of the component so
setMonarchTokensProvider('sql', ...) and any global configureSQLLanguage work
only once: create a module-level initialization guard (e.g., a top-level boolean
like sqlMonacoInitialized) and call defineSQLThemes(monaco) and
configureSQLLanguage(monaco) only when that guard is false, then set it true;
keep loader.init() and cell-specific editor setup in useEffect but remove the
global language registration from the per-instance path and reference the
functions configureSQLLanguage/defineSQLThemes and the guard to ensure single
registration.

🤖 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/components/notebook/NotebookCell.tsx`:
- Around line 471-483: The memo comparator for NotebookCell (wrapping
NotebookCellComponent) is missing the notebookId prop, causing stale
notebook-scoped behavior; update the comparator used in the memo call to also
compare prevProps.notebookId === nextProps.notebookId so changes to notebookId
force a re-render (this affects where notebookId is forwarded to OutputPanel).

In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 484-506: The export currently reads from the stale
`notebook.cells` snapshot; change the export to use the current editor state
(e.g., `localCells` or the component's live cell state) so users get the latest
edits—build `exportData` using `localCells` (falling back to `notebook.cells` if
needed), and update the enclosing callback’s dependency array (the useCallback
or handler where export is defined) to include that live state variable
(`localCells` or equivalent) so the export always reflects the most recent
editor content; update references in NotebookEditor (export handler) and any
function names like the export click handler accordingly.
- Around line 401-410: The drag reordering mutates prevCells using
source/destination indices derived from the rendered, sorted list, so when
prevCells isn't already sorted by cell.order you move the wrong item; in
setLocalCells, first derive the current rendered order (e.g., const sorted =
prevCells.slice().sort((a,b)=>a.order-b.order)) or map rendered indices to cell
IDs, then perform the splice on that sorted array (or locate the moved cell by
id using the source index from the rendered list), update each cell.order from
the new sorted array, and return the updated unsorted storage in the same
canonical structure so UI and state remain consistent.

In `@src/renderer/components/notebook/SQLCell.tsx`:
- Around line 565-600: The mousedown handler (handleMouseDown) registers
document-level mousemove/mouseup and mutates document.body styles but the effect
cleanup only removes the mousedown from resizeHandleRef, so if the component
unmounts during a drag the document listeners and cursor/userSelect stay active;
fix by ensuring the effect's cleanup always removes the document event listeners
and resets document.body.style.cursor/userSelect and setIsDragging(false) — make
handleMouseMove and handleMouseUp accessible to cleanup (e.g., declare them
outside/with refs or store them on a local variable within useEffect) so the
returned cleanup function removes document.removeEventListener('mousemove',
handleMouseMove) and document.removeEventListener('mouseup', handleMouseUp),
resets document.body.style.cursor = '' and document.body.style.userSelect = '',
and clears dragging state; keep using resizeHandleRef for attaching/detaching
the mousedown listener.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 94-106: The effect in useEffect that clears activeConnectionId
runs too early because it checks connections and duckLakeInstances immediately
after isConnectionHydrated, causing a valid persisted activeConnectionId to be
reset when those lists are still empty; update the effect to also wait for the
connection lists to finish loading (e.g., add and use explicit booleans like
areConnectionsLoaded and areDuckLakeInstancesLoaded or a combined
isConnectionsReady flag) before validating: only compute connectionExists
against connections and duckLakeInstances once those loading flags are true, and
otherwise skip clearing by returning early; keep references to
isConnectionHydrated, activeConnectionId, connections, duckLakeInstances, and
setActiveConnectionId to locate the change.

---

Duplicate comments:
In `@src/renderer/components/notebook/NotebookCell.tsx`:
- Around line 179-183: On unmount the NotebookCell's resize cleanup only removes
the mousedown listener and calls dragCleanupRef.current but doesn't fully undo
drag side-effects; update the cleanup in the effect that references
resizeHandle/handleMouseDown to explicitly cancel any pending
requestAnimationFrame (use cancelAnimationFrame on the stored raf id), restore
document.body.style.cursor to its prior value and clear any selection
(document.getSelection().removeAllRanges()), and ensure dragCleanupRef.current()
is invoked and also removes any mousemove/mouseup listeners it created;
reference resizeHandle, handleMouseDown, and dragCleanupRef to locate and extend
the tear-down logic so all RAFs, body cursor changes, and selection state are
cleared on unmount.

In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1250-1304: The dialog TextField handlers use the deprecated
onKeyPress; update both to onKeyDown to handle Enter consistently: replace the
onKeyPress handlers for the rename and duplicate dialogs' TextField components
(the ones referencing renameNotebookName/duplicateNotebookName and calling
confirmRenameNotebook/confirmDuplicateNotebook) with onKeyDown handlers that
check for e.key === 'Enter', call e.preventDefault(), and then invoke
confirmRenameNotebook() or confirmDuplicateNotebook() only if the trimmed name
is non-empty; ensure you keep autoFocus, value, onChange, and required
unchanged.

---

Nitpick comments:
In `@src/renderer/components/notebook/SQLCell.tsx`:
- Around line 493-500: The Monaco SQL setup in useEffect is being run per
SQLCell instance; move the one-time language registration out of the component
so setMonarchTokensProvider('sql', ...) and any global configureSQLLanguage work
only once: create a module-level initialization guard (e.g., a top-level boolean
like sqlMonacoInitialized) and call defineSQLThemes(monaco) and
configureSQLLanguage(monaco) only when that guard is false, then set it true;
keep loader.init() and cell-specific editor setup in useEffect but remove the
global language registration from the per-instance path and reference the
functions configureSQLLanguage/defineSQLThemes and the guard to ensure single
registration.

In `@src/renderer/components/queryResult/index.tsx`:
- Line 653: The prop is being force-cast with customPagination as any because
its shape doesn't match CustomTable's expected pagination type; update the
pagination data to conform to the expected interface or declare a proper type
for it and use that instead of any. Locate the customPagination object and the
CustomTable component prop definition (prop name customPagination) and either
(a) adjust the properties/keys/types on customPagination to match CustomTable's
pagination interface, or (b) export/define a pagination interface/type (e.g.,
PaginationProps) and type the customPagination variable accordingly so you can
pass it without using "as any". Ensure any transform/mapping logic between your
data and CustomTable uses that typed shape.
- Around line 168-173: customPagination is exposing order, orderBy and keyword
but fetchPage in the DuckLake pagination flow never uses them; either remove
these unused states (order, orderBy, keyword) from the customPagination return
to avoid misleading API, or wire them into fetchPage's request parameters and
backend call so server-side sorting/filtering works; update the customPagination
block (references: customPagination, fetchPage, order, orderBy, keyword) to
either delete the unused properties or add a clear TODO comment if you intend to
implement server-side sorting/filtering later.
- Around line 71-72: The code uses (results as any).originalSql because
QueryResponseType lacks that property; add originalSql?: string to the
QueryResponseType definition in backend.ts and update any exported type so the
renderer imports the augmented type, then remove the type assertion in
src/renderer/components/queryResult/index.tsx and access results.originalSql (or
use a nullish fallback to exportContext?.originalSql) to restore type safety;
reference the QueryResponseType type in backend.ts and the originalSql usage in
index.tsx to make the change.
- Around line 442-444: The conditional rendering uses redundant non-null
assertions on results.duration even though earlier logic already checks
results.duration !== undefined; remove the unnecessary `!` operators so
references read `results.duration` (in the JSX block inside the QueryResult
component in src/renderer/components/queryResult/index.tsx) to rely on
TypeScript's type narrowing and avoid suppressing potential type issues.
- Around line 369-397: Add a defensive else branch after the existing
ducklake/duckdb branches to handle unexpected exportContext.connectionType
values: detect when neither branch runs and throw a descriptive Error (including
the actual exportContext.connectionType and any relevant identifiers like
exportContext.duckLakeInstanceId/connectionId) so the caller can catch it and
ensure isExporting is cleared; place this else alongside the existing if/else-if
in the export logic that uses window.electron.ipcRenderer.invoke and variables
duckLakeExportResult/duckDbExportResult so it covers future changes to
canExportParquet guard logic.
- Around line 97-131: The fetchPage callback can update state after the
component unmounts; add a mounted ref (e.g., mountedRef) that is set to true in
useEffect and false in the cleanup, check mountedRef.current before any
setLoading, setFetchError, setColumns, setRows, setTotalCount calls (and before
returning early when seq mismatches) so in-flight responses don't update
unmounted components, and ensure fetchPage still references mountedRef in its
closure (add mountedRef to dependencies if necessary).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a443db6-af9e-4946-a2ef-2b62e1457008

📥 Commits

Reviewing files that changed from the base of the PR and between d8e310b and 6abac22.

📒 Files selected for processing (6)

• src/renderer/components/notebook/NotebookCell.tsx
• src/renderer/components/notebook/NotebookEditor.tsx
• src/renderer/components/notebook/OutputPanel.tsx
• src/renderer/components/notebook/SQLCell.tsx
• src/renderer/components/queryResult/index.tsx
• src/renderer/screens/notebooks/index.tsx

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the NotebookCell component and its memo comparator
cat -n src/renderer/components/notebook/NotebookCell.tsx | sed -n '465,495p'

Repository: rosettadb/dbt-studio

Length of output: 952

---

🏁 Script executed:

# Check the OutputPanel component to verify notebookId usage
cat -n src/renderer/components/notebook/NotebookCell.tsx | sed -n '410,425p'

Repository: rosettadb/dbt-studio

Length of output: 754

---

🏁 Script executed:

# Let's also check the component props interface to see what props are expected
rg -A 20 'interface.*NotebookCellProps|type.*NotebookCellProps' src/renderer/components/notebook/NotebookCell.tsx

Repository: rosettadb/dbt-studio

Length of output: 654

---

🏁 Script executed:

# Check OutputPanel component to understand its notebookId usage
rg -B 5 -A 20 'OutputPanel' src/renderer/components/notebook/NotebookCell.tsx | head -80

Repository: rosettadb/dbt-studio

Length of output: 1845

---

memo comparator omits notebookId, risking stale notebook-scoped behavior.

The component accepts notebookId as a prop and passes it to OutputPanel, but the memo comparator at line 476+ does not include it in its equality check. This means when notebookId changes while other compared props remain the same, the component will skip re-rendering and retain stale notebook context.

Add notebookId to the comparator:

Fix

     return (
       prevProps.cell.id === nextProps.cell.id &&
       prevProps.cell.content === nextProps.cell.content &&
       prevProps.cell.output === nextProps.cell.output &&
       prevProps.index === nextProps.index &&
       prevProps.isExecuting === nextProps.isExecuting &&
-      prevProps.connectionId === nextProps.connectionId
+      prevProps.connectionId === nextProps.connectionId &&
+      prevProps.notebookId === nextProps.notebookId
     );

📝 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

	export const NotebookCell = memo(
	NotebookCellComponent,
	(prevProps, nextProps) => {
	// Return true if props are equal (skip re-render)
	// Return false if props are different (re-render)
	return (
	prevProps.cell.id === nextProps.cell.id &&
	prevProps.cell.content === nextProps.cell.content &&
	prevProps.cell.output === nextProps.cell.output &&
	prevProps.index === nextProps.index &&
	prevProps.isExecuting === nextProps.isExecuting &&
	prevProps.connectionId === nextProps.connectionId
	);
	export const NotebookCell = memo(
	NotebookCellComponent,
	(prevProps, nextProps) => {
	// Return true if props are equal (skip re-render)
	// Return false if props are different (re-render)
	return (
	prevProps.cell.id === nextProps.cell.id &&
	prevProps.cell.content === nextProps.cell.content &&
	prevProps.cell.output === nextProps.cell.output &&
	prevProps.index === nextProps.index &&
	prevProps.isExecuting === nextProps.isExecuting &&
	prevProps.connectionId === nextProps.connectionId &&
	prevProps.notebookId === nextProps.notebookId
	);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookCell.tsx` around lines 471 - 483,
The memo comparator for NotebookCell (wrapping NotebookCellComponent) is missing
the notebookId prop, causing stale notebook-scoped behavior; update the
comparator used in the memo call to also compare prevProps.notebookId ===
nextProps.notebookId so changes to notebookId force a re-render (this affects
where notebookId is forwarded to OutputPanel).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Drag reorder is applied to a different ordering than the UI list.

Line 401 reorders prevCells directly, but drag indices come from the sorted render order (Lines 740-742). If prevCells order diverges from cell.order, the wrong cell is moved.

Suggested fix

-      setLocalCells((prevCells) => {
-        const reorderedCells = Array.from(prevCells);
+      setLocalCells((prevCells) => {
+        // Match the same order used by the rendered draggable list
+        const reorderedCells = [...prevCells].sort((a, b) => a.order - b.order);
         const [movedCell] = reorderedCells.splice(source.index, 1);
         reorderedCells.splice(destination.index, 0, movedCell);

         // Update order property
         const updatedCells = reorderedCells.map((cell, index) => ({
           ...cell,
           order: index,
         }));

📝 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

	setLocalCells((prevCells) => {
	const reorderedCells = Array.from(prevCells);
	const [movedCell] = reorderedCells.splice(source.index, 1);
	reorderedCells.splice(destination.index, 0, movedCell);
	// Update order property
	const updatedCells = reorderedCells.map((cell, index) => ({
	...cell,
	order: index,
	}));
	setLocalCells((prevCells) => {
	// Match the same order used by the rendered draggable list
	const reorderedCells = [...prevCells].sort((a, b) => a.order - b.order);
	const [movedCell] = reorderedCells.splice(source.index, 1);
	reorderedCells.splice(destination.index, 0, movedCell);
	// Update order property
	const updatedCells = reorderedCells.map((cell, index) => ({
	...cell,
	order: index,
	}));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookEditor.tsx` around lines 401 - 410,
The drag reordering mutates prevCells using source/destination indices derived
from the rendered, sorted list, so when prevCells isn't already sorted by
cell.order you move the wrong item; in setLocalCells, first derive the current
rendered order (e.g., const sorted =
prevCells.slice().sort((a,b)=>a.order-b.order)) or map rendered indices to cell
IDs, then perform the splice on that sorted array (or locate the moved cell by
id using the source index from the rendered list), update each cell.order from
the new sorted array, and return the updated unsorted storage in the same
canonical structure so UI and state remain consistent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Export uses server snapshot instead of current editor state.

Line 486 exports notebook.cells, which can lag behind localCells during debounced updates. Users can export stale content right after edits.

Suggested fix

-    const exportData = {
-      ...notebook,
-      cells: notebook.cells.map((cell) => ({
+    const exportData = {
+      ...notebook,
+      cells: [...localCells].sort((a, b) => a.order - b.order).map((cell) => ({
         ...cell,
         output: cell.output
           ? {
               ...cell.output,
               data: [], // Remove data array to reduce file size
             }
           : undefined,
       })),
     };
@@
-  }, [notebook]);
+  }, [notebook, localCells]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/NotebookEditor.tsx` around lines 484 - 506,
The export currently reads from the stale `notebook.cells` snapshot; change the
export to use the current editor state (e.g., `localCells` or the component's
live cell state) so users get the latest edits—build `exportData` using
`localCells` (falling back to `notebook.cells` if needed), and update the
enclosing callback’s dependency array (the useCallback or handler where export
is defined) to include that live state variable (`localCells` or equivalent) so
the export always reflects the most recent editor content; update references in
NotebookEditor (export handler) and any function names like the export click
handler accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Resize drag listeners can leak on unmount during active drag.

Cleanup at Line 595 removes only the mousedown handler. If unmount happens before mouseup, document listeners and cursor/user-select styles can remain stuck.

Suggested fix

+  const dragCleanupRef = useRef<(() => void) | null>(null);

   useEffect(() => {
     const handleMouseDown = (e: MouseEvent) => {
@@
       const handleMouseUp = () => {
         document.removeEventListener('mousemove', handleMouseMove);
         document.removeEventListener('mouseup', handleMouseUp);
         document.body.style.cursor = '';
         document.body.style.userSelect = '';
         setIsDragging(false);
+        dragCleanupRef.current = null;
       };

       document.addEventListener('mousemove', handleMouseMove);
       document.addEventListener('mouseup', handleMouseUp);
       document.body.style.cursor = 'row-resize';
       document.body.style.userSelect = 'none';
+      dragCleanupRef.current = () => {
+        document.removeEventListener('mousemove', handleMouseMove);
+        document.removeEventListener('mouseup', handleMouseUp);
+        document.body.style.cursor = '';
+        document.body.style.userSelect = '';
+      };
     };

@@
       return () => {
         resizeHandle.removeEventListener('mousedown', handleMouseDown);
+        dragCleanupRef.current?.();
+        dragCleanupRef.current = null;
       };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/notebook/SQLCell.tsx` around lines 565 - 600, The
mousedown handler (handleMouseDown) registers document-level mousemove/mouseup
and mutates document.body styles but the effect cleanup only removes the
mousedown from resizeHandleRef, so if the component unmounts during a drag the
document listeners and cursor/userSelect stay active; fix by ensuring the
effect's cleanup always removes the document event listeners and resets
document.body.style.cursor/userSelect and setIsDragging(false) — make
handleMouseMove and handleMouseUp accessible to cleanup (e.g., declare them
outside/with refs or store them on a local variable within useEffect) so the
returned cleanup function removes document.removeEventListener('mousemove',
handleMouseMove) and document.removeEventListener('mouseup', handleMouseUp),
resets document.body.style.cursor = '' and document.body.style.userSelect = '',
and clears dragging state; keep using resizeHandleRef for attaching/detaching
the mousedown listener.