Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new boolean column Changes
Sequence DiagramsequenceDiagram
participant User
participant AcceptInvite as AcceptInvitationFn
participant DB as SupabaseDB
participant LogSnag as LogSnag
participant CloudLog as CloudLog
User->>AcceptInvite: accept invitation
AcceptInvite->>DB: INSERT public.users (created_via_invite=true)
alt column exists
DB-->>AcceptInvite: success
else column missing
DB-->>AcceptInvite: error (missing column)
AcceptInvite->>CloudLog: log missing-column error
AcceptInvite->>DB: INSERT public.users (without created_via_invite)
DB-->>AcceptInvite: success
end
alt created_via_invite is falsy
AcceptInvite->>LogSnag: track signup event
LogSnag-->>AcceptInvite: ack
else created_via_invite is true
Note right of AcceptInvite: skip LogSnag.track for invite signups
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Adds a users.created_via_invite flag to distinguish invite-created accounts from self-signups, then uses it to exclude invited users from the “User Joined” LogSnag event and the daily registration metric.
Changes:
- Add
created_via_inviteboolean column onpublic.users(defaultfalse) with documentation comment. - Set
created_via_invite = truewhen creating thepublic.usersrow via/private/accept_invitation(with a backward-compatible fallback). - Filter
registers_todayand gate “User Joined” tracking based oncreated_via_invite.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260209014020_user_created_via_invite.sql | Adds users.created_via_invite column + column comment for metrics semantics. |
| supabase/functions/_backend/utils/supabase.types.ts | Updates backend-generated Supabase DB types to include created_via_invite. |
| supabase/functions/_backend/triggers/on_user_create.ts | Skips “User Joined” LogSnag event for invite-created users. |
| supabase/functions/_backend/triggers/logsnag_insights.ts | Excludes invite-created users from registers_today metric with fallback behavior. |
| supabase/functions/_backend/private/accept_invitation.ts | Marks users created through invite acceptance with created_via_invite: true (with fallback). |
| src/types/supabase.types.ts | Updates frontend-generated Supabase DB types to include created_via_invite. |
| icon: '🎉', | ||
| user_id: record.id, | ||
| notify: false, | ||
| }).catch() |
There was a problem hiding this comment.
Promise.prototype.catch() without a rejection handler does not swallow errors; await LogSnag.track(...).catch() will still throw if track() rejects. If the intent is best-effort analytics, provide an explicit handler (optionally logging) so a LogSnag outage doesn't fail the user-create trigger.
| }).catch() | |
| }).catch((error) => { | |
| cloudlog({ | |
| requestId: c.get('requestId'), | |
| message: 'LogSnag.track user-register failed', | |
| error, | |
| }) | |
| }) |
| let { error: insertError } = await supabaseAdmin.from('users').insert(insertPayload) | ||
|
|
||
| // Backward compatible rollout: if the column doesn't exist yet, retry without it. | ||
| if (insertError?.message?.toLowerCase().includes('created_via_invite')) { | ||
| const { created_via_invite: _createdViaInvite, ...fallbackPayload } = insertPayload | ||
| ;({ error: insertError } = await supabaseAdmin.from('users').insert(fallbackPayload)) | ||
| } |
There was a problem hiding this comment.
The backward-compat fallback (retry insert without created_via_invite) is silent. When this triggers, it changes metrics semantics and makes it hard to diagnose schema/cache mismatches; consider logging the original insertError (at least once / at debug level) before retrying.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } = await supabaseAdmin.from('users').insert(insertUserPayload).select().single() | ||
|
|
||
| // Backward compatible rollout: if the column doesn't exist yet, retry without it. | ||
| if (userNormalTableError?.message?.toLowerCase().includes('created_via_invite')) { | ||
| const { created_via_invite: _createdViaInvite, ...fallbackPayload } = insertUserPayload | ||
| ;({ error: userNormalTableError, data } = await supabaseAdmin.from('users').insert(fallbackPayload).select().single()) | ||
| } |
There was a problem hiding this comment.
Same as above: the silent fallback on created_via_invite insert errors makes it difficult to detect when invited users are being recorded without the flag. Logging the initial userNormalTableError before retrying would improve observability during rollout.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if (filtered.error?.message?.toLowerCase().includes('created_via_invite')) { | ||
| const legacy = await supabase | ||
| .from('users') | ||
| .select('id', { count: 'exact', head: true }) | ||
| .gte('created_at', last24h) | ||
| if (legacy.error) | ||
| cloudlog({ requestId: c.get('requestId'), message: 'registers_today legacy error', error: legacy.error }) | ||
| return legacy.count ?? 0 | ||
| } |
There was a problem hiding this comment.
When falling back to the legacy registers_today count due to a missing/unknown created_via_invite column, the triggering filtered.error isn’t logged. Logging that error (in addition to any legacy error) would help spot schema/cache rollouts where the filtered metric is temporarily unavailable.
| const insertUserPayload = { | ||
| id: user.user.id, | ||
| email: invitation.email, | ||
| first_name: invitation.first_name, | ||
| last_name: invitation.last_name, | ||
| enable_notifications: true, | ||
| opt_for_newsletters: body.opt_for_newsletters, | ||
| }).select().single() | ||
| created_via_invite: true, | ||
| } | ||
|
|
||
| let { | ||
| error: userNormalTableError, | ||
| data, | ||
| } = await supabaseAdmin.from('users').insert(insertUserPayload).select().single() | ||
|
|
||
| // Backward compatible rollout: if the column doesn't exist yet, retry without it. | ||
| if (userNormalTableError?.message?.toLowerCase().includes('created_via_invite')) { | ||
| const { created_via_invite: _createdViaInvite, ...fallbackPayload } = insertUserPayload | ||
| ;({ error: userNormalTableError, data } = await supabaseAdmin.from('users').insert(fallbackPayload).select().single()) | ||
| } |
There was a problem hiding this comment.
This change introduces new persisted behavior (marking invite-created accounts via created_via_invite) that affects onboarding metrics. There are tests for /private/accept_invitation error cases, but no test covering the successful path asserting the users.created_via_invite flag is set (and remains false for normal signups). Adding a success-path test would prevent regressions.
|
Addressed review comments (AI generated):
|
|
Follow-up on CodeRabbit notes (AI generated):
Docstring-coverage warning appears to be a CodeRabbit pre-merge check artifact (this PR doesn’t add new exported APIs); no repo CI check is failing on that. |
…1607) * Initial plan * feat: log all initial insert errors before retry decision Co-authored-by: riderx <4084527+riderx@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
|
Summary (AI generated)
Exclude invite-created accounts from onboarding registration metrics by introducing
public.users.created_via_invite.Motivation (AI generated)
“User Joined” and daily registrations are currently inflated by invited members (non-technical users, users joining an org that already has apps, and existing users joining additional orgs).
Business Impact (AI generated)
Improves accuracy of onboarding and growth metrics used for product decisions and reporting, without changing user-facing behavior.
Test Plan (AI generated)
bun lint:backend && bun lintINSERT INTO public.users (id, email) VALUES (gen_random_uuid(), 'self@test.com');then verifycreated_via_invite = false.INSERT INTO public.users (id, email, created_via_invite) VALUES (gen_random_uuid(), 'invited@test.com', true);then verifycreated_via_invite = true.Screenshots (AI generated)
N/A
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
New Features
Bug Fixes / Resiliency
Tests