Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesTelemetry is aggregated into task-completion installments with circuit breaking, pending-capture draining, and model-cache throttling. Consent handling now combines stored preferences with VS Code’s live telemetry state, while tool analytics use validated names and explicit invalid-tool bucketing. Telemetry and task analytics
Consent, model cache, and retry validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant TelemetryService
participant TelemetryClient
Task->>Task: accumulate message and tool deltas
Task->>TelemetryService: flush TASK_COMPLETED installment
TelemetryService->>TelemetryClient: capture task-completion properties
Task->>TelemetryService: flush shutdown installment
TelemetryService->>TelemetryClient: drain captures and shut down
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
edelauna
commented
Jul 12, 2026
|
|
||
| expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() | ||
| expect(mockTask.didEditFile).toBe(true) | ||
| expect(mockTask.recordToolUsage).toHaveBeenCalledWith("edit_file") |
Contributor
Author
There was a problem hiding this comment.
removed since we're no longer emitting per message, but rather as a summary once the task completes
… providers + privacy doc fix
edelauna
requested review from
JamesRobert20,
hannesrudolph,
navedmerchant and
taltas
as code owners
![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: 4
🧹 Nitpick comments (2)
src/__tests__/extension.spec.ts (1)
324-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear
onDidChangeTelemetryEnabledmock between tests to avoid stale handler references.
vi.resetModules()resets the module cache but does not clearvi.fn()call history. SinceonDidChangeTelemetryEnabledis created once in the hoistedvi.mock("vscode", …)factory, its call history accumulates across tests. Tests at lines 357, 378, and 404 all retrievemock.calls[0][0], which is always the handler registered by the first test — not the current test's ownactivatecall. The tests pass today because the handler logic is identical and mock instances are shared, but this is fragile: removing or reordering the first test would silently break the others.♻️ Proposed fix
beforeEach(async () => { vi.resetModules() const vscode = await import("vscode") ;(vscode.env as any).isTelemetryEnabled = true + vi.mocked(vscode.env.onDidChangeTelemetryEnabled).mockClear() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/extension.spec.ts` around lines 324 - 328, Update the beforeEach setup in the extension tests to clear the shared onDidChangeTelemetryEnabled mock, including its call history, after resetting modules and before each test runs. Ensure tests retrieving mock.calls[0][0] observe the handler registered by the current activate call rather than a prior test.src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts (1)
219-241: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend malicious-name coverage to the "mcp_" prefix case.
The chosen
maliciousNamedoesn't start with"mcp_", so it doesn't exercise theisValidToolNameprefix-heuristic bypass (see the corresponding comment inpresentAssistantMessage.tsLines 604-620). Consider adding a case wherenameis something like"mcp_'; DROP TABLE users; --"to lock in the expected fix once addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts` around lines 219 - 241, Extend the test in “does not record raw model-supplied tool names in tool usage analytics” to use a malicious tool name beginning with “mcp_”, such as the existing injection payload with that prefix. Keep the assertions covering both recordToolUsage and recordToolError so the isValidToolName prefix-bypass path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/telemetry/src/TelemetryService.ts`:
- Around line 373-377: Update the pending-call drain in TelemetryService
shutdown to repeatedly await pendingClientCalls until the set is empty, ensuring
calls added during an in-flight drain are also completed before invoking
client.shutdown(). Keep the existing client shutdown flow unchanged after the
drain stabilizes.
In `@src/core/task/Task.ts`:
- Around line 2627-2636: The retry flow around shouldAddUserMessageToHistory and
the yesButtonClicked branch must keep user history and counts symmetric. When
removing the temporary user turn, decrement messageCounts.user, mark
userMessageWasRemoved true for the retry item, and ensure the retried request
restores the user message; add a regression test covering manual retry after an
empty-assistant response.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 632-636: Update the telemetry state handling in webviewDidLaunch
and the telemetrySetting handler to also require vscode.env.isTelemetryEnabled,
matching the logic in extension.ts. Ensure either path cannot re-enable
telemetry when VS Code’s global telemetry toggle is disabled, while preserving
the existing telemetrySetting preference behavior.
In `@webview-ui/src/i18n/locales/hi/welcome.json`:
- Line 15: Update the helpImproveMessage translation in the Hindi welcome locale
to replace the incorrect term “दूरसंचार कोड” with “टेलीमेट्री” or “टेलीमेट्री
डेटा,” while preserving the surrounding meaning and settingsLink markup.
---
Nitpick comments:
In `@src/__tests__/extension.spec.ts`:
- Around line 324-328: Update the beforeEach setup in the extension tests to
clear the shared onDidChangeTelemetryEnabled mock, including its call history,
after resetting modules and before each test runs. Ensure tests retrieving
mock.calls[0][0] observe the handler registered by the current activate call
rather than a prior test.
In
`@src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts`:
- Around line 219-241: Extend the test in “does not record raw model-supplied
tool names in tool usage analytics” to use a malicious tool name beginning with
“mcp_”, such as the existing injection payload with that prefix. Keep the
assertions covering both recordToolUsage and recordToolError so the
isValidToolName prefix-bypass path is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe02bd8b-7937-4172-aa49-30937b9de55a
📒 Files selected for processing (62)
PRIVACY.mdpackages/telemetry/src/PostHogTelemetryClient.tspackages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/PostHogTelemetryClient.test.tspackages/telemetry/src/__tests__/TelemetryService.circuit-breaker.test.tspackages/telemetry/src/__tests__/TelemetryService.shutdown.test.tspackages/telemetry/src/__tests__/TelemetryService.task-completed.test.tspackages/types/src/__tests__/telemetry.isTelemetryOptedIn.test.tspackages/types/src/__tests__/telemetry.taskProperties.test.tspackages/types/src/telemetry.tspackages/types/src/tool.tssrc/__tests__/extension.spec.tssrc/__tests__/nested-delegation-resume.spec.tssrc/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.tssrc/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.persistence.spec.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/messageCounting.spec.tssrc/core/task/messageCounting.tssrc/core/tools/ApplyPatchTool.tssrc/core/tools/AttemptCompletionTool.tssrc/core/tools/EditFileTool.tssrc/core/tools/EditTool.tssrc/core/tools/GenerateImageTool.tssrc/core/tools/SearchReplaceTool.tssrc/core/tools/__tests__/attemptCompletionTool.spec.tssrc/core/tools/__tests__/editFileTool.spec.tssrc/core/tools/__tests__/editTool.spec.tssrc/core/tools/__tests__/searchReplaceTool.spec.tssrc/core/webview/__tests__/telemetrySettingsTracking.spec.tssrc/core/webview/webviewMessageHandler.tssrc/extension.tssrc/shared/tools.tswebview-ui/src/__tests__/TelemetryClient.spec.tswebview-ui/src/components/chat/__tests__/ChatView.scroll-debug-repro.spec.tsxwebview-ui/src/components/common/TelemetryBanner.tsxwebview-ui/src/components/common/__tests__/TelemetryBanner.spec.tsxwebview-ui/src/components/settings/About.tsxwebview-ui/src/components/settings/__tests__/About.spec.tsxwebview-ui/src/i18n/locales/ca/welcome.jsonwebview-ui/src/i18n/locales/de/welcome.jsonwebview-ui/src/i18n/locales/en/welcome.jsonwebview-ui/src/i18n/locales/es/welcome.jsonwebview-ui/src/i18n/locales/fr/welcome.jsonwebview-ui/src/i18n/locales/hi/welcome.jsonwebview-ui/src/i18n/locales/id/welcome.jsonwebview-ui/src/i18n/locales/it/welcome.jsonwebview-ui/src/i18n/locales/ja/welcome.jsonwebview-ui/src/i18n/locales/ko/welcome.jsonwebview-ui/src/i18n/locales/nl/welcome.jsonwebview-ui/src/i18n/locales/pl/welcome.jsonwebview-ui/src/i18n/locales/pt-BR/welcome.jsonwebview-ui/src/i18n/locales/ru/welcome.jsonwebview-ui/src/i18n/locales/tr/welcome.jsonwebview-ui/src/i18n/locales/vi/welcome.jsonwebview-ui/src/i18n/locales/zh-CN/welcome.jsonwebview-ui/src/i18n/locales/zh-TW/welcome.jsonwebview-ui/src/utils/TelemetryClient.ts
💤 Files with no reviewable changes (6)
- src/core/tools/tests/editTool.spec.ts
- src/core/tools/tests/searchReplaceTool.spec.ts
- src/core/tools/ApplyPatchTool.ts
- src/core/tools/tests/editFileTool.spec.ts
- src/core/tools/GenerateImageTool.ts
- src/core/assistant-message/tests/presentAssistantMessage-custom-tool.spec.ts
Comment on lines
+2627
to
+2636
| // See shouldAddUserMessageToHistory for the full add/skip rules (retry/empty/removed). | ||
| const isEmptyUserContent = currentUserContent.length === 0 | ||
| const shouldAddUserMessage = | ||
| ((currentItem.retryAttempt ?? 0) === 0 && !isEmptyUserContent) || currentItem.userMessageWasRemoved | ||
| const shouldAddUserMessage = shouldAddUserMessageToHistory({ | ||
| retryAttempt: currentItem.retryAttempt, | ||
| isEmptyUserContent, | ||
| userMessageWasRemoved: currentItem.userMessageWasRemoved, | ||
| }) | ||
| if (shouldAddUserMessage) { | ||
| await this.addToApiConversationHistory({ role: "user", content: finalUserContent }) | ||
| TelemetryService.instance.captureConversationMessage(this.taskId, "user") | ||
| this.messageCounts.user++ |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and line ranges
printf '\n== Task.ts line map ==\n'
nl -ba src/core/task/Task.ts | sed -n '2608,2655p'
printf '\n== Task.ts retry handling around 3640-3715 ==\n'
nl -ba src/core/task/Task.ts | sed -n '3640,3715p'
printf '\n== Search for shouldAddUserMessageToHistory and userMessageWasRemoved ==\n'
rg -n "shouldAddUserMessageToHistory|userMessageWasRemoved|messageCounts\.user" src/core/task/Task.ts src -g '!**/dist/**' -g '!**/build/**'
printf '\n== Test files ==\n'
rg -n "messageCounting\.spec\.ts|Task\.spec\.ts|apiConversationHistory|messageCounts" src test . -g '*spec.ts' -g '*test.ts' -g '!**/dist/**' -g '!**/build/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/core/task/Task.ts | sed -n '2608,2655p'
printf '\n---\n'
nl -ba src/core/task/Task.ts | sed -n '3640,3715p'
printf '\n---\n'
rg -n "shouldAddUserMessageToHistory|userMessageWasRemoved|messageCounts\\.user" src/core/task/Task.ts src -g '!**/dist/**' -g '!**/build/**'
printf '\n---\n'
rg -n "messageCounting\\.spec\\.ts|Task\\.spec\\.ts|apiConversationHistory|messageCounts" . -g '*spec.ts' -g '*test.ts' -g '!**/dist/**' -g '!**/build/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print only the exact surrounding regions and the helper definition if present.
grep -n "function shouldAddUserMessageToHistory\|const shouldAddUserMessageToHistory\|userMessageWasRemoved" -n src/core/task/Task.ts || true
nl -ba src/core/task/Task.ts | sed -n '2588,2645p'
echo '---'
nl -ba src/core/task/Task.ts | sed -n '3638,3712p'
echo '---'
find . -name 'messageCounting.spec.ts' -o -name 'Task.spec.ts'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== helper and nearby logic =='
sed -n '2478,2510p' src/core/task/Task.ts
echo '== message add/pop/counter region =='
sed -n '2624,2642p' src/core/task/Task.ts
echo '== no-assistant-response pop region =='
sed -n '3648,3664p' src/core/task/Task.ts
echo '== retry pushes region =='
sed -n '3680,3712p' src/core/task/Task.ts
echo '== search for counter decrements =='
grep -n 'messageCounts\.user' src/core/task/Task.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -n "function shouldAddUserMessageToHistory\|const shouldAddUserMessageToHistory\|shouldAddUserMessageToHistory =" -n src/core/task/Task.ts || true
sed -n '2510,2565p' src/core/task/Task.ts
echo '---'
sed -n '4690,4710p' src/core/task/Task.ts
echo '---'
grep -n "telemetryMessageCountsBaseline\|messageCounts =" -n src/core/task/Task.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
find . \( -name 'messageCounting.spec.ts' -o -name 'Task.spec.ts' \) -print
echo '---'
grep -R -n "no assistant messages\|api_req_failed\|userMessageWasRemoved\|messageCounts.user" . --include='*spec.ts' --include='*test.ts' | head -200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow search for the helper definition if it is below the current section.
awk '
/shouldAddUserMessageToHistory/ {print NR ":" $0}
' src/core/task/Task.ts
echo '--- helper vicinity ---'
sed -n '2565,2605p' src/core/task/Task.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -R -n "shouldAddUserMessageToHistory" . --include='*.ts' --include='*.tsx' --include='*.js' --include='*.jsx'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -n "shouldAddUserMessageToHistory\|userMessageWasRemoved\|telemetryMessageCountsBaseline\|messageCounts.user" src/core/task/Task.ts
echo '--- helper vicinity ---'
sed -n '2518,2598p' src/core/task/Task.ts
echo '--- telemetry vicinity ---'
sed -n '4688,4710p' src/core/task/Task.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
find . \( -name 'messageCounting.spec.ts' -o -name 'Task.spec.ts' \) -print
echo '--- tests mentioning the retry flow ---'
grep -R -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|no assistant messages" . --include='*spec.ts' --include='*test.ts' | head -200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== src/core/task/messageCounting.ts =='
sed -n '1,200p' src/core/task/messageCounting.ts
echo '== src/core/task/__tests__/messageCounting.spec.ts =='
sed -n '1,220p' src/core/task/__tests__/messageCounting.spec.ts
echo '== src/core/task/__tests__/Task.spec.ts (retry-related snippets) =='
grep -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|shouldAddUserMessageToHistory" src/core/task/__tests__/Task.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the Task.spec.ts sections around any retry/history tests.
grep -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|shouldAddUserMessageToHistory" src/core/task/__tests__/Task.spec.ts | cut -d: -f1 | sort -n | uniq | while read -r line; do
start=$((line-12))
end=$((line+20))
if [ "$start" -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" src/core/task/__tests__/Task.spec.ts
echo '---'
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/core/task/messageCounting.ts
printf '\n---\n'
sed -n '1,220p' src/core/task/__tests__/messageCounting.spec.ts
printf '\n---\n'
grep -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|shouldAddUserMessageToHistory" src/core/task/__tests__/Task.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus only on the helper and the nearby Task tests if they exist.
sed -n '1,220p' src/core/task/messageCounting.ts
echo '---'
sed -n '1,220p' src/core/task/__tests__/messageCounting.spec.ts
echo '---'
grep -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|shouldAddUserMessageToHistory" src/core/task/__tests__/Task.spec.ts || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/core/task/messageCounting.ts
printf '\n---\n'
sed -n '1,220p' src/core/task/__tests__/messageCounting.spec.ts
printf '\n---\n'
grep -n "api_req_failed\|api_req_retried\|userMessageWasRemoved\|messageCounts.user\|shouldAddUserMessageToHistory" src/core/task/__tests__/Task.spec.ts || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3003
Keep the empty-assistant retry path symmetric
- Decrement
messageCounts.userwhen the temporary user turn is popped, and setuserMessageWasRemoved: truein theyesButtonClickedretry branch; otherwise the retried request skips restoring the user message and the counter drifts on each failed cycle. - Add a regression test for the manual retry path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/task/Task.ts` around lines 2627 - 2636, The retry flow around
shouldAddUserMessageToHistory and the yesButtonClicked branch must keep user
history and counts symmetric. When removing the temporary user turn, decrement
messageCounts.user, mark userMessageWasRemoved true for the retry item, and
ensure the retried request restores the user message; add a regression test
covering manual retry after an empty-assistant response.
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
added
awaiting-review
github-actions
Bot
added
awaiting-review
github-actions
Bot
removed
the
awaiting-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/webviewMessageHandler.ts (1)
641-651: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winwebviewDidLaunch's telemetry update isn't serialized with
telemetrySettingQueue, reopening the race the queue was built to fix.This block reads
telemetrySettingfrom an asyncgetStateToPostToWebview()snapshot and applies it viaTelemetryService.instance.updateTelemetryState(...)outsidetelemetrySettingQueue. If a concurrent"telemetrySetting"message is processed while this.then()is pending, this fire-and-forget continuation can resolve afterward with a stale value and clobber the queued (correct) update — the same interleaving class of bug the queue was added to prevent, just forwebviewDidLaunchvs.telemetrySettinginstead oftelemetrySettingvs.telemetrySetting.It's also redundant:
provider.postStateToWebview()already computed full state at line 575; recomputing it here just to read one field is unnecessary whengetGlobalState("telemetrySetting")(already used the same way in thetelemetrySettinghandler below) is synchronous.🔒 Suggested fix: read state synchronously, removing the async gap
- provider.getStateToPostToWebview().then((state) => { - const { telemetrySetting } = state - TelemetryService.instance.updateTelemetryState( - isTelemetryOptedIn(telemetrySetting) && vscode.env.isTelemetryEnabled, - ) - }) + const telemetrySetting = getGlobalState("telemetrySetting") || "unset" + TelemetryService.instance.updateTelemetryState( + isTelemetryOptedIn(telemetrySetting) && vscode.env.isTelemetryEnabled, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/webviewMessageHandler.ts` around lines 641 - 651, Update the webviewDidLaunch telemetry initialization to read telemetrySetting synchronously via the existing getGlobalState path instead of awaiting provider.getStateToPostToWebview(). Apply the value through the existing telemetrySettingQueue so launch initialization is serialized with telemetrySetting message handling, and remove the stale asynchronous continuation while preserving the VS Code global telemetry check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/i18n/locales/fr/settings.json`:
- Around line 966-967: Update the description value for the telemetry setting in
the French locale so its informal possessives use the formal forms “votre” and
“vos”, while preserving the existing formal wording and meaning.
In `@webview-ui/src/i18n/locales/hi/settings.json`:
- Line 967: Update the Hindi telemetry description string in the settings locale
to include the disclosure that telemetry can be disabled at any time, while
preserving the existing translation and privacy-policy link content.
In `@webview-ui/src/i18n/locales/it/settings.json`:
- Line 967: Update the Italian telemetry description in the settings locale to
state that telemetry can be disabled at any time, preserving the existing
privacy-link text and matching the equivalent disclosure used by other locales.
In `@webview-ui/src/i18n/locales/ja/settings.json`:
- Line 967: Update the Japanese telemetry description in the settings locale to
state that telemetry can be disabled at any time, while preserving the existing
explanation and privacyLink markup.
In `@webview-ui/src/i18n/locales/ko/settings.json`:
- Line 967: Update the Korean telemetry description value in the settings locale
to include that telemetry can be disabled at any time, while preserving the
existing installation-identifier, data-collection, code/prompt exclusion, and
privacy-link text.
In `@webview-ui/src/i18n/locales/tr/settings.json`:
- Line 966: Update the Turkish “label” value in the settings locale to use
grammatically correct telemetry wording, preferably “Hata ve kullanım
raporlamasına izin ver” or the equivalent approved alternative.
In `@webview-ui/src/i18n/locales/vi/settings.json`:
- Line 966: Update the Vietnamese telemetry label value near the settings
localization entry to clearly refer to both error reports and usage data, using
wording equivalent to “Cho phép báo cáo lỗi và dữ liệu sử dụng.”
In `@webview-ui/src/i18n/locales/zh-CN/settings.json`:
- Line 966: Update the Chinese settings label in the relevant permission entry
from “允许错误和使用情况报告” to “允许发送错误和使用情况报告”, preserving the existing key and
surrounding translations.
In `@webview-ui/src/i18n/locales/zh-TW/settings.json`:
- Line 994: Update the Traditional Chinese telemetry description in the settings
locale to include the equivalent disclosure that telemetry can be disabled at
any time, while preserving the existing translation and privacy-policy link.
---
Outside diff comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 641-651: Update the webviewDidLaunch telemetry initialization to
read telemetrySetting synchronously via the existing getGlobalState path instead
of awaiting provider.getStateToPostToWebview(). Apply the value through the
existing telemetrySettingQueue so launch initialization is serialized with
telemetrySetting message handling, and remove the stale asynchronous
continuation while preserving the VS Code global telemetry check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23f8bdfe-2bef-4513-837b-ce97a8f573a1
📒 Files selected for processing (58)
PRIVACY.mdpackages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.shutdown.test.tspackages/types/src/vscode-extension-host.tssrc/__tests__/extension.spec.tssrc/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-validation-error.spec.tssrc/core/assistant-message/__tests__/toTelemetryToolName.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/task/Task.tssrc/core/task/__tests__/messageCounting.retrySymmetry.spec.tssrc/core/tools/AttemptCompletionTool.tssrc/core/tools/__tests__/attemptCompletionTool.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/extension.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/TelemetryClient.spec.tswebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/ca/welcome.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/de/welcome.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/en/welcome.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/es/welcome.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/fr/welcome.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/hi/welcome.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/id/welcome.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/it/welcome.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ja/welcome.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/ko/welcome.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/nl/welcome.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pl/welcome.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/pt-BR/welcome.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/ru/welcome.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/tr/welcome.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/vi/welcome.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-CN/welcome.jsonwebview-ui/src/i18n/locales/zh-TW/settings.jsonwebview-ui/src/i18n/locales/zh-TW/welcome.jsonwebview-ui/src/utils/TelemetryClient.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- webview-ui/src/i18n/locales/ca/welcome.json
- webview-ui/src/i18n/locales/en/welcome.json
- webview-ui/src/i18n/locales/ru/welcome.json
- webview-ui/src/i18n/locales/id/welcome.json
- webview-ui/src/i18n/locales/zh-CN/welcome.json
- webview-ui/src/i18n/locales/zh-TW/welcome.json
- webview-ui/src/i18n/locales/vi/welcome.json
- webview-ui/src/i18n/locales/pt-BR/welcome.json
- webview-ui/src/i18n/locales/nl/welcome.json
- src/core/assistant-message/tests/presentAssistantMessage-unknown-tool.spec.ts
- webview-ui/src/i18n/locales/tr/welcome.json
- webview-ui/src/i18n/locales/fr/welcome.json
- webview-ui/src/i18n/locales/pl/welcome.json
- src/core/task/tests/messageCounting.retrySymmetry.spec.ts
- src/extension.ts
- webview-ui/src/i18n/locales/es/welcome.json
- src/core/tools/AttemptCompletionTool.ts
- webview-ui/src/i18n/locales/ko/welcome.json
- src/tests/extension.spec.ts
- webview-ui/src/i18n/locales/hi/welcome.json
- packages/telemetry/src/TelemetryService.ts
- src/core/assistant-message/presentAssistantMessage.ts
- src/core/tools/tests/attemptCompletionTool.spec.ts
- src/core/task/Task.ts
| "label": "गुमनाम त्रुटि और उपयोग रिपोर्टिंग की अनुमति दें", | ||
| "description": "गुमनाम उपयोग डेटा और त्रुटि रिपोर्ट भेजकर Zoo Code को बेहतर बनाने में मदद करें। यह टेलीमेट्री कोड, प्रॉम्प्ट या व्यक्तिगत जानकारी एकत्र नहीं करती। अधिक विवरण के लिए हमारी <privacyLink>गोपनीयता नीति</privacyLink> देखें। आप इसे कभी भी बंद कर सकते हैं।" | ||
| "label": "त्रुटि और उपयोग रिपोर्टिंग की अनुमति दें", | ||
| "description": "आपके नाम या खाते के बजाय एक इंस्टॉल-लिंक्ड पहचानकर्ता से जुड़े उपयोग डेटा और त्रुटि रिपोर्ट भेजकर Zoo Code को बेहतर बनाने में मदद करें। यह टेलीमेट्री आपका कोड या प्रॉम्प्ट एकत्र नहीं करती। अधिक विवरण के लिए हमारी <privacyLink>गोपनीयता नीति</privacyLink> देखें। आप इसे कभी भी बंद कर सकते हैं।" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Restore the opt-out disclosure.
This Hindi description omits that telemetry can be disabled at any time, unlike the settings behavior and peer locale strings. Add the equivalent sentence to keep consent messaging complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/hi/settings.json` at line 967, Update the Hindi
telemetry description string in the settings locale to include the disclosure
that telemetry can be disabled at any time, while preserving the existing
translation and privacy-policy link content.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/webview/__tests__/webviewMessageHandler.spec.ts (1)
1546-1599: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSolid regression coverage for the queue-ordering race; real-timer approach noted.
Good targeted reproduction of the pre-fix race (stale async state read clobbering a queued update) and a clear assertion on the final
updateTelemetryStatecall. Relying on realsetTimeoutdelays (2ms/10ms/50ms) to enforce ordering is a pragmatic but inherently timing-sensitive choice for CI; consider fake timers or an explicit deferred-promise handshake if this ever becomes flaky. As per coding guidelines,**/*.{test,spec}.{ts,tsx,js}should "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling," which this test satisfies for the state-transition concern being covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts` around lines 1546 - 1599, Make the regression test deterministic by replacing the real setTimeout delays in the webviewDidLaunch/telemetrySetting race with fake timers or explicit deferred-promise handshakes. Preserve the ordering where launch snapshots state, telemetrySetting disables telemetry, and the queued launch continuation completes afterward before asserting the final updateTelemetryState call.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts`:
- Around line 1546-1599: Make the regression test deterministic by replacing the
real setTimeout delays in the webviewDidLaunch/telemetrySetting race with fake
timers or explicit deferred-promise handshakes. Preserve the ordering where
launch snapshots state, telemetrySetting disables telemetry, and the queued
launch continuation completes afterward before asserting the final
updateTelemetryState call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b619b5d1-3fa4-4cb1-bff4-d2a7af968de5
📒 Files selected for processing (10)
src/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (8)
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- src/core/webview/webviewMessageHandler.ts
github-actions
Bot
added
the
awaiting-review
Contributor
|
3 issues outstanding.
Reviewed 8eedfac |
github-actions
Bot
added
has-conflicts
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.
Related GitHub Issue
Closes: #830
Description
PostHog's Product Analytics quota was 2.7x over its free-tier limit (2.7M/1M events in the June 6 - July 6 cycle), driven mainly by a
CODE_INDEX_ERRORretry-storm bug (309 people generating 254,786 events) and per-turnConversation Message/Tool Usedevents. Separately, a telemetry gap analysis found the consent banner treated dismissal (and the default "unset" state) as opt-in rather than a real choice, and that the underlying default itself was undisclosed.Volume reduction:
CODE_INDEX_ERROR: sliding 10-minute window, trips after 50 occurrences, independent of any other telemetry the same install sends (so unrelated events can't mask or reset a burst).Conversation Message/Tool Usedare no longer emitted per-turn/per-tool-call. Instead,Task.messageCountsandTask.toolUsageare summarized intotoolsUsed/messageCounton theTask Completedevent. Tool usage is now recorded exactly once per attempt at a single central point (presentAssistantMessage), removing a double-count where several tool handlers also recorded success locally.Task Completednow fires once per model-initiatedattempt_completioncall (taggedcompletionReason: "attempt_completion") rather than only when the user clicks Accept — a task the user declines, gives feedback on, or never explicitly accepts previously reported nothing at all. It also fires on a 30-minute idle timeout and on task/extension shutdown (completionReason: "idle"/"shutdown") so long-running or abandoned tasks aren't invisible either. Each installment reports only the delta since the previous one for that task (tracked separately fromtask.toolUsage/messageCounts, which stay full running totals for the publicTaskCompletedAPI event and the UI), so summing a task's installments reconstructs its full counts without double-counting. Skips emission for stale replays of an already-completed subtask (revisiting it from history).Model Cache Empty Responsethrottled to once per distinct provider+endpoint (matching the same compound cache-key identity the model cache itself uses) until a non-empty response is seen — including for auth-scoped providers (e.g.zoo-gateway), where the throttle now correctly re-arms after a later non-empty response even though those providers skip caching.Consent / privacy:
isTelemetryOptedIn()(packages/types) returnstrueunless the setting is explicitly"disabled". A fresh install ("unset") and an explicit"enabled"both capture telemetry; only an explicit Decline opts out. This replaces the previous ad hoctelemetrySetting !== "disabled"checks that were scattered across 5 call sites with a single source of truth, and keeps the default itself honestly disclosed inPRIVACY.mdand the banner copy rather than silently opting users in via an ambiguous dismiss gesture."unset"(so the disclosed default stays in effect), and does not itself record a choice either way. Explicit Accept/Decline buttons let a user affirmatively opt in or out. Localized across all 18 supported locales.vscode.env.onDidChangeTelemetryEnabled, ANDing the currentvscode.env.isTelemetryEnabledvalue into the gate so a live OS/editor-level telemetry-disable actually takes effect immediately, rather than only being caught on the next read of the deprecatedtelemetry.telemetryLevelsetting.TelemetryService.shutdown()is now properly awaited on deactivation, tracks in-flightcapture()/captureException()promises and drains them before flushing/closing clients, and is wrapped so a shutdown failure can't skip terminal cleanup.PRIVACY.mdclarified: telemetry is enabled by default (opt-out, changeable in settings), and the "does not collect code or prompts" bullet now notes this applies to the PostHog path only — the separate Cloud task-sync pipeline does transmit task messages when enabled.invalid_tool_callkey for unrecognized/malformed calls), so a model-supplied name can never reach PostHog as an arbitrarytoolsUsedproperty key.webview-ui's PostHog client now explicitly disablesautocapture,capture_pageview, andcapture_pageleaveon init, so the webview doesn't passively collect DOM interactions/navigation beyond the explicit events this PR already sends. (Thesrc-side client usesposthog-node, the server SDK, which has no browser autocapture/session-recording/heatmap surface to begin with.)Explicitly out of scope for this PR:
vscode.env.machineId— assessed and dropped; it's already a random per-install UUID, not a hostname/username-derived identifier, so hashing it would pseudonymize an already-non-identifying value while losing existing PostHog person continuity.captureExceptionpayloads, potentially movingCODE_INDEX_ERRORfrom Product Analytics to Error Tracking) — Error Tracking was only 6% over quota vs. Product Analytics' 2.7x, so it wasn't the priority; flagged for a follow-up.Test Procedure
pnpm check-typesandpnpm lintclean across all packages.src, 1456 inwebview-ui, 43 in@roo-code/telemetry, 238 in@roo-code/types— all passing. New/updated coverage includes:invalid_tool_callanalytics key rather than the raw model-supplied nameisTelemetryOptedIn()opt-out-by-default semantics ("unset"/"enabled"both opted in, only"disabled"opts out) and the resulting opt-in/opt-out transition eventsonDidChangeTelemetryEnabledlistener honoringvscode.env.isTelemetryEnabledTask Completedinstallment delta-tracking: first flush, second flush reporting only the new delta, no-op when nothing changed, idle-timer flush, shutdown flush, and skipping emission for a stale history replay of an already-completed subtaskattempt_completionemits aTask Completedinstallment immediately regardless of whether the user accepts/declines/gives feedback, with correct (non-duplicated)toolsUsed/messageCountdeltas and zeroConversation Message/Tool Usedevents.Pre-Submission Checklist
Documentation Updates
PRIVACY.mdupdated to state telemetry is enabled by default (opt-out) and to clarify the Cloud task-sync pipeline's scope relative to PostHog telemetry.Summary by CodeRabbit