Skip to content

Send the embedded script URL with pageviews - #138

Merged
adriaandotcom merged 1 commit into
v12from
w-14/script-src
Oct 20, 2026
Merged

adriaandotcom merged 1 commit into
v12from
w-14/script-src

Conversation

@simpleanalytics-agent

@simpleanalytics-agent simpleanalytics-agent commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #137

Pageviews now include src: the URL of the script tag, with everything after ? removed. For example, https://scripts.simpleanalyticscdn.com/latest.js?v=3 is sent as src=https://scripts.simpleanalyticscdn.com/latest.js.

  • It reuses the existing scriptElement (document.currentScript, falling back to the CDN script tag), so no new lookup is added.
  • It is sent with pageviews only, not with events or time-on-page appends.
  • Without a script element (for example a tag manager that injects the code), src is left out. An inline script sends an empty src. A script element without a string src (for example an SVG <script>) is also skipped, so it can't throw and stop tracking.

Depends on the queue: lib/sanitize.js only keeps known fields, so the queue needs to accept and store src before this data is kept. Without that change, the field is ignored and nothing breaks.

Size (minified / gzip -9, excluding the dated header): latest.js +34 / +14, light.js +34 / +15, other builds +34 / +14–15. Brotli: +14 on latest.js, +16 on light.js.

Effect on customers: no change to existing data, event names or counts. Each pageview request gets one extra parameter.

Security implications

Has security impact - described as: pageviews include the embedded script URL. The query string is removed, so tokens or parameters in the script src are never sent. The URL describes the site's installation, not the visitor.

Testing

  • pnpm run build and pnpm run test:unit (60 passing). The first version, without the src guard, failed the existing data-metadata-collector test, whose stub currentScript has no src.
  • One-off check against the built latest.dev.js, latest.js and light.js in jsdom: no script element → no src; …/latest.js?v=3&x=y → …/latest.js; https://example.com/proxy.js → unchanged; inline script → empty src.
  • Prettier passes on the changed lines (the existing schedulePing loop on v12 is not Prettier-formatted, and I left it unchanged). git diff --check passes.
  • BrowserStack was not run locally.

Open workspace in Conductor

Checklist

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

Fixes #137

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: QUIET
  • Plan: Advanced
  • Run ID: 74aa17ad-2b9a-4fbf-8989-90faeefdc740
📥 Commits

Reviewing files that changed from the base of the PR and between 3f1cffe and b670f60.

📒 Files selected for processing: 1
  • src/default.js

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Pageview payloads now include the embedding script’s source URL without query parameters when available. If the script element or its source is unavailable, the payload uses an undefined value.

Changes: Pageview payload

Layer / File(s) Summary
Add script URL to pageview payload
src/default.js
The pageview payload includes the embedding script URL without query parameters. It uses an undefined value when the script element or source is unavailable.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Feature

Suggested reviewers: adriaandotcom

Merge Risk: ⚪ Minimal · up to b670f

The pageview change has no established outstanding issue and is ready to merge after normal checks.

Architecture risk: 🔵 Low · up to b670f

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/default.js: The pageview payload now sends the embedding script’s URL without query parameters as src, or an undefined value when the script element or source is unavailable.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: adding the embedded script URL to pageview payloads.
Linked Issues check Passed Issue [#137] requires the script URL on pageviews, removal of the URL query string, and queue support for src. The PR evidence reports pageview-only src, query removal, reuse of scriptElement, h…
Out of Scope Changes check Passed The reported changes stay within issue [#137]. The src guard, pageview-only placement, queue support, and tests directly support the requested script URL collection. No unrelated behavior, event dat…
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Full details: Linked Issues check

Explanation

Issue [#137] requires the script URL on pageviews, removal of the URL query string, and queue support for src. The PR evidence reports pageview-only src, query removal, reuse of scriptElement, handling for missing and non-string sources, queue acceptance, and unit plus jsdom validation. These changes satisfy the coding requirements.

Full details: Out of Scope Changes check

Explanation

The reported changes stay within issue [#137]. The src guard, pageview-only placement, queue support, and tests directly support the requested script URL collection. No unrelated behavior, event data, or time-on-page data is reported as changed.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Comment thread src/default.js
@github-actions

Copy link
Copy Markdown
Claude review checkpoint

Reviewed commit b670f60374071d9c5417203a26a3f8f30978e840. 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 20, 2026
@github-actions

Copy link
Copy Markdown

SDLC label: change: needs review because adds a new field to the pageview payload sent from every customer installation, changing what the tracking script collects by default

@adriaandotcom
adriaandotcom merged commit 61d2ab0 into v12 Oct 20, 2026
9 checks passed
@adriaandotcom
adriaandotcom deleted the w-14/script-src branch October 20, 2026 21:35
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.

2 participants