Refresh Logger page params (dl/dt) on SPA navigations

[philipwalton]

Problem

Logger captures dl (location.href) and dt (document.title) once at construction (src/javascript/Logger.ts, _pageParams init, ~lines 54-56). After an SPA navigation (src/javascript/content-loader.ts), subsequent beacons carry the updated event-level ep.page_path but the original document location and title in the page-level dl/dt params. Since the analytics design already tracks original_page_path separately (src/javascript/log.ts), the stale dl/dt appears to be an oversight rather than intent.

Root cause

_pageParams.dl and _pageParams.dt were only ever written in the constructor; nothing updated them when the SPA content loader swapped the page.

What changed

• src/javascript/Logger.ts
  • New public refreshPageParams() method that re-reads location.href and the (suffix-stripped) document.title into dl/dt. The title-stripping one-liner is factored into a getPageTitle() helper so the /\s+—.*$/ regex isn't duplicated.
  • Because the beacon body is rebuilt from the mutable _pageParams on every _queue() call, simply mutating dl/dt would have relabeled already-queued but unsent events with the new page's params. So refreshPageParams() also rotates the beacon group: it leaves the pending fetchLater() beacon scheduled (deliberately not aborted, unlike the superseded-beacon path in _queue()) and starts a new group (_sendCount++, clear queue, drop the result/controller references). Events keep the page params that were current when they were logged; this reuses the same rotation mechanism _queue() already applies after a beacon activates.
  • refreshPageParams() captures dl/dt synchronously, then awaits _presendDependencies before rotating. Since event() awaits the same promises before queuing, microtask FIFO ordering guarantees every event logged before the refresh call is queued into the old group and every event logged after lands in the new one — closing an in-flight-event race flagged in review.
• src/javascript/content-loader.ts — trackPageview() calls log.refreshPageParams() before logging the page_view. This runs after loadPage(), i.e. after executeContainerScripts() has applied the partial's document.title. Deliberate choice: the earlier route_transition event (logged in fetchPageContent(), before the content is swapped) keeps the pre-navigation dl/dt — that's the document state at the time it's logged — while page_view and everything after report the new URL/title. The beacon rotation is what makes this guarantee hold.
• src/javascript/Logger.test.ts — two new browser-project unit tests: one simulates an SPA navigation (history.pushState + document.title assignment) and asserts the next beacon's dl/dt, _s bump, and that the pre-refresh beacon is left un-aborted with its events; another covers the in-flight race (event logged while a presend dependency is pending, then refresh) and asserts the event stays in the pre-refresh group.
• test/e2e/log.ts — encodes the new intended semantics:
  • "should send pageview hits on SPA pageloads" now asserts the post-navigation beacon's dl matches the new article URL.
  • "should send pageview hits on back/forward navigations": removed the two intermediate clearBeacons() calls and instead waits for each navigation's page_view beacon (matched by navigation_type: route_change) before navigating again. The old test implicitly depended on the abort-and-resend batching behavior (cleared events reappeared in the final rebuilt beacon); with rotation, each navigation finalizes its beacon and events are not re-sent, and the per-step waits also make the final in-order triple assertion deterministic.

Verification

All run inside the worktree on this branch:

• npm run lint — 0 errors
• npm run types:check — pass
• npm run test:unit — 16 files / 69 tests pass (includes the 2 new Logger tests)
• npm run build — pass
• npm test (full e2e, multiple runs) — all log.ts tests pass, including both SPA specs with the new assertions. Tolerated failures observed, all pre-existing/known:
  • homepage.ts "working links to all published articles" — deterministic (atom-feed order, PR Sort the Atom feed newest-first #103)
  • content-loading.ts "should not attempt to load non-HTML content" — deterministic (same root cause)
  • content-loading.ts "should show an error if the content cannot be loaded" — intermittent, passes on retry
  • log.ts "should track engagement time" — environmental on the test machine (needs browser window focus); passed when the window had focus
  • worker.ts priority-hints specs intermittently timed out (in clearStorage()'s /__reset readiness wait) in some full-suite runs and passed in others, with identical code. Running the spec alone 4x showed the same first-attempt flake (~1 min hang) that always passes on the automatic retry, so it's a pre-existing timing flake. refreshPageParams() is only invoked on SPA navigations, which worker.ts never performs, so this diff's code path is never exercised there.

Codex review

Three rounds (read-only):

• Round 1: REQUEST_CHANGES — flagged that mutating dl/dt in place would relabel already-queued events (and made the route_transition comment false). Fixed via the beacon-group rotation described above.
• Round 2: REQUEST_CHANGES — flagged the in-flight event() race (event logged pre-refresh but queued post-refresh while awaiting presend dependencies). Fixed by awaiting the same presend dependencies in refreshPageParams(); regression unit test added.
• Round 3: APPROVE, no blocking issues. Its suggestion to make the refresh/page_view sequencing explicit was adopted (trackPageview() now awaits the refresh).

Unresolved non-blocking suggestions: (a) new RegExp(${articles[0]?.path}$) in the e2e spec doesn't regex-escape the path — current article slugs contain no regex-significant characters, and this matches the file's existing new RegExp(...) usage; (b) the in-flight-race unit test asserts grouping (_s, un-aborted first beacon) but doesn't additionally change the URL/title within that test — the dl/dt values themselves are covered by the other new test; (c) explicit : void/return-type annotations on the new method were skipped to match the class's existing style.

Please scrutinize

• This changes analytics semantics. If the stale dl/dt was intentional (e.g. GA-style "dl = the originally loaded document"), close this PR — the source report flagged this issue as "verify against intent". Note original_page_path already preserves the landing path at the event level.
• Beacon delivery granularity changed for SPA sessions: previously one beacon body was continuously rebuilt (abort + reschedule) across SPA navigations; now each SPA navigation finalizes the pending beacon and starts a new one, so multi-page SPA sessions send one beacon per page rather than one combined beacon. _s (send count) increments per navigation accordingly, and /log ingestion will see more, smaller requests. Verify this is acceptable for the collection endpoint.
• The route_transition event intentionally keeps the pre-navigation dl/dt (document state at log time); if you'd rather it carry the destination's params, move the refreshPageParams() call earlier.
• The e2e back/forward test rewrite (dropping intermediate clearBeacons()) encodes the new no-resend semantics — please confirm the test still checks what you intended it to.

Conflicts with sibling PRs

• PR Fix the never-true initial-state check in Logger._updateState #106 (fix/logger-state-init) touches Logger.ts (different method — constructor/state init).
• PRs Harden Navigation API interception in the content loader #116/Ship partial titles as a data attribute instead of re-executed inline scripts #117 touch content-loader.ts; Ship partial titles as a data attribute instead of re-executed inline scripts #117 modifies loadPage() near trackPageview(). This diff adds only one call + comment inside trackPageview(), so rebases should be mechanical.

🤖 Generated with Claude Code