Merged
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR systematically removes eslint exemptions by improving type safety across the telemetry system. The changes replace loose any types with more specific types like unknown, ITelemetryData, and proper type annotations.
Key Changes:
- Replaced
anywithunknownthroughout the telemetry system for better type safety - Updated test assertions from
inoperator checks to direct property access with truthiness checks - Added type guards and proper type conversions in telemetry data handling code
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
eslint.config.js |
Removed 17 files from the no-restricted-syntax rule exemption list for any and in operator |
src/vs/platform/telemetry/common/telemetry.ts |
Updated ITelemetryData index signature from any to string | unknown | undefined |
src/vs/platform/telemetry/common/telemetryUtils.ts |
Replaced any with unknown in function signatures and added type guards; updated cleanData to handle undefined data |
src/vs/platform/telemetry/common/telemetryService.ts |
Removed cast and added null coalescing when logging to appenders |
src/vs/platform/telemetry/common/telemetryLogAppender.ts |
Changed log method parameter type from any to unknown |
src/vs/platform/telemetry/common/telemetryIpc.ts |
Updated ITelemetryLog.data and method signatures to use ITelemetryData instead of any |
src/vs/platform/telemetry/common/gdprTypings.ts |
Changed ClassifiedEvent mapped type values from any to unknown |
src/vs/platform/telemetry/common/1dsAppender.ts |
Replaced any with unknown and added proper validated data handling |
src/vs/platform/telemetry/browser/1dsAppender.ts |
Updated constructor parameter type from any to unknown |
src/vs/platform/telemetry/node/1dsAppender.ts |
Updated constructor and response header types from any to appropriate types |
src/vs/platform/telemetry/node/errorTelemetry.ts |
Changed Promise rejection types from any to unknown |
src/vs/workbench/services/timer/browser/timerService.ts |
Added ITelemetryData import and double cast for metrics object |
src/vs/workbench/browser/parts/editor/editorGroupView.ts |
Changed return type of toEditorTelemetryDescriptor from object to ITelemetryData |
src/vs/workbench/api/common/extHostTelemetry.ts |
Added type definition for event data and added type guards for error properties |
src/vs/workbench/services/telemetry/test/node/commonProperties.test.ts |
Replaced 'property' in obj checks with direct property access in assertions |
src/vs/workbench/services/telemetry/test/browser/commonProperties.test.ts |
Replaced 'property' in obj checks with direct property access in assertions |
| from?: string; | ||
| target?: string; | ||
| [key: string]: any; | ||
| [key: string]: string | unknown | undefined; |
There was a problem hiding this comment.
The type definition string | unknown | undefined is redundant. Since unknown is the top type that includes all other types (including string and undefined), this union is equivalent to just unknown. Consider simplifying to:
[key: string]: unknown;
Suggested change
| [key: string]: string | unknown | undefined; | |
| [key: string]: unknown; |
| // Some telemetry modules prefer to break properties and measurmements up | ||
| // We mix common properties into the properties tab. | ||
| let updatedData = 'properties' in data ? (data.properties ?? {}) : data; | ||
| let updatedData = data.properties ? (data.properties ?? {}) : data; |
There was a problem hiding this comment.
The expression data.properties ? (data.properties ?? {}) is redundant. If data.properties is truthy, the nullish coalescing operator ?? {} will never use the fallback empty object since the value is already truthy. This should be simplified to:
let updatedData = data.properties ?? data;This correctly uses data.properties if it exists (even if it's an empty object), otherwise falls back to data.
Suggested change
| let updatedData = data.properties ? (data.properties ?? {}) : data; | |
| let updatedData = data.properties ?? data; |
| } | ||
| */ | ||
| this._telemetryService.publicLog('startupTimeVaried', metrics); | ||
| this._telemetryService.publicLog('startupTimeVaried', metrics as unknown as ITelemetryData); |
There was a problem hiding this comment.
The double type cast as unknown as ITelemetryData is a code smell that suggests type system issues. This bypasses type safety and should be avoided. Consider either:
- Ensuring that
IStartupMetricsproperly extends or is compatible withITelemetryData - Creating a proper conversion function that maps
IStartupMetricstoITelemetryData - If the GDPR annotation guarantees type safety, using
publicLog2with proper type parameters instead ofpublicLog
Suggested change
| this._telemetryService.publicLog('startupTimeVaried', metrics as unknown as ITelemetryData); | |
| type StartupTimeVariedClassification = { | |
| owner: 'jrieken'; | |
| // The GDPR annotation includes all properties from IStartupMetrics | |
| // so we use the include directive here. The actual classification is handled by the GDPR tooling. | |
| }; | |
| this._telemetryService.publicLog2<IStartupMetrics, StartupTimeVariedClassification>('startupTimeVaried', metrics); |
Tyriar
previously approved these changes
Nov 10, 2025
Tyriar
approved these changes
Nov 10, 2025
Kosta-Github
added a commit
to Kosta-Github/vscode
that referenced
this pull request
Nov 11, 2025
…nal-tooltips-markdown * 'main' of https://github.com/microsoft/vscode: (56 commits) edits: show diff for sensitive edit confirmations (microsoft#276620) Enable Back button on the Manage Accounts picker (microsoft#276622) Ignore obsolete chat content parts when loading persisted session (microsoft#276615) settings cleanup (microsoft#276602) Remove unused `args: any` parameter Terminal suggest - include persistent options in suggestions and improve suggestion grouping (microsoft#276409) fix selections not being added (microsoft#276600) Embed AI search into the existing search view message (microsoft#276586) Cleanup some eslint exemptions (microsoft#276581) fix microsoft#276579 (microsoft#276590) SCM - cleanup some more eslint rules (microsoft#276571) Bump gpu types and skip lib check for gpu typing issue Fix in smoke tests Remove `forChatSessionTypeAndId` Fix one more import detect `press any/a key` and ask if user wants to send `a` to terminal (microsoft#276554) Filter subagent and todo tools from subagent requests (microsoft#276553) Expand hover setting to allow for key modifier mode (microsoft#274001) Allow partial monacoEnvironment.getWorker/getWorkerUrl Update imports ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.