feat(ide): add telemetry tracking to VSCode extension#2505
Merged
Conversation
Migrate Mixpanel-based telemetry from v2 to track extension usage events while respecting VSCode's telemetry settings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review - VSCode Telemetry ImplementationTodo List
Code Review SummaryI've thoroughly reviewed the telemetry implementation. Overall, this is a well-implemented feature that properly respects user privacy settings. Here's my detailed analysis: ✅ StrengthsPrivacy & Security
Code Quality
Performance
|
Contributor
📝 WalkthroughWalkthroughAdds Mixpanel telemetry to the VS Code extension: new telemetry module and machine-id utility, activation tracking, post-build token injection, package.json dependency and build script updates, ESLint rule change, and CI env var for the telemetry token. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/ide/vscode/src/extension/vscode-telemetry.ts (1)
62-62: Singleton initialization triggers synchronous system commands at module load.The default export instantiates
VSCodeTelemetryimmediately when the module is imported, which synchronously callsgetMachineId()(usingexecSync). This happens during extension activation before the language client starts.Consider lazy initialization to defer the machine ID computation until the first
track()call, avoiding potential startup delay.♻️ Proposed lazy initialization pattern
export class VSCodeTelemetry { private readonly mixpanel: Mixpanel | undefined; - private readonly deviceId = this.getDeviceId(); + private _deviceId: string | undefined; // ... other fields ... + private get deviceId(): string { + if (!this._deviceId) { + this._deviceId = this.getDeviceId(); + } + return this._deviceId; + } + // ... rest of class ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/vscode-telemetry.ts` at line 62, The module currently exports a pre-instantiated singleton (export default new VSCodeTelemetry()) which runs getMachineId() synchronously at import time; change to lazy initialization so VSCodeTelemetry (and its execSync getMachineId call) is only constructed on first use: replace the eager export with a lazily-created singleton accessor (e.g., a getTelemetry() / getInstance() function or a default exported proxy object) that creates new VSCodeTelemetry() the first time track() is invoked, ensuring VSCodeTelemetry's constructor or its getMachineId() call is deferred until needed; keep the class name VSCodeTelemetry and the track() method signature so callers don't have to change.packages/ide/vscode/src/extension/machine-id-utils.ts (1)
66-69: Consider adding a timeout toexecSync.The
execSynccall could theoretically hang if the system command stalls, blocking the extension host during activation. A timeout ensures graceful fallback to UUID.⏱️ Proposed fix with timeout
try { - const value = execSync(guid[platform as keyof typeof guid]); + const value = execSync(guid[platform as keyof typeof guid], { timeout: 5000 }); const id = expose(value.toString()); return hash(id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/machine-id-utils.ts` around lines 66 - 69, The execSync call that runs guid[platform] can hang; modify the block in machine-id-utils.ts where execSync is used (the call referencing execSync, guid, expose and hash) to pass an options object with a reasonable timeout (e.g. 2000–5000 ms) and encoding if needed, and keep the existing try/catch fallback to generate a UUID if the command times out or throws; ensure the thrown error from execSync is caught and handled the same way the code currently handles other failures so expose(id) and hash(id) are only executed on a successful, timely execSync result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ide/vscode/src/extension/machine-id-utils.ts`:
- Around line 33-60: The expose function currently uses non-null assertions
after split() calls (in function expose) which can produce runtime undefined
access; replace those unsafe asserts with explicit checks: after each split
(e.g., .split('IOPlatformUUID'), .split('REG_SZ')), verify the resulting array
has the expected index and the value is a non-empty string before continuing,
and if parsing fails throw a descriptive Error so the caller (getMachineId) can
catch and fall back; ensure the win32/darwin parsing branches validate
intermediate values prior to calling replace()/toLowerCase(), and apply the same
defensive check for linux/freebsd branches if toString() output can be empty.
In `@packages/ide/vscode/src/extension/main.ts`:
- Line 5: Telemetry currently only records extension:activate; add tracking for
the other two TelemetryEvents by calling
telemetry.trackEvent('extension:zmodel-preview') inside the preview/preview
command handler (the command registration or function that opens the zmodel
preview) and telemetry.trackEvent('extension:zmodel-save') inside the document
save listener (the onDidSaveTextDocument handler or the function that processes
saves for zmodel documents). Locate the preview command registration and the
save listener in main.ts (or their handler functions) and insert the
telemetry.trackEvent calls so the TelemetryEvents type values
extension:zmodel-preview and extension:zmodel-save are emitted when those
actions occur; if those handlers do not exist, implement the preview command or
save hook and invoke telemetry.trackEvent there or remove the unused
TelemetryEvents definitions.
---
Nitpick comments:
In `@packages/ide/vscode/src/extension/machine-id-utils.ts`:
- Around line 66-69: The execSync call that runs guid[platform] can hang; modify
the block in machine-id-utils.ts where execSync is used (the call referencing
execSync, guid, expose and hash) to pass an options object with a reasonable
timeout (e.g. 2000–5000 ms) and encoding if needed, and keep the existing
try/catch fallback to generate a UUID if the command times out or throws; ensure
the thrown error from execSync is caught and handled the same way the code
currently handles other failures so expose(id) and hash(id) are only executed on
a successful, timely execSync result.
In `@packages/ide/vscode/src/extension/vscode-telemetry.ts`:
- Line 62: The module currently exports a pre-instantiated singleton (export
default new VSCodeTelemetry()) which runs getMachineId() synchronously at import
time; change to lazy initialization so VSCodeTelemetry (and its execSync
getMachineId call) is only constructed on first use: replace the eager export
with a lazily-created singleton accessor (e.g., a getTelemetry() / getInstance()
function or a default exported proxy object) that creates new VSCodeTelemetry()
the first time track() is invoked, ensuring VSCodeTelemetry's constructor or its
getMachineId() call is deferred until needed; keep the class name
VSCodeTelemetry and the track() method signature so callers don't have to
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6964b798-dc45-41ee-a93b-69f940640a32
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/ide/vscode/package.jsonpackages/ide/vscode/src/extension/machine-id-utils.tspackages/ide/vscode/src/extension/main.tspackages/ide/vscode/src/extension/vscode-telemetry.ts
Add a post-build step that replaces the telemetry token placeholder in the bundled output using environment variables from .env files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review - VSCode Telemetry ImplementationTodo List
Code Review SummaryI've thoroughly reviewed the telemetry implementation. Overall, this is a well-implemented feature that properly respects user privacy settings. Here's my detailed analysis: ✅ StrengthsPrivacy & Security
Code Quality
Performance
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ide/vscode/scripts/post-build.ts`:
- Around line 13-15: The script currently reads file into the variable content
and calls content.replace('<VSCODE_TELEMETRY_TRACKING_TOKEN>', telemetryToken)
which may do nothing (placeholder missing) or only replace the first occurrence;
update post-build.ts to perform a global replacement (e.g., replaceAll or a
global regex) and then verify the replacement actually occurred by comparing the
original and new content or by checking for presence of
'<VSCODE_TELEMETRY_TRACKING_TOKEN>' after replacement; if any placeholder
remains or no replacement happened, throw an error / exit non-zero so the build
fails rather than silently publishing an unresolved token (refer to
variables/content, telemetryToken and the write step to locate where to add the
checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86f65f38-d2a0-4a4a-8e0b-d62e175a04b9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/ide/vscode/package.jsonpackages/ide/vscode/scripts/post-build.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ide/vscode/package.json
Contributor
|
Claude finished @ymc9's task —— View job Code Review - VSCode Telemetry ImplementationTodo List
Code Review SummaryI've thoroughly reviewed the telemetry implementation. Overall, this is a well-implemented feature that properly respects user privacy settings. Here's my detailed analysis: ✅ StrengthsPrivacy & Security
Code Quality
Performance
|
Contributor
|
Claude finished @ymc9's task —— View job Code Review - VSCode Telemetry ImplementationTodo List
Code Review SummaryI've thoroughly reviewed the telemetry implementation. Overall, this is a well-implemented feature that properly respects user privacy settings. Here's my detailed analysis: ✅ StrengthsPrivacy & Security
Code Quality
Performance
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review - VSCode Telemetry ImplementationTodo List
Code Review SummaryI've thoroughly reviewed the telemetry implementation. Overall, this is a well-implemented feature that properly respects user privacy settings. Here's my detailed analysis: ✅ StrengthsPrivacy & Security
Code Quality
Performance
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ide/vscode/eslint.config.mjs (1)
7-9: Replace globalno-prototype-builtinsdisable with targeted fix.Line 8 disables a safety rule globally for the entire VSCode package. There is only one call site using this pattern that can be fixed directly:
Location:
packages/ide/vscode/src/extension/machine-id-utils.ts:23Current:
process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')Fix:
Object.prototype.hasOwnProperty.call(process.env, 'PROCESSOR_ARCHITEW6432')Remove the rule disable from
eslint.config.mjs(lines 7-9) and apply this targeted fix to avoid normalizing unsafe property check patterns across the package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/eslint.config.mjs` around lines 7 - 9, Remove the global rule disable in eslint.config.mjs for no-prototype-builtins and instead update the single unsafe call in packages/ide/vscode/src/extension/machine-id-utils.ts (currently using process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')) to use the safe form via Object.prototype.hasOwnProperty.call against process.env; this keeps the rule enabled project-wide and fixes the single call site without weakening linting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ide/vscode/eslint.config.mjs`:
- Around line 7-9: Remove the global rule disable in eslint.config.mjs for
no-prototype-builtins and instead update the single unsafe call in
packages/ide/vscode/src/extension/machine-id-utils.ts (currently using
process.env.hasOwnProperty('PROCESSOR_ARCHITEW6432')) to use the safe form via
Object.prototype.hasOwnProperty.call against process.env; this keeps the rule
enabled project-wide and fixes the single call site without weakening linting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13bc620f-f274-4af9-b3f1-5c17b161b275
📒 Files selected for processing (1)
packages/ide/vscode/eslint.config.mjs
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
extension:activate,extension:zmodel-preview, andextension:zmodel-saveeventsisTelemetryEnabledsetting — no data sent when telemetry is disabledTest plan
pnpm buildinpackages/ide/vscode)vscode.env.isTelemetryEnabledis true<VSCODE_TELEMETRY_TRACKING_TOKEN>with actual Mixpanel token before publishing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Style