Use native metered connection monitor - #331897
Use native metered connection monitor#331897Dmitriy Vasyura (dmitrivMS) wants to merge 12 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
Pull request overview
Uses native OS metered-connection detection on desktop while retaining browser detection for vscode.dev.
Changes:
- Adds
@vscode/meteredmonitoring and IPC propagation. - Refactors browser and Electron metered services.
- Adds packaging configuration and regression tests.
Show a summary per file
| File | Description |
|---|---|
src/vs/platform/meteredConnection/test/electron-main/meteredConnectionMainService.test.ts |
Tests native monitoring lifecycle and updates. |
src/vs/platform/meteredConnection/test/electron-browser/meteredConnectionService.test.ts |
Tests IPC state synchronization. |
src/vs/platform/meteredConnection/electron-main/meteredConnectionMainService.ts |
Implements native OS monitoring. |
src/vs/platform/meteredConnection/electron-main/meteredConnectionChannel.ts |
Removes renderer-to-main state updates. |
src/vs/platform/meteredConnection/electron-browser/meteredConnectionService.ts |
Uses the main-process IPC state. |
src/vs/platform/meteredConnection/common/meteredConnectionIpc.ts |
Updates IPC commands and synchronization. |
src/vs/platform/meteredConnection/common/meteredConnection.ts |
Generalizes underlying connection state. |
src/vs/platform/meteredConnection/browser/meteredConnectionService.ts |
Scopes browser detection to web. |
src/vs/platform/environment/test/node/nativeModules.integrationTest.ts |
Verifies native module loading. |
src/vs/code/electron-main/app.ts |
Wires the native main service. |
package.json |
Adds and allowlists the dependency. |
package-lock.json |
Locks native module dependencies. |
eslint.config.js |
Allowlists the module import. |
build/.moduleignore |
Packages the native binary. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 13/14 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Mohammad javad Dianat (dianatofficial)
left a comment
There was a problem hiding this comment.
Verified the diff. Changes align with project standards.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Base:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Sysrooted Linux linking is broken, prerequisites are missing from other Linux build paths, and the proposed API documentation needs initialization qualification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
package.json — Adding this native addon currently breaks Linux dependency installation under VS Code's sysrooted… |
|
package.json — The Linux build dependency for this native package is only added to three workflows. Other… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
src/vs/code/electron-main/app.ts — This wires telemetry only into the metered service; the main-process TelemetryService config at… View comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical buffering flaw can transmit telemetry generated while the connection was metered.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/vs/platform/telemetry/common/telemetryService.ts — Buffered events are classified only from the connection state at flush time. If the native monitor… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
package.json — The Linux build dependency for this native package is only added to three workflows. Other… View resolved comment |
|
package.json — Adding this native addon currently breaks Linux dependency installation under VS Code's sysrooted… View resolved comment |
|
src/vs/code/electron-main/app.ts — This wires telemetry only into the metered service; the main-process TelemetryService config at… View resolved comment |
| // Buffer events until experiment properties and the initial metered connection state are available. | ||
| if (!this._isExperimentPropertySet || !this._isMeteredConnectionInitialized) { | ||
| if (this._pendingEvents.length < TelemetryService.MAX_BUFFER_SIZE) { | ||
| this._pendingEvents.push({ eventName, eventLevel, data }); | ||
| } |
|
|
||
| // Send all buffered events now that experiment properties are available | ||
| for (const event of this._pendingEvents) { | ||
| this._doLog(event.eventName, event.eventLevel, event.data); |
There was a problem hiding this comment.
AI Review: The new readiness buffer also affects telemetry services without experiment buffering, including the shared process. If telemetry.telemetryLevel changes from all to off (or error) while native initialization is pending, queued events are later replayed through _doLog, bypassing _log's current-level check. A source-based check reproduced usage and error events being appended after switching to off, and usage after switching to error. Please filter buffered events against the current telemetry level before replay and add coverage for a consent downgrade during initialization.
| public readonly onDidChangeIsConnectionMetered = this._onDidChangeIsConnectionMetered.event; | ||
|
|
||
| private _isConnectionMetered = false; | ||
| private _isConnectionMetered = true; |
There was a problem hiding this comment.
AI Review: This conservative pending value also reaches explicit preference changes. With extensions.autoUpdate='on', Disable Auto Update for Extensions remains available, but updateAutoUpdateForAllExtensions(false) calls the network-gated isAutoUpdateEnabled(), gets false, and returns before confirmation or changing the setting. On an unmetered startup with pending native initialization, the user's request is silently discarded and auto-updates remain enabled afterward. A source-based check reproduced this with the pending IPC client. Compare getAutoUpdateValue() !== 'off' when deciding whether an explicit preference change is redundant, and add a pending-initialization regression test. Keep the conservative network state for automatic work.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>


@vscode/meteredin the Electron main process for native metered-connection detectionFixes #330586
Fixes #321334