feat: Google Ads connector#585
Open
Blaumaus wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a Cloud-only Google Ads integration with OAuth connection, encrypted token storage, campaign synchronization, analytics APIs, dashboard views, campaign attribution, database migrations, frontend settings, and documentation. ChangesGoogle Ads Integration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectSettings
participant AdsController
participant AdsService
participant GoogleAdsAdapter
participant Google
User->>ProjectSettings: start Google Ads connection
ProjectSettings->>AdsController: request connect URL
AdsController->>AdsService: generateConnectURL
AdsService-->>ProjectSettings: OAuth URL
ProjectSettings->>Google: authorize
Google-->>AdsController: OAuth callback
AdsController->>AdsService: handleOAuthCallback
User->>AdsController: select account
AdsController->>GoogleAdsAdapter: list accounts and currency
AdsController->>AdsService: persist account and start backfill
sequenceDiagram
participant AdsView
participant AdsAnalyticsController
participant AdsService
participant ClickHouse
AdsView->>AdsAnalyticsController: request dashboard data
AdsAnalyticsController->>AdsService: fetch stats and chart
AdsService->>ClickHouse: query metrics, events, and revenue
ClickHouse-->>AdsService: aggregated data
AdsService-->>AdsView: dashboard payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/integrations/google-ads.mdx (1)
1-92: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix formatting to pass CI.
Pipeline reports oxfmt --check formatting issues in this file. Run the formatter locally and commit the result.
npx oxfmt docs/content/docs/integrations/google-ads.mdx🤖 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 `@docs/content/docs/integrations/google-ads.mdx` around lines 1 - 92, The Google Ads docs page needs formatting fixes to satisfy oxfmt --check. Run the formatter on the Google Ads MDX content and commit the resulting formatting-only changes, keeping the existing content intact; use the doc content in the google-ads.mdx file as the target for the cleanup.Source: Pipeline failures
🧹 Nitpick comments (7)
web/app/pages/Project/tabs/Traffic/TrafficView.tsx (1)
492-510: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAds campaign map effect re-fetches on unrelated URL param changes.
Depending on the whole
searchParamsobject (instead of just thefrom/tovalues actually used) causes this effect to re-run — and hit the Ads backend — whenever any unrelated search param changes (filters, compare toggles, etc.) while the "ca" sub-tab happens to be active.⚡ Suggested fix
+ const adsFrom = searchParams.get('from') || undefined + const adsTo = searchParams.get('to') || undefined + useEffect(() => { if (!id || isSelfhosted || panelsActiveTabs.source !== 'ca') { return } fetchCampaignMap(id, { period, - from: searchParams.get('from') || undefined, - to: searchParams.get('to') || undefined, + from: adsFrom, + to: adsTo, timezone, }) }, [ id, panelsActiveTabs.source, period, timezone, fetchCampaignMap, - searchParams, + adsFrom, + adsTo, ])🤖 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 `@web/app/pages/Project/tabs/Traffic/TrafficView.tsx` around lines 492 - 510, The campaign map effect in TrafficView re-runs on unrelated URL changes because it depends on the whole searchParams object instead of only the values it actually reads. Update the useEffect dependencies around fetchCampaignMap to track just the derived from/to values (or memoized equivalents) along with id, panelsActiveTabs.source, period, timezone, and fetchCampaignMap, so the Ads backend is only hit when those relevant inputs change.web/app/pages/Project/Settings/ProjectSettings.tsx (2)
1530-1534: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSync error banner discards the actual error message.
adsSyncErroris populated with the real error text from the backend, but the banner only renders a generic static string and never interpolatesadsSyncErroritself, hiding useful diagnostic info from users trying to fix a broken sync.{adsSyncError ? ( <div className='rounded-md border border-amber-200 bg-amber-50 p-3 text-sm text-amber-900 dark:border-amber-900/60 dark:bg-amber-950/40 dark:text-amber-100'> - {t('project.settings.ads.syncError')} + {t('project.settings.ads.syncError', { error: adsSyncError })} </div> ) : null}🤖 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 `@web/app/pages/Project/Settings/ProjectSettings.tsx` around lines 1530 - 1534, The sync error banner in ProjectSettings is ignoring the real backend error stored in adsSyncError and only showing a generic translation key. Update the render block that checks adsSyncError to display the actual adsSyncError value (optionally alongside the localized label) so users can see the concrete failure reason. Keep the change localized to the banner JSX in ProjectSettings and preserve the existing conditional display logic.
656-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate Google OAuth URL validation logic.
The
accounts.google.comsafe-URL check is duplicated verbatim between the GSC and Ads fetcher effects (and will likely be copy-pasted again for future providers). Consider extracting a sharedgetSafeGoogleAuthUrl(url: string)helper.♻️ Suggested extraction
const getSafeGoogleAuthUrl = (url: string): string | null => { try { const parsed = new URL(url) if (parsed.protocol !== 'https:') return null if (parsed.username || parsed.password) return null if (parsed.hostname !== 'accounts.google.com') return null return parsed.toString() } catch { return null } }🤖 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 `@web/app/pages/Project/Settings/ProjectSettings.tsx` around lines 656 - 677, The Google OAuth URL validation is duplicated in the ProjectSettings effects, so extract the shared hostname/protocol checks into a reusable getSafeGoogleAuthUrl helper and use it from the ads-connect flow and the matching GSC path. Keep the helper in a nearby shared location within ProjectSettings.tsx (or a local utilities module) and replace the inline URL parsing blocks with calls to that helper so future provider flows reuse the same logic.backend/migrations/mysql/2026_07_07_google_ads.sql (1)
1-11: 🚀 Performance & Scalability | 🔵 TrivialConsider an index to support the cron polling query.
The scheduled sync job (
task-manager.service.ts) filters projects bygoogleAdsRefreshTokenEnc IS NOT NULL AND googleAdsCustomerId IS NOT NULL AND googleAdsSyncError IS NULLevery 6 hours. Without a supporting index, this becomes a full table scan onprojectas the table grows. Consider adding a composite/partial index covering these columns (e.g.,googleAdsCustomerId, googleAdsSyncError).🤖 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 `@backend/migrations/mysql/2026_07_07_google_ads.sql` around lines 1 - 11, Add an index to the new Google Ads columns in this migration so the cron polling query in task-manager.service.ts does not scan the full project table. Update the ALTER TABLE for project to include a composite index that supports filtering by googleAdsCustomerId, googleAdsRefreshTokenEnc, and googleAdsSyncError (or the closest practical index for the MySQL version used), so the scheduled sync job can efficiently find eligible projects.backend/apps/cloud/src/ads/ads.service.ts (1)
496-588: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider narrowing the event-type scan in campaign session/revenue attribution queries.
Neither
getSessionsPerCampaignValuenorgetRevenuePerCampaignValuefiltersevents.type, unlike most other analytics queries in this file/service. Sinceso/me/caattribution fields are session-scoped (identical across a session's rows), restricting totype IN ('pageview', 'custom_event')would reduce scanned rows without changing theuniqExact(psid)/attribution results.🤖 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 `@backend/apps/cloud/src/ads/ads.service.ts` around lines 496 - 588, The campaign attribution queries in getSessionsPerCampaignValue and getRevenuePerCampaignValue are scanning all event types even though only session-scoped attribution rows are needed. Add an events.type filter (matching the other analytics queries, e.g. pageview/custom_event) to both SQL statements in AdsService so the ClickHouse scan is narrower while preserving the uniqExact(psid) and revenue attribution results.backend/apps/cloud/src/analytics/analytics.service.ts (1)
7123-7175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate campaign-matching logic with
AdsService.findCampaignByUtmValue.This private method reimplements the exact same
ad_metricsDISTINCT query andnormalizeCampaignKeymatching found inAdsService.findCampaignByUtmValue(backend/apps/cloud/src/ads/ads.service.ts, lines 894-938).AnalyticsServicedoesn't injectAdsService, so the two implementations will need to be kept in sync manually going forward.🤖 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 `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 7123 - 7175, The `findAdCampaignByUtm` method is duplicating the same `ad_metrics` lookup and `normalizeCampaignKey` matching already implemented in `AdsService.findCampaignByUtmValue`. Refactor `AnalyticsService` to reuse that existing logic instead of maintaining a separate copy, either by injecting `AdsService` or by extracting a shared helper used by both methods. Keep the return shape in `findAdCampaignByUtm` consistent by mapping from the shared result if needed.backend/apps/cloud/src/ads/ads.controller.ts (1)
104-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant runtime check.
SelectAdsAccountDto.customerIdis already validated via@IsNotEmpty()and@Matches(/^\d{1,20}$/), so a request missingcustomerIdshould already be rejected by the globalValidationPipebefore reaching this handler, making this check effectively dead code.🤖 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 `@backend/apps/cloud/src/ads/ads.controller.ts` around lines 104 - 106, The `AdsController` handler is duplicating validation that `SelectAdsAccountDto.customerId` already enforces through class-validator and the global `ValidationPipe`. Remove the explicit `if (!body?.customerId)` guard from the controller method that processes the request body, and rely on the DTO validation in `SelectAdsAccountDto` to reject missing or invalid `customerId` values before the handler runs.
🤖 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 `@backend/apps/cloud/src/ads/adapters/google-ads.adapter.ts`:
- Around line 242-243: The currency normalization in google-ads.adapter’s
account conversion flow silently defaults to USD when googleAdsCurrency or
revenueCurrency is missing, which can misstate values for non-USD accounts.
Update the logic around the originalCurrency and targetCurrency handling to
detect unknown/missing account currency, avoid assuming USD in that case, and
surface a clear warning or error path so the google ads conversion step does not
proceed with incorrect currency assumptions.
- Around line 86-126: Add request timeouts to the Google Ads fetch calls and
consider retry/backoff for transient failures. In `GoogleAdsAdapter`, update
both `search()` and `listAccessibleAccounts()` to use an `AbortController` (or
equivalent timeout wrapper) so `fetch()` cannot hang indefinitely when
`syncCampaignMetrics` or the account lookup runs inside an HTTP request. Also
handle 429/5xx responses with a small retry strategy before throwing, while
preserving the existing error parsing and pagination behavior in `search()`.
- Around line 242-267: The per-row calls to currencyService.convert() inside the
results loop are doing repeated async conversion work for fixed currencies. In
the sync logic in google-ads.adapter.ts, resolve the conversion once before
iterating over results using originalCurrency and targetCurrency, then apply
that rate locally to each row for cost and conversionsValue; keep the
same-currency behavior unchanged since CurrencyService already no-ops there.
In `@backend/apps/cloud/src/ads/ads-analytics.controller.ts`:
- Around line 39-49: The access check in getViewableProject is missing the
x-password header, so password-protected projects cannot be validated through
the Ads analytics endpoints. Update getViewableProject in the Ads analytics
controller to accept the request headers and pass headers['x-password'] into
projectService.allowedToView alongside project and userId, matching the other
project-view paths used by /log/ads, /log/ads/campaigns, and
/log/ads/campaign-map.
In `@backend/apps/cloud/src/ads/ads.controller.ts`:
- Around line 146-181: The sync endpoint in AdsController currently awaits
googleAdsAdapter.syncCampaignMetrics directly on the request path without any
timeout, so a slow upstream can hang the request indefinitely. Update the sync()
flow to call syncCampaignMetrics through a timeout/abort mechanism (using the
existing adsService/googleAdsAdapter integration points) and make sure the
timeout is handled cleanly before proceeding to updateLastSyncAt. Keep the fix
localized around sync() and syncCampaignMetrics so the request fails fast
instead of waiting forever.
In `@backend/apps/cloud/src/ads/ads.service.ts`:
- Around line 196-207: The Google userinfo lookup in ads.service.ts can hang the
OAuth callback because the fetch has no abort path. Update the fetch call inside
the accountEmail try block to pass an AbortSignal.timeout(...) signal so the
request fails fast if Google is slow, and keep the existing error handling in
place; use the fetch call around the userinfo endpoint and the accountEmail
assignment as the main location to change.
- Around line 661-723: The comparison ranges in getAdsStats overlap because
previousFrom uses groupFrom as the previous window’s upper boundary, which
causes the boundary day to be counted in both current and previous metrics.
Update getAdsStats to compute a non-overlapping previousTo first, then derive
previousFrom from that so the current and previous calls to getCampaignRows use
distinct ranges. Keep the change localized around the periodDays calculation and
the Promise.all inputs.
In `@backend/apps/cloud/src/ads/interfaces/ads.interface.ts`:
- Around line 64-73: AdsCampaignSummary is exported but currently unused, which
triggers knip in CI. Either remove the unused export from ads.interface.ts if it
is dead, or wire AdsCampaignSummary into the relevant ads.service.ts
campaign-summary/campaign-map flow by using it as the explicit type for the
returned summary object so the export is actually consumed.
In `@backend/apps/cloud/src/project/entity/project.entity.ts`:
- Around line 174-206: Add a composite index on the Google Ads sync-eligibility
fields used by syncAdsData so the cron doesn’t scan the full project table each
run. Update ProjectEntity to define an index over googleAdsRefreshTokenEnc,
googleAdsCustomerId, and googleAdsSyncError (or the exact filter set used by the
sync query), using the existing entity decorators so the database can support
the lookup efficiently.
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 1523-1553: The syncAdsData loop in task-manager.service.ts is
launching syncCampaignMetrics for all projects at once, which can overwhelm
Google Ads quotas. Update the project iteration to use a bounded concurrency
pattern like the existing mapLimit-based loops in verifyPendingProxyDomains and
recheckLiveProxyDomains, and keep the current per-project try/catch,
markSyncError, and logger.error behavior inside the limited worker.
In `@docs/content/docs/analytics-dashboard/revenue-tracking.mdx`:
- Around line 341-345: The “Ad campaign ROI” paragraph contains a stale
cross-reference to “shown above” for swetrix_session_id that does not exist in
the nearby Stripe/Paddle examples. Update the copy in revenue-tracking.mdx so it
no longer points readers to a nonexistent example, and instead clearly state
that sessionId on the API or swetrix_session_id in metadata should be provided
to trace purchases; keep the wording aligned with the existing Google Ads and
Ads dashboard section.
In `@web/app/api/api.server.ts`:
- Around line 2989-3019: Update getAdsCampaignMapServer to pass an explicit
timeoutMs of 60000 in its serverFetch options so it matches
getAdsDashboardServer and getAdsCampaignsServer. Keep the existing query param
and header handling in place, and add the timeout alongside headers in the
serverFetch call.
In `@web/app/hooks/useAnalyticsProxy.ts`:
- Around line 1187-1189: The campaign-map fetch in useAnalyticsProxy is
swallowing failures by returning null from the catch block without any error
reporting. Update the fetch/error handling in useAnalyticsProxy so it captures
the thrown error, stores a readable error state like the other proxy hooks (for
example alongside the existing useAdsDashboardProxy/useGSCDashboardProxy
pattern), and exposes that state to consumers instead of only returning null.
Keep the catch/finally flow intact, but ensure the hook can surface the failure
through a returned error value or equivalent state.
- Around line 1164-1197: useAdsCampaignMapProxy currently updates map and
currency without guarding against out-of-order responses, so an older
fetchCampaignMap call can overwrite newer state. Add the same stale-response
protection pattern used by useAdsDashboardProxy and useGSCDashboardProxy in this
file: introduce a requestIdRef (or equivalent), increment it for each
fetchCampaignMap invocation, and only apply setMap/setCurrency when the response
matches the latest request before clearing isLoading.
In `@web/app/pages/Project/Settings/ProjectSettings.tsx`:
- Around line 706-712: Gate the entire Google Ads settings section in
ProjectSettings so it is hidden for self-hosted installs, matching the other
Cloud-only integrations. Update the Ads-related render/visibility logic around
the ads section and the ads-status bootstrap fetch so it only runs when
!isSelfhosted, and make sure the error/reset path in the ads-status handling
also clears adsAvailable so the section does not fall through to the connect
prompt. Use the existing isSelfhosted checks and the Ads-related state handlers
in ProjectSettings as the main anchors for the fix.
In `@web/app/pages/Project/tabs/Ads/AdsView.tsx`:
- Around line 450-503: The MetricCard trend indicators in AdsView are reversed
for several metrics. Update the goodChangeDirection props so the spend card uses
'down', while the clicks, adSessions, and attributedRevenue cards use 'up'; keep
the existing valueMapper and change calculations intact. Use the MetricCard
usages in AdsView to locate and swap only these flags.
In `@web/app/routes/ads-connected.tsx`:
- Around line 87-155: AdsHashHandler is rendering the “Invalid callback
parameters” error before the hash callback values are safely available, which
can cause SSR/hydration mismatches. Move the URL parsing out of the initial
useState render path or add a readiness flag in AdsHashHandler so state/code are
only evaluated after client-side parsing completes, and only render the error
StatusPage once parsing has finished and the params are truly missing.
- Around line 42-60: Restrict the fallback in ads-connected flow so
`processAdsToken(code, state)` is only retried for failures that are definitely
before the authorization code is redeemed; update the `serverFetch` handling in
the ads-connected route to stop treating generic server errors as safe
client-side retries, or have the backend return an explicit “already exchanged”
signal. Also adjust the hash-redirect branch in the same route so the server
does not render “Invalid callback parameters” before the client can read the
fragment, preventing the hydration swap and incorrect intermediate state.
In `@web/public/locales/en.json`:
- Around line 2270-2282: The Google Ads connect CTA copy in the ads localization
entry overstates what connection alone enables by implying ROI is immediately
available; update the text in the ads connect message to qualify that attributed
metrics depend on Revenue Tracking and session IDs, while keeping the change
localized to the relevant string key in the ads section.
- Around line 2902-2918: The ads localization string for attribution currently
only mentions the payment-provider metadata flow, so update attributionNote in
the ads section to describe both supported revenue attribution paths: passing
the Swetrix session ID to a payment provider and sending sessionId directly in
the revenue API request body. Reword the copy so it is API-centric and still
references utm_campaign matching for campaigns.
---
Outside diff comments:
In `@docs/content/docs/integrations/google-ads.mdx`:
- Around line 1-92: The Google Ads docs page needs formatting fixes to satisfy
oxfmt --check. Run the formatter on the Google Ads MDX content and commit the
resulting formatting-only changes, keeping the existing content intact; use the
doc content in the google-ads.mdx file as the target for the cleanup.
---
Nitpick comments:
In `@backend/apps/cloud/src/ads/ads.controller.ts`:
- Around line 104-106: The `AdsController` handler is duplicating validation
that `SelectAdsAccountDto.customerId` already enforces through class-validator
and the global `ValidationPipe`. Remove the explicit `if (!body?.customerId)`
guard from the controller method that processes the request body, and rely on
the DTO validation in `SelectAdsAccountDto` to reject missing or invalid
`customerId` values before the handler runs.
In `@backend/apps/cloud/src/ads/ads.service.ts`:
- Around line 496-588: The campaign attribution queries in
getSessionsPerCampaignValue and getRevenuePerCampaignValue are scanning all
event types even though only session-scoped attribution rows are needed. Add an
events.type filter (matching the other analytics queries, e.g.
pageview/custom_event) to both SQL statements in AdsService so the ClickHouse
scan is narrower while preserving the uniqExact(psid) and revenue attribution
results.
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 7123-7175: The `findAdCampaignByUtm` method is duplicating the
same `ad_metrics` lookup and `normalizeCampaignKey` matching already implemented
in `AdsService.findCampaignByUtmValue`. Refactor `AnalyticsService` to reuse
that existing logic instead of maintaining a separate copy, either by injecting
`AdsService` or by extracting a shared helper used by both methods. Keep the
return shape in `findAdCampaignByUtm` consistent by mapping from the shared
result if needed.
In `@backend/migrations/mysql/2026_07_07_google_ads.sql`:
- Around line 1-11: Add an index to the new Google Ads columns in this migration
so the cron polling query in task-manager.service.ts does not scan the full
project table. Update the ALTER TABLE for project to include a composite index
that supports filtering by googleAdsCustomerId, googleAdsRefreshTokenEnc, and
googleAdsSyncError (or the closest practical index for the MySQL version used),
so the scheduled sync job can efficiently find eligible projects.
In `@web/app/pages/Project/Settings/ProjectSettings.tsx`:
- Around line 1530-1534: The sync error banner in ProjectSettings is ignoring
the real backend error stored in adsSyncError and only showing a generic
translation key. Update the render block that checks adsSyncError to display the
actual adsSyncError value (optionally alongside the localized label) so users
can see the concrete failure reason. Keep the change localized to the banner JSX
in ProjectSettings and preserve the existing conditional display logic.
- Around line 656-677: The Google OAuth URL validation is duplicated in the
ProjectSettings effects, so extract the shared hostname/protocol checks into a
reusable getSafeGoogleAuthUrl helper and use it from the ads-connect flow and
the matching GSC path. Keep the helper in a nearby shared location within
ProjectSettings.tsx (or a local utilities module) and replace the inline URL
parsing blocks with calls to that helper so future provider flows reuse the same
logic.
In `@web/app/pages/Project/tabs/Traffic/TrafficView.tsx`:
- Around line 492-510: The campaign map effect in TrafficView re-runs on
unrelated URL changes because it depends on the whole searchParams object
instead of only the values it actually reads. Update the useEffect dependencies
around fetchCampaignMap to track just the derived from/to values (or memoized
equivalents) along with id, panelsActiveTabs.source, period, timezone, and
fetchCampaignMap, so the Ads backend is only hit when those relevant inputs
change.
🪄 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: f7e8f8c7-d7b1-4c79-8c78-6746f22b8726
📒 Files selected for processing (46)
backend/.env.examplebackend/apps/cloud/src/ads/adapters/google-ads.adapter.tsbackend/apps/cloud/src/ads/ads-analytics.controller.tsbackend/apps/cloud/src/ads/ads.controller.tsbackend/apps/cloud/src/ads/ads.module.tsbackend/apps/cloud/src/ads/ads.service.tsbackend/apps/cloud/src/ads/dto/get-ads.dto.tsbackend/apps/cloud/src/ads/dto/select-account.dto.tsbackend/apps/cloud/src/ads/interfaces/ads.interface.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/app.module.tsbackend/apps/cloud/src/common/utils.tsbackend/apps/cloud/src/project/entity/project.entity.tsbackend/apps/cloud/src/task-manager/task-manager.module.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/migrations/clickhouse/2026_07_07_ad_metrics.jsbackend/migrations/clickhouse/initialise_database.jsbackend/migrations/mysql/2026_07_07_google_ads.sqldocs/components/IntegrationsGrid.tsxdocs/content/docs/analytics-dashboard/ads.mdxdocs/content/docs/analytics-dashboard/meta.jsondocs/content/docs/analytics-dashboard/revenue-tracking.mdxdocs/content/docs/integration-guides.mdxdocs/content/docs/integrations/google-ads.mdxdocs/content/docs/integrations/meta.jsondocs/content/docs/introduction.mdxdocs/content/docs/traffic-sources.mdxdocs/lib/source.tsweb/app/api/api.server.tsweb/app/hooks/useAnalyticsProxy.tsweb/app/hooks/useAuthProxy.tsweb/app/lib/constants/index.tsweb/app/lib/models/Project.tsweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/tabs/Ads/AdsView.tsxweb/app/pages/Project/tabs/Ads/ads-chart-options.tsweb/app/pages/Project/tabs/Profiles/ProfileDetails.tsxweb/app/pages/Project/tabs/Sessions/SessionDetailView.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsxweb/app/routes/ads-connected.tsxweb/app/routes/api.analytics.tsweb/app/routes/api.auth.tsweb/app/routes/projects.settings.$id.tsxweb/app/utils/routes.tsweb/public/locales/en.json
Comment on lines
+450
to
+503
| <MetricCard | ||
| label={t('project.ads.spend')} | ||
| value={stats?.cost ?? 0} | ||
| change={ | ||
| stats ? _round(stats.cost - stats.previous.cost, 2) : undefined | ||
| } | ||
| goodChangeDirection='up' | ||
| valueMapper={(value, type) => | ||
| `${type === 'badge' && value > 0 ? '+' : ''}${formatMoney(value)}` | ||
| } | ||
| /> | ||
| <MetricCard | ||
| label={t('project.ads.clicks')} | ||
| value={stats?.clicks ?? 0} | ||
| change={stats ? stats.clicks - stats.previous.clicks : undefined} | ||
| goodChangeDirection='down' | ||
| valueMapper={(value, type) => | ||
| `${type === 'badge' && value > 0 ? '+' : ''}${nFormatter(value, 1)}` | ||
| } | ||
| /> | ||
| <MetricCard | ||
| label={t('project.ads.cpc')} | ||
| value={stats?.cpc ?? 0} | ||
| valueMapper={(value) => formatMoney(value)} | ||
| /> | ||
| <MetricCard | ||
| label={t('project.ads.ctr')} | ||
| value={stats?.ctr ?? 0} | ||
| valueMapper={(value) => `${value}%`} | ||
| /> | ||
| <MetricCard | ||
| label={t('project.ads.adSessions')} | ||
| value={stats?.sessions ?? 0} | ||
| change={ | ||
| stats ? stats.sessions - stats.previous.sessions : undefined | ||
| } | ||
| goodChangeDirection='down' | ||
| valueMapper={(value, type) => | ||
| `${type === 'badge' && value > 0 ? '+' : ''}${nFormatter(value, 1)}` | ||
| } | ||
| /> | ||
| <MetricCard | ||
| label={t('project.ads.attributedRevenue')} | ||
| value={stats?.revenue ?? 0} | ||
| change={ | ||
| stats | ||
| ? _round(stats.revenue - stats.previous.revenue, 2) | ||
| : undefined | ||
| } | ||
| goodChangeDirection='down' | ||
| valueMapper={(value, type) => | ||
| `${type === 'badge' && value > 0 ? '+' : ''}${formatMoney(value)}` | ||
| } | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check MetricCard's goodChangeDirection semantics and existing usage patterns elsewhere
rg -n -A15 'goodChangeDirection' web/app/pages/Project/tabs/Traffic/MetricCards.tsx
rg -n -B3 -A3 "goodChangeDirection=" web/app/pages/Project/tabs/Traffic/TrafficView.tsxRepository: Swetrix/swetrix
Length of output: 8184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ads view and any nearby helpers/types that explain the metric semantics
ast-grep outline web/app/pages/Project/tabs/Ads/AdsView.tsx --view expanded
printf '\n---\n'
sed -n '430,520p' web/app/pages/Project/tabs/Ads/AdsView.tsx | cat -n
printf '\n=== Similar metric usage in project tabs ===\n'
rg -n -A3 -B3 "goodChangeDirection=" web/app/pages/Project/tabs -g '*.tsx'Repository: Swetrix/swetrix
Length of output: 18240
Swap these goodChangeDirection flags
spend should be 'down', while clicks, adSessions, and attributedRevenue should be 'up'. As written, the cards highlight the opposite trend as good.
🤖 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 `@web/app/pages/Project/tabs/Ads/AdsView.tsx` around lines 450 - 503, The
MetricCard trend indicators in AdsView are reversed for several metrics. Update
the goodChangeDirection props so the spend card uses 'down', while the clicks,
adSessions, and attributedRevenue cards use 'up'; keep the existing valueMapper
and change calculations intact. Use the MetricCard usages in AdsView to locate
and swap only these flags.
# Conflicts: # web/app/hooks/useAnalyticsProxy.ts # web/app/pages/Project/View/ViewProject.tsx # web/app/pages/Project/tabs/Traffic/TrafficView.tsx # web/app/routes/api.analytics.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/apps/cloud/src/ads/ads.service.ts`:
- Around line 671-684: Update the period and previous-window calculations around
getCampaignRows to avoid local-time parsing of the UTC timestamp strings
groupFrom and groupTo. Use native UTC Date arithmetic or the configured
dayjs.utc parsing/formatting so periodDays and the previousFrom/previousTo
boundaries remain aligned to exact UTC day offsets across DST transitions.
🪄 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: 43f31de3-13be-4612-9891-590a98329c36
📒 Files selected for processing (16)
backend/apps/cloud/src/ads/adapters/google-ads.adapter.tsbackend/apps/cloud/src/ads/ads-analytics.controller.tsbackend/apps/cloud/src/ads/ads.controller.tsbackend/apps/cloud/src/ads/ads.service.tsbackend/apps/cloud/src/ads/interfaces/ads.interface.tsbackend/apps/cloud/src/project/entity/project.entity.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/migrations/mysql/2026_07_07_google_ads.sqldocs/content/docs/analytics-dashboard/revenue-tracking.mdxdocs/content/docs/integrations/google-ads.mdxweb/app/api/api.server.tsweb/app/hooks/useAnalyticsProxy.tsweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsxweb/app/routes/ads-connected.tsxweb/public/locales/en.json
💤 Files with no reviewable changes (2)
- backend/apps/cloud/src/ads/interfaces/ads.interface.ts
- backend/apps/cloud/src/ads/ads.controller.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/content/docs/integrations/google-ads.mdx
- backend/migrations/mysql/2026_07_07_google_ads.sql
- backend/apps/cloud/src/task-manager/task-manager.service.ts
- docs/content/docs/analytics-dashboard/revenue-tracking.mdx
- web/app/routes/ads-connected.tsx
- web/app/api/api.server.ts
- backend/apps/cloud/src/project/entity/project.entity.ts
- backend/apps/cloud/src/ads/ads-analytics.controller.ts
- web/public/locales/en.json
- backend/apps/cloud/src/ads/adapters/google-ads.adapter.ts
- web/app/pages/Project/tabs/Traffic/TrafficView.tsx
- web/app/pages/Project/Settings/ProjectSettings.tsx
Comment on lines
+671
to
+684
| const periodDays = dayjs(groupTo).diff(dayjs(groupFrom), 'day') || 1 | ||
|
|
||
| // End the previous window just before the current one starts - ad_metrics | ||
| // is daily-grain, so sharing the boundary would count that whole day twice | ||
| const previousTo = dayjs(groupFrom) | ||
| .subtract(1, 'second') | ||
| .format('YYYY-MM-DD HH:mm:ss') | ||
| const previousFrom = dayjs(previousTo) | ||
| .subtract(periodDays, 'day') | ||
| .format('YYYY-MM-DD HH:mm:ss') | ||
|
|
||
| const [current, previous] = await Promise.all([ | ||
| this.getCampaignRows(pid, groupFrom, groupTo), | ||
| this.getCampaignRows(pid, previousFrom, previousTo), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid local time parsing for UTC strings.
Parsing UTC string timestamps (like 'YYYY-MM-DD HH:mm:ss') with dayjs(groupFrom) treats them as local server time. If the code runs in an environment where the local timezone observes Daylight Saving Time (e.g., a developer's local machine), diff and subtract across DST boundaries can yield incorrect hour shifts or 0 days.
Use native UTC Date math, or explicitly parse and format using dayjs.utc (if configured), to ensure boundaries remain strictly aligned to exact day offsets without local time interference.
🛡️ Proposed native Date math alternative
- const periodDays = dayjs(groupTo).diff(dayjs(groupFrom), 'day') || 1
-
- // End the previous window just before the current one starts - ad_metrics
- // is daily-grain, so sharing the boundary would count that whole day twice
- const previousTo = dayjs(groupFrom)
- .subtract(1, 'second')
- .format('YYYY-MM-DD HH:mm:ss')
- const previousFrom = dayjs(previousTo)
- .subtract(periodDays, 'day')
- .format('YYYY-MM-DD HH:mm:ss')
-
- const [current, previous] = await Promise.all([
- this.getCampaignRows(pid, groupFrom, groupTo),
- this.getCampaignRows(pid, previousFrom, previousTo),
- ])
+ const timeFrom = new Date(groupFrom + 'Z').getTime()
+ const timeTo = new Date(groupTo + 'Z').getTime()
+ const periodDays = Math.round((timeTo - timeFrom) / 86400000) || 1
+
+ // End the previous window just before the current one starts - ad_metrics
+ // is daily-grain, so sharing the boundary would count that whole day twice
+ const prevToTime = timeFrom - 1000
+ const previousTo = new Date(prevToTime).toISOString().slice(0, 19).replace('T', ' ')
+ const prevFromTime = prevToTime - periodDays * 86400000
+ const previousFrom = new Date(prevFromTime).toISOString().slice(0, 19).replace('T', ' ')
+
+ const [current, previous] = await Promise.all([
+ this.getCampaignRows(pid, groupFrom, groupTo),
+ this.getCampaignRows(pid, previousFrom, previousTo),
+ ])📝 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.
Suggested change
| const periodDays = dayjs(groupTo).diff(dayjs(groupFrom), 'day') || 1 | |
| // End the previous window just before the current one starts - ad_metrics | |
| // is daily-grain, so sharing the boundary would count that whole day twice | |
| const previousTo = dayjs(groupFrom) | |
| .subtract(1, 'second') | |
| .format('YYYY-MM-DD HH:mm:ss') | |
| const previousFrom = dayjs(previousTo) | |
| .subtract(periodDays, 'day') | |
| .format('YYYY-MM-DD HH:mm:ss') | |
| const [current, previous] = await Promise.all([ | |
| this.getCampaignRows(pid, groupFrom, groupTo), | |
| this.getCampaignRows(pid, previousFrom, previousTo), | |
| const timeFrom = new Date(groupFrom + 'Z').getTime() | |
| const timeTo = new Date(groupTo + 'Z').getTime() | |
| const periodDays = Math.round((timeTo - timeFrom) / 86400000) || 1 | |
| // End the previous window just before the current one starts - ad_metrics | |
| // is daily-grain, so sharing the boundary would count that whole day twice | |
| const prevToTime = timeFrom - 1000 | |
| const previousTo = new Date(prevToTime).toISOString().slice(0, 19).replace('T', ' ') | |
| const prevFromTime = prevToTime - periodDays * 86400000 | |
| const previousFrom = new Date(prevFromTime).toISOString().slice(0, 19).replace('T', ' ') | |
| const [current, previous] = await Promise.all([ | |
| this.getCampaignRows(pid, groupFrom, groupTo), | |
| this.getCampaignRows(pid, previousFrom, previousTo), |
🤖 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 `@backend/apps/cloud/src/ads/ads.service.ts` around lines 671 - 684, Update the
period and previous-window calculations around getCampaignRows to avoid
local-time parsing of the UTC timestamp strings groupFrom and groupTo. Use
native UTC Date arithmetic or the configured dayjs.utc parsing/formatting so
periodDays and the previousFrom/previousTo boundaries remain aligned to exact
UTC day offsets across DST transitions.
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.
Changes
#503
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit