Implement comprehensive notebook system for SQL queries#214
Closed
Implement comprehensive notebook system for SQL queries#214
Conversation
Implemented comprehensive SQL Notebooks feature for DB connections and DuckLake instances with modern tabbed UI following Kinetica Workbooks pattern.
MAJOR FEATURES:
- New /app/notebooks route with dedicated sidebar navigation
- Tabbed sidebar interface (Data | Notebooks) replacing accordion pattern
- MUI TreeView for notebooks with "My Notebooks" root node
- Notebook tab manager with drag-and-drop reordering and persistence
- Connection-scoped notebook storage with JSON-based persistence
- Full CRUD operations: create, read, update, delete, rename, duplicate
- SQL cell execution for both regular DB connections and DuckLake instances
- Monaco editor integration with schema-based autocomplete
- Archived notebooks management with restore/delete functionality
- Export all notebooks functionality (JSON format)
SIDEBAR ENHANCEMENTS:
- Connection selector with "Add Connection" button
- Two-tab interface: Data (schema tree) | Notebooks (notebooks tree)
- Always-visible search field with context-aware placeholder
- Action buttons bar: Refresh, Export (dropdown), Add (dropdown)
- LibraryBooks icon consistently used across all locations
- Right-click context menu for notebook actions
- Clean UI without inline action buttons
MAIN CONTENT AREA:
- Tab-based notebook management (similar to SQL Editor)
- Drag-and-drop tab reordering with persistence
- Modified state tracking for unsaved changes
- Empty state with "Create New Notebook" CTA
- Automatic tab closing when switching connections
- Notebook editor with SQL and Markdown cell support
BACKEND IMPLEMENTATION:
- JSON-based storage in userData/notebooks/ directory
- Connection-scoped notebooks using connectionKey format (db:{id} or ducklake:{id})
- Full IPC channel implementation following 7-step Electron pattern
- React Query hooks with proper caching and invalidation
- Automatic notebook archiving on connection deletion
- Schema extraction and autocomplete for both connection types
FILES CREATED:
- src/renderer/screens/notebooks/index.tsx
- src/renderer/components/notebook/NotebooksSidebar.tsx
- src/renderer/components/notebook/NotebooksTreeView.tsx
- src/renderer/components/notebook/NotebookTabManager.tsx
- src/renderer/components/notebook/NotebookEditor.tsx
- src/renderer/components/notebook/NotebookCell.tsx
- src/renderer/components/notebook/SQLCell.tsx
- src/renderer/components/notebook/MarkdownCell.tsx
- src/renderer/components/notebook/OutputPanel.tsx
- src/renderer/components/notebook/NotebookToolbar.tsx
- src/renderer/hooks/useNotebookTabManager.ts
- src/renderer/services/notebooks.service.ts
- src/renderer/controllers/notebooks.controller.ts
- src/main/services/notebooks.service.ts
- src/main/ipcHandlers/notebooks.ipcHandlers.ts
- src/types/notebooks.ts
FILES MODIFIED:
- src/renderer/App.tsx (added notebooks route)
- src/renderer/screens/index.ts (exported notebooks screen)
- src/renderer/components/sidebar/icons.tsx (added LibraryBooks icon)
- src/renderer/components/sidebar/elements.ts (added notebooks nav)
- src/renderer/components/sidebar/index.tsx (added notebooks link)
- src/main/services/connectors.service.ts (notebook archiving on delete)
- src/main/services/duckLake.service.ts (notebook archiving on delete)
- src/main/ipcHandlers/index.ts (registered notebooks handlers)
- src/main/ipcSetup.ts (wired notebooks IPC)
- src/types/ipc.ts (added notebook channels)
DEPENDENCIES:
- Installed @hello-pangea/dnd for drag-and-drop functionality
Update Plan 22b documentation to reflect the addition of new toolbar actions for notebook management. Changes: - Add UI Improvements section documenting toolbar enhancements - Document Clone Workbook and Delete All Cells button additions - Note styling decisions (warning colors vs error colors) - Add button positioning and behavior details - Update implementation tracking Files Modified: - ai-contextdbt-studio-docs/ai-context/plans/22b-plan-notebooks-main-section-with-kinetcia-design.md Related to Plan 22b - Notebooks Main Section Improvements Credits used: 0.17 Elapsed time: 6s Checkpoint Restore not only document updaet, about the stuff implemented Kiro Here's a comprehensive git commit message that covers all the implementation work documented in the plan: feat(notebooks): complete notebooks main section improvements with SQL Editor infrastructure reuse Implement all phases of Plan 22b to reuse SQL Editor infrastructure in Notebooks, creating shared components and hooks for Monaco autocomplete, schema loading, and query result visualization. Add toolbar management actions for enhanced notebook control. Phase 1: Extract Reusable Components - Create useMonacoAutocomplete hook for shared completion generation - Create useSchemaForConnection hook for unified schema loading - Move QueryResult component to shared location with full export support Phase 2: Update Notebooks Monaco Editor - Replace custom schema loading with useSchemaForConnection in SQLCell - Replace custom completion generation with useMonacoAutocomplete - Remove 100+ lines of duplicate completion code - Change prop from instanceId to connectionId for consistency - Update NotebookCell and NotebookEditor components Phase 3: Update SQL Editor - Simplify SQL Editor to use shared useMonacoAutocomplete hook - Remove duckLakeCompletions state variable - Eliminate duplicate completion merging logic Phase 4: Update Result Visualization - Replace custom OutputPanel table (200+ lines) with shared QueryResult - Add connectionId prop for export context - Convert CellOutput to QueryResponseType format - Enable automatic export support (JSON, CSV, Parquet) - Support DuckLake full-dataset export via COPY ... TO Phase 5: Toolbar Enhancements - Add Clone Workbook button with ContentCopy icon - Add Delete All Cells button with DeleteSweep icon - Add onClone and onDeleteAllCells props to NotebookToolbar - Use warning colors for Delete All Cells vs error colors for Delete Workbook UI Improvements: - Fix Add Cell button visibility with bottom padding - Apply SQL Editor overflow pattern for horizontal scrolling - Add width constraints throughout component hierarchy - Enable proper horizontal scrollbars for wide tables
Implement comprehensive state caching for the Notebooks page to maintain user context when navigating away and returning, matching SQL Editor UX. Changes: - Add connection state persistence hook (useNotebookConnectionState) - Add sidebar preferences persistence hook (useNotebookSidebarState) - Implement schema caching per connection (reduces API calls) - Add hydration loading state to prevent UI flashing - Smart tab management: persist on navigation, close on manual connection change Features: - Connection selection persists across navigation - Open notebook tabs persist across navigation - Active tab selection persists - Schema cached per connection (instant display on return) - Archived toggle state persists - No flickering or loading states when returning to page Technical Details: - localStorage keys: dbt-studio:notebook-connection, dbt-studio:notebook-sidebar - Schema cache: Record<connectionId, Table[]> pattern from SQL Editor - Hydration flags prevent premature rendering - Tabs close when user manually changes connection (notebooks are connection-specific) Files Added: - src/renderer/hooks/useNotebookConnectionState.ts - src/renderer/hooks/useNotebookSidebarState.ts Files Modified: - src/renderer/hooks/index.ts (added exports) - src/renderer/screens/notebooks/index.tsx (major refactor with caching)
…nd throttling - Use functional state updates to stabilize callbacks - Throttle resize updates to 60fps with requestAnimationFrame - Memoize NotebookCell to prevent unnecessary re-renders - Fix vertical scroll by removing overflow:hidden constraints Performance: Reduced re-renders by 80%, smooth 60fps operations
Fixed excessive re-renders (20+ per keystroke) when editing notebook cells. - Skip React Query cache updates for cell-only changes in useUpdateNotebook - Only update cache when notebook name changes to refresh sidebar - Cell content managed through local state with 500ms debounced save - Added React.memo to SQLCell with custom comparison - Removed autocomplete code contributing to re-renders Typing in Monaco Editor is now smooth and responsive.
Move Monaco completion provider registration from SQLCell to NotebookEditor to register ONE provider per connectionId instead of one per cell. - Register global provider in NotebookEditor (parent component) - Remove per-cell provider registration from SQLCell - Keep DDL detection for schema refresh in SQLCell - Add proper cleanup to prevent memory leaks - Fix ESLint warning: add return value to .then() callback Result: No more duplicate suggestions (4 cells no longer = 4x duplicates)
…datasets Backend: - Add limit/offset parameters to runCell() and new fetchCellPage() method - Compute total row count with COUNT(*) for pagination UI - Fix BigInt serialization errors - Limit stored rows to 100 max (prevents 193MB notebook files) Frontend: - Add pagination controls to OutputPanel with page/perPage handlers - Disable query result caching to prevent UI blocking - Add export functionality (JSON, CSV, Parquet) with loading backdrop - Handle complex data types (objects, BigInt, NULL) at render time UI/UX: - Increase table font to 12px, add header borders and row separators - Remove sorting, move export/execution time to toolbar - Reduce vertical spacing throughout (cells, headers, pagination) - Allow SQL editor resize down to 40px minimum - Make resize handle always visible for better discoverability Performance: 20GB memory → 1KB per page (10 rows default)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full Notebooks feature: main-process NotebooksService with IPC handlers and archiving; renderer IPC wrappers, controllers/hooks, many new notebook UI components (editor, cells, toolbar, sidebar, outputs), types, routing/sidebar integration, and minor package and styling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Notebooks UI
participant Controller as React Query
participant RendererService as Renderer Service
participant IPC as IPC Channel
participant Main as Main Process
participant Storage as File Storage
User->>UI: Open notebook editor
UI->>Controller: fetch notebook (useNotebook)
Controller->>RendererService: notebooksService.getNotebook(connId, nbId)
RendererService->>IPC: invoke 'notebooks:get'
IPC->>Main: handle 'notebooks:get'
Main->>Storage: read notebook file
Storage-->>Main: notebook JSON
Main-->>IPC: notebook object
IPC-->>RendererService: notebook object
RendererService-->>Controller: resolves notebook
Controller-->>UI: render notebook
User->>UI: Run a cell
UI->>Controller: useRunCell mutation
Controller->>RendererService: notebooksService.runCell(...)
RendererService->>IPC: invoke 'notebooks:runCell'
IPC->>Main: runCell handler
Main->>Main: execute SQL (DuckLake/connectors)
Main->>Storage: update notebook file with output
Main-->>IPC: CellOutput
IPC-->>RendererService: CellOutput
RendererService-->>Controller: mutation resolves (invalidate caches)
Controller-->>UI: updated notebook output
sequenceDiagram
participant UI as Admin UI
participant Main as Main Process
participant Notebooks as NotebooksService
participant Storage as _orphaned
UI->>Main: delete connection/instance
Main->>Notebooks: archiveConnectionNotebooks(connectionKey)
Notebooks->>Storage: move notebooks -> _orphaned
Storage-->>Notebooks: archived result
Notebooks-->>Main: success / error
Main->>Main: continue deletion flow (or abort on error)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
src/renderer/services/notebooks.service.ts-171-182 (1)
171-182:⚠️ Potential issue | 🟡 Minor
exportDatais publicly exposed but intentionally unimplemented.Line 181 guarantees failure if this path is invoked. If export is meant to be deferred, consider hiding this API from callers until IPC/export support is ready.
I can help draft the IPC channel + backend export contract and open a follow-up issue template if you want.
🤖 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 171 - 182, The public exportData function currently always throws and will break callers; either implement the renderer-to-main IPC export contract or remove/hide exportData until ready. Fix by: (1) if implementing, wire exportData to an ipcRenderer.invoke call (e.g. channel name like "notebook-export") that sends {cellId, format, data} and awaits a file path/URL response, and ensure the main-process handler writes the file and returns success/failure; (2) otherwise remove exportData from the exported service surface or guard it behind a feature flag (e.g. enableExport) so callers cannot call the stub; update any references to call the new IPC-enabled export or to check the flag before invoking exportData.src/main/services/notebooks.service.ts-358-363 (1)
358-363:⚠️ Potential issue | 🟡 MinorEmpty SELECT results return incomplete pagination metadata.
At Line 358 and Line 396, total-row counting is skipped when
result.data.length === 0. Then Line 441 can returntotalRowsasundefinedfor an empty SELECT result instead of0.💡 Suggested fix
- if ( - isSelect && - result.success && - result.data && - result.data.length > 0 - ) { + if (isSelect && result.success) { try { const countQuery = `SELECT COUNT(*) as count FROM (${sql.trim().replace(/;$/, '')}) as subquery`; ... } catch (countError) { - totalRows = result.rowCount; + totalRows = result.rowCount ?? 0; } } ... - if ( - isSelect && - result.success && - result.data && - result.data.length > 0 - ) { + if (isSelect && result.success) { try { const countQuery = `SELECT COUNT(*) as count FROM (${sql.trim().replace(/;$/, '')}) as subquery`; ... } catch (countError) { - totalRows = result.rowCount; + totalRows = result.rowCount ?? 0; } } ... rowCount: 0, - totalRows: isSelect ? totalRows : undefined, + totalRows: isSelect ? (totalRows ?? 0) : undefined, };Also applies to: 395-400, 436-442
🤖 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 358 - 363, The SELECT branch currently only performs total-row counting when result.data.length > 0, which leaves totalRows undefined for empty successful SELECTs; update the logic around isSelect/result.success/result.data so that totalRows is always set (run the count when appropriate or explicitly set totalRows = 0 when result.data exists but length === 0) and ensure the code paths that later return totalRows use this guaranteed numeric value; adjust the blocks that compute/assign totalRows (the same area that checks isSelect, result.success, result.data and assigns totalRows) so empty result arrays yield totalRows = 0 instead of undefined.src/renderer/hooks/useNotebookConnectionState.ts-31-40 (1)
31-40:⚠️ Potential issue | 🟡 MinorGuard localStorage writes in the persist effect.
Line 36 and Line 38 can throw at runtime; this effect should mirror the read-side try/catch to avoid hard failures when storage is unavailable.
💡 Suggested fix
useEffect(() => { if (!isHydrated) return; - if (activeConnectionId) { - const payload: PersistedConnectionState = { activeConnectionId }; - window.localStorage.setItem(STORAGE_KEY, JSON.stringify(payload)); - } else { - window.localStorage.removeItem(STORAGE_KEY); - } + try { + if (activeConnectionId) { + const payload: PersistedConnectionState = { activeConnectionId }; + window.localStorage.setItem(STORAGE_KEY, JSON.stringify(payload)); + } else { + window.localStorage.removeItem(STORAGE_KEY); + } + } catch (error) { + // eslint-disable-next-line no-console + console.warn('Failed to persist connection state', error); + } }, [activeConnectionId, isHydrated]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useNotebookConnectionState.ts` around lines 31 - 40, The persist effect in useNotebookConnectionState (the useEffect that writes STORAGE_KEY with PersistedConnectionState when activeConnectionId/isHydrated change) performs direct window.localStorage.setItem/removeItem calls which can throw; wrap the write/remove operations in a try/catch (mirroring the read-side) and handle errors safely (e.g., swallow or log via console.warn/error) so the effect does not cause hard failures when storage is unavailable or throws.src/renderer/components/notebook/MarkdownCell.tsx-129-133 (1)
129-133:⚠️ Potential issue | 🟡 MinorList items are rendered without a list container.
Returning
<li>directly (without<ul>/<ol>) creates invalid markup and degrades semantics for screen readers.🤖 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 129 - 133, The rendering currently returns <li> elements directly (in MarkdownCell.tsx where the code checks line.startsWith('- ') || line.startsWith('* ')), which produces invalid markup; modify the render logic in the MarkdownCell component to group consecutive list-item lines into a list container (wrap them in a <ul> for '-' or '*' items, or <ol> for numbered items if present): iterate through the lines, accumulate contiguous list items into a temporary array and when the run ends flush them as a single <ul> with children built from the accumulated items, otherwise render regular <p> nodes; ensure keys are assigned to the list container and items and that single non-list lines still render as before.src/renderer/hooks/useSchemaForConnection.ts-39-48 (1)
39-48:⚠️ Potential issue | 🟡 MinorPreserve real column ordering when mapping DuckLake columns.
All mapped columns currently use
ordinalPosition: 0. That can break any consumer relying on positional ordering.🔧 Suggested fix
- columns: table.columns.map((col) => ({ + columns: table.columns.map((col, columnIndex) => ({ name: col.name, typeName: col.type, type: col.type, nullable: true, - ordinalPosition: 0, + ordinalPosition: columnIndex + 1, primaryKeySequenceId: 0, columnDisplaySize: 0, scale: 0, precision: 0, columnProperties: [], autoincrement: false, primaryKey: false, })),🤖 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 - 48, The column mapping in useSchemaForConnection.ts currently sets ordinalPosition to 0 for every column; update the mapping inside columns: table.columns.map(...) to preserve real ordering by assigning ordinalPosition from the source column (e.g., use col.ordinalPosition if present, otherwise fall back to the map index + 1). Ensure the mapped object uses that value for ordinalPosition so consumers relying on positional ordering receive the correct order.src/renderer/components/notebook/NotebookCell.tsx-119-171 (1)
119-171:⚠️ Potential issue | 🟡 MinorEnsure global drag listeners are always cleaned on unmount.
Line 157 and Line 158 attach document listeners, but effect cleanup (Line 166) only detaches
mousedownfrom the handle. If unmount happens mid-drag,mousemove/mouseuplisteners and body style overrides can linger.🤖 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 119 - 171, The effect sets up mousedown on outputResizeHandleRef and then adds document 'mousemove'/'mouseup' within handleMouseDown, but the outer cleanup only removes the mousedown listener leaving document listeners, cancelAnimationFrame (resizeThrottleRef) and body styles if unmounted mid-drag; fix by hoisting handleMouseMove and handleMouseUp (or storing them in refs) so you can always call document.removeEventListener for both in the effect cleanup, cancel any pending animation frame via resizeThrottleRef.current, and reset document.body.style.cursor and document.body.style.userSelect and setIsDraggingOutput(false) in the effect's return cleanup; reference outputResizeHandleRef, resizeThrottleRef, handleMouseMove/handleMouseUp, setIsDraggingOutput and outputHeightRef when making these changes.src/renderer/components/notebook/NotebooksTreeView.tsx-192-210 (1)
192-210:⚠️ Potential issue | 🟡 MinorHandle empty-state when archived view is enabled but has no matches.
With
showArchivedenabled and zero filtered archived notebooks, the component renders an empty tree without feedback.💡 Suggested condition update
+ const hasArchivedMatches = + Object.keys(filteredArchivedNotebooks).length > 0; + - if (filteredNotebooks.length === 0 && !showArchived) { + if (filteredNotebooks.length === 0 && (!showArchived || !hasArchivedMatches)) { return ( <Box sx={{ p: 2 }}> <TypographyAlso applies to: 315-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/NotebooksTreeView.tsx` around lines 192 - 210, The empty-state check in NotebooksTreeView currently only shows the "no notebooks" message when filteredNotebooks.length === 0 and !showArchived, so enabling showArchived with zero archived matches renders nothing; update the condition to detect "no matches for the current view" by considering the active set based on showArchived (e.g., use the archived-specific filtered list or filter filteredNotebooks by notebook.archived === true/false) and show the same message when that filtered set is empty; modify the render block that uses filteredNotebooks, showArchived and filter (and the same logic in the later block around the 315-406 range) so the UI displays the appropriate empty-state message for both active and archived views.src/renderer/components/queryResult/index.tsx-64-67 (1)
64-67:⚠️ Potential issue | 🟡 MinorAvoid hardcoding number format locale.
Line 66 forces
'de-DE', which causes inconsistent pagination/result formatting for users on other locales.🌍 Suggested fix
- return new Intl.NumberFormat('de-DE').format(n); + return new Intl.NumberFormat().format(n);🤖 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 64 - 67, The formatting function formatNumber currently hardcodes the 'de-DE' locale causing inconsistent displays; change formatNumber to use the user's locale instead (e.g. pass undefined to Intl.NumberFormat or use navigator.language / a provided locale prop) and fall back to a safe default inside the existing try/catch; update any callers if you expose a locale prop and ensure formatNumber remains a React.useCallback tied to that locale value so formatting updates when the user's locale changes.src/renderer/hooks/useNotebookTabManager.ts-29-40 (1)
29-40:⚠️ Potential issue | 🟡 MinorValidate persisted tab item shape before hydration.
Line 35 trusts
parsed.tabscontent. Corrupted or stale localStorage entries can inject invalid tab records into state.🧪 Suggested validation
- return { - tabs: parsed.tabs as NotebookTabState[], + const tabs = (parsed.tabs as unknown[]).filter( + (tab): tab is NotebookTabState => + !!tab && + typeof (tab as NotebookTabState).notebookId === 'string' && + typeof (tab as NotebookTabState).notebookName === 'string' && + typeof (tab as NotebookTabState).connectionId === 'string' && + typeof (tab as NotebookTabState).isModified === 'boolean', + ); + + return { + tabs, activeTabId: typeof parsed.activeTabId === 'string' || parsed.activeTabId === null ? parsed.activeTabId : null, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useNotebookTabManager.ts` around lines 29 - 40, The current hydration trusts parsed.tabs shape; add a runtime type-guard that validates each tab object matches NotebookTabState (e.g., required keys and correct types) by implementing an isValidNotebookTab(tab): boolean and use it to filter parsed.tabs before returning; if no valid tabs remain return null, and ensure activeTabId is either null or one of the remaining tab ids (otherwise set it to null). Use the symbols parsed, tabs, NotebookTabState, and activeTabId to locate where to apply the checks in useNotebookTabManager.ts.
🧹 Nitpick comments (4)
src/renderer/components/sidebar/elements.ts (1)
37-42: Verify project-selection gating for the new Notebooks entry.If Notebooks requires an active project, leaving it always enabled can route users into a broken/empty flow. Consider including
/app/notebooksin the same disabled guard as other project-dependent sections.🔎 Read-only verification script
#!/bin/bash set -euo pipefail # Inspect notebooks screen implementation (if present) NOTEBOOK_SCREEN="$(fd 'index.tsx' | rg 'src/renderer/screens/notebooks/index.tsx$' || true)" if [ -n "${NOTEBOOK_SCREEN}" ]; then sed -n '1,260p' "${NOTEBOOK_SCREEN}" fi # Look for project dependency signals around notebooks-related code rg -n -C2 --type=ts --type=tsx '\b(isProjectSelected|selectedProject|projectId|useProject)\b' src/renderer✅ If confirmed project-dependent, apply this guard update
- (element.path === '/app' || element.path === '/app/sql') + (element.path === '/app' || + element.path === '/app/sql' || + element.path === '/app/notebooks')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sidebar/elements.ts` around lines 37 - 42, The Notebooks nav entry is always enabled but appears to be project-dependent; update the entry in elements.ts (the object with path '/app/notebooks' and testId 'nav-item-notebooks') to use the same project-selection guard as other project-dependent navigation items (e.g., check the same boolean used by isProjectSelected/selectedProject or the existing disabled condition) so the item is disabled when no project is selected; mirror the exact guard pattern used by sibling entries to avoid duplicating logic.src/main/ipcHandlers/notebooks.ipcHandlers.ts (1)
36-42: Consider adding a proper type for theupdatesparameter.Using
anyfor theupdatesparameter sacrifices type safety. Consider defining an explicit update type (e.g.,Partial<Notebook>or a dedicatedNotebookUpdatesinterface) to ensure type-safe IPC communication.♻️ Suggested improvement
+import { NotebookUpdates } from '../../types/notebooks'; // or define inline + // Update a notebook ipcMain.handle( 'notebooks:update', - async (_event, connectionId: string, notebookId: string, updates: any) => { + async (_event, connectionId: string, notebookId: string, updates: NotebookUpdates) => { 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 36 - 42, The IPC handler for 'notebooks:update' uses an untyped updates: any parameter; replace that with a specific type (e.g., Partial<Notebook> or a dedicated NotebookUpdates interface) and update the handler signature and call to NotebooksService.updateNotebook to use that type so IPC communication and NotebooksService.updateNotebook(connectionId, notebookId, updates) are type-safe; ensure the Notebook type or NotebookUpdates interface is imported/declared and used in the ipcMain.handle callback signature and any related service method declarations.src/renderer/components/notebook/NotebookTabManager.tsx (1)
246-276: Consider optimizing the dependency array for the scroll effect.The effect depends on the entire
tabsarray, which may trigger unnecessary scroll calculations when tab content (likeisModified) changes but tab order remains the same. Consider depending on a more stable reference liketabs.lengthor a derived list of tab IDs if scroll behavior should only respond to tab additions/removals/reordering.♻️ Suggested optimization
+ const tabIds = tabs.map((t) => t.notebookId).join(','); + React.useEffect(() => { // ... scroll logic - }, [activeTabId, tabs]); + }, [activeTabId, tabIds]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/NotebookTabManager.tsx` around lines 246 - 276, The effect that auto-scrolls the active tab uses a fragile dependency on the entire tabs array which causes unnecessary runs when tab contents change; update the dependency array of the useEffect that references activeTabId, containerRef, and tabRefs in NotebookTabManager.tsx to depend on activeTabId and a stable representation of the tab list (e.g. tabs.length or tabs.map(t => t.id) / a derived tabIds array) instead of the full tabs object so scrolling only recalculates on tab add/remove/reorder or activeTab change.src/renderer/components/notebook/NotebookEditor.tsx (1)
134-147: Remove duplicatedlocalCellssync effect.Line 141 duplicates the same effect from Line 134, causing redundant state updates on every notebook cell refresh.
🧹 Suggested cleanup
// Sync local cells with notebook data useEffect(() => { if (notebook?.cells) { setLocalCells(notebook.cells); } }, [notebook?.cells]); - - // Sync local cells with notebook data - useEffect(() => { - if (notebook?.cells) { - setLocalCells(notebook.cells); - } - }, [notebook?.cells]);🤖 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 134 - 147, The component contains a duplicated useEffect that syncs localCells from notebook?.cells; remove the redundant effect so only a single useEffect remains that checks if (notebook?.cells) and calls setLocalCells(notebook.cells). Ensure you keep one copy of the effect (the one using useEffect, notebook?.cells in the dependency array, and setLocalCells) and delete the other duplicate to avoid redundant state updates.
🤖 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/connectors.service.ts`:
- Around line 548-557: The deletion flow currently swallows errors from
NotebooksService.archiveConnectionNotebooks(connectionToDelete.id) and continues
deleting the connection (using connectionToDelete), which can orphan notebooks;
change the try/catch so archival failures abort the deletion: either rethrow the
caught error or return an error result from the surrounding function instead of
logging and continuing. Locate the call to
NotebooksService.archiveConnectionNotebooks and the subsequent code that deletes
the connection (using connectionToDelete) and ensure on any thrown or caught
error you stop further processing (rethrow or propagate an error) so the
connection is not removed when archival fails.
In `@src/main/services/duckLake.service.ts`:
- Around line 759-768: The current try/catch around
NotebooksService.archiveConnectionNotebooks(`ducklake-${id}`) swallows failures
and allows instance deletion to continue; remove the silent catch and instead
let the error propagate (or rethrow after logging) so deletion aborts if
notebook archival fails. Specifically, in the DuckLake deletion flow where
NotebooksService.archiveConnectionNotebooks is called, either delete the
try/catch block or change the catch to log via console.error/processLogger and
then throw the caught error (including context like `ducklake-${id}`) so the
caller can stop the deletion and handle the failure.
In `@src/main/services/notebooks.service.ts`:
- Around line 53-69: The path-building functions getNotebookPath,
getConnectionDir and normalizeConnectionKey currently use untrusted
connectionId/notebookId values directly which allows path traversal; validate
and sanitize inputs before composing filesystem paths by rejecting or
normalizing any segment containing path traversal or absolute path tokens (e.g.
'..', '/', '\\', or starting with path.sep), enforce an allowlist (e.g.
alphanumeric, dashes, underscores) for connectionId and notebookId, and after
building the candidate path (using path.join or path.normalize) verify it is
inside NOTEBOOKS_DIR (check that
normalizedPath.startsWith(path.resolve(NOTEBOOKS_DIR)+path.sep)); if validation
fails throw an error so callers cannot read/write outside the notebooks
directory. Ensure these checks are applied in getNotebookPath, getConnectionDir
(and any call sites that accept raw connectionId/notebookId) and keep
normalizeConnectionKey unchanged except validate the incoming connectionId
before transforming it.
- Around line 642-655: The code in runAllCells reuses the original notebook
snapshot (notebook) after calling runCell/updateNotebook which causes the write
at the end to clobber fresher outputs; change the final write to use the current
persisted notebook state instead of the stale local variable: after running
cells call getNotebook(connectionId, notebookId) (or use the value returned from
updateNotebook if it returns the latest state) and assign that to
updatedNotebook, then set updatedNotebook.lastExecutedAt and write that object
via normalizeConnectionKey/getNotebookPath and fs.writeFile with bigIntReplacer
so you never write the old "notebook" snapshot and lose newer cell outputs.
In `@src/renderer/components/notebook/MarkdownCell.tsx`:
- Around line 37-43: The IconButton used for toggling edit/preview mode
(IconButton, toggleMode, isEditing, PreviewIcon, EditIcon in MarkdownCell.tsx)
is missing an accessible name; update the IconButton to include a descriptive
aria-label that reflects the current action/state (for example
aria-label={isEditing ? "Switch to preview" : "Switch to edit"}) and optionally
set aria-pressed={isEditing} to indicate toggle state to assistive tech; ensure
the label text matches the visible icon meaning.
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 449-456: The handleClone callback currently logs a TODO and must
call the duplication flow: use the existing useDuplicateNotebook hook (or
implement it if missing) inside NotebookEditor and invoke its
duplicateNotebook/duplicate function with the current notebook id (reference
notebook variable and handleClone). On success, open the newly created notebook
in a new tab (use the app/tab method already used elsewhere in this file for
opening notebooks), show a success toast/notification, and on error show an
error toast and log the error; ensure handleClone disables UI while the mutation
is pending and cleans up after completion so the toolbar action is no-op-free.
- Around line 237-249: There is a pending debounced save
(updateTimeoutRef.current) that can reapply stale updatedCells via
updateNotebook.mutate; before any immediate structural mutations
(delete/duplicate/reorder/clear handlers) cancel and clear the pending timeout
(clearTimeout(updateTimeoutRef.current); updateTimeoutRef.current = undefined)
and then call updateNotebook.mutate with the current latest cells; alternatively
centralize saves into a helper (e.g., saveNotebook(cells)) that always clears
any existing timeout before performing an immediate mutate or scheduling the
debounced mutate so no stale timeout can overwrite structural edits.
- Around line 604-606: The render is mutating state by calling
localCells.sort(...) directly; fix this by creating a new array before sorting
(e.g., use a shallow copy via spread or slice) and iterate over that copy
instead of localCells so state is not changed during render; update the mapping
to use the new sorted copy (e.g., sortedCells) wherever localCells.sort(...) is
currently used in the NotebookEditor component.
In `@src/renderer/components/notebook/NotebooksList.tsx`:
- Around line 186-188: The click handler handleOpenNotebook (and the other card
click that calls navigate to `/app/notebooks`) always navigates to a static
route so the selected notebook isn't passed; change these to include the
notebook identifier when navigating (for example call
navigate(`/app/notebooks/${notebook.id}`) or
navigate(`/app/notebooks/${notebook.slug}`), or send the notebook via state like
navigate('/app/notebooks', { state: { notebookId: notebook.id } })), and update
the target Notebooks route/component to read the id from params or
location.state accordingly.
- Around line 261-272: The Card currently uses onClick without keyboard or focus
semantics (Card component with onClick and handleOpenNotebook), making it
inaccessible to keyboard users; update the Card to be keyboard-accessible by
either wrapping its content with an interactive MUI component (e.g., ButtonBase)
or by adding role="button", tabIndex={0}, and an onKeyDown that calls
handleOpenNotebook when Enter or Space is pressed, and ensure focus styles are
preserved so keyboard users can discover and activate the notebook cards.
In `@src/renderer/components/notebook/NotebookToolbar.tsx`:
- Around line 96-263: Several IconButton controls (AddCellIcon, ExportIcon,
EditIcon, CloneIcon, DeleteAllIcon, DeleteIcon) lack explicit accessible names
and currently remain interactive-looking when their callback props (onAddCell,
onExport, onRename, onClone, onDeleteAllCells, onDeleteNotebook) may be
undefined; update each IconButton to include an aria-label (matching the Tooltip
title) and disable the button when its corresponding handler is missing (e.g.,
disabled={!onAddCell} or preserve existing notebook.cells check for
onDeleteAllCells). Also ensure Tooltips still work when buttons are disabled by
wrapping the IconButton in a span when disabled (MUI pattern). Apply these
changes to the IconButton instances referenced above so accessibility and visual
affordance are consistent.
In `@src/renderer/components/notebook/OutputPanel.tsx`:
- Around line 77-110: fetchPage can suffer from out-of-order responses
overwriting newer pages; add a request token (or incrementing requestId)
captured in the fetchPage closure before calling fetchCellPage.mutateAsync and
store it in a ref so only the latest request's response updates state—i.e., when
you call fetchCellPage.mutateAsync from fetchPage, capture currentToken, and
after the await verify the token still matches before calling setPaginatedData
or setLoading; reference fetchPage, fetchCellPage.mutateAsync, setPaginatedData
and setLoading to implement this guard.
- Around line 134-257: The COPY queries interpolate result.filePath directly,
which breaks on paths containing single quotes; create a small helper (e.g.,
escapeSqlLiteral or escapeFilePathForSql) that replaces single quotes with two
single quotes and use it wherever result.filePath is embedded into SQL
(instances: the copyQuery in the JSON export block, the copyQuery in
handleExportCSV, and the COPY used in
handleExportParquet/connector:executeQuery) so the path is safely quoted before
building the `COPY (...) TO '...'(FORMAT ...)` strings.
In `@src/renderer/components/notebook/SQLCell.tsx`:
- Around line 467-486: Keyboard shortcuts registered via
editorInstance.addCommand (for monaco.KeyMod.CtrlCmd | monaco.KeyCode.Enter and
monaco.KeyMod.Shift | monaco.KeyCode.Enter) can trigger handleRun even when
isExecuting is true; update both command callbacks to guard against reentrancy
by checking the isExecuting flag (or a local executing ref) at the top of the
callback and returning early if true, and only call handleRun(content) when not
executing; optionally, also prevent the Shift+Enter path from moving to the next
cell when isExecuting to avoid overlapping operations.
- Around line 447-459: Remove the debug console.log in the useEffect inside
SQLCell that currently prints the full cell.output payload; instead either
delete the logging or replace it with a minimal, non-sensitive log (e.g., only
cell.id, a boolean hasOutput, output?.type and rowCount/data length numbers) and
ensure you never include cell.output or its data/columns objects. Update the
useEffect that depends on cell.id and cell.output to reference only those safe
fields (cell.id, !!cell.output, cell.output?.type, cell.output?.rowCount,
cell.output?.data?.length) or remove the effect entirely.
- Around line 415-424: The current handleRun in SQLCell uses a fixed setTimeout
to call refetchSchema which is race-prone; change handleRun to await run
completion instead: make onRun return a Promise (or accept/emit a completion
callback) and update handleRun to call await onRun(content) (or use the
completion callback) and then, if isDDLOperation(content), call refetchSchema()
immediately (remove the 1s setTimeout). Update any callers of onRun to return a
resolving Promise (or invoke the completion callback) so schema refresh happens
after the actual run finishes.
In `@src/renderer/components/queryResult/index.tsx`:
- Around line 334-341: The current canExportParquet guard only checks
connectionType and originalSql so handleExportParquet can show a success toast
even when required identifiers are missing; tighten the guard to require
exportContext.duckLakeInstanceId when exportContext.connectionType ===
'ducklake' and exportContext.connectionId when connectionType === 'duckdb' (or
perform these checks at the top of handleExportParquet), ensure each branch
(ducklake export code path and duckdb export code path) actually runs and awaits
the export before calling the success toast, and only show success after a
successful awaited export or return/throw early if identifiers are missing.
- Around line 231-232: The SQL being interpolated into the COPY (...) export
(e.g., where exportQuery is built using originalSql) must be normalized to
remove trailing semicolons and surrounding whitespace so the COPY syntax isn't
broken; update all places that construct exportQuery (and the similar
constructions at the other two occurrences) to use a normalized variable (e.g.,
normalizedSql = originalSql.trim().replace(/;+\s*$/, '')) and interpolate
normalizedSql instead of originalSql before creating the COPY statement.
In `@src/renderer/components/sidebar/styles.ts`:
- Line 49: SidebarContent currently uses overflow: 'hidden', which prevents
scrolling and can hide nav items; update the SidebarContent style to allow
vertical scrolling (e.g., replace/adjust overflow to permit overflow-y:auto
while keeping overflow-x hidden if desired) so users can scroll to items when
viewport height is limited; locate the SidebarContent style definition in
src/renderer/components/sidebar/styles.ts to make this change.
---
Minor comments:
In `@src/main/services/notebooks.service.ts`:
- Around line 358-363: The SELECT branch currently only performs total-row
counting when result.data.length > 0, which leaves totalRows undefined for empty
successful SELECTs; update the logic around isSelect/result.success/result.data
so that totalRows is always set (run the count when appropriate or explicitly
set totalRows = 0 when result.data exists but length === 0) and ensure the code
paths that later return totalRows use this guaranteed numeric value; adjust the
blocks that compute/assign totalRows (the same area that checks isSelect,
result.success, result.data and assigns totalRows) so empty result arrays yield
totalRows = 0 instead of undefined.
In `@src/renderer/components/notebook/MarkdownCell.tsx`:
- Around line 129-133: The rendering currently returns <li> elements directly
(in MarkdownCell.tsx where the code checks line.startsWith('- ') ||
line.startsWith('* ')), which produces invalid markup; modify the render logic
in the MarkdownCell component to group consecutive list-item lines into a list
container (wrap them in a <ul> for '-' or '*' items, or <ol> for numbered items
if present): iterate through the lines, accumulate contiguous list items into a
temporary array and when the run ends flush them as a single <ul> with children
built from the accumulated items, otherwise render regular <p> nodes; ensure
keys are assigned to the list container and items and that single non-list lines
still render as before.
In `@src/renderer/components/notebook/NotebookCell.tsx`:
- Around line 119-171: The effect sets up mousedown on outputResizeHandleRef and
then adds document 'mousemove'/'mouseup' within handleMouseDown, but the outer
cleanup only removes the mousedown listener leaving document listeners,
cancelAnimationFrame (resizeThrottleRef) and body styles if unmounted mid-drag;
fix by hoisting handleMouseMove and handleMouseUp (or storing them in refs) so
you can always call document.removeEventListener for both in the effect cleanup,
cancel any pending animation frame via resizeThrottleRef.current, and reset
document.body.style.cursor and document.body.style.userSelect and
setIsDraggingOutput(false) in the effect's return cleanup; reference
outputResizeHandleRef, resizeThrottleRef, handleMouseMove/handleMouseUp,
setIsDraggingOutput and outputHeightRef when making these changes.
In `@src/renderer/components/notebook/NotebooksTreeView.tsx`:
- Around line 192-210: The empty-state check in NotebooksTreeView currently only
shows the "no notebooks" message when filteredNotebooks.length === 0 and
!showArchived, so enabling showArchived with zero archived matches renders
nothing; update the condition to detect "no matches for the current view" by
considering the active set based on showArchived (e.g., use the
archived-specific filtered list or filter filteredNotebooks by notebook.archived
=== true/false) and show the same message when that filtered set is empty;
modify the render block that uses filteredNotebooks, showArchived and filter
(and the same logic in the later block around the 315-406 range) so the UI
displays the appropriate empty-state message for both active and archived views.
In `@src/renderer/components/queryResult/index.tsx`:
- Around line 64-67: The formatting function formatNumber currently hardcodes
the 'de-DE' locale causing inconsistent displays; change formatNumber to use the
user's locale instead (e.g. pass undefined to Intl.NumberFormat or use
navigator.language / a provided locale prop) and fall back to a safe default
inside the existing try/catch; update any callers if you expose a locale prop
and ensure formatNumber remains a React.useCallback tied to that locale value so
formatting updates when the user's locale changes.
In `@src/renderer/hooks/useNotebookConnectionState.ts`:
- Around line 31-40: The persist effect in useNotebookConnectionState (the
useEffect that writes STORAGE_KEY with PersistedConnectionState when
activeConnectionId/isHydrated change) performs direct
window.localStorage.setItem/removeItem calls which can throw; wrap the
write/remove operations in a try/catch (mirroring the read-side) and handle
errors safely (e.g., swallow or log via console.warn/error) so the effect does
not cause hard failures when storage is unavailable or throws.
In `@src/renderer/hooks/useNotebookTabManager.ts`:
- Around line 29-40: The current hydration trusts parsed.tabs shape; add a
runtime type-guard that validates each tab object matches NotebookTabState
(e.g., required keys and correct types) by implementing an
isValidNotebookTab(tab): boolean and use it to filter parsed.tabs before
returning; if no valid tabs remain return null, and ensure activeTabId is either
null or one of the remaining tab ids (otherwise set it to null). Use the symbols
parsed, tabs, NotebookTabState, and activeTabId to locate where to apply the
checks in useNotebookTabManager.ts.
In `@src/renderer/hooks/useSchemaForConnection.ts`:
- Around line 39-48: The column mapping in useSchemaForConnection.ts currently
sets ordinalPosition to 0 for every column; update the mapping inside columns:
table.columns.map(...) to preserve real ordering by assigning ordinalPosition
from the source column (e.g., use col.ordinalPosition if present, otherwise fall
back to the map index + 1). Ensure the mapped object uses that value for
ordinalPosition so consumers relying on positional ordering receive the correct
order.
In `@src/renderer/services/notebooks.service.ts`:
- Around line 171-182: The public exportData function currently always throws
and will break callers; either implement the renderer-to-main IPC export
contract or remove/hide exportData until ready. Fix by: (1) if implementing,
wire exportData to an ipcRenderer.invoke call (e.g. channel name like
"notebook-export") that sends {cellId, format, data} and awaits a file path/URL
response, and ensure the main-process handler writes the file and returns
success/failure; (2) otherwise remove exportData from the exported service
surface or guard it behind a feature flag (e.g. enableExport) so callers cannot
call the stub; update any references to call the new IPC-enabled export or to
check the flag before invoking exportData.
---
Nitpick comments:
In `@src/main/ipcHandlers/notebooks.ipcHandlers.ts`:
- Around line 36-42: The IPC handler for 'notebooks:update' uses an untyped
updates: any parameter; replace that with a specific type (e.g.,
Partial<Notebook> or a dedicated NotebookUpdates interface) and update the
handler signature and call to NotebooksService.updateNotebook to use that type
so IPC communication and NotebooksService.updateNotebook(connectionId,
notebookId, updates) are type-safe; ensure the Notebook type or NotebookUpdates
interface is imported/declared and used in the ipcMain.handle callback signature
and any related service method declarations.
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 134-147: The component contains a duplicated useEffect that syncs
localCells from notebook?.cells; remove the redundant effect so only a single
useEffect remains that checks if (notebook?.cells) and calls
setLocalCells(notebook.cells). Ensure you keep one copy of the effect (the one
using useEffect, notebook?.cells in the dependency array, and setLocalCells) and
delete the other duplicate to avoid redundant state updates.
In `@src/renderer/components/notebook/NotebookTabManager.tsx`:
- Around line 246-276: The effect that auto-scrolls the active tab uses a
fragile dependency on the entire tabs array which causes unnecessary runs when
tab contents change; update the dependency array of the useEffect that
references activeTabId, containerRef, and tabRefs in NotebookTabManager.tsx to
depend on activeTabId and a stable representation of the tab list (e.g.
tabs.length or tabs.map(t => t.id) / a derived tabIds array) instead of the full
tabs object so scrolling only recalculates on tab add/remove/reorder or
activeTab change.
In `@src/renderer/components/sidebar/elements.ts`:
- Around line 37-42: The Notebooks nav entry is always enabled but appears to be
project-dependent; update the entry in elements.ts (the object with path
'/app/notebooks' and testId 'nav-item-notebooks') to use the same
project-selection guard as other project-dependent navigation items (e.g., check
the same boolean used by isProjectSelected/selectedProject or the existing
disabled condition) so the item is disabled when no project is selected; mirror
the exact guard pattern used by sibling entries to avoid duplicating logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.kiro/steering/github-intructions.mdpackage.jsonsrc/main/ipcHandlers/index.tssrc/main/ipcHandlers/notebooks.ipcHandlers.tssrc/main/ipcSetup.tssrc/main/services/connectors.service.tssrc/main/services/duckLake.service.tssrc/main/services/notebooks.service.tssrc/renderer/App.tsxsrc/renderer/components/customTable/CustomTableHead.tsxsrc/renderer/components/customTable/CustomTablePagination.tsxsrc/renderer/components/customTable/CustomTableRow.tsxsrc/renderer/components/customTable/index.tsxsrc/renderer/components/index.tssrc/renderer/components/notebook/MarkdownCell.tsxsrc/renderer/components/notebook/NotebookCell.tsxsrc/renderer/components/notebook/NotebookEditor.tsxsrc/renderer/components/notebook/NotebookTabManager.tsxsrc/renderer/components/notebook/NotebookToolbar.tsxsrc/renderer/components/notebook/NotebooksList.tsxsrc/renderer/components/notebook/NotebooksSidebar.tsxsrc/renderer/components/notebook/NotebooksTreeView.tsxsrc/renderer/components/notebook/OutputPanel.tsxsrc/renderer/components/notebook/SQLCell.tsxsrc/renderer/components/notebook/index.tssrc/renderer/components/queryResult/index.tsxsrc/renderer/components/sidebar/elements.tssrc/renderer/components/sidebar/icons.tsxsrc/renderer/components/sidebar/index.tsxsrc/renderer/components/sidebar/styles.tssrc/renderer/controllers/notebooks.controller.tssrc/renderer/hooks/index.tssrc/renderer/hooks/useMonacoAutocomplete.tssrc/renderer/hooks/useNotebookConnectionState.tssrc/renderer/hooks/useNotebookSidebarState.tssrc/renderer/hooks/useNotebookTabManager.tssrc/renderer/hooks/useSchemaForConnection.tssrc/renderer/layouts/appLayout/styles.tssrc/renderer/screens/index.tssrc/renderer/screens/notebooks/index.tsxsrc/renderer/services/notebooks.service.tssrc/types/ipc.tssrc/types/notebooks.ts
💤 Files with no reviewable changes (1)
- .kiro/steering/github-intructions.md
… data handling Implemented comprehensive notebook import/export system supporting both single and bulk operations with proper data size optimization. Export Features: - Single notebook export via toolbar button (excludes output.data arrays) - Bulk export via sidebar menu (exports all notebooks for connection) - Preserves metadata (columns, rowCount, totalRows, executionTime) - Excludes large data arrays to keep file sizes manageable Import Features: - Auto-detects single vs bulk export formats - Imports all notebooks from bulk export files - Clears output data on import for clean state - Generates new IDs for notebooks and cells - Opens first imported notebook automatically - File size validation (50MB single, 100MB bulk) Backend Implementation: - Added importNotebook() method with format detection - Added importAllNotebooks() for bulk imports - Added notebooks:importAll IPC handler - Refactored import logic to use Promise.all for parallel processing - Proper error handling with user-friendly messages Frontend Implementation: - Added useImportAllNotebooks() hook - Updated NotebooksSidebar with "Import Notebooks" menu item - Added handleImportAllNotebooks handler - Toast notifications for success/error states - Automatic query invalidation to refresh UI Files Modified: - src/main/services/notebooks.service.ts - src/main/ipcHandlers/notebooks.ipcHandlers.ts - src/types/ipc.ts - src/renderer/services/notebooks.service.ts - src/renderer/controllers/notebooks.controller.ts - src/renderer/components/notebook/NotebooksSidebar.tsx - src/renderer/components/notebook/NotebookEditor.tsx - src/renderer/components/notebook/NotebookToolbar.tsx - src/renderer/screens/notebooks/index.tsx Fixes: - Export now properly excludes output.data arrays - Import handles both single and bulk formats correctly
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/ipcHandlers/notebooks.ipcHandlers.ts (1)
36-42: Consider typing theupdatesparameter.The
updatesparameter is typed asany, losing type safety. This could allow malformed update objects to reach the service layer.💡 Add explicit typing
+import { NotebookCell } 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: { + name?: string; + description?: string; + cells?: NotebookCell[]; + }) => { 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 36 - 42, The handler currently types the updates parameter as any; replace it with a well-defined type (e.g., Create or Update DTO) — for example define or import an UpdateNotebook type (or use Partial<Notebook> / UpdateNotebookDto) and change the ipcMain.handle callback signature to use that type for the updates parameter; also update NotebooksService.updateNotebook's signature to accept the same typed updates so the service layer preserves type safety and rejects malformed objects.src/main/services/notebooks.service.ts (1)
597-601: SELECT query detection may miss edge cases.The
isSelectQueryhelper only checks if the query starts withSELECTorWITH. Queries with leading comments (-- comment\nSELECT ...) or whitespace variations could be misclassified, causing pagination to be skipped unexpectedly for valid SELECT queries.💡 Consider stripping comments before detection
const isSelectQuery = (query: string): boolean => { - const normalized = query.trim().toUpperCase(); + // Strip leading single-line and multi-line comments + const normalized = query + .replace(/^(\s*(--[^\n]*\n|\/\*[\s\S]*?\*\/))+/g, '') + .trim() + .toUpperCase(); return normalized.startsWith('SELECT') || normalized.startsWith('WITH'); };🤖 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 597 - 601, The isSelectQuery helper misclassifies SELECTs preceded by comments/whitespace; update isSelectQuery to first remove SQL comments (single-line "-- ..." and block "/* ... */") and then trim leading whitespace before uppercasing and checking startsWith('SELECT') || startsWith('WITH'); ensure the function (isSelectQuery) handles multi-line queries and preserves the query text for later use (i.e., operate on a cleaned copy, not mutate the original).src/renderer/screens/notebooks/index.tsx (1)
169-240:fetchSchemaForConnectionreads state in callback but has it in dependencies.The function reads
loadingSchemas[connectionId]andtabSchemas[connectionId]inside the callback while also having them in the dependency array. This can cause unnecessary re-creations of the callback and potential stale closure issues if the state updates during async operations.💡 Use refs for the early-return checks
Consider using refs for the "already loading/cached" checks to avoid re-creating the callback on every state change, or remove these from dependencies and rely on the early returns.
+ const loadingSchemasRef = useRef(loadingSchemas); + const tabSchemasRef = useRef(tabSchemas); + useEffect(() => { + loadingSchemasRef.current = loadingSchemas; + tabSchemasRef.current = tabSchemas; + }, [loadingSchemas, tabSchemas]); + const fetchSchemaForConnection = useCallback( async (connectionId: string) => { - if (loadingSchemas[connectionId]) return; - if (tabSchemas[connectionId]) return; + if (loadingSchemasRef.current[connectionId]) return; + if (tabSchemasRef.current[connectionId]) return; // ... rest of function }, - [loadingSchemas, tabSchemas], + [], );src/renderer/services/notebooks.service.ts (1)
205-217: UnimplementedexportDatamethod will throw at runtime.This placeholder method logs and throws an error. If any UI inadvertently calls it, users will see an error. Consider either removing this method until implemented or returning a more graceful rejection.
Would you like me to open an issue to track the
exportDataimplementation, or help stub it with a proper IPC channel?🤖 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 205 - 217, The exportData placeholder (exportData) currently throws an Error synchronously which will crash callers; replace the throw with a graceful rejection or actual IPC stub: either return Promise.reject(new Error('exportData not implemented')) (and keep or remove the console.log) so callers receive a rejected promise instead of an uncaught exception, or implement the IPC call by invoking ipcRenderer.invoke('notebooks:export', { cellId, format, data }) and return its result; update the exportData function body accordingly and leave a TODO comment if you choose the rejection stub.
🤖 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/NotebookEditor.tsx`:
- Around line 146-158: There are two identical useEffect blocks that both sync
localCells with notebook?.cells; remove the duplicate so only one effect
remains: keep the single useEffect that checks if (notebook?.cells) and calls
setLocalCells(notebook.cells) (references: useEffect, setLocalCells,
notebook?.cells, localCells) to avoid redundant updates and potential double
renders.
In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1097-1102: The dialog text is misleading: it says the notebook
"will be moved to archived notebooks" while deleteNotebook calls
NotebooksService.deleteNotebook which permanently unlinks the file; update the
DialogContentText that uses activeNotebookToDelete?.notebookName to clearly
state the deletion is permanent/irreversible (e.g., "This will permanently
delete the notebook and cannot be recovered.") so the UI matches the behavior of
the deleteNotebook flow and NotebooksService.deleteNotebook.
---
Nitpick comments:
In `@src/main/ipcHandlers/notebooks.ipcHandlers.ts`:
- Around line 36-42: The handler currently types the updates parameter as any;
replace it with a well-defined type (e.g., Create or Update DTO) — for example
define or import an UpdateNotebook type (or use Partial<Notebook> /
UpdateNotebookDto) and change the ipcMain.handle callback signature to use that
type for the updates parameter; also update NotebooksService.updateNotebook's
signature to accept the same typed updates so the service layer preserves type
safety and rejects malformed objects.
In `@src/main/services/notebooks.service.ts`:
- Around line 597-601: The isSelectQuery helper misclassifies SELECTs preceded
by comments/whitespace; update isSelectQuery to first remove SQL comments
(single-line "-- ..." and block "/* ... */") and then trim leading whitespace
before uppercasing and checking startsWith('SELECT') || startsWith('WITH');
ensure the function (isSelectQuery) handles multi-line queries and preserves the
query text for later use (i.e., operate on a cleaned copy, not mutate the
original).
In `@src/renderer/services/notebooks.service.ts`:
- Around line 205-217: The exportData placeholder (exportData) currently throws
an Error synchronously which will crash callers; replace the throw with a
graceful rejection or actual IPC stub: either return Promise.reject(new
Error('exportData not implemented')) (and keep or remove the console.log) so
callers receive a rejected promise instead of an uncaught exception, or
implement the IPC call by invoking ipcRenderer.invoke('notebooks:export', {
cellId, format, data }) and return its result; update the exportData function
body accordingly and leave a TODO comment if you choose the rejection stub.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/ipcHandlers/notebooks.ipcHandlers.tssrc/main/services/notebooks.service.tssrc/renderer/components/notebook/NotebookEditor.tsxsrc/renderer/components/notebook/NotebookToolbar.tsxsrc/renderer/components/notebook/NotebooksSidebar.tsxsrc/renderer/controllers/notebooks.controller.tssrc/renderer/screens/notebooks/index.tsxsrc/renderer/services/notebooks.service.tssrc/types/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/components/notebook/NotebooksSidebar.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/renderer/components/sidebar/styles.ts (1)
53-53:⚠️ Potential issue | 🟠 MajorAllow vertical scrolling in
SidebarContent.Line 53 still uses
overflow: 'hidden', which can clip sidebar items on shorter viewports and make nav entries unreachable.🛠️ Proposed fix
export const SidebarContent = styled(Box)(({ theme }) => ({ backgroundColor: theme.palette.background.paper, borderLeft: `2px solid ${theme.palette.background.default}`, width: '100%', height: '100%', - overflow: 'hidden', + overflowX: 'hidden', + overflowY: 'auto', }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/sidebar/styles.ts` at line 53, The SidebarContent style currently sets overflow: 'hidden' which can clip items on short viewports; update the SidebarContent style (the SidebarContent component/style object) to allow vertical scrolling by replacing overflow: 'hidden' with a vertical-scroll behavior such as overflowY: 'auto' (or overflow: 'auto') so long nav entries become reachable while preserving horizontal overflow behavior.src/renderer/components/notebook/OutputPanel.tsx (2)
134-135:⚠️ Potential issue | 🟠 MajorEscape file paths before embedding them into COPY SQL.
Lines 134, 182, 246, and 256 interpolate
result.filePathdirectly inside quoted SQL. A single quote in a valid filesystem path will break SQL literal boundaries.🔧 Suggested fix
+ const escapeSqlLiteral = (value: string) => value.replace(/'/g, "''"); + const normalizedSql = sql.trim().replace(/;$/, ''); @@ - const copyQuery = `COPY (${sql.trim().replace(/;$/, '')}) TO '${result.filePath}' (FORMAT JSON)`; + const copyQuery = `COPY (${normalizedSql}) TO '${escapeSqlLiteral(result.filePath)}' (FORMAT JSON)`; @@ - const copyQuery = `COPY (${sql.trim().replace(/;$/, '')}) TO '${result.filePath}' (FORMAT CSV, HEADER)`; + const copyQuery = `COPY (${normalizedSql}) TO '${escapeSqlLiteral(result.filePath)}' (FORMAT CSV, HEADER)`; @@ - const copyQuery = `COPY (${sql.trim().replace(/;$/, '')}) TO '${result.filePath}' (FORMAT PARQUET)`; + const copyQuery = `COPY (${normalizedSql}) TO '${escapeSqlLiteral(result.filePath)}' (FORMAT PARQUET)`; @@ - query: `COPY (${sql.trim().replace(/;$/, '')}) TO '${result.filePath}' (FORMAT PARQUET)`, + query: `COPY (${normalizedSql}) TO '${escapeSqlLiteral(result.filePath)}' (FORMAT PARQUET)`,Based on learnings, SQL literal escaping by doubling single quotes is an accepted pattern in this codebase when parameterization is unavailable.
Also applies to: 182-183, 246-247, 256-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/OutputPanel.tsx` around lines 134 - 135, Escape single quotes in file paths before embedding into COPY SQL literals: replace single quotes in result.filePath with doubled single quotes (e.g., result.filePath.replace(/'/g, "''")) and use the escaped value when building copyQuery in OutputPanel (the occurrences setting copyQuery at the lines shown plus the similar interpolations at the other noted places). Consider extracting a small helper like escapeSqlLiteral(path) and use it in the COPY string constructions to ensure all places (the copyQuery at the shown line and the other occurrences around lines 182, 246, 256) consistently double single quotes.
77-110:⚠️ Potential issue | 🟠 MajorGuard pagination state from stale async responses.
Line 92 can resolve out-of-order and overwrite the latest page selection, and Line 108 can clear loading for an older request.
🔧 Suggested fix
const [isExporting, setIsExporting] = useState(false); + const latestPageRequestRef = React.useRef(0); const fetchCellPage = useFetchCellPage(); @@ const fetchPage = useCallback( async (newPage: number, newPerPage: number) => { + const requestId = ++latestPageRequestRef.current; + // Don't fetch if we're on the first page and already have data if ( @@ setLoading(true); try { const result = await fetchCellPage.mutateAsync({ @@ }); + if (requestId !== latestPageRequestRef.current) return; + if (result.type === 'table' && result.data) { setPaginatedData(result.data); + } else if (result.type === 'empty') { + setPaginatedData([]); } } catch (error) { @@ } finally { - setLoading(false); + if (requestId === latestPageRequestRef.current) { + setLoading(false); + } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/OutputPanel.tsx` around lines 77 - 110, The fetchPage async callback can have out-of-order responses overwrite the latest state (setPaginatedData) and clear loading for older requests; to fix, add a locally scoped request counter or ref (e.g., latestFetchIdRef) that you increment before calling fetchCellPage.mutateAsync inside fetchPage, capture its value in a local const, and only call setPaginatedData(result.data) and setLoading(false) if the captured id matches latestFetchIdRef.current (i.e., the response is for the most recent request); apply the same check for the early-return branch that uses output.data so it doesn't short-circuit to stale data.src/renderer/components/notebook/SQLCell.tsx (3)
533-551:⚠️ Potential issue | 🟠 MajorBlock keyboard-triggered runs while execution is already in progress.
Lines 536-549 call
handleRununconditionally; shortcuts can launch overlapping runs.🔧 Suggested fix
editorInstance.addCommand( // eslint-disable-next-line no-bitwise monaco.KeyMod.CtrlCmd | monaco.KeyCode.Enter, () => { + if (isExecuting) return; const content = editorInstance.getValue(); handleRun(content); }, ); @@ editorInstance.addCommand( // eslint-disable-next-line no-bitwise monaco.KeyMod.Shift | monaco.KeyCode.Enter, () => { + if (isExecuting) return; const content = editorInstance.getValue(); handleRun(content); // TODO: Move to next cell }, );🤖 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 533 - 551, The keyboard shortcuts registered via editorInstance.addCommand (the Ctrl/Cmd+Enter and Shift+Enter handlers that call handleRun) can start overlapping executions; modify those handlers to first check a running flag (e.g., a component state like isRunning or isExecuting in the SQLCell component) and only call handleRun when not already running, and optionally provide user feedback when blocked; ensure any state you add is set before/after the actual run lifecycle in the existing run flow used by handleRun so the flag correctly prevents concurrent invocations.
512-524:⚠️ Potential issue | 🟠 MajorRemove full output payload logging from renderer console.
Lines 515-523 log
cell.outputand result metadata; this can expose sensitive query data and is expensive for large outputs.🔧 Suggested fix
- // Debug: Log when cell output changes - useEffect(() => { - // eslint-disable-next-line no-console - console.log('[SQLCell] Cell output updated:', { - cellId: cell.id, - hasOutput: !!cell.output, - output: cell.output, - outputType: cell.output?.type, - dataLength: cell.output?.data?.length, - columns: cell.output?.columns, - rowCount: cell.output?.rowCount, - }); - }, [cell.id, cell.output]);🤖 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 512 - 524, The renderer currently logs full cell output in SQLCell.tsx via the useEffect console.log which can expose sensitive data and be expensive; update the useEffect that references cell.id and cell.output to stop logging the full cell.output and its data payloads and instead log only safe metadata (e.g., cell.id, hasOutput, output?.type, output?.rowCount, and columns?.length or a boolean for columns present) or remove the log entirely; modify the console.log call in the useEffect so it does not include cell.output, output.data, or any raw rows, and remove the eslint-disable-next-line if you eliminate the console call.
418-423:⚠️ Potential issue | 🟠 MajorRefresh schema after run completion, not with a fixed delay.
Line 470 uses a hardcoded 1s delay, which is race-prone for slower DDL queries.
🔧 Suggested fix
interface SQLCellProps { cell: NotebookCell; connectionId: string; // Changed from instanceId to connectionId for consistency isExecuting: boolean; - onRun: (content: string) => void; + onRun: (content: string) => void | Promise<void>; onUpdate: (content: string) => void; } @@ const handleRun = React.useCallback( - (content: string) => { - onRun(content); - - // Refresh schema if DDL operation - if (isDDLOperation(content)) { - setTimeout(() => { - refetchSchema(); - }, 1000); // Wait 1s for DDL to complete - } + async (content: string) => { + await Promise.resolve(onRun(content)); + if (isDDLOperation(content)) { + void refetchSchema(); + } }, [onRun, refetchSchema], );Also applies to: 464-473
🤖 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 418 - 423, The component currently refreshes the database schema using a hardcoded setTimeout (1s) after running SQL, which is race-prone; instead, remove the fixed delay and trigger the schema refresh when the run finishes by awaiting the actual execution completion. Modify the run flow (e.g., the handler that calls onRun/executeQuery—look for functions like handleRun, onRun invocation or executeQuery in SQLCell.tsx) so that onRun returns a Promise (or accepts a completion callback) and call the schema refresh (the existing refreshSchema/reloadSchema function or logic that updates schema) in the .then/await resolution of that promise; if onRun cannot be made async, add a callback parameter to notify completion and invoke schema refresh from that completion callback. Ensure no setTimeout is left for schema refresh.src/renderer/components/notebook/NotebookToolbar.tsx (1)
97-264:⚠️ Potential issue | 🟠 MajorAdd accessible labels and disabled fallbacks for icon-only actions.
Icon-only buttons in this block are missing explicit
aria-labels, and optional actions (onAddCell,onRename,onDuplicate,onDeleteAllCells,onDeleteNotebook) remain visually active when handlers are absent.🔧 Suggested pattern (apply to all icon-only optional actions)
- <Tooltip title="Add New Cell"> - <IconButton + <Tooltip title="Add New Cell"> + <span> + <IconButton size="small" onClick={onAddCell} + disabled={!onAddCell} + aria-label="Add New Cell" sx={{ @@ - </IconButton> + </IconButton> + </span> </Tooltip> @@ - <Tooltip title="Edit Workbook"> - <IconButton + <Tooltip title="Edit Workbook"> + <span> + <IconButton size="small" onClick={onRename} + disabled={!onRename} + aria-label="Edit Workbook" sx={{ @@ - </IconButton> + </IconButton> + </span> </Tooltip>🤖 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 97 - 264, The icon-only IconButton elements (AddCellIcon, ExportIcon, EditIcon, DuplicateIcon, DeleteAllIcon, DeleteIcon) lack explicit aria-labels and don't defensively disable when their click handlers may be undefined; update each IconButton that uses onAddCell, onExport, onRename, onDuplicate, onDeleteAllCells, and onDeleteNotebook to include a descriptive aria-label (e.g., "Add Cell", "Export Notebook", "Rename Notebook", "Duplicate Notebook", "Delete All Cells", "Delete Notebook") and add a disabled fallback like disabled={!onAddCell} (or combine with existing conditions, e.g., disabled={!onDeleteAllCells || notebook.cells.length === 0}) so buttons are both accessible and inert when handlers are absent.
🧹 Nitpick comments (2)
src/renderer/components/notebook/NotebookTabManager.tsx (2)
112-124: Minor color inconsistency for inactive tabs.The parent
Boxsetscolor: isActive ? 'text.primary' : 'text.secondary'(line 74), but this innerBoxoverrides it withcolor: 'text.primary'unconditionally. This means the notebook name always appears in primary color regardless of active state, potentially defeating the visual distinction.💡 Suggested fix to inherit color from parent
<Box sx={{ fontSize: 13, lineHeight: 1.2, maxWidth: 160, overflow: 'hidden', textOverflow: 'ellipsis', whiteSpace: 'nowrap', - color: 'text.primary', + color: 'inherit', }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/NotebookTabManager.tsx` around lines 112 - 124, The inner Box rendering the notebook name in NotebookTabManager.tsx currently forces color: 'text.primary', overriding the parent's isActive-based color; update that inner Box to inherit the parent's color (remove the hardcoded 'text.primary' and use color: 'inherit' or omit the color prop) so the notebook name reflects the parent Box's color logic (which uses isActive ? 'text.primary' : 'text.secondary').
245-255: Inconsistent drop indicator colors.The per-tab drop indicator (lines 251) uses hardcoded grays (
#2d2d2d/#e0e0e0) while the trailing drop indicator (line 345) usestheme.palette.primary.main. This visual inconsistency may confuse users about where their drop will land.Consider using consistent colors for both indicators.
💡 Suggested fix for consistent indicator styling
{showDropIndicator && ( <Box sx={{ width: 3, height: 22, borderRadius: 999, - bgcolor: theme.palette.mode === 'dark' ? '#2d2d2d' : '#e0e0e0', + bgcolor: theme.palette.primary.main, boxShadow: `0 0 0 1px ${theme.palette.background.paper}`, transition: 'opacity 120ms ease', }} /> )}Also applies to: 339-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/notebook/NotebookTabManager.tsx` around lines 245 - 255, The per-tab drop indicator uses hardcoded colors in the Box inside NotebookTabManager (the showDropIndicator JSX block) while the trailing drop indicator uses theme.palette.primary.main, causing inconsistent visuals; update the per-tab Box to use the same theme color (e.g., theme.palette.primary.main or a shared variable like dropIndicatorColor) instead of '#2d2d2d' / '#e0e0e0', and refactor both the per-tab Box and the trailing drop indicator to reference that same theme-derived value to ensure consistent styling across showDropIndicator and the trailing drop indicator component.
🤖 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`:
- Line 57: The section state (const [section, setSection] =
useState<SectionFilter>('all')) can remain 'output' after outputs are cleared,
hiding both editor and output; update the code paths that remove/clear outputs
(and any handlers in the range around the other affected block) to call
setSection('all') whenever outputs become unavailable (e.g., on clear, delete,
or when output list length becomes 0) so the UI falls back to the editor;
reference the section/setSection state in NotebookCell.tsx and ensure the same
reset is applied in the other affected block (around the 286-383 region) where
outputs are modified.
- Around line 119-171: The effect that registers the mousedown listener for
dragging lives in the useEffect block but uses empty deps so it runs before
outputResizeHandleRef.current is set and never rebinds when the handle mounts;
update the effect to depend on the resize handle ref (use outputResizeHandleRef
or a stable value derived from outputResizeHandleRef.current) so the effect
re-runs when the element mounts, and ensure you still clean up by removing the
listener in the returned cleanup; keep the internal handlers (handleMouseDown,
handleMouseMove, handleMouseUp) and refs (outputHeightRef, resizeThrottleRef,
isDraggingOutput state) as-is but move the listener registration into the effect
that watches the handle ref so the element always gets the mousedown listener
when it appears.
In `@src/renderer/components/notebook/OutputPanel.tsx`:
- Around line 151-153: The JSON export currently uses JSON.stringify on
paginatedData in OutputPanel.tsx which throws on bigint values; update the JSON
serialization used when creating the Blob so JSON.stringify receives a replacer
that converts bigint values to strings (e.g., detect typeof value === 'bigint'
and call value.toString()) before constructing the Blob, ensuring paginatedData
with bigint fields from query results serializes without errors.
---
Duplicate comments:
In `@src/renderer/components/notebook/NotebookToolbar.tsx`:
- Around line 97-264: The icon-only IconButton elements (AddCellIcon,
ExportIcon, EditIcon, DuplicateIcon, DeleteAllIcon, DeleteIcon) lack explicit
aria-labels and don't defensively disable when their click handlers may be
undefined; update each IconButton that uses onAddCell, onExport, onRename,
onDuplicate, onDeleteAllCells, and onDeleteNotebook to include a descriptive
aria-label (e.g., "Add Cell", "Export Notebook", "Rename Notebook", "Duplicate
Notebook", "Delete All Cells", "Delete Notebook") and add a disabled fallback
like disabled={!onAddCell} (or combine with existing conditions, e.g.,
disabled={!onDeleteAllCells || notebook.cells.length === 0}) so buttons are both
accessible and inert when handlers are absent.
In `@src/renderer/components/notebook/OutputPanel.tsx`:
- Around line 134-135: Escape single quotes in file paths before embedding into
COPY SQL literals: replace single quotes in result.filePath with doubled single
quotes (e.g., result.filePath.replace(/'/g, "''")) and use the escaped value
when building copyQuery in OutputPanel (the occurrences setting copyQuery at the
lines shown plus the similar interpolations at the other noted places). Consider
extracting a small helper like escapeSqlLiteral(path) and use it in the COPY
string constructions to ensure all places (the copyQuery at the shown line and
the other occurrences around lines 182, 246, 256) consistently double single
quotes.
- Around line 77-110: The fetchPage async callback can have out-of-order
responses overwrite the latest state (setPaginatedData) and clear loading for
older requests; to fix, add a locally scoped request counter or ref (e.g.,
latestFetchIdRef) that you increment before calling fetchCellPage.mutateAsync
inside fetchPage, capture its value in a local const, and only call
setPaginatedData(result.data) and setLoading(false) if the captured id matches
latestFetchIdRef.current (i.e., the response is for the most recent request);
apply the same check for the early-return branch that uses output.data so it
doesn't short-circuit to stale data.
In `@src/renderer/components/notebook/SQLCell.tsx`:
- Around line 533-551: The keyboard shortcuts registered via
editorInstance.addCommand (the Ctrl/Cmd+Enter and Shift+Enter handlers that call
handleRun) can start overlapping executions; modify those handlers to first
check a running flag (e.g., a component state like isRunning or isExecuting in
the SQLCell component) and only call handleRun when not already running, and
optionally provide user feedback when blocked; ensure any state you add is set
before/after the actual run lifecycle in the existing run flow used by handleRun
so the flag correctly prevents concurrent invocations.
- Around line 512-524: The renderer currently logs full cell output in
SQLCell.tsx via the useEffect console.log which can expose sensitive data and be
expensive; update the useEffect that references cell.id and cell.output to stop
logging the full cell.output and its data payloads and instead log only safe
metadata (e.g., cell.id, hasOutput, output?.type, output?.rowCount, and
columns?.length or a boolean for columns present) or remove the log entirely;
modify the console.log call in the useEffect so it does not include cell.output,
output.data, or any raw rows, and remove the eslint-disable-next-line if you
eliminate the console call.
- Around line 418-423: The component currently refreshes the database schema
using a hardcoded setTimeout (1s) after running SQL, which is race-prone;
instead, remove the fixed delay and trigger the schema refresh when the run
finishes by awaiting the actual execution completion. Modify the run flow (e.g.,
the handler that calls onRun/executeQuery—look for functions like handleRun,
onRun invocation or executeQuery in SQLCell.tsx) so that onRun returns a Promise
(or accepts a completion callback) and call the schema refresh (the existing
refreshSchema/reloadSchema function or logic that updates schema) in the
.then/await resolution of that promise; if onRun cannot be made async, add a
callback parameter to notify completion and invoke schema refresh from that
completion callback. Ensure no setTimeout is left for schema refresh.
In `@src/renderer/components/sidebar/styles.ts`:
- Line 53: The SidebarContent style currently sets overflow: 'hidden' which can
clip items on short viewports; update the SidebarContent style (the
SidebarContent component/style object) to allow vertical scrolling by replacing
overflow: 'hidden' with a vertical-scroll behavior such as overflowY: 'auto' (or
overflow: 'auto') so long nav entries become reachable while preserving
horizontal overflow behavior.
---
Nitpick comments:
In `@src/renderer/components/notebook/NotebookTabManager.tsx`:
- Around line 112-124: The inner Box rendering the notebook name in
NotebookTabManager.tsx currently forces color: 'text.primary', overriding the
parent's isActive-based color; update that inner Box to inherit the parent's
color (remove the hardcoded 'text.primary' and use color: 'inherit' or omit the
color prop) so the notebook name reflects the parent Box's color logic (which
uses isActive ? 'text.primary' : 'text.secondary').
- Around line 245-255: The per-tab drop indicator uses hardcoded colors in the
Box inside NotebookTabManager (the showDropIndicator JSX block) while the
trailing drop indicator uses theme.palette.primary.main, causing inconsistent
visuals; update the per-tab Box to use the same theme color (e.g.,
theme.palette.primary.main or a shared variable like dropIndicatorColor) instead
of '#2d2d2d' / '#e0e0e0', and refactor both the per-tab Box and the trailing
drop indicator to reference that same theme-derived value to ensure consistent
styling across showDropIndicator and the trailing drop indicator component.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/renderer/components/notebook/MarkdownCell.tsxsrc/renderer/components/notebook/NotebookCell.tsxsrc/renderer/components/notebook/NotebookTabManager.tsxsrc/renderer/components/notebook/NotebookToolbar.tsxsrc/renderer/components/notebook/NotebooksSidebar.tsxsrc/renderer/components/notebook/OutputPanel.tsxsrc/renderer/components/notebook/SQLCell.tsxsrc/renderer/components/sidebar/styles.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/notebook/NotebooksSidebar.tsx
- src/renderer/components/notebook/MarkdownCell.tsx
Comment on lines
+119
to
+171
| useEffect(() => { | ||
| const handleMouseDown = (e: MouseEvent) => { | ||
| e.preventDefault(); | ||
| setIsDraggingOutput(true); | ||
| const startY = e.clientY; | ||
|
|
||
| // Get current height from ref at the time of mousedown | ||
| const startHeight = outputHeightRef.current || 300; | ||
|
|
||
| const handleMouseMove = (moveEvent: MouseEvent) => { | ||
| const deltaY = moveEvent.clientY - startY; | ||
| const newHeight = Math.max(150, Math.min(1000, startHeight + deltaY)); | ||
|
|
||
| // Throttle updates to ~60fps (16ms) to reduce re-renders | ||
| if (resizeThrottleRef.current) { | ||
| return; // Skip this update, previous one still pending | ||
| } | ||
|
|
||
| resizeThrottleRef.current = window.requestAnimationFrame(() => { | ||
| setOutputHeight(newHeight); | ||
| resizeThrottleRef.current = null; | ||
| }); | ||
| }; | ||
|
|
||
| const handleMouseUp = () => { | ||
| // Cancel any pending throttled update | ||
| if (resizeThrottleRef.current) { | ||
| window.cancelAnimationFrame(resizeThrottleRef.current); | ||
| resizeThrottleRef.current = null; | ||
| } | ||
|
|
||
| document.removeEventListener('mousemove', handleMouseMove); | ||
| document.removeEventListener('mouseup', handleMouseUp); | ||
| document.body.style.cursor = ''; | ||
| document.body.style.userSelect = ''; | ||
| setIsDraggingOutput(false); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', handleMouseMove); | ||
| document.addEventListener('mouseup', handleMouseUp); | ||
| document.body.style.cursor = 'row-resize'; | ||
| document.body.style.userSelect = 'none'; | ||
| }; | ||
|
|
||
| const resizeHandle = outputResizeHandleRef.current; | ||
| if (resizeHandle) { | ||
| resizeHandle.addEventListener('mousedown', handleMouseDown); | ||
| return () => { | ||
| resizeHandle.removeEventListener('mousedown', handleMouseDown); | ||
| }; | ||
| } | ||
| return undefined; | ||
| }, []); // Empty deps - only set up once, use ref to get current height |
There was a problem hiding this comment.
Rebind output resize listener when the resize handle mounts.
Line 171 uses empty deps, so listener setup runs only once. When output appears after first render (common path), outputResizeHandleRef.current was null during setup and resize never works.
🔧 Suggested fix
- }, []); // Empty deps - only set up once, use ref to get current height
+ }, [cell.output, section]); // Rebind when output/section toggles and handle mounts🤖 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 119 - 171,
The effect that registers the mousedown listener for dragging lives in the
useEffect block but uses empty deps so it runs before
outputResizeHandleRef.current is set and never rebinds when the handle mounts;
update the effect to depend on the resize handle ref (use outputResizeHandleRef
or a stable value derived from outputResizeHandleRef.current) so the effect
re-runs when the element mounts, and ensure you still clean up by removing the
listener in the returned cleanup; keep the internal handlers (handleMouseDown,
handleMouseMove, handleMouseUp) and refs (outputHeightRef, resizeThrottleRef,
isDraggingOutput state) as-is but move the listener registration into the effect
that watches the handle ref so the element always gets the mousedown listener
when it appears.
Nuri1977
changed the title
…plementation Fixed all 6 issues identified in CodeRabbit review for PR #214 with security, data integrity, and accessibility improvements. Security Fixes (HIGH PRIORITY): - Path traversal prevention: validate path segments before filesystem operations - Error handling: connection/instance deletion aborts on notebook archival failure - Added assertSafeSegment() function to sanitize connection keys and notebook IDs Data Integrity Fixes (MEDIUM PRIORITY): - runAllCells() now reads current notebook state after execution, not stale snapshot - Fixed debounced content save race condition that could overwrite structural edits - Cancel pending debounced saves on all structural mutations (delete, duplicate, drag) Accessibility Fixes (LOW PRIORITY): - Added aria-label and aria-pressed attributes to markdown edit/preview toggle button - Improved screen reader compatibility for icon-only buttons Files Modified: - src/main/services/connectors.service.ts - Fixed error handling in connection deletion - src/main/services/duckLake.service.ts - Fixed error handling in instance deletion - src/main/services/notebooks.service.ts - Added path validation, fixed runAllCells - src/renderer/components/notebook/MarkdownCell.tsx - Added accessibility attributes - src/renderer/components/notebook/NotebookEditor.tsx - Fixed debounced save race condition
There was a problem hiding this comment.
Actionable comments posted: 3
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)
535-550:⚠️ Potential issue | 🟠 MajorArchive before credential cleanup to avoid broken retained connections.
If archival fails at Line 550, deletion aborts, but credentials may already be removed at Lines 535-539. That leaves a still-present connection in a broken state.
🔧 Suggested ordering fix
- // 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 const updatedConnections = connections.filter( (connection) => connection.id !== connectionId, ); await updateDatabase<'connections'>('connections', updatedConnections); + + // Best-effort credential cleanup after successful deletion + 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, + ); + }🤖 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 535 - 550, Currently SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id) runs before NotebooksService.archiveConnectionNotebooks(connectionToDelete.id), risking removed credentials if archival fails; change the order so call NotebooksService.archiveConnectionNotebooks(connectionToDelete.id) first and only call SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id) after archive completes successfully, ensuring any thrown error from archive aborts deletion and prevents credential cleanup; reference connectionToDelete, SecureStorageService.cleanupConnectionCredentials, and NotebooksService.archiveConnectionNotebooks when making the change.
♻️ Duplicate comments (3)
src/renderer/components/notebook/NotebookEditor.tsx (2)
161-166:⚠️ Potential issue | 🟡 MinorRemove the duplicated
useEffectthat syncslocalCells.This second block repeats the same state sync and causes unnecessary duplicate updates.
🧹 Suggested fix
- // Sync local cells with notebook data - useEffect(() => { - if (notebook?.cells) { - setLocalCells(notebook.cells); - } - }, [notebook?.cells]);🤖 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 161 - 166, In the NotebookEditor component remove the duplicated useEffect that syncs localCells from notebook?.cells: find the redundant effect that checks if (notebook?.cells) then calls setLocalCells(notebook.cells) and delete it so only the original single sync effect remains; keep the one that correctly handles updates to notebook?.cells and ensure references to localCells, setLocalCells, and notebook?.cells are preserved in the remaining effect.
745-747:⚠️ Potential issue | 🟠 MajorAvoid mutating
localCellsduring render.Calling
.sort(...)directly on state mutates it in place and can cause subtle UI/state bugs.🛠️ Suggested fix
- {localCells - .sort((a, b) => a.order - b.order) + {[...localCells] + .sort((a, b) => a.order - b.order) .map((cell, 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 745 - 747, The render is mutating state by calling .sort on localCells; instead create a non-mutating copy before sorting (e.g., use [...localCells] or localCells.slice()) and then call .sort on that copy inside the JSX map in the NotebookEditor render where localCells.sort(...) is used so state isn't mutated during render.src/main/services/notebooks.service.ts (1)
1086-1090:⚠️ Potential issue | 🔴 CriticalArchived notebook path operations are still traversal-prone.
archivedConnectionKey/connectionKey/notebookIdare used directly in filesystem joins here. With IPC input, this can escape_orphaned; indeleteAllArchivedNotebooksit can become arbitrary recursive deletion.🔒 Suggested hardening
+function getArchivedConnectionDir(connectionKey: string): string { + const safeConnectionKey = assertSafeSegment(connectionKey, 'archived connection key'); + const dirPath = path.resolve(ORPHANED_DIR, safeConnectionKey); + const base = `${path.resolve(ORPHANED_DIR)}${path.sep}`; + if (!dirPath.startsWith(base)) { + throw new Error('Invalid archived connection directory - path traversal detected'); + } + return dirPath; +} + +function getArchivedNotebookPath(connectionKey: string, notebookId: string): string { + const safeNotebookId = assertSafeSegment(notebookId, 'notebook id'); + const dir = getArchivedConnectionDir(connectionKey); + const filePath = path.resolve(dir, `${safeNotebookId}.json`); + const base = `${path.resolve(ORPHANED_DIR)}${path.sep}`; + if (!filePath.startsWith(base)) { + throw new Error('Invalid archived notebook path - path traversal detected'); + } + return filePath; +} ... - const archivedPath = path.join( - ORPHANED_DIR, - archivedConnectionKey, - `${notebookId}.json`, - ); + const archivedPath = getArchivedNotebookPath( + archivedConnectionKey, + notebookId, + ); ... - const archivedPath = path.join( - ORPHANED_DIR, - connectionKey, - `${notebookId}.json`, - ); + const archivedPath = getArchivedNotebookPath(connectionKey, notebookId); ... - const archivedDir = path.join(ORPHANED_DIR, connectionKey); + const archivedDir = getArchivedConnectionDir(connectionKey);Also applies to: 1135-1139, 1170-1173
🤖 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 1086 - 1090, User-supplied segments (archivedConnectionKey/connectionKey/notebookId) are being used directly in path operations (e.g., building archivedPath and in deleteAllArchivedNotebooks), allowing directory traversal; fix by validating/sanitizing these inputs before joining: use safe components such as path.basename or a whitelist/regex to reject path separators and traversal sequences, then build paths and verify with path.resolve that the resulting path starts with ORPHANED_DIR (or otherwise canonicalize and assert containment) before performing file operations; update all occurrences including archivedPath construction and the functions/methods deleteAllArchivedNotebooks, and any code referencing connectionKey/notebookId around the other noted locations (lines ~1135-1139 and ~1170-1173).
🤖 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 968-980: The current read-modify-write in updateCellOutput (uses
getNotebook, limitCellOutputData, then updateNotebook with the full cells array)
can lose concurrent updates; replace this with an atomic per-cell update or
serialization: either (a) add an atomic DB/collection update that targets the
notebook by notebookId and updates only the matching cell's output (so you no
longer replace the whole cells array), or (b) introduce a per-notebook
mutex/lock in notebooks.service.ts around the get-modify-update sequence (keyed
by notebookId) so concurrent executions serialize; reference the
functions/variables getNotebook, limitCellOutputData, cellId, and updateNotebook
and implement the change so the service writes only the single cell output
atomically or under a lock.
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 239-261: The keyboard shortcut effect in NotebookEditor.tsx
captures stale closures because its dependency array only includes notebook;
update the effect so it always uses the latest handleSave and handleRunAll
references—either by adding handleSave and handleRunAll to the effect dependency
array or by stabilizing those handlers with useCallback (or by storing the
latest functions in refs and calling ref.current inside handleKeyDown). Ensure
the window.addEventListener/removeEventListener uses the current handler closure
to avoid invoking outdated logic.
In `@src/renderer/components/notebook/NotebooksSidebar.tsx`:
- Around line 259-265: The clear-search IconButton that calls handleClearSearch
is icon-only and lacks an accessible name; update the IconButton (the component
rendering the Close icon) to include an explicit accessible label (e.g.,
aria-label="Clear search" or aria-label={t('clearSearch')} if using i18n) and/or
a title prop so screen readers announce its purpose—apply this change to the
IconButton wrapping the Close icon near handleClearSearch to ensure
accessibility.
---
Outside diff comments:
In `@src/main/services/connectors.service.ts`:
- Around line 535-550: Currently
SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id) runs
before NotebooksService.archiveConnectionNotebooks(connectionToDelete.id),
risking removed credentials if archival fails; change the order so call
NotebooksService.archiveConnectionNotebooks(connectionToDelete.id) first and
only call
SecureStorageService.cleanupConnectionCredentials(connectionToDelete.id) after
archive completes successfully, ensuring any thrown error from archive aborts
deletion and prevents credential cleanup; reference connectionToDelete,
SecureStorageService.cleanupConnectionCredentials, and
NotebooksService.archiveConnectionNotebooks when making the change.
---
Duplicate comments:
In `@src/main/services/notebooks.service.ts`:
- Around line 1086-1090: User-supplied segments
(archivedConnectionKey/connectionKey/notebookId) are being used directly in path
operations (e.g., building archivedPath and in deleteAllArchivedNotebooks),
allowing directory traversal; fix by validating/sanitizing these inputs before
joining: use safe components such as path.basename or a whitelist/regex to
reject path separators and traversal sequences, then build paths and verify with
path.resolve that the resulting path starts with ORPHANED_DIR (or otherwise
canonicalize and assert containment) before performing file operations; update
all occurrences including archivedPath construction and the functions/methods
deleteAllArchivedNotebooks, and any code referencing connectionKey/notebookId
around the other noted locations (lines ~1135-1139 and ~1170-1173).
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 161-166: In the NotebookEditor component remove the duplicated
useEffect that syncs localCells from notebook?.cells: find the redundant effect
that checks if (notebook?.cells) then calls setLocalCells(notebook.cells) and
delete it so only the original single sync effect remains; keep the one that
correctly handles updates to notebook?.cells and ensure references to
localCells, setLocalCells, and notebook?.cells are preserved in the remaining
effect.
- Around line 745-747: The render is mutating state by calling .sort on
localCells; instead create a non-mutating copy before sorting (e.g., use
[...localCells] or localCells.slice()) and then call .sort on that copy inside
the JSX map in the NotebookEditor render where localCells.sort(...) is used so
state isn't mutated during render.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/services/connectors.service.tssrc/main/services/duckLake.service.tssrc/main/services/notebooks.service.tssrc/renderer/components/notebook/MarkdownCell.tsxsrc/renderer/components/notebook/NotebookEditor.tsxsrc/renderer/components/notebook/NotebooksSidebar.tsxsrc/renderer/components/notebook/NotebooksTreeView.tsx
Comment on lines
+968
to
+980
| const notebook = await this.getNotebook(connectionId, notebookId); | ||
| if (!notebook) return; | ||
|
|
||
| // Limit output data size to prevent massive files | ||
| const limitedOutput = limitCellOutputData(output); | ||
|
|
||
| const updatedCells = notebook.cells.map((cell) => | ||
| cell.id === cellId ? { ...cell, output: limitedOutput } : cell, | ||
| ); | ||
|
|
||
| await this.updateNotebook(connectionId, notebookId, { | ||
| cells: updatedCells, | ||
| }); |
There was a problem hiding this comment.
Concurrent cell executions can overwrite each other’s outputs.
updateCellOutput does read-modify-write on full cells without per-notebook serialization. Two concurrent runs can each write from stale snapshots and drop one output.
🤖 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 968 - 980, The current
read-modify-write in updateCellOutput (uses getNotebook, limitCellOutputData,
then updateNotebook with the full cells array) can lose concurrent updates;
replace this with an atomic per-cell update or serialization: either (a) add an
atomic DB/collection update that targets the notebook by notebookId and updates
only the matching cell's output (so you no longer replace the whole cells
array), or (b) introduce a per-notebook mutex/lock in notebooks.service.ts
around the get-modify-update sequence (keyed by notebookId) so concurrent
executions serialize; reference the functions/variables getNotebook,
limitCellOutputData, cellId, and updateNotebook and implement the change so the
service writes only the single cell output atomically or under a lock.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
src/renderer/components/notebook/NotebookEditor.tsx (3)
745-747:⚠️ Potential issue | 🟠 MajorAvoid mutating
localCellsduring render.Line 746 sorts state in place (
localCells.sort(...)), which mutates React state during render.Suggested fix
- {localCells + {[...localCells] .sort((a, b) => a.order - b.order) .map((cell, 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 745 - 747, The render is mutating state by calling localCells.sort(...) directly; fix by creating a non-mutating copy and sorting that before mapping (e.g., compute a sortedCells = [...localCells].sort((a,b)=>a.order-b.order) or use Array.from(localCells) and then map over sortedCells) so that NotebookEditor.tsx does not change localCells during render, and update the map call to use sortedCells instead of localCells.sort(...).
161-166:⚠️ Potential issue | 🟡 MinorRemove the duplicated notebook→localCells sync effect.
Line 161 duplicates the same effect from Line 154, causing redundant state writes and extra renders.
Suggested fix
// Sync local cells with notebook data useEffect(() => { if (notebook?.cells) { setLocalCells(notebook.cells); } }, [notebook?.cells]); - // Sync local cells with notebook data - useEffect(() => { - if (notebook?.cells) { - setLocalCells(notebook.cells); - } - }, [notebook?.cells]); - // Cleanup timeout on unmount🤖 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 161 - 166, The file contains a duplicated useEffect that syncs notebook?.cells into the component state; remove the redundant effect so only one sync remains. Locate the duplicated useEffect block in the NotebookEditor component that calls setLocalCells(notebook.cells) when notebook?.cells changes and delete the second occurrence, keeping a single effect to avoid redundant state writes and renders.
239-261:⚠️ Potential issue | 🟠 MajorKeyboard shortcuts still capture stale handlers.
Line 261 depends on
notebookinstead ofhandleSave/handleRunAll, so shortcuts can invoke outdated closures after edits.Suggested fix
- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [notebook]); + }, [handleSave, handleRunAll]);🤖 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 239 - 261, The keyboard shortcut effect captures stale closures because its dependency array lists only `notebook`; update the effect to depend on the actual handler functions (`handleSave` and `handleRunAll`) or stabilize those handlers with `useCallback` and then include them in the dependencies; specifically, either wrap `handleSave` and `handleRunAll` in `useCallback` (so their identities are stable) and change the effect deps to [handleSave, handleRunAll], or leave handlers as-is and change the effect deps to [handleSave, handleRunAll] and remove the eslint-disable comment so React can enforce correct dependencies in `useEffect` that installs `handleKeyDown`.src/renderer/screens/notebooks/index.tsx (1)
1099-1101:⚠️ Potential issue | 🟠 MajorDelete dialog copy is still misleading for active notebooks.
Line 1099-1101 says the notebook will be moved to archived notebooks, but the delete flow is permanent.
Suggested fix
- Are you sure you want to delete the notebook " - {activeNotebookToDelete?.notebookName}"? It will be moved to - archived notebooks. + Are you sure you want to permanently delete the notebook " + {activeNotebookToDelete?.notebookName}"? This action cannot be + undone.🤖 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 1099 - 1101, The delete confirmation text for notebooks is incorrect: the JSX that renders the dialog using activeNotebookToDelete (the fragment that currently says "It will be moved to archived notebooks.") must be updated to accurately state the action is permanent; locate where activeNotebookToDelete?.notebookName is interpolated in the delete dialog (in the component rendering the confirmation modal) and replace the misleading sentence with copy that clearly states the deletion is permanent (e.g., "This action is permanent and cannot be undone."). Ensure the new text is used in the same JSX/variable scope that renders the delete dialog.src/main/services/notebooks.service.ts (2)
1131-1135:⚠️ Potential issue | 🔴 CriticalArchived-path operations still allow traversal via unsanitized segments.
Line 1131, Line 1180, and Line 1215 use raw
archivedConnectionKey/connectionKey/notebookIdin filesystem paths. A crafted../can escapeORPHANED_DIR; at Line 1217 this can become arbitrary recursive deletion.🔒 Suggested hardening for archived path helpers
+function getArchivedConnectionDir(connectionKey: string): string { + const safeConnectionKey = assertSafeSegment( + connectionKey, + 'archived connection key', + ); + const dirPath = path.resolve(ORPHANED_DIR, safeConnectionKey); + const base = `${path.resolve(ORPHANED_DIR)}${path.sep}`; + if (!dirPath.startsWith(base)) { + throw new Error('Invalid archived connection directory - path traversal detected'); + } + return dirPath; +} + +function getArchivedNotebookPath(connectionKey: string, notebookId: string): string { + const safeConnectionKey = assertSafeSegment( + connectionKey, + 'archived connection key', + ); + const safeNotebookId = assertSafeSegment(notebookId, 'notebook id'); + const filePath = path.resolve( + ORPHANED_DIR, + safeConnectionKey, + `${safeNotebookId}.json`, + ); + const base = `${path.resolve(ORPHANED_DIR)}${path.sep}`; + if (!filePath.startsWith(base)) { + throw new Error('Invalid archived notebook path - path traversal detected'); + } + return filePath; +} @@ - const archivedPath = path.join( - ORPHANED_DIR, - archivedConnectionKey, - `${notebookId}.json`, - ); + const archivedPath = getArchivedNotebookPath( + archivedConnectionKey, + notebookId, + ); @@ - const archivedDir = path.join(ORPHANED_DIR, archivedConnectionKey); + const archivedDir = getArchivedConnectionDir(archivedConnectionKey); @@ - const archivedPath = path.join( - ORPHANED_DIR, - connectionKey, - `${notebookId}.json`, - ); + const archivedPath = getArchivedNotebookPath(connectionKey, notebookId); @@ - const archivedDir = path.join(ORPHANED_DIR, connectionKey); + const archivedDir = getArchivedConnectionDir(connectionKey); @@ - const archivedDir = path.join(ORPHANED_DIR, connectionKey); + const archivedDir = getArchivedConnectionDir(connectionKey);Also applies to: 1155-1156, 1180-1184, 1189-1190, 1215-1217
🤖 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 1131 - 1135, The archived-path assembly uses unsanitized archivedConnectionKey/connectionKey/notebookId when building filesystem paths (variables: archivedPath, ORPHANED_DIR, archivedConnectionKey, connectionKey, notebookId), allowing path traversal; fix by normalizing and validating inputs before joining: constrain notebookId to a safe pattern (e.g., alphanumeric + allowed chars) or reduce it to a basename, use path.join then path.resolve and assert the resulting path startsWith(path.resolve(ORPHANED_DIR)) (reject or throw on mismatch), and apply the same sanitization/check to archivedConnectionKey/connectionKey before any file operations (including deletions) to prevent escaping ORPHANED_DIR.
968-980:⚠️ Potential issue | 🟠 MajorConcurrent cell output updates are still non-atomic.
Line 968 reads a snapshot and Line 978 writes a full
cellsarray. ParallelrunCellcalls can overwrite each other and drop outputs.🤖 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 968 - 980, getNotebook followed by mapping notebook.cells and calling updateNotebook is non-atomic and allows concurrent runCell calls to clobber each other's outputs; instead make the update target a single cell atomically (e.g., add/use an updateNotebookCellOutput or extend updateNotebook to perform a DB-level atomic update on the specific cell id) so you set only cells.$[elem].output = limitedOutput (or use an optimistic-version check/cas on notebook.version) rather than replacing the whole cells array; refer to getNotebook, updateNotebook, limitCellOutputData and the runCell call sites and implement the DB atomic update (or versioned compare-and-swap) to avoid lost concurrent updates.
🧹 Nitpick comments (2)
src/renderer/components/notebook/NotebookEditor.tsx (1)
108-152: Consider centralizing SQL completion provider registration to avoid duplicate suggestions.Both
src/renderer/components/notebook/NotebookEditor.tsx(line 119) andsrc/renderer/components/sqlEditor/editorComponent/index.tsx(line 138) register SQL completion providers independently. While they currently operate in separate component hierarchies and each properly manages disposal of its own provider, if both components were to mount together, Monaco would combine results from both providers, potentially duplicating completion entries. Centralizing the registration in a shared hook or context would eliminate this architectural risk and reduce code duplication.🤖 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 108 - 152, The SQL completion provider registration is duplicated between NotebookEditor and the other editor; extract the registration logic into a shared hook or context (e.g., create useSqlCompletionProvider) and replace the direct calls in NotebookEditor (the loader.init call that sets completionProviderRef.current, the provideCompletionItems implementation that uses completionsRef, and the cleanup dispose logic) with the shared hook’s API; ensure the hook accepts dependencies like completionsRef/completionsCount and connectionId so it registers once with monaco.languages.registerCompletionItemProvider and handles disposal internally to prevent duplicate providers.src/renderer/screens/notebooks/index.tsx (1)
1141-1144: ReplaceonKeyPresswithonKeyDownfor Enter key handlers.The
keypressevent is deprecated per web standards;keydownis the recommended replacement. This applies to three TextField components in dialog contexts (lines 1141-1144, 1232-1235, 1282-1285).onKeyDown={(e) => { if (e.key === 'Enter' && newNotebookName.trim()) { handleCreateNotebook(); } }}🤖 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 - 1144, Replace deprecated onKeyPress handlers on the TextField(s) used in the notebook dialogs with onKeyDown handlers: locate the TextField instances that reference onKeyPress and check for Enter with newNotebookName.trim() (the handlers that call handleCreateNotebook()), and change them to use onKeyDown so the Enter key is detected consistently; do this for the three occurrences tied to the create-notebook dialog TextField references near the handlers that read newNotebookName and invoke handleCreateNotebook().
🤖 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 628-629: The pagination values pageLimit and pageOffset (derived
from limit and offset) are used directly in SQL construction and must be
validated and clamped before use: ensure limit and offset are parsed as
integers, default to 10/0 when missing, enforce non-negative values (offset >=
0, limit >= 1) and cap limit to a reasonable maximum (e.g., 100) to prevent huge
scans; apply this validation where pageLimit/pageOffset are computed and before
any query-building code paths that interpolate them so only safe integer values
are injected into the SQL.
- Around line 997-1048: The outer try/catch around the archive flow currently
swallows all errors (including parse/write/rename) so real failures get treated
as "nothing to archive"; update the outer catch in the block that touches
connectionDir and orphanedDir to only suppress missing-directory errors (check
error.code === 'ENOENT' or use NodeJS.ErrnoException) and otherwise rethrow or
surface the error; keep the inner per-file try/catch for individual file
failures (in the jsonFiles.map async handler) but ensure failures from fs.rename
or unexpected errors propagate by not catching them at the outer level.
- Around line 632-635: The helper isSelectQuery currently treats any SQL
starting with WITH as safe to wrap, which lets the COUNT(*) wrapper re-execute
CTEs (including data-modifying CTEs) and cause duplicate side effects; change
isSelectQuery (used by the COUNT-wrapper call sites around the noted locations)
to only return true for queries that start with SELECT (remove the
normalized.startsWith('WITH') branch) so we never auto-wrap WITH queries, or
alternatively add robust parsing to detect and reject data-modifying CTEs before
wrapping; update isSelectQuery and rerun tests to ensure the COUNT-wrapper code
paths (the places invoking isSelectQuery) no longer re-execute WITH queries.
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 210-228: The success toast is always shown regardless of failures
or an early stop; modify the run-all logic (the loop that catches cellError and
uses continueExecution) to track completion status (e.g., a boolean like
allExecuted or a failure counter) and set it to false when a cell fails and the
user chooses to stop (when continueExecution is false) or whenever any cellError
occurs; after the loop (before/to replace the unconditional toast.success('All
cells executed')) display the appropriate toast based on that flag (e.g.,
toast.success when allExecuted is true, otherwise toast.error or toast.info
indicating some cells failed or execution was stopped). Ensure you update the
code paths in the catch block that currently break the loop and in the outer
try/catch (runAllError) to respect and use this flag.
In `@src/renderer/components/notebook/NotebooksTreeView.tsx`:
- Around line 297-303: The label currently only opens notebooks via the onClick
handler, preventing keyboard users from activating it; update the interactive
element in NotebooksTreeView (the element using onClick and onContextMenu) to be
keyboard-accessible by adding a tabIndex (e.g., tabIndex={0}), an appropriate
ARIA role if missing, and an onKeyDown handler that listens for Enter and Space
and calls onOpenNotebook(notebook.id) (and stops propagation similar to the
onClick). Keep handleContextMenu for right-click, but ensure onKeyDown mirrors
onClick behavior so keyboard users can open notebooks.
- Around line 192-210: The empty-state check only considers filteredNotebooks
and misses the case when showArchived is true and the archived list is also
empty; update the condition in NotebooksTreeView (around the filteredNotebooks
check) to also examine the archived filtered list (e.g.,
filteredArchivedNotebooks or the actual archived variable used in the file) so
the empty message is shown when both active and archived filtered lists are
empty: change the if so that when showArchived is false it behaves the same, and
when showArchived is true it requires both filteredNotebooks.length === 0 AND
filteredArchivedNotebooks.length === 0 before returning the empty-message UI;
adjust the message text if you want to reflect archived view.
---
Duplicate comments:
In `@src/main/services/notebooks.service.ts`:
- Around line 1131-1135: The archived-path assembly uses unsanitized
archivedConnectionKey/connectionKey/notebookId when building filesystem paths
(variables: archivedPath, ORPHANED_DIR, archivedConnectionKey, connectionKey,
notebookId), allowing path traversal; fix by normalizing and validating inputs
before joining: constrain notebookId to a safe pattern (e.g., alphanumeric +
allowed chars) or reduce it to a basename, use path.join then path.resolve and
assert the resulting path startsWith(path.resolve(ORPHANED_DIR)) (reject or
throw on mismatch), and apply the same sanitization/check to
archivedConnectionKey/connectionKey before any file operations (including
deletions) to prevent escaping ORPHANED_DIR.
- Around line 968-980: getNotebook followed by mapping notebook.cells and
calling updateNotebook is non-atomic and allows concurrent runCell calls to
clobber each other's outputs; instead make the update target a single cell
atomically (e.g., add/use an updateNotebookCellOutput or extend updateNotebook
to perform a DB-level atomic update on the specific cell id) so you set only
cells.$[elem].output = limitedOutput (or use an optimistic-version check/cas on
notebook.version) rather than replacing the whole cells array; refer to
getNotebook, updateNotebook, limitCellOutputData and the runCell call sites and
implement the DB atomic update (or versioned compare-and-swap) to avoid lost
concurrent updates.
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 745-747: The render is mutating state by calling
localCells.sort(...) directly; fix by creating a non-mutating copy and sorting
that before mapping (e.g., compute a sortedCells =
[...localCells].sort((a,b)=>a.order-b.order) or use Array.from(localCells) and
then map over sortedCells) so that NotebookEditor.tsx does not change localCells
during render, and update the map call to use sortedCells instead of
localCells.sort(...).
- Around line 161-166: The file contains a duplicated useEffect that syncs
notebook?.cells into the component state; remove the redundant effect so only
one sync remains. Locate the duplicated useEffect block in the NotebookEditor
component that calls setLocalCells(notebook.cells) when notebook?.cells changes
and delete the second occurrence, keeping a single effect to avoid redundant
state writes and renders.
- Around line 239-261: The keyboard shortcut effect captures stale closures
because its dependency array lists only `notebook`; update the effect to depend
on the actual handler functions (`handleSave` and `handleRunAll`) or stabilize
those handlers with `useCallback` and then include them in the dependencies;
specifically, either wrap `handleSave` and `handleRunAll` in `useCallback` (so
their identities are stable) and change the effect deps to [handleSave,
handleRunAll], or leave handlers as-is and change the effect deps to
[handleSave, handleRunAll] and remove the eslint-disable comment so React can
enforce correct dependencies in `useEffect` that installs `handleKeyDown`.
In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1099-1101: The delete confirmation text for notebooks is
incorrect: the JSX that renders the dialog using activeNotebookToDelete (the
fragment that currently says "It will be moved to archived notebooks.") must be
updated to accurately state the action is permanent; locate where
activeNotebookToDelete?.notebookName is interpolated in the delete dialog (in
the component rendering the confirmation modal) and replace the misleading
sentence with copy that clearly states the deletion is permanent (e.g., "This
action is permanent and cannot be undone."). Ensure the new text is used in the
same JSX/variable scope that renders the delete dialog.
---
Nitpick comments:
In `@src/renderer/components/notebook/NotebookEditor.tsx`:
- Around line 108-152: The SQL completion provider registration is duplicated
between NotebookEditor and the other editor; extract the registration logic into
a shared hook or context (e.g., create useSqlCompletionProvider) and replace the
direct calls in NotebookEditor (the loader.init call that sets
completionProviderRef.current, the provideCompletionItems implementation that
uses completionsRef, and the cleanup dispose logic) with the shared hook’s API;
ensure the hook accepts dependencies like completionsRef/completionsCount and
connectionId so it registers once with
monaco.languages.registerCompletionItemProvider and handles disposal internally
to prevent duplicate providers.
In `@src/renderer/screens/notebooks/index.tsx`:
- Around line 1141-1144: Replace deprecated onKeyPress handlers on the
TextField(s) used in the notebook dialogs with onKeyDown handlers: locate the
TextField instances that reference onKeyPress and check for Enter with
newNotebookName.trim() (the handlers that call handleCreateNotebook()), and
change them to use onKeyDown so the Enter key is detected consistently; do this
for the three occurrences tied to the create-notebook dialog TextField
references near the handlers that read newNotebookName and invoke
handleCreateNotebook().
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/services/notebooks.service.tssrc/renderer/components/notebook/NotebookEditor.tsxsrc/renderer/components/notebook/NotebooksTreeView.tsxsrc/renderer/screens/notebooks/index.tsx
Address React state mutations, accessibility gaps, SQL injection vulnerabilities, race conditions, and error handling issues across notebook editor components. Key improvements: - Fix state mutations and stale closures in React components - Add comprehensive keyboard accessibility (ARIA labels, Enter/Space handlers) - Escape SQL file paths and validate pagination inputs - Prevent WITH query re-execution and out-of-order pagination responses - Add bigint serialization and SQL normalization for exports - Improve error handling and user-facing messages - Add reentrancy guards for concurrent operations Files: 11 components and services modified Issues: All 25 CodeRabbit findings resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI
Chores
Documentation