Skip to content

Preserve browser query attribution for manual pageviews - #109

Closed
simple-analytics-ai[bot] wants to merge 11 commits into
v12from
fix/v12-browser-query-attribution
Closed

simple-analytics-ai[bot] wants to merge 11 commits into
v12from
fix/v12-browser-query-attribution

Conversation

@simple-analytics-ai

@simple-analytics-ai simple-analytics-ai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Summary

Closes #107

Manual page views replaced the browser URL query with the manually supplied query, which lost landing-page attribution for the page view and for every event sent afterwards. getQueryParams() now reads location.search again instead of accepting an overwriteSearch argument:

- getQueryParams(ignoreSource, manualQuery)
+ getQueryParams(ignoreSource) // reads location.search

The manually supplied pathname is still honoured. The query is stripped from pathOverwrite before getPath() is called, so myPathOverwriter receives { path: "/product" } for sa_pageview("/product?product-id=1"), and data-ignore-pages, hash routes, numeric paths and numeric path-overwriter results keep working. The manual query is kept only as part of the internal lastSendPath key (from #108) so that manual page views differing only in their query are not deduplicated; it is never sent to the API.

Changes:

  • Keep reviewed manual-path guards with browser query attribution - Carry the item 1 ignore-pages and hash fixes into the separate item 2 PR. Preserve browser-URL attribution while retaining only the manual query duplicate key and keeping payload...
  • Merge reviewed manual-path corrections into the attribution branch - Keep item 2 separately reviewable against the updated item 1 base after validating the combined source and generated bundles.
  • Keep numeric manual paths compatible in the attribution stack - Carry the reviewed item 1 string guard and numeric regression into item 2 while preserving browser-URL attribution.
  • Merge final manual-path compatibility guard into attribution branch - Preserve the one-change review scope of item 2 against the updated item 1 base. The merged tree passed all 21 unit tests and the build.
  • Keep pathname-only callbacks in the attribution stack - Carry the requested callback compatibility correction from PR Preserve distinct manual pageviews in v12 #108 into its dependent attribution PR. Remove queries before getPath, preserve distinct-query duplicate detection, a...
  • Merge pathname-only callback correction into attribution branch - Preserve the requested existing path-overwriter behavior throughout the PR stack. Resolve generated bundle conflicts with the validated attribution tree; both builds, all 21 unit...
  • ...and 1 earlier commit(s).

Security implications

Has security impact - described as: query attribution is read from the browser URL again and stays filtered through the UTM allowlist, data-allow-params and the querykeys opt-out, so a manually supplied query string can no longer place unfiltered URL parameters in the recorded path. The manual query is retained only as an in-memory deduplication key and is removed before the path callback runs.

Testing

Run by Claude on Node 22.23.3:

  • npm run build: passed; dist/latest/latest.dev.js is unchanged, so the committed build matches src/.
  • npx mocha test/unit/*.test.js: 21 passing, covering browser-query attribution for page views and events, pathname-only path-overwriter input, distinct-query and identical-call handling, ignored paths, hash routes, and numeric paths/results. jsdom is not declared in devDependencies, so it had to be installed with --no-save to run these tests.
  • npx prettier --check src test: the only warning is the pre-existing test/deploy.test.mts, which this PR does not touch.
  • npx eslint src/*.js: passed.
  • Manual jsdom check: sa_pageview("/order?email=leak@example.com&token=abc") sends path=/order and no request (pixel or /append beacon) contains the manual query.
  • BrowserStack not run.

Checklist

  • Linked to an issue
  • Tested
  • Asked for a review

Simple Analytics Codex added 2 commits October 15, 2026 13:00
Requested in the compatibility review: fix query-only manual pageview loss and preserve full path-overwriter input. Keep the filtered outgoing path and review campaign attribution separately.

Refs #107
Requested in item 2: keep the previous browser-URL query behavior for pageviews and events. Remove the manual-query override while retaining case-insensitive filtering and the querykeys opt-out.

Refs #107
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Claude review checkpoint

Reviewed commit 10b02b1b6b85bd81785f8e32d1191258f44575ac. This is used to keep later automated reviews focused on changes Claude has not checked yet.

@github-actions github-actions Bot added the change: needs review Changes affecting security, data protection, or system stability. label Oct 15, 2026
@github-actions

Copy link
Copy Markdown

SDLC label: change: needs review because Alters what visitor URL data the public tracking script collects: manual pageview query strings are sent verbatim in the path field, bypassing the allowParams/UTM allowlist and the querykeys opt-out, and landing-page campaign parameters are re-sent on subsequent pageviews and events

Simple Analytics Codex added 3 commits October 15, 2026 13:07
Address automated review of item 1: normalize the manual query after the path-overwriter callback and before ignore-pages/hash handling, while keeping its query in the duplicate key.

Refs #107
Carry the item 1 ignore-pages and hash fixes into the separate item 2 PR. Preserve browser-URL attribution while retaining only the manual query duplicate key and keeping payload paths filtered.

Refs #107
Keep item 2 separately reviewable against the updated item 1 base after validating the combined source and generated bundles.

Refs #107
@github-actions

Copy link
Copy Markdown

src/default.js:752 — if (overwrite) path = path.split("?")[0]; runs on a value that is only guaranteed to be a string when the caller passed one; the isString(pathOverwrite) guard that previously fenced this logic in pageview is gone, so a non-string argument (or a path overwriter returning one) now throws an uncaught TypeError out of sa_pageview into the customer's page. Verified against both builds: sa_pageview(12345) sends path=12345 on 3a6d633, and raises TypeError: path.split is not a function on f5d84e4.

Smallest fix:

if (overwrite && isString(path)) path = path.split("?")[0];

(The line arrives via the merged fix/v12-manual-pageview-paths base, but this branch is what ships it; GitHub's file list for this PR is stale, so this could not be anchored inline.)

Simple Analytics Codex added 3 commits October 15, 2026 13:19
Address review of item 1: only split string path values, preserving numeric manual paths and numeric path-overwriter results without introducing an exception into the customer page.

Refs #107
Carry the reviewed item 1 string guard and numeric regression into item 2 while preserving browser-URL attribution.

Refs #107
Preserve the one-change review scope of item 2 against the updated item 1 base. The merged tree passed all 21 unit tests and the build.

Refs #107
@simple-analytics-ai

Copy link
Copy Markdown
Author

The numeric-path finding is addressed in the updated #108 base and merged into this branch at d3d6e56. Normalization only splits strings; numeric manual paths and numeric path-overwriter results remain accepted. All 21 unit tests pass. The PR diff against #108 remains limited to browser-query attribution.

Requested during review: keep the existing path-overwriter behavior without query strings, while retaining distinct manual-query pageviews. Restore query splitting before getPath and update the callback and duplicate checks.
Simple Analytics Codex added 2 commits October 15, 2026 15:12
Carry the requested callback compatibility correction from PR #108 into its dependent attribution PR. Remove queries before getPath, preserve distinct-query duplicate detection, and regenerate the bundles. All 21 unit tests and both builds pass.
Preserve the requested existing path-overwriter behavior throughout the PR stack. Resolve generated bundle conflicts with the validated attribution tree; both builds, all 21 unit tests, formatting and diff checks pass.
@github-actions

Copy link
Copy Markdown

Documentation update

_docs/20_install-script/02_custom.md

Location: ## Use custom collection anyway, directly after the paragraph that ends with "If you don't give a path it will use window.location.pathname as a default."

Copy and paste this into the documentation repository:

> We only record the pathname you pass. If your path contains a query string (for example `sa_pageview("/product?product-id=1")`), everything from the `?` onwards is dropped and the page view is recorded as `/product`.

URL parameters are never read from the path you pass to `sa_pageview`. They are always read from the browser's address bar (`window.location.search`) and filtered through the UTM allowlist, your [allow params](/allow-params) setting, and the `querykeys` option of [ignore metrics](/ignore-metrics). This keeps landing-page attribution (such as `utm_source`) intact for the page view and for events sent afterwards, and it prevents personal data in a manually supplied query from being sent to us.

_docs/20_script-settings/03_overwrite-path.md

Location: replacing the existing blockquote "You can not overwrite the hash from the URL, only the path."

Copy and paste this into the documentation repository:

> You can not overwrite the hash from the URL, only the path.

> The `path` your function receives never contains a query string. If you trigger a [custom page view](/trigger-custom-page-views) with `sa_pageview("/product?product-id=1")`, your function is called with `{ path: "/product" }`.

@simple-analytics-ai

simple-analytics-ai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Author

Update to #109 (comment):

PR #109 was closed without merging. I removed both of its proposed docs changes from the pending bundle, including the browser-query-only attribution copy. The earlier local draft is retired; nothing from it was published to the docs repository.

Base automatically changed from fix/v12-manual-pageview-paths to v12 October 15, 2026 14:32
@simple-analytics-ai

Copy link
Copy Markdown
Author

Closing because the agreed contract is to use an explicitly supplied manual query, with existing filtering, and fall back to location.search only when no manual query is supplied. Removing overwriteSearch would undo that contract. Events continue to read their browser query independently. The current v12 implementation already preserves this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change: needs review Changes affecting security, data protection, or system stability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants