feat(analytics): close onboarding data blind spots#2491
Open
WcaleNieWolny wants to merge 5 commits into
Open
Conversation
- fix App Created / Org Created PostHog identity: events were captured with
the org UUID as distinct_id so signup funnels showed 0% conversion; now
resolve orgs.created_by (org id stays in groups/tags)
- add 'First Device Update Delivered' activation event: daily detection in
logsnag_insights cron via new apps.first_update_delivered_at column,
idempotent claim, historical backlog drained silently
- CLI: tag init/doctor/upload events with capgo_plugins + capgo_plugin_count,
add elapsed_ms per onboarding step, give 'canceled' events last_step context
- frontend: posthog.group('organization') on org switch, first-touch
attribution capture (UTM/referrer/click ids) persisted via $set_once,
login/signup failure + signup-completed events, step_elapsed_ms on
dashboard onboarding steps
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds plugin-discovery and merged CLI telemetry tags; captures first-touch attribution; records onboarding step elapsed times; emits login/signup analytics; enriches PostHog person/org data; and tracks first-device-update milestones with a DB-backed idempotent timestamp. ChangesCLI Plugin Discovery and Telemetry Enrichment
Client-Side Attribution and Onboarding Timing
Authentication Flow Analytics Events
PostHog Person and Organization Enrichment
Backend App Activation and Organization Creator Attribution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b3d1a477e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const firstTouch: FirstTouch = { | ||
| captured_at: new Date().toISOString(), | ||
| landing_url: window.location.href, |
There was a problem hiding this comment.
When first-touch capture runs on an auth callback with an external referrer or any UTM parameter, this stores the full window.location.href and setUser later sends it to PostHog as a person property. This app has auth flows that parse access_token, refresh_token, code, and confirmation_url from query/hash URLs, so those bearer/verification tokens can be persisted in analytics. Store only a sanitized path plus allow-listed marketing parameters, or explicitly strip auth-related query/hash values before saving.
Useful? React with 👍 / 👎.
Comment on lines
+869
to
+872
| WHERE apps.first_update_delivered_at IS NULL | ||
| AND dv.install > 0 | ||
| GROUP BY apps.app_id, apps.owner_org, apps.created_at, orgs.created_by | ||
| LIMIT 200 |
There was a problem hiding this comment.
For deployments with a large daily_version history, this query is unbounded: it joins every app whose new flag is still null to all historical daily_version rows with install > 0, groups them, and only then applies LIMIT 200. I checked supabase/schemas/prod.sql; daily_version is the time-series table with indexes on app_id/date, not on install, so the initial backfill can repeatedly scan a large historical join inside the daily insights cron. Add a bounded candidate set/indexed predicate or process from an indexed daily_version/date window so the cron cannot become dominated by this backfill.
Useful? React with 👍 / 👎.
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/init/command.ts (1)
1199-1204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCentralize cancel telemetry so enriched tags are never dropped.
Line 1201 (and the other cancel branches listed) still send bare cancel events, so
last_step/elapsed_msare missing on many cancellation paths. That creates schema drift for the same onboarding cancellation outcome and weakens downstream funnel analysis.Suggested fix direction
async function cancelCommand(command: boolean | string | symbol, orgId: string, apikey: string) { if (pIsCancel(command)) { - await markSnag('onboarding-v2', orgId, apikey, 'canceled', undefined, '🤷') - pOutro(`Bye 👋\n💡 You can resume the onboarding anytime by running the same command again`) - exit() + await exitCanceledInitOnboarding(orgId, apikey) } }Apply the same consolidation to direct
markSnag(..., 'canceled', ...)branches so all cancel exits share one payload contract.Also applies to: 1234-1237, 1997-1999, 3789-3791, 3873-3875, 3894-3896
🤖 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 `@cli/src/init/command.ts` around lines 1199 - 1204, The cancel flow in cancelCommand and other branches calls markSnag('canceled', ...) directly, which omits enriched tags (last_step, elapsed_ms); replace these direct markSnag calls with the unified cancel telemetry helper used elsewhere (e.g., call the centralized function that enriches cancellation payloads—referencing markSnag, cancelCommand, and pIsCancel to locate the code) so every cancellation path (including pOutro/exit branches) sends the same payload contract; ensure the helper adds last_step and elapsed_ms and use it in all other cancel branches instead of raw markSnag calls.
🤖 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.
Inline comments:
In `@cli/src/utils.ts`:
- Around line 257-306: The cachedCapgoPackages global currently ignores
packageJsonPath so listCapgoPackages(packageJsonPath) always returns the first
result; change the cache to key by the normalized packageJsonPath set (e.g., use
a Map keyed by a deterministic string like a sorted, comma-joined absolute paths
or single path) and adjust listCapgoPackages to compute that key from
packageJsonPath (normalizing, splitting, trimming, resolving to absolute paths)
before checking/setting the cache; update cachedCapgoPackages to the new Map
type and ensure getCapgoPluginTags still calls
listCapgoPackages(packageJsonPath) unchanged so telemetry respects the provided
path(s).
In `@src/services/posthog.ts`:
- Around line 96-109: The code passes potentially undefined fields from
getFirstTouch() into posthog.setPersonProperties (see getFirstTouch and
posthog.setPersonProperties), causing undefined values to be stored; update the
logic to build the $set_once object by filtering the firstTouch map so only keys
with defined (non-undefined) values are included before calling
posthog.setPersonProperties, e.g., iterate over the firstTouch fields or use a
small helper to copy only defined properties (first_touch_landing_url,
first_touch_referrer, first_touch_utm_*, first_touch_ref,
first_touch_captured_at) and pass that filtered object as the second arg.
In `@supabase/functions/_backend/triggers/logsnag_insights.ts`:
- Around line 875-913: The current loop claims the app by running
drizzleClient.execute(...) to set first_update_delivered_at before calling
sendEventToTracking(c,...), which makes the idempotency irreversible if tracking
fails; change the flow so each row is handled in its own try/catch and only
persist the first_update_delivered_at after sendEventToTracking returns
successfully (or if you must claim first, ensure you rollback the claim on send
failure by clearing first_update_delivered_at via another drizzleClient.execute
call), and ensure failures for one row log via cloudlogErr and continue
processing remaining rows instead of aborting the whole batch.
- Around line 859-873: The query that populates AppActivationRow via
drizzleClient.execute currently applies LIMIT 200 without ordering, which can
let old activations starve recent ones; modify the SQL in the query that
computes activation_date to prioritize recent activations by adding an ORDER BY
activation_date DESC before LIMIT 200 (or alternatively implement two passes:
one selecting recent activations ordered by activation_date DESC and another for
historical backfill), ensuring the computed activation_date is used for ordering
so fresh activations are processed first.
- Around line 859-883: The query and update currently use apps.app_id which is
not unique; modify the initial SELECT (the query executed into result) to also
return the primary key apps.id (e.g., include apps.id in the SELECT/Group By),
then in the update executed via drizzleClient.execute use that apps.id (row.id)
in the WHERE clause instead of row.app_id and RETURNING id (or both id and
app_id if you need app_id later); ensure the variable used when building the
UPDATE is row.id (the PK) so each physical apps row is claimed uniquely.
---
Outside diff comments:
In `@cli/src/init/command.ts`:
- Around line 1199-1204: The cancel flow in cancelCommand and other branches
calls markSnag('canceled', ...) directly, which omits enriched tags (last_step,
elapsed_ms); replace these direct markSnag calls with the unified cancel
telemetry helper used elsewhere (e.g., call the centralized function that
enriches cancellation payloads—referencing markSnag, cancelCommand, and
pIsCancel to locate the code) so every cancellation path (including pOutro/exit
branches) sends the same payload contract; ensure the helper adds last_step and
elapsed_ms and use it in all other cancel branches instead of raw markSnag
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea07958a-6bff-4b02-948a-df5bcc20c6d2
📒 Files selected for processing (24)
cli/src/app/debug.tscli/src/app/info.tscli/src/bundle/upload.tscli/src/init/command.tscli/src/utils.tscli/test/test-doctor-analytics.mjscli/test/test-v2-event-migration.mjssrc/components/dashboard/StepsApp.vuesrc/components/dashboard/StepsBuild.vuesrc/components/dashboard/StepsBundle.vuesrc/main.tssrc/pages/login.vuesrc/pages/register.vuesrc/services/attribution.tssrc/services/onboardingTimer.tssrc/services/posthog.tssrc/stores/organization.tssupabase/functions/_backend/triggers/logsnag_insights.tssupabase/functions/_backend/triggers/on_app_create.tssupabase/functions/_backend/triggers/on_organization_create.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/utils/tracking.tssupabase/migrations/20260611200850_add_first_update_delivered_at.sqltests/tracking.unit.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Comment on lines
+257
to
+306
| // Cached so analytics call sites read package.json at most once per process. | ||
| let cachedCapgoPackages: string[] | undefined | ||
|
|
||
| /** | ||
| * List the @capgo/* packages declared in the project's package.json files | ||
| * (dependencies + devDependencies), sorted alphabetically. | ||
| * Used for telemetry only: never throws, returns [] when nothing is readable. | ||
| * The first call wins the cache, later calls reuse it regardless of path. | ||
| */ | ||
| export function listCapgoPackages(packageJsonPath?: string): string[] { | ||
| if (cachedCapgoPackages) | ||
| return cachedCapgoPackages | ||
| const found = new Set<string>() | ||
| try { | ||
| const files = packageJsonPath | ||
| ? packageJsonPath.split(',').map(file => file.trim()).filter(Boolean) | ||
| : [join(findRoot(cwd()), PACKNAME)] | ||
| for (const file of files) { | ||
| try { | ||
| if (!existsSync(file)) | ||
| continue | ||
| const pkg = JSON.parse(readFileSync(file, 'utf-8')) | ||
| const dependencies = [...Object.keys(pkg.dependencies ?? {}), ...Object.keys(pkg.devDependencies ?? {})] | ||
| for (const dependency of dependencies) { | ||
| if (dependency.startsWith('@capgo/')) | ||
| found.add(dependency) | ||
| } | ||
| } | ||
| catch { | ||
| // Unreadable or invalid package.json: skip it, telemetry must not break commands. | ||
| } | ||
| } | ||
| } | ||
| catch { | ||
| // Root resolution failed: report no plugins instead of throwing. | ||
| } | ||
| cachedCapgoPackages = [...found].sort() | ||
| return cachedCapgoPackages | ||
| } | ||
|
|
||
| /** | ||
| * Plugin tags shared by key analytics events (init steps, doctor, upload). | ||
| */ | ||
| export function getCapgoPluginTags(packageJsonPath?: string): { capgo_plugins: string, capgo_plugin_count: number } { | ||
| const plugins = listCapgoPackages(packageJsonPath) | ||
| return { | ||
| capgo_plugins: plugins.join(','), | ||
| capgo_plugin_count: plugins.length, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Cache key ignores packageJsonPath, so telemetry can become path-order dependent.
After the first call, listCapgoPackages(...) always returns the first cached result and ignores later packageJsonPath values. That breaks the path-specific contract expected by callers (getCapgoPluginTags(options.packageJson) in Doctor/Upload) and can attach plugin tags from the wrong manifest set in long-lived processes.
💡 Suggested fix (cache by normalized path set)
-// Cached so analytics call sites read package.json at most once per process.
-let cachedCapgoPackages: string[] | undefined
+// Cache per normalized package.json path set.
+const cachedCapgoPackagesByPath = new Map<string, string[]>()
export function listCapgoPackages(packageJsonPath?: string): string[] {
- if (cachedCapgoPackages)
- return cachedCapgoPackages
- const found = new Set<string>()
+ const files = packageJsonPath
+ ? packageJsonPath.split(',').map(file => file.trim()).filter(Boolean)
+ : [join(findRoot(cwd()), PACKNAME)]
+ const normalizedFiles = files.map(file => resolve(file)).sort((a, b) => a.localeCompare(b))
+ const cacheKey = normalizedFiles.join(',')
+ const cached = cachedCapgoPackagesByPath.get(cacheKey)
+ if (cached)
+ return cached
+
+ const found = new Set<string>()
try {
- const files = packageJsonPath
- ? packageJsonPath.split(',').map(file => file.trim()).filter(Boolean)
- : [join(findRoot(cwd()), PACKNAME)]
for (const file of files) {
try {
if (!existsSync(file))
continue
const pkg = JSON.parse(readFileSync(file, 'utf-8'))
@@
catch {
// Root resolution failed: report no plugins instead of throwing.
}
- cachedCapgoPackages = [...found].sort()
- return cachedCapgoPackages
+ const result = [...found].sort()
+ cachedCapgoPackagesByPath.set(cacheKey, result)
+ return result
}🤖 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 `@cli/src/utils.ts` around lines 257 - 306, The cachedCapgoPackages global
currently ignores packageJsonPath so listCapgoPackages(packageJsonPath) always
returns the first result; change the cache to key by the normalized
packageJsonPath set (e.g., use a Map keyed by a deterministic string like a
sorted, comma-joined absolute paths or single path) and adjust listCapgoPackages
to compute that key from packageJsonPath (normalizing, splitting, trimming,
resolving to absolute paths) before checking/setting the cache; update
cachedCapgoPackages to the new Map type and ensure getCapgoPluginTags still
calls listCapgoPackages(packageJsonPath) unchanged so telemetry respects the
provided path(s).
Comment on lines
+96
to
+109
| const firstTouch = getFirstTouch() | ||
| if (firstTouch) { | ||
| // setPersonProperties(setProps, setOnceProps): pass the first-touch map | ||
| // as $set_once so it never overwrites previously captured attribution | ||
| posthog.setPersonProperties({}, { | ||
| first_touch_landing_url: firstTouch.landing_url, | ||
| first_touch_referrer: firstTouch.referrer, | ||
| first_touch_utm_source: firstTouch.utm_source, | ||
| first_touch_utm_medium: firstTouch.utm_medium, | ||
| first_touch_utm_campaign: firstTouch.utm_campaign, | ||
| first_touch_ref: firstTouch.ref, | ||
| first_touch_captured_at: firstTouch.captured_at, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Filter out undefined optional properties before passing to PostHog.
The FirstTouch type defines UTM and click-id parameters as optional (Partial<Record<SignalParam, string>>). When these properties are undefined, lines 103-107 will include them in the setPersonProperties call with undefined values (e.g., { first_touch_utm_source: undefined }). PostHog will store these as null/undefined, making it difficult to distinguish "never captured" from "captured but absent," and potentially affecting analytics filtering and aggregation.
🛡️ Proposed fix to filter undefined properties
const firstTouch = getFirstTouch()
if (firstTouch) {
- // setPersonProperties(setProps, setOnceProps): pass the first-touch map
- // as $set_once so it never overwrites previously captured attribution
- posthog.setPersonProperties({}, {
- first_touch_landing_url: firstTouch.landing_url,
- first_touch_referrer: firstTouch.referrer,
- first_touch_utm_source: firstTouch.utm_source,
- first_touch_utm_medium: firstTouch.utm_medium,
- first_touch_utm_campaign: firstTouch.utm_campaign,
- first_touch_ref: firstTouch.ref,
- first_touch_captured_at: firstTouch.captured_at,
- })
+ // setPersonProperties(setProps, setOnceProps): pass the first-touch map
+ // as $set_once so it never overwrites previously captured attribution
+ const setOnceProps: Record<string, string> = {
+ first_touch_landing_url: firstTouch.landing_url,
+ first_touch_referrer: firstTouch.referrer,
+ first_touch_captured_at: firstTouch.captured_at,
+ }
+ if (firstTouch.utm_source) setOnceProps.first_touch_utm_source = firstTouch.utm_source
+ if (firstTouch.utm_medium) setOnceProps.first_touch_utm_medium = firstTouch.utm_medium
+ if (firstTouch.utm_campaign) setOnceProps.first_touch_utm_campaign = firstTouch.utm_campaign
+ if (firstTouch.utm_content) setOnceProps.first_touch_utm_content = firstTouch.utm_content
+ if (firstTouch.utm_term) setOnceProps.first_touch_utm_term = firstTouch.utm_term
+ if (firstTouch.ref) setOnceProps.first_touch_ref = firstTouch.ref
+ if (firstTouch.gclid) setOnceProps.first_touch_gclid = firstTouch.gclid
+ if (firstTouch.fbclid) setOnceProps.first_touch_fbclid = firstTouch.fbclid
+ posthog.setPersonProperties({}, setOnceProps)
}🤖 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 `@src/services/posthog.ts` around lines 96 - 109, The code passes potentially
undefined fields from getFirstTouch() into posthog.setPersonProperties (see
getFirstTouch and posthog.setPersonProperties), causing undefined values to be
stored; update the logic to build the $set_once object by filtering the
firstTouch map so only keys with defined (non-undefined) values are included
before calling posthog.setPersonProperties, e.g., iterate over the firstTouch
fields or use a small helper to copy only defined properties
(first_touch_landing_url, first_touch_referrer, first_touch_utm_*,
first_touch_ref, first_touch_captured_at) and pass that filtered object as the
second arg.
Comment on lines
+859
to
+873
| const result = await drizzleClient.execute<AppActivationRow>(sql` | ||
| SELECT | ||
| apps.app_id, | ||
| apps.owner_org, | ||
| apps.created_at AS app_created_at, | ||
| orgs.created_by AS org_created_by, | ||
| MIN(dv.date)::date AS activation_date | ||
| FROM public.apps AS apps | ||
| INNER JOIN public.daily_version AS dv ON dv.app_id = apps.app_id | ||
| LEFT JOIN public.orgs AS orgs ON orgs.id = apps.owner_org | ||
| WHERE apps.first_update_delivered_at IS NULL | ||
| AND dv.install > 0 | ||
| GROUP BY apps.app_id, apps.owner_org, apps.created_at, orgs.created_by | ||
| LIMIT 200 | ||
| `) |
Contributor
There was a problem hiding this comment.
Prioritize recent activations before applying LIMIT 200.
Line 872 caps an unordered backlog. During the initial drain, older rows can consume the batch while fresh activations wait long enough to cross the > 7 day guard on Line 891, so their event is never emitted. Sort by the computed activation date descending, or split “recent activations” from “historical backfill” into separate passes.
Suggested query shape
- const result = await drizzleClient.execute<AppActivationRow>(sql`
- SELECT
- apps.app_id,
- apps.owner_org,
- apps.created_at AS app_created_at,
- orgs.created_by AS org_created_by,
- MIN(dv.date)::date AS activation_date
- FROM public.apps AS apps
- INNER JOIN public.daily_version AS dv ON dv.app_id = apps.app_id
- LEFT JOIN public.orgs AS orgs ON orgs.id = apps.owner_org
- WHERE apps.first_update_delivered_at IS NULL
- AND dv.install > 0
- GROUP BY apps.app_id, apps.owner_org, apps.created_at, orgs.created_by
- LIMIT 200
- `)
+ const result = await drizzleClient.execute<AppActivationRow>(sql`
+ WITH activations AS (
+ SELECT
+ apps.app_id,
+ apps.owner_org,
+ apps.created_at AS app_created_at,
+ orgs.created_by AS org_created_by,
+ MIN(dv.date)::date AS activation_date
+ FROM public.apps AS apps
+ INNER JOIN public.daily_version AS dv ON dv.app_id = apps.app_id
+ LEFT JOIN public.orgs AS orgs ON orgs.id = apps.owner_org
+ WHERE apps.first_update_delivered_at IS NULL
+ AND dv.install > 0
+ GROUP BY apps.app_id, apps.owner_org, apps.created_at, orgs.created_by
+ )
+ SELECT *
+ FROM activations
+ ORDER BY activation_date DESC
+ LIMIT 200
+ `)🤖 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 `@supabase/functions/_backend/triggers/logsnag_insights.ts` around lines 859 -
873, The query that populates AppActivationRow via drizzleClient.execute
currently applies LIMIT 200 without ordering, which can let old activations
starve recent ones; modify the SQL in the query that computes activation_date to
prioritize recent activations by adding an ORDER BY activation_date DESC before
LIMIT 200 (or alternatively implement two passes: one selecting recent
activations ordered by activation_date DESC and another for historical
backfill), ensuring the computed activation_date is used for ordering so fresh
activations are processed first.
Comment on lines
+859
to
+883
| const result = await drizzleClient.execute<AppActivationRow>(sql` | ||
| SELECT | ||
| apps.app_id, | ||
| apps.owner_org, | ||
| apps.created_at AS app_created_at, | ||
| orgs.created_by AS org_created_by, | ||
| MIN(dv.date)::date AS activation_date | ||
| FROM public.apps AS apps | ||
| INNER JOIN public.daily_version AS dv ON dv.app_id = apps.app_id | ||
| LEFT JOIN public.orgs AS orgs ON orgs.id = apps.owner_org | ||
| WHERE apps.first_update_delivered_at IS NULL | ||
| AND dv.install > 0 | ||
| GROUP BY apps.app_id, apps.owner_org, apps.created_at, orgs.created_by | ||
| LIMIT 200 | ||
| `) | ||
|
|
||
| for (const row of result.rows ?? []) { | ||
| const activationDate = new Date(row.activation_date) | ||
| const claimed = await drizzleClient.execute<{ app_id: string }>(sql` | ||
| UPDATE public.apps | ||
| SET first_update_delivered_at = ${activationDate} | ||
| WHERE app_id = ${row.app_id} | ||
| AND first_update_delivered_at IS NULL | ||
| RETURNING app_id | ||
| `) |
Contributor
There was a problem hiding this comment.
Claim the row by a unique app key, not app_id alone.
Line 880 updates by app_id, but supabase/functions/_backend/utils/postgres_schema.ts defines apps.id as the unique key and does not make apps.app_id unique. If two orgs share the same bundle id, one iteration can set first_update_delivered_at on both rows and suppress the second org’s event. Select the app row PK here and update by that key instead.
Safer keying
interface AppActivationRow extends Record<string, unknown> {
+ app_pk: string
app_id: string
owner_org: string
app_created_at: string
org_created_by: string | null
activation_date: string
@@
SELECT
+ apps.id AS app_pk,
apps.app_id,
apps.owner_org,
apps.created_at AS app_created_at,
orgs.created_by AS org_created_by,
MIN(dv.date)::date AS activation_date
@@
- const claimed = await drizzleClient.execute<{ app_id: string }>(sql`
+ const claimed = await drizzleClient.execute<{ app_pk: string }>(sql`
UPDATE public.apps
SET first_update_delivered_at = ${activationDate}
- WHERE app_id = ${row.app_id}
+ WHERE id = ${row.app_pk}
AND first_update_delivered_at IS NULL
- RETURNING app_id
+ RETURNING id AS app_pk
`)🤖 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 `@supabase/functions/_backend/triggers/logsnag_insights.ts` around lines 859 -
883, The query and update currently use apps.app_id which is not unique; modify
the initial SELECT (the query executed into result) to also return the primary
key apps.id (e.g., include apps.id in the SELECT/Group By), then in the update
executed via drizzleClient.execute use that apps.id (row.id) in the WHERE clause
instead of row.app_id and RETURNING id (or both id and app_id if you need app_id
later); ensure the variable used when building the UPDATE is row.id (the PK) so
each physical apps row is claimed uniquely.
Comment on lines
+875
to
+913
| for (const row of result.rows ?? []) { | ||
| const activationDate = new Date(row.activation_date) | ||
| const claimed = await drizzleClient.execute<{ app_id: string }>(sql` | ||
| UPDATE public.apps | ||
| SET first_update_delivered_at = ${activationDate} | ||
| WHERE app_id = ${row.app_id} | ||
| AND first_update_delivered_at IS NULL | ||
| RETURNING app_id | ||
| `) | ||
| // Already claimed by a concurrent run; skip to keep the event one-time. | ||
| if (!(claimed.rows ?? []).length) | ||
| continue | ||
|
|
||
| // Backlog drain guard: historical apps (activated long before this column | ||
| // existed) get the flag set silently; only fresh activations emit events. | ||
| const daysSinceActivation = Math.floor((Date.now() - activationDate.getTime()) / (24 * 60 * 60 * 1000)) | ||
| if (daysSinceActivation > 7) | ||
| continue | ||
|
|
||
| const daysSinceAppCreated = Math.max(0, Math.floor((activationDate.getTime() - new Date(row.app_created_at).getTime()) / (24 * 60 * 60 * 1000))) | ||
| await sendEventToTracking(c, { | ||
| channel: 'app-activation', | ||
| event: 'First Device Update Delivered', | ||
| icon: '🚀', | ||
| user_id: row.org_created_by ?? row.owner_org, | ||
| groups: { organization: row.owner_org }, | ||
| tags: { | ||
| app_id: row.app_id, | ||
| owner_org: row.owner_org, | ||
| days_since_app_created: daysSinceAppCreated, | ||
| }, | ||
| setPersonProperties: false, | ||
| notify: false, | ||
| }, { background: false }) | ||
| } | ||
| } | ||
| catch (error) { | ||
| cloudlogErr({ requestId: c.get('requestId'), message: 'trackFirstUpdateDelivered error', error }) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t make the idempotency claim irreversible before the tracking call succeeds.
Lines 877-883 mark the app as processed before sendEventToTracking(..., { background: false }) completes. If PostHog/Bento throws, the outer catch on Line 911 logs and exits, but the row stays claimed, so that activation is lost permanently and the rest of the batch is skipped. This needs per-row error isolation plus a retryable “claimed vs tracked” flow, or an explicit rollback of the claim on send failure.
🤖 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 `@supabase/functions/_backend/triggers/logsnag_insights.ts` around lines 875 -
913, The current loop claims the app by running drizzleClient.execute(...) to
set first_update_delivered_at before calling sendEventToTracking(c,...), which
makes the idempotency irreversible if tracking fails; change the flow so each
row is handled in its own try/catch and only persist the
first_update_delivered_at after sendEventToTracking returns successfully (or if
you must claim first, ensure you rollback the claim on send failure by clearing
first_update_delivered_at via another drizzleClient.execute call), and ensure
failures for one row log via cloudlogErr and continue processing remaining rows
instead of aborting the whole batch.
Matches the CI-generated schema for the new apps column from PR #2491.
The function returns the full apps row via a.*, so adding first_update_delivered_at changed the row shape and broke its declared RETURNS TABLE (pgTAP test 57: 'structure of query does not match function result type'). Drop and recreate it with the column included.
stores/organization.ts now tags PostHog with the org group on org switch, which pulls getLocalConfig (and isLocal via setOrganization) from ~/services/supabase inside the store watch.
…blindspots # Conflicts: # cli/src/bundle/upload.ts
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@cli/src/bundle/upload.ts`:
- Around line 996-998: Fix the duplicated word in the user-facing error string
inside the conditional that checks options.external (the if block using
options.external && !options.external.startsWith('https://')), and update the
uploadFail call to use a corrected message like: "External link should start
with \"https://\"; current is \"${options.external}\"" so the CLI output no
longer contains the repeated "should".
- Around line 1434-1443: The loop that soft-deletes versions when
shouldDeleteLinkedBundle is true can mark app_versions.deleted via
deleteLinkedBundleOnUpload even if the same version is still referenced by other
non-target channels; update the logic in the block that iterates
linkedBundlesToDelete (and related helpers like deleteLinkedBundleOnUpload) to
first check for cross-channel references before soft-deleting: for each
linkedBundle/version, query whether any other channel (outside the current
upload target set) still references that version, only call
deleteLinkedBundleOnUpload and set app_versions.deleted = true when no external
references exist, and ensure you dedupe by version id using deletedVersionIds as
already present to avoid duplicate work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1986c8f-1d00-487f-9eaa-0a4d5ef6a0e0
📒 Files selected for processing (3)
cli/src/bundle/upload.tscli/src/utils.tssrc/main.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
💤 Files with no reviewable changes (1)
- cli/src/utils.ts
|
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@cli/src/bundle/upload.ts`:
- Around line 996-998: Fix the duplicated word in the user-facing error string
inside the conditional that checks options.external (the if block using
options.external && !options.external.startsWith('https://')), and update the
uploadFail call to use a corrected message like: "External link should start
with \"https://\"; current is \"${options.external}\"" so the CLI output no
longer contains the repeated "should".
- Around line 1434-1443: The loop that soft-deletes versions when
shouldDeleteLinkedBundle is true can mark app_versions.deleted via
deleteLinkedBundleOnUpload even if the same version is still referenced by other
non-target channels; update the logic in the block that iterates
linkedBundlesToDelete (and related helpers like deleteLinkedBundleOnUpload) to
first check for cross-channel references before soft-deleting: for each
linkedBundle/version, query whether any other channel (outside the current
upload target set) still references that version, only call
deleteLinkedBundleOnUpload and set app_versions.deleted = true when no external
references exist, and ensure you dedupe by version id using deletedVersionIds as
already present to avoid duplicate work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1986c8f-1d00-487f-9eaa-0a4d5ef6a0e0
📒 Files selected for processing (3)
cli/src/bundle/upload.tscli/src/utils.tssrc/main.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
💤 Files with no reviewable changes (1)
- cli/src/utils.ts
🛑 Comments failed to post (2)
cli/src/bundle/upload.ts (2)
996-998:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicated word in user-facing error text.
Line 997 has
should should; this should be cleaned up for CLI output quality.Suggested fix
- uploadFail(`External link should should start with "https://" current is "${options.external}"`) + uploadFail(`External link should start with "https://"; current value is "${options.external}"`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (options.external && !options.external.startsWith('https://')) { uploadFail(`External link should start with "https://"; current value is "${options.external}"`) }🤖 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 `@cli/src/bundle/upload.ts` around lines 996 - 998, Fix the duplicated word in the user-facing error string inside the conditional that checks options.external (the if block using options.external && !options.external.startsWith('https://')), and update the uploadFail call to use a corrected message like: "External link should start with \"https://\"; current is \"${options.external}\"" so the CLI output no longer contains the repeated "should".
1434-1443:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard cross-channel references before soft-deleting linked versions.
Line 1434-1443 now iterates multiple target channels and calls
deleteLinkedBundleOnUpload, which marksapp_versions.deleted = trueby version id. If that version is still referenced by any non-target channel, this can silently invalidate bundles outside the requested upload scope.Suggested fix
- if (shouldDeleteLinkedBundle) { + if (shouldDeleteLinkedBundle) { const deletedVersionIds = new Set<number>() for (const linkedBundle of linkedBundlesToDelete) { if (!linkedBundle.version || deletedVersionIds.has(linkedBundle.version.id)) continue + const { data: otherChannelRef } = await supabase + .from('channels') + .select('id') + .eq('app_id', appid) + .eq('version', linkedBundle.version.id) + .not('name', 'in', `(${channelsToAssign.map(channel => `"${channel}"`).join(',')})`) + .limit(1) + .maybeSingle() + + if (otherChannelRef) { + log.warn(`Skipping deletion of version ${linkedBundle.version.name} because it is still linked to another channel`) + continue + } if (options.verbose) log.info(`[Verbose] Deleting previously linked bundle in channel ${linkedBundle.channel}...`) await deleteLinkedBundleOnUpload(supabase, linkedBundle.version) deletedVersionIds.add(linkedBundle.version.id) } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (shouldDeleteLinkedBundle) { const deletedVersionIds = new Set<number>() for (const linkedBundle of linkedBundlesToDelete) { if (!linkedBundle.version || deletedVersionIds.has(linkedBundle.version.id)) continue const { data: otherChannelRef } = await supabase .from('channels') .select('id') .eq('app_id', appid) .eq('version', linkedBundle.version.id) .not('name', 'in', `(${channelsToAssign.map(channel => `"${channel}"`).join(',')})`) .limit(1) .maybeSingle() if (otherChannelRef) { log.warn(`Skipping deletion of version ${linkedBundle.version.name} because it is still linked to another channel`) continue } if (options.verbose) log.info(`[Verbose] Deleting previously linked bundle in channel ${linkedBundle.channel}...`) await deleteLinkedBundleOnUpload(supabase, linkedBundle.version) deletedVersionIds.add(linkedBundle.version.id) } }🤖 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 `@cli/src/bundle/upload.ts` around lines 1434 - 1443, The loop that soft-deletes versions when shouldDeleteLinkedBundle is true can mark app_versions.deleted via deleteLinkedBundleOnUpload even if the same version is still referenced by other non-target channels; update the logic in the block that iterates linkedBundlesToDelete (and related helpers like deleteLinkedBundleOnUpload) to first check for cross-channel references before soft-deleting: for each linkedBundle/version, query whether any other channel (outside the current upload target set) still references that version, only call deleteLinkedBundleOnUpload and set app_versions.deleted = true when no external references exist, and ensure you dedupe by version id using deletedVersionIds as already present to avoid duplicate work.
Contributor
🧪 Builder onboarding TUI preview — ✅ passed▶ Open the interactive HTML report (zoomable journey tree + cast playback) Commit: a40da2c · Job summary with the result table |
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 (AI generated)
App Created/Org Createdwere captured in PostHog with the org UUID asdistinct_id(on_app_create.tssentuser_id: ownerOrg). Production data shows the consequence: over the last 90 days, 0 of 1,458 signups could be joined to anApp Createdevent — every signup→activation funnel reads 0%. Both triggers now resolveorgs.created_byas the person (org id stays in$groupsand a newowner_orgtag).First Device Update Delivered: the single most important missing metric ("how many users get the app actually working?") had no event (Update Probed: 9 users/90d). A new nullableapps.first_update_delivered_atcolumn is claimed idempotently by the dailylogsnag_insightscron (joinsdaily_versioninstalls, LIMIT 200/run, primary-DB-safe — NOT in the hot/statspath per the replica contract). Historical apps get the flag backfilled silently; only activations ≤7 days old emit the PostHog event (withdays_since_app_created, org group,setPersonProperties: false).capgo_plugins(sorted@capgo/*list) +capgo_plugin_counttags on init onboarding steps,Doctor Ran, and both upload events — enables the plugin-mix × onboarding-completion correlation. Init steps now carryelapsed_ms, and thecanceledevent finally reportslast_step+ elapsed time.$set_once(todayutm_source=chatgpt.comarrives on /login and is dropped; 85% of signups show$initial_referring_domain=capgo.app);posthog.group('organization', …)on org switch so browser events join org cohorts;user:login-failed/user:signup-failed/user:signup-completedevents with coarse reasons (no PII);step_elapsed_mson dashboard onboarding step events.Motivation (AI generated)
A full audit of Capgo's analytics (6-surface code audit + PostHog production queries) showed the core onboarding/growth questions are currently unanswerable: funnels are split across un-joinable identities, activation is untracked, attribution is lost at first touch, and step timing doesn't exist. 90-day baseline: 1,458 signups → 493 start onboarding (34%) → 203 reach the upload step → 91 complete (6% of signups). This PR closes the highest-impact instrumentation gaps in this repo.
Business Impact (AI generated)
First Device Update Delivered) becomes a trackable north-star metric and a future Bento recovery-email trigger.Test Plan (AI generated)
bun lint/bun lint:backendclean (one pre-existing warning in untouchedcompatibilityEvents.ts)bun typecheck(CLI tsgo + backend tsgo + frontend vue-tsc) cleantests/tracking.unit.test.ts3/3 pass (coverssetPersonPropertiespass-through)test-doctor-analytics,test-v2-event-migration, onboarding telemetry/progress/recovery suites)App Createddistinct_id joins$identifypersons in PostHog (funnel signup→App Created > 0%)First Device Update Deliveredevents appear only for fresh activations andapps.first_update_delivered_atbackfillsstep_elapsed_msand browser events carry theorganizationgroupGenerated with AI
Summary by CodeRabbit