fix: keep campaign parameters in browser page context - #37
Conversation
BrowserAnalytics built its page context from window.location.pathname alone, so the query string never left the browser. Providers that report a URL read context.page.url and fall back to context.page.path, and that fallback was always taken, which dropped every utm_* parameter before delivery and made campaign traffic arrive as direct. Populate the url, search, host and protocol fields that EventContext["page"] already declared, from one getPageContext() snapshot shared by initialize() and pageView(). OpenPanel's screenView() now receives the full URL, matching what its own SDK sends when it tracks screen views itself.
The adapter test proves page.url is emitted; this covers the other half, that the OpenPanel provider prefers it over page.path and forwards the query string to screenView. Also note in the changeset that counts move on routes that call pageView() when query parameters change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9f315a49c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| title: document.title, | ||
| referrer: document.referrer, | ||
| }, | ||
| page: this.getPageContext(), |
There was a problem hiding this comment.
When initialization passes this snapshot to updateContext, that method reconstructs page using only path, title, and referrer, silently discarding url, search, host, and protocol. Consequently, subsequent track() and pageLeave() calls still receive no campaign-bearing URL; only the immediate pageView() provider call works because it separately substitutes the local page snapshot. Merge every declared page field into the stored context instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d153f99 — thanks, this was a real gap.
updateContext rebuilt page from path, title and referrer only, so the four new fields were discarded the moment they were stored. pageView() masked it by substituting its own local snapshot into the provider call, which is exactly why the adapter test passed while the bug was live; track() and pageLeave() read this.context and still saw no URL.
It now merges the supplied fields over the stored ones. Two details worth noting:
- Only fields the caller actually supplied may overwrite, so a partial update such as
updateContext({ page: { path } })no longer erases a storedurl. A plain spread would have let anundefinedwipe it. searchis kept as a real value when empty. A truthy fallback would have made a URL with no query string inherit the previous page's parameters.
Two regression tests cover it: track() after pageView() carries page.url, and partial updates neither erase nor go stale. Both fail against the previous updateContext — verified by reverting just that hunk.
updateContext rebuilt page from path, title and referrer only, so the url, search, host and protocol fields were discarded as soon as they were stored. pageView() substitutes its own local snapshot and so kept working, but track() and pageLeave() read the stored context and still received no campaign-bearing URL. Merge the supplied fields over the stored ones instead. Only fields the caller actually provided may overwrite, so a partial update no longer erases what it omits, and search is treated as a real value when empty rather than falling back to the previous page's query string. Reported by Codex review on #37.
1 participant
Problem
BrowserAnalyticsbuilt its page context fromwindow.location.pathnamealone:Providers that report a URL read
context.page.urland fall back tocontext.page.path— and because the adapter never populatedurl, that fallback was always taken. The query string never left the browser, so everyutm_source/utm_medium/utm_campaignvalue was dropped before delivery and campaign traffic arrived as direct.Consuming apps could not work around it:
pageView()overwrites the wholepageobject viaupdateContext({ page }), clobbering anypage.urla caller sets beforehand, and its only parameter isproperties, not context.Evidence
Found while diagnosing a real OpenPanel project where paid referral clicks were landing but showing no source. Both attribution channels were empty for the same traffic:
The referrer half had a separate cause on the sending site (
rel="noreferrer"). This PR fixes the UTM half.Fix
One
getPageContext()snapshot, shared byinitialize()andpageView(), populating theurl,search,hostandprotocolfields thatEventContext["page"]already declared but nothing ever filled.This reaches every provider that reports a URL — OpenPanel, Bento, EmitKit, Pirsch and the proxy. The proxy's ingestion validator already allowlists all four fields (
hasValidClientContext), so they pass through without a schema change.On the OpenPanel
__pathshapescreenView()now receives the full URL, so__pathchanges from a bare pathname to an absolute URL. This is deliberate and matches OpenPanel's own behaviour — its web SDK defaultsscreenView()towindow.location.hrefwhen it tracks screen views itself:Passing only the pathname was the deviation. The dashboard resolves path and domain server-side (hence its "Show domain" toggle).
Two behavioural notes, both called out in the changeset:
screenView()dedupes on the value it is handed, so one pathname under different query strings is now distinct. An app callingpageView()on query-param changes (filters, pagination, tabs) will emit onescreen_viewper change rather than one in total — expect counts on those routes to rise.Tests
test/client-analytics.test.ts— the adapter emitspage.url/page.searchwith the query string intact. Verified this fails withundefinedwhen the fix is reverted.test/openpanel-client-provider.test.ts— the OpenPanel provider preferspage.urloverpage.pathand forwards the full URL toscreenView.373 tests pass;
typecheckandbiome lintclean.