Repository navigation
feat(analytics): report account signups to X Ads - #9212
waleedlatif1 wants to merge 2 commits into
Conversation
|
@cubic review |
|
@greptile |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
[Medium impact] Adds signup conversion tracking to X Ads and other vendors. The previous testing and component requirements still need to be met before merging; no new issue was found. FindingsSummaryReports completed signups to X Ads when marketing consent allows it.
|
| const finishSignUp = async () => { | ||
| if (session?.user && isSocialSignInProvider(provider)) { | ||
| await trackSocialSignUpConversions({ | ||
| marker, | ||
| provider, | ||
| userId: session.user.id, | ||
| measurement: consentRequired && measurement, | ||
| marketing: consentRequired && marketing, | ||
| }) | ||
| } | ||
| window.location.href = destination | ||
| } |
There was a problem hiding this comment.
SocialSignUpCompletion makes the user's redirect depend on outside tracking code. If a vendor function throws, window.location.href never runs, and redirectStartedRef prevents another attempt. Put the redirect in a finally block so a tracking error cannot leave a signed-up user on “Finishing signup…”.
| const finishSignUp = async () => { | |
| if (session?.user && isSocialSignInProvider(provider)) { | |
| await trackSocialSignUpConversions({ | |
| marker, | |
| provider, | |
| userId: session.user.id, | |
| measurement: consentRequired && measurement, | |
| marketing: consentRequired && marketing, | |
| }) | |
| } | |
| window.location.href = destination | |
| } | |
| const finishSignUp = async () => { | |
| try { | |
| if (session?.user && isSocialSignInProvider(provider)) { | |
| await trackSocialSignUpConversions({ | |
| marker, | |
| provider, | |
| userId: session.user.id, | |
| measurement: consentRequired && measurement, | |
| marketing: consentRequired && marketing, | |
| }) | |
| } | |
| } finally { | |
| window.location.href = destination | |
| } | |
| } |
| const trackXEvent = vi.fn() | ||
| const trackGoogleEvent = vi.fn() | ||
| const trackFreebuffEvent = vi.fn() | ||
| window.twq = trackXEvent | ||
| window.gtag = trackGoogleEvent | ||
| window.freebuff = trackFreebuffEvent |
There was a problem hiding this comment.
The social signup test replaces every vendor function with a mock and checks their calls. CLAUDE.md explicitly forbids tests that mock every collaborator and assert toHaveBeenCalledWith. Replace this with a test through the real signup boundary that checks consent, one-time reporting, and arrival at the requested page. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
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!
| import { useConsentScript } from '@c15t/nextjs/headless' | ||
| import { X_PIXEL_SCRIPT } from '@/lib/consent/scripts' | ||
|
|
||
| export function SignupConsentTracking() { |
There was a problem hiding this comment.
Required props interfaces are missing
SignupConsentTracking has no props interface. The component guide requires one for every component. Add an explicit props interface here and to the new SocialSignUpCompletePage, which has the same omission. This repository requirement must be satisfied before merging.
Context Used: Component patterns and structure for React components (source)
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!
|
@cubic review |
|
@greptile |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 11 files
Confidence score: 3/5
- A tracking error in
social-sign-up-completion.tsxcan leave a signed-up user stuck on “Finishing signup…” instead of redirecting. Move the redirect into afinallyblock.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/app/(auth)/signup/complete/social-sign-up-completion.tsx">
<violation number="1" location="apps/sim/app/(auth)/signup/complete/social-sign-up-completion.tsx:37">
P2: Put the redirect in a `finally` block so a tracking error cannot leave a signed-up user stuck on “Finishing signup…”.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| await trackSignUpCompletionConversions({ | ||
| marker, | ||
| method: provider, | ||
| userId: session.user.id, | ||
| measurement: consentRequired && measurement, | ||
| marketing: consentRequired && marketing, | ||
| }) | ||
| } | ||
| window.location.href = destination | ||
| } |
There was a problem hiding this comment.
P2: Put the redirect in a finally block so a tracking error cannot leave a signed-up user stuck on “Finishing signup…”.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/app/(auth)/signup/complete/social-sign-up-completion.tsx, line 37:
<comment>Put the redirect in a `finally` block so a tracking error cannot leave a signed-up user stuck on “Finishing signup…”.</comment>
<file context>
@@ -0,0 +1,62 @@
+
+ const finishSignUp = async () => {
+ if (session?.user && isSignUpConversionMethod(provider)) {
+ await trackSignUpCompletionConversions({
+ marker,
+ method: provider,
</file context>
| await trackSignUpCompletionConversions({ | |
| marker, | |
| method: provider, | |
| userId: session.user.id, | |
| measurement: consentRequired && measurement, | |
| marketing: consentRequired && marketing, | |
| }) | |
| } | |
| window.location.href = destination | |
| } | |
| try { | |
| if (session?.user && isSignUpConversionMethod(provider)) { | |
| await trackSignUpCompletionConversions({ | |
| marker, | |
| method: provider, | |
| userId: session.user.id, | |
| measurement: consentRequired && measurement, | |
| marketing: consentRequired && marketing, | |
| }) | |
| } | |
| } finally { | |
| window.location.href = destination | |
| } |
Summary
tw-q5xbl-rhis8), so X Ads campaigns can optimize for signups.sign_uplabel will follow separately.Type of Change
Testing
See agent notes below and CI.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01YXGD8yrPh6YUMvcLKFPQMs