feat: Delete python virtual environment directory when removing deepnote environment

[tkislan]

Signed-off-by: Tomas Kislan tomas@kislan.sk

Fixes #

Summary by CodeRabbit

• Bug Fixes

  • Virtual environment directories are now removed from disk when an environment is deleted.
  • Cancellation handling during environment cleanup is improved to avoid unexpected failures.
  • Deletion errors are logged and surfaced to the output without disrupting the overall flow.

• Tests

  • Added tests that verify on-disk cleanup and proper teardown of temporary storage.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

DeepnoteEnvironmentManager constructor now accepts an IFileSystem dependency (imported and publicly exposed). deleteEnvironment adds Cancellation.throwIfCanceled checks and attempts to remove the venv directory via fileSystem.delete(...). Disk errors are caught: success is logged, failures emit a warning and an output message but do not fail the overall delete flow. Tests were updated to inject a mock IFileSystem that performs real deletions, create a temporary global storage dir, verify on-disk venv removal, and clean up the temp directory in teardown.

Sequence Diagram

sequenceDiagram
    participant Caller as Caller
    participant Manager as DeepnoteEnvironmentManager\n(+ IFileSystem)
    participant FS as IFileSystem

    Caller->>Manager: deleteEnvironment(envId, token)

    Note over Manager: Pre-check cancellation
    Manager->>Manager: Cancellation.throwIfCanceled(token)

    Note over Manager: Existing cleanup (persistence/events)
    Manager->>Manager: persistence.delete(envId)
    Manager->>Manager: fire events

    Note over Manager: Post-cleanup cancellation check
    Manager->>Manager: Cancellation.throwIfCanceled(token)

    rect rgb(220,245,225)
        Note over Manager,FS: NEW — guarded disk deletion via injected IFileSystem
        Manager->>FS: delete(environmentDirectory)
        FS-->>Manager: success
        alt delete throws
            FS-->>Manager: error
            Manager->>Manager: log warning & write to outputChannel
        end
    end

    Note over Manager: Errors logged and swallowed (do not rethrow)

    Manager-->>Caller: complete

Loading

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding functionality to delete the Python virtual environment directory when removing a Deepnote environment.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b79d0ef and 60cab8d.

📒 Files selected for processing (2)

• src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (3 hunks)
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (2)

src/platform/common/constants.ts (1)

• STANDARD_OUTPUT_CHANNEL (32-32)

src/platform/logging/index.ts (1)

• logger (35-48)

🔇 Additional comments (7)

src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (4)

5-6: LGTM - Necessary imports for filesystem testing.

Imports support temporary directory management and IFileSystem mocking.

Also applies to: 12-12

---

22-23: Well-structured test setup.

Temporary directory isolation and mock configuration properly support integration-style verification of deletion behavior.

Also applies to: 36-36, 38-41, 44-51, 57-58

---

62-67: Proper cleanup in teardown.

Guards and recursive deletion ensure test artifacts are removed.

---

265-289: Excellent test coverage for deletion.

Properly verifies end-to-end directory removal with precondition and postcondition assertions.

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (3)

13-13: LGTM - IFileSystem import.

Enables dependency injection of filesystem operations.

---

34-35: Proper dependency injection.

Constructor parameters correctly use inversify decorators for IOutputChannel and IFileSystem.

---

204-217: Solid deletion implementation.

Proper cancellation checks surround the deletion attempt. Graceful error handling logs failures without blocking environment removal, which is appropriate for idempotency.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (9ee12ab) to head (60cab8d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #150   +/-   ##
===========================
===========================

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between dee6066 and aa8334c.

📒 Files selected for processing (2)

• src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (3 hunks)
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (4 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

**/*.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/kernels/deepnote/environments/deepnoteEnvironmentManager.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

Files:

• src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.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/deepnote/environments/deepnoteEnvironmentManager.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.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/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts

**/*.unit.test.ts

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

Place unit tests in files matching *.unit.test.ts

Files:

• src/kernels/deepnote/environments/deepnoteEnvironmentManager.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/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts

🧬 Code graph analysis (1)

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)

src/platform/logging/index.ts (1)

• logger (35-48)

🔇 Additional comments (1)

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)

35-36: IFileSystem injection added correctly.

The dependency injection follows coding guidelines by injecting the interface rather than a concrete implementation.

[Artmann]

🚀