Skip to content

Conversation

@jankuca
Copy link
Contributor

@jankuca jankuca commented Oct 21, 2025

Resolves GRN-4968

Screenshot 2025-10-21 at 13 08 14

The native commands discard the metadata of the block, disconnecting the copy from the integration. This PR makes sure all that is preserved.

Summary by CodeRabbit

  • New Features
    • Deepnote notebooks preserve cell metadata (including outputs and language) across copy, cut, and paste via a Deepnote-specific handler activated at startup.
  • Bug Fixes
    • Newly added Deepnote cells get fresh IDs and correct ordering automatically; metadata corrections are applied in batched updates with robust logging and error handling.

@linear
Copy link

linear bot commented Oct 21, 2025

@jankuca jankuca changed the title feat: SQL block duplicating command (Copy cell down) feat: SQL block duplicating commands (and copy&paste) Oct 21, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

📝 Walkthrough

Walkthrough

Adds DeepnoteCellCopyHandler (exported) and registers it as an IExtensionSyncActivationService. The handler intercepts notebook copy/cut/paste commands for .deepnote notebooks, encodes cell metadata to/from the clipboard with a marker, duplicates cell content/outputs/language, deletes originals on cut, and listens for notebook document changes to assign fresh IDs and updated sortingKey for newly added Deepnote cells. Metadata fixes are batched into a single WorkspaceEdit with a re-entrancy guard, error handling, and logging.

Sequence Diagram

sequenceDiagram
    participant User
    participant VSCode as VSCode Notebook
    participant Handler as DeepnoteCellCopyHandler
    participant Clipboard
    participant Workspace

    rect #EEF8FF
    Note right of Handler: Intercepts commands\n(copyDown, copyUp, copy, cut, paste)
    end

    User->>VSCode: Trigger copy / cut
    VSCode->>Handler: Intercept (if .deepnote)
    Handler->>Handler: Extract cell content, outputs, language, metadata
    Handler->>Clipboard: Write payload with marker "___DEEPNOTE_CELL_METADATA___"
    alt Cut
        Handler->>Workspace: Delete original cell
    end
    Handler->>VSCode: Insert duplicate / update selection

    User->>VSCode: Trigger paste
    VSCode->>Handler: Intercept paste (if .deepnote)
    Handler->>Clipboard: Read payload, detect marker
    alt Marker present
        Handler->>Handler: Reconstruct cell, generate new id & sortingKey
        Handler->>Workspace: Insert cell with preserved metadata (adjusted id/sortingKey)
    else
        VSCode->>Workspace: Fallback default paste
    end

    Note over Handler,Workspace: onDidChangeNotebookDocument queues adds\nand applyMetadataFixes batches WorkspaceEdit with re-entrancy guard
    Handler->>Workspace: Apply batched metadata updates
Loading

Suggested reviewers

  • jamesbhobbs
  • Artmann
  • andyjakubowski
  • saltenasl

Pre-merge checks

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat: SQL block duplicating commands (and copy&paste)" directly relates to the main change: a new handler that preserves Deepnote cell metadata during copy/cut/paste and duplicate operations. The title is concise, clear, and accurately captures the core functionality being added without unnecessary noise or vagueness.
Linked Issues Check ✅ Passed The PR description states the objective is to preserve block metadata when duplicating SQL blocks, resolving GRN-4968. The code changes directly implement this by introducing DeepnoteCellCopyHandler to intercept copy/cut/paste commands and preserve cell metadata through clipboard-based transport and batched metadata fixes. The implementation aligns with the stated objective of maintaining metadata during these operations.
Out of Scope Changes Check ✅ Passed All changes are directly scoped to the stated objective: the new DeepnoteCellCopyHandler class implements metadata preservation for copy/paste/duplicate operations, and its registration in the service registry is necessary supporting infrastructure. No unrelated refactoring, extraneous features, or scope creep is evident from the provided summaries.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 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 e58d440 and e7d9e07.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: jankuca
PR: deepnote/vscode-deepnote#101
File: src/notebooks/deepnote/deepnoteCellCopyHandler.ts:45-47
Timestamp: 2025-10-22T13:19:33.414Z
Learning: In the vscode-deepnote repository, for DeepnoteCellCopyHandler in src/notebooks/deepnote/deepnoteCellCopyHandler.ts, the design explicitly stores cell metadata directly in the OS clipboard rather than using in-memory tokens. The author prefers the clipboard to be the source of truth with no state in the app.
📚 Learning: 2025-10-22T13:19:33.414Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#101
File: src/notebooks/deepnote/deepnoteCellCopyHandler.ts:45-47
Timestamp: 2025-10-22T13:19:33.414Z
Learning: In the vscode-deepnote repository, for DeepnoteCellCopyHandler in src/notebooks/deepnote/deepnoteCellCopyHandler.ts, the design explicitly stores cell metadata directly in the OS clipboard rather than using in-memory tokens. The author prefers the clipboard to be the source of truth with no state in the app.

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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/deepnoteCellCopyHandler.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (6)
  • NotebookCellData (2523-2549)
  • NotebookCellOutput (71-103)
  • NotebookCellOutputItem (16-69)
  • WorkspaceEdit (649-833)
  • NotebookEdit (2444-2507)
  • NotebookRange (2413-2443)
src/notebooks/deepnote/dataConversionUtils.ts (2)
  • generateBlockId (8-15)
  • generateSortingKey (20-26)
⏰ 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). (3)
  • GitHub Check: Build & Test
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Package Extension
🔇 Additional comments (6)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (6)

1-34: LGTM: Clean imports and clipboard schema.

Standard imports properly ordered, clipboard marker and interface clearly defined.


43-59: LGTM: Activation wires up command overrides and change listener.

Proper injectable pattern, disposables tracked for cleanup. Command interception approach confirmed working per past discussion.


61-124: LGTM: Interceptors preserve native behavior for non-Deepnote notebooks.

Each interceptor correctly checks notebookType === 'deepnote' and falls back to default: commands. Pattern applied consistently across all five commands.


194-296: LGTM: Change handler regenerates IDs/sortingKeys while preserving integration metadata.

Re-entrancy guard prevents recursion, batched edits are efficient, error handling is solid. Correctly preserves sql_integration_id and other fields.


298-400: LGTM: Clipboard operations preserve metadata and fallback gracefully.

Copy/cut store full metadata per design choice. Paste correctly falls back to native behavior when marker is missing or parse fails. Shallow copy at line 378 is safe since JSON.parse creates fresh object trees.

Based on learnings.


163-163: No compatibility issues found — structuredClone is fully supported.

VS Code 1.101 ships with Node.js 22.15.1, and structuredClone is available in Node.js 17.0.0 and later. VS Code 1.103.0 will bundle an equally modern or newer Node.js version, making your use of structuredClone safe without requiring a fallback.


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

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: 6

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 83b7ab9 and a92758c.

📒 Files selected for processing (3)
  • package.json (1 hunks)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.node.ts

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

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/notebooks/serviceRegistry.node.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/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (3)
📚 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/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteCellCopyHandler.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/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-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
🪛 Gitleaks (8.28.0)
package.json

[high] 75-75: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (2)
src/notebooks/serviceRegistry.node.ts (1)

156-159: DI registration looks correct.

Singleton activation service registration mirrors adjacent services. No concerns.

src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1)

148-156: Selection handling: only the first selected cell is duplicated.

Built-in commands duplicate all selected cells. If parity is required, iterate over the selection range.
Would you like a patch to support multi-cell selections?

Base automatically changed from jk/feat/sql-block-credentials to main October 21, 2025 13:25
@jankuca jankuca force-pushed the jk/feat/sql-block-duplicate branch from a92758c to 5b014bf Compare October 21, 2025 15:18
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71%. Comparing base (9c81863) to head (585bb26).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #101   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        527     527           
  Lines      39500   39500           
  Branches    4935    4935           
=====================================
  Hits       28312   28312           
  Misses      9547    9547           
  Partials    1641    1641           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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: 1

♻️ Duplicate comments (6)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (5)

52-57: Replace built-in command registration with Deepnote-specific commands.

Registering notebook.cell.* IDs collides with core VS Code commands and will throw. Use deepnote.* command IDs and wire via keybindings in package.json.

Apply this diff:

-      // Override built-in notebook copy/cut commands to preserve metadata for Deepnote notebooks
-      this.disposables.push(commands.registerCommand('notebook.cell.copyDown', () => this.copyCellDownInterceptor()));
-      this.disposables.push(commands.registerCommand('notebook.cell.copyUp', () => this.copyCellUpInterceptor()));
-      this.disposables.push(commands.registerCommand('notebook.cell.copy', () => this.copyCellInterceptor()));
-      this.disposables.push(commands.registerCommand('notebook.cell.cut', () => this.cutCellInterceptor()));
-      this.disposables.push(commands.registerCommand('notebook.cell.paste', () => this.pasteCellInterceptor()));
+      // Deepnote-specific commands for copy/cut/paste
+      this.disposables.push(commands.registerCommand('deepnote.copyCell', () => this.copyCellToClipboard(false)));
+      this.disposables.push(commands.registerCommand('deepnote.cutCell', () => this.copyCellToClipboard(true)));
+      this.disposables.push(commands.registerCommand('deepnote.pasteCell', () => this.pasteCellFromClipboard()));

Then remove the unused interceptor methods (lines 67-126).


69-73: Use notebookType instead of file extension.

.endsWith('.deepnote') is brittle. Prefer editor.notebook.notebookType === 'deepnote'.

Apply everywhere:

- if (editor && editor.notebook.uri.path.endsWith('.deepnote')) {
+ if (editor && editor.notebook.notebookType === 'deepnote') {

Also applies to: 81-86, 95-100, 107-112, 121-126, 143-146, 213-215, 324-327, 373-376


163-181: Consolidate id/sortingKey mutation in onDidChange.

Currently you mutate these fields during copy/paste and again in the change handler. Do it once in onDidChangeNotebookDocument to avoid double-mutation.

Apply this diff:

         // Copy all metadata, but generate new ID and sortingKey
         if (cellToCopy.metadata) {
-            const copiedMetadata = { ...cellToCopy.metadata };
-            // Generate new unique ID
-            copiedMetadata.id = generateBlockId();
-            // Update sortingKey in pocket if it exists
-            if (copiedMetadata.__deepnotePocket) {
-                copiedMetadata.__deepnotePocket = {
-                    ...copiedMetadata.__deepnotePocket,
-                    sortingKey: generateSortingKey(insertIndex)
-                };
-            } else if (copiedMetadata.sortingKey) {
-                copiedMetadata.sortingKey = generateSortingKey(insertIndex);
-            }
-            newCell.metadata = copiedMetadata;
+            // Copy metadata verbatim; onDidChange will assign new id/sortingKey
+            newCell.metadata = { ...cellToCopy.metadata };

Repeat for paste logic at lines 400-417.

Also applies to: 400-417


182-189: Scrub PII from metadata logs.

Full JSON logs can leak integration IDs, tokens, etc.

As per coding guidelines.

Apply minimal changes:

- logger.info(`DeepnoteCellCopyHandler: Copying cell with metadata preserved: ${JSON.stringify(copiedMetadata, null, 2)}`);
+ logger.info('DeepnoteCellCopyHandler: Copying cell with metadata preserved (metadata omitted for PII)');

Repeat at lines 240-241, 276-281, 348-354, 363-364, 419-425, 436-439. Optionally, add a sanitizeMetadata() helper to whitelist safe fields.

Also applies to: 240-241, 276-281, 348-354, 363-364, 419-425, 436-439


192-194: Deep-copy outputs to avoid shared references.

Shallow copy preserves object references. Create new NotebookCellOutput instances.

  if (cellToCopy.outputs.length > 0) {
-     newCell.outputs = cellToCopy.outputs.map((output) => output);
+     newCell.outputs = cellToCopy.outputs.map((o) =>
+         new NotebookCellOutput(
+             o.items.map((i) => new NotebookCellOutputItem(i.data, i.mime)),
+             o.metadata
+         )
+     );
  }
package.json (1)

74-79: Add missing keybindings and command metadata.

Only copyCellDown has a binding. Missing copyCellUp and C/X/V for copy/cut/paste.

Add Shift+Alt+Up:

      {
        "key": "shift+alt+down",
        "mac": "shift+alt+down",
        "when": "notebookType == deepnote && notebookEditorFocused && !inputFocus",
        "command": "deepnote.copyCellDown"
+     },
+     {
+       "key": "shift+alt+up",
+       "mac": "shift+alt+up",
+       "when": "notebookType == deepnote && notebookEditorFocused && !inputFocus",
+       "command": "deepnote.copyCellUp"
      }

Add under contributes.commands:

{
  "command": "deepnote.copyCellDown",
  "title": "Copy Cell Down",
  "category": "Deepnote"
},
{
  "command": "deepnote.copyCellUp",
  "title": "Copy Cell Up",
  "category": "Deepnote"
},
{
  "command": "deepnote.copyCell",
  "title": "Copy Cell",
  "category": "Deepnote"
},
{
  "command": "deepnote.cutCell",
  "title": "Cut Cell",
  "category": "Deepnote"
},
{
  "command": "deepnote.pasteCell",
  "title": "Paste Cell",
  "category": "Deepnote"
}

Add C/X/V keybindings for Mac (Cmd) and Windows (Ctrl) with !inputFocus guard.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a92758c and 5b014bf.

📒 Files selected for processing (3)
  • package.json (1 hunks)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.node.ts

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

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/notebooks/serviceRegistry.node.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/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (3)
📚 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/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteCellCopyHandler.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-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} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (4)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/test/datascience/mockDocument.ts (1)
  • offset (31-33)
src/test/mocks/vsc/extHostedTypes.ts (3)
  • NotebookCellData (2523-2549)
  • NotebookEdit (2444-2507)
  • NotebookRange (2413-2443)
src/notebooks/deepnote/dataConversionUtils.ts (2)
  • generateBlockId (8-15)
  • generateSortingKey (20-26)
⏰ 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). (4)
  • GitHub Check: Build & Test
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Package Extension
  • GitHub Check: copilot-setup-steps
🔇 Additional comments (1)
src/notebooks/serviceRegistry.node.ts (1)

70-70: LGTM: Activation service registration.

Singleton registration follows the standard pattern for extension activation services.

Also applies to: 156-159

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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b014bf and 0c661e0.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (2)
📚 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} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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/deepnoteCellCopyHandler.ts
⏰ 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). (3)
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Test
  • GitHub Check: Build & Package Extension
🔇 Additional comments (5)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (5)

61-86: Good gating by notebookType.

Using editor.notebook.notebookType === 'deepnote' avoids brittle path checks. Looks correct.


138-188: Copy‑down/up flow looks correct.

  • Inserts at proper index and moves selection to the new cell.
  • Deepnote‑only guard prevents accidental use elsewhere.

190-265: Metadata refresh strategy is solid.

  • Processes only Deepnote cells.
  • Regenerates id and sortingKey for added cells; batches edits; guarded against re‑entrancy.

161-167: Good: no raw metadata in logs.

Logs avoid printing full metadata/IDs. This aligns with our PII‑scrubbing guidance.

Based on learnings

Also applies to: 218-221, 255-257, 325-335


49-56: Remove "default:" prefix from command IDs—breaks fallback execution.

The fallback calls use incorrect command IDs. Per VS Code docs, default:notebook.cell.* is keybinding notation only; the executable command IDs are plain (e.g., "notebook.cell.copyDown"). Update all five interceptors:

  • Line 72, 84, 96, 108, 120: Change 'default:notebook.cell.*''notebook.cell.*'

Without this fix, fallback execution for non-Deepnote notebooks will fail silently.

⛔ Skipped due to 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.
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

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: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c661e0 and 5b2cf58.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (2)
📚 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} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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/deepnoteCellCopyHandler.ts
⏰ 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). (3)
  • GitHub Check: Build & Package Extension
  • GitHub Check: Check Licenses
  • GitHub Check: Build & Test
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1)

49-56: Verify availability of default:notebook.cell. fallbacks across your target VS Code engine.*

If any default:* command is unavailable, non‑Deepnote notebooks will error when we delegate.

Run once at activation and cache:

const known = await commands.getCommands(true);
const hasDefaults = [
  'default:notebook.cell.copy',
  'default:notebook.cell.cut',
  'default:notebook.cell.paste',
  'default:notebook.cell.copyDown',
  'default:notebook.cell.copyUp',
].every((id) => known.includes(id));
// If !hasDefaults, consider keybinding-based routing instead of overriding IDs.

Also applies to: 65-72, 78-85, 91-98, 104-111, 117-124

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b2cf58 and e58d440.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/notebooks/deepnote/deepnoteCellCopyHandler.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/notebooks/deepnote/deepnoteCellCopyHandler.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: jankuca
PR: deepnote/vscode-deepnote#101
File: src/notebooks/deepnote/deepnoteCellCopyHandler.ts:45-47
Timestamp: 2025-10-22T13:19:33.414Z
Learning: In the vscode-deepnote repository, for DeepnoteCellCopyHandler in src/notebooks/deepnote/deepnoteCellCopyHandler.ts, the design explicitly stores cell metadata directly in the OS clipboard rather than using in-memory tokens. The author prefers the clipboard to be the source of truth with no state in the app.
📚 Learning: 2025-10-22T13:19:33.414Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#101
File: src/notebooks/deepnote/deepnoteCellCopyHandler.ts:45-47
Timestamp: 2025-10-22T13:19:33.414Z
Learning: In the vscode-deepnote repository, for DeepnoteCellCopyHandler in src/notebooks/deepnote/deepnoteCellCopyHandler.ts, the design explicitly stores cell metadata directly in the OS clipboard rather than using in-memory tokens. The author prefers the clipboard to be the source of truth with no state in the app.

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/notebooks/deepnote/deepnoteCellCopyHandler.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/deepnoteCellCopyHandler.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (3)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/test/mocks/vsc/extHostedTypes.ts (5)
  • NotebookCellData (2523-2549)
  • NotebookCellOutput (71-103)
  • NotebookCellOutputItem (16-69)
  • NotebookEdit (2444-2507)
  • NotebookRange (2413-2443)
src/notebooks/deepnote/dataConversionUtils.ts (2)
  • generateBlockId (8-15)
  • generateSortingKey (20-26)
⏰ 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). (2)
  • GitHub Check: Build & Package Extension
  • GitHub Check: Build & Test
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteCellCopyHandler.ts (1)

342-400: LGTM: Fallback behavior correctly preserves native paste.

When clipboard lacks the marker (line 363) or parsing fails (line 398), the code falls back to default:notebook.cell.paste, preventing functional regression for standard clipboard content.

@jankuca jankuca marked this pull request as ready for review October 22, 2025 13:55
@jankuca jankuca enabled auto-merge (squash) October 22, 2025 15:18
@jankuca jankuca merged commit f142367 into main Oct 22, 2025
12 checks passed
@jankuca jankuca deleted the jk/feat/sql-block-duplicate branch October 22, 2025 15:21
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