Skip to content

Run the pageview callback when the pageview is skipped - #132

Merged
adriaandotcom merged 1 commit into
v12from
conductor/fix-skipped-pageview-callback-130
Oct 19, 2026
Merged

adriaandotcom merged 1 commit into
v12from
conductor/fix-skipped-pageview-callback-130

Conversation

@simpleanalytics-agent

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

Copy link
Copy Markdown
Contributor

Summary

Fixes #130

Targets v12 (#40).

sa_pageview(path, callback) only ran the callback from the pixel's onload/onerror. When the pageview was skipped, because the path repeated or matched data-ignore-pages, the callback never ran, so code waiting for it hung. Both cases return early at the same spot in pageview(). That early return now calls the callback, as sa_event already does for skipped events. A passed callback now runs exactly once per call:

Call Request Callback before Callback after
sa_pageview("/a", cb) (sent) 1 once (pixel load/error) once (pixel load/error)
sa_pageview("/a", cb) again (repeated) 0 never once, right away
sa_pageview("/ignored", cb) / (path, metadata, cb) 0 never once, right away
namespaced ba_pageview(...) same as above same as above same as above

Sent pageviews still get the callback only from the pixel, so it can't run twice. Automatic pageviews (load, pushState, popstate, hashchange) pass no callback and send the same requests as before. No other early return in the pageview path skips a passed callback.

The call is wrapped in the existing /** if (or spa hash) **/ flag, the same one that defines sa_pageview. Without it, light.js (which has no sa_pageview) would have grown by 27 bytes, because uglify would no longer drop the unused callback setup.

Size: latest.js 9189 → 9199 bytes minified (+10) and 4614 → 4617 gzip (+3, gzip -9 -n). The other event builds grow by +10 minified and 0 to +2 gzip. light.js, custom/light.js and auto-events.js are byte-identical.

Effect on customers: sites send the same requests as before. Only sites that pass a callback to sa_pageview (new in v12) see a change: their callback now also runs for skipped pageviews.

Only src/ and tests change. dist/ is rebuilt separately.

Security implications

  • No security impact
  • Has security impact - described as:

Testing

  • Regression tests in test/unit/pageview-callback.test.js cover a repeated path, an ignored path with metadata, and the namespaced ba_pageview, and count calls for a sent pageview. The three skip tests failed before the fix (callbacks ran 0 times) and pass after it.
  • pnpm install --frozen-lockfile, pnpm run build and pnpm run test:unit (59 passing). Prettier 3.9.10 --check on the changed files and git diff --check pass. ESLint shows three errors that also exist on v12, none in changed lines.
  • Ran the minified latest.js in jsdom with namespace: "ba" and ignorePages: "/ignore". Calls: ba_pageview("/same"), sa_pageview("/same"), ba_pageview("/ignore", metadata), ba_pageview("/other"). Callback calls went from [1, 0, 0, 1] to [1, 1, 1, 1]. Both builds sent the same two requests (/same, /other).
  • GPT-6 Astra reviewed the commit for correctness, regressions, byte size and test coverage. It found nothing to change. It built every script and checked the 11 builds that define sa_pageview. It confirmed one callback for sent and skipped pageviews, including image errors, delayed loads, and client hints that resolve, reject or throw. Automatic navigation payloads and counts were unchanged. Shorter variants it tried were no smaller after gzip.
  • BrowserStack was not run locally.

Open workspace in Conductor

Summary by CodeRabbit

  • Bug Fixes
    • Pageview callbacks now run when a pageview is skipped as a duplicate or ignored path in builds with automatic pageview collection or SPA hash support.
    • Builds without those features continue to skip the callback when a pageview is skipped.

sa_pageview(path, callback) only called the callback from the pixel's
load or error handler, so a skipped pageview (a repeated path or a path
excluded by ignore-pages) never ran it and code waiting for it hung.
Call the callback on that early return as well, so it runs once per
call, and cover repeated, ignored and namespaced calls in the unit
tests.

Sent pageviews keep calling the callback once from the pixel. The call
is limited to builds with manual pageviews, so light.js is unchanged;
the other scripts grow by 10 bytes minified and 1 to 2 bytes gzip.

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

coderabbitai Bot commented Oct 19, 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: 3d9d3329-5df0-4cd2-b644-0c7f45e1c5b5
📥 Commits

Reviewing files that changed from the base of the PR and between 58fa30d and 76dc1e3.

📒 Files selected for processing: 2
  • src/default.js
  • test/unit/pageview-callback.test.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

When automatic pageview collection or SPA hash support is enabled, skipped pageviews now invoke the supplied callback. Unit tests cover callbacks and request paths for sent, duplicate, ignored, and namespaced pageviews.

Changes: Pageview callbacks

Layer / File(s) Summary
Callback behavior and tests
src/default.js, test/unit/pageview-callback.test.js
For builds with automatic pageview collection or SPA hash support, skipped paths invoke the callback before returning. Tests check callbacks and requests for normal, duplicate, ignored, and namespaced pageviews.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: adriaandotcom

Merge Risk: ⚪ Minimal · up to 76dc1

The callback change has no identified merge-blocking issue; it is ready for normal checks.

Security architecture risk: ⚪ Minimal · up to 76dc1

The change completes callbacks for skipped pageviews without weakening page filtering or adding requests. The reviewed change introduces no material security risk. Skipped callbacks run immediately, so their timing differs from callbacks completed by an image request.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The incremental execution scope is the calling page’s supplied callback when its pageview is skipped. The inspected change does not add a request, destination, credential or service authority; any downstream action depends on the caller’s callback code.

Trust Boundaries and Controls

  • observed — The existing ignore-page and duplicate-path controls remain ahead of state mutation and transport. Completing the callback does not bypass those controls or resume the skipped send path.

Resilience and Maintainability Implications

  • inferred — A skipped callback can re-enter the API or throw synchronously. The skipped invocation has not reserved or mutated pageview state that would require rollback or cleanup. This is a caller-control-flow compatibility consideration, not an established security-boundary failure; synchronous callback completion also exists in the event API’s early-return paths.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
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: invoking the pageview callback when a pageview is skipped.
Linked Issues check Passed Issue #130 requires one callback execution for every sa_pageview call that provides a callback, including repeated and ignored paths. The PR summary reports the early returns now invoke the callback…
Out of Scope Changes check Passed The changes stay within issue #130. src/default.js implements the skipped-pageview callback behavior. test/unit/pageview-callback.test.js adds regression coverage for sent, repeated, ignored, and …
Full details: Linked Issues check

Explanation

Issue #130 requires one callback execution for every sa_pageview call that provides a callback, including repeated and ignored paths. The PR summary reports the early returns now invoke the callback. The test changes verify one callback for a sent pageview, both callbacks for repeated paths, one callback for an ignored path with metadata, and equivalent namespaced behavior. The tests also verify that skipped calls send no request.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #130. src/default.js implements the skipped-pageview callback behavior. test/unit/pageview-callback.test.js adds regression coverage for sent, repeated, ignored, and namespaced pageviews. These tests directly support the issue objective. No unrelated product change is shown.

  • Fix all pre-merge checks with AI
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-actions

Copy link
Copy Markdown

Documentation update

_docs/20_install-script/02_custom.md

Location: Use custom collection anyway, right after the line "If you call sa_pageview with the same path as the previous page view, we don't send it again."

Copy and paste this into the documentation repository:

You can pass a callback function as the last argument. We run it when the page view request has finished:

```js
window.sa_pageview("/pricing", function () {
  // The page view is sent
});

window.sa_pageview("/pricing", { plan: "pro" }, function () {
  // The page view is sent
});

We also run the callback when we skip the page view, for example when the path is the same as the previous page view, or when it matches data-ignore-pages. In that case we run it right away and we don't send a request. This way code that waits for the callback, like navigating away, never hangs. The callback runs exactly once per call to sa_pageview.

@github-actions

Copy link
Copy Markdown
Claude review checkpoint

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

@github-actions github-actions Bot added the change: routine Changes to internal tools or low-risk changes (e.g., design updates or content modifications) label Oct 19, 2026
@github-actions

Copy link
Copy Markdown

SDLC label: change: routine because Bug fix that runs an already-supported sa_pageview callback on the skip path; no change to collected data, permissions, billing, schemas, or production infrastructure

@adriaandotcom
adriaandotcom merged commit d46eefe into v12 Oct 19, 2026
3 checks passed
@adriaandotcom
adriaandotcom deleted the conductor/fix-skipped-pageview-callback-130 branch October 19, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change: routine Changes to internal tools or low-risk changes (e.g., design updates or content modifications)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants