Skip to content

Conversation

@Artmann
Copy link
Member

@Artmann Artmann commented Oct 15, 2025

CleanShot 2025-10-15 at 14 22 13

Summary by CodeRabbit

  • New Features

    • Copy and Export Table actions for DataFrame content (clipboard and CSV save).
    • Page-size selector and improved pagination controls with icon-only buttons and stable element IDs.
    • UI messages/localization added for DataFrame controls.
  • Bug Fixes

    • More consistent, user-friendly error messages for missing data, cancelled saves, and other action failures.
  • Tests

    • Comprehensive unit tests covering CSV conversion, message handling, copy/export flows, and error paths.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

📝 Walkthrough

Walkthrough

Implemented a message-driven DataFrame workflow: extension-side DataframeController now routes DataframeCommand messages (copyTable, exportTable, goToPage, selectPageSize) to handlers that extract DataFrameObject from cell outputs, convert to CSV, update cell metadata, persist edits, and re-execute cells as needed. Webview-side DataframeRenderer adds IconButton, UUID-based select IDs, clsx/tailwind-merge for classes, and posts copy/export/select/goToPage messages. Added unit tests for CSV conversion, dataframe extraction, copy/export flows, message routing, and VS Code API interactions. package.json now includes clsx and tailwind-merge. Localization keys for dataframe UI were added.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant R as DataframeRenderer (Webview)
  participant V as VSCode Webview Channel
  participant C as DataframeController (Extension)
  participant N as VSCode Notebook/Workspace
  participant CL as Clipboard
  participant FS as FileSystem
  participant W as VSCode Window

  rect rgba(230, 240, 255, 0.5)
  note over R,C: New message-driven actions
  U->>R: Click "Copy" or "Export"
  R->>V: postMessage {command: copyTable|exportTable, cellId}
  V->>C: onDidReceiveMessage(...)
  alt copyTable
    C->>N: Find cell by cellId / read outputs
    C->>C: getDataframeFromDataframeOutput()
    C->>C: dataframeToCsv()
    C->>CL: writeText(csv)
    C->>W: showInformationMessage("Copied")
  else exportTable
    C->>N: Find cell by cellId / read outputs
    C->>C: getDataframeFromDataframeOutput()
    C->>C: dataframeToCsv()
    C->>W: showSaveDialog()
    alt User selects path
      C->>FS: writeFile(csv)
      C->>W: showInformationMessage("Exported")
    else Cancel or no path
      C->>C: Abort silently
    end
  end
  end

  opt Error paths
    C->>W: showErrorMessage(...)
  end
Loading
sequenceDiagram
  autonumber
  actor U as User
  participant R as DataframeRenderer (Webview)
  participant V as VSCode Webview Channel
  participant C as DataframeController (Extension)
  participant N as VSCode Notebook/Workspace
  participant K as Kernel

  rect rgba(240, 255, 240, 0.5)
  note over R,C: Updated pagination and page size
  U->>R: Change page size or navigate page
  alt selectPageSize
    R->>V: postMessage {command: selectPageSize, cellId, pageSize}
    V->>C: onDidReceiveMessage(...)
    C->>N: Update cell.metadata.deepnote_table_state.pageSize
    C->>N: applyEdit()
    C->>K: execute cell
  else goToPage
    R->>V: postMessage {command: goToPage, cellId, pageIndex}
    V->>C: onDidReceiveMessage(...)
    C->>N: Update cell.metadata.deepnote_table_state.pageIndex
    C->>N: applyEdit()
    C->>K: execute cell
  end
  end

  opt Error paths
    C->>W: showErrorMessage(...)
  end
Loading

Suggested reviewers

  • jankuca
  • andyjakubowski

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly captures the primary feature addition of copying and exporting data frames and aligns with the PR changes by focusing on the core functionality without extraneous detail.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56e49ae and 4cc735b.

📒 Files selected for processing (6)
  • src/messageTypes.ts (1 hunks)
  • src/platform/common/utils/localize.ts (1 hunks)
  • src/platform/webviews/webviewHost.ts (1 hunks)
  • src/webviews/extension-side/dataframe/dataframeController.ts (5 hunks)
  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts (1 hunks)
  • src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/messageTypes.ts
  • src/platform/webviews/webviewHost.ts
  • src/webviews/extension-side/dataframe/dataframeController.ts
  • src/platform/common/utils/localize.ts
  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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/messageTypes.ts
  • src/platform/webviews/webviewHost.ts
  • src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
  • src/webviews/extension-side/dataframe/dataframeController.ts
  • src/platform/common/utils/localize.ts
  • src/webviews/extension-side/dataframe/dataframeController.unit.test.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/webviews/webviewHost.ts
  • src/platform/common/utils/localize.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/webviews/extension-side/dataframe/dataframeController.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 then property to avoid hanging tests

