Cut over computed-posthog-events to using Keycloak's sub over email as the user's distinct_id#2805
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPostHog event projections now derive Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 review @greptileai review |
|
✅ Action performedReview finished.
|
Greptile SummaryThis PR completes the cut-over of all computed PostHog events from using email to using the Keycloak
Confidence Score: 5/5Safe to merge — the identity cut-over is complete and consistent across all event types, with clear fallback semantics. The helper rename and distinctId swap are mechanically straightforward. The two-tier fallback strategy (anon anchor for application-funnel events, null/drop for post-login events) is deliberate, well-commented, and exercised by a comprehensive test suite including new end-to-end identity-model tests. No event type is left in a mixed state. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Event projection triggered] --> B{Has userId?}
B -- No --> C{Application funnel event?}
B -- Yes --> D[getKeycloakIdentifierByUserId lookup]
D --> E{keycloakIdentifier found?}
E -- Yes --> F[distinctId = keycloakIdentifier]
E -- No --> C
C -- Yes --> G[distinctId = courseRegistrationAnonDistinctId]
C -- No --> H[distinctId = null - event dropped]
F --> I[Event sent to PostHog]
G --> I
H --> J[Event silently skipped]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Event projection triggered] --> B{Has userId?}
B -- No --> C{Application funnel event?}
B -- Yes --> D[getKeycloakIdentifierByUserId lookup]
D --> E{keycloakIdentifier found?}
E -- Yes --> F[distinctId = keycloakIdentifier]
E -- No --> C
C -- Yes --> G[distinctId = courseRegistrationAnonDistinctId]
C -- No --> H[distinctId = null - event dropped]
F --> I[Event sent to PostHog]
G --> I
H --> J[Event silently skipped]
Reviews (2): Last reviewed commit: "[refactor] computed-posthog-events: use ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test('skips a registration with no linked user (no identity to attribute to)', async () => { | ||
| await testDb.insert(courseRegistrationTable, { | ||
| id: 'a1', courseId: 'c1', email: 'acc@x.com', acceptedAt: '2026-06-01T10:00:00.000Z', | ||
| }); | ||
|
|
||
| const ph = mockPostHogBackend(); | ||
| await forwardAllEventsToPostHog(); | ||
|
|
||
| expect(ph.events.filter((e) => e.event === 'application_accepted')).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
A "skips when no linked user" test was added for application_accepted, but the same behaviour change affects application_rejected and application_withdrawn (both now resolve distinctId via userId → keycloakIdentifier and produce null when the chain is broken). A facilitated certificate_issued row that carries a certificateId but has userId: null would also be silently dropped now, where previously it was sent with the email. Adding analogous coverage for these three cases would make the silent-skip contract explicit across all the newly-migrated event types.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libraries/computed-posthog-events/src/definitions.ts (1)
106-150: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPotential silent loss for rows scanned before
keycloakIdentifierexistsThe Keycloak-backed projections resolve
distinctIdat scan time and drop rows when the linked user has no identifier yet (or skip them entirely inidentify_applicants).forwardEventTypeToPostHogrejects those events before they reachposthogEmittedEventsTable, so once the monotonicsincecursor moves past the row’s timestamp, it will not be reconsidered after a later backfill. If that backfill can lag, this needs an explicit retry/backfill path or a guarantee that all referenced users are populated first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libraries/computed-posthog-events/src/definitions.ts` around lines 106 - 150, Update calculateDropoutEvents and the related Keycloak-backed projection flow so events whose linked users lack a keycloakIdentifier are not permanently lost when the since cursor advances. Add an explicit retry/backfill mechanism for unresolved users, or enforce and validate that all referenced users are populated before advancing the cursor; preserve these rows for later emission once identifiers become available.
🧹 Nitpick comments (1)
libraries/computed-posthog-events/src/definitions.ts (1)
140-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
distinctIdresolution pattern could be a shared helper.The
(x ? map.get(x) : undefined) ?? nullexpression is repeated across dropout, accept/reject/withdraw, certificate, exercise, resource, and project events. Extracting a small helper (e.g.resolveDistinctId(userId, map)) would remove the duplication and centralise the fallback semantics.♻️ Proposed helper
+const resolveDistinctId = ( + userId: string | null | undefined, + keycloakIdentifierByUserId: Map<string, string>, +): string | null => (userId ? keycloakIdentifierByUserId.get(userId) : undefined) ?? null;Then replace each occurrence, e.g.:
- distinctId: (r.userId ? keycloakIdentifierByUserId.get(r.userId) : undefined) ?? null, + distinctId: resolveDistinctId(r.userId, keycloakIdentifierByUserId),Also applies to: 235-235, 260-260, 285-285, 317-317, 332-332, 376-376, 419-419, 480-480
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libraries/computed-posthog-events/src/definitions.ts` at line 140, Extract the repeated `(userId ? map.get(userId) : undefined) ?? null` logic into a shared `resolveDistinctId` helper in the definitions module, preserving its null fallback semantics. Replace the corresponding `distinctId` expressions across the dropout, accept/reject/withdraw, certificate, exercise, resource, and project event definitions with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libraries/computed-posthog-events/src/definitions.ts`:
- Around line 106-150: Update calculateDropoutEvents and the related
Keycloak-backed projection flow so events whose linked users lack a
keycloakIdentifier are not permanently lost when the since cursor advances. Add
an explicit retry/backfill mechanism for unresolved users, or enforce and
validate that all referenced users are populated before advancing the cursor;
preserve these rows for later emission once identifiers become available.
---
Nitpick comments:
In `@libraries/computed-posthog-events/src/definitions.ts`:
- Line 140: Extract the repeated `(userId ? map.get(userId) : undefined) ??
null` logic into a shared `resolveDistinctId` helper in the definitions module,
preserving its null fallback semantics. Replace the corresponding `distinctId`
expressions across the dropout, accept/reject/withdraw, certificate, exercise,
resource, and project event definitions with calls to this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86efca6b-aecc-4142-8192-39f26115ba34
📒 Files selected for processing (2)
libraries/computed-posthog-events/src/definitions.test.tslibraries/computed-posthog-events/src/definitions.ts
6049e6d to
47e9ae3
Compare
47e9ae3 to
1cd5bed
Compare
Render
Destroyed
Render
Destroyed
1 participant
Description
See the migration plan, this covers "PR 4" and "PR 5":
Issue
#2713
Developer checklist