Skip to content

Trait history, billing lifecycle events, and ingestion credential scrubbing#538

Merged
izadoesdev merged 11 commits into
mainfrom
staging
Jul 3, 2026
Merged

Trait history, billing lifecycle events, and ingestion credential scrubbing#538
izadoesdev merged 11 commits into
mainfrom
staging

Conversation

@izadoesdev

@izadoesdev izadoesdev commented Jul 3, 2026

Copy link
Copy Markdown
Member

Summary

Ships the identity/trait-history work plus the credential-leak hardening from this week, alongside the tracker/dashboard fixes already on staging.

Security: sensitive query params scrubbed at ingestion

  • Basket now redacts a denylist of query params (password, token, api_key, email, otp, ...) from url/path/referrer/href on every write path, including hash fragments (OAuth implicit flow). Values become REDACTED, keys stay visible so leaky customer forms remain detectable.
  • Context: found real plaintext credentials in stored URLs (ours and customers'); leaked rows were purged from ClickHouse directly.

Profile trait history (Postgres)

  • New profile_trait_changes table: each row is a full post-change trait snapshot plus a {key: {old, new}} diff, written transactionally inside upsertProfile (SELECT ... FOR UPDATE). Answers "when did the plan change" and "what were the traits at time T" without diff folding.
  • Deliberately Postgres, not ClickHouse: transactional with the upsert, trivial GDPR deletes, and events already carry plan at event time so point-in-time joins aren't needed.

Billing lifecycle events

  • Autumn customer.products.updated webhook now calls recordPlanChange: plan trait upsert (source: "billing") + lifecycle custom events (plan_upgraded, subscription_canceled, ...) with profile_id attached. Gated on SELF_ANALYTICS_WEBSITE_ID (unset = no-op); scenario type is compile-checked against the webhook's zod enum.

Agent/MCP

  • get_profile_history tool added to buildProfileTools — available to both the in-app agent and MCP with website-access enforcement.

Dogfooding

  • Dashboard identifies with plan / has_active_subscription / billing_scope traits and stamps plan on every event via the newly exported setGlobalProperties.

Deploy notes

  • SELF_ANALYTICS_WEBSITE_ID must be set on the API service for lifecycle recording to activate (safe no-op until then).
  • profile_trait_changes is already applied to production Postgres.

Testing

  • basket: 485 tests passing (incl. new redaction + trait diff coverage)
  • api: 155 tests passing (autumn webhook mocks updated)
  • db drift tests + full monorepo typecheck green

Summary by cubic

Adds Postgres-backed trait history and billing lifecycle recording, and scrubs sensitive query params at ingestion to stop credential leaks. Also enriches profile lists with identity info and adds small tracker/CI fixes.

  • New Features

    • Scrub sensitive query params across all ingestion paths (password, token, api_key, email, otp, hash fragments). Values become REDACTED; keys stay.
    • Store profile trait change history in Postgres (profile_trait_changes) with full snapshots and diffs, written transactionally in upsertProfile; new get_profile_history tool.
    • Record billing lifecycle from Autumn webhooks via @databuddy/services (recordPlanChange): set plan trait (source: "billing") and emit custom events (e.g., plan_upgraded). Gated by SELF_ANALYTICS_WEBSITE_ID.
    • Query API attaches display_name and email to profile_list results (PII-safe), and the dashboard users page uses these to avoid flicker.
    • Dashboard identifies billing traits and sets a global plan property; @databuddy/sdk now exports setGlobalProperties.
  • Bug Fixes

    • Fix interesting_sessions builder and tighten session queries.
    • Tracker: enforce sess_... session IDs; reject malformed URL overrides; use sha256 in deploy utils.
    • CI: add ClickHouse service and init; new EXPLAIN suite validates all query builders.

Written for commit 4422b2e. Summary will update on new commits.

Review in cubic

izadoesdev added 11 commits July 3, 2026 22:34
new execution suite compiles all registered builders and EXPLAINs
them against ClickHouse, catching analyzer errors that compile-only
tests cannot; ci gains a clickhouse service and schema init so the
suite enforces instead of skipping. Fixes interesting_sessions,
which the sweep caught failing on the production server (correlated
CTE references and unaliased qualified projections).
getContentHash now uses node:crypto so sri.spec.ts can run under
Playwright's node runner, unblocking the deploy test gate.
@vercel

vercel Bot commented Jul 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
dashboard (staging) Ready Ready Preview, Comment Jul 3, 2026 10:19pm
databuddy-status Ready Ready Preview, Comment Jul 3, 2026 10:19pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
documentation (staging) Skipped Skipped Jul 3, 2026 10:19pm

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0be40ce2-3c23-4fd5-9a28-94567883a23f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch staging

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dosubot

dosubot Bot commented Jul 3, 2026

Copy link
Copy Markdown

📄 Knowledge review

🆕 New pages

1 new page was drafted from this PR.

Page Library
Profile Trait Change Tracking Databuddy's Space

✏️ Documentation updates

1 page was updated by changes in this PR.

Page Library Status
SDK Usage and Configuration Databuddy's Space ✅ Updated
📝 SDK Usage and Configuration — changes
@@ -33,7 +33,23 @@
 | `maskPatterns`        | string[]  | —            | Glob patterns to mask sensitive paths. |
 | `filter`              | function  | —            | Filter function to conditionally skip events. |
 
-All analytics are anonymized by default. No cookies, fingerprinting, or PII are collected. The SDK is safe for SSR/React Server Components and only injects the script on the client side.  
+All analytics are anonymized by default. No cookies, fingerprinting, or PII are collected. The SDK is safe for SSR/React Server Components and only injects the script on the client side.
+
+### Automatic Credential Scrubbing
+
+Databuddy automatically redacts sensitive query parameters at ingestion time to prevent credential leaks. URL, path, referrer, and href fields have sensitive query parameters (password, token, api_key, email, otp, cvv, ssn, etc.) replaced with `REDACTED` before storage.
+
+This applies to both query strings and hash fragments (including OAuth implicit flow tokens). Parameter keys remain visible so teams can identify leaky forms, but values are protected:
+
+```
+https://app.com/login?email=user@example.com&password=hunter2
+→ https://app.com/login?email=REDACTED&password=REDACTED
+
+https://app.com/callback#access_token=abc123&state=xyz
+→ https://app.com/callback#access_token=REDACTED&state=xyz
+```
+
+No configuration is required — credential scrubbing is applied automatically to protect against accidental credential exposure in URLs.  
 [Reference](https://github.com/databuddy-analytics/Databuddy/blob/78f406152290b45d3ace08be460e270ee0a01ffd/packages/sdk/README.md#L11-L118)
 
 ## SDK Methods
@@ -143,6 +159,27 @@
 ```js
 window.databuddy.identify("user_123", { email: "jo@acme.com" });
 ```
+
+#### Tracking Billing Lifecycle Events
+
+Traits can be used to track subscription and billing state. By setting traits like `plan`, `has_active_subscription`, and `billing_scope`, you enable lifecycle event tracking (subscription_started, plan_upgraded, plan_downgraded, subscription_renewed, subscription_canceled, etc.).
+
+```ts
+import { identify, setGlobalProperties } from "@databuddy/sdk";
+
+// On login, set billing traits
+identify(user.id, {
+  email: user.email,
+  plan: "pro",
+  has_active_subscription: true,
+  billing_scope: "organization"  // or "personal"
+});
+
+// Attach plan to all future events
+setGlobalProperties({ plan: "pro" });
+```
+
+Server-side systems (webhooks, billing providers) can update traits and emit lifecycle events by calling `identify()` with updated traits. The dashboard tracks when traits change and what they were at any point in time.
 
 ### setTraits
 

Leave Feedback Ask Dosu about Databuddy Add Dosu to your team

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 31 files

Confidence score: 2/5

  • packages/ai/src/query/builders/sessions.ts regresses the paths_for_top/names_for_top CTE scope by aggregating across all sessions in range instead of just paginated top_sessions, which can skew top-session results and significantly increase query cost under load — restore the prefilter join/constraint to top_sessions before merging.
  • packages/services/src/identity.ts leaves a race in upsertProfile for first-time identifies because SELECT ... FOR UPDATE does not lock a row that does not yet exist, so concurrent requests on the same (websiteId, profile...) can create duplicates or fail unpredictably — add a create-path concurrency guard (e.g., unique-key upsert/retry or advisory lock) and verify with a concurrent test before merging.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/ai/src/query/builders/sessions.ts">

<violation number="1" location="packages/ai/src/query/builders/sessions.ts:304">
P1: The `paths_for_top` and `names_for_top` CTEs lost the prefilter that limited aggregation to the paginated `top_sessions` set. As a result, they now `GROUP BY session_id` across every non-empty session in the date range (potentially millions) instead of just the selected top sessions (default 10), then discard the surplus in the final LEFT JOIN. This significantly inflates scan volume and memory/CPU for those two aggregations and raises timeout risk for the `interesting_sessions` endpoint. Restoring `AND session_id IN (SELECT session_id FROM top_sessions)` in both CTEs keeps the query scoped to the small result set it actually needs.</violation>
</file>

<file name="packages/services/src/identity.ts">

<violation number="1" location="packages/services/src/identity.ts:193">
P1: The `SELECT ... FOR UPDATE` inside the `upsertProfile` transaction only protects against concurrent updates when the profile row already exists. For two concurrent first-time identifies on the same `(websiteId, profileId)`, neither transaction acquires a lock (no row to lock), both read `{}` as `existingTraits`, and the second transaction's `profileTrait_changes` record will have incorrect `oldValue`s because it never saw the first transaction's insert. This breaks trait-history correctness for the bootstrap case.

To prevent this, acquire a transaction-scoped advisory lock before the SELECT, consistent with the existing pattern in `packages/rpc/src/routers/feedback.ts`, so that only one first-time upsert proceeds at a time per profile.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant Tracker as Client Tracker (SDK)
    participant Basket as Basket Ingestion API
    participant Autumn as Autumn (Billing Webhook)
    participant API as Databuddy Query API
    participant SDK as SDK / Dashboard Client
    participant Postgres as Postgres (RDS)
    participant ClickHouse as ClickHouse (Analytics)
    participant Services as @databuddy/services
    participant AI as AI Agent / MCP

    Note over Tracker,ClickHouse: Sensor/Sensitive Query Param Scrubbing (All Write Paths)

    Tracker->>Basket: POST /track, /identify, /outlink, /error, /vital
    Basket->>Basket: sanitizeUrl() → redactSensitiveQueryParams() on url/path/referrer/href
    Basket->>Basket: Redact denylist params (password, token, api_key, email, otp, ...) + hash fragments
    Basket->>ClickHouse: store scrubbed event (REDACTED values, keys preserved)

    Note over Postgres,ClickHouse: Profile Trait History (Postgres, Transactional)

    Basket->>Postgres: upsertProfile(websiteId, profileId, splitTraits)
    Postgres->>Postgres: SELECT ... FOR UPDATE on existing traits
    Postgres->>Postgres: applyTraits() → compute diff (old vs new for each key)
    Postgres->>Postgres: INSERT INTO profile_trait_changes (full snapshot + changes JSONB)
    Postgres-->>Basket: return { changes, traits }

    Note over Autumn,Postgres: Billing Lifecycle Events (Dogfooding)

    Autumn->>API: POST /webhooks/autumn (customer.products.updated, includes scenario)
    API->>Services: recordPlanChange({ customerId, planId, scenario })
    alt SELF_ANALYTICS_WEBSITE_ID set
        Services->>Postgres: upsertProfile(websiteId, customerId, { plan }, source="billing")
        Postgres->>Postgres: Apply trait changes, record in profile_trait_changes
        Services->>ClickHouse: INSERT INTO custom_events (event_name, profile_id=customerId, source=billing)
        Note over Services,ClickHouse: e.g. plan_upgraded, subscription_canceled
    else env var unset / no-op
        Services-->>API: return (skip)
    end
    API-->>Autumn: { success: true }

    Note over API,SDK: Profile Enrichment in Query API (PII-safe)

    SDK->>API: GET /v1/query (profile_list type)
    API->>ClickHouse: Execute query, get results with profile_id
    API->>Postgres: attachProfileIdentities() → SELECT displayName, email FROM profiles WHERE profile_id IN (...)
    Postgres-->>API: identity list
    API->>API: revealPii() on displayName, email (decrypt if encrypted)
    API->>SDK: results with display_name, email attached (null if no identity)
    SDK->>SDK: Render user name/email directly (no useProfileIdentities hook needed)

    Note over AI,Postgres: Profile History Tool (Agent / MCP)

    AI->>AI: get_profile_history tool invoked (agent or MCP)
    AI->>API: (internal) query with websiteId, profileId
    API->>Postgres: SELECT changes, traits, source, changedAt FROM profile_trait_changes ORDER BY createdAt DESC LIMIT N
    Postgres-->>API: history entries
    API-->>AI: { history: [...], count: N }
    AI->>AI: Answer "when did plan change" / "traits at time T"

    Note over SDK,ClickHouse: Dashboard Billing Identity

    SDK->>SDK: IdentifyBillingTraits component
    SDK->>SDK: identify(userId, { plan, has_active_subscription, billing_scope })
    SDK->>SDK: setGlobalProperties({ plan }) → attaches plan to all future events
    Tracker->>Basket: subsequent events include global plan property
    Basket->>ClickHouse: store event with plan property
Loading

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

AND time >= toDateTime({startDate:String})
AND time <= toDateTime({endDate:String})
AND session_id IN (SELECT session_id FROM top_sessions)
AND session_id != ''

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The paths_for_top and names_for_top CTEs lost the prefilter that limited aggregation to the paginated top_sessions set. As a result, they now GROUP BY session_id across every non-empty session in the date range (potentially millions) instead of just the selected top sessions (default 10), then discard the surplus in the final LEFT JOIN. This significantly inflates scan volume and memory/CPU for those two aggregations and raises timeout risk for the interesting_sessions endpoint. Restoring AND session_id IN (SELECT session_id FROM top_sessions) in both CTEs keeps the query scoped to the small result set it actually needs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/query/builders/sessions.ts, line 304:

<comment>The `paths_for_top` and `names_for_top` CTEs lost the prefilter that limited aggregation to the paginated `top_sessions` set. As a result, they now `GROUP BY session_id` across every non-empty session in the date range (potentially millions) instead of just the selected top sessions (default 10), then discard the surplus in the final LEFT JOIN. This significantly inflates scan volume and memory/CPU for those two aggregations and raises timeout risk for the `interesting_sessions` endpoint. Restoring `AND session_id IN (SELECT session_id FROM top_sessions)` in both CTEs keeps the query scoped to the small result set it actually needs.</comment>

<file context>
@@ -301,7 +301,7 @@ export const SessionsBuilders: Record<string, SimpleQueryConfig> = {
 						AND time >= toDateTime({startDate:String})
 						AND time <= toDateTime({endDate:String})
-						AND session_id IN (SELECT session_id FROM top_sessions)
+						AND session_id != ''
 					GROUP BY session_id
 				),
</file context>

)
)
.limit(1)
.for("update");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The SELECT ... FOR UPDATE inside the upsertProfile transaction only protects against concurrent updates when the profile row already exists. For two concurrent first-time identifies on the same (websiteId, profileId), neither transaction acquires a lock (no row to lock), both read {} as existingTraits, and the second transaction's profileTrait_changes record will have incorrect oldValues because it never saw the first transaction's insert. This breaks trait-history correctness for the bootstrap case.

To prevent this, acquire a transaction-scoped advisory lock before the SELECT, consistent with the existing pattern in packages/rpc/src/routers/feedback.ts, so that only one first-time upsert proceeds at a time per profile.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/services/src/identity.ts, line 193:

<comment>The `SELECT ... FOR UPDATE` inside the `upsertProfile` transaction only protects against concurrent updates when the profile row already exists. For two concurrent first-time identifies on the same `(websiteId, profileId)`, neither transaction acquires a lock (no row to lock), both read `{}` as `existingTraits`, and the second transaction's `profileTrait_changes` record will have incorrect `oldValue`s because it never saw the first transaction's insert. This breaks trait-history correctness for the bootstrap case.

To prevent this, acquire a transaction-scoped advisory lock before the SELECT, consistent with the existing pattern in `packages/rpc/src/routers/feedback.ts`, so that only one first-time upsert proceeds at a time per profile.</comment>

<file context>
@@ -99,33 +160,74 @@ export async function upsertProfile(
+				)
+			)
+			.limit(1)
+			.for("update");
+		const existingTraits = (existingRows[0]?.traits ?? {}) as Record<
+			string,
</file context>

@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR ships three interconnected features: credential scrubbing at ingestion (sanitizeUrl redacts sensitive query params including OAuth hash fragments), a Postgres-backed trait-history table (profile_trait_changes) written transactionally inside upsertProfile, and Autumn billing webhook integration that records lifecycle events and plan-trait upserts. It also tightens tracker session-ID validation, moves profile identity resolution server-side into the query layer, and adds self-analytics dogfooding via IdentifyBillingTraits.

  • Credential scrubbing: sanitizeUrl wraps redactSensitiveQueryParams (33-entry denylist, hash-fragment aware) and is applied across all write paths in basket — url, path, referrer, href, and error/vitals paths.
  • Trait history: upsertProfile now runs inside a SELECT … FOR UPDATE transaction; when traits change, a full post-change snapshot plus {key: {old, new}} diff is written to profile_trait_changes; a new get_profile_history AI tool exposes this to the agent/MCP layer.
  • Billing lifecycle: recordPlanChange (gated on SELF_ANALYTICS_WEBSITE_ID) upserts a plan trait and inserts a typed lifecycle custom event into ClickHouse on every Autumn customer.products.updated webhook.

Confidence Score: 3/5

The credential-scrubbing and session-ID hardening are straightforward and well-tested. The trait-history and billing-lifecycle code works correctly in the single-writer case but has two structural gaps that can produce silently wrong data in production.

The profile_trait_changes table has no foreign key from profile_id to profiles, so GDPR cascade deletes silently leave orphaned trait history — the stated motivation for choosing Postgres over ClickHouse. The SELECT … FOR UPDATE lock strategy does not protect against concurrent first-time profile creates, which can write history rows for transient states that were immediately overwritten. These two issues affect the correctness and compliance properties of the feature, not just edge-case performance.

packages/db/src/drizzle/schema/identity.ts (missing FK), packages/services/src/identity.ts (first-insert race), and packages/ai/src/query/builders/sessions.ts (unbounded CTE scan) deserve a second look before this ships to production.

Important Files Changed

Filename Overview
packages/db/src/drizzle/schema/identity.ts Adds profileTraitChanges table with composite index; missing FK from profileId to profiles means GDPR cascade deletes won't work automatically.
packages/services/src/identity.ts Wraps upsertProfile in a SELECT FOR UPDATE transaction and records trait diffs; race condition on first-time profiles (no row to lock) can produce duplicate/inaccurate history rows.
apps/basket/src/utils/validation.ts Adds sanitizeUrl and redactSensitiveQueryParams with hash-fragment support; URLSearchParams.toString() silently re-encodes non-sensitive params when any sensitive param is present.
packages/services/src/billing-lifecycle.ts New file wiring Autumn billing webhooks to profile upserts and ClickHouse custom events; correctly gated on SELF_ANALYTICS_WEBSITE_ID and wrapped in try/catch at the call site.
packages/ai/src/query/builders/sessions.ts Replaces session_id IN (SELECT … top_sessions) with session_id != '' in two CTEs, unbounding the ClickHouse scan to all sessions in the time window before the outer JOIN filters results.
apps/dashboard/components/providers/identify-billing-traits.tsx New component that identifies the logged-in user with billing traits on every page navigation; fires a full Postgres transaction on each render cycle even when traits are unchanged.
packages/ai/src/ai/tools/profiles.ts Adds get_profile_history tool to profile toolset; correctly scopes by websiteId via resolveSite and paginates with a bounded limit.
apps/api/src/routes/webhooks/autumn.ts Calls recordPlanChange on products.updated webhook; sandbox guard correctly refactored from the shouldSkipSlack logic and errors are caught without failing the webhook response.
apps/api/src/routes/query.ts Adds attachProfileIdentities to enrich profile_list query results with decrypted PII server-side; errors are swallowed with captureError so they don't break the query response.
packages/tracker/src/core/tracker.ts Tightens SESSION_ID_PATTERN to require the sess_ prefix, rejecting bare UUIDs from URL params; tests updated to match.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Tracker as Browser Tracker
    participant Basket as basket ingestion
    participant PG as Postgres
    participant CH as ClickHouse
    participant Autumn as Autumn Webhook
    participant API as api service

    Tracker->>Basket: POST /identify with traits
    Basket->>Basket: sanitizeUrl redacts sensitive params
    Basket->>PG: BEGIN tx SELECT profile FOR UPDATE
    PG-->>Basket: existingTraits
    Basket->>Basket: applyTraits compute diff
    Basket->>PG: UPSERT profile row
    Basket->>PG: INSERT profile_trait_changes if changed
    PG-->>Basket: commit
    Basket-->>Tracker: 200 OK

    Autumn->>API: POST /webhooks/autumn products.updated
    API->>PG: UPSERT profile with plan trait
    API->>PG: INSERT profile_trait_changes
    API->>CH: INSERT custom_events lifecycle event
    API-->>Autumn: 200 OK

    API->>PG: SELECT profile_trait_changes get_profile_history
    PG-->>API: trait snapshots and diffs
Loading
%%{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"}}}%%
sequenceDiagram
    participant Tracker as Browser Tracker
    participant Basket as basket ingestion
    participant PG as Postgres
    participant CH as ClickHouse
    participant Autumn as Autumn Webhook
    participant API as api service

    Tracker->>Basket: POST /identify with traits
    Basket->>Basket: sanitizeUrl redacts sensitive params
    Basket->>PG: BEGIN tx SELECT profile FOR UPDATE
    PG-->>Basket: existingTraits
    Basket->>Basket: applyTraits compute diff
    Basket->>PG: UPSERT profile row
    Basket->>PG: INSERT profile_trait_changes if changed
    PG-->>Basket: commit
    Basket-->>Tracker: 200 OK

    Autumn->>API: POST /webhooks/autumn products.updated
    API->>PG: UPSERT profile with plan trait
    API->>PG: INSERT profile_trait_changes
    API->>CH: INSERT custom_events lifecycle event
    API-->>Autumn: 200 OK

    API->>PG: SELECT profile_trait_changes get_profile_history
    PG-->>API: trait snapshots and diffs
Loading

Reviews (1): Last reviewed commit: "refactor(services): tighten billing life..." | Re-trigger Greptile

Comment on lines +52 to +73
// latest row with createdAt <= T, no diff folding needed.
traits: jsonb().$type<Record<string, unknown>>().default({}).notNull(),
changes: jsonb()
.$type<
Record<
string,
{
old: string | number | boolean | null;
new: string | number | boolean | null;
}
>
>()
.default({})
.notNull(),
source: text().default("identify").notNull(),
createdAt: timestamp({ precision: 3, withTimezone: true })
.defaultNow()
.notNull(),
},
(table) => [
index("profile_trait_changes_profile_idx").on(
table.websiteId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Missing FK from profileId to profiles undermines the stated GDPR story

The profileTraitChanges table has a FK from websiteId to websites (with cascade delete) but no FK from profileId to profiles. A GDPR delete that removes a row from profiles will not cascade to profile_trait_changes, leaving behind trait history for that user even though their profile is gone. The PR description calls out "trivial GDPR deletes" as a motivation for choosing Postgres over ClickHouse, so an explicit cascading FK on profileId seems necessary to deliver on that promise.

Comment on lines +183 to +193
const existingRows = await tx
.select({ traits: profiles.traits })
.from(profiles)
.where(
and(
eq(profiles.websiteId, websiteId),
eq(profiles.profileId, profileId)
)
)
.limit(1)
.for("update");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 SELECT … FOR UPDATE doesn't lock on a non-existent row — first-time profiles can produce duplicate / inconsistent history

When a profile doesn't exist yet, SELECT … FOR UPDATE returns no rows and acquires no predicate lock in Postgres. Two concurrent first-time upsertProfile calls for the same (websiteId, profileId) pair will both see existingTraits = {}, both call applyTraits({}, rest, []), and both attempt to insert into profileTraitChanges. The INSERT ON CONFLICT DO UPDATE on profiles is safe (one wins, the other updates), but profileTraitChanges has no uniqueness guard, so both writes succeed, leaving duplicate — and potentially contradictory — history rows: if the two calls carry different trait maps (e.g. back-to-back identify events during sign-up), a history row for a transient intermediate state that was immediately overwritten can appear as a permanent entry.

Comment on lines 301 to +316
@@ -313,7 +313,7 @@ export const SessionsBuilders: Record<string, SimpleQueryConfig> = {
website_id = {websiteId:String}
AND timestamp >= toDateTime({startDate:String})
AND timestamp <= toDateTime({endDate:String})
AND session_id IN (SELECT session_id FROM top_sessions)
AND session_id != ''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Replacing session_id IN (SELECT … top_sessions) with session_id != '' expands the scan to all sessions in the time window

The two CTEs (custom_events_per_session and names_for_top) previously scoped their reads to only the sessions that would appear in the final result via the correlated subquery. The new predicate session_id != '' removes that scoping — ClickHouse now reads every custom event and every page-name row for the entire requested time range before the outer LEFT JOIN discards non-top-sessions rows. For high-traffic websites this could be orders-of-magnitude more data than before, potentially causing query timeouts. The final SELECT output is still correct (the LEFT JOIN on ts.session_id bounds it), but the intermediate scan is no longer bounded.

Comment on lines +92 to +107
"cvv",
"ssn",
"email",
"e-mail",
"phone",
"tel",
"username",
]);

const REDACTED_VALUE = "REDACTED";

function redactParams(query: string): string {
const params = new URLSearchParams(query);
let changed = false;
for (const key of [...params.keys()]) {
if (SENSITIVE_QUERY_PARAMS.has(key.toLowerCase())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 URLSearchParams.toString() silently re-encodes non-sensitive params when any sensitive param is found

When changed is true, the function returns params.toString(), which percent-encodes the entire query string using URLSearchParams rules. This re-encodes space characters as + instead of %20, normalises %2B to +, and may change other application-specific encodings for non-sensitive keys that the caller never intended to alter. The problem only manifests when at least one sensitive param is present in the same query string, making it inconsistent: /search?q=hello%20world&utm=foo is stored unchanged, but /search?q=hello%20world&token=abc is stored as /search?q=hello+world&token=REDACTED, silently losing the %20+ distinction for q.

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!

Comment on lines +19 to +34
useEffect(() => {
if (isDashboardE2E || isLoading || !(userId && currentPlanId)) {
return;
}
identify(userId, {
plan: currentPlanId,
has_active_subscription: hasActiveSubscription,
billing_scope: isOrganizationBilling ? "organization" : "personal",
});
setGlobalProperties({ plan: currentPlanId });
}, [
userId,
currentPlanId,
hasActiveSubscription,
isOrganizationBilling,
isLoading,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 identify fires a full Postgres transaction on every page navigation

The useEffect runs whenever isLoading transitions, which happens on every route change that remounts this provider. Because rest = { plan, has_active_subscription, billing_scope } is never empty, upsertProfile always falls into the hasTraitUpdates path and runs a full transaction (SELECT … FOR UPDATE + INSERT ON CONFLICT DO UPDATE + possibly INSERT INTO profile_trait_changes), even when the traits are identical to what was last stored. Adding a client-side guard (e.g., comparing values before calling identify, or moving the call to a stable singleton) would prevent redundant DB round-trips for every page load.

@izadoesdev
izadoesdev merged commit bd86a7f into main Jul 3, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant