🤖 feat: add Usage Telemetry toggle to Settings → General - #3850
Conversation
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b20a688ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both findings addressed in 9610baa:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9610baa072
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-2 finding addressed in 47d8ad0: when the reconciling |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d8ad024f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-3 finding addressed in c5e3eae: the switch renders disabled while |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5e3eae115
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both round-4 findings addressed in e54b346:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e54b34668d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-5 finding addressed in a9819ad: notifications arriving while local writes are in flight are no longer dropped — they set a missed-notification flag that replays the backend refresh when the pending-writes counter drains, so an external change landing during the write window (including our own write's early notification) always reconciles. Covered by a test where another client's enable arrives mid-flight during a local opt-out and the switch ends ON. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9819ad28a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-6 finding addressed in 34006f2: the persistence verification now re-reads with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34006f2930
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-7 finding addressed in 893cd6b: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893cd6b2fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-8 finding addressed in e6aea88: the subscription now triggers a re-sync as soon as the iterator is connected, so a change landing between the initial snapshot and establishment can no longer strand a stale switch. Covered by a test that holds the subscription unestablished while another client opts out and asserts the switch syncs on connect (no event ever pushed for the missed change). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6aea88e97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Both round-9 findings addressed in 49a35c2:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49a35c27c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
49a35c2 to
bd10767
Compare
|
@codex review Round-10 finding addressed (branch also rebased onto latest main, clean replay): deferred-notification replays now go through a ref the api-change effect keeps pointed at the CURRENT client generation, so a write settling after an API replacement can no longer consume the notification against the disconnected client. Covered by a test choreographing write-pending → API swap → late old-client failure, asserting the switch syncs through the replacement client. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd107679e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round-12 finding — cross-process
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
telemetryEnabled config field consulted by the telemetry service (env var MUX_DISABLE_TELEMETRY remains a hard override); toggling applies live by shutting down or re-initializing the PostHog client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Serialize setConfigEnabled applies and make initialize() re-entrant so rapid Settings toggles can't interleave PostHog shutdown/init and strand a live client after the user opted out. - Null the client before awaiting shutdown so no event can be captured into a flushing client, and re-check the config opt-out per capture() for API-server callers that bypass the toggle route. - isExplicitlyDisabled() now includes the config opt-out so features gated on explicit opt-out (e.g. link sharing) treat the Settings toggle the same as MUX_DISABLE_TELEMETRY=1. - Expose telemetryDisabledByEnv via getConfig and render the switch hard-disabled with an explanatory note when the environment override is active, instead of pretending the toggle controls anything. - Revert the optimistic switch state when persisting the change fails — a privacy control must not read "off" while collection continues. - Move the toggle into its own Privacy group and document that the env var must be exactly "1". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 1: - updateTelemetryEnabled now re-reads the config from disk after editConfig and fails loudly on mismatch before touching the live client: saveConfig swallows write errors (full disk, unwritable config.json), so the route could report success for a privacy opt-out that silently reverts on next launch. Router test proves it by making the config dir read-only. - The Settings switch tags each toggle with a monotonically increasing intent id: a superseded request's failure no longer blind-flips the switch (clobbering the user's latest choice mid rapid-toggle), and the latest intent's failure reloads the backend truth instead of guessing. - Fix router.test.ts config-route tests broken by the earlier telemetryDisabledByEnv addition (partial ORPCContext now stubs telemetryService). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 2: when a toggle write fails and the reconciling getConfig also fails (connection dropped after the request may have persisted and applied), the fallback rendered !checked — "off" after a failed enable while telemetry may actually be collecting. Indeterminate backend truth now always renders ON: showing "off" while collection may continue is the one lie a privacy toggle can't tell. The next successful config load reconciles the real value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 3: with a browser-mode outage (api: null, settings still mounted), clicking the switch flipped it optimistically and returned without issuing a write — rendering OFF while the backend may keep collecting, and silently discarding the intent. The switch now renders disabled without a usable API, and the handler refuses to flip before a deliverable write (covering the drop between render and click). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… clients Codex review round 4: - isTelemetryDisabledByConfig() read through loadConfigOrDefault(), which swallows read/parse errors and returns defaults — a corrupted config.json silently re-enabled telemetry for an opted-out user at startup and per capture(). It now reads with throwOnError and fails CLOSED: an unreadable existing file reports disabled, while a missing file (fresh install) stays enabled and callers remain non-fatal. - GeneralSection consumes the config.onConfigChanged stream so a second window/tab tracks telemetry changes made elsewhere instead of showing a stale switch while collection state already changed. Refreshes are guarded by the load nonce plus a pending-writes counter so our own in-flight writes reconcile through their own settle path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ites Codex review round 5: the pending-writes guard dropped notifications outright, and enqueueConfigEdit emits onConfigChanged before the RPC resolves — so even our own final write's notification arrives while the counter is positive, and an external change landing during the write window was lost forever (switch stuck OFF while another client enabled collection). Deferred notifications now set a flag that replays the backend refresh when the write queue drains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 6: the verification step reused isTelemetryDisabledByConfig(), whose fail-closed read (unreadable file => disabled) is right for enablement checks but let a failed disable write plus a failed read masquerade as a confirmed opt-out — the RPC reported success for a preference that resumes collecting on restart. The route now re-reads with throwOnError and a read failure fails the RPC with a distinct error, before touching the live client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 7: loadConfigOrDefault guards its read with existsSync(), which reports EACCES traversal failures as "missing" — so an opted-out user whose ~/.mux becomes unreachable read as enabled-by-default despite the strict-read fix. isTelemetryDisabledByConfig now stats the file explicitly: only a genuine ENOENT (fresh install) means enabled; every other stat/read/parse failure fails closed, without crashing startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 8: a config change landing between the initial getConfig snapshot and the onConfigChanged subscription's establishment had no listener — the switch stayed stale until the next unrelated edit. The subscription now refreshes once connected, closing the gap deterministically. Test holds the subscription unestablished while an external opt-out lands and asserts the switch syncs on connect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent reconciliation Codex review round 9: - A process that started while the shared config said opted-out never creates a PostHog client, so another process's re-enable left it dead until restart. capture() now kicks a lazy serialized initialize() when config says enabled but the client is null — rate-limited (30s) because the other enablement gates may legitimately keep it null. - An API replacement (browser-mode reconnect) bumps the telemetry intent counter, so a late rejection from the superseded client cannot run failure reconciliation against state the new client has since confirmed; the config subscription re-establishes and re-syncs on the new client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review round 10: the settle-replay callback captured the render's refreshTelemetryFromBackend, so a write settling after an API replacement replayed the deferred notification through the disconnected client — a failed read there consumed the notification and stranded the switch stale. Replays now go through a ref the api-change effect keeps pointed at the current client generation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bd10767 to
a898f93
Compare
1 participant
Summary
Adds a Usage Telemetry toggle to Settings → General (under a new Privacy group) so telemetry can be disabled (and re-enabled) from the UI. The choice persists as
telemetryEnabledin~/.mux/config.jsonand applies immediately — disabling shuts the PostHog client down mid-session, re-enabling re-runs the full enablement check.MUX_DISABLE_TELEMETRY=1remains a hard override that wins over the toggle; when the environment forces telemetry off, the switch renders disabled with a note saying so instead of pretending to control anything.Background
Telemetry originally had a client-side opt-out (referenced in #905 when reporting moved to the backend), but that surface disappeared along the way, leaving the environment variable as the only user-facing switch — which is hard to apply to a Dock-launched macOS app (GUI apps don't inherit shell profiles). A Settings toggle is the conventional surface for this in developer tools; docs continue to point at
payload.tsfor transparency about what is sent.Implementation
telemetryEnabled?: booleanconfig field (absent/true = enabled, false = disabled), stored sparsely: re-enabling deletes the key.shouldEnableTelemetrygains adisabledByConfiginput;TelemetryServicereceives anisDisabledByConfigcallback from the service container and consults it duringinitialize()and percapture()— the per-event re-check keeps API-server processes honest even if a toggle apply hasn't reached them.config.updateTelemetryEnabledroute persists the choice and callstelemetryService.setConfigEnabled()for the live apply. Applies are serialized (a promise chain) andinitialize()is re-entrant-safe, so rapid toggling can't interleave PostHog shutdown/init and strand a live client after an opt-out;shutdown()nulls the client before awaiting the flush so nothing can capture into a draining client.isExplicitlyDisabled()now includes the config opt-out, so features gated on explicit opt-out (e.g. link sharing) treat the Settings toggle the same asMUX_DISABLE_TELEMETRY=1.getConfigexposestelemetryDisabledByEnv; the Settings row renders the switch hard-disabled with an explanatory note when the environment override is active.Config.saveConfigswallows disk errors, so the route re-reads the persisted value aftereditConfigand fails loudly (before touching the live client) when the write didn't land. On the frontend, each toggle records an intent id — a superseded request's failure no-ops, and the latest intent's failure reloads the backend truth; if that truth is unreachable too, the switch renders ON (indeterminate state must never read "off" while collection may continue) until a successful config load reconciles it. Rapid toggling can't be clobbered by an early failure.1.Review-round hardening
Eleven Codex review rounds tightened the privacy edges (all threads resolved):
~/.muxthatexistsSyncwould mask as "missing" — reports disabled; only a genuine ENOENT (fresh install) means enabled. The RPC's persistence verification uses a strict read whose failure fails the request rather than masquerading as a confirmed opt-out.initialize();shutdown()nulls before flushing;capture()re-checks the config per event and lazily re-initializes (rate-limited) when another process re-enables telemetry.Validation
isExplicitlyDisabled()reflects the config opt-out;telemetryEnabledround-trips thesaveConfigwhitelist (including clearing back to default).Risks
Low. The enablement change is additive (one new early-return input); with the field absent, behavior is byte-identical to today. The live-apply path reuses the existing
shutdown()/initialize()lifecycle, now serialized against concurrent applies. Worst case on a config read failure inside the callback is telemetry staying in its startup state.🤖 Generated with Claude Code