Google Ads integration#4148
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughChangesGoogle Ads integration support is added across OAuth installation, customer and conversion-action configuration, Google Ads API access, lead and sale conversion uploads, installed-workspace tracking, and dashboard wiring. Error metadata logging is centralized, and webhook receiver identification is updated. Google Ads Integration
Error metadata logging
Webhook utility update
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)OAuth Install FlowsequenceDiagram
participant User
participant InstallAction
participant GoogleAdsOAuthProvider
participant CallbackRoute
participant Prisma
User->>InstallAction: request Google Ads install URL
InstallAction->>GoogleAdsOAuthProvider: generateAuthUrl(workspace.id)
GoogleAdsOAuthProvider-->>User: return authorization URL
User->>CallbackRoute: OAuth callback with code
CallbackRoute->>GoogleAdsOAuthProvider: exchange code for tokens
CallbackRoute->>Prisma: persist credentials and settings
CallbackRoute-->>User: redirect to settings
Conversion Upload FlowsequenceDiagram
participant ConversionTracker
participant QueueGoogleAdsConversionUpload
participant QStash
participant UploadRoute
participant UploadGoogleAdsConversion
participant GoogleAdsApi
ConversionTracker->>QueueGoogleAdsConversionUpload: enqueue lead or sale conversion
QueueGoogleAdsConversionUpload->>QStash: publish deduplicated job
QStash->>UploadRoute: POST upload-conversion
UploadRoute->>UploadGoogleAdsConversion: validate payload
UploadGoogleAdsConversion->>GoogleAdsApi: upload click conversion
GoogleAdsApi-->>UploadGoogleAdsConversion: return upload response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/scripts/create-integration.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog message says "created" but upsert can also update.
With
prisma.integration.upsert, this script may update an existing record rather than create a new one. The log message should reflect both cases for clarity during bootstrap runs.♻️ Suggested fix
- console.log(`${integration.name} integration created`, integration); + console.log(`${integration.name} integration upserted`, integration);🤖 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 `@apps/web/scripts/create-integration.ts` at line 33, The log message in create-integration.ts is misleading because prisma.integration.upsert can either create or update an Integration. Update the console.log in the create-integration script to reflect both outcomes, using the integration result from the upsert flow and a neutral message such as “created or updated” so bootstrap logs stay accurate.apps/web/lib/integrations/google-ads/upload-conversion.ts (1)
125-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeftover debug log runs on every conversion upload.
console.log("uploadClickConversion response", response)looks like a debug artifact and will emit the full API response on every successful upload. Remove it or gate it behind a debug flag / lower log level to avoid production log noise.♻️ Proposed removal
- console.log("uploadClickConversion response", response); - const partialFailureError = (response as any)?.partialFailureError;🤖 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 `@apps/web/lib/integrations/google-ads/upload-conversion.ts` at line 125, The uploadClickConversion flow in upload-conversion.ts still emits a leftover debug console.log for every successful upload; remove this logging or guard it behind an explicit debug flag/lower log level in uploadClickConversion so production runs do not print the full API response.apps/web/lib/integrations/google-ads/api.ts (1)
32-64: 🚀 Performance & Scalability | 🔵 TrivialEvery
trackLead/trackSalenow issues an extrainstalledIntegration.findFirsteven for workspaces without Google Ads.This DB lookup runs for every lead/sale (matching the
TODO: How to optimize this call?). Although it's invoked insidewaitUntilside-effects and doesn't block the response, it adds a per-event query across all workspaces. Consider gating it behind a cheaper signal (e.g. a cached workspace flag / Redis set of Google-Ads-enabled workspaces) so the query only runs when the integration is actually installed.Want me to open a follow-up issue or sketch a cached-flag approach?
🤖 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 `@apps/web/lib/integrations/google-ads/api.ts` around lines 32 - 64, The queueGoogleAdsConversionUpload path is doing a per-event installedIntegration.findFirst lookup for every trackLead/trackSale, even when Google Ads is not installed. Move this check behind a cheaper prefilter, such as a cached workspace-level flag or Redis set of Google-Ads-enabled workspaces, and keep the existing qstash.publishJSON flow unchanged. Use queueGoogleAdsConversionUpload, installedIntegration.findFirst, and the GOOGLE_ADS_INTEGRATION_ID/payload.workspaceId gating logic to locate the optimization.apps/web/app/(ee)/api/gad/callback/route.ts (1)
32-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider moving
findFirstOrThrowinside the try-catch for graceful error handling.If the Google Ads integration record doesn't exist in the database (e.g., bootstrap script hasn't been run),
findFirstOrThrowon line 32 throws outside the try-catch block, resulting in an unhandled 500 error. Moving it inside the try block would let the catch handler redirect the user to the login page with a user-friendly error message instead.♻️ Suggested refactor
export const GET = async (req: Request) => { const { searchParams } = new URL(req.url); const session = await getSession(); if (!session?.user.id) { const callbackPath = `/api/gad/callback?${searchParams.toString()}`; redirect(`/login?next=${encodeURIComponent(callbackPath)}`); } - const integration = await prisma.integration.findFirstOrThrow({ - where: { - id: GOOGLE_ADS_INTEGRATION_ID, - }, - select: { - slug: true, - }, - }); - let workspaceSlug: string | null = null; let errorMessage: string | null = null; try { + const integration = await prisma.integration.findFirstOrThrow({ + where: { + id: GOOGLE_ADS_INTEGRATION_ID, + }, + select: { + slug: true, + }, + }); + const { token, contextId: workspaceId } = await googleAdsOAuthProvider.exchangeCodeForToken<string>(req);Note:
integration.slugis used on line 148, so you'd need to scope the variable accordingly (e.g., declare it outside the try or store the slug).🤖 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 `@apps/web/app/`(ee)/api/gad/callback/route.ts around lines 32 - 39, Move the prisma.integration.findFirstOrThrow lookup in the callback route into the existing try-catch so missing Google Ads integration records are handled gracefully instead of throwing an unhandled 500. Make sure the integration slug remains available where it is later used in the route, either by declaring the variable outside the try or extracting and storing the slug from the integration result, and keep the catch path redirecting to login with the user-friendly error handling already in place.
🤖 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 `@apps/web/lib/api/conversions/track-sale.ts`:
- Around line 627-635: The Google Ads upload in queueGoogleAdsConversionUpload
is sending conversionValue in minor units instead of major units. Update the
track-sale conversion payload so the amount passed from amount is converted to
major currency units before queuing the upload, keeping the rest of the
workspaceId, eventType, clickId, conversionDateTime, eventId, and currencyCode
fields unchanged.
In `@apps/web/lib/integrations/google-ads/oauth.ts`:
- Around line 131-142: The Google Ads OAuth setup currently hardcodes the
redirect URI, which breaks authorization outside local development. Update the
googleAdsOAuthProvider configuration in oauth.ts so redirectUri is derived from
an environment-aware value instead of the fixed localhost callback, and ensure
it still points to the correct Google Ads callback route in each environment.
---
Nitpick comments:
In `@apps/web/app/`(ee)/api/gad/callback/route.ts:
- Around line 32-39: Move the prisma.integration.findFirstOrThrow lookup in the
callback route into the existing try-catch so missing Google Ads integration
records are handled gracefully instead of throwing an unhandled 500. Make sure
the integration slug remains available where it is later used in the route,
either by declaring the variable outside the try or extracting and storing the
slug from the integration result, and keep the catch path redirecting to login
with the user-friendly error handling already in place.
In `@apps/web/lib/integrations/google-ads/api.ts`:
- Around line 32-64: The queueGoogleAdsConversionUpload path is doing a
per-event installedIntegration.findFirst lookup for every trackLead/trackSale,
even when Google Ads is not installed. Move this check behind a cheaper
prefilter, such as a cached workspace-level flag or Redis set of
Google-Ads-enabled workspaces, and keep the existing qstash.publishJSON flow
unchanged. Use queueGoogleAdsConversionUpload, installedIntegration.findFirst,
and the GOOGLE_ADS_INTEGRATION_ID/payload.workspaceId gating logic to locate the
optimization.
In `@apps/web/lib/integrations/google-ads/upload-conversion.ts`:
- Line 125: The uploadClickConversion flow in upload-conversion.ts still emits a
leftover debug console.log for every successful upload; remove this logging or
guard it behind an explicit debug flag/lower log level in uploadClickConversion
so production runs do not print the full API response.
In `@apps/web/scripts/create-integration.ts`:
- Line 33: The log message in create-integration.ts is misleading because
prisma.integration.upsert can either create or update an Integration. Update the
console.log in the create-integration script to reflect both outcomes, using the
integration result from the upsert flow and a neutral message such as “created
or updated” so bootstrap logs stay accurate.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54391bad-16d5-4b27-a688-315b00efc940
📒 Files selected for processing (19)
apps/web/.env.exampleapps/web/app/(ee)/api/gad/callback/route.tsapps/web/app/(ee)/api/gad/conversion-actions/route.tsapps/web/app/(ee)/api/gad/upload-conversion/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/integrations/[integrationSlug]/page-client.tsxapps/web/lib/actions/get-integration-install-url.tsapps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/integrations/google-ads/api.tsapps/web/lib/integrations/google-ads/constants.tsapps/web/lib/integrations/google-ads/oauth.tsapps/web/lib/integrations/google-ads/schema.tsapps/web/lib/integrations/google-ads/ui/settings.tsxapps/web/lib/integrations/google-ads/update-google-ads-settings.tsapps/web/lib/integrations/google-ads/upload-conversion.tsapps/web/lib/integrations/install.tsapps/web/lib/webhook/utils.tsapps/web/scripts/create-integration.tspackages/utils/src/constants/integrations.ts
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 `@apps/web/lib/integrations/google-ads/upload-conversion.ts`:
- Around line 39-61: Update extractGoogleAdsClickId to wrap getSearchParams(url)
in a try...catch, returning null when URL parsing fails and logging a warning
with the malformed URL error details. Preserve the existing gclid, gbraid, and
wbraid extraction behavior for valid URLs.
- Around line 147-155: Normalize conversionDateTime before passing it to
googleAdsApi.uploadClickConversion: parse the incoming timestamp string and
format it as Google Ads requires, yyyy-MM-dd HH:mm:ss±HH:mm, rather than
forwarding the toISOString() value with milliseconds and Z. Keep the existing
upload payload and other fields unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4e98f19-008e-404a-beeb-2c9158730676
📒 Files selected for processing (11)
apps/web/.env.exampleapps/web/app/(ee)/api/gad/conversion-actions/route.tsapps/web/lib/actions/get-integration-install-url.tsapps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/integrations/google-ads/api.tsapps/web/lib/integrations/google-ads/constants.tsapps/web/lib/integrations/google-ads/oauth.tsapps/web/lib/integrations/google-ads/update-google-ads-settings.tsapps/web/lib/integrations/google-ads/upload-conversion.tsapps/web/lib/integrations/google-ads/utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/lib/integrations/google-ads/constants.ts
- apps/web/lib/api/conversions/track-lead.ts
- apps/web/app/(ee)/api/gad/conversion-actions/route.ts
- apps/web/lib/api/conversions/track-sale.ts
- apps/web/lib/integrations/google-ads/update-google-ads-settings.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/(ee)/api/gad/callback/route.ts (1)
134-138: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrevent leaking internal error details to the frontend.
Falling back to
error.messagefor any genericErrorcan leak internal stack details to the user via the redirect URL (e.g., Prisma's "No Project found", a stringifiedZodErrorJSON array, or aTypeError).Restrict
error.messagestrictly to knownDubApiErrorinstances.🔒 Proposed fix
} catch (error) { errorMessage = - error instanceof DubApiError || error instanceof Error + error instanceof DubApiError ? error.message : "Failed to connect Google Ads. Please try again."; }🤖 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 `@apps/web/app/`(ee)/api/gad/callback/route.ts around lines 134 - 138, Update the errorMessage assignment in the Google Ads callback error handler to use error.message only for DubApiError instances. For generic Error values and all other thrown values, return the existing safe fallback message instead of exposing internal details through the redirect URL.apps/web/lib/integrations/google-ads/update-google-ads-settings.ts (1)
69-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow
loginCustomerIdin conversion-action validation. The current prefix check only acceptscustomers/${customerId}/conversionActions/, which rejects valid cross-account conversion actions owned by the manager account. Use the providedloginCustomerIdhere, or accept both the selected customer and login account prefixes.🤖 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 `@apps/web/lib/integrations/google-ads/update-google-ads-settings.ts` around lines 69 - 99, Update the conversion-action validation in the customerId block to accept actions prefixed by the provided loginCustomerId as well as the selected customer ID. Reuse the normalized account IDs when constructing valid prefixes, and preserve rejection of actions that match neither account.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/app/`(ee)/api/gad/callback/route.ts:
- Around line 134-138: Update the errorMessage assignment in the Google Ads
callback error handler to use error.message only for DubApiError instances. For
generic Error values and all other thrown values, return the existing safe
fallback message instead of exposing internal details through the redirect URL.
In `@apps/web/lib/integrations/google-ads/update-google-ads-settings.ts`:
- Around line 69-99: Update the conversion-action validation in the customerId
block to accept actions prefixed by the provided loginCustomerId as well as the
selected customer ID. Reuse the normalized account IDs when constructing valid
prefixes, and preserve rejection of actions that match neither account.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 017cc261-1f71-4b95-8943-1dd77f4d89ca
📒 Files selected for processing (13)
apps/web/app/(ee)/api/gad/callback/route.tsapps/web/app/(ee)/api/gad/conversion-actions/route.tsapps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/rewards/queue-reward-processing.tsapps/web/lib/axiom/server.tsapps/web/lib/cron/qstash-workflow.tsapps/web/lib/integrations/google-ads/api.tsapps/web/lib/integrations/google-ads/schema.tsapps/web/lib/integrations/google-ads/ui/settings.tsxapps/web/lib/integrations/google-ads/update-google-ads-settings.tsapps/web/lib/integrations/google-ads/upload-conversion.tsapps/web/lib/partner-referrals/attribute-referring-partner.tsapps/web/lib/upstash/redis-streams/workspace-click-events.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/(ee)/api/gad/conversion-actions/route.ts
- apps/web/lib/integrations/google-ads/schema.ts
- apps/web/lib/integrations/google-ads/upload-conversion.ts
- apps/web/lib/api/conversions/track-lead.ts
- apps/web/lib/integrations/google-ads/ui/settings.tsx
- apps/web/lib/integrations/google-ads/api.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/integrations/google-ads/upload-conversion.ts (1)
163-173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize Google Ads timestamps before uploading.
The
conversionDateTimestring comes fromnew Date().toISOString(), which produces a string in the formatYYYY-MM-DDTHH:mm:ss.sssZ. However, the Google Ads API strictly requires theyyyy-mm-dd hh:mm:ss+|-hh:mmformat. You should format it correctly before executing the upload request.🛡️ Proposed fix
+ // Convert ISO 8601 (e.g. 2024-05-01T12:00:00.000Z) to Google Ads format (yyyy-mm-dd hh:mm:ss+|-hh:mm) + const formattedConversionDateTime = conversionDateTime.includes("T") + ? conversionDateTime.replace("T", " ").substring(0, 19) + "+00:00" + : conversionDateTime; + const response = await googleAdsApi.uploadClickConversion({ customerId: settings.customerId, conversionAction, googleClickId, - conversionDateTime, + conversionDateTime: formattedConversionDateTime, conversionValue, currencyCode, conversionCount, eventId, });🤖 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 `@apps/web/lib/integrations/google-ads/upload-conversion.ts` around lines 163 - 173, Normalize conversionDateTime before the googleAdsApi.uploadClickConversion call in the upload conversion flow. Convert the ISO timestamp from new Date().toISOString() into Google Ads’ required yyyy-mm-dd hh:mm:ss+|-hh:mm format, removing milliseconds and replacing the T separator and trailing Z with the required space and UTC offset.
🧹 Nitpick comments (1)
apps/web/lib/integrations/google-ads/upload-conversion.ts (1)
175-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove debug logging.
Consider removing the
console.logleft behind from debugging, or convert it to a structuredlogger.infolog if keeping this visibility is necessary.♻️ Proposed fix
- console.log("uploadClickConversion response", response); - return response;🤖 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 `@apps/web/lib/integrations/google-ads/upload-conversion.ts` around lines 175 - 177, Remove the console.log statement in the uploadClickConversion response handling. If visibility into the response is needed for monitoring or debugging, replace it with a structured logger.info call instead of using console.log. The statement to remove or replace is the console.log("uploadClickConversion response", response) line that appears before the return statement.
🤖 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 `@apps/web/lib/integrations/google-ads/upload-conversion.ts`:
- Around line 18-40: Wrap the getSearchParams call and click-ID extraction logic
in extractGoogleAdsClickId with try...catch; return null from the catch when URL
parsing throws, while preserving the existing gclid, gbraid, wbraid, and
no-match behavior.
---
Outside diff comments:
In `@apps/web/lib/integrations/google-ads/upload-conversion.ts`:
- Around line 163-173: Normalize conversionDateTime before the
googleAdsApi.uploadClickConversion call in the upload conversion flow. Convert
the ISO timestamp from new Date().toISOString() into Google Ads’ required
yyyy-mm-dd hh:mm:ss+|-hh:mm format, removing milliseconds and replacing the T
separator and trailing Z with the required space and UTC offset.
---
Nitpick comments:
In `@apps/web/lib/integrations/google-ads/upload-conversion.ts`:
- Around line 175-177: Remove the console.log statement in the
uploadClickConversion response handling. If visibility into the response is
needed for monitoring or debugging, replace it with a structured logger.info
call instead of using console.log. The statement to remove or replace is the
console.log("uploadClickConversion response", response) line that appears before
the return statement.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a73ee0de-e0c3-4311-8c3a-8c5573a4b399
📒 Files selected for processing (15)
apps/web/app/(ee)/api/cron/google-ads/sync-installed-workspaces/route.tsapps/web/app/(ee)/api/google-ads/callback/route.tsapps/web/app/(ee)/api/google-ads/conversion-actions/route.tsapps/web/app/(ee)/api/google-ads/upload-conversion/route.tsapps/web/app/api/integrations/uninstall/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/integrations/[integrationSlug]/page-client.tsxapps/web/lib/api/conversions/track-lead.tsapps/web/lib/api/conversions/track-sale.tsapps/web/lib/integrations/google-ads/api.tsapps/web/lib/integrations/google-ads/installed-workspaces.tsapps/web/lib/integrations/google-ads/oauth.tsapps/web/lib/integrations/google-ads/schema.tsapps/web/lib/integrations/google-ads/ui/settings.tsxapps/web/lib/integrations/google-ads/upload-conversion.tsapps/web/vercel.json
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/web/vercel.json
- apps/web/lib/api/conversions/track-lead.ts
- apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/integrations/[integrationSlug]/page-client.tsx
- apps/web/lib/integrations/google-ads/schema.ts
- apps/web/lib/api/conversions/track-sale.ts
- apps/web/lib/integrations/google-ads/installed-workspaces.ts
- apps/web/app/api/integrations/uninstall/route.ts
- apps/web/lib/integrations/google-ads/ui/settings.tsx
- apps/web/lib/integrations/google-ads/oauth.ts
- apps/web/lib/integrations/google-ads/api.ts
Summary by CodeRabbit