feat: Deepnote style data frames #28
Conversation
Artmann
changed the title
📝 WalkthroughWalkthroughThis PR introduces a new webview-side dataframe renderer with Tailwind-based styling and wires renderer messaging to an extension-side DataframeController for pagination/state updates. The build system adds PostCSS/Tailwind processing for specific CSS and a new esbuild entry for the dataframe renderer bundle. Kernel execution now generates Python code via @deepnote/blocks using blocks created from notebook cells, and output metadata propagation gains cellId/cellIndex to support renderers. Deepnote pocket handling moves id to top-level metadata and stops storing outputs in the pocket. Multiple components add localization, toolkit installer becomes version-aware, CI gains packages: read, and new docs (blocks.md) replace serialization.md. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Renderer as DataframeRenderer (webview)
participant Messaging as VSCode Renderer Messaging
participant Ext as DataframeController (extension)
participant VSCode as VS Code Notebook API
participant Kernel as Python Kernel
User->>Renderer: Change page size / navigate
Renderer->>Messaging: postMessage({ command, cellId, ... })
Messaging->>Ext: onDidReceiveMessage({ editor, message })
Ext->>VSCode: ApplyNotebookEdit(update deepnote_table_state)
Ext->>VSCode: Re-execute cell (by cellId)
VSCode->>Kernel: Execute generated code
note over Kernel,VSCode: Code produced via createPythonCode(...)
Kernel-->>VSCode: Outputs with metadata (cellId, cellIndex, ...)
VSCode-->>Renderer: Render output item
Renderer->>Renderer: Re-render table with new state
sequenceDiagram
autonumber
participant Cell as NotebookCell
participant Converter as Pocket/Converter
participant Blocks as createBlockFromPocket
participant Codegen as createPythonCode (@deepnote/blocks)
participant Kernel as Python Kernel
Cell->>Converter: Extract pocket + top-level metadata (id at top-level)
Converter->>Blocks: createBlockFromPocket(pocket, metadata)
Blocks-->>Converter: DeepnoteBlock (outputs not taken from pocket)
Converter->>Codegen: createPythonCode(block)
Codegen-->>Kernel: Python code
Kernel-->>Cell: Outputs (with cellId/cellIndex in metadata)
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
build/esbuild/build.ts(1 hunks)package.json(1 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)src/webviews/webview-side/dataframe-renderer/styles.css(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports
Files:
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/webviews/webview-side/dataframe-renderer/index.tsbuild/esbuild/build.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/webviews/webview-side/dataframe-renderer/index.tsbuild/esbuild/build.ts
🧠 Learnings (1)
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts wires VSCode activation
Applied to files:
src/webviews/webview-side/dataframe-renderer/index.ts
🧬 Code graph analysis (2)
src/webviews/webview-side/dataframe-renderer/index.ts (1)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (1)
DataframeRenderer(7-11)
build/esbuild/build.ts (6)
gulpfile.js (1)
path(14-14)build/launchWebUtils.js (1)
path(4-4)build/preLaunchWebTest.js (1)
path(4-4)src/kernels/common/baseJupyterSessionConnection.ts (1)
path(23-25)build/ci/postInstall.js (1)
path(8-8)build/webpack/common.js (1)
path(6-6)
🔇 Additional comments (2)
src/webviews/webview-side/dataframe-renderer/styles.css (1)
1-42: LGTM!Comprehensive dataframe table styling with proper VSCode theme integration.
package.json (1)
1812-1821: LGTM!Renderer registration correctly configured. Entrypoint path matches the build output.
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
Outdated
Show resolved
Hide resolved
| const root = document.createElement('div'); | ||
| element.appendChild(root); | ||
|
|
||
| ReactDOM.render(React.createElement(DataframeRenderer, { data }), root); |
There was a problem hiding this comment.
Add React cleanup in disposeOutputItem.
ReactDOM.render mounts a component but disposeOutputItem doesn't unmount it, causing memory leaks when outputs are disposed.
Store the root element reference and call ReactDOM.unmountComponentAtNode(root) in disposeOutputItem:
export const activate: ActivationFunction = (_context: RendererContext<unknown>) => {
+ const roots = new Map<string, HTMLElement>();
+
return {
renderOutputItem(outputItem: OutputItem, element: HTMLElement) {
try {
const data = outputItem.json();
console.log('Dataframe renderer - received data:', data);
const root = document.createElement('div');
element.appendChild(root);
+ roots.set(outputItem.id, root);
ReactDOM.render(React.createElement(DataframeRenderer, { data }), root);
} catch (error) {
console.error('Error rendering dataframe:', error);
const errorDiv = document.createElement('div');
errorDiv.style.padding = '10px';
errorDiv.style.color = 'var(--vscode-errorForeground)';
errorDiv.textContent = `Error rendering dataframe: ${error}`;
element.appendChild(errorDiv);
}
},
- disposeOutputItem(_id?: string) {
- // Cleanup if needed
+ disposeOutputItem(id?: string) {
+ if (id) {
+ const root = roots.get(id);
+ if (root) {
+ ReactDOM.unmountComponentAtNode(root);
+ roots.delete(id);
+ }
+ }
}
};
};Also applies to: 31-33
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/index.ts around lines 20 and
also lines 31-33, ReactDOM.render is used to mount DataframeRenderer but
disposeOutputItem never unmounts it, causing memory leaks; modify the module to
store the root DOM element (or React root reference) when rendering and then
call ReactDOM.unmountComponentAtNode(root) inside disposeOutputItem (or use the
appropriate ReactDOM.unmount API for your React version) to unmount and clean up
the component before removing the element.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
src/webviews/webview-side/dataframe-renderer/index.ts (3)
13-16: Use ILogger service instead of console.log.Per coding guidelines, use ILogger for logging.
As per coding guidelines.
23-23: Use ILogger service instead of console.error.Per coding guidelines, use ILogger for error logging.
As per coding guidelines.
21-21: Add React cleanup in disposeOutputItem.ReactDOM.render mounts components without unmounting in disposeOutputItem, causing memory leaks.
Store root element and call ReactDOM.unmountComponentAtNode(root) in disposeOutputItem. See previous review for full diff.
Also applies to: 32-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
package.json(1 hunks)src/kernels/deepnote/deepnoteController.ts(1 hunks)src/kernels/execution/cellExecution.ts(1 hunks)src/notebooks/controllers/vscodeNotebookController.ts(3 hunks)src/webviews/extension-side/dataframe/rendererComms.ts(1 hunks)src/webviews/extension-side/serviceRegistry.node.ts(2 hunks)src/webviews/extension-side/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)src/webviews/webview-side/dataframe-renderer/styles.css(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/webviews/extension-side/serviceRegistry.web.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports
Files:
src/webviews/extension-side/serviceRegistry.web.tssrc/kernels/deepnote/deepnoteController.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/webviews/extension-side/serviceRegistry.node.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/webviews/extension-side/dataframe/rendererComms.tssrc/kernels/execution/cellExecution.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/deepnote/deepnoteController.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/webviews/extension-side/dataframe/rendererComms.tssrc/kernels/execution/cellExecution.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/deepnoteController.tssrc/kernels/execution/cellExecution.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/webviews/extension-side/serviceRegistry.node.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecution.ts
🧠 Learnings (9)
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/webviews/extension-side/ipywidgets/rendererComms.ts : RendererComms should track widget model IDs in outputs, handle renderer queries, and coordinate display messages with IPyWidgetMessageDispatcher
Applied to files:
src/webviews/extension-side/serviceRegistry.web.tssrc/webviews/extension-side/dataframe/rendererComms.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/deepnote/deepnoteController.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/kernels/execution/cellExecution.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Create a UI provider for kernel selection for the new kernel source
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Register the new kernel source in KernelSourceCommandHandler
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/interactiveWindowController.ts : InteractiveWindowController must manage kernel selection/startup/lifecycle, system info cell handling, restart logic, and maintain owner file associations for kernel context
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`
Applied to files:
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/webviews/webview-side/dataframe-renderer/index.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts wires VSCode activation
Applied to files:
src/webviews/webview-side/dataframe-renderer/index.ts
🧬 Code graph analysis (6)
src/webviews/extension-side/serviceRegistry.web.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/kernels/deepnote/deepnoteController.ts (1)
src/kernels/kernelController.ts (1)
KernelController(7-15)
src/notebooks/controllers/vscodeNotebookController.ts (2)
src/kernels/deepnote/deepnoteController.ts (1)
DeepnoteController(19-28)src/kernels/types.ts (1)
IKernelController(930-933)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (3)
gulpfile.js (1)
key(160-160)src/test/mocks/vsc/extHostedTypes.ts (1)
updateCellMetadata(2480-2482)src/test/datascience/ipywidgets/samples/jupyter-require/core.js (1)
context(348-351)
src/webviews/extension-side/serviceRegistry.node.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/webviews/webview-side/dataframe-renderer/index.ts (2)
src/webviews/extension-side/dataframe/rendererComms.ts (1)
activate(49-52)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (1)
DataframeRenderer(23-70)
🪛 Biome (2.1.2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
[error] 42-42: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🪛 GitHub Actions: CI
src/kernels/deepnote/deepnoteController.ts
[error] 20-20: ESLint: Useless constructor. Remove the constructor or use it to initialize fields. @typescript-eslint/no-useless-constructor
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
[error] 24-24: TS6133: 'resultsPerPage' is declared but its value is never read.
🪛 GitHub Check: Lint & Format
src/kernels/deepnote/deepnoteController.ts
[failure] 26-26:
'DeepnoteNotebookCellExecution' was used before it was defined
[failure] 20-20:
Useless constructor
src/webviews/extension-side/dataframe/rendererComms.ts
[failure] 72-72:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
[failure] 66-66:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
🔇 Additional comments (14)
package.json (1)
1812-1821: LGTM!Notebook renderer registration is correct. MIME type and entry point align with the new dataframe renderer implementation.
src/notebooks/controllers/vscodeNotebookController.ts (1)
551-551: LGTM!DeepnoteController integration follows the existing pattern. The wrapper intercepts cell execution at the right points.
Also applies to: 564-564
src/webviews/extension-side/serviceRegistry.web.ts (1)
30-30: LGTM!Service registration follows the established pattern for renderer communication services.
Also applies to: 69-72
src/webviews/extension-side/serviceRegistry.node.ts (1)
20-20: LGTM!Mirrors web registry. Consistent service registration across runtimes.
Also applies to: 69-72
src/webviews/extension-side/dataframe/rendererComms.ts (7)
1-15: Imports follow guidelines.Third-party and local imports are properly separated, and ILogger is correctly used.
17-39: Well-structured command types.Clear, type-safe definitions for all commands.
41-47: Proper lifecycle management.Injectable service with correct disposal pattern.
49-52: Activation setup is correct.Renderer messaging channel established with proper event subscription.
80-98: Cell metadata update is correct.Proper use of WorkspaceEdit and NotebookEdit APIs with null checking.
100-108: Placeholder ready for enhancement.Current implementation logs activity. Comment indicates future metadata storage option.
110-113: Clipboard integration is correct.Proper use of VS Code clipboard API.
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (3)
1-2: Imports are correct.
4-21: Props interface is well-defined.Clear type safety for renderer props.
44-67: Rendering logic is correct.Column filtering, row mapping, and layout structure are properly implemented.
| constructor(controller: NotebookController) { | ||
| super(controller); | ||
| } |
There was a problem hiding this comment.
Remove useless constructor.
Constructor only calls super(). TypeScript doesn't require explicit super() constructor when no additional logic is needed.
Apply this diff:
export class DeepnoteController extends KernelController {
- constructor(controller: NotebookController) {
- super(controller);
- }
-
public override createNotebookCellExecution(cell: NotebookCell): NotebookCellExecution {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(controller: NotebookController) { | |
| super(controller); | |
| } | |
| export class DeepnoteController extends KernelController { | |
| public override createNotebookCellExecution(cell: NotebookCell): NotebookCellExecution { |
🧰 Tools
🪛 GitHub Actions: CI
[error] 20-20: ESLint: Useless constructor. Remove the constructor or use it to initialize fields. @typescript-eslint/no-useless-constructor
🪛 GitHub Check: Lint & Format
[failure] 20-20:
Useless constructor
🤖 Prompt for AI Agents
In src/kernels/deepnote/deepnoteController.ts around lines 20 to 22, the
constructor only calls super(controller) and is therefore redundant; remove the
explicit constructor entirely so the class uses the implicit parent constructor,
leaving no other logic changes.
| switch (message.command) { | ||
| case 'selectPageSize': | ||
| this.handleSelectPageSize(editor, message); | ||
| break; | ||
| case 'goToPage': | ||
| this.handleGoToPage(editor, message); | ||
| break; | ||
| case 'copyTableData': | ||
| this.handleCopyTableData(message); | ||
| break; | ||
| case 'exportDataframe': | ||
| this.handleExportDataframe(editor, message); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Await async handlers to prevent silent failures.
handleSelectPageSize and handleCopyTableData are async but called without await. Errors will be silently swallowed, and execution order becomes non-deterministic.
Apply this diff:
switch (message.command) {
case 'selectPageSize':
- this.handleSelectPageSize(editor, message);
+ void this.handleSelectPageSize(editor, message);
break;
case 'goToPage':
this.handleGoToPage(editor, message);
break;
case 'copyTableData':
- this.handleCopyTableData(message);
+ void this.handleCopyTableData(message);
break;
case 'exportDataframe':
this.handleExportDataframe(editor, message);
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (message.command) { | |
| case 'selectPageSize': | |
| this.handleSelectPageSize(editor, message); | |
| break; | |
| case 'goToPage': | |
| this.handleGoToPage(editor, message); | |
| break; | |
| case 'copyTableData': | |
| this.handleCopyTableData(message); | |
| break; | |
| case 'exportDataframe': | |
| this.handleExportDataframe(editor, message); | |
| break; | |
| } | |
| switch (message.command) { | |
| case 'selectPageSize': | |
| void this.handleSelectPageSize(editor, message); | |
| break; | |
| case 'goToPage': | |
| this.handleGoToPage(editor, message); | |
| break; | |
| case 'copyTableData': | |
| void this.handleCopyTableData(message); | |
| break; | |
| case 'exportDataframe': | |
| this.handleExportDataframe(editor, message); | |
| break; | |
| } |
🧰 Tools
🪛 GitHub Check: Lint & Format
[failure] 72-72:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
[failure] 66-66:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/rendererComms.ts around lines 64 to 77,
async handlers (handleSelectPageSize and handleCopyTableData) are invoked
without awaiting, which can swallow errors and make ordering non-deterministic;
update the switch to await these async calls (e.g., await
this.handleSelectPageSize(editor, message); await
this.handleCopyTableData(message);) and ensure the enclosing function is
declared async (or otherwise returns/handles the resulting Promise) so errors
propagate and execution order is preserved.
| private handleExportDataframe(editor: NotebookEditor, message: ExportDataframeCommand) { | ||
| const cell = editor.notebook.cellAt(message.cellIndex); | ||
| logger.info( | ||
| `[DataframeRenderer] exportDataframe called for cell ${ | ||
| message.cellIndex | ||
| } (${cell?.document.uri.toString()})` | ||
| ); | ||
| // TODO: Implement dataframe export functionality | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO: Implement export functionality.
Export feature is a placeholder. Consider opening an issue to track this requirement.
Do you want me to open an issue to track the dataframe export implementation?
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
Outdated
Show resolved
Hide resolved
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
Outdated
Show resolved
Hide resolved
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
======================================
Coverage 70% 70%
======================================
Files 512 513 +1
Lines 37506 37667 +161
Branches 4811 4819 +8
======================================
+ Hits 26473 26621 +148
- Misses 9441 9447 +6
- Partials 1592 1599 +7
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)
365-369: Avoid fsPath in provider handle (stability/portability)Handles should not contain OS-specific paths/backslashes. Use the venv hash for a stable identifier.
- const serverProviderHandle: JupyterServerProviderHandle = { + const serverProviderHandle: JupyterServerProviderHandle = { extensionId: JVSC_EXTENSION_ID, id: 'deepnote-server', - handle: `deepnote-toolkit-server-${baseFileUri.fsPath}` + handle: `deepnote-toolkit-server-${this.toolkitInstaller.getVenvHash(baseFileUri)}` };Based on learnings
524-547: Sanitize loading controller idController id embeds full file path. Prefer a short, stable id (e.g., use venv hash) to avoid length/special-char issues.
- const loadingController = notebooks.createNotebookController( - `deepnote-loading-${notebookKey}`, + const loadingController = notebooks.createNotebookController( + `deepnote-loading-${this.toolkitInstaller.getVenvHash(notebook.uri.with({ query: '', fragment: '' }))}`, DEEPNOTE_NOTEBOOK_TYPE, 'Loading Deepnote Kernel...' );As per coding guidelines
♻️ Duplicate comments (6)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (3)
46-46: Replace console.log with ILogger service.Multiple console.log statements violate coding guidelines.
As per coding guidelines.
Also applies to: 52-52, 65-65, 74-74, 82-82, 91-91
153-164: Add explicit button type.Missing type="button" can cause unintended form submission.
Apply this diff:
<button + type="button" className={`
170-181: Add explicit button type.Missing type="button" can cause unintended form submission.
Apply this diff:
<button + type="button" className={`src/webviews/webview-side/dataframe-renderer/index.ts (2)
21-55: Replace console logging with ILoggerConsole logging breaches the project logging rule—wire this through
ILogger(or the approved webview logging bridge) instead.
As per coding guidelines
18-61: Unmount React roots on disposeRoots stay mounted, leaking listeners. Track each
outputItem.id, callReactDOM.unmountComponentAtNode, and drop the node duringdisposeOutputItem.-export const activate: ActivationFunction = (context: RendererContext<unknown>) => { - return { +export const activate: ActivationFunction = (context: RendererContext<unknown>) => { + const roots = new Map<string, HTMLElement>(); + + return { renderOutputItem(outputItem: OutputItem, element: HTMLElement) { @@ - ReactDOM.render( + ReactDOM.render( React.createElement(DataframeRenderer, { context, data, metadata: dataFrameMetadata, cellId, cellIndex }), root ); + if (outputItem.id) { + roots.set(outputItem.id, root); + } } catch (error) { @@ - disposeOutputItem(_id?: string) { - // Cleanup if needed + disposeOutputItem(id?: string) { + if (!id) { + return; + } + const root = roots.get(id); + if (root) { + ReactDOM.unmountComponentAtNode(root); + root.remove(); + roots.delete(id); + } } }; };build/esbuild/build.ts (1)
321-324: Watch flag should follow other webview builds.This renderer should also use
watch: watchAllso it rebuilds undernpm run watch-all, just like the other webview bundles. The earlier feedback still applies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
CLAUDE.md(1 hunks)DEEPNOTE_KERNEL_IMPLEMENTATION.md(5 hunks)README.md(1 hunks)build/esbuild/build.ts(5 hunks)package.json(5 hunks)postcss.config.js(1 hunks)src/extension.common.ts(2 hunks)src/kernels/deepnote/deepnoteToolkitInstaller.node.ts(6 hunks)src/kernels/deepnote/types.ts(2 hunks)src/kernels/execution/cellExecution.ts(3 hunks)src/kernels/execution/cellExecutionMessageHandler.ts(3 hunks)src/kernels/execution/helpers.ts(9 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(8 hunks)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts(3 hunks)src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts(1 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts(9 hunks)src/notebooks/deepnote/deepnoteNotebookManager.ts(2 hunks)src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts(1 hunks)src/notebooks/deepnote/pocket.ts(3 hunks)src/notebooks/deepnote/pocket.unit.test.ts(7 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/types.ts(1 hunks)src/platform/logging/types.ts(1 hunks)src/standalone/intellisense/resolveCompletionItem.unit.test.ts(1 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)src/webviews/extension-side/serviceRegistry.node.ts(2 hunks)src/webviews/extension-side/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)src/webviews/webview-side/dataframe-renderer/tailwind.css(1 hunks)tailwind.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/webviews/extension-side/serviceRegistry.web.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/webviews/extension-side/serviceRegistry.web.tssrc/platform/logging/types.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/notebooks/deepnote/deepnoteRequirementsHelper.node.tssrc/kernels/deepnote/types.tssrc/notebooks/types.tssrc/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/extension.common.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/kernels/execution/helpers.tssrc/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/webviews/extension-side/serviceRegistry.node.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tsbuild/esbuild/build.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/logging/types.tssrc/kernels/deepnote/types.tssrc/notebooks/types.tssrc/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/extension.common.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/kernels/execution/helpers.tssrc/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/pocket.tsbuild/esbuild/build.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/logging/types.ts
src/platform/logging/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
Implement logging using OutputChannelLogger/ConsoleLogger and honor LogLevel; ensure PII scrubbing in log output
Files:
src/platform/logging/types.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.tssrc/notebooks/serviceRegistry.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/webviews/extension-side/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/execution/helpers.tssrc/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
src/notebooks/deepnote/deepnoteNotebookManager.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteNotebookManager.ts handles Deepnote notebook state management
Files:
src/notebooks/deepnote/deepnoteNotebookManager.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/helpers.tssrc/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs Deepnote data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧠 Learnings (24)
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/**/ioc/types.ts : Define dependency injection tokens as Symbols in types files (e.g., export const IFoo = Symbol('IFoo')) and declare matching TypeScript interfaces
Applied to files:
src/platform/logging/types.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`
Applied to files:
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/webviews/webview-side/dataframe-renderer/index.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-07T19:51:40.201Z
Learnt from: m1so
PR: deepnote/vscode-deepnote#25
File: src/kernels/deepnote/deepnoteServerStarter.node.ts:150-154
Timestamp: 2025-10-07T19:51:40.201Z
Learning: The deepnote-toolkit server is a separate Python package maintained in a different repository from the VS Code extension (vscode-deepnote). The VS Code extension launches the toolkit as a subprocess and communicates via environment variables and HTTP. The toolkit implementation cannot be verified from the extension codebase.
Applied to files:
src/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/kernels/deepnote/types.tssrc/notebooks/types.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Use Uri.joinPath() to construct file paths instead of manual string concatenation
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/serviceRegistry.{node,web}.ts : Use dependency injection via the service registry for component creation and registration
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Register the new kernel source in KernelSourceCommandHandler
Applied to files:
src/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-25T13:16:13.796Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
Applied to files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-09-25T09:58:15.799Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:37-38
Timestamp: 2025-09-25T09:58:15.799Z
Learning: In Deepnote environments, the virtual environment must be created at ~/venv path because deepnote-toolkit expects this specific location.
Applied to files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks
Applied to files:
src/notebooks/deepnote/deepnoteNotebookManager.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecution.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/kernels/execution/cellExecutionMessageHandler.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/types.ts : Use the shared core interfaces/types from types.ts across the IPyWidgets controller code
Applied to files:
src/webviews/extension-side/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/message/ipyWidgetMessageDispatcher.ts : On kernel restarts, clear pending state, resolve waiting promises, and re-establish hooks and comm targets
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/jupyterKernelSpec.ts : Manage kernel spec registration and updates; create temporary kernel specs for non-standard kernels when needed
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/notebookIPyWidgetCoordinator.ts : NotebookIPyWidgetCoordinator should manage per-notebook lifecycle, controller selection, webview attachment, and creation of CommonMessageCoordinator instances
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (17)
src/webviews/extension-side/serviceRegistry.web.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (3)
gulpfile.js (1)
key(160-160)src/test/datascience/ipywidgets/samples/jupyter-require/core.js (1)
context(348-351)src/notebooks/controllers/vscodeNotebookController.ts (1)
label(113-115)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (2)
src/platform/logging/types.ts (2)
ILogger(9-9)ILogger(10-19)src/test/mocks/vsc/index.ts (1)
CancellationToken(100-112)
src/standalone/intellisense/resolveCompletionItem.unit.test.ts (1)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(132-170)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (2)
IDeepnoteInitNotebookRunner(283-283)IDeepnoteInitNotebookRunner(284-286)src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (2)
IDeepnoteRequirementsHelper(142-142)IDeepnoteRequirementsHelper(143-145)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/kernels/deepnote/types.ts (1)
DEEPNOTE_TOOLKIT_VERSION(157-157)src/platform/logging/index.ts (1)
logger(35-48)
src/webviews/webview-side/dataframe-renderer/index.ts (2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)
DataframeMetadata(6-8)DataframeRenderer(39-192)src/webviews/extension-side/dataframe/dataframeController.ts (1)
activate(53-56)
src/notebooks/deepnote/pocket.unit.test.ts (1)
src/notebooks/deepnote/pocket.ts (2)
extractPocketFromCellMetadata(44-46)createBlockFromPocket(48-84)
src/extension.common.ts (2)
src/platform/logging/types.ts (2)
ILogger(9-9)ILogger(10-19)src/platform/logging/index.ts (1)
logger(35-48)
src/kernels/execution/helpers.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
src/kernels/execution/cellExecution.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)build/webTestReporter.js (1)
dedent(9-9)
src/kernels/execution/cellExecutionMessageHandler.ts (2)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(132-170)src/platform/common/utils.ts (2)
concatMultilineString(211-225)formatStreamText(275-278)
src/webviews/extension-side/dataframe/dataframeController.ts (3)
src/platform/common/types.ts (1)
IDisposable(211-213)src/platform/logging/index.ts (1)
logger(35-48)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteOutput(23-51)src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
src/webviews/extension-side/serviceRegistry.node.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/notebooks/deepnote/pocket.ts (1)
src/notebooks/deepnote/dataConversionUtils.ts (1)
generateBlockId(8-15)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (6)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (2)
IDeepnoteInitNotebookRunner(283-283)IDeepnoteInitNotebookRunner(284-286)src/notebooks/types.ts (2)
IDeepnoteNotebookManager(27-27)IDeepnoteNotebookManager(28-37)src/kernels/types.ts (3)
IKernelProvider(548-548)IKernelProvider(549-564)IKernel(439-446)src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (2)
IDeepnoteRequirementsHelper(142-142)IDeepnoteRequirementsHelper(143-145)src/kernels/deepnote/types.ts (1)
DEEPNOTE_NOTEBOOK_TYPE(162-162)src/test/mocks/vsc/index.ts (1)
CancellationTokenSource(114-125)
🪛 Biome (2.1.2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
[error] 153-164: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 170-181: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🪛 ESLint
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
[error] 19-19: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 25-25: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 markdownlint-cli2 (0.18.1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md
371-371: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
372-372: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
377-377: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
378-378: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
384-384: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
385-385: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
390-390: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
391-391: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
397-397: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
398-398: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (39)
CLAUDE.md (1)
4-4: LGTM.Clear guidance on platform-correct path construction.
Based on learnings.
src/standalone/intellisense/resolveCompletionItem.unit.test.ts (1)
549-549: LGTM.Explicit wrapper enables future parameter passing (cellIndex, cellId) as cellOutputToVSCCellOutput signature evolved.
src/webviews/extension-side/serviceRegistry.node.ts (1)
20-20: LGTM.Standard DI registration for DataframeController follows established patterns.
Also applies to: 69-69
src/webviews/webview-side/dataframe-renderer/tailwind.css (1)
1-3: LGTM.Standard Tailwind CSS directives.
src/notebooks/serviceRegistry.node.ts (1)
56-57: LGTM.DI registrations for Deepnote init-notebook services follow established patterns.
Also applies to: 140-141
postcss.config.js (1)
1-6: LGTM.Standard PostCSS configuration with Tailwind v4 and autoprefixer.
Based on learnings.
README.md (1)
12-13: LGTM.Documentation accurately reflects new init-notebook and dependency management capabilities.
src/extension.common.ts (2)
40-40: LGTM.Import follows DI token pattern.
131-131: LGTM.Singleton registration follows established pattern and enables DI-based logging access.
src/platform/logging/types.ts (1)
9-9: LGTM.Symbol export follows DI token pattern. Based on learnings.
src/notebooks/deepnote/pocket.ts (4)
6-8: LGTM.Comments clearly document the intentional exclusion of
idfrom pocket fields.
11-18: LGTM.Interface update consistent with field list change.
52-59: LGTM.Extraction and cleanup logic properly handles id migration from metadata to block.id.
69-69: LGTM.Fallback ensures block always has an id.
src/notebooks/types.ts (1)
35-36: LGTM.Interface extension supports per-project init-notebook tracking with clear method signatures.
src/notebooks/deepnote/deepnoteNotebookManager.ts (3)
15-15: LGTM.Set provides efficient tracking of init-notebook execution state per project.
78-85: LGTM.Method correctly implements interface contract with clear documentation.
87-93: LGTM.Method correctly implements interface contract with clear documentation.
src/webviews/extension-side/serviceRegistry.web.ts (2)
30-30: LGTM.Import follows established pattern.
69-69: LGTM.Registration follows established activation service pattern.
src/notebooks/deepnote/deepnoteDataConverter.ts (7)
31-31: LGTM.Per-block indexing enables cellIndex/cellId metadata propagation.
Also applies to: 57-57
205-209: LGTM.Signature extension supports metadata propagation with clear parameter names.
220-225: LGTM.Metadata propagation for error outputs is consistent and clear.
286-299: LGTM.Metadata handling properly preserves existing fields while adding cell identity.
312-320: LGTM.Stream output metadata consistent with other output types.
325-333: LGTM.Unknown output metadata handling consistent with known output types.
342-350: LGTM.Fallback output metadata completes consistent propagation across all output paths.
src/kernels/execution/cellExecution.ts (3)
35-35: LGTM.Import supports clean formatting of prepended Python code.
532-538: LGTM.Escape order is correct (backslashes first) and sufficient for embedding JSON strings.
410-434: LGTM.Table state injection is well-structured with proper logging and safe string escaping.
src/webviews/extension-side/dataframe/dataframeController.ts (1)
1-1: Add the standard header.All new TypeScript sources need the Microsoft copyright/license header; please add it.
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-09T11:21:57.494Z Learning: Applies to **/*.{ts,tsx} : Do not add the Microsoft copyright header to new filesLearnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-09T11:21:57.494Z Learning: Use dependency injection with inversifyLearnt from: CR PR: deepnote/vscode-extension#0 File: .github/instructions/platform.instructions.md:0-0 Timestamp: 2025-09-03T13:00:18.307Z Learning: Applies to src/platform/**/*.ts : Use Inversify decorators for DI: annotate classes with injectable() and inject dependencies with inject(Token)src/kernels/execution/helpers.ts (7)
118-121: LGTM: Mapper signatures accept cell provenanceMap now carries (cellIndex, cellId) through. Good.
132-168: LGTM: Propagation in cellOutputToVSCCellOutputOptional args forwarded; sensible fallback to display_data.
172-185: LGTM: Metadata enriched with cellIndex/cellIdNon-invasive; fields are optional and won’t leak into ipynb save path.
219-222: LGTM: Display translator now includes provenance metadataForwarding through getOutputMetadata looks correct.
256-260: LGTM: Stream translator uses enriched metadataNo behavior change; adds provenance.
303-311: LGTM: CellOutputMetadata extendedFields are optional; no breaking change.
571-582: LGTM: Error translator attaches originalError with provenanceGood for diagnostics; maintains existing shape.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
401-409: Kernel spec selection: good; keep hash in one placeUsing getVenvHash aligns selector with installer. Nice.
| private async handleGoToPage(editor: NotebookEditor, message: GoToPageCommand) { | ||
| let cell; | ||
| let cellIndex: number; | ||
|
|
||
| // Try to find cell by cellId first (more reliable) | ||
| if (message.cellId) { | ||
| const cells = editor.notebook.getCells(); | ||
| const foundCell = cells.find((c) => c.metadata.id === message.cellId); | ||
|
|
||
| if (foundCell) { | ||
| cell = foundCell; | ||
| cellIndex = foundCell.index; | ||
| logger.info(`[DataframeController] Found cell by cellId ${message.cellId} at index ${cellIndex}`); | ||
| } else { | ||
| const errorMessage = `Unable to navigate to page: Could not find the cell with ID ${message.cellId}. The cell may have been deleted.`; | ||
| logger.error(`[DataframeController] ${errorMessage}`); | ||
| await window.showErrorMessage(errorMessage); | ||
| throw new Error(errorMessage); | ||
| } | ||
| } else { | ||
| const errorMessage = | ||
| 'Unable to navigate to page: No cell identifier provided. ' + | ||
| 'Please re-run the cell to update the output metadata.'; | ||
| logger.error(`[DataframeController] ${errorMessage}`); | ||
| await window.showErrorMessage(errorMessage); | ||
| throw new Error(errorMessage); | ||
| } |
There was a problem hiding this comment.
Restore the cellIndex fallback.
goToPage errors whenever the renderer only supplies cellIndex (common before metadata includes id). handleSelectPageSize still supports that path; goToPage should mirror it to avoid breaking paging on those cells.
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around lines 149
to 175, the current logic throws if message.cellId is missing or not found, but
the renderer may supply only message.cellIndex; restore the fallback so
handleGoToPage also accepts and uses message.cellIndex when cellId is absent or
lookup by id fails. Specifically: if message.cellId lookup fails, check for a
valid numeric message.cellIndex within editor.notebook.cellCount and select that
cell (set cell and cellIndex) instead of erroring; likewise if message.cellId is
undefined initially, use message.cellIndex when present; ensure you log which
path was used (found by id or by index) and only show/throw an error if neither
id nor a valid index is available.
There was a problem hiding this comment.
We want to throw an error if there is no cell with the given ID.
There was a problem hiding this comment.
@Artmann, understood—I've noted that handleGoToPage should strictly require a valid cellId and throw if it's missing or not found. I'll keep that in mind for future reviews.
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
Outdated
Show resolved
Hide resolved
| // Including the version ensures a new venv is created when the toolkit version changes | ||
| const hash = this.getVenvHash(deepnoteFileUri); | ||
| return Uri.joinPath(this.context.globalStorageUri, 'deepnote-venvs', hash); | ||
| return Uri.joinPath(this.context.globalStorageUri, 'deepnote-venvs', `${hash}-${DEEPNOTE_TOOLKIT_VERSION}`); |
There was a problem hiding this comment.
This creates a new venv if there is a new version of the Deepnote Toolkit.
Artmann
force-pushed
the
chris/display-data-frames
branch
from
930dcde to
1ba9ab4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3)
133-146: Don’t nuke existing venvs unless they’re broken.Delete only if the interpreter is missing; otherwise skip re‑creating and proceed to pip installs. This avoids unnecessary churn and speeds upgrades.
Apply:
- // Remove old venv if it exists but is broken - if (await this.fs.exists(venvPath)) { - logger.info('Removing existing broken venv'); - await workspace.fs.delete(venvPath, { recursive: true }); - } + // Remove old venv only if broken (no interpreter inside it) + const pythonInVenv = + process.platform === 'win32' + ? Uri.joinPath(venvPath, 'Scripts', 'python.exe') + : Uri.joinPath(venvPath, 'bin', 'python'); + if (await this.fs.exists(venvPath)) { + if (!(await this.fs.exists(pythonInVenv))) { + logger.info('Removing existing broken venv'); + await workspace.fs.delete(venvPath, { recursive: true }); + } else { + logger.info('Using existing venv'); + } + } @@ - const venvResult = await processService.exec(baseInterpreter.uri.fsPath, ['-m', 'venv', venvPath.fsPath], { - throwOnStdErr: false - }); + let venvResult = { stderr: '' } as { stderr?: string }; + if (!(await this.fs.exists(pythonInVenv))) { + venvResult = await processService.exec( + baseInterpreter.uri.fsPath, + ['-m', 'venv', venvPath.fsPath], + { throwOnStdErr: false } + ); + }
248-250: Cache interpreter on success.Set the cache so subsequent lookups are O(1).
- this.outputChannel.appendLine('✓ Deepnote toolkit ready'); - return venvInterpreter; + this.outputChannel.appendLine('✓ Deepnote toolkit ready'); + this.venvPythonPaths.set(venvPath.fsPath, venvInterpreter.uri); + return venvInterpreter;
295-299: Windows-safe display name.Split on path separators via uri utils instead of '/'.
- const parts = deepnoteFileUri.fsPath.split('/'); - return parts[parts.length - 1] || 'notebook'; + // Use vscode-path helpers for cross-platform basenames + return (require('../../platform/vscode-path/resources') as typeof import('../../platform/vscode-path/resources')) + .basename(deepnoteFileUri) || 'notebook';Also add at top-level imports:
+// import * as uriPath from '../../platform/vscode-path/resources';src/kernels/execution/cellExecutionMessageHandler.ts (1)
989-999: Stream append targets the wrong output.You check the last output’s items, but then store outputs[0] as the target. This can append to the first output incorrectly.
Apply:
- this.lastUsedStreamOutput = { - output: this.cell.outputs[0], - stream: msg.content.name - }; + this.lastUsedStreamOutput = { + output: this.cell.outputs[this.cell.outputs.length - 1], + stream: msg.content.name + };src/notebooks/deepnote/pocket.ts (1)
66-73: Fallback block type should reflect cell.kind.Defaulting to 'code' can misclassify markup cells lacking type in pocket.
Apply:
- const block: DeepnoteBlock = { + const block: DeepnoteBlock = { blockGroup: pocket?.blockGroup || 'default-group', content: cell.value, - id: cellId || generateBlockId(), + id: cellId || generateBlockId(), metadata, - sortingKey: pocket?.sortingKey || generateSortingKey(index), - type: pocket?.type || 'code' + sortingKey: pocket?.sortingKey || generateSortingKey(index), + type: pocket?.type ?? ((cell as any).kind === NotebookCellKind.Code ? 'code' : 'markdown') };And add import:
-import type { NotebookCellData } from 'vscode'; +import type { NotebookCellData } from 'vscode'; +import { NotebookCellKind } from 'vscode';
♻️ Duplicate comments (5)
build/esbuild/build.ts (1)
320-324: Use watchAll for consistency.Same ask as before: align with other webview builds so watch‑all rebuilds this as well.
- { target: 'web', watch: isWatchMode } + { target: 'web', watch: watchAll }src/webviews/webview-side/dataframe-renderer/index.ts (1)
39-61: Add React cleanup to prevent memory leaks.
ReactDOM.rendermounts components butdisposeOutputItemnever unmounts them, causing memory leaks when outputs are disposed.Store root elements and call
ReactDOM.unmountComponentAtNode(root)indisposeOutputItem:export const activate: ActivationFunction = (context: RendererContext<unknown>) => { + const roots = new Map<string, HTMLElement>(); + return { renderOutputItem(outputItem: OutputItem, element: HTMLElement) { try { const data = outputItem.json(); console.log('Dataframe renderer - received data:', data); const metadata = outputItem.metadata as Metadata | undefined; console.log('[DataframeRenderer] Full metadata:', metadata); console.log('[DataframeRenderer] Full outputItem:', outputItem); const dataFrameMetadata = metadata?.metadata as DataframeMetadata | undefined; const cellId = metadata?.cellId; const cellIndex = metadata?.cellIndex; console.log('[DataframeRenderer] Extracted cellId:', cellId, 'cellIndex:', cellIndex); const root = document.createElement('div'); element.appendChild(root); + roots.set(outputItem.id, root); ReactDOM.render( React.createElement(DataframeRenderer, { context, data, metadata: dataFrameMetadata, cellId, cellIndex }), root ); } catch (error) { console.error('Error rendering dataframe:', error); const errorDiv = document.createElement('div'); errorDiv.style.padding = '10px'; errorDiv.style.color = 'var(--vscode-errorForeground)'; errorDiv.textContent = `Error rendering dataframe: ${error}`; element.appendChild(errorDiv); } }, - disposeOutputItem(_id?: string) { - // Cleanup if needed + disposeOutputItem(id?: string) { + if (id) { + const root = roots.get(id); + if (root) { + ReactDOM.unmountComponentAtNode(root); + roots.delete(id); + } + } } }; };src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)
46-92: Replace console logging with ILogger usage or remove the calls.Per guidelines we must not ship
console.login TSX. Please route these messages through the ILogger plumbing (or drop them if unnecessary). Based on learnings.- console.log('[DataframeRenderer] Rendering with:', { cellId, cellIndex, data, metadata }); ... - console.log('[DataframeRenderer] handlePageSizeChange called with cellId:', cellId, 'cellIndex:', cellIndex); ... - console.log('[DataframeRenderer] Posting message:', message); ... - console.log('[DataframeRenderer] handlePageChange called with cellId:', cellId, 'page:', newPageIndex); ... - console.log('[DataframeRenderer] Posting message:', message);
14-27: Eliminateanyin column/row typings.
anybreaks our lint gate. Please tighten these shapes (e.g.Record<string, unknown>or a proper interface). Based on learnings.- columns: { - dtype: string; - name: string; - stats: any; - }[]; + columns: { + dtype: string; + name: string; + stats: Record<string, unknown>; + }[]; ... - rows: { - _deepnote_index_column: number; - [key: string]: any; - }[]; + rows: Array< + { + _deepnote_index_column: number; + } & Record<string, unknown> + >;src/webviews/extension-side/dataframe/dataframeController.ts (1)
149-175: Restore thecellIndexfallback whencellIdis absent.Without the index path, paging fails for outputs that still only expose
cellIndex. MirrorhandleSelectPageSize.if (message.cellId) { const cells = editor.notebook.getCells(); const foundCell = cells.find((c) => c.metadata.id === message.cellId); if (foundCell) { cell = foundCell; cellIndex = foundCell.index; logger.info(`[DataframeController] Found cell by cellId ${message.cellId} at index ${cellIndex}`); } else { const errorMessage = `Unable to navigate to page: Could not find the cell with ID ${message.cellId}. The cell may have been deleted.`; logger.error(`[DataframeController] ${errorMessage}`); await window.showErrorMessage(errorMessage); throw new Error(errorMessage); } - } else { + } else if (message.cellIndex !== undefined) { + try { + cell = editor.notebook.cellAt(message.cellIndex); + cellIndex = message.cellIndex; + logger.info(`[DataframeController] Using cellIndex ${cellIndex} (cellId not available)`); + } catch (error) { + const errorMessage = `Unable to navigate to page: Cell at index ${message.cellIndex} not found. The notebook structure may have changed.`; + logger.error(`[DataframeController] ${errorMessage}`, error); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + } + } else { const errorMessage = 'Unable to navigate to page: No cell identifier provided. ' + 'Please re-run the cell to update the output metadata.'; logger.error(`[DataframeController] ${errorMessage}`); await window.showErrorMessage(errorMessage); throw new Error(errorMessage); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
build/esbuild/build.ts(5 hunks)package.json(5 hunks)postcss.config.js(1 hunks)src/kernels/deepnote/deepnoteController.ts(1 hunks)src/kernels/deepnote/deepnoteToolkitInstaller.node.ts(2 hunks)src/kernels/deepnote/types.ts(1 hunks)src/kernels/execution/cellExecution.ts(2 hunks)src/kernels/execution/cellExecutionMessageHandler.ts(3 hunks)src/kernels/execution/helpers.ts(9 hunks)src/notebooks/controllers/vscodeNotebookController.ts(3 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(8 hunks)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts(3 hunks)src/notebooks/deepnote/pocket.ts(3 hunks)src/notebooks/deepnote/pocket.unit.test.ts(7 hunks)src/standalone/intellisense/resolveCompletionItem.unit.test.ts(1 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)src/webviews/extension-side/serviceRegistry.node.ts(2 hunks)src/webviews/extension-side/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)src/webviews/webview-side/dataframe-renderer/tailwind.css(1 hunks)tailwind.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
build/esbuild/build.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/kernels/deepnote/types.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecution.tssrc/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/execution/helpers.tssrc/kernels/deepnote/deepnoteController.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
build/esbuild/build.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/kernels/deepnote/types.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecution.tssrc/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/webviews/extension-side/serviceRegistry.node.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/execution/helpers.tssrc/webviews/extension-side/serviceRegistry.web.tssrc/kernels/deepnote/deepnoteController.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs Deepnote data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/webviews/extension-side/serviceRegistry.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/types.tssrc/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/execution/helpers.tssrc/kernels/deepnote/deepnoteController.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecution.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/execution/helpers.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/standalone/intellisense/resolveCompletionItem.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/webviews/extension-side/serviceRegistry.web.ts
🧠 Learnings (14)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-25T09:58:15.799Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:37-38
Timestamp: 2025-09-25T09:58:15.799Z
Learning: In Deepnote environments, the virtual environment must be created at ~/venv path because deepnote-toolkit expects this specific location.
Applied to files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/types.tssrc/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-10-07T19:51:40.201Z
Learnt from: m1so
PR: deepnote/vscode-deepnote#25
File: src/kernels/deepnote/deepnoteServerStarter.node.ts:150-154
Timestamp: 2025-10-07T19:51:40.201Z
Learning: The deepnote-toolkit server is a separate Python package maintained in a different repository from the VS Code extension (vscode-deepnote). The VS Code extension launches the toolkit as a subprocess and communicates via environment variables and HTTP. The toolkit implementation cannot be verified from the extension codebase.
Applied to files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/types.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`
Applied to files:
src/webviews/webview-side/dataframe-renderer/index.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/kernels/deepnote/types.tssrc/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecution.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/deepnote/deepnoteController.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Create a UI provider for kernel selection for the new kernel source
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/interactiveWindowController.ts : InteractiveWindowController must manage kernel selection/startup/lifecycle, system info cell handling, restart logic, and maintain owner file associations for kernel context
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/types.ts : Use the shared core interfaces/types from types.ts across the IPyWidgets controller code
Applied to files:
src/webviews/extension-side/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/kernels/execution/cellExecutionMessageHandler.tssrc/kernels/execution/helpers.ts
🧬 Code graph analysis (13)
build/esbuild/build.ts (1)
build/webpack/common.js (1)
path(6-6)
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteOutput(23-51)src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
src/kernels/deepnote/types.ts (1)
DEEPNOTE_TOOLKIT_VERSION(157-157)
src/webviews/extension-side/dataframe/dataframeController.ts (2)
src/platform/common/types.ts (1)
IDisposable(211-213)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)
src/kernels/execution/cellExecution.ts (1)
src/notebooks/deepnote/pocket.ts (1)
createBlockFromPocket(48-84)
src/standalone/intellisense/resolveCompletionItem.unit.test.ts (1)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(132-170)
src/notebooks/deepnote/pocket.unit.test.ts (1)
src/notebooks/deepnote/pocket.ts (2)
extractPocketFromCellMetadata(44-46)createBlockFromPocket(48-84)
src/notebooks/controllers/vscodeNotebookController.ts (2)
src/kernels/deepnote/deepnoteController.ts (1)
DeepnoteController(78-84)src/kernels/types.ts (1)
IKernelController(930-933)
src/webviews/extension-side/serviceRegistry.node.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/kernels/execution/cellExecutionMessageHandler.ts (2)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(132-170)src/platform/common/utils.ts (2)
concatMultilineString(211-225)formatStreamText(275-278)
src/webviews/extension-side/serviceRegistry.web.ts (1)
src/platform/activation/types.ts (2)
IExtensionSyncActivationService(15-15)IExtensionSyncActivationService(22-24)
src/kernels/deepnote/deepnoteController.ts (1)
src/kernels/kernelController.ts (1)
KernelController(7-15)
src/notebooks/deepnote/pocket.ts (1)
src/notebooks/deepnote/dataConversionUtils.ts (1)
generateBlockId(8-15)
🪛 Biome (2.1.2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
[error] 153-164: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 170-181: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🔇 Additional comments (17)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
31-37: Version-scoped venvs: good; confirm discovery path.Including DEEPNOTE_TOOLKIT_VERSION in the venv path is the right call to auto‑reprovision on upgrades. Please confirm the toolkit doesn’t assume a fixed ~/venv path at runtime; if it does, we need a symlink or alternate discovery. Based on learnings.
141-175: Long-running execs lack timeouts/cancellation wiring.Consider passing a timeout and/or hooking the CancellationToken into exec so we can kill stuck venv/pip processes. If the process service supports it, thread token/timeout through these exec calls.
Also applies to: 167-201
src/kernels/execution/helpers.ts (1)
118-121: Consistent per‑cell provenance propagation.Signatures and metadata wiring look correct; cellIndex/cellId stay in VS Code metadata and aren’t serialized into nbformat. LGTM.
Also applies to: 132-169, 172-186, 218-254, 256-260, 301-321, 571-582
build/esbuild/build.ts (1)
90-97: Tailwind/PostCSS integration: looks good.Scoped processing to dataframe renderer and keeps other CSS paths unchanged. LGTM.
Also applies to: 141-161, 189-192
src/standalone/intellisense/resolveCompletionItem.unit.test.ts (1)
549-549: Avoid passing map’s index/array to cellOutputToVSCCellOutput
Wrapping with an explicit arrow function ensures only the output is passed as (cellOutput); map’s index/array won’t be treated as (cellIndex, cellId). No other direct.map(cellOutputToVSCCellOutput)usages found.src/webviews/webview-side/dataframe-renderer/tailwind.css (1)
1-3: Tailwind entry looks good.Directives are correct and scoped via config.
src/notebooks/deepnote/pocket.ts (1)
6-12: Good: keep id at top-level metadata, not in pocket.This avoids pocket churn and eases runtime access.
tailwind.config.js (1)
1-24: Tailwind config looks solid and webview-safe.Scoped content, VS Code tokens, and preflight disabled are appropriate.
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
31-31: Good: propagate cellIndex/cellId into outputs.Consistent metadata across all output types enables downstream routing.
Also applies to: 57-58
postcss.config.js (1)
1-6: Approve PostCSS plugin configuration Tailwind v4+ (^4.1.14) and '@tailwindcss/postcss' (^4.1.14) are installed.src/kernels/deepnote/types.ts (1)
89-95: Verify tests and consumers of IDeepnoteToolkitInstaller support new getVenvHash()
DeepnoteToolkitInstaller implements getVenvHash; verify tests and any callers of IDeepnoteToolkitInstaller handle the added method.src/webviews/extension-side/serviceRegistry.node.ts (1)
20-20: Registration and activate() for DataframeController verified
Web registry mirrors the node registry and DataframeController implements activate().src/kernels/execution/cellExecution.ts (1)
411-423: LGTM!The Deepnote block conversion and code generation logic is correctly implemented. Cell data is properly assembled, converted to a block via
createBlockFromPocket, and code is generated usingcreatePythonCode.src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
1-521: LGTM!Test updates correctly reflect the refactored metadata structure where
idlives at top-level instead of inside__deepnotePocket. All test cases are consistent with this change.src/notebooks/controllers/vscodeNotebookController.ts (1)
551-551: LGTM!DeepnoteController integration is correct. The controller is properly instantiated for cell execution creation.
Also applies to: 564-564
src/notebooks/deepnote/pocket.unit.test.ts (1)
1-209: LGTM!Test updates correctly reflect the refactored pocket structure where
idis at top-level metadata rather than inside__deepnotePocket. All assertions are consistent.package.json (1)
1812-1822: LGTM!The dataframe renderer contribution is properly configured with the correct entrypoint, mime type, and messaging support.
src/kernels/deepnote/types.ts
Outdated
| export const DEEPNOTE_TOOLKIT_VERSION = '0.2.30.dev29+890433e'; | ||
| export const DEEPNOTE_TOOLKIT_WHEEL_URL = `https://deepnote-staging-runtime-artifactory.s3.amazonaws.com/deepnote-toolkit-packages/${encodeURIComponent( | ||
| DEEPNOTE_TOOLKIT_VERSION | ||
| )}/deepnote_toolkit-${encodeURIComponent(DEEPNOTE_TOOLKIT_VERSION)}-py3-none-any.whl`; | ||
| export const DEEPNOTE_DEFAULT_PORT = 8888; |
There was a problem hiding this comment.
Avoid hardcoding dev version and staging URL in main.
Using a dev build (0.2.30.dev29+890433e) and a staging S3 bucket can break production installs. Gate behind a feature flag or revert to a stable, public wheel before merging.
Consider:
- Read version/channel from settings or env, defaulting to a stable release.
- Parameterize base URL to avoid baking staging endpoints into main.
🤖 Prompt for AI Agents
In src/kernels/deepnote/types.ts around lines 157 to 161, the code hardcodes a
dev toolkit version and a staging S3 wheel URL; replace this by reading the
toolkit version/channel and the base wheel URL from configuration or environment
(with sensible defaults to a stable public release and public URL), and gate use
of non-stable/staging values behind a feature flag or explicit env variable.
Update DEEPNOTE_TOOLKIT_VERSION to be derived from settings/env (fallback to a
stable release), make DEEPNOTE_TOOLKIT_WHEEL_URL compose from a configurable
base URL plus the encoded version, and ensure any staging-only behavior requires
an enabled feature flag to avoid leaking staging endpoints into production.
| private transformOutputsForVsCode( | ||
| outputs: DeepnoteOutput[], | ||
| cellIndex: number, | ||
| cellId: string | ||
| ): NotebookCellOutput[] { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make base64 conversion cross-runtime (Node + Web).
base64ToUint8Array uses atob (web API). In Node, prefer Buffer. Update the helper to support both.
Apply outside this hunk:
// Replace existing helper
private base64ToUint8Array(base64: string): Uint8Array {
// Browser path
if (typeof atob === 'function') {
const binaryString = atob(base64);
const bytes = new Uint8Array(binaryString.length);
for (let i = 0; i < binaryString.length; i++) {
bytes[i] = binaryString.charCodeAt(i);
}
return bytes;
}
// Node path
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const Buf: any = (globalThis as any).Buffer;
if (Buf && typeof Buf.from === 'function') {
return new Uint8Array(Buf.from(base64, 'base64'));
}
// Fallback: safe empty
return new Uint8Array();
}🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteDataConverter.ts around lines 205 to 209, the
helper that converts base64 to Uint8Array uses atob which only works in
browsers; update or replace the helper so it detects runtime and supports both
Web (use atob) and Node (use globalThis.Buffer.from) and returns a Uint8Array,
with a safe empty Uint8Array fallback if neither is available; ensure the new
helper is used by transformOutputsForVsCode and keep types compatible with
TypeScript (avoid direct any if possible).
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (2)
55-61: Multi‑root: avoid always using the first workspaceSelect the correct workspace (active editor’s folder if available), not index 0.
- const workspaceFolders = workspace.workspaceFolders; - if (!workspaceFolders || workspaceFolders.length === 0) { + const workspaceFolders = workspace.workspaceFolders; + if (!workspaceFolders || workspaceFolders.length === 0) { this.logger.info('No workspace folder found, cannot create requirements.txt'); return; } + // Prefer the active editor's workspace folder when available + const activeUri = window.activeTextEditor?.document.uri; + const workspaceFolder = + (activeUri && workspace.getWorkspaceFolder(activeUri)) || workspaceFolders[0];And update later references to use workspaceFolder instead of workspaceFolders[0].
89-93: Scope “Don’t Ask Again” per workspaceCurrent key is global; scope it to the chosen workspace to avoid cross‑workspace side effects.
- const dontAskState = this.persistentStateFactory.createGlobalPersistentState<boolean>( - DONT_ASK_OVERWRITE_REQUIREMENTS_KEY, - false // default: ask user - ); + const scopeKey = `${DONT_ASK_OVERWRITE_REQUIREMENTS_KEY}:${workspaceFolder.uri.toString()}`; + const dontAskState = + this.persistentStateFactory.createGlobalPersistentState<boolean>(scopeKey, false);
♻️ Duplicate comments (6)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (3)
14-18: Implement the interface.Class should declare
implements IDeepnoteInitNotebookRunner.@injectable() -export class DeepnoteInitNotebookRunner { +export class DeepnoteInitNotebookRunner implements IDeepnoteInitNotebookRunner {As per coding guidelines
93-99: Remove brittle cancellation check.Comparing
error.messageto'Cancelled'is fragile. Token checks suffice.} catch (error) { - // Check if this is a cancellation error - if (error instanceof Error && error.message === 'Cancelled') { - logger.info(`Init notebook cancelled for project ${projectId}`); - return; - } // Log error but don't throw - we want to let user continue anyway logger.error(`Error running init notebook for project ${projectId}:`, error);As per coding guidelines
137-147: Localize user-facing strings.Wrap progress titles and messages with
l10n.t().+import { NotebookDocument, ProgressLocation, window, CancellationTokenSource, CancellationToken, l10n } from 'vscode'; @@ - title: `🚀 Initializing project environment`, + title: l10n.t('🚀 Initializing project environment'), @@ - title: `Init: "${initNotebook.name}"`, + title: l10n.t('Init: "{0}"', initNotebook.name),Apply similarly to lines 187, 213-214, 240-242, 270.
As per coding guidelines
src/kernels/execution/cellExecutionMessageHandler.ts (1)
82-88: Helper successfully eliminates duplication.The extraction of
getCellIdaddresses the previous review's concern about repeated fallback logic. Implementation is clean and consistent across all call sites.Based on past review comments.
src/webviews/extension-side/dataframe/dataframeController.ts (1)
141-165: Restore cellIndex fallback (goToPage).Must accept index when id missing or not found; otherwise navigation breaks. This mirrors selectPageSize.
Apply this diff:
- if (!message.cellId) { - const errorMessage = - 'Unable to navigate to page: No cell identifier provided. ' + - 'Please re-run the cell to update the output metadata.'; - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } - - const cells = editor.notebook.getCells(); - const cell = cells.find((c) => c.metadata.id === message.cellId); - - if (!cell) { - const errorMessage = `Unable to navigate to page: Could not find the cell with ID ${message.cellId}. The cell may have been deleted.`; - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } + const cells = editor.notebook.getCells(); + let cell = message.cellId ? cells.find((c) => c.metadata.id === message.cellId) : undefined; + let cellIndex = cell?.index; + + if (!cell && typeof message.cellIndex === 'number') { + const idx = message.cellIndex; + if (idx >= 0 && idx < editor.notebook.cellCount) { + cell = editor.notebook.cellAt(idx); + cellIndex = idx; + logger.debug?.(`[DataframeController] goToPage: using cellIndex fallback ${idx}`); + } + } + + if (!cell || cellIndex === undefined) { + const errorMessage = + `Unable to navigate to page: Could not resolve target cell by id (${message.cellId ?? 'n/a'}) or index (${message.cellIndex ?? 'n/a'}).`; + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + }src/webviews/webview-side/dataframe-renderer/index.ts (1)
19-21: Unmount React roots in disposeOutputItem.Without unmount, outputs leak on dispose/re-render.
Apply this diff:
export const activate: ActivationFunction = (context: RendererContext<unknown>) => { - return { + const roots = new Map<string, HTMLElement>(); + return { renderOutputItem(outputItem: OutputItem, element: HTMLElement) { logMessage(`Dataframe renderer - rendering output item: ${outputItem.id}`); try { const data = outputItem.json(); logMessage(`Dataframe renderer - received data with ${Object.keys(data).length} keys`); @@ - const root = document.createElement('div'); + const root = document.createElement('div'); element.appendChild(root); + roots.set(outputItem.id, root); @@ - ReactDOM.render( + ReactDOM.render( React.createElement(DataframeRenderer, { context, data, metadata: dataFrameMetadata, cellId, cellIndex }), root ); } catch (error) { logErrorMessage(`Error rendering dataframe: ${error}`); const errorDiv = document.createElement('div'); errorDiv.style.padding = '10px'; errorDiv.style.color = 'var(--vscode-errorForeground)'; errorDiv.textContent = `Error rendering dataframe: ${error}`; element.appendChild(errorDiv); } }, - disposeOutputItem(_id?: string) { - // Cleanup if needed + disposeOutputItem(id?: string) { + if (id) { + const root = roots.get(id); + if (root) { + ReactDOM.unmountComponentAtNode(root); + roots.delete(id); + } + } } }; };Also applies to: 36-48, 59-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
build/esbuild/build.ts(5 hunks)src/kernels/execution/cellExecutionMessageHandler.ts(4 hunks)src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts(1 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts(16 hunks)src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts(5 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
build/esbuild/build.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/webviews/webview-side/dataframe-renderer/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
build/esbuild/build.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionMessageHandler.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionMessageHandler.ts
🧠 Learnings (7)
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`
Applied to files:
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/webviews/webview-side/dataframe-renderer/index.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/{interactiveWindow.ts,interactiveWindowController.ts} : Implement robust kernel error handling: automatic restart, clear error display in system info cells, graceful degradation when kernels are unavailable, and user notifications for critical failures
Applied to files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/kernels/execution/cellExecutionMessageHandler.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Use Uri.joinPath() to construct file paths instead of manual string concatenation
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/**/{fileSystem.ts,fileSystem.node.ts,platformService.web.ts,platformService.node.ts} : Use URI-based file operations and vscode-path for cross-platform path handling in file system and platform service implementations
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
🧬 Code graph analysis (4)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/kernels/execution/cellExecutionMessageHandler.ts (1)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(132-170)
src/webviews/webview-side/dataframe-renderer/index.ts (1)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)
DataframeMetadata(7-9)DataframeRenderer(56-215)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (1)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
injectable(49-556)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (13)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (1)
237-237: LGTM! Progress calculation fixed.The off-by-one issue is resolved; final block now correctly reports 100%.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
15-16: Excellent localization and error handling.All user-facing strings now use
l10n.t()and error logging correctly passes error objects as second parameter. Changes align with coding guidelines.As per coding guidelines
Also applies to: 131-134, 246-246, 259-259, 283-283, 323-323, 333-333, 345-345, 359-359, 381-381, 415-423, 440-440, 478-478, 515-515, 525-525, 537-537
src/kernels/execution/cellExecutionMessageHandler.ts (3)
645-647: Consistent cell ID propagation inaddToCellData.Correctly applies the helper and passes both
cellIndexandcellIdto output conversion.
1016-1026: Stream message handling updated consistently.All three code paths in
handleStreamMessagenow use the centralized helper. The append, replace, and create-new-output branches all propagate cell identity correctly.Also applies to: 1032-1042, 1049-1059
1177-1187: Display data updates preserve cell identity.Update-display-data path correctly retrieves and passes the cell ID, maintaining consistency with the main execution paths.
build/esbuild/build.ts (4)
12-14: LGTM!Standard imports for PostCSS/Tailwind CSS processing.
92-96: LGTM!Clean optional parameter for conditional Tailwind processing.
189-191: LGTM!Clear feature toggle based on source path.
320-324: LGTM! Past review issue resolved.The dataframe renderer build entry now correctly uses
watch: watchAll, consistent with other webview renderers.src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (1)
127-131: Fix non-Tailwind class 'flex-grow-1'.Use flex-grow (Tailwind) to ensure styles apply.
[suggest_minor_issue]
Apply this diff:- <div key={column.name} className="flex-none flex-grow-1"> + <div key={column.name} className="flex-none flex-grow">src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (3)
11-14: DI contract + symbol pattern looks goodSymbol + interface export align with inversify DI and keep types clear.
67-69: Good: Uri.joinPath used for path constructionCorrect per guidelines; avoids manual string joins.
As per coding guidelines
96-103: Good: user‑facing strings localizedUsing l10n.t for prompt and buttons is correct.
As per coding guidelines
| await workspace.applyEdit(edit); | ||
|
|
||
| // Re-execute the cell to apply the new page size | ||
| logger.info(`[DataframeRenderer] Re-executing cell ${cellIndex} with new page size`); | ||
|
|
||
| await commands.executeCommand('notebook.cell.execute', { | ||
| ranges: [{ start: cellIndex, end: cellIndex + 1 }], | ||
| document: editor.notebook.uri | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Check applyEdit result and bail if it fails.
applyEdit returns a boolean; handle failures explicitly before re-execution.
Apply this diff:
- await workspace.applyEdit(edit);
+ const ok = await workspace.applyEdit(edit);
+ if (!ok) {
+ const errorMessage = 'Failed to persist table state to cell metadata.';
+ logger.error(`[DataframeController] ${errorMessage}`);
+ await window.showErrorMessage(errorMessage);
+ throw new Error(errorMessage);
+ }Also applies to: 184-193
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around lines
130-139 (and also apply the same change to lines 184-193),
workspace.applyEdit(...) is being awaited but its boolean return value is
ignored; update the code to capture the result, and if it is false log an
error/warning (including context like cellIndex and attempted edit) and return
or otherwise bail out before calling commands.executeCommand to re-execute the
cell. Ensure both occurrences check the boolean, log via the same logger used in
the file, and only proceed to re-execute the notebook cell when applyEdit
returns true.
| private async handleExportDataframe(editor: NotebookEditor, message: ExportDataframeCommand) { | ||
| const cell = editor.notebook.cellAt(message.cellIndex); | ||
|
|
||
| logger.info( | ||
| `[DataframeRenderer] exportDataframe called for cell ${ | ||
| message.cellIndex | ||
| } (${cell?.document.uri.toString()})` | ||
| ); | ||
| // TODO: Implement dataframe export functionality | ||
| } |
There was a problem hiding this comment.
Guard cellIndex before cellAt (prevents throw).
cellAt throws on out-of-bounds. Validate first and show a user-friendly error.
Apply this diff:
- const cell = editor.notebook.cellAt(message.cellIndex);
-
- logger.info(
+ const idx = message.cellIndex;
+ if (idx < 0 || idx >= editor.notebook.cellCount) {
+ const errorMessage = `Unable to export dataframe: Invalid cell index ${idx}.`;
+ logger.error(`[DataframeController] ${errorMessage}`);
+ await window.showErrorMessage(errorMessage);
+ throw new Error(errorMessage);
+ }
+ const cell = editor.notebook.cellAt(idx);
+
+ logger.info(
`[DataframeRenderer] exportDataframe called for cell ${
- message.cellIndex
+ idx
} (${cell?.document.uri.toString()})`
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async handleExportDataframe(editor: NotebookEditor, message: ExportDataframeCommand) { | |
| const cell = editor.notebook.cellAt(message.cellIndex); | |
| logger.info( | |
| `[DataframeRenderer] exportDataframe called for cell ${ | |
| message.cellIndex | |
| } (${cell?.document.uri.toString()})` | |
| ); | |
| // TODO: Implement dataframe export functionality | |
| } | |
| private async handleExportDataframe(editor: NotebookEditor, message: ExportDataframeCommand) { | |
| const idx = message.cellIndex; | |
| if (idx < 0 || idx >= editor.notebook.cellCount) { | |
| const errorMessage = `Unable to export dataframe: Invalid cell index ${idx}.`; | |
| logger.error(`[DataframeController] ${errorMessage}`); | |
| await window.showErrorMessage(errorMessage); | |
| throw new Error(errorMessage); | |
| } | |
| const cell = editor.notebook.cellAt(idx); | |
| logger.info( | |
| `[DataframeRenderer] exportDataframe called for cell ${ | |
| idx | |
| } (${cell?.document.uri.toString()})` | |
| ); | |
| // TODO: Implement dataframe export functionality | |
| } |
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around lines 201
to 210, the code calls editor.notebook.cellAt(message.cellIndex) without
validating message.cellIndex; cellAt throws for out-of-bounds indices. Fix by
first checking that message.cellIndex is a non-negative integer less than
editor.notebook.cellCount (and that editor and editor.notebook exist), and if
invalid call a user-friendly error path (e.g., logger.warn and
vscode.window.showErrorMessage or similar) and return early; otherwise proceed
to call cellAt and continue with the export logic.
| logMessage( | ||
| `[DataframeRenderer] Rendering with cellId: ${cellId}, cellIndex: ${cellIndex}, data: ${JSON.stringify( | ||
| data | ||
| )}, metadata: ${JSON.stringify(metadata)}` | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce log volume (optional).
Logging full data/metadata is noisy. Prefer concise summaries in prod.
Replace JSON.stringify payloads with brief counts/ids, gated behind a debug flag if available.
Also applies to: 73-75
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx around
lines 63-67 (and similarly lines 73-75), the current log prints full data and
metadata via JSON.stringify which floods logs; change the log to emit concise
summaries instead (e.g., data length/row count, key IDs, or first N items) and
only include full JSON when a debug flag is enabled; implement gating by
checking the existing debug/config flag (or process.env.NODE_ENV !==
'production' / a local DEBUG flag) so production logs remain small while full
payloads are available when debugging.
| logMessage(`Dataframe renderer - rendering output item: ${outputItem.id}`); | ||
| try { | ||
| const data = outputItem.json(); | ||
| logMessage(`Dataframe renderer - received data with ${Object.keys(data).length} keys`); | ||
|
|
||
| const metadata = outputItem.metadata as Metadata | undefined; | ||
| logMessage(`[DataframeRenderer] Full metadata: ${JSON.stringify(metadata)}`); | ||
|
|
||
| const dataFrameMetadata = metadata?.metadata as DataframeMetadata | undefined; | ||
| const cellId = metadata?.cellId; | ||
| const cellIndex = metadata?.cellIndex; | ||
|
|
||
| logMessage(`[DataframeRenderer] Extracted cellId: ${cellId}, cellIndex: ${cellIndex}`); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trim verbose logs (avoid dumping data/metadata).
Large JSON logs hurt perf and risk PII. Log summaries only.
Apply this diff:
- logMessage(`Dataframe renderer - rendering output item: ${outputItem.id}`);
+ logMessage(`Dataframe renderer: render ${outputItem.id}`);
@@
- logMessage(`Dataframe renderer - received data with ${Object.keys(data).length} keys`);
+ logMessage(`Dataframe renderer: data keys=${Object.keys(data).length}`);
@@
- logMessage(`[DataframeRenderer] Full metadata: ${JSON.stringify(metadata)}`);
+ logMessage(`[DataframeRenderer] metadata present=${!!metadata}`);
@@
- logMessage(`[DataframeRenderer] Extracted cellId: ${cellId}, cellIndex: ${cellIndex}`);
+ logMessage(`[DataframeRenderer] cellId=${cellId ?? 'n/a'} cellIndex=${cellIndex ?? 'n/a'}`);
@@
- logErrorMessage(`Error rendering dataframe: ${error}`);
+ logErrorMessage(`Error rendering dataframe: ${String(error)}`);Also applies to: 49-56
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/index.ts around lines 22-35 (and
similarly lines 49-56), current logs dump full data/metadata JSON which is
verbose and may contain PII; replace those statements with concise summaries:
log counts, types, presence flags and identifiers only (e.g., number of keys,
whether metadata exists, data shape info, cellId/cellIndex) and remove
JSON.stringify outputs; ensure logging calls avoid printing full object contents
and instead use small deterministic strings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build/esbuild/build.ts (1)
166-180: Structural error: premature callback closure.Line 167 closes the
onLoadhandler prematurely. Lines 169-180 referenceargsbut are outside the callback scope, causing the pipeline syntax error at line 180.Remove line 167. The default CSS processing (lines 169-180) must be inside the
onLoadcallback:- } - // …rest of the onLoad handler… - }); - // Default behavior for other CSS files const options = { entryPoints: [args.path], ...opt }; options.loader = options.loader || {}; // Add the same loaders we add for other places Object.keys(loader).forEach((key) => { if (options.loader && !options.loader[key]) { options.loader[key] = loader[key]; } }); const { errors, warnings, outputFiles } = await esbuild.build(options); return { errors, warnings, contents: outputFiles![0].text, loader: 'text' }; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
build/esbuild/build.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
build/esbuild/build.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
build/esbuild/build.ts
🧬 Code graph analysis (1)
build/esbuild/build.ts (1)
build/webpack/common.js (1)
path(6-6)
🪛 GitHub Actions: CI
build/esbuild/build.ts
[error] 180-180: SyntaxError: ',' expected. (180:14) during Prettier check in 'npm run format'.
🔇 Additional comments (4)
build/esbuild/build.ts (4)
92-96: LGTM!The
enableTailwindparameter enables conditional Tailwind processing.
196-198: LGTM!Tailwind processing is correctly enabled only for the dataframe renderer.
327-331: LGTM!The dataframe renderer build correctly uses
watchAllfor consistency with other webview renderers.
12-14: Dependencies are up-to-date. Installed versions ofpostcss(8.5.6),@tailwindcss/postcss(4.1.14),tailwindcss(4.1.14), andautoprefixer(10.4.21) match the latest stable releases.
|
|
||
| onLoad({ filter: /.*/, namespace: 'style-content' }, async (args) => { | ||
| // Process with PostCSS/Tailwind if enabled and file exists | ||
| if (enableTailwind && args.path.includes('tailwind.css') && fs.existsSync(args.path)) { |
There was a problem hiding this comment.
Hardcoded filename check is fragile.
The check args.path.includes('tailwind.css') assumes a specific filename. Renaming or adding multiple Tailwind files will break processing.
Consider a more flexible approach (e.g., check for @tailwind directives in content or use a pattern/config).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (1)
160-162: Remove absolute path from log (PII).Line 161 logs
requirementsPath(absolute filesystem path). Use workspace name instead.Apply this diff:
- this.logger.info( - `Created requirements.txt with ${normalizedRequirements.length} dependencies at ${requirementsPath}` - ); + this.logger.info( + `Created requirements.txt with ${normalizedRequirements.length} dependencies in workspace "${workspaceFolders[0].name}"` + );As per coding guidelines
build/esbuild/build.ts (1)
143-143: Hardcoded filename check remains fragile.The check
args.path.includes('tailwind.css')was flagged in previous reviews but not addressed. Renaming or adding multiple Tailwind files will break processing.Consider checking for
@tailwinddirectives in content or use a configurable pattern.src/webviews/extension-side/dataframe/dataframeController.ts (5)
63-86: Stop logging payloads.Still logging the entire message, leaking table contents. Log only whitelisted fields and skip noisy copy commands. As per coding guidelines.
- logger.info('DataframeController received message', message); + const { command } = message; + if (command !== 'copyTableData') { + logger.info('DataframeController received message', { + command, + cellId: (message as { cellId?: string }).cellId, + cellIndex: (message as { cellIndex?: number }).cellIndex + }); + }
101-138: Restore cellIndex fallback for selectPageSize.Renderer still sends index-only messages; ignoring them breaks paging. Reintroduce index validation and fallback, and surface both identifiers in the error.
- const cells = editor.notebook.getCells(); - const cell = cells.find((c) => c.metadata.id === message.cellId); - - if (!cell) { - const errorMessage = l10n.t( - 'Unable to update page size: Could not find the cell with ID {0}. The cell may have been deleted.', - message.cellId ?? '' - ); + const cells = editor.notebook.getCells(); + let cell = message.cellId ? cells.find((c) => c.metadata.id === message.cellId) : undefined; + let cellIndex = cell?.index; + + if (!cell && typeof message.cellIndex === 'number' && Number.isInteger(message.cellIndex)) { + const idx = message.cellIndex; + if (idx >= 0 && idx < editor.notebook.cellCount) { + cell = editor.notebook.cellAt(idx); + cellIndex = idx; + logger.info(`[DataframeController] selectPageSize: resolved via cellIndex ${idx}`); + } + } + + if (!cell || cellIndex === undefined) { + const errorMessage = l10n.t( + 'Unable to update page size: Could not resolve target cell by id ({0}) or index ({1}).', + message.cellId ?? 'n/a', + message.cellIndex ?? 'n/a' + ); logger.error(`[DataframeController] ${errorMessage}`); await window.showErrorMessage(errorMessage); throw new Error(errorMessage); } - - const cellIndex = cell.index;
134-199: Handle applyEdit failures before re-running cells.workspace.applyEdit can fail; re-executing after a rejected edit leaves the UI wrong. Capture the boolean and bail. Apply the same guard in the later block.
- await workspace.applyEdit(edit); + const editApplied = await workspace.applyEdit(edit); + if (!editApplied) { + const errorMessage = l10n.t('Failed to persist table state to cell metadata.'); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + }
145-200: Go-to-page still lacks cellIndex fallback.Renderer keeps sending index-only messages; forcing cellId breaks navigation. Accept a valid index when id lookup fails and align the error text.
- if (!message.cellId) { - const errorMessage = l10n.t( - 'Unable to navigate to page: No cell identifier provided. Please re-run the cell to update the output metadata.' - ); + if (message.cellId === undefined && message.cellIndex === undefined) { + const errorMessage = l10n.t( + 'Unable to navigate to page: No cell identifier provided. Please re-run the cell to update the output metadata.' + ); logger.error(`[DataframeController] ${errorMessage}`); await window.showErrorMessage(errorMessage); throw new Error(errorMessage); } const cells = editor.notebook.getCells(); - const cell = cells.find((c) => c.metadata.id === message.cellId); - - if (!cell) { - const errorMessage = l10n.t( - 'Unable to navigate to page: Could not find the cell with ID {0}. The cell may have been deleted.', - message.cellId ?? '' - ); + let cell = message.cellId ? cells.find((c) => c.metadata.id === message.cellId) : undefined; + let cellIndex = cell?.index; + + if (!cell && typeof message.cellIndex === 'number' && Number.isInteger(message.cellIndex)) { + const idx = message.cellIndex; + if (idx >= 0 && idx < editor.notebook.cellCount) { + cell = editor.notebook.cellAt(idx); + cellIndex = idx; + logger.info(`[DataframeController] goToPage: resolved via cellIndex ${idx}`); + } + } + + if (!cell || cellIndex === undefined) { + const errorMessage = l10n.t( + 'Unable to navigate to page: Could not resolve target cell by id ({0}) or index ({1}).', + message.cellId ?? 'n/a', + message.cellIndex ?? 'n/a' + ); logger.error(`[DataframeController] ${errorMessage}`); await window.showErrorMessage(errorMessage); throw new Error(errorMessage); } // Update page index in table state within cell metadata const existingTableState = cell.metadata.deepnote_table_state || {}; const updatedTableState = { ...existingTableState, pageIndex: message.page }; - const cellIndex = cell.index; + const cellIndex = cellIndex!;
208-215: Validate export cellIndex before cellAt.cellAt throws on out-of-range indices. Guard and raise a localized error instead of crashing.
- const cell = editor.notebook.cellAt(message.cellIndex); + const { cellIndex } = message; + if (cellIndex < 0 || cellIndex >= editor.notebook.cellCount) { + const errorMessage = l10n.t('Unable to export dataframe: Invalid cell index {0}.', cellIndex); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + } + + const cell = editor.notebook.cellAt(cellIndex); logger.info( `[DataframeRenderer] exportDataframe called for cell ${ - message.cellIndex + cellIndex } (${cell?.document.uri.toString()})` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
build/esbuild/build.ts(6 hunks)src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts(6 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
build/esbuild/build.tssrc/webviews/extension-side/dataframe/dataframeController.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
build/esbuild/build.tssrc/notebooks/deepnote/deepnoteRequirementsHelper.node.tssrc/webviews/extension-side/dataframe/dataframeController.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
🧠 Learnings (5)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Use Uri.joinPath() to construct file paths instead of manual string concatenation
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/**/{fileSystem.ts,fileSystem.node.ts,platformService.web.ts,platformService.node.ts} : Use URI-based file operations and vscode-path for cross-platform path handling in file system and platform service implementations
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.tssrc/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-09-24T21:29:29.065Z
Learnt from: jankuca
PR: deepnote/deepnote#18525
File: apps/webapp-client/src/features/project-table-of-contents/markdown-toc-utils.test.ts:131-169
Timestamp: 2025-09-24T21:29:29.065Z
Learning: In the Deepnote codebase, CRLF line endings are automatically converted to LF on save, so testing CRLF scenarios in markdown parsing functions is unnecessary.
Applied to files:
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/webviews/extension-side/dataframe/dataframeController.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (1)
src/test/mocks/vsc/index.ts (1)
CancellationToken(100-112)
🔇 Additional comments (7)
src/notebooks/deepnote/deepnoteRequirementsHelper.node.ts (4)
19-24: LGTM!Interface implementation and DI setup follow best practices.
44-57: LGTM!Normalization logic correctly filters non-strings, trims, removes empties, and deduplicates using
Set. Type guard ensuresstring[]downstream.
77-92: LGTM!EOL normalization avoids CRLF/LF mismatches that would trigger needless file rewrites.
34-36: LGTM!Cancellation checks at key points (start, pre-I/O, post-prompt, post-write) ensure responsive cancellation handling.
Also applies to: 67-69, 120-122, 155-158
build/esbuild/build.ts (3)
12-14: LGTM!PostCSS toolchain imports are correct and properly used.
92-96: LGTM!Backward-compatible addition of
enableTailwindparameter.
327-331: LGTM!Build entry correctly uses
watchAllfor watch flag consistency with other webview renderers.
| // Enable Tailwind processing for dataframe renderer | ||
| const enableTailwind = source.includes(path.join('dataframe-renderer', 'index.ts')); | ||
| plugins.push(style({ enableTailwind })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more robust path matching.
The check source.includes(path.join('dataframe-renderer', 'index.ts')) is fragile if the file is moved or renamed. Consider a more explicit path comparison or document the coupling.
Example:
- const enableTailwind = source.includes(path.join('dataframe-renderer', 'index.ts'));
+ const enableTailwind = source === path.join(extensionFolder, 'src', 'webviews', 'webview-side', 'dataframe-renderer', 'index.ts');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Enable Tailwind processing for dataframe renderer | |
| const enableTailwind = source.includes(path.join('dataframe-renderer', 'index.ts')); | |
| plugins.push(style({ enableTailwind })); | |
| // Enable Tailwind processing for dataframe renderer | |
| const enableTailwind = source === path.join( | |
| extensionFolder, | |
| 'src', | |
| 'webviews', | |
| 'webview-side', | |
| 'dataframe-renderer', | |
| 'index.ts' | |
| ); | |
| plugins.push(style({ enableTailwind })); |
🤖 Prompt for AI Agents
In build/esbuild/build.ts around lines 196 to 198, the current
source.includes(path.join('dataframe-renderer', 'index.ts')) check is brittle;
replace it with a more robust path comparison such as normalizing the path and
testing for a suffix (e.g.,
path.normalize(source).endsWith(path.join('dataframe-renderer','index.ts'))) or
resolving and comparing relative paths from the project root, or match by
basename + parent directory (e.g., path.basename(source) === 'index.ts' &&
path.dirname(path.normalize(source)).endsWith('dataframe-renderer'));
alternatively, if coupling is intentional, add a comment documenting that this
exact path is required and why.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/kernels/execution/helpers.ts (1)
178-223: Avoid metadata key collisions when merging block metadataBlock metadata is merged over cellId/cellIndex and can overwrite them. If that’s not intended, namespace block metadata (e.g., metadata.block = cellMetadata) or set cellId/cellIndex last.
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
205-226: Fix object spread of possibly undefined blockMetadataUsing
...blockMetadatathrows when undefined. Use nullish coalescing.Apply this diff:
- const metadata: Record<string, unknown> = { - cellId, - ...blockMetadata, - cellIndex - }; + const metadata: Record<string, unknown> = { + cellId, + ...(blockMetadata ?? {}), + cellIndex + }; @@ - const metadata: Record<string, unknown> = { - cellId, - ...blockMetadata, - cellIndex - }; + const metadata: Record<string, unknown> = { + cellId, + ...(blockMetadata ?? {}), + cellIndex + }; @@ - const metadata: Record<string, unknown> = { - cellId, - ...blockMetadata, - cellIndex - }; + const metadata: Record<string, unknown> = { + cellId, + ...(blockMetadata ?? {}), + cellIndex + }; @@ - const metadata: Record<string, unknown> = { - cellId, - ...blockMetadata, - cellIndex - }; + const metadata: Record<string, unknown> = { + cellId, + ...(blockMetadata ?? {}), + cellIndex + }; @@ - const metadata: Record<string, unknown> = { - cellId, - ...blockMetadata, - cellIndex - }; + const metadata: Record<string, unknown> = { + cellId, + ...(blockMetadata ?? {}), + cellIndex + };Also applies to: 289-304, 316-326, 329-340, 347-358
♻️ Duplicate comments (5)
build/esbuild/build.ts (3)
141-165: Fragile filename check and logging violation.Two issues:
Line 143: Hardcoded
'tailwind.css'check is fragile. Renaming the file or adding multiple Tailwind files will break processing.Line 162: Uses
console.errorinstead of theILoggerservice, violating coding guidelines.For issue 1, consider checking for
@tailwinddirectives in content or using a configurable pattern. For issue 2, inject and useILogger:- console.error(`PostCSS processing failed for ${args.path}:`, error); + // Use ILogger service instead of console.error
196-198: Fragile path check.Line 197 uses
source.includes(path.join('dataframe-renderer', 'index.ts')), which is brittle if the file is moved or renamed.Consider a more robust approach:
- const enableTailwind = source.includes(path.join('dataframe-renderer', 'index.ts')); + const enableTailwind = path.normalize(source).endsWith(path.join('dataframe-renderer', 'index.ts'));Or explicitly match the full path:
- const enableTailwind = source.includes(path.join('dataframe-renderer', 'index.ts')); + const enableTailwind = source === path.join(extensionFolder, 'src', 'webviews', 'webview-side', 'dataframe-renderer', 'index.ts');
327-331: Inconsistent watch flag.Line 330 uses
watch: isWatchModewhile other webview renderers (lines 313, 325, 335, 340, 345) usewatch: watchAll. This renderer won't rebuild when runningnpm run watch-all.Apply this diff:
build( path.join(extensionFolder, 'src', 'webviews', 'webview-side', 'dataframe-renderer', 'index.ts'), path.join(extensionFolder, 'dist', 'webviews', 'webview-side', 'dataframeRenderer', 'dataframeRenderer.js'), - { target: 'web', watch: isWatchMode } + { target: 'web', watch: watchAll } ),src/notebooks/deepnote/deepnoteDataConverter.ts (1)
262-270: atob/btoa will crash under Node; make base64 conversion cross‑runtime
base64ToUint8Arrayusesatob(browser‑only). This file runs in the extension host (Node). Replace helper to support both Web and Node; also update reverse (btoa) usage.Apply this diff for the helper:
- private base64ToUint8Array(base64: string): Uint8Array { - const binaryString = atob(base64); - const bytes = new Uint8Array(binaryString.length); - for (let i = 0; i < binaryString.length; i++) { - bytes[i] = binaryString.charCodeAt(i); - } - return bytes; - } + private base64ToUint8Array(base64: string): Uint8Array { + // Browser path + if (typeof (globalThis as any).atob === 'function') { + const binaryString = (globalThis as any).atob(base64); + const bytes = new Uint8Array(binaryString.length); + for (let i = 0; i < binaryString.length; i++) { + bytes[i] = binaryString.charCodeAt(i); + } + return bytes; + } + // Node path + const Buf: any = (globalThis as any).Buffer; + if (Buf && typeof Buf.from === 'function') { + return new Uint8Array(Buf.from(base64, 'base64')); + } + return new Uint8Array(); + }And replace reverse encoding in transformOutputsForDeepnote (outside this hunk) to avoid
btoa:- } else if (item.mime === 'image/png') { - data['image/png'] = btoa(String.fromCharCode(...new Uint8Array(item.data))); - } else if (item.mime === 'image/jpeg') { - data['image/jpeg'] = btoa(String.fromCharCode(...new Uint8Array(item.data))); + } else if (item.mime === 'image/png') { + // Node: Buffer to base64, Browser: btoa fallback + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const Buf: any = (globalThis as any).Buffer; + data['image/png'] = Buf + ? Buf.from(item.data).toString('base64') + : btoa(String.fromCharCode(...new Uint8Array(item.data))); + } else if (item.mime === 'image/jpeg') { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const Buf: any = (globalThis as any).Buffer; + data['image/jpeg'] = Buf + ? Buf.from(item.data).toString('base64') + : btoa(String.fromCharCode(...new Uint8Array(item.data)));Also applies to: 273-279
src/webviews/webview-side/dataframe-renderer/index.ts (1)
21-21: Console usage remains - see past review comments.Console.log/error calls are still present. Past comments flagged this and indicated it was addressed in commit a51a150, but the current code still contains these calls.
Per coding guidelines: use ILogger service for all logging in .ts/.tsx files.
Also applies to: 24-24, 27-27, 33-33, 49-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
build/esbuild/build.ts(6 hunks)src/kernels/execution/cellExecutionMessageHandler.ts(4 hunks)src/kernels/execution/helpers.ts(10 hunks)src/notebooks/deepnote/blocks.md(1 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(9 hunks)src/notebooks/deepnote/pocket.ts(3 hunks)src/notebooks/deepnote/serialization.md(0 hunks)src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx(1 hunks)src/webviews/webview-side/dataframe-renderer/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/notebooks/deepnote/serialization.md
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/pocket.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/kernels/execution/helpers.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/notebooks/deepnote/deepnoteDataConverter.tsbuild/esbuild/build.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/pocket.tssrc/webviews/webview-side/dataframe-renderer/index.tssrc/kernels/execution/helpers.tssrc/kernels/execution/cellExecutionMessageHandler.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsxsrc/notebooks/deepnote/deepnoteDataConverter.tsbuild/esbuild/build.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/blocks.md
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/helpers.tssrc/kernels/execution/cellExecutionMessageHandler.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/helpers.tssrc/kernels/execution/cellExecutionMessageHandler.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs Deepnote data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧠 Learnings (5)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Applied to files:
src/notebooks/deepnote/pocket.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use the `ILogger` service instead of `console.log`
Applied to files:
src/webviews/webview-side/dataframe-renderer/index.tssrc/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/kernels/execution/cellExecutionMessageHandler.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (5)
src/notebooks/deepnote/pocket.ts (1)
src/notebooks/deepnote/dataConversionUtils.ts (1)
generateBlockId(8-15)
src/webviews/webview-side/dataframe-renderer/index.ts (2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)
DataframeMetadata(6-8)DataframeRenderer(54-210)src/webviews/extension-side/dataframe/dataframeController.ts (1)
activate(54-57)
src/kernels/execution/helpers.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
src/kernels/execution/cellExecutionMessageHandler.ts (1)
src/kernels/execution/helpers.ts (1)
cellOutputToVSCCellOutput(137-176)
src/notebooks/deepnote/deepnoteDataConverter.ts (4)
src/notebooks/deepnote/pocket.ts (1)
addPocketToCellMetadata(20-42)src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteOutput(9-9)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteOutput(23-51)src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
🪛 markdownlint-cli2 (0.18.1)
src/notebooks/deepnote/blocks.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (10)
build/esbuild/build.ts (2)
12-14: LGTM!Imports are appropriate for PostCSS/Tailwind integration.
92-96: LGTM!Function signature properly extended with optional
enableTailwindparameter.src/notebooks/deepnote/pocket.ts (1)
52-60: Top‑level id flow is soundReading id from top‑level metadata, removing it from metadata, and assigning block.id is correct and keeps VS Code outputs separate from pocket. LGTM.
Also applies to: 66-73
src/kernels/execution/helpers.ts (3)
118-126: API expansion for per‑cell provenance: goodMapper/caller signatures now carry cellIndex, cellId, cellMetadata and are wired correctly.
Also applies to: 136-175
238-274: Consistent provenance on all output typesdisplay_data/execute_result, stream, and error now include cellId/cellIndex in metadata. Looks good.
Please confirm downstream consumers read these fields from output.metadata (not output.items[].metadata).
Also applies to: 276-285, 596-613
329-336: CellOutputMetadata additions look correctAdding cellIndex and cellId to the metadata interface aligns with usage.
src/kernels/execution/cellExecutionMessageHandler.ts (2)
82-88: Centralized cellId helperGood extraction. Prefers Deepnote metadata id with URI fallback.
645-649: Per‑cell context propagation: goodPassing cell.index, cellId, and cell.metadata into output translation across paths is consistent. LGTM.
Also applies to: 1017-1029, 1036-1047, 1055-1066, 1185-1196
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
31-31: Block → cell metadata and outputs: good
- Passing block.id to metadata.id and to output transformation aligns with new pocket semantics.
- Converting VS Code outputs back to Deepnote only when present is fine.
Please confirm tests cover cells without block.metadata (to exercise the
(blockMetadata ?? {})path).Also applies to: 56-57, 80-84
src/notebooks/deepnote/blocks.md (1)
1-628: Excellent comprehensive documentation.This document thoroughly covers the Deepnote blocks architecture - three representations, pocket system, converters, metadata flow, and includes detailed examples. Well-structured and accurate based on the codebase context.
| ``` | ||
| ┌─────────────────┐ | ||
| │ .deepnote File │ | ||
| │ (YAML format) │ | ||
| └────────┬────────┘ | ||
| │ Deserialize (serializer) | ||
| ▼ | ||
| ┌─────────────────┐ | ||
| │ VS Code Cell │ | ||
| │ (with pocket) │ | ||
| └────────┬────────┘ | ||
| │ Execute (cellExecution) | ||
| ▼ | ||
| ┌─────────────────┐ | ||
| │ Python Code │ | ||
| │ (via blocks) │ | ||
| └─────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The ASCII diagram should specify a language (or use text if no syntax highlighting needed).
Apply this diff:
-```
+```text
┌─────────────────┐
│ .deepnote File │As per static analysis hint.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In src/notebooks/deepnote/blocks.md around lines 15 to 32, the fenced code block
containing the ASCII diagram lacks a language specifier; update the opening
fence to include a language (use "text") so the block starts with ```text and
add the provided first two lines from the diff inside the block (i.e., the
leading " ┌─────────────────┐" and " │ .deepnote File │") to match the
requested change.
| console.log('[DataframeRenderer] Rendering dataframe', { | ||
| cellId, | ||
| data, | ||
| metadata | ||
| }); | ||
|
|
There was a problem hiding this comment.
Remove console logs or use logger
Replace console.log with the extension’s logging facility or gate behind a debug flag; avoid noisy logs in production.
As per coding guidelines.
Also applies to: 70-73, 85-96, 101-112
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx around
lines 60-65 (and also apply same changes to lines 70-73, 85-96, 101-112): remove
all console.log/console.debug calls and replace them with the extension's
logging utility (or wrap them behind a debug flag), e.g. call the centralized
logger with appropriate levels and contextual fields or gate the logs by a
boolean isDebug before logging; ensure imports and typing for the logger are
added if missing and preserve the original log message content and structured
data when calling the logger.
| const tableState = useMemo((): TableState => JSON.parse(metadata?.table_state_spec || '{}'), [metadata]); | ||
| const [pageSize, setPageSize] = useState(tableState.pageSize || 10); | ||
| const [pageIndex, setPageIndex] = useState(tableState.pageIndex || 0); | ||
|
|
There was a problem hiding this comment.
Harden table_state_spec parsing
JSON.parse can throw. Add try/catch and safe defaults.
Apply this diff:
- const tableState = useMemo((): TableState => JSON.parse(metadata?.table_state_spec || '{}'), [metadata]);
- const [pageSize, setPageSize] = useState(tableState.pageSize || 10);
- const [pageIndex, setPageIndex] = useState(tableState.pageIndex || 0);
+ const tableState = useMemo((): TableState => {
+ const spec = metadata?.table_state_spec;
+ if (!spec) {
+ return {} as unknown as TableState;
+ }
+ try {
+ return JSON.parse(spec) as TableState;
+ } catch {
+ return {} as unknown as TableState;
+ }
+ }, [metadata]);
+ const [pageSize, setPageSize] = useState<number>(Number(tableState.pageSize) > 0 ? Number(tableState.pageSize) : 10);
+ const [pageIndex, setPageIndex] = useState<number>(Number.isInteger(tableState.pageIndex) && tableState.pageIndex >= 0 ? tableState.pageIndex : 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tableState = useMemo((): TableState => JSON.parse(metadata?.table_state_spec || '{}'), [metadata]); | |
| const [pageSize, setPageSize] = useState(tableState.pageSize || 10); | |
| const [pageIndex, setPageIndex] = useState(tableState.pageIndex || 0); | |
| const tableState = useMemo((): TableState => { | |
| const spec = metadata?.table_state_spec; | |
| if (!spec) { | |
| return {} as unknown as TableState; | |
| } | |
| try { | |
| return JSON.parse(spec) as TableState; | |
| } catch { | |
| return {} as unknown as TableState; | |
| } | |
| }, [metadata]); | |
| const [pageSize, setPageSize] = useState<number>( | |
| Number(tableState.pageSize) > 0 ? Number(tableState.pageSize) : 10 | |
| ); | |
| const [pageIndex, setPageIndex] = useState<number>( | |
| Number.isInteger(tableState.pageIndex) && tableState.pageIndex >= 0 | |
| ? tableState.pageIndex | |
| : 0 | |
| ); |
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx around
lines 66 to 69, the JSON.parse call on metadata?.table_state_spec can throw and
currently isn't protected; wrap the parse in a try/catch inside the useMemo so
parsing errors return a safe default object (e.g., {}), and then derive pageSize
and pageIndex from that safe tableState; ensure useMemo depends on metadata and
that pageSize/pageIndex state initializers use the validated values (with
fallbacks like 10 and 0).
| const handlePageChange = (newPageIndex: number) => { | ||
| setPageIndex(newPageIndex); | ||
|
|
||
| console.log(`[DataframeRenderer] handlePageChange called with cellId: ${cellId}, page: ${newPageIndex}`); | ||
|
|
||
| const message = { | ||
| command: 'goToPage', | ||
| cellId, | ||
| page: newPageIndex | ||
| }; | ||
|
|
||
| console.log(`[DataframeRenderer] Posting message: ${JSON.stringify(message)}`); | ||
|
|
||
| context.postMessage?.(message); | ||
| }; |
There was a problem hiding this comment.
Clamp pagination inputs
Ensure page changes stay within [0, totalPages‑1] even if called programmatically.
Apply this diff:
- const handlePageChange = (newPageIndex: number) => {
- setPageIndex(newPageIndex);
+ const handlePageChange = (newPageIndex: number) => {
+ const clamped = Math.max(0, Math.min(newPageIndex, totalPages - 1));
+ setPageIndex(clamped);
@@
- page: newPageIndex
+ page: clampedAlso applies to: 170-204
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx around
lines 98-112 (and similarly for the handler at 170-204), the handlePageChange
currently accepts any newPageIndex; clamp the value to the valid range [0,
totalPages - 1] before calling setPageIndex and posting the message. Implement a
clampedPage = Math.max(0, Math.min(newPageIndex, Math.max(0, totalPages - 1)))
(or equivalent), use clampedPage for setPageIndex and in the posted message, and
handle the edge case where totalPages is 0 by setting page to 0; ensure both
locations use the same clamping logic.
| {value === null || value === undefined ? 'None' : String(value)} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Localize user‑facing strings
Wrap “None”, “rows, columns”, “Previous page”, “Next page”, “Page X of Y”, and “/ page” in l10n.t().
As per coding guidelines.
Also applies to: 146-151, 170-204
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx around
lines 137-139 (and similarly for 146-151 and 170-204), user-facing literal
strings ("None", "rows, columns", "Previous page", "Next page", "Page X of Y",
"/ page") must be wrapped with l10n.t(); replace the raw literals with calls to
l10n.t('...') (using appropriate keys or the literal string as the message) and
ensure any interpolated values use l10n.t with placeholders where needed (e.g.,
l10n.t('Page {current} of {total}', { current, total })). Make sure to import
l10n if not already present and run a quick search in the file for other
user-facing literals to wrap similarly.
| ReactDOM.render( | ||
| React.createElement(DataframeRenderer, { | ||
| context, | ||
| data, | ||
| metadata: dataFrameMetadata, | ||
| cellId, | ||
| cellIndex | ||
| }), | ||
| root |
There was a problem hiding this comment.
ReactDOM.unmount still missing - memory leak.
ReactDOM.render mounts components without cleanup in disposeOutputItem, causing memory leaks when outputs are disposed. Past review comment flagged this but it remains unaddressed.
Store root elements and call ReactDOM.unmountComponentAtNode(root) in disposeOutputItem:
export const activate: ActivationFunction = (context: RendererContext<unknown>) => {
+ const roots = new Map<string, HTMLElement>();
+
return {
renderOutputItem(outputItem: OutputItem, element: HTMLElement) {
console.log(`Dataframe renderer - rendering output item: ${outputItem.id}`);
try {
const data = outputItem.json();
console.log(`Dataframe renderer - received data with ${Object.keys(data).length} keys`);
const metadata = outputItem.metadata as Metadata | undefined;
console.log('[DataframeRenderer] Full metadata', metadata);
const dataFrameMetadata = metadata?.metadata as DataframeMetadata | undefined;
const cellId = metadata?.cellId;
const cellIndex = metadata?.cellIndex;
console.log(`[DataframeRenderer] Extracted cellId: ${cellId}, cellIndex: ${cellIndex}`);
const root = document.createElement('div');
element.appendChild(root);
+ roots.set(outputItem.id, root);
ReactDOM.render(
React.createElement(DataframeRenderer, {
context,
data,
metadata: dataFrameMetadata,
cellId,
cellIndex
}),
root
);
} catch (error) {
console.error(`Error rendering dataframe: ${error}`);
const errorDiv = document.createElement('div');
errorDiv.style.padding = '10px';
errorDiv.style.color = 'var(--vscode-errorForeground)';
errorDiv.textContent = `Error rendering dataframe: ${error}`;
element.appendChild(errorDiv);
}
},
- disposeOutputItem(_id?: string) {
- // Cleanup if needed
+ disposeOutputItem(id?: string) {
+ if (id) {
+ const root = roots.get(id);
+ if (root) {
+ ReactDOM.unmountComponentAtNode(root);
+ roots.delete(id);
+ }
+ }
}
};
};Also applies to: 58-60
🤖 Prompt for AI Agents
In src/webviews/webview-side/dataframe-renderer/index.ts around lines 38-46 (and
similarly 58-60), ReactDOM.render mounts the DataframeRenderer into a DOM root
but never unmounts it, causing memory leaks when outputs are disposed; fix by
storing each root element (e.g., in a Map keyed by cellId or output id) at
render time and then calling ReactDOM.unmountComponentAtNode(root) inside
disposeOutputItem before removing the DOM node and deleting the Map entry so the
React component is properly cleaned up.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kernels/execution/cellExecutionCreator.ts (1)
30-37: Align clearOutputOnStartWithTime naming and semantics
The code always clears outputs on first start but the JSDoc says “when start is called with a time.” Either:
- Add
&& startTime !== undefinedto the clear-output guard- Rename the param (and JSDoc) to reflect clear-on-first-start (e.g.
clearOutputOnFirstStart)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(6 hunks)src/kernels/execution/cellExecutionCreator.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/execution/cellExecutionCreator.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/kernels/execution/cellExecutionCreator.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.ts
🧬 Code graph analysis (1)
src/kernels/execution/cellExecutionCreator.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/utils/misc.ts (1)
noop(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: copilot-setup-steps
🔇 Additional comments (3)
package.json (3)
2113-2113: Verify @deepnote/blocks v1.2.0 compatibility
Imports/usages in:
• src/notebooks/deepnote/deepnoteTypes.ts (DeepnoteBlock, DeepnoteFile)
• src/notebooks/deepnote/converters/textBlockConverter.ts (createMarkdown, stripMarkdown)
• src/kernels/execution/cellExecution.ts (createPythonCode)
Confirm no signature or behavior changes in these APIs and that metadata propagation remains intact.
2183-2183: Verify esbuild-postcss@0.0.4 compatibility with esbuild v0.25 & PostCSS 8
- esbuild-postcss@0.0.4 hasn’t been updated in ~4 years and lacks official support for esbuild v0.25 or PostCSS 8—likely incompatible with Tailwind v4’s @tailwindcss/postcss plugin.
- Test your esbuild v0.25 + postcss@8 + tailwindcss@4 + esbuild-postcss setup locally or switch to a maintained esbuild→PostCSS plugin (e.g., @chialab/esbuild-plugin-postcss).
1812-1822: Approve notebookRenderer contribution requiresMessaging "optional" is supported and build emits dist/webviews/webview-side/dataframeRenderer/dataframeRenderer.js as expected.
|
|
||
| // Clear outputs immediately on first start if configured to do so | ||
| // This ensures old outputs are removed before any new outputs arrive from the kernel | ||
| if (this.clearOutputOnStartWithTime) { | ||
| logger.trace(`Start cell ${this.cell.index} execution @ ${startTime} (clear output)`); | ||
| logger.trace(`Start cell ${this.cell.index} execution (clear output)`); | ||
| this._impl.clearOutput().then(noop, noop); | ||
| } else { | ||
| } | ||
|
|
||
| this._impl.start(startTime); | ||
| this._startTime = startTime; | ||
|
|
||
| if (startTime) { | ||
| logger.trace(`Start cell ${this.cell.index} execution @ ${startTime}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
clearOutput-before-start is not ordered; possible output flicker/loss
clearOutput() is async but start() is invoked immediately after. Kernel outputs may arrive before the clear resolves and be wiped, or logs will misrepresent ordering. Either:
- Chain start inside the clearOutput promise (ensuring ordering), or
- Adjust comments/logs to reflect that clear is only enqueued, and verify no flicker/loss in practice.
Recommend at least verifying under load.
🏁 Script executed:
#!/bin/bash
echo "=== clearOutputOnStartWithTime occurrences ==="
rg -n 'clearOutputOnStartWithTime' -n src/kernels/execution/cellExecutionCreator.ts
echo
echo "=== start() method and clearOutput usage ==="
rg -nP 'start\(' -C5 src/kernels/execution/cellExecutionCreator.ts
echo
echo "=== JSDoc blocks around start ==="
rg -nP '/\*\*' -C5 src/kernels/execution/cellExecutionCreator.tsLength of output: 3692
🏁 Script executed:
#!/bin/bash
sed -n '1,45p' src/kernels/execution/cellExecutionCreator.tsLength of output: 1494
Await clearOutput before start and align behavior with docs
- clearOutput() is async and races with start() → outputs may be wiped after arriving; await or chain start in clearOutput.then().
- JSDoc says “clear when start is called with a time” but code always clears when flag is true; adjust logic or update the comment.
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 63-76, the code
currently calls clearOutput() without awaiting it and always clears when the
flag is true, which can race with start() and disagrees with the JSDoc; change
the logic so that clearing only happens when clearOutputOnStartWithTime is true
AND startTime is provided, and ensure start() is invoked only after
clearOutput() completes (chain or await the promise and handle errors with
noop). Update the trace logs accordingly so the "clear output" trace is emitted
when clearing occurs and the regular start trace after start() is called.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)package.json(6 hunks)src/kernels/execution/cellExecutionCreator.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/execution/cellExecutionCreator.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/kernels/execution/cellExecutionCreator.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecutionCreator.ts
🧬 Code graph analysis (1)
src/kernels/execution/cellExecutionCreator.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/utils/misc.ts (1)
noop(11-11)
🔇 Additional comments (3)
package.json (3)
2184-2184: Remove esbuild-postcss compatibility warning
The build pipeline in build/esbuild/build.ts invokes PostCSS/Tailwind directly (postcss([tailwindcss, autoprefixer])) and does not use esbuild-postcss@0.0.4. The compatibility concern is not applicable.Likely an incorrect or invalid review comment.
2113-2113: @deepnote/blocks bump Imports in src/kernels/execution/cellExecution.ts (createPythonCode), src/notebooks/deepnote/deepnoteTypes.ts (DeepnoteBlock/File), src/notebooks/deepnote/converters/textBlockConverter.ts (createMarkdown/stripMarkdown); confirm v1.2.0 introduces no breaking API changes to these.
1812-1822: Renderer entrypoint: confirm build output
- requiresMessaging
"optional"is valid per VS Code schema.- confirm that
dist/webviews/webview-side/dataframeRenderer/dataframeRenderer.jsis emitted by your build.
|
|
||
| // Clear outputs immediately on first start if configured to do so | ||
| // This ensures old outputs are removed before any new outputs arrive from the kernel | ||
| if (this.clearOutputOnStartWithTime) { | ||
| logger.trace(`Start cell ${this.cell.index} execution @ ${startTime} (clear output)`); | ||
| logger.trace(`Start cell ${this.cell.index} execution (clear output)`); | ||
| this._impl.clearOutput().then(noop, noop); | ||
| } else { | ||
| } |
There was a problem hiding this comment.
Race condition: clearOutput not awaited before start.
clearOutput() returns a Thenable but isn't awaited, so _impl.start(startTime) on Line 71 may execute before outputs are actually cleared. Comment claims outputs are "removed before any new outputs arrive from the kernel," but implementation doesn't guarantee this ordering.
Consider making start() async or documenting that the clear is best-effort:
- start(startTime?: number): void {
+ async start(startTime?: number): Promise<void> {
// Allow this to be called more than once (so we can switch out a kernel during running a cell)
if (!this.started) {
this._started = true;
// Clear outputs immediately on first start if configured to do so
// This ensures old outputs are removed before any new outputs arrive from the kernel
if (this.clearOutputOnStartWithTime) {
logger.trace(`Start cell ${this.cell.index} execution (clear output)`);
- this._impl.clearOutput().then(noop, noop);
+ await this._impl.clearOutput();
}
this._impl.start(startTime);
this._startTime = startTime;
if (startTime) {
logger.trace(`Start cell ${this.cell.index} execution @ ${startTime}`);
}
}
}Note: This requires updating all callers to handle the async signature.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 63 to 69,
clearOutput() is called but not awaited so _impl.start(startTime) can run before
outputs are cleared, violating the "removed before any new outputs arrive"
guarantee; update the method to await this._impl.clearOutput() before proceeding
(i.e., make the enclosing start path async and await clearOutput or await it
conditionally), and then propagate the async change by updating all callers to
handle the new Promise-based signature (or alternatively document the call as
best-effort if you cannot change all callers).
🧹 Nitpick | 🔵 Trivial
Promise lifecycle not tracked.
Per coding guidelines, "Monitor and dispose pending promises to prevent leaks during teardown." The clearOutput() promise is fire-and-forget with .then(noop, noop), and errors are silently swallowed.
If keeping synchronous signature, at minimum log errors:
if (this.clearOutputOnStartWithTime) {
logger.trace(`Start cell ${this.cell.index} execution (clear output)`);
- this._impl.clearOutput().then(noop, noop);
+ this._impl.clearOutput().then(
+ noop,
+ (err) => logger.warn(`Failed to clear output for cell ${this.cell.index}`, err)
+ );
}🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 63 to 69, the call
to this._impl.clearOutput() is fire-and-forget and swallows errors; capture the
returned promise, attach a rejection handler that logs the error (e.g.,
logger.error) instead of noop, and store the promise reference in a class-level
tracker (e.g., this._pendingPromises or this._clearOutputPromise) so it can be
awaited or cancelled during teardown/dispose to avoid leaks; update the
dispose/teardown logic to await or clear that tracked promise.
| permissions: | ||
| id-token: write | ||
| contents: read | ||
| packages: read |
There was a problem hiding this comment.
Without this, we were not allowed to install the @deepnote/blocks package.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
103-112: Make base64 conversion cross-runtime (Node + Web).This issue was flagged in a previous review.
atobonly works in browsers; Node requiresBuffer. The helper should detect runtime and support both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/kernels/execution/cellExecutionCreator.ts(2 hunks)src/kernels/execution/cellExecutionCreator.unit.test.ts(1 hunks)src/kernels/execution/helpers.ts(10 hunks)src/notebooks/controllers/vscodeNotebookController.ts(2 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/execution/cellExecutionCreator.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/kernels/execution/helpers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/kernels/execution/cellExecutionCreator.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/kernels/execution/helpers.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/kernels/execution/helpers.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/kernels/execution/helpers.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs Deepnote data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
🧠 Learnings (8)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecutionCreator.tssrc/notebooks/controllers/vscodeNotebookController.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/**/*.ts : Monitor and dispose pending promises to prevent leaks during teardown
Applied to files:
src/kernels/execution/cellExecutionCreator.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Create a UI provider for kernel selection for the new kernel source
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/interactiveWindowController.ts : InteractiveWindowController must manage kernel selection/startup/lifecycle, system info cell handling, restart logic, and maintain owner file associations for kernel context
Applied to files:
src/notebooks/controllers/vscodeNotebookController.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (5)
src/kernels/execution/cellExecutionCreator.ts (3)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/utils/misc.ts (1)
noop(11-11)src/notebooks/controllers/notebookIPyWidgetCoordinator.ts (1)
controller(27-32)
src/notebooks/controllers/vscodeNotebookController.ts (1)
src/kernels/types.ts (1)
IKernelController(930-933)
src/notebooks/deepnote/deepnoteDataConverter.ts (4)
src/notebooks/deepnote/pocket.ts (1)
addPocketToCellMetadata(20-42)src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteOutput(9-9)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteOutput(23-51)src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
src/kernels/execution/cellExecutionCreator.unit.test.ts (2)
src/kernels/types.ts (1)
IKernelController(930-933)src/kernels/execution/cellExecutionCreator.ts (2)
NotebookCellExecutionWrapper(22-121)CellExecutionCreator(126-171)
src/kernels/execution/helpers.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (2)
NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)
🪛 GitHub Check: Lint & Format
src/kernels/execution/cellExecutionCreator.unit.test.ts
[failure] 234-234:
Unexpected empty method 'start'
[failure] 216-216:
Unexpected empty method 'start'
[failure] 186-186:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
[failure] 169-169:
Unexpected empty method 'end'
[failure] 168-168:
Unexpected empty method 'start'
[failure] 128-128:
Unexpected empty method 'end'
[failure] 91-91:
Unexpected empty method 'end'
[failure] 45-45:
Unexpected empty method 'end'
[failure] 28-28:
Unexpected empty arrow function
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (13)
src/notebooks/controllers/vscodeNotebookController.ts (2)
563-563: LGTM - consistent refactoring.Direct assignment of
this.controlleraligns with the removal of theKernelControllerwrapper at line 550. The subsequent controller reassignment logic (lines 574-579) remains sound.
550-550: Replace KernelController usages in interactive-windowIn src/interactive-window/interactiveWindow.ts (import at line 55, instantiation at line 259) remove
new KernelController(controller)and passcontrollerdirectly, confirming it implementsIKernelController.⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-extension#0 File: .github/instructions/interactiveWindow.instructions.md:0-0 Timestamp: 2025-09-03T12:54:35.368Z Learning: Applies to src/interactive-window/interactiveWindowController.ts : InteractiveWindowController must manage kernel selection/startup/lifecycle, system info cell handling, restart logic, and maintain owner file associations for kernel contextLearnt from: CR PR: deepnote/vscode-extension#0 File: .github/instructions/interactiveWindow.instructions.md:0-0 Timestamp: 2025-09-03T12:54:35.368Z Learning: Applies to src/interactive-window/interactiveWindow.ts : InteractiveWindow should serve as the ViewModel for a single window, integrate with notebook documents, execute cells, provide debugging capabilities and kernel lifecycle management, and transform Python file code into notebook cellsLearnt from: CR PR: deepnote/vscode-extension#0 File: .github/instructions/notebooks.instructions.md:0-0 Timestamp: 2025-09-03T12:59:14.489Z Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)Learnt from: CR PR: deepnote/vscode-extension#0 File: .github/instructions/notebooks.instructions.md:0-0 Timestamp: 2025-09-03T12:59:14.489Z Learning: Applies to src/notebooks/controllers/kernelSource/**/*.ts : Create a UI provider for kernel selection for the new kernel sourcesrc/notebooks/deepnote/deepnoteDataConverter.ts (2)
31-31: LGTM!The
__hadOutputsflag correctly preserves round-trip fidelity by tracking whether a block originally had outputs, avoiding spurious empty arrays on round-trip. Passingindex,block.id, andblock.metadatatotransformOutputsForVsCodeenables proper per-cell metadata propagation.Also applies to: 51-62
218-375: LGTM!The signature expansion and consistent metadata construction across all output types (error, execute_result/display_data, stream, unknown, fallback) correctly propagates
cellId,cellIndex, andblockMetadatato enable per-cell provenance tracking. The merge order (lines 302-314) correctly ensures output-specific metadata wins conflicts.src/kernels/execution/helpers.ts (5)
118-176: LGTM: Provenance parameters threaded consistently.The optional cellIndex, cellId, and cellMetadata parameters are properly typed and passed through the translation pipeline.
178-226: LGTM: Metadata merge strategy is sound.The ordering (cellId → cellMetadata → cellIndex → output metadata) ensures output-specific metadata takes precedence. Dual storage at top level and in
metadata.metadataserves the documented dual purpose.
240-288: LGTM: Translation functions consistently propagate provenance.All output translators now accept and forward the optional provenance parameters to
getOutputMetadata.
599-615: LGTM: Error output translation updated.Provenance parameters correctly propagated through error translation path.
331-338: LGTM: Interface documents provenance fields.
cellIndexandcellIdfields properly added toCellOutputMetadata.src/kernels/execution/cellExecutionCreator.ts (2)
128-154: LGTM: Execution replacement handles API constraint correctly.The logic properly ends existing executions (which can't be reused per VS Code API) and creates fresh wrappers. Auto-starting new executions when old ones were running maintains continuity during controller switches.
63-78: Document race risk & log clearOutput failures
- Confirm/document that kernel outputs may arrive before
clearOutput()completes and thus be cleared.- Replace
.then(noop, noop)with.catch(ex => logger.warn(\Failed to clear cell ${this.cell.index} output on start`, ex))`.src/kernels/execution/cellExecutionCreator.unit.test.ts (2)
1-195: LGTM: Comprehensive test coverage for execution lifecycle.Tests properly validate start ordering, idempotency, and lifecycle transitions. Manual spies for call order tracking are appropriate.
197-277: LGTM: Re-execution test validates fresh wrapper creation.Test correctly verifies that
getOrCreatealways ends old executions and creates fresh wrappers, with automatic start propagation when the old execution was running.
| // Convert VS Code outputs to Deepnote format | ||
| // Outputs are managed by VS Code natively, not stored in the pocket | ||
| // Only set outputs if the block originally had them (tracked by __hadOutputs flag) | ||
| const hadOutputs = cell.metadata?.__hadOutputs; | ||
| if (hadOutputs && !block.outputs && cell.outputs) { | ||
| block.outputs = this.transformOutputsForDeepnote(cell.outputs); | ||
| } | ||
|
|
||
| // Clean up internal tracking flags from metadata | ||
| if (block.metadata && '__hadOutputs' in block.metadata) { | ||
| delete block.metadata.__hadOutputs; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify __hadOutputs cleanup works correctly.
The code reads __hadOutputs from cell.metadata (line 89) but deletes it from block.metadata (line 95). This assumes createBlockFromPocket copies the flag from cell to block metadata. If it doesn't, the cleanup fails silently.
Run this script to verify the metadata flow:
🏁 Script executed:
#!/bin/bash
# Description: Verify createBlockFromPocket copies __hadOutputs to block.metadata
# Find createBlockFromPocket implementation
echo "=== createBlockFromPocket implementation ==="
ast-grep --pattern $'export function createBlockFromPocket($$$) {
$$$
}'
# Check if __hadOutputs is a deepnoteBlockSpecificField that gets moved to pocket
echo -e "\n=== deepnoteBlockSpecificFields list ==="
rg -nP 'deepnoteBlockSpecificFields\s*=\s*\[' -A 10
# Find how block.metadata is populated from cell.metadata
echo -e "\n=== Block metadata assignment in createBlockFromPocket ==="
rg -nP 'block\.metadata\s*=' --type=ts -C 3 src/notebooks/deepnote/pocket.tsLength of output: 1690
Add __hadOutputs to deepnoteBlockSpecificFields or delete from cell.metadata
__hadOutputs isn’t in deepnoteBlockSpecificFields (src/notebooks/deepnote/pocket.ts), so it’s never copied into block.metadata and delete block.metadata.__hadOutputs never runs. Either include __hadOutputs in the fields copied by createBlockFromPocket or remove the flag from cell.metadata instead.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteDataConverter.ts around lines 86 to 97, the
code checks and deletes block.metadata.__hadOutputs but that flag is never
copied into block.metadata because __hadOutputs is missing from
deepnoteBlockSpecificFields in src/notebooks/deepnote/pocket.ts; fix by either
(A) add "__hadOutputs" to deepnoteBlockSpecificFields so createBlockFromPocket
copies it into block.metadata (then the existing delete will work), or (B) if
__hadOutputs should remain only on the incoming cell, change this file to delete
the flag from cell.metadata instead of block.metadata (i.e., remove
block.metadata delete and delete cell.metadata.__hadOutputs after outputs
handling).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/kernels/execution/cellExecutionCreator.ts (1)
59-81: Race condition resolved; start now precedes clearOutput.The VS Code API requirement is satisfied (start before clearOutput), and error handling is improved with logging. However, since clearOutput is not awaited, the comment on Line 69 slightly overstates the guarantee.
Consider rewording Line 69:
- // This ensures old outputs are removed before any new outputs arrive from the kernel + // This clears old outputs shortly after start; new kernel outputs may arrive before clearing completes.github/workflows/ci.yml (1)
96-100: Correct: job-level permissions override defaults; keepingpackages: readhere is necessary.Because the build job defines
permissions, it overrides workflow-level ones. Without Line 99,packages: readwould be dropped and npm install from GH Packages would fail. Your change is right.Optionally:
- Keep global
packages: readfor other jobs that run npm ci (simpler), and keep the build job override as-is; or- Drop the global permission and add
packages: readto each job that installs deps (least-privilege).Forks: installs will still fail on fork PRs (no repo secrets). If you need fork builds, use a PAT with
read:packagesor a proxy.src/webviews/extension-side/dataframe/dataframeController.ts (5)
61-61: Still logging full message (data leak).This logs the entire message payload, leaking table contents when command is
copyTableData. Log only command + ids.Based on past review comments.
88-115: Still missing cellIndex fallback.Handler errors when
cellIdis absent or not found, ignoringmessage.cellIndex. Past comments flagged this.Based on past review comments.
134-134: Still not checking applyEdit result.
applyEditreturns boolean. Past comments flagged this. Check result and bail on failure before re-executing.Based on past review comments.
145-172: Still missing cellIndex fallback (goToPage).Same issue as
handleSelectPageSize: errors whencellIdis absent or not found, ignoringmessage.cellIndex. Past comments flagged this.Based on past review comments.
191-191: Still not checking applyEdit result (goToPage).Same issue as
handleSelectPageSize:applyEditreturns boolean that's ignored. Past comments flagged this.Based on past review comments.
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (1)
152-152: Fix localization parameterization.Template literal interpolation breaks localization. Use parameterized form instead.
- title: l10n.t(`Init: "${initNotebook.name}"`), + title: l10n.t('Init: "{0}"', initNotebook.name),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)src/kernels/execution/cellExecutionCreator.ts(3 hunks)src/kernels/execution/cellExecutionCreator.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts(3 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
🧠 Learnings (8)
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-10-14T14:15:55.496Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#28
File: src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts:93-99
Timestamp: 2025-10-14T14:15:55.496Z
Learning: For user Artmann in the deepnote/vscode-deepnote repository: avoid making nitpick or trivial-level review comments. Focus only on substantial issues like bugs, security concerns, performance problems, and significant architectural or design issues.
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-01T20:18:36.836Z
Learnt from: saltenasl
PR: deepnote/vscode-deepnote#0
File: :0-0
Timestamp: 2025-10-01T20:18:36.836Z
Learning: For the repository deepnote/vscode-deepnote, user saltenasl prefers reviews without nitpicks or minor comments. Focus only on significant issues.
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Applied to files:
src/kernels/execution/cellExecutionCreator.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/**/*.ts : Monitor and dispose pending promises to prevent leaks during teardown
Applied to files:
src/kernels/execution/cellExecutionCreator.ts
🧬 Code graph analysis (1)
src/kernels/execution/cellExecutionCreator.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/utils/misc.ts (1)
noop(11-11)
🔇 Additional comments (14)
src/kernels/execution/cellExecutionCreator.ts (2)
4-11: LGTM!Type-only imports reduce bundle size and clarify intent.
131-157: LGTM! Fresh wrapper on every re-execution.The logic correctly ends existing executions and creates fresh wrappers, preserving the started state. The comments clearly explain the behavior.
src/kernels/execution/cellExecutionCreator.unit.test.ts (8)
1-32: LGTM! Clean test setup.Proper use of Mocha/Chai and ts-mockito. Setup initializes all mocks consistently.
33-62: LGTM! Validates start-before-clearOutput ordering.Manual spy correctly verifies the fix for the race condition flagged in previous reviews.
64-76: LGTM! Negative test for clearOutputOnStartWithTime.Confirms clearOutput is not called when the flag is false.
78-116: LGTM! Verifies startTime propagation and ordering.Comprehensive test covering both parameter passing and call sequence.
118-153: LGTM! Confirms idempotent start().Multiple start() calls correctly execute clearOutput and start only once.
155-163: LGTM! Validates started flag lifecycle.Simple but essential test for state transitions.
165-206: LGTM! Tests clearOutput on reused execution.Line 198 correctly uses
voidoperator to handle the unhandled promise warning.
209-293: LGTM! Comprehensive re-execution test.Validates the new getOrCreate behavior: old execution ended, fresh wrapper created and started, clearOutput called appropriately. Covers all assertions for the replacement logic.
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (4)
2-9: LGTM!Type-qualified imports and l10n addition are correct.
13-13: LGTM!Type-only import is appropriate here.
145-145: LGTM!Localization correctly applied to static progress title.
245-245: LGTM!Progress calculation now correctly reaches 100% on the final block.
| type SelectPageSizeCommand = { | ||
| cellId?: string; | ||
| command: 'selectPageSize'; | ||
| size: number; | ||
| }; | ||
|
|
||
| type GoToPageCommand = { | ||
| cellId?: string; | ||
| command: 'goToPage'; | ||
| page: number; | ||
| }; | ||
|
|
||
| type CopyTableDataCommand = { | ||
| cellId?: string; | ||
| command: 'copyTableData'; | ||
| data: string; | ||
| }; | ||
|
|
||
| type ExportDataframeCommand = { | ||
| cellId?: string; | ||
| command: 'exportDataframe'; | ||
| }; | ||
|
|
||
| type DataframeCommand = SelectPageSizeCommand | GoToPageCommand | CopyTableDataCommand | ExportDataframeCommand; |
There was a problem hiding this comment.
Add cellIndex?: number to command types.
Past comments and handler logic indicate the renderer can send cellIndex as a fallback. Type definitions must include it to ensure type safety.
Apply this diff:
type SelectPageSizeCommand = {
cellId?: string;
+ cellIndex?: number;
command: 'selectPageSize';
size: number;
};
type GoToPageCommand = {
cellId?: string;
+ cellIndex?: number;
command: 'goToPage';
page: number;
};
type CopyTableDataCommand = {
cellId?: string;
+ cellIndex?: number;
command: 'copyTableData';
data: string;
};
type ExportDataframeCommand = {
cellId?: string;
+ cellIndex?: number;
command: 'exportDataframe';
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type SelectPageSizeCommand = { | |
| cellId?: string; | |
| command: 'selectPageSize'; | |
| size: number; | |
| }; | |
| type GoToPageCommand = { | |
| cellId?: string; | |
| command: 'goToPage'; | |
| page: number; | |
| }; | |
| type CopyTableDataCommand = { | |
| cellId?: string; | |
| command: 'copyTableData'; | |
| data: string; | |
| }; | |
| type ExportDataframeCommand = { | |
| cellId?: string; | |
| command: 'exportDataframe'; | |
| }; | |
| type DataframeCommand = SelectPageSizeCommand | GoToPageCommand | CopyTableDataCommand | ExportDataframeCommand; | |
| type SelectPageSizeCommand = { | |
| cellId?: string; | |
| cellIndex?: number; | |
| command: 'selectPageSize'; | |
| size: number; | |
| }; | |
| type GoToPageCommand = { | |
| cellId?: string; | |
| cellIndex?: number; | |
| command: 'goToPage'; | |
| page: number; | |
| }; | |
| type CopyTableDataCommand = { | |
| cellId?: string; | |
| cellIndex?: number; | |
| command: 'copyTableData'; | |
| data: string; | |
| }; | |
| type ExportDataframeCommand = { | |
| cellId?: string; | |
| cellIndex?: number; | |
| command: 'exportDataframe'; | |
| }; | |
| type DataframeCommand = | |
| | SelectPageSizeCommand | |
| | GoToPageCommand | |
| | CopyTableDataCommand | |
| | ExportDataframeCommand; |
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around lines 19
to 42, the command type definitions are missing the optional cellIndex property
that the renderer may send as a fallback; update each command type
(SelectPageSizeCommand, GoToPageCommand, CopyTableDataCommand,
ExportDataframeCommand) to include cellIndex?: number and ensure the
DataframeCommand union reflects those updated types so the handlers accept
cellIndex without type errors.
There was a problem hiding this comment.
We want to rely on cellId instead of cellIndex.
There was a problem hiding this comment.
@Artmann, understood. The current types correctly reflect that design—cellId only, no cellIndex fallback. I'll retract my suggestion.
✏️ Learnings added
Learnt from: Artmann
PR: deepnote/vscode-deepnote#28
File: src/webviews/extension-side/dataframe/dataframeController.ts:19-42
Timestamp: 2025-10-14T16:18:46.584Z
Learning: In the vscode-deepnote codebase, for dataframe renderer commands (SelectPageSizeCommand, GoToPageCommand, CopyTableDataCommand, ExportDataframeCommand), use cellId as the cell identifier and do not add cellIndex as a fallback.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (message.command === 'copyTableData') { | ||
| // TODO: Implement dataframe export functionality | ||
| return; | ||
| } | ||
|
|
||
| if (message.command === 'exportDataframe') { | ||
| // TODO: Implement dataframe export functionality | ||
| return; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Complete export/copy stubs.
These TODOs block dataframe export and clipboard features.
Do you want me to generate implementations or open tracking issues?
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around lines 75
to 82, the two TODO stubs for message.command === 'copyTableData' and
'exportDataframe' leave clipboard/export functionality unimplemented; replace
them with handlers that validate the incoming payload (e.g., ensure
message.payload.rows/columns or message.payload.data and optional format),
transform the data into the requested export format (CSV/JSON/TSV) using
existing util functions or a small serializer, and then either copy the
serialized string to the clipboard (use the VS Code webview API or a provided
copy util) for 'copyTableData' or trigger a file export flow for
'exportDataframe' (post a message back to the extension host with the serialized
data and filename or call the existing export helper). Ensure both handlers
catch errors, log or post error responses back to the caller, and send success
confirmations (so the UI can show success/failure).
There was a problem hiding this comment.
I'll do them later. Don't worry about it.
| </button> | ||
| </div> | ||
|
|
||
| <div>{/* Actions */}</div> |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (1)
22-22: Class still missing interface declaration.The class does not implement
IDeepnoteInitNotebookRunnerdespite the interface being defined at line 292.Apply this diff:
@injectable() -export class DeepnoteInitNotebookRunner { +export class DeepnoteInitNotebookRunner implements IDeepnoteInitNotebookRunner {.github/workflows/ci.yml (1)
13-17: Deduplicate packages: read; adopt one strategy.Pick one:
- Least‑privilege: remove global packages: read and grant it only to jobs that install from GH Packages (lint, typecheck, build, check_licenses, audits).
- Or keep global and avoid per‑job overrides, but build needs a job block (id‑token), so you’d still need packages: read there.
Also, if you expect fork PRs to build, GITHUB_TOKEN cannot read private GH Packages; use a PAT with read:packages or a proxy.
Also applies to: 96-100
src/webviews/extension-side/dataframe/dataframeController.ts (4)
88-143: Add cellIndex fallback for selectPageSize.The handler requires
cellIdand errors if not found, but the renderer may send onlycellIndex(common before metadata includes id). Accept and validatecellIndexas a fallback to avoid breaking pagination.Apply this diff:
- if (!message.cellId) { - const errorMessage = l10n.t( - 'Unable to update page size: No cell identifier provided. Please re-run the cell to update the output metadata.' - ); - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } - const cells = editor.notebook.getCells(); - const cell = cells.find((c) => c.metadata.id === message.cellId); - - if (!cell) { - const errorMessage = l10n.t( - 'Unable to update page size: Could not find the cell with ID {0}. The cell may have been deleted.', - message.cellId ?? '' - ); - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } - - const cellIndex = cell.index; + let cell = message.cellId ? cells.find((c) => c.metadata.id === message.cellId) : undefined; + let cellIndex = cell?.index; + + // Fallback to cellIndex if cellId lookup fails + if (!cell && typeof message.cellIndex === 'number') { + const idx = message.cellIndex; + if (idx >= 0 && idx < editor.notebook.cellCount) { + cell = editor.notebook.cellAt(idx); + cellIndex = idx; + logger.debug?.(`[DataframeController] selectPageSize: using cellIndex fallback ${idx}`); + } + } + + if (!cell || cellIndex === undefined) { + const errorMessage = l10n.t( + 'Unable to update page size: Could not resolve cell by id ({0}) or index ({1}).', + message.cellId ?? 'n/a', + (message as any).cellIndex ?? 'n/a' + ); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + }Note: Add
cellIndex?: numbertoSelectPageSizeCommandtype definition.
134-134: Check applyEdit result before re-execution.
applyEditreturns a boolean. If it fails, re-executing the cell is wasteful and may confuse users.Apply this diff:
- await workspace.applyEdit(edit); + const success = await workspace.applyEdit(edit); + if (!success) { + const errorMessage = l10n.t('Failed to update cell metadata.'); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + }
145-200: Add cellIndex fallback for goToPage.Like
handleSelectPageSize, this handler should acceptcellIndexas a fallback whencellIdis absent or not found. The renderer may send only the index.Apply this diff:
- if (!message.cellId) { - const errorMessage = l10n.t( - 'Unable to navigate to page: No cell identifier provided. Please re-run the cell to update the output metadata.' - ); - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } - const cells = editor.notebook.getCells(); - const cell = cells.find((c) => c.metadata.id === message.cellId); - - if (!cell) { - const errorMessage = l10n.t( - 'Unable to navigate to page: Could not find the cell with ID {0}. The cell may have been deleted.', - message.cellId ?? '' - ); - - logger.error(`[DataframeController] ${errorMessage}`); - - await window.showErrorMessage(errorMessage); - - throw new Error(errorMessage); - } - - // Update page index in table state within cell metadata + let cell = message.cellId ? cells.find((c) => c.metadata.id === message.cellId) : undefined; + let cellIndex = cell?.index; + + // Fallback to cellIndex if cellId lookup fails + if (!cell && typeof message.cellIndex === 'number') { + const idx = message.cellIndex; + if (idx >= 0 && idx < editor.notebook.cellCount) { + cell = editor.notebook.cellAt(idx); + cellIndex = idx; + logger.debug?.(`[DataframeController] goToPage: using cellIndex fallback ${idx}`); + } + } + + if (!cell || cellIndex === undefined) { + const errorMessage = l10n.t( + 'Unable to navigate to page: Could not resolve cell by id ({0}) or index ({1}).', + message.cellId ?? 'n/a', + (message as any).cellIndex ?? 'n/a' + ); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + } + const existingTableState = cell.metadata.deepnote_table_state || {}; const updatedTableState = { ...existingTableState, pageIndex: message.page }; - const cellIndex = cell.index; - const edit = new WorkspaceEdit();Note: Add
cellIndex?: numbertoGoToPageCommandtype definition.
191-191: Check applyEdit result before re-execution.Same as
handleSelectPageSize: check the boolean result and bail if the edit fails.Apply this diff:
- await workspace.applyEdit(edit); + const success = await workspace.applyEdit(edit); + if (!success) { + const errorMessage = l10n.t('Failed to update cell metadata.'); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + }src/kernels/execution/cellExecutionCreator.ts (1)
30-37: JSDoc mismatch: behavior ignores “with a time” qualifierDocs say “clear the output when start is called with a time,” but the code clears whenever the flag is true, regardless of startTime.
Align either the code or the doc:
- Code path: require startTime be provided:
- if (this.clearOutputOnStartWithTime) { + if (this.clearOutputOnStartWithTime && typeof startTime === 'number') {
- Or update the JSDoc to drop the “with a time” condition.
Which behavior do we want? I can update tests accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)src/kernels/execution/cellExecutionCreator.ts(3 hunks)src/kernels/execution/cellExecutionCreator.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts(3 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/webviews/extension-side/dataframe/dataframeController.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/kernels/execution/cellExecutionCreator.tssrc/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/kernels/execution/cellExecutionCreator.unit.test.tssrc/webviews/extension-side/dataframe/dataframeController.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
🧠 Learnings (7)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Applied to files:
src/kernels/execution/cellExecutionCreator.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/**/*.ts : Monitor and dispose pending promises to prevent leaks during teardown
Applied to files:
src/kernels/execution/cellExecutionCreator.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.tssrc/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-10-14T14:15:55.496Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#28
File: src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts:93-99
Timestamp: 2025-10-14T14:15:55.496Z
Learning: For user Artmann in the deepnote/vscode-deepnote repository: avoid making nitpick or trivial-level review comments. Focus only on substantial issues like bugs, security concerns, performance problems, and significant architectural or design issues.
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-01T20:18:36.836Z
Learnt from: saltenasl
PR: deepnote/vscode-deepnote#0
File: :0-0
Timestamp: 2025-10-01T20:18:36.836Z
Learning: For the repository deepnote/vscode-deepnote, user saltenasl prefers reviews without nitpicks or minor comments. Focus only on significant issues.
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts
🧬 Code graph analysis (3)
src/kernels/execution/cellExecutionCreator.ts (3)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/utils/misc.ts (1)
noop(11-11)src/notebooks/controllers/notebookIPyWidgetCoordinator.ts (1)
controller(27-32)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (1)
src/test/datascience/ipywidgets/samples/beakerx/extension.js (1)
codeBlocks(20949-20949)
src/kernels/execution/cellExecutionCreator.unit.test.ts (3)
src/kernels/types.ts (1)
IKernelController(930-933)src/test/mocks/vsc/index.ts (1)
CancellationToken(100-112)src/kernels/execution/cellExecutionCreator.ts (2)
NotebookCellExecutionWrapper(22-124)CellExecutionCreator(129-174)
🔇 Additional comments (5)
src/notebooks/deepnote/deepnoteInitNotebookRunner.node.ts (3)
2-9: LGTM.Type-only imports and l10n addition are correct.
145-145: LGTM.Progress titles correctly localized.
Also applies to: 152-152
245-245: LGTM.Progress now reaches 100% on the final block.
.github/workflows/ci.yml (1)
96-100: Correct: job-level packages: read is needed here.Once a job defines its own permissions, it overrides global. Adding packages: read to build is required for installing @deepnote/blocks.
src/kernels/execution/cellExecutionCreator.unit.test.ts (1)
188-206: Nice: explicit void on unawaited promiseThe linter warning is addressed with
void wrapper.clearOutput();. LGTM.
| // Must call start() before clearOutput() per VS Code API requirements | ||
| this._impl.start(startTime); | ||
| this._startTime = startTime; | ||
| // We clear the output as soon as we start, | ||
| // We generally start with a time, when we receive a response from the kernel, | ||
| // indicating the fact that the kernel has started processing the output. | ||
| // That's when we clear the output. (ideally it should be cleared as soon as its queued, but thats an upstream core issue). | ||
|
|
||
| // Clear outputs immediately after start if configured to do so | ||
| // This ensures old outputs are removed before any new outputs arrive from the kernel | ||
| if (this.clearOutputOnStartWithTime) { | ||
| logger.trace(`Start cell ${this.cell.index} execution @ ${startTime} (clear output)`); | ||
| this._impl.clearOutput().then(noop, noop); | ||
| } else { | ||
| logger.trace(`Start cell ${this.cell.index} execution (clear output)`); | ||
|
|
||
| this._impl | ||
| .clearOutput() | ||
| .then(noop, (err) => logger.warn(`Failed to clear output for cell ${this.cell.index}`, err)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Race persists: clearOutput is not awaited; output loss/flicker possible
Start is called, then clearOutput runs fire-and-forget. Kernel/user outputs may append before clear completes and be wiped. The comment “ensures old outputs are removed before any new outputs arrive” is not guaranteed.
Fix by tracking the clear promise and gating subsequent output writes on it.
Apply these changes:
- Add a private tracker:
export class NotebookCellExecutionWrapper implements NotebookCellExecution {
public _started: boolean = false;
private _startTime?: number;
+ private _clearOutputPromise?: Promise<void>;
public errorInfo: CellExecutionError;- Capture and handle the clear promise in start():
if (this.clearOutputOnStartWithTime) {
logger.trace(`Start cell ${this.cell.index} execution (clear output)`);
- this._impl
- .clearOutput()
- .then(noop, (err) => logger.warn(`Failed to clear output for cell ${this.cell.index}`, err));
+ this._clearOutputPromise = this._impl.clearOutput().catch((err) => {
+ logger.warn(`Failed to clear output for cell ${this.cell.index}`, err);
+ }).finally(() => {
+ this._clearOutputPromise = undefined;
+ });
}- Gate output methods on the pending clear:
clearOutput(cell?: NotebookCell): Thenable<void> {
this.startIfNecessary();
- return this._impl.clearOutput(cell);
+ const run = () => this._impl.clearOutput(cell);
+ return this._clearOutputPromise ? this._clearOutputPromise.then(run) : run();
}
replaceOutput(out: NotebookCellOutput | NotebookCellOutput[], cell?: NotebookCell): Thenable<void> {
this.startIfNecessary();
- return this._impl.replaceOutput(out, cell);
+ const run = () => this._impl.replaceOutput(out, cell);
+ return this._clearOutputPromise ? this._clearOutputPromise.then(run) : run();
}
appendOutput(out: NotebookCellOutput | NotebookCellOutput[], cell?: NotebookCell): Thenable<void> {
this.startIfNecessary();
- return this._impl.appendOutput(out, cell);
+ const run = () => this._impl.appendOutput(out, cell);
+ return this._clearOutputPromise ? this._clearOutputPromise.then(run) : run();
}
replaceOutputItems(items: NotebookCellOutputItem | NotebookCellOutputItem[], output: NotebookCellOutput): Thenable<void> {
this.startIfNecessary();
- return this._impl.replaceOutputItems(items, output);
+ const run = () => this._impl.replaceOutputItems(items, output);
+ return this._clearOutputPromise ? this._clearOutputPromise.then(run) : run();
}
appendOutputItems(items: NotebookCellOutputItem | NotebookCellOutputItem[], output: NotebookCellOutput): Thenable<void> {
this.startIfNecessary();
- return this._impl.appendOutputItems(items, output);
+ const run = () => this._impl.appendOutputItems(items, output);
+ return this._clearOutputPromise ? this._clearOutputPromise.then(run) : run();
}Optionally, make start() async and await the clear, but the above preserves the sync signature while ensuring ordering for all output writes. Based on learnings
Would you like me to add a focused test that simulates clearOutput latency and asserts appendOutput is gated?
🏁 Script executed:
#!/bin/bash
set -e
file=$(fd --full-path 'cellExecutionCreator.ts' | head -n 1)
echo "Inspecting: $file"
echo "=== clearOutputOnStartWithTime context ==="
rg -n -C2 'clearOutputOnStartWithTime' "$file"
echo
echo "=== start(...) method snippet ==="
rg -n -C3 'start\(' "$file"
echo
echo "=== output methods ==="
rg -n -C1 -e 'appendOutput' -e 'replaceOutput' -e 'clearOutput(' "$file"
echo
echo "=== getOrCreate occurrences ==="
rg -n 'getOrCreate' -g '*.ts'Length of output: 3739
Track clearOutput() promise to avoid races
The fire-and-forget clear can wipe new outputs. Introduce a private _clearOutputPromise in NotebookCellExecutionWrapper and gate all output methods (clearOutput, replaceOutput, appendOutput, etc.) on its resolution.
Align JSDoc with behavior
Update the clearOutputOnStartWithTime param doc to reflect that clearing happens on every start(), not only when startTime is provided.
Remove auto-start in getOrCreate
Current logic immediately calls start() on a new wrapper if the previous execution was started, bypassing CellExecutionQueue. Defer or remove this auto-start so the queue orchestrates executions.
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 64-76: the current
fire-and-forget call to this._impl.clearOutput() can race and wipe new outputs,
the JSDoc for clearOutputOnStartWithTime is inaccurate, and getOrCreate
auto-starts a new wrapper bypassing the execution queue. Fix by adding a private
_clearOutputPromise on NotebookCellExecutionWrapper and assign it when
clearOutput() is invoked (set to the clearOutput() promise and clear it on
settle); update all output-modifying methods (clearOutput, replaceOutput,
appendOutput, etc.) to wait for or chain off _clearOutputPromise before
performing their actions; update the JSDoc for clearOutputOnStartWithTime to
state clearing occurs on every start() call (not only when startTime provided);
and remove or defer the automatic start() call in getOrCreate so a newly created
wrapper is not started implicitly but instead left for the CellExecutionQueue to
orchestrate.
There was a problem hiding this comment.
We'll live with this for now.
There was a problem hiding this comment.
@Artmann, understood. I've noted your decision to defer this fix for now.
🧠 Learnings used
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution
| // Clear outputs immediately after start if configured to do so | ||
| // This ensures old outputs are removed before any new outputs arrive from the kernel | ||
| if (this.clearOutputOnStartWithTime) { | ||
| logger.trace(`Start cell ${this.cell.index} execution @ ${startTime} (clear output)`); | ||
| this._impl.clearOutput().then(noop, noop); | ||
| } else { | ||
| logger.trace(`Start cell ${this.cell.index} execution (clear output)`); | ||
|
|
||
| this._impl | ||
| .clearOutput() | ||
| .then(noop, (err) => logger.warn(`Failed to clear output for cell ${this.cell.index}`, err)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify API order requirement and guarantee language
The comment asserts VS Code API requires start() before clearOutput(). Please verify and adjust documentation if needed. Also weaken “ensures old outputs are removed…” since ordering isn’t guaranteed without gating.
🌐 Web query:
Does VS Code's NotebookCellExecution API require calling start() before clearOutput(), and are output methods valid prior to start()?
💡 Result:
Short answer: No — start() is not required. clearOutput(), append/replaceOutput(Items) and similar output methods can be called on a NotebookCellExecution before you call start(); start() only changes the execution state (Pending → Executing) and the running clock/visual state. The API docs describe the lifecycle (execution is created in Pending; start(...) moves it to Executing) but do not state that output methods are invalid until start() is called. [1][2]
Sources:
- NotebookCellExecution API (methods: clearOutput, appendOutput, replaceOutput; lifecycle description). [1]
- NotebookController / createNotebookCellExecution reference (creates a NotebookCellExecution object; start changes state). [2]
Update comment to reflect API semantics
- clearOutput() can be called before start(); start() only transitions state
- Change “ensures old outputs are removed before any new outputs arrive from the kernel” to “attempts to remove old outputs before new ones arrive”
Location: src/kernels/execution/cellExecutionCreator.ts (lines 68–76)
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 68 to 76, update
the inline comment to reflect API semantics: change the sentence that reads
“ensures old outputs are removed before any new outputs arrive from the kernel”
to a more accurate “attempts to remove old outputs before new ones arrive,” and
note that clearOutput() may be called before start() because start() only
transitions state; keep the surrounding logging and promise handling as-is.
There was a problem hiding this comment.
We'll live with this for now.
There was a problem hiding this comment.
@Artmann, understood! I've noted your decision.
🐰💭
(Respecting your call)
🧠 Learnings used
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/CellExecutionMessageHandler.ts : CellExecutionMessageHandler must process kernel messages (iopub, execute_reply) and update VS Code cell outputs
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution
| // If the old execution was started, start the new one immediately | ||
| // This handles the case where we're switching controllers mid-execution | ||
| if (wasStarted) { | ||
| cellExecution.start(new Date().getTime()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Avoid auto-starting in getOrCreate; let the CellExecutionQueue own start()
Auto-starting the new wrapper when the old one “wasStarted” bypasses the queue and can violate “enqueue via CellExecutionQueue” guidance. Also risks overlapping starts during re-exec flows.
Suggestion:
- Don’t call start() here. Return the fresh wrapper and let the queue/runner decide when to start.
- If needed, pass a hint back to the caller (e.g., wasPreviouslyStarted) to inform scheduling.
- if (wasStarted) {
- cellExecution.start(new Date().getTime());
- }Based on learnings
Confirm that re-exec paths (e.g., dataframe pagination) enqueue the cell instead of relying on auto-start here.
🏁 Script executed:
#!/bin/bash
set -e
grep -n "getOrCreate" src/kernels/execution/cellExecutionCreator.ts
sed -n '1,200p' src/kernels/execution/cellExecutionCreator.tsLength of output: 6715
🏁 Script executed:
#!/bin/bash
set -e
grep -R "CellExecutionQueue" -n srcLength of output: 1603
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,300p' src/kernels/execution/cellExecutionQueue.tsLength of output: 14264
🏁 Script executed:
#!/bin/bash
set -e
grep -n "class CellExecutionFactory" -n src/kernels/execution/cellExecution.ts
sed -n '1,200p' src/kernels/execution/cellExecution.tsLength of output: 9191
Remove auto-start from getOrCreate
Delete this block in src/kernels/execution/cellExecutionCreator.ts (lines 146–151):
- // If the old execution was started, start the new one immediately
- if (wasStarted) {
- cellExecution.start(new Date().getTime());
- }All starts must go through CellExecutionQueue; it will enqueue and invoke start() in order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the old execution was started, start the new one immediately | |
| // This handles the case where we're switching controllers mid-execution | |
| if (wasStarted) { | |
| cellExecution.start(new Date().getTime()); | |
| } |
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.ts around lines 146 to 151, remove
the block that calls cellExecution.start() when wasStarted is true so
getOrCreate does not auto-start executions; ensure getOrCreate only returns the
execution instance and that any logic relying on the previous auto-start is
moved to CellExecutionQueue (which should be responsible for enqueuing and
invoking start() in order).
There was a problem hiding this comment.
This breaks the outputs.
There was a problem hiding this comment.
🧩 Analysis chain
@Artmann, understood—I'll investigate what breaks when we remove that auto-start. Let me check the execution flow to see where start() is typically called and identify which scenario depends on this.
🏁 Script executed:
#!/bin/bash
# Find all places where getOrCreate is called and check if they call start()
rg -nP -A5 -B2 'getOrCreate\(' --type=ts src/kernels/execution/
# Check how notebook.cell.execute command flows into execution
rg -nP 'executeCommand.*notebook\.cell\.execute' --type=ts src/
# Look for controller switching logic
rg -nP 'controllerId.*!==|switchController|controller.*switch' --type=ts src/kernels/
Length of output: 7291
@Artmann, you're absolutely right—my suggestion was incorrect. After analyzing the execution flow, I see why the auto-start is necessary:
Re-execution scenario (e.g., dataframe pagination):
- Cell is already executing and displaying outputs (execution1 is started)
- User changes page size → triggers re-execution via
notebook.cell.executecommand getOrCreateends execution1 and creates fresh execution2- Without auto-start: execution2 never starts → no new outputs generated → broken
The auto-start specifically handles re-execution of already-running cells, which is distinct from initial cell execution that goes through the queue. The test even confirms this with the comment "Simulate re-execution (like what happens when pageSize changes in dataframe)".
The current implementation is correct for supporting re-execution flows. My apologies for the confusion.
Is there a different concern about the execution handling that I should address instead?
🧬 Code Graph Analysis Results
[src/platform/common/utils/misc.ts:11-11]
export function noop() {}
[src/test/mocks/vsc/extHostedTypes.ts:71-103]
export class NotebookCellOutput {
static isNotebookCellOutput(candidate: any): candidate is vscode.NotebookCellOutput {
if (candidate instanceof NotebookCellOutput) {
return true;
}
if (!candidate || typeof candidate !== 'object') {
return false;
}
return (
typeof (candidate).id === 'string' &&
Array.isArray((candidate).items)
);
}
id: string;
items: NotebookCellOutputItem[];
metadata?: Record<string, any>;
constructor(
items: NotebookCellOutputItem[],
idOrMetadata?: string | Record<string, any>,
metadata?: Record<string, any>
) {
this.items = items;
if (typeof idOrMetadata === 'string') {
this.id = idOrMetadata;
this.metadata = metadata;
} else {
this.id = generateUuid();
this.metadata = idOrMetadata ?? metadata;
}
}
}
[src/test/mocks/vsc/extHostedTypes.ts:16-69]
export class NotebookCellOutputItem {
static isNotebookCellOutputItem(obj: unknown): obj is vscode.NotebookCellOutputItem {
if (obj instanceof NotebookCellOutputItem) {
return true;
}
if (!obj) {
return false;
}
return (
typeof (<vscode.NotebookCellOutputItem>obj).mime === 'string' &&
(<vscode.NotebookCellOutputItem>obj).data instanceof Uint8Array
);
}
static error(err: Error | { name: string; message?: string; stack?: string }): NotebookCellOutputItem {
const obj = {
name: err.name,
message: err.message,
stack: err.stack
};
return NotebookCellOutputItem.json(obj, 'application/vnd.code.notebook.error');
}
static stdout(value: string): NotebookCellOutputItem {
return NotebookCellOutputItem.text(value, 'application/vnd.code.notebook.stdout');
}
static stderr(value: string): NotebookCellOutputItem {
return NotebookCellOutputItem.text(value, 'application/vnd.code.notebook.stderr');
}
static bytes(value: Uint8Array, mime: string = 'application/octet-stream'): NotebookCellOutputItem {
return new NotebookCellOutputItem(value, mime);
}
static #encoder = new TextEncoder();
static text(value: string, mime: string = 'text/plain'): NotebookCellOutputItem {
const bytes = NotebookCellOutputItem.#encoder.encode(String(value));
return new NotebookCellOutputItem(bytes, mime);
}
static json(value: any, mime: string = 'text/x-json'): NotebookCellOutputItem {
const rawStr = JSON.stringify(value, undefined, '\t');
return NotebookCellOutputItem.text(rawStr, mime);
}
constructor(
public data: Uint8Array,
public mime: string
) {
this.mime = mime;
}
}
[src/kernels/types.ts:930-933]
export interface IKernelController {
id: string;
createNotebookCellExecution(cell: NotebookCell): NotebookCellExecution;
}
[src/kernels/execution/codeExecution.ts:42-44]
public get result(): Promise {
return this._done.promise;
}
[src/kernels/execution/cellExecution.ts:69-71]
public get result(): Promise {
return this._result.promise;
}
⛔ Learnings removed
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
🧠 Learnings used
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/execution/**/*.ts : Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/webviews/extension-side/dataframe/dataframeController.ts (2)
191-199: Check applyEdit result before re-execution.Same issue as handleSelectPageSize: if applyEdit fails, re-executing is incorrect.
Apply this diff:
- await workspace.applyEdit(edit); + const success = await workspace.applyEdit(edit); + if (!success) { + const errorMessage = l10n.t('Failed to update cell metadata for page navigation.'); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + } // Re-execute the cell to apply the new page
134-142: Check applyEdit result before re-execution.applyEdit returns a boolean. If it fails, re-executing with stale metadata is pointless and confusing.
Apply this diff:
- await workspace.applyEdit(edit); + const success = await workspace.applyEdit(edit); + if (!success) { + const errorMessage = l10n.t('Failed to update cell metadata for page size change.'); + logger.error(`[DataframeController] ${errorMessage}`); + await window.showErrorMessage(errorMessage); + throw new Error(errorMessage); + } // Re-execute the cell to apply the new page size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)src/kernels/execution/cellExecutionCreator.unit.test.ts(1 hunks)src/webviews/extension-side/dataframe/dataframeController.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/webviews/extension-side/dataframe/dataframeController.tssrc/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
src/kernels/execution/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Notebook-level execution must enqueue cells via CellExecutionQueue and manage per-cell lifecycles through CellExecution
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
🧠 Learnings (3)
📚 Learning: 2025-10-14T16:18:46.584Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#28
File: src/webviews/extension-side/dataframe/dataframeController.ts:19-42
Timestamp: 2025-10-14T16:18:46.584Z
Learning: In the vscode-deepnote codebase, for dataframe renderer commands (SelectPageSizeCommand, GoToPageCommand, CopyTableDataCommand, ExportDataframeCommand), use cellId as the cell identifier and do not add cellIndex as a fallback.
Applied to files:
src/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/webviews/extension-side/dataframe/dataframeController.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Applied to files:
src/kernels/execution/cellExecutionCreator.unit.test.ts
🧬 Code graph analysis (2)
src/webviews/extension-side/dataframe/dataframeController.ts (2)
src/platform/common/types.ts (1)
IDisposable(211-213)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)
src/kernels/execution/cellExecutionCreator.unit.test.ts (2)
src/kernels/types.ts (1)
IKernelController(930-933)src/kernels/execution/cellExecutionCreator.ts (2)
NotebookCellExecutionWrapper(22-124)CellExecutionCreator(129-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
| // Now manually call clearOutput to simulate re-execution | ||
| // This simulates what CellExecutionCreator.getOrCreate() does when reusing a started wrapper |
There was a problem hiding this comment.
Clarify comment about wrapper reuse.
The comment states "This simulates what CellExecutionCreator.getOrCreate() does when reusing a started wrapper," but per the implementation, getOrCreate() always ends the old wrapper and creates a fresh one—it doesn't reuse. The test itself is valid (verifying clearOutput() can be called on a started wrapper), but the comment is misleading.
Update the comment to reflect actual behavior:
- // Now manually call clearOutput to simulate re-execution
- // This simulates what CellExecutionCreator.getOrCreate() does when reusing a started wrapper
+ // Now manually call clearOutput to verify it can be invoked on an already-started wrapper📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Now manually call clearOutput to simulate re-execution | |
| // This simulates what CellExecutionCreator.getOrCreate() does when reusing a started wrapper | |
| // Now manually call clearOutput to verify it can be invoked on an already-started wrapper |
🤖 Prompt for AI Agents
In src/kernels/execution/cellExecutionCreator.unit.test.ts around lines 196-197,
the existing comment incorrectly states that this simulates
CellExecutionCreator.getOrCreate() reusing a started wrapper; update the comment
to accurately state that getOrCreate() ends the old wrapper and creates a fresh
one, and that this test simply verifies clearOutput() can be called on a started
wrapper (i.e., it manually calls clearOutput to simulate operating on an
already-started wrapper, not to simulate reuse).
| _comms: NotebookRendererMessaging, | ||
| { editor, message }: { editor: NotebookEditor; message: DataframeCommand } | ||
| ) { | ||
| logger.info('DataframeController received message', message); |
There was a problem hiding this comment.
Still logging entire message payload.
Line 61 logs the full message, which can include table data (CopyTableDataCommand.data). Past reviews flagged this. Log only command + cellId.
Apply this diff:
- logger.info('DataframeController received message', message);
+ const safeLog = { command: message.command, cellId: (message as any).cellId };
+ logger.info('DataframeController received message', safeLog);Based on coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info('DataframeController received message', message); | |
| const safeLog = { command: message.command, cellId: (message as any).cellId }; | |
| logger.info('DataframeController received message', safeLog); |
🤖 Prompt for AI Agents
In src/webviews/extension-side/dataframe/dataframeController.ts around line 61,
the current logger.info call logs the entire incoming message (including
potentially large/sensitive table payloads). Change the log to only include the
message.command and message.cellId (e.g., logger.info('DataframeController
received message', { command: message.command, cellId: message.cellId })) and
avoid referencing message.data or the full message object; guard against
undefined fields when constructing the log object per coding guidelines.
Summary by CodeRabbit