Skip to content

Conversation

@Artmann
Copy link
Member

@Artmann Artmann commented Oct 27, 2025

Summary by CodeRabbit

  • Chores

    • Removed several test dependencies from non-conda and virtualenv configurations (e.g., matplotlib, ipympl, ipykernel).
    • Relaxed/updated test requirement pins and added a security-focused block of minimum versions for multiple tooling dependencies.
    • Updated project dependencies: upgraded glob, removed styled-components, and added bare-events.
  • Refactor

    • Simplified test discovery and asynchronous file-globbing to streamline test execution.
  • Tests

    • Added unit tests covering filesystem globbing behavior and conda-related discovery logic.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

📝 Walkthrough

Walkthrough

Test requirement files were edited: several dependencies removed from non-conda and venv test requirement files (matplotlib, ipympl, ipykernel, multiple ipywidgets7 entries) and ipywidgets8 requirements updated (numpy relaxed to >=1.22.2 and a transitive-security block of minimum versions added). package.json updated: glob -> ^9.3.5, styled-components removed, bare-events added (deps and devDeps). Source code replaced callback/promisified glob usage with direct awaitable glob calls or named imports ({ glob } / { globSync }), removed promisify usage, and moved test file discovery to occur before Mocha registration/run in testRunner. No public API signatures changed.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as Function (old)
    participant GlobCB as glob(pattern, callback)
    participant Callback as Callback
    participant Continue as Continue Flow

    Note over GlobCB,Callback #E3F2FD: Old flow (callback-based / promisified)
    Caller->>GlobCB: glob(pattern, callback)
    GlobCB->>Callback: invoke callback(err, files)
    alt err
        Callback->>Caller: reject / error handling
    else success
        Callback->>Caller: resolve(files)
        Caller->>Continue: process files
    end
Loading
sequenceDiagram
    autonumber
    participant Caller as Function (new)
    participant GlobPromise as glob(pattern)
    participant Continue as Continue Flow

    Note over GlobPromise,Continue #E8F5E9: New flow (await-based / direct promise)
    Caller->>GlobPromise: await glob(pattern)
    GlobPromise-->>Caller: returns files or throws
    alt throws
        Caller->>Caller: catch error
    else success
        Caller->>Continue: process files
    end
Loading
sequenceDiagram
    autonumber
    participant FileSystem as fileSystem.node
    participant Glob as { glob } / { globSync }

    Note over FileSystem,Glob #FFF3E0: Import/API style change: named imports used, promisify removed
    FileSystem->>Glob: call glob(pattern, options) or globSync(pattern, options)
    Glob-->>FileSystem: returns files
Loading
sequenceDiagram
    autonumber
    participant TestRunner as testRunner
    participant Glob as glob(pattern)
    participant Mocha as Mocha
    participant Jupyter as JupyterServer

    Note over TestRunner,Glob #FFF3E0: testRunner now collects files before registering/running
    TestRunner->>Glob: await glob(testPattern, ignore)
    Glob-->>TestRunner: files[]
    TestRunner->>Mocha: register files[]
    TestRunner->>Jupyter: start server
    TestRunner->>Mocha: run tests
    alt failures
        Mocha-->>TestRunner: nonzero exit
    else success
        Mocha-->>TestRunner: success
    end
    TestRunner->>Jupyter: stop server
Loading

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% 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 accurately describes the dependency upgrade aspect to address vulnerabilities, which is present in the changeset (updated glob in package.json, numpy and transitive security updates in venv-test-ipywidgets8-requirements.txt). However, the title overlooks the extensive refactoring of glob usage patterns across multiple files (telemetryGenerator, test utilities, fileSystem, condaService), which comprises a substantial portion of the changes. The title is partially related—it captures a real aspect of the work but doesn't reflect the full scope of refactoring involved.

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

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

❌ Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72%. Comparing base (fd0882a) to head (81e6725).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/platform/common/platform/fileSystem.node.ts 50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #125   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        536     537    +1     
  Lines      40837   40889   +52     
  Branches    4990    4991    +1     
=====================================
+ Hits       29596   29650   +54     
+ Misses      9578    9572    -6     
- Partials    1663    1667    +4     
Files with missing lines Coverage Δ
...tform/common/platform/fileSystem.node.unit.test.ts 100% <100%> (ø)
src/platform/common/platform/fileSystem.node.ts 30% <50%> (+2%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 27, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 27, 2025
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 864f275 and be12c96.

📒 Files selected for processing (3)
  • src/platform/common/platform/fileSystem.node.ts (2 hunks)
  • src/test/interpreters/condaService.node.ts (2 hunks)
  • src/test/package.nls.json.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/!(*.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/test/package.nls.json.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/test/package.nls.json.unit.test.ts
  • src/test/interpreters/condaService.node.ts
  • src/platform/common/platform/fileSystem.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/test/package.nls.json.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/test/package.nls.json.unit.test.ts
**/*.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/test/interpreters/condaService.node.ts
  • src/platform/common/platform/fileSystem.node.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/common/platform/fileSystem.node.ts
src/platform/**/*.node.ts

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

src/platform/**/*.node.ts: Provide Node.js-specific implementations in .node.ts files (e.g., FileSystem with fs-extra, PlatformService with OS detection)
Desktop implementations may use Node capabilities (fs-extra, process spawning, native modules) consistent with their responsibilities

Files:

  • src/platform/common/platform/fileSystem.node.ts
src/platform/**/{fileSystem.ts,fileSystem.node.ts,platformService.web.ts,platformService.node.ts}

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

Use URI-based file operations and vscode-path for cross-platform path handling in file system and platform service implementations

Files:

  • src/platform/common/platform/fileSystem.node.ts
src/platform/common/platform/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

Expose file system operations through IFileSystem/IFileSystemNode and platform data via IPlatformService instead of direct APIs

Files:

  • src/platform/common/platform/fileSystem.node.ts
🧬 Code graph analysis (1)
src/test/package.nls.json.unit.test.ts (1)
src/test/constants.node.ts (1)
  • EXTENSION_ROOT_DIR_FOR_TESTS (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). (3)
  • GitHub Check: Build & Package Extension
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Test
🔇 Additional comments (7)
src/test/interpreters/condaService.node.ts (2)

4-4: LGTM: Named import correct for glob v9.

Migration from default to named import aligns with glob v9 API.


67-67: LGTM: Direct promise usage correct.

Native promises in glob v9 eliminate need for promisify. Error handling preserved.

src/test/package.nls.json.unit.test.ts (2)

8-8: LGTM: globSync import correct.

Proper named import for synchronous glob operations in v9.


15-21: LGTM: globSync usage correct.

Properly removed sync: true option since globSync is explicitly synchronous in v9.

src/platform/common/platform/fileSystem.node.ts (3)

6-6: LGTM: Named import correct.

Consistent glob v9 API usage across codebase.


21-23: LGTM: Direct promise return correct.

Glob v9 returns promises natively. Removing promisify wrapper simplifies code.


6-6: No action required—glob v9.3.5 is secure.

Verification confirms glob v9.3.5 contains no known vulnerabilities and is patched. No blocking security issues found.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 27, 2025
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 be12c96 and 81e6725.

📒 Files selected for processing (2)
  • src/platform/common/platform/fileSystem.node.unit.test.ts (1 hunks)
  • src/test/interpreters/condaService.node.unit.test.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 .ts files (not suffixed with .node or .web)

Files:

  • src/platform/common/platform/fileSystem.node.unit.test.ts
  • src/test/interpreters/condaService.node.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/platform/common/platform/fileSystem.node.unit.test.ts
  • src/test/interpreters/condaService.node.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/platform/common/platform/fileSystem.node.unit.test.ts
  • src/test/interpreters/condaService.node.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/common/platform/fileSystem.node.unit.test.ts
src/platform/common/platform/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

Expose file system operations through IFileSystem/IFileSystemNode and platform data via IPlatformService instead of direct APIs

Files:

  • src/platform/common/platform/fileSystem.node.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/platform/common/platform/fileSystem.node.unit.test.ts
  • src/test/interpreters/condaService.node.unit.test.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 & Test
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Package Extension
🔇 Additional comments (2)
src/test/interpreters/condaService.node.unit.test.ts (1)

38-91: Test structure and assertions look solid.

Tests properly isolate state with temp directories, restore environment variables, and exercise glob patterns comprehensively (basic, wildcards, brace expansion, no-match, error).

src/platform/common/platform/fileSystem.node.unit.test.ts (1)

23-91: Test coverage is comprehensive and well-designed.

Tests validate basic patterns, recursive globs, hidden file filtering, no-match cases, and multi-extension patterns. Assertions correctly verify results. Temp directory isolation ensures test independence.

@Artmann Artmann marked this pull request as ready for review October 27, 2025 18:29
@Artmann Artmann requested a review from a team as a code owner October 27, 2025 18:29
@Artmann Artmann enabled auto-merge (squash) October 27, 2025 18:29
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

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

working well, thanks!

@Artmann Artmann merged commit 08a3719 into main Oct 28, 2025
13 checks passed
@Artmann Artmann deleted the chris/update-vurnable-packages branch October 28, 2025 07:42
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