fix: consoldating two ad tracking into one#60
Open
KartikLabhshetwar wants to merge 6 commits intomainfrom
Open
fix: consoldating two ad tracking into one#60KartikLabhshetwar wants to merge 6 commits intomainfrom
KartikLabhshetwar wants to merge 6 commits intomainfrom
Conversation
|
Paragon Review Unavailable Hi @KartikLabhshetwar! To enable Paragon reviews on this repository, please register at https://home.polarity.cc Once registered, connect your GitHub account and Paragon will automatically review your pull requests. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚅 Deployed to the SMRY-pr-60 environment in smry
|
📝 WalkthroughWalkthroughConsolidates client ad tracking to a single /api/px flow: removes Changes
Sequence DiagramsequenceDiagram
participant Client as Client (use-gravity-ad)
participant PxEndpoint as /api/px
participant Local as trackAdEvent (Local Analytics)
participant Gravity as Gravity Service
Client->>PxEndpoint: POST /api/px (body: type, sessionId, metadata) or POST /api/px?url=encodedImpUrl
PxEndpoint->>PxEndpoint: determine event type (body.type or query)
alt event == "impression" and impUrl/query.url present
PxEndpoint->>PxEndpoint: validate impUrl endsWith "trygravity.ai"
alt valid
PxEndpoint->>Gravity: GET impUrl (UA: "13ft-impression-proxy/1.0", timeout 6s)
Gravity-->>PxEndpoint: response (status)
PxEndpoint->>PxEndpoint: set gravity_status_code / forwardingSuccess
else invalid
PxEndpoint-->>PxEndpoint: set validationError / log
end
end
PxEndpoint->>Local: trackAdEvent(payload with event_type, sessionId, metadata, gravity_status_code?, error?)
Local-->>PxEndpoint: ack
PxEndpoint-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/routes/gravity.ts`:
- Around line 87-98: The domain check in the impression handling (eventType,
impUrl, parsedUrl.hostname) is too permissive because
hostname.endsWith("trygravity.ai") will allow names like "evil-trygravity.ai";
change the validation to lower-case the hostname and allow only exact
"trygravity.ai" or subdomains by requiring a leading dot for subdomains (i.e.,
hostname === "trygravity.ai" || hostname.endsWith(".trygravity.ai")) so spoofed
hostnames without the dot are rejected; update the try/catch block where
parsedUrl is created to use this stricter check and return the same 400 error on
failure.
Contributor
Greptile OverviewGreptile SummaryThis PR consolidates two separate ad tracking endpoints ( Key Changes:
Architecture: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Client (use-gravity-ad.ts)
participant API as Server (/api/px)
participant Gravity as Gravity AI
participant Analytics as ClickHouse
Note over Client,Analytics: New Unified Ad Tracking Flow
rect rgb(240, 248, 255)
Note over Client,API: Impression Event
Client->>API: POST /api/px?url={impUrl}<br/>body: {type: "impression", ...ad data}
API->>API: Validate impUrl from trygravity.ai
alt Valid impression URL
API->>Gravity: GET {impUrl}<br/>(forward for billing)
Gravity-->>API: 200 OK
API->>Analytics: trackAdEvent(status: "filled")
else Invalid/Missing URL
API->>Analytics: trackAdEvent(status: "forwarding_failed")
else Timeout/Error
API->>Analytics: trackAdEvent(status: "forwarding_failed")
end
API-->>Client: 204 No Content
end
rect rgb(255, 248, 240)
Note over Client,Analytics: Click Event
Client->>API: POST /api/px<br/>body: {type: "click", ...ad data}
API->>Analytics: trackAdEvent(status: "filled")
API-->>Client: 204 No Content
end
rect rgb(248, 255, 240)
Note over Client,Analytics: Dismiss Event
Client->>API: POST /api/px<br/>body: {type: "dismiss", ...ad data}
API->>Analytics: trackAdEvent(status: "filled")
API-->>Client: 204 No Content
end
Note over Client,Analytics: Key Changes:<br/>1. Single /api/px endpoint (removed /adtrack)<br/>2. Server-side Gravity forwarding for impressions<br/>3. All events tracked locally<br/>4. Non-blocking 204 responses
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| query: t.Object({ | ||
| url: t.String(), | ||
| url: t.Optional(t.String()), |
Contributor
There was a problem hiding this comment.
url field is marked optional but should be required for consistency
Suggested change
| url: t.Optional(t.String()), | |
| url: t.String(), |
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!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/routes/gravity.ts`:
- Around line 107-124: The impression-forwarding fetch inside the isValidUrl
branch should use an AbortController with the existing GRAVITY_TIMEOUT_MS to
avoid hanging: create an AbortController, pass controller.signal to
fetch(impUrl, { method: "GET", headers: {...}, signal }), start a timeout that
calls controller.abort() after GRAVITY_TIMEOUT_MS, and clear that timeout after
the fetch completes; update the try/catch around the fetch in the block that
sets gravityForwarded and logger to handle aborted requests (identify via
error.name === 'AbortError') and include that context in validationError/logger
messages for impUrl and response.status where available.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/routes/gravity.ts`:
- Around line 141-158: Change the hard-coded status:"filled" in the trackAdEvent
call to reflect actual forwarding outcome: derive status as "filled" only when
there is no validationError and gravityStatusCode indicates success (2xx),
otherwise set a failure status like "forward_failed" (or "validation_error" when
validationError exists); include validationError and gravityStatusCode already
present so analytics can distinguish validation failures, HTTP non‑2xx
responses, and timeouts—update the status expression in the trackAdEvent payload
(the call to trackAdEvent, referencing eventType, validationError,
gravityStatusCode and body.sessionId) accordingly.
- Around line 82-108: The impression handler currently only reads query.url into
impUrl and may silently accept missing URLs; change the code that sets impUrl to
first try query.url then fall back to body.impUrl (e.g., impUrl = query.url ||
body.impUrl), and if eventType === "impression" and impUrl is still falsy, set
validationError to a clear message like "Missing impression URL", log a warning
(using logger.warn) and ensure isValidUrl remains false so the forwarding branch
(the block guarded by isValidUrl) is skipped; keep the existing URL
parsing/hostname check (parsedUrl.hostname.endsWith("trygravity.ai")) and
preserve gravityStatusCode behavior.
| // Return 204 No Content - the client doesn't need a response | ||
| // Always track event locally for analytics (even if Gravity forwarding failed) | ||
| trackAdEvent({ | ||
| event_type: eventType as "impression" | "click" | "dismiss", |
Contributor
There was a problem hiding this comment.
eventType is cast without validation - if body.type is undefined, line 85 defaults to undefined, causing this cast to be unsafe
Suggested change
| event_type: eventType as "impression" | "click" | "dismiss", | |
| event_type: eventType || "impression", |
Comment on lines
+309
to
+310
| const trackUrl = type === "impression" | ||
| ? getApiUrl(`/api/px?url=${encodeURIComponent(ad.impUrl)}`) |
Contributor
There was a problem hiding this comment.
For impressions, impUrl is now sent both in query string and request body - this creates redundancy that could lead to sync issues if they differ
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 by CodeRabbit
Refactor
Chores