-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Opt-in telemetry #1438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Opt-in telemetry #1438
Conversation
|
src/core/Cline.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but I noticed this while adding events - I don't think we want to pop the subtask until the attempt_completion command has finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 0d85c3bcf44edabdc80b04d118108bf039577445 in 2 minutes and 53 seconds
More details
- Looked at
1331lines of code in27files - Skipped
3files when reviewing. - Skipped posting
29drafted comments based on config settings.
1. src/services/telemetry/TelemetryService.ts:192
- Draft comment:
Issue: The updateTelemetryState method in TelemetryService takes a boolean (didUserOptIn) but then passes it to PostHogClient.updateTelemetryState, which expects a TelemetrySetting string ("enabled", "disabled", or "unset"). This type mismatch could lead to unintended behavior. Please update the TelemetryService API so that it accepts a TelemetrySetting instead of a boolean, or add a conversion mapping. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70%
The comment claims PostHogClient.updateTelemetryState expects a TelemetrySetting string, but looking at the actual implementation, it takes a boolean. The comment is factually incorrect. Both classes use boolean parameters consistently throughout the telemetry enabling/disabling flow.
Could there be a newer version of PostHog that uses string settings that I'm not aware of? Should I check the PostHog documentation?
Even if newer PostHog versions use strings, this implementation clearly uses boolean values with optIn()/optOut() methods, so the comment is still incorrect for this codebase.
The comment should be deleted because it incorrectly claims there is a type mismatch when both methods consistently use boolean parameters.
2. src/utils/TelemetryClient.ts:8
- Draft comment:
Great use of posthog. Consider adding explicit return types on methods (e.g. updateTelemetryState, capture) for stricter type safety and clearer documentation. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
3. src/utils/TelemetryClient.ts:35
- Draft comment:
In capture(), errors are silently ignored. Consider logging the error (at least at a debug level) to help diagnose telemetry issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/utils/__tests__/TelemetryClient.test.ts:16
- Draft comment:
Nice tests verifying the singleton behavior and interaction with posthog-js. Consider adding a comment noting that the mock checks init options (api_host, persistence, and loaded callback). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/shared/TelemetrySetting.ts:1
- Draft comment:
The union type for TelemetrySetting is clear; ensure that the inline use of 'satisfies TelemetrySetting' in JSX is compatible with your TypeScript and ESLint configurations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
The comment is asking the PR author to ensure compatibility with TypeScript and ESLint configurations, which is against the rules of asking the author to ensure or verify something. It doesn't provide a specific suggestion or point out a clear issue with the code.
6. webview-ui/src/components/common/TelemetryBanner.tsx:29
- Draft comment:
Using 'satisfies TelemetrySetting' inline is neat, but verify that this pattern is supported by your build pipeline and documented for new developers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
This comment is asking the PR author to verify something about their build pipeline, which is against the rules. It doesn't provide a specific code suggestion or improvement.
7. webview-ui/src/components/settings/AdvancedSettings.tsx:14
- Draft comment:
Code is well‐structured. As a minor improvement, consider adding accessible labels or ARIA attributes for interactive elements to improve accessibility. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
8. webview-ui/src/components/settings/SettingsView.tsx:145
- Draft comment:
The state update logic and merging of configuration changes is well managed. Adding inline comments in complex handler sections could further improve maintainability. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
9. webview-ui/src/context/ExtensionStateContext.tsx:79
- Draft comment:
The mergeExtensionState function is clear and concise. Ensure that the merging logic covers edge cases for nested objects as the state evolves. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. webview-ui/src/context/__tests__/ExtensionStateContext.test.tsx:72
- Draft comment:
Good tests verifying the initial state and hook usage. Keep test cases updated if the state shape changes. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%
None
11. webview-ui/src/App.tsx:109
- Draft comment:
Integration of telemetry opt-in in the main App is clean. Ensure that focus and accessibility behaviors work consistently across all states. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%
This comment is asking the PR author to ensure something, which violates the rule against asking for confirmation or ensuring behavior. It doesn't provide a specific suggestion or point out a specific issue in the code.
12. .github/workflows/marketplace-publish.yml:45
- Draft comment:
Typo Alert: The package filename is referenced as 'roo-cline-${current_package_version}.vsix'. Please double-check if this should instead be 'roo-code-${current_package_version}.vsix' to remain consistent with the repository name and expected naming conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. .vscodeignore:27
- Draft comment:
Typo alert: The file pattern '.roomodes' on line 27 seems unusual and might be a typographical error. If this was not intended, please verify the correct spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. src/core/Cline.ts:268
- Draft comment:
Typographical error: In the comment describing the root task, 'top most patrnt' should be corrected to 'top most parent'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. src/core/webview/ClineProvider.ts:478
- Draft comment:
Typo: In the comment above the initClineWithSubTask method, 'ans' should be 'and'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. src/core/webview/__tests__/ClineProvider.test.ts:409
- Draft comment:
There appears to be a duplicate assertion for the string "https://us-assets.i.posthog.com". Consider removing one of these duplicate checks if it is not intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 70% vs. threshold = 70%
The comment is pointing out a real issue - having duplicate test assertions doesn't provide any additional test coverage and makes the test more verbose than necessary. However, this is a relatively minor code quality issue in a test file. The test still works correctly, it's just not as clean as it could be.
The comment is technically correct but may not be important enough to warrant a PR comment. Test files often have some redundancy and this doesn't affect functionality.
While minor, removing duplicate assertions helps keep tests cleaner and more maintainable. Each assertion should have a clear purpose.
The comment identifies a real but minor issue. Since it's an actionable suggestion that would improve code quality, even if only slightly, it's worth keeping.
17. src/core/webview/__tests__/ClineProvider.test.ts:1445
- Draft comment:
The error message string 'Error create new api configuration' seems grammatically off. Consider revising it to 'Error creating new API configuration' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. src/services/telemetry/TelemetryService.ts:6
- Draft comment:
Typo detected: The interface name 'ClineProviderInterface' might be intended to be 'ClientProviderInterface'. Please review and update if necessary. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70%
The name appears intentional, not a typo. The consistent usage and explicit forward declaration comment suggest this is a deliberate name. Without access to the broader codebase, we can't definitively say it's wrong. Making assumptions about naming without full context could lead to incorrect changes.
I could be wrong if there really is a ClientProvider elsewhere in the codebase that this should align with. However, the explicit forward declaration comment makes this unlikely.
Even if there was a ClientProvider elsewhere, the explicit forward declaration comment and consistent usage strongly suggest this is intentional naming.
The comment should be deleted as it makes assumptions about naming without sufficient context and the code shows clear signs of intentional naming.
19. src/shared/ExtensionMessage.ts:35
- Draft comment:
Typographical error: The union string 'requestyModels' in the 'ExtensionMessage' interface appears to be a typo. Consider renaming it to 'requestModels' for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
20. src/shared/WebviewMessage.ts:5
- Draft comment:
Typo suggestion: 'ClineAskResponse' may be intended to be spelled 'ClientAskResponse' for clarity. Please review if this is a naming mistake. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. src/shared/WebviewMessage.ts:46
- Draft comment:
Typo suggestion: 'refreshRequestyModels' appears to have an extra 'y' and might be intended as 'refreshRequestModels'. Please verify and update if needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
22. src/shared/WebviewMessage.ts:92
- Draft comment:
Typo suggestion: 'setopenAiCustomModelInfo' seems to incorrectly omit proper camel casing. Consider renaming it to 'setOpenAiCustomModelInfo' to follow typical naming conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
23. src/shared/globalState.ts:15
- Draft comment:
Typo detected: 'requestyApiKey' on line 15 might be a mistake. Consider renaming it to 'requestApiKey' if 'requesty' was unintended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
24. src/shared/globalState.ts:89
- Draft comment:
Possible typo detected: 'requestyModelId' (line 89) and 'requestyModelInfo' (line 90) might contain an extra 'y'. Consider revising them to 'requestModelId' and 'requestModelInfo' if the 'y' is unintentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
25. src/shared/globalState.ts:94
- Draft comment:
Typo detected: 'mistralCodestralUrl' on line 94 might be misspelled. Please verify the intended naming (for example, it might be meant to be 'mistralCodedUrl' or similar) and correct it if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
26. src/shared/globalState.ts:69
- Draft comment:
Potential typo in the inline comment at line 69: "Language setting for Cline's communication". Verify if 'Cline' is intended or if it should be 'client's'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
27. webview-ui/src/App.tsx:46
- Draft comment:
Consider renaming the function 'checkUnsaveChanges' to 'checkUnsavedChanges' for clarity, as the common term is 'unsaved changes'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
28. webview-ui/src/components/settings/SettingsView.tsx:191
- Draft comment:
Minor typographical suggestion: consider renaming 'checkUnsaveChanges' to 'checkUnsavedChanges' for clarity and consistency with the 'Unsaved changes' phrasing used elsewhere in the UI. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
29. webview-ui/src/context/__tests__/ExtensionStateContext.test.tsx:78
- Draft comment:
It appears that the property 'clineMessages' might be a typo. Did you mean 'clientMessages'? Please check and correct if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_MyfD981ox7GEXCgW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
e894ca1 to
7af1293
Compare
60ea0e4 to
8b4d8c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onChange handler for the VSCodeCheckbox directly casts e.target.checked without explicit typing. Consider using a proper type (e.g., React.ChangeEvent<HTMLInputElement>) to improve type-safety.
| onChange={(e: any) => { | |
| onChange={(e: React.ChangeEvent<HTMLInputElement>) => { |
Important
This pull request introduces opt-in telemetry using PostHog for the Roo Code extension, including telemetry service implementation, UI updates for telemetry settings, and associated tests.
TelemetryServiceinTelemetryService.tsto handle telemetry events using PostHog.telemetryClientinTelemetryClient.tsfor webview telemetry handling.extension.tsto initialize and shutdown telemetry service.TelemetryBannerinTelemetryBanner.tsxto prompt users to opt-in to telemetry.SettingsView.tsxandSettingsFooter.tsxto include telemetry settings.App.tsxandChatView.tsxto handle telemetry state and display the banner.telemetrySettingtoExtensionStateinExtensionMessage.tsandExtensionStateContext.tsx.globalState.tsto includetelemetrySettingkey.TelemetryClient.test.tsandExtensionStateContext.test.tsx.This description was created by
for 50f7658. It will automatically update as commits are pushed.