Skip to content

+semver:minor Made it possible to disable and enable tracking on the fly - #42

Merged
tombogle merged 6 commits into
masterfrom
enable-change-to-allow-tracking-property
Mar 19, 2026
Merged

+semver:minor Made it possible to disable and enable tracking on the fly#42
tombogle merged 6 commits into
masterfrom
enable-change-to-allow-tracking-property

Conversation

@tombogle

@tombogletombogle commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

This change is Reviewable

devin-ai-integration[bot]

This comment was marked as resolved.

@tombogle
tombogle marked this pull request as draft March 16, 2026 20:33
@tombogle
tombogle marked this pull request as draft March 16, 2026 20:33
@tombogle
tombogle marked this pull request as draft March 16, 2026 20:33
@tombogle
tombogle marked this pull request as ready for review March 18, 2026 15:53
Comment threadsrc/DesktopAnalytics/Analytics.cs Outdated

@devin-ai-integrationdevin-ai-integrationBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 8 additional findings in Devin Review.

Open in Devin Review

Comment threadsrc/DesktopAnalytics/Analytics.cs Outdated
Comment threadsrc/DesktopAnalytics/Analytics.cs Outdated
Comment on lines +1077 to +1079
public static void FlushClient()
{
s_singleton.Client.Flush();
s_singleton._client.Flush();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 FlushClient and Statistics don't guard against uninitialized client

FlushClient() (Analytics.cs:1079) and the Statistics property (Analytics.cs:1022) access _client without checking AllowTracking. For SegmentClient, Flush() calls _analytics.Flush() directly (SegmentClient.cs:67) — if Initialize was never called (tracking disabled), _analytics is null and this throws NullReferenceException. Statistics happens to be safe because it reads from the static StatMonitor. This is a pre-existing issue, but the new ability to toggle tracking makes it easier to encounter: a caller might reasonably call FlushClient() without realizing the client was never initialized.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@tombogletombogle left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombogle resolved 1 discussion.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on andrew-polk and nabalone).

@andrew-polkandrew-polk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrew-polk reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on nabalone and tombogle).


src/DesktopAnalytics/Analytics.cs line 275 at r3 (raw file):

 s_locationInfo =newJsonObject();
UpdateServerInformationOnThisUser(true);

This is more of a preference thing, but I think it is helpful to do UpdateServerInformationOnThisUser(initializing: true);

@tombogle
tombogle merged commit c729bd8 into masterMar 19, 2026
3 of 4 checks passed
@tombogle
tombogle deleted the enable-change-to-allow-tracking-property branch March 19, 2026 22:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@tombogle@andrew-polk