Skip to content

Conversation

@Artmann
Copy link
Member

@Artmann Artmann commented Nov 12, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Telemetry is now disabled by default and will not be emitted regardless of prior workspace settings.
  • Tests

    • Unit tests updated to remove or relax telemetry assertions; telemetry verification is disabled and tests now ensure no errors occur when telemetry is off.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

📝 Walkthrough

Walkthrough

isTelemetryDisabled() was changed to always return true, so telemetry is treated as disabled regardless of workspace or global settings. Tests and assertions that previously verified telemetry events or hashed properties were removed or simplified to no-ops or checks that sending telemetry does not throw. Several unit tests and helper functions that validated telemetry hashes or event emission were trimmed or updated to stop asserting telemetry behavior.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant App as Application
    participant Telemetry as Telemetry Module
    participant Sink as Telemetry Sink

    Note over Telemetry: Old flow (before change)
    App->>Telemetry: request to send event
    Telemetry->>Telemetry: isTelemetryDisabled() reads workspace/global settings
    alt telemetry enabled
        Telemetry->>Sink: send event
        Sink-->>Telemetry: ack
        Telemetry-->>App: success
    else telemetry disabled
        Telemetry-->>App: noop / skipped
    end
Loading
sequenceDiagram
    autonumber
    participant App as Application
    participant Telemetry as Telemetry Module
    participant Sink as Telemetry Sink

    Note over Telemetry: New flow (after change)
    App->>Telemetry: request to send event
    Telemetry->>Telemetry: isTelemetryDisabled() -> true
    Telemetry-->>App: noop / skipped
    Note over Sink: Sink is never invoked
Loading

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 'Disable all telemetry' directly and accurately describes the main objective of the changeset—disabling telemetry across multiple files.

📜 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 614985a and 058895c.

📒 Files selected for processing (3)
  • src/interactive-window/shiftEnterBanner.unit.test.ts (3 hunks)
  • src/platform/common/experiments/telemetry.unit.test.ts (2 hunks)
  • src/platform/telemetry/index.unit.test.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/telemetry/index.unit.test.ts
  • src/platform/common/experiments/telemetry.unit.test.ts
src/platform/telemetry/**/*.ts

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

Send telemetry via platform telemetry utilities and use helpers.ts for safe hashing/anonymization

Files:

  • src/platform/telemetry/index.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/telemetry/index.unit.test.ts
  • src/platform/common/experiments/telemetry.unit.test.ts
  • src/interactive-window/shiftEnterBanner.unit.test.ts
src/interactive-window/**/*.ts

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

src/interactive-window/**/*.ts: Consider platform differences (Desktop/Node vs Web) for features and fallbacks when implementing functionality
Use dependency injection to keep components testable
Respect CancellationToken in async operations and support cancellation in long-running tasks
Use EventEmitter for event-driven updates and ensure proper disposal/cleanup of listeners to prevent leaks
Monitor and track promises/async operations to ensure proper cleanup and avoid dangling work on dispose
Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage

Files:

  • src/interactive-window/shiftEnterBanner.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). (1)
  • GitHub Check: Build & Test
🔇 Additional comments (5)
src/platform/telemetry/index.unit.test.ts (1)

7-13: Import simplification aligns with test removal.

The removal of ts-mockito and WorkspaceConfiguration-related imports is consistent with removing isTelemetryDisabled() tests.

src/interactive-window/shiftEnterBanner.unit.test.ts (2)

13-13: LGTM: Unused import removed.

Telemetry symbol correctly removed from imports since telemetry verification is no longer performed in this test file.


63-89: Telemetry verification appropriately removed.

The comments clearly document where telemetry assertions were removed. The test at line 72 that verifies Reporter.eventNames is empty remains appropriate—it confirms no events fire when the banner is disabled.

src/platform/common/experiments/telemetry.unit.test.ts (2)

17-19: LGTM: Stub appropriately simplified.

With telemetry disabled, the no-op stub is sufficient. The comment clearly documents the change.


33-38: Test appropriately simplified to defensive check.

Verifying postEvent doesn't throw is a sensible defensive test even when telemetry is disabled—ensures the API remains stable.


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

@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73%. Comparing base (4b6ec72) to head (058895c).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #202   +/-   ##
=====================================
- Coverage     73%     73%   -1%     
=====================================
  Files        577     577           
  Lines      47663   47608   -55     
  Branches    5597    5591    -6     
=====================================
- Hits       34939   34856   -83     
- Misses     10887   10922   +35     
+ Partials    1837    1830    -7     
Files with missing lines Coverage Δ
...c/interactive-window/shiftEnterBanner.unit.test.ts 100% <ø> (ø)
...platform/common/experiments/telemetry.unit.test.ts 100% <100%> (ø)
src/platform/telemetry/index.ts 79% <100%> (-1%) ⬇️
src/platform/telemetry/index.unit.test.ts 100% <100%> (ø)
...tandalone/import-export/importTracker.unit.test.ts 94% <100%> (+<1%) ⬆️

... 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 Nov 12, 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

Caution

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

⚠️ Outside diff range comments (4)
src/platform/common/experiments/telemetry.unit.test.ts (1)

19-25: Consider removing unused stub.

The sendTelemetryEventStub is set up but never called since telemetry is disabled. Consider removing this setup to simplify the test.

-    sendTelemetryEventStub = sinon
-        .stub(Telemetry, 'sendTelemetryEvent')
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        .callsFake((eventName: string, _, properties: any) => {
-            const telemetry = { eventName, properties };
-            telemetryEvents.push(telemetry);
-        });
src/interactive-window/shiftEnterBanner.unit.test.ts (2)

14-14: Remove unused import.

Pipeline correctly flags Telemetry as unused. Remove it.

 import {
     isTestExecution,
     isUnitTestExecution,
     setTestExecution,
-    setUnitTestExecution,
-    Telemetry
+    setUnitTestExecution
 } from '../platform/common/constants';

31-43: Consider removing unused Reporter stub.

Since telemetry is disabled, the Reporter class and stub setup (lines 38-43) are no longer exercised. Consider cleanup unless needed for future re-enablement.

src/platform/telemetry/index.unit.test.ts (1)

82-93: Tests verify telemetry sends despite permanent disable.

The comment at line 82 correctly notes telemetry is now permanently disabled—isTelemetryDisabled() returns true. However, tests at lines 84+ still assert telemetry events are recorded. These tests mock reporter.sendTelemetryEvent() directly, bypassing the disable check in sendTelemetryEvent() (line 133: if (isTelemetryDisabled()) return;).

Update tests to verify sendTelemetryEvent() is a no-op when telemetry is disabled, or clarify why telemetry should remain functional in tests.

📜 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 2afbc42 and 5fcb76c.

📒 Files selected for processing (4)
  • src/interactive-window/shiftEnterBanner.unit.test.ts (2 hunks)
  • src/platform/common/experiments/telemetry.unit.test.ts (1 hunks)
  • src/platform/telemetry/index.unit.test.ts (2 hunks)
  • src/standalone/import-export/importTracker.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/telemetry/index.unit.test.ts
  • src/platform/common/experiments/telemetry.unit.test.ts
src/platform/telemetry/**/*.ts

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

Send telemetry via platform telemetry utilities and use helpers.ts for safe hashing/anonymization

Files:

  • src/platform/telemetry/index.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/telemetry/index.unit.test.ts
  • src/standalone/import-export/importTracker.unit.test.ts
  • src/interactive-window/shiftEnterBanner.unit.test.ts
  • src/platform/common/experiments/telemetry.unit.test.ts
src/interactive-window/**/*.ts

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

src/interactive-window/**/*.ts: Consider platform differences (Desktop/Node vs Web) for features and fallbacks when implementing functionality
Use dependency injection to keep components testable
Respect CancellationToken in async operations and support cancellation in long-running tasks
Use EventEmitter for event-driven updates and ensure proper disposal/cleanup of listeners to prevent leaks
Monitor and track promises/async operations to ensure proper cleanup and avoid dangling work on dispose
Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage

Files:

  • src/interactive-window/shiftEnterBanner.unit.test.ts
🧬 Code graph analysis (1)
src/standalone/import-export/importTracker.unit.test.ts (1)
src/telemetry.ts (1)
  • ResourceTypeTelemetryProperty (32-38)
🪛 GitHub Actions: CD
src/interactive-window/shiftEnterBanner.unit.test.ts

[error] 14-14: TS6133: 'Telemetry' is declared but its value is never read.

🪛 GitHub Actions: CI
src/interactive-window/shiftEnterBanner.unit.test.ts

[error] 14-14: TS6133: 'Telemetry' is declared but its value is never read.

🪛 GitHub Check: Lint & Format
src/standalone/import-export/importTracker.unit.test.ts

[warning] 56-56:
'hashes' is defined but never used. Allowed unused args must match /_\w*/u


[warning] 55-55:
'resourceType' is assigned a value but never used


[warning] 54-54:
'when' is assigned a value but never used

🪛 GitHub Check: TypeCheck
src/standalone/import-export/importTracker.unit.test.ts

[failure] 56-56:
'hashes' is declared but its value is never read.


[failure] 55-55:
'resourceType' is declared but its value is never read.


[failure] 54-54:
'when' is declared but its value is never read.

🔇 Additional comments (2)
src/platform/common/experiments/telemetry.unit.test.ts (1)

40-45: Test updated appropriately.

The doesNotThrow pattern correctly verifies the method handles disabled telemetry gracefully.

src/interactive-window/shiftEnterBanner.unit.test.ts (1)

64-64: Comments appropriately document removed assertions.

Also applies to: 89-89

coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 12, 2025
@Artmann Artmann marked this pull request as ready for review November 12, 2025 15:04
@Artmann Artmann requested a review from a team as a code owner November 12, 2025 15:04
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.

thanks!!

@Artmann Artmann merged commit 26ad568 into main Nov 12, 2025
12 of 13 checks passed
@Artmann Artmann deleted the chris/disable-telemetry branch November 12, 2025 15:29
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