-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1094 allow users to opt out of posthog #625
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
base: eng-1135-replacing-sendmail-with-explicit-posthog-errors
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,10 @@ import { | |
| DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, | ||
| DISCOURSE_TOOL_SHORTCUT_KEY, | ||
| STREAMLINE_STYLING_KEY, | ||
| DISALLOW_TRACKING, | ||
| } from "~/data/userSettings"; | ||
| import { enablePostHog, disablePostHog } from "~/utils/posthog"; | ||
| import internalError from "~/utils/internalError"; | ||
| import KeyboardShortcutInput from "./KeyboardShortcutInput"; | ||
| import { getSetting, setSetting } from "~/utils/extensionSettings"; | ||
| import streamlineStyling from "~/styles/streamlineStyling"; | ||
|
|
@@ -263,6 +266,38 @@ const HomePersonalSettings = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => { | |
| </> | ||
| } | ||
| /> | ||
| <Checkbox | ||
| defaultChecked={getSetting(DISALLOW_TRACKING, false)} | ||
| onChange={(e) => { | ||
| const target = e.target as HTMLInputElement; | ||
| const disallow = target.checked; | ||
| void setSetting(DISALLOW_TRACKING, disallow) | ||
| .then(() => { | ||
| if (disallow) { | ||
| disablePostHog(); | ||
| } else { | ||
| enablePostHog(); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| target.checked = !disallow; | ||
| internalError({ | ||
| error, | ||
| userMessage: "Could not change settings", | ||
| }); | ||
| }); | ||
| }} | ||
| labelElement={ | ||
| <> | ||
| Disable tracking | ||
| <Description | ||
| description={ | ||
| "Disable our recording of errors and minimal activity we use to improve our product." | ||
| } | ||
| /> | ||
| </> | ||
| } | ||
| /> | ||
|
Comment on lines
+269
to
+300
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s switch to copy that emphasizes product diagnostics and improvement rather than “tracking." Suggestion: Disable product diagnostics |
||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import getCurrentUserUid from "roamjs-components/queries/getCurrentUserUid"; | ||
| import { getVersionWithDate } from "./getVersion"; | ||
| import posthog from "posthog-js"; | ||
| import type { CaptureResult } from "posthog-js"; | ||
| import { getSetting } from "./extensionSettings"; | ||
| import { DISALLOW_TRACKING } from "~/data/userSettings"; | ||
|
|
||
| let initialized = false; | ||
|
|
||
| const doInitPostHog = (): void => { | ||
| if (initialized) return; | ||
|
Comment on lines
+8
to
+11
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix race condition: set The 🔎 Apply this diff to fix the race condition: const doInitPostHog = (): void => {
if (initialized) return;
+ initialized = true;
const propertyDenylist = new Set([Then remove the assignment from the loaded callback since it's no longer needed there: posthog.capture("Extension Loaded");
- initialized = true;
},Also applies to: 58-58 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think multiple calls in quick succession are unlikely, and oth I don't want to set the flag before a successful initialization.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maparent, that's a reasonable approach. Prioritizing successful initialization verification over preventing duplicate initialization attempts makes sense given your architecture and the low likelihood of concurrent calls. Your current implementation favors being able to retry on failure, which is a valid trade-off. 🧠 Learnings used |
||
| const propertyDenylist = new Set([ | ||
| "$ip", | ||
| "$device_id", | ||
| "$geoip_city_name", | ||
| "$geoip_latitude", | ||
| "$geoip_longitude", | ||
| "$geoip_postal_code", | ||
| "$geoip_subdivision_1_name", | ||
| "$raw_user_agent", | ||
| "$current_url", | ||
| "$referrer", | ||
| "$referring_domain", | ||
| "$initial_current_url", | ||
| "$pathname", | ||
| ]); | ||
| posthog.init("phc_SNMmBqwNfcEpNduQ41dBUjtGNEUEKAy6jTn63Fzsrax", { | ||
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| api_host: "https://us.i.posthog.com", | ||
| person_profiles: "identified_only", | ||
| capture_pageview: false, | ||
| property_denylist: [...propertyDenylist], | ||
| before_send: (result: CaptureResult | null) => { | ||
| if (result !== null) { | ||
| result.properties = Object.fromEntries( | ||
| Object.entries(result.properties).filter( | ||
| ([k]) => !propertyDenylist.has(k), | ||
| ), | ||
| ); | ||
| } | ||
| return result; | ||
| }, | ||
| /* eslint-enable @typescript-eslint/naming-convention */ | ||
| autocapture: false, | ||
| loaded: (posthog) => { | ||
| const { version, buildDate } = getVersionWithDate(); | ||
| const userUid = getCurrentUserUid(); | ||
| const graphName = window.roamAlphaAPI.graph.name; | ||
| posthog.identify(userUid, { | ||
| graphName, | ||
| }); | ||
| posthog.register({ | ||
| version: version || "-", | ||
| buildDate: buildDate || "-", | ||
| graphName, | ||
| }); | ||
| posthog.capture("Extension Loaded"); | ||
| initialized = true; | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| export const enablePostHog = (): void => { | ||
| doInitPostHog(); | ||
| posthog.opt_in_capturing(); | ||
| }; | ||
|
|
||
| export const disablePostHog = (): void => { | ||
| if (initialized) posthog.opt_out_capturing(); | ||
| }; | ||
|
|
||
| export const initPostHog = (): void => { | ||
| const disabled = getSetting(DISALLOW_TRACKING, false); | ||
| if (!disabled) { | ||
| doInitPostHog(); | ||
| } | ||
| }; | ||
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.
Similar here with "recording".
Suggestion: Disable sending usage signals and error reports that help us improve the product.