Files:

  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: Artmann
PR: deepnote/vscode-deepnote#28
File: src/webviews/extension-side/dataframe/dataframeController.ts:19-42
Timestamp: 2025-10-14T16:18:46.614Z
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.
📚 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/webview-side/dataframe-renderer/DataframeRenderer.tsx
📚 Learning: 2025-09-03T14:50:58.575Z
Learnt from: jankuca
PR: deepnote/deepnote#17463
File: apps/webapp-client/src/components/notebook/cell-sidebar/ExecuteSectionActionDropdownItem.tsx:18-46
Timestamp: 2025-09-03T14:50:58.575Z
Learning: In the Deepnote codebase, avoid suggesting accessibility improvements (like aria-label attributes) or internationalization changes (like centralizing string constants) if these patterns are not consistently applied elsewhere in the codebase, as maintaining consistency with existing patterns is prioritized over individual improvements.

Applied to files:

  • src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
📚 Learning: 2025-10-15T12:48:33.517Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#70
File: src/webviews/extension-side/dataframe/dataframeController.ts:192-200
Timestamp: 2025-10-15T12:48:33.517Z
Learning: VS Code's SaveDialogOptions interface only supports: defaultUri (Uri), filters (object mapping names to extension arrays), saveLabel (string), and title (string). There is no suggestedName property.

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 typed error classes from `src/platform/errors/` when throwing or handling errors

Applied to files:

  • src/webviews/extension-side/dataframe/dataframeController.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/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/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage

Applied to files:

  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
🧬 Code graph analysis (2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (3)
src/platform/common/uuid.ts (1)
  • generateUuid (10-61)
src/webviews/extension-side/dataframe/dataframeController.ts (2)
  • handleCopyTable (151-189)
  • handleExportTable (191-247)
src/webviews/webview-side/react-common/locReactSide.ts (1)
  • getLocString (11-17)
src/webviews/extension-side/dataframe/dataframeController.unit.test.ts (4)
src/platform/common/types.ts (1)
  • IDisposable (211-213)
src/webviews/extension-side/dataframe/dataframeController.ts (1)
  • dispose (68-70)
src/test/mocks/vsc/extHostedTypes.ts (3)
  • NotebookCellOutput (71-103)
  • NotebookCellOutputItem (16-69)
  • NotebookCellData (2523-2549)
src/test/datascience/editor-integration/helpers.ts (1)
  • createMockedNotebookDocument (159-204)
🔇 Additional comments (16)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (6)

1-9: LGTM!

Imports align with new functionality: clsx/twMerge for className composition, generateUuid for stable IDs, and getLocString for localization (addressing past review feedback).


75-75: LGTM!

Stable, unique ID generation per component instance using useMemo prevents ID collisions.


117-126: LGTM!

Handler follows existing patterns and correctly uses cellId per project conventions.


128-137: LGTM!

Consistent with handleCopyTable and follows established patterns.


256-277: LGTM!

Clean, reusable button component with proper prop spreading and className composition.


279-281: LGTM!

Standard Tailwind pattern combining clsx for conditional classes with twMerge for conflict resolution.

src/webviews/extension-side/dataframe/dataframeController.ts (3)

151-189: LGTM!

Proper validation, localization, and clipboard integration with clear error handling.


298-345: LGTM!

Logger tag correct and consistent with other handlers.


347-374: LGTM!

Clean message routing with proper validation and logging for unknown commands.

src/webviews/extension-side/dataframe/dataframeController.unit.test.ts (4)

21-47: LGTM!

Proper test setup with comprehensive mocks and clean teardown using dispose pattern.


304-367: LGTM!

Tests both error and success paths for dataframe extraction with proper mock outputs.


369-737: LGTM!

Thorough coverage of copy and export flows with proper error handling and content verification.


739-868: LGTM!

Message routing tested with clean helper functions for mock creation and proper integration points.

src/platform/common/utils/localize.ts (1)

808-814: LGTM!

Localization keys follow existing patterns with proper l10n.t usage and descriptive names.

src/messageTypes.ts (1)

161-167: LGTM!

Type definitions properly added for new localization keys, consistent with existing interface structure.

src/platform/webviews/webviewHost.ts (1)

261-268: LGTM!

Localization chain completed with proper mapping from localize.WebViews to frontend payload.


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

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 92.92453% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 70%. Comparing base (45867aa) to head (4cc735b).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ws/extension-side/dataframe/dataframeController.ts 71% 19 Missing and 8 partials ⚠️
...on-side/dataframe/dataframeController.unit.test.ts 99% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main     #70    +/-   ##
======================================
  Coverage     70%     70%            
======================================
  Files        513     515     +2     
  Lines      37701   38159   +458     
  Branches    4822    4854    +32     
======================================
+ Hits       26648   27053   +405     
- Misses      9456    9501    +45     
- Partials    1597    1605     +8     
Files with missing lines Coverage Δ
src/messageTypes.ts 100% <ø> (ø)
src/platform/common/utils/localize.ts 86% <100%> (+<1%) ⬆️
src/platform/webviews/webviewHost.ts 48% <ø> (ø)
...on-side/dataframe/dataframeController.unit.test.ts 99% <99%> (ø)
...ws/extension-side/dataframe/dataframeController.ts 60% <71%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Artmann Artmann force-pushed the chris/copy-and-export-data-frames branch from bfc7976 to 5da8ac4 Compare October 15, 2025 12:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

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

⚠️ Outside diff range comments (2)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)

64-69: Remove or gate console logs.

Avoid console.log in TSX; either use a webview logger or guard behind a dev flag. Noise in production console makes troubleshooting harder.

Also applies to: 87-98, 103-114, 116-136


188-197: Avoid duplicate classes on IconButton.

IconButton already sets border/background/size. Drop the repeated className on usage to rely on defaults.

-<IconButton
-  aria-label="Previous page"
-  className={`
-    border border-[var(--vscode-panel-border)] bg-[var(--vscode-button-secondaryBackground)] hover:bg-[var(--vscode-button-secondaryHoverBackground)]
-    text-[var(--vscode-button-secondaryForeground)]
-    disabled:opacity-50 disabled:cursor-not-allowed
-    flex items-center justify-center
-    h-[20px] w-[20px]
-  `}
+<IconButton
+  aria-label="Previous page"
   disabled={pageIndex === 0}
   title="Previous page"
   type="button"
   onClick={() => handlePageChange(pageIndex - 1)}
>

Also applies to: 207-216

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45867aa and 56e49ae.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • package.json (2 hunks)
  • src/webviews/extension-side/dataframe/dataframeController.ts (6 hunks)
  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts (1 hunks)
  • src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (6 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 .ts files (not suffixed with .node or .web)

Files:

  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
  • src/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
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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.unit.test.ts
  • src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx
  • src/webviews/extension-side/dataframe/dataframeController.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/webviews/extension-side/dataframe/dataframeController.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 then property to avoid hanging tests

Files:

  • src/webviews/extension-side/dataframe/dataframeController.unit.test.ts
🧠 Learnings (2)
📚 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/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage

Applied to files:

  • src/webviews/extension-side/dataframe/dataframeController.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/webviews/extension-side/dataframe/dataframeController.ts
🧬 Code graph analysis (3)
src/webviews/extension-side/dataframe/dataframeController.unit.test.ts (5)
src/notebooks/controllers/notebookIPyWidgetCoordinator.ts (1)
  • controller (27-32)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (60-79)
  • mockedVSCodeNamespaces (17-17)
src/webviews/extension-side/dataframe/dataframeController.ts (1)
  • dispose (68-70)
src/test/datascience/editor-integration/helpers.ts (1)
  • createMockedNotebookDocument (159-204)
src/test/datascience/mockTextEditor.ts (1)
  • document (67-69)
src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx (2)
src/platform/common/uuid.ts (1)
  • generateUuid (10-61)
src/webviews/extension-side/dataframe/dataframeController.ts (2)
  • handleCopyTable (117-155)
  • handleExportTable (157-211)
src/webviews/extension-side/dataframe/dataframeController.ts (1)
src/platform/common/process/types.node.ts (1)
  • Output (15-18)
🔇 Additional comments (1)
package.json (1)

2128-2128: Deps look good; confirm bundling for webview.

clsx and tailwind-merge versions align with Tailwind v4 usage. Ensure these are included in the webview bundle and not pulled into extension host unintentionally.

Also applies to: 2167-2167

@Artmann Artmann marked this pull request as ready for review October 15, 2025 13:08
Copy link
Contributor

@andyjakubowski andyjakubowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@Artmann Artmann merged commit 92b4461 into main Oct 15, 2025
10 checks passed
@Artmann Artmann deleted the chris/copy-and-export-data-frames branch October 15, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants