feat: stamp is_staff on all GA4 events

[yodem]

Problem

Raised in #library-assistant: our usage reports (time-to-appetizer, click rates on topic links / pin-in-location / thinking steps) can't exclude internal staff traffic. As the team ramps up its own usage, staff sessions become a meaningful share of a still-small sample and skew every rate. Michael asked whether we're skewing the results; Josh asked for a filter flag "present in all events."

Root cause

The widget already receives the staff bit — ReaderApp.jsx passes is-moderator, and reader/views.py:301 sets it from Django's request.user.is_staff. It was only used for the settings gear and Braintrust metadata, and never reached GA4.

Structurally, all seven window.gtag(...) calls in LCChatbot.svelte were inline and independent, each passing only its own params — there was no shared layer where a common property could be attached.

Change

Route every event through a single track() helper that stamps is_staff on all of them:

let isStaff = $derived(isModerator === true || isModerator === 'true' ? 'true' : 'false');

function track(event, params = {}) {
  if (typeof window.gtag !== 'function') return;
  window.gtag('event', event, { ...params, is_staff: isStaff });
}

Sent as a string because GA4 custom dimensions are text. Covers assistant_click, assistant_element_shown, and assistant_message_sent.

Convention documented in src/CLAUDE.md: never call window.gtag directly — an event that bypasses track() is invisible to the staff filter and will silently skew reports.

Verification

Live-verified against the dev server with gtag stubbed (not just a build check):

• is-moderator="true" → assistant_click and assistant_message_sent carry is_staff: "true"
• attribute absent (the real-user path) → is_staff: "false"
• the observer-driven assistant_element_shown path carries it too

Follow-ups (not in this PR)

• is_staff must be registered as an event-scoped custom dimension in the GA4 admin — until then the param is collected but not available as a report/segment filter.
• Historical events have no is_staff, so numbers already shared can't be retroactively filtered.

🤖 Generated with Claude Code

[gitvelocity-reviewer]

📊 Code Quality Score: 10/100

25 × 0.4 = 10

Category	Score	Factors
🔭 Scope	5/20	Two files, single subsystem; 6 call sites refactored within one component; no new endpoints or external integrations
🏗️ Architecture	7/20	New track() abstraction centralizes cross-cutting analytics concern; CLAUDE.md establishes behavioral contract with anti-pattern guidance; small but meaningful pattern introduction
⚙️ Implementation	5/20	Simple wrapper function; $derived rune handles boolean/string coercion edge case; no algorithmic complexity; pre-existing duplicate key bug carried forward
⚠️ Risk	3/20	Refactor of existing behavior; only new behavior is is_staff stamping on all events; easily reversible; no data migration or schema changes
✅ Quality	4/15	Good CLAUDE.md documentation with anti-pattern guidance and behavioral contract; no tests added for track() helper or is_staff derivation; duplicate key bug not fixed
🔒 Perf / Security	1/5	No-op guard prevents errors in gtag-absent environments; no security concerns introduced

Was this score accurate? 👍 Yes · 👎 No

_{Scored by GitVelocity · How are scores calculated?}

[yodem]

Multi-agent review (4 parallel agents: code quality, Svelte 5 runes, privacy/security, test coverage)

Two real defects were found in this PR and are fixed in de1a3f5. Both were introduced by the refactor itself.

1. is-moderator was read three different ways (would have defeated the PR's purpose)

Custom-element attributes arrive uncoerced as strings. The component had:

Consumer	Test
GA4 is_staff (new)	=== true || === 'true' — strict
Settings gear (:1152)	{#if isModerator} — truthiness
Braintrust tag (api.js:77)	if (isStaff) — truthiness

For any value outside {true, "true"} — e.g. "True", "1" — the gear would show and Braintrust would tag the session as staff while GA4 recorded is_staff: "false". That is precisely the silent skew this PR exists to eliminate, and it would have shipped green.

Latent (not live) only because ReaderApp.jsx:2476 passes is-moderator={this.props.is_moderator || undefined}, so non-staff get the attribute omitted. The safety of the flag rested on an undocumented invariant in another repo.

Fix: normalize once (isModeratorBool), and have all three consumers read that boolean. Note "false" is a truthy string in JS — excluded explicitly, since treating it as staff would misreport every real user.

2. new URL(raw) was hoisted out of the gtag guard and can throw

Routing the response-link event through track() moved new URL(raw) outside typeof window.gtag === 'function'. new URL() throws on a malformed absolute href (e.g. a bare https://), so on a host without gtag (demo harness, chrome extension) a bad assistant-authored link would throw a TypeError inside the click listener and kill the handler.

Fix: try/catch, fall back to the raw href. Also removes a double new URL() construction on the same line.

Verification

Live in-browser against the dev server (gtag stubbed):

• is-moderator="True" → is_staff: "true" (was "false" before the fix)
• malformed link with no gtag → no throw
• malformed link with gtag → still emits, raw href preserved
• all 8 plausible host values (true/"true"/"True"/"1"/""/false/"false"/absent) → gear, Braintrust, and GA4 now agree in every case

Not fixed here (pre-existing, filed separately)

• context.labs is client-controlled (server/chat/V2/views.py:359). Any visitor can POST labs: true and unlock Labs tools. Should be derived server-side from the session. Worth its own ticket.
• The appetizer event sends the rendered topic sentence alongside the host's user_id; topics like health/sexuality subjects put this near GA's sensitive categories rules. Analytics owner should confirm. Also, GA4 truncates event-param values at 100 chars, so long Hebrew sentences are silently cut.
• assistant_click sends the same value as both text and link_text (wastes one of GA4's 25 param slots).

Caveat on the green check

CI does not test this diff. There is no frontend test harness in this repo (no vitest/jest, no tests under src/); ci.yaml runs pytest only. The green "Tests" check exercises zero lines of this PR. The track() invariant is enforced by a doc sentence alone — a CI grep gate or an ESLint no-restricted-globals rule would make it real. Recommended as a follow-up.

[yodem]

Review dispositions

All findings triaged. The two defects in this PR are fixed (de1a3f5); everything else is pre-existing and deliberately not fixed here, to keep this PR to the one thing it's for — adding the flag.

Finding	Disposition
is-moderator read 3 different ways (would defeat the flag)	Fixed — normalized to one boolean, all 3 consumers read it
new URL() throws outside the gtag guard	Fixed — try/catch, falls back to raw href
context.labs client-trusted → anyone can unlock Labs tools	Filed: sc-45794 (backend auth change, own PR)
Appetizer text: GA4 100-char truncation + sensitive-category join with user_id	For @josh Seidman / analytics owner — pre-existing, not caused by this PR
assistant_click sends same string as text and link_text	Left as-is — dropping one breaks any saved report keyed on text; analytics owner's call
Impressions re-fire when panel is closed/reopened (observer + seen WeakSet rebuilt)	Pre-existing — inflates the impression counts these events measure. Worth a follow-up.
No frontend test harness; track() invariant is doc-only	Follow-up PR — repo has never had one; CI runs pytest only

Documented in the Sefaria wiki runbook (la-click-tracking-data-feature-name), including the custom-element string-coercion gotcha — the same bug class is already live in index.html (default-open="false" arrives as a truthy string, so the demo widget opens anyway).

[yodem]

Re-verified on the full Sefaria stack (not the demo harness)

Earlier verification ran against the standalone :5173 demo harness, which fakes the host — it stubs gtag and never sets is-moderator. Re-ran against the real thing: Sefaria Django :8000 + chatbot backend :8001 + widget, real gtag from base.html, real React embed.

This surfaced the fact that makes the coercion fix load-bearing: the host passes is-moderator as the string "true" (React setAttribute on a custom element → Svelte hands it through uncoerced). The string path is the only path in production — so the previous strict-equality check worked by luck, and was one host-side change away from reporting every staff session as a real user.

User	Host passes	Event carries	Staff gear
staff (is_staff=True)	is-moderator="true" (string)	is_staff: "true"	shown
real user (non-staff)	attribute omitted	is_staff: "false"	hidden

Also sent a real message through the live agent: assistant_message_sent → is_staff: "false" on the real pipeline.

Caveats, stated plainly:

• The appetizer assistant_element_shown did not stream within my wait window, so it remains verified at :5173 only.
• CI still does not cover any of this — there is no frontend test harness in the repo (ci.yaml runs pytest only), so the green check exercises zero lines of this diff. All verification here is manual.

[coolify-sefaria-github]

The preview deployment for sefaria/ai-chatbot:server is ready. 🟢

Open app | Open Build Logs | Open Application Logs

Last updated at: 2026-07-14 09:20:58 CET

[dcschreiber]

(Claude writing on Daniel's behalf)

⚠️ Now conflicting with main — and the conflict is semantic, not just textual

Since this branch was cut, #166/#170 landed on main and added la_version: APP_VERSION to the GA4 events (currently on 4 of the 7 gtag calls: the two remaining assistant_click variants, Toggle to …, and assistant_message_sent). This PR's track() call sites were written against the pre-APP_VERSION file, so GitHub now reports the PR as CONFLICTING.

The dangerous part is the resolution, not the conflict markers: resolving in favor of this branch silently drops la_version from every event — the exact "silently skews reports" failure mode this PR's own CLAUDE.md note warns about, just for the version dimension instead of the staff one.

Suggested resolution: merge main in and stamp la_version inside track() alongside is_staff:

function track(event, params = {}) {
  if (typeof window.gtag !== 'function') return;
  window.gtag('event', event, { ...params, is_staff: isStaff, la_version: APP_VERSION });
}

That is exactly what track() exists for (cross-cutting params attached in one place), and as a bonus it makes la_version consistent across all 7 events — on main today it's only on 4, which looks like an oversight rather than a choice. (If it was a choice, keep it per-call-site instead — but then say so in the CLAUDE.md note.) Either way, please update the src/CLAUDE.md analytics section to say track() stamps both is_staff and la_version, and re-run the live verification after the merge since the earlier verification predates #170.

[dcschreiber]

I'm approving because it seems like you anyways can't merge until you fix the conflicts, which was my one comment.