fix(web): default anonymous metrics on + mint installationId unless opted out#4131
Conversation
…pted out Settings → Privacy showed "Opted out" (匿名 ID = 已退出) for any install whose config had no installationId — even when the user never clicked "Don't share". mergeDaemonConfig left upgraded / never-prompted installs at metrics-on-but-no-id (or with no telemetry record at all), and the one-shot privacyDecisionAt migration then suppressed the default-on consent banner for those users, so they sat opted out with no way back in. After merging daemon privacy state, any install that has NOT explicitly opted out (telemetry.metrics === false, which "Don't share" persists alongside installationId: null) now gets anonymous metrics turned on and a stable installationId minted. The merged config is synced back to the daemon via syncConfigToDaemon, so the daemon-side reporting gate actually opens. The more sensitive content / artifactManifest channels are left at the user's last choice (default off) rather than silently enabled. Red specs: apps/web/tests/state/config.test.ts (on-but-no-id mints an id; brand-new install defaults on; explicit opt-out preserved without an id).
Siri-Ray
left a comment
There was a problem hiding this comment.
@lefarcen Thanks for tightening up the anonymous-id path here. I found one privacy-significant issue in the merge behavior that needs to be fixed before this lands: the new default-on branch can still persist content telemetry for an install with no daemon privacy state, even though this PR says only anonymous metrics should be enabled.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
nettee
left a comment
There was a problem hiding this comment.
I found one blocking privacy-default issue in the new mergeDaemonConfig bootstrap path: the code and PR both frame this change as auto-enabling only anonymous metrics, but the fresh-install path still carries forward the local content default. Details are inline.
|
Hey @lefarcen — the regression write-up is clear, especially the distinction between anonymous metrics vs. the more sensitive content / artifact-manifest channels. One PR-body follow-up before the next pass: the template parser still sees Surface area and Bug fix verification as missing, so could you tighten those two bits up in the body? In particular, please tick the applicable Surface area boxes in the expected checklist format and add a short Bug fix verification note that explicitly captures the red → green check for the fresh-install privacy-default path.
|
…default Review (Siri-Ray, nettee): the default-on branch claimed only anonymous metrics auto-enabled while content stayed off, but the daemon ships a metrics+content telemetry default, so the fresh-install path actually persisted content: true — a code/claim mismatch. Per maintainer decision, the product default (metrics + content on, artifactManifest off — the same surface the first-run banner's opt-in enables) is intended. This PR does not change which channels are on; it only mints the missing installationId and keeps the default channels on so Settings stops showing "Opted out". Reworded the comment to state this honestly, made the content default explicit (?? true), and added content/ artifactManifest assertions to the brand-new-install spec so the channel policy cannot silently regress. Explicit opt-out (metrics === false) still short-circuits the block.
|
Thanks @Siri-Ray @nettee — you correctly caught a code/claim mismatch: the daemon's Resolved by a maintainer decision rather than forcing the channels off: metrics + content on by default is the intended product surface (it's exactly what the first-run banner's "I get it" opt-in already enables, and what the daemon has defaulted to). This PR does not change that policy — it only mints the missing Pushed
Please re-review. |
nettee
left a comment
There was a problem hiding this comment.
@lefarcen I re-checked the changed ranges on the current head, and the follow-up resolves the earlier content-default mismatch cleanly: mergeDaemonConfig now matches the daemon's metrics+content default, keeps artifactManifest off, and the added state tests lock the fresh-install, missing-id, and explicit-opt-out cases to that intended behavior. I couldn't rerun the Vitest suite in this prepared worktree because apps/web/node_modules is absent here, but I don't see any remaining actionable issue in the current diff. Nice cleanup on the code/comment/test alignment.
Siri-Ray
left a comment
There was a problem hiding this comment.
@lefarcen I reviewed the current changed ranges for the telemetry bootstrap fix. The follow-up now aligns mergeDaemonConfig with the daemon's existing metrics+content default, keeps artifactManifest off by default, preserves explicit opt-outs, and adds focused state coverage for the fresh-install, missing-id, and opted-out paths. I could not rerun the Vitest suite in this prepared worktree because vitest is not installed here, but the code/comment/test alignment looks sound on this head. Thanks for the careful follow-up on the privacy-default behavior.








































































Why
Author's use case: Checking Settings → Privacy on a local install, the 匿名 ID / Anonymous ID field showed 「已退出 / Opted out」 even though I had never clicked 「不分享 / Don't share」.
The pain: The "Opted out" label is driven purely by
installationId == null(PrivacySection.tsx:value={cfg.installationId ?? t('settings.privacyOptedOut')}). AninstallationIdis only ever minted at an explicit opt-in moment (banner accept, Settings → Share, toggling a switch on, Delete-my-data rotation) — never at install time. The daemon, meanwhile, ships a metrics+content default (app-config.tsapplyTelemetryDefaults→{ metrics: true, content: true }) but never mints an id. So a fresh / upgraded / never-prompted install sits with telemetry on but no id, which renders as "Opted out" — and because that id is also the distinct id, daemon-side reporting never actually stitched.On top of that, the daemon shipping a default
telemetryobject trips the one-shotprivacyDecisionAtmigration inmergeDaemonConfig, soshowPrivacyConsent(gated onprivacyDecisionAt == null) is never true — the first-run disclosure banner effectively never shows. Net: the product's "data sharing on by default" never reached these users and they looked opted out.What users will see
In Settings → Privacy, an install that has not explicitly opted out now shows a real Anonymous ID instead of "Opted out", with 匿名指标 (Anonymous metrics) and 对话和工具内容 (Conversation & tool content) on — the product default, identical to what the first-run banner's "I get it" opt-in enables. (项目产物清单 / artifact manifest stays off, as it does on that surface.) Users who previously clicked Don't share are untouched:
metrics: falsekeeps them opted out with no id.How
In
mergeDaemonConfig(apps/web/src/state/config.ts), after merging the daemon's privacy state:explicitlyOptedOut = next.telemetry?.metrics === false(the shape Settings → "Don't share" persists, alongsideinstallationId: null).installationId: mint one viarandomUUID()and keep the product-default channels on (metrics: true,contentdefaulting on,artifactManifestoff), preserving any channel the user had explicitly turned off.The merged result is persisted back by the bootstrap's
syncConfigToDaemon(next), so the daemon-side reporting gate (telemetry.metrics === true) opens. Idempotent: once an id exists the block is skipped on every later boot.Surface area
odprivacy/consent subcommand exists today; this rides the shared/api/app-configstate, so no CLI surface to add here.Validation
pnpm --filter @open-design/web exec vitest run tests/state/config.test.ts→ 49/49 (new specs: brand-new install defaults metrics+content on with an id and artifactManifest off; on-but-no-id mints an id; explicit opt-out preserved without an id — the first two went red before the fix).pnpm --filter @open-design/web exec vitest run tests/components/{App.connectors,PrivacySection,App.mediaProviders,PrivacyConsentModal}.test.tsx→ 14/14pnpm --filter @open-design/web typecheck→ clean ·pnpm guard→ 54/54OD_DATA_DIR): clean boot auto-minted an id with no interaction (Settings showed the UUID, both toggles on); seeding an explicit opt-out (metrics:false,id:null) viaPUT /api/app-configand reloading kept "Opted out" with no id; seeding the bug state (metrics:true,id:null) and reloading auto-minted an id without any toggle.🤖 Generated with Claude Code