Skip to content

Skip missing windows without errors and drop the saAutomatedLink global - #142

Merged
adriaandotcom merged 1 commit into
mainfrom
codex/drop-window-guard-and-global
Oct 22, 2026
Merged

adriaandotcom merged 1 commit into
mainfrom
codex/drop-window-guard-and-global

Conversation

@simpleanalytics-agent

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

Copy link
Copy Markdown
Contributor

Summary

Fixes #141

  • No errors without a window. Both scripts now pass typeof window < "u" && window to their wrapper and return early when it is false. Before this, auto-events.js passed a bare window and threw ReferenceError: window is not defined outside a browser. Its own typeof window === "undefined" check never ran, because the call site threw first. typeof returns "undefined" (which sorts after "u") only when there is no window, so this replaces the longer "undefined" != typeof window check in latest.js.
  • No window.saAutomatedLink. It was global only because of the onclick="saAutomatedLink(this, '…')" attributes, which v12 replaced with click listeners. It is now a local function declaration. It wasn't documented, and the only other user was the retired cloudflare-app repository.

Size compared with main (same build, gzip -9, brotli 11):

File Minified Gzip Brotli
latest.js −9 B −2 B +4 B
light.js −9 B −5 B −2 B
auto-events.js −14 B −10 B +1 B

I also tried skipping the whole wrapper with typeof window < "u" && (function (…) {…})(window, …). That saves 37 B minified / 7 B gzip on latest.js, but Prettier then reindents all 1,194 lines of src/default.js, so I left it out.

dist/ is not included.

Security implications

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

Testing

  • pnpm run test:unit: 72 passing.
  • New test/unit/no-window.test.js evaluates latest.js, light.js and auto-events.js in a context without window. On main, auto-events.js throws ReferenceError: window is not defined. With this change, none of them throw.
  • The auto-events tests click links instead of calling window.saAutomatedLink. The namespaced tracker tests (ba_event, my_event) bind a real link in jsdom, click it, and check that window.saAutomatedLink is undefined. They fail on main.
  • pnpm run test:deploy: 20 passing. pnpm run typecheck passes.
  • Prettier passes for the changed files. ESLint reports nothing new: the 4 errors in src/default.js already exist on main.
  • BrowserStack is left to CI.

Open workspace in Conductor

Summary by CodeRabbit

  • Bug Fixes
    • Scripts can now load in environments where the browser window object is unavailable.
  • Compatibility
    • The standalone global link-tracking function is no longer exposed. Applications that relied on it may need to update their integration.

auto-events.js referenced window at the call site, so it threw outside
a browser. Both scripts now pass `typeof window < "u" && window`, which
is also shorter than the previous check in latest.js.

saAutomatedLink was only global for the onclick attributes that v12
replaced with click listeners, so it is now a local function. The tests
click real links instead of calling the global.

Fixes #141

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

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: QUIET
  • Plan: Advanced
  • Run ID: 45a5ba22-2791-4662-b0b4-1e93c022b160
📥 Commits

Reviewing files that changed from the base of the PR and between 0dfaca5 and 8fb4b3a.

📒 Files selected for processing: 5
  • src/auto-events.js
  • src/default.js
  • test/unit/auto-events.test.js
  • test/unit/helpers/dom-auto-events.js
  • test/unit/no-window.test.js
💤 Files with no reviewable changes: 1
  • test/unit/helpers/dom-auto-events.js

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

📝 Walkthrough

The scripts update their checks for a missing window. The auto-events script no longer exposes saAutomatedLink on window. Tests cover click tracking and loading distribution scripts without window.

Changes: Windowless execution and link tracking

Layer / File(s) Summary
Window guards and handler scope
src/auto-events.js, src/default.js
The scripts use updated checks for a missing window. saAutomatedLink is now a function declaration and is no longer assigned to window.
Click tracking and windowless tests
test/unit/auto-events.test.js, test/unit/helpers/dom-auto-events.js, test/unit/no-window.test.js
The click tests use a local harness and cover phone, outbound, download, and email cases. Tracker tests trigger listeners on a document-attached link. The removed DOM helper is replaced by a test that checks script loading without window.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: adriaandotcom

Merge Risk: ⚪ Minimal · up to 8fb4b

The supported build-and-test workflow remains available. No actionable merge-blocking issue was established.

Security architecture risk: ⚪ Minimal · up to 8fb4b

The change removes a global handler without expanding tracking authority or changing the existing click-processing controls. No material security risk was identified as introduced or worsened by this PR.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reviewed change remains within the embedding page's existing tracking context. It neither adds a collection destination nor increases the authority of the configured tracker; removing the global handler reduces direct invocation surface.

Trust Boundaries and Controls

  • observed — The flagged getAttribute, hasAttribute, and preventDefault declarations are test mocks. Production link processing retains its existing collection settings, scheme and hostname checks, and data-simple-event exclusion. Link-derived data reaches the same tracker call as before.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 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 summarizes both primary changes: handling missing window values without errors and removing the saAutomatedLink global.
Linked Issues check Passed The PR meets the coding requirements in #141. src/auto-events.js passes typeof window < "u" && window to its wrapper and returns when the value is falsy. This prevents evaluation without window.…
Out of Scope Changes check Passed The changes stay within #141. The auto-events test updates verify click behavior after removal of the global. The deleted JSDOM helper is replaced by the focused click test harness. The new no-window …
Full details: Linked Issues check

Explanation

The PR meets the coding requirements in #141. src/auto-events.js passes typeof window &lt; "u" &amp;&amp; window to its wrapper and returns when the value is falsy. This prevents evaluation without window. The file defines saAutomatedLink locally and does not assign window.saAutomatedLink. src/default.js uses the shorter missing-window guard. test/unit/no-window.test.js covers all three scripts without window, and the reported unit tests cover link clicks and removal of the global.

Full details: Out of Scope Changes check

Explanation

The changes stay within #141. The auto-events test updates verify click behavior after removal of the global. The deleted JSDOM helper is replaced by the focused click test harness. The new no-window test directly verifies the missing-window requirement. No unrelated product behavior or files are identified.

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

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

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

@github-actions

Copy link
Copy Markdown
Contributor
Claude review checkpoint

Reviewed commit 8fb4b3afb9d2afc2ad375cc80f52e1757a65fd0c. 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 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

SDLC label: change: routine because Bug fix and refactor in the tracking scripts: guards the IIFE against a missing window so the bundles no longer throw off-browser, and removes an undocumented global. No change to what data is collected, stored, or who can access it, and no auth, billing, schema, or infrastructure impact.

@adriaandotcom
adriaandotcom merged commit 233f1bb into main Oct 22, 2026
5 checks passed
@adriaandotcom
adriaandotcom deleted the codex/drop-window-guard-and-global branch October 22, 2026 13:51
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.

Scripts throw or add globals when there is no window

2 participants