Conversation
Contributor
📝 WalkthroughWalkthroughReplaces hashed email usage in CLI observability: raw email is used as distinct_id when present, the event properties always include a $set with the email, and an explicit alias ($create_alias) request is sent to merge prior hashed IDs with the new email. Also adds a changeset marking a patch release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/cli/utils/observability.ts`:
- Around line 19-24: The change switches distinct_id from a hashed email to raw
email causing identity splits; restore a consistent stable key by keeping the
SHA-256 hashed email as the distinct_id (and distinct_id_source) in this module
or implement an alias/migration: either (A) revert the return in the function
that sets distinct_id/org_id to use the same hashed-email logic as
packages/compiler/src/utils/observability.ts, or (B) emit an alias call (or
include both ids) to PostHog linking the old hashed id to the new raw email id
so events merge (use the same symbol names distinct_id/distinct_id_source and
add an alias/migration step before switching). Ensure both observability modules
use the identical id format to avoid splitting profiles.
- Around line 71-73: The current spread creates a new properties.$set that
replaces any caller-provided fields so only email remains; change the
construction so it merges into an existing properties.$set by building $set from
the existing properties.$set (or an empty object) and then conditionally adding
email, e.g. create $set: { ...(properties.$set || {}), ...(email ? { email } :
{}) } and spread that into the outer properties object (preserve other
properties via ...properties); update the code around the properties spread to
use this merged $set so caller-provided person properties are not clobbered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b222a181-72ed-48d3-a982-475566d3864c
📒 Files selected for processing (2)
.changeset/fancy-baboons-drum.mdpackages/cli/src/cli/utils/observability.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/cli/utils/observability.ts (1)
121-121: Consider normalizing email before use as distinct_id.The email is used directly without normalization. If the backend returns emails with varying cases (e.g.,
User@example.comvsuser@example.com), the same user could end up with multiple distinct profiles and multiple alias attempts that don't merge correctly.Consider normalizing with
.toLowerCase().trim()before using asdistinct_idand before hashing for the alias:Suggested change
if (email) { + const normalizedEmail = email.toLowerCase().trim(); - const hashedEmail = crypto.createHash("sha256").update(email).digest("hex"); + const hashedEmail = crypto.createHash("sha256").update(normalizedEmail).digest("hex"); const aliasData = JSON.stringify({ api_key: POSTHOG_API_KEY, event: "$create_alias", - distinct_id: email, + distinct_id: normalizedEmail,Also apply normalization in
determineDistinctIdat line 22.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/utils/observability.ts` at line 121, Normalize the email before using it as a distinct identifier and before hashing for aliasing: update the code paths where distinct_id is set (the object property distinct_id: email) and where alias/hash is computed to call a shared normalization helper or inline .toLowerCase().trim(); also apply the same normalization in the determineDistinctId function so all distinct id generation and aliasing use the exact normalized value to prevent duplicate profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/cli/utils/observability.ts`:
- Around line 115-140: Add the same alias emission implemented in the CLI to the
SDK observability flow: when email is present, compute hashedEmail =
crypto.createHash("sha256").update(email).digest("hex"), build aliasData JSON
containing api_key: POSTHOG_API_KEY, event: "$create_alias", distinct_id: email
and properties.alias: hashedEmail, then send it via https.request using the same
options/headers (Content-Type and Content-Length), attach the same
aliasReq.on("timeout", ...) and aliasReq.on("error", ...) handlers, write and
end the request, and enforce destruction after REQUEST_TIMEOUT_MS; ensure you
reference the same variables/names (email, hashedEmail, aliasData, aliasReq,
REQUEST_TIMEOUT_MS, POSTHOG_API_KEY, options) and place this logic alongside the
SDK's existing analytics send path so legacy hashed profiles merge with
raw-email identities.
- Line 115: The temporary alias removal date in the TODO for the "hashed
distinct_ids with new raw email" migration is too short; extend the migration
window to 3–6 months (or until analytics show negligible unmerged profiles) by
updating the TODO date and any associated expiry constant or config used by the
alias logic (the alias/merge code that handles old hashed distinct_ids -> new
raw email); ensure the comment and the config/constant that triggers removal
reflect the new deadline and, if present, add telemetry/metrics gating so the
alias logic can be disabled only after confirming low orphan rate.
---
Nitpick comments:
In `@packages/cli/src/cli/utils/observability.ts`:
- Line 121: Normalize the email before using it as a distinct identifier and
before hashing for aliasing: update the code paths where distinct_id is set (the
object property distinct_id: email) and where alias/hash is computed to call a
shared normalization helper or inline .toLowerCase().trim(); also apply the same
normalization in the determineDistinctId function so all distinct id generation
and aliasing use the exact normalized value to prevent duplicate profiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9a4b1fb-3533-4ea2-bcd1-f1f483b66854
📒 Files selected for processing (1)
packages/cli/src/cli/utils/observability.ts
Comment on lines
+115
to
+140
| // TODO: remove after 2026-03-25 — temporary alias to merge old hashed distinct_ids with new raw email | ||
| if (email) { | ||
| const hashedEmail = crypto.createHash("sha256").update(email).digest("hex"); | ||
| const aliasData = JSON.stringify({ | ||
| api_key: POSTHOG_API_KEY, | ||
| event: "$create_alias", | ||
| distinct_id: email, | ||
| properties: { | ||
| alias: hashedEmail, | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
|
|
||
| const aliasReq = https.request({ | ||
| ...options, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "Content-Length": Buffer.byteLength(aliasData).toString(), | ||
| }, | ||
| }); | ||
| aliasReq.on("timeout", () => aliasReq.destroy()); | ||
| aliasReq.on("error", () => {}); | ||
| aliasReq.write(aliasData); | ||
| aliasReq.end(); | ||
| setTimeout(() => { if (!aliasReq.destroyed) aliasReq.destroy(); }, REQUEST_TIMEOUT_MS); | ||
| } |
Contributor
There was a problem hiding this comment.
SDK lacks alias logic — legacy SDK profiles won't merge.
The SDK at packages/sdk/src/utils/observability.ts now uses raw email as distinct_id (matching this change), but it does not send the $create_alias event. SDK users who previously had events tracked under hashed email will have orphaned profiles that never get merged with their new raw-email identity.
Consider adding the same alias logic to the SDK module, or confirm that SDK analytics continuity is intentionally being sacrificed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli/utils/observability.ts` around lines 115 - 140, Add the
same alias emission implemented in the CLI to the SDK observability flow: when
email is present, compute hashedEmail =
crypto.createHash("sha256").update(email).digest("hex"), build aliasData JSON
containing api_key: POSTHOG_API_KEY, event: "$create_alias", distinct_id: email
and properties.alias: hashedEmail, then send it via https.request using the same
options/headers (Content-Type and Content-Length), attach the same
aliasReq.on("timeout", ...) and aliasReq.on("error", ...) handlers, write and
end the request, and enforce destruction after REQUEST_TIMEOUT_MS; ensure you
reference the same variables/names (email, hashedEmail, aliasData, aliasReq,
REQUEST_TIMEOUT_MS, POSTHOG_API_KEY, options) and place this logic alongside the
SDK's existing analytics send path so legacy hashed profiles merge with
raw-email identities.
| req.write(payload); | ||
| req.end(); | ||
|
|
||
| // TODO: remove after 2026-03-25 — temporary alias to merge old hashed distinct_ids with new raw email |
Contributor
There was a problem hiding this comment.
Two-week migration window may be insufficient.
The TODO indicates removal after 2026-03-25, which is only ~13 days from now. Users who don't upgrade and run the CLI within this window will never trigger the alias, leaving their old hashed profiles orphaned.
Consider extending the window (e.g., 3-6 months) to maximize the number of users who upgrade and get their profiles merged, or plan to keep the alias logic until analytics confirms minimal unmerged profiles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli/utils/observability.ts` at line 115, The temporary alias
removal date in the TODO for the "hashed distinct_ids with new raw email"
migration is too short; extend the migration window to 3–6 months (or until
analytics show negligible unmerged profiles) by updating the TODO date and any
associated expiry constant or config used by the alias logic (the alias/merge
code that handles old hashed distinct_ids -> new raw email); ensure the comment
and the config/constant that triggers removal reflect the new deadline and, if
present, add telemetry/metrics gating so the alias logic can be disabled only
after confirming low orphan rate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit