Repository navigation
fix: serve analytics from our own origin so pageviews aren't lost - #31
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
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 |
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
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.
Loading the tracker from
umami.kdawg.devmakes it a third-party script, and a good share of visitors never request it — the pageview is lost before it leaves the browser. #30 shipped it that way; this moves it to our own origin.The tracker now loads from
/stats/script.js. Umami derives its collect endpoint from the directory the script came from, so the browser posts to/stats/api/send. Both are rewrites onto API routes.Why API routes and not a plain rewrite to the upstream
The hop now happens server-side, so without help Umami sees our address for every visitor. Its session key is
uuid(website, ip, userAgent, …), so every visitor would fold into one session. Two things fall out of that:/stats/api/sendforwards the real client IP asx-splitpro-client-ip, and the Umami service hasCLIENT_IP_HEADER=x-splitpro-client-ipset (already applied, see below). It also forwards the user agent, which Umami uses for device/browser/OS and for its bot filter, and replaysx-umami-cache.umami.kdawg.dev. That hostname is proxied by Cloudflare, which overwritescf-ipcountrywith the country of whoever connects — us. Umami prefers that header over its GeoIP lookup, so going through it put every visitor in one country. Verified: a forwarded hit claiming a US address was recorded as AU.Also here
getClientIpexisted in two copies (tRPC context and the NextAuth route) and I needed a third, so it moved tosrc/server/clientIp.ts. Same precedence as before (cf-connecting-ip, thenx-forwarded-for, then the socket), now also ignoring empty header values instead of returning''.Verified locally
Against a throwaway Umami site, through
next devand the rewrites, then deleted:GET /stats/script.js→ 200, byte-identical to upstream,text/javascript, cached an hour.POST /stats/api/sendwithx-forwarded-for: 8.8.8.8and an iPhone UA → 200, recorded as country US, device mobile, correct path. Without the IP header it came back AU (my address) with everything collapsed into one visitor.tsc --noEmitclean,next lintclean for these files, prettier clean. (13 files are prettier-dirty onmainalready; untouched here.)Notes
CLIENT_IP_HEADERis already set on the Umami service, which is safe to do ahead of this merge: Umami only honours it when the header is present, and today's direct browser posts don't send it. So nothing changes until this deploys.x-splitpro-client-ipon a direct post to the Umami host would be believed. The collect endpoint is already public and unauthenticated and the website ID is public, so forged events were possible regardless — this doesn't widen it.UMAMI_UPSTREAM_URLdefaults to the Railway domain inenv.js, so no new Railway variable is needed on split-pro.