feat(site): ask for analytics consent instead of measuring nothing - #154
Conversation
This file argued that a banner asking permission to do something the site does not need to do would be the failure this product refuses elsewhere, and loaded gtag on every page in Consent Mode's denied state instead. The argument was right about banners and wrong about what it bought. Denied mode still sent a hit for every page view; it sent one with no stable client id, so every view arrived as a new user in a new session. The site shipped Google's tag to every reader and got numbers in which users, sessions, bounce rate and returning readers were all noise — and nothing in the reports said so. Refusing to ask is only principled when the answer is that nothing is collected, and something was. Nothing is loaded now until measurement is allowed, and when it loads the consent it was denied is granted, so the identifier exists and the sessions are real. The question is asked only of the readers who are owed it. The EEA, the UK and the Crown Dependencies require permission; elsewhere measurement may run with a way to turn it off. Cloudflare already knows which applies and says so at `/cdn-cgi/trace`, so most readers are measured properly and never see a banner. A failed lookup asks — the safe direction, and what a build outside Cloudflare gets. Do Not Track and Global Privacy Control keep the position they had, and now sit above the site's own controls too: a reader who has answered this in their browser is not asked again and is not measured either way. Withdrawal is the half that is easy to leave out, so a new `/privacy` page carries a standing control that states what is currently happening before offering the buttons, and declining actually revokes: it sets the `ga-disable-` flag, pushes consent back to denied, and deletes the cookies acceptance set. That page is linked from the row that ends every documentation page as well as from the site footer, because outside the prompt regions no banner appears and the footer renders on splash pages alone — an opt-out only reachable from the landing page is not reachable. The query string was already dropped from the recorded location; the referrer is now reduced to its origin for the same reason, and the injected tag states its own referrer policy rather than inheriting the browser's default. `privacy` joins ORDER in both llms.txt generators, which otherwise fail the build rather than let a section go silently unindexed. Ported from labstack/fanout#219, including the four fixes that landed there after review. Claude-Session: https://claude.ai/code/session_0188fRgp2DyMXzUcJgdMZSyu
govulncheck fails the repository gate on GO-2026-6355 and GO-2026-6354, both reachable from `transport.sshHandshake` through `ssh.NewClientConn`: a peer can deadlock an established or an undecided channel and hold the connection open. Every `ob` command that touches a remote host goes through that call, so the gate is right to be red. Fixed in golang.org/x/crypto v0.56.0, which is what this takes. No API change and no call site moves; `just check` passes and govulncheck reports nothing. Claude-Session: https://claude.ai/code/session_012KwFfm2mZt7RhFip1PLpbU
There was a problem hiding this comment.
🟡 Changes recommended
The new footer CSS targets .meta a and will unintentionally restyle the existing meta links (e.g., EditLink), risking a UI regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the site’s analytics behavior to avoid loading Google’s tag until analytics are permitted, adds an explicit privacy/analytics control page, and ensures the new page is discoverable from documentation pages and the splash footer.
Changes:
- Introduces conditional GA4 loading based on region, DNT/GPC, and explicit consent (with a reversible preference stored in localStorage).
- Adds a new
/privacydocs page plus UI components for a consent banner and a standing preference control. - Links
/privacyfrom documentation footers and includes the new section in LLM-oriented doc indexes.
File summaries
| File | Description |
|---|---|
| site/src/pages/llms.txt.ts | Adds the new privacy docs section to the LLM index ordering and titles. |
| site/src/pages/llms-full.txt.ts | Includes the privacy section in the “complete documentation” concatenation ordering. |
| site/src/content/docs/privacy.mdx | Adds the privacy/analytics policy page and embeds the preference control component. |
| site/src/components/SiteFooter.astro | Adds a footer link to /privacy on splash pages. |
| site/src/components/PageFrame.astro | Renders the consent banner markup on all pages. |
| site/src/components/Footer.astro | Adds a /privacy link to doc-page meta footer and styles links in the meta row. |
| site/src/components/AnalyticsPreference.astro | New privacy-page “standing” control UI backed by window.oneboxAnalytics. |
| site/src/components/AnalyticsConsent.astro | New consent prompt markup and styling (logic handled by Analytics.astro). |
| site/src/components/Analytics.astro | Implements consent/region gating, tag injection, preference persistence, and withdrawal behavior. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .meta a { | ||
| color: var(--sl-color-gray-3); | ||
| text-decoration: none; | ||
| } | ||
|
|
||
| .meta a:hover { | ||
| color: var(--sl-color-white); | ||
| text-decoration: underline; | ||
| } |
There was a problem hiding this comment.
Not what happens — Astro scopes this rule, so it already reaches only the Privacy link.
.meta a compiles to a selector carrying this component's scope on both elements:
.meta:where(.astro-jo6i4kqk) a:where(.astro-jo6i4kqk){color:var(--sl-color-gray-3);text-decoration:none}And in the built page, EditLink's anchor carries its own scope class, not this one:
<div class="meta sl-flex astro-jo6i4kqk">
<a href="…/edit/main/…" class="sl-flex print:hidden astro-xk5gmzsg">…Edit page</a>
<p>Last updated: <time …>Sep 1, 2026</time></p>
<a href="/privacy" class="astro-jo6i4kqk">Privacy</a>
</div>Scoped styles don't cross into child components, which is exactly why line 75 needs :global to reach the <p> that LastUpdated renders. The same boundary is what keeps this rule off EditLink, so "one text link, not the whole band" is the behaviour and not just the intent. Adding an explicit class here would restate a scope the compiler already applies.
| // still count; returning readers are simply not re-identified. A banner | ||
| // asking permission to do something the site does not need to do would be | ||
| // the same failure this product refuses elsewhere. | ||
| // 2. Nothing loads until measurement is allowed. Not the tag, not a request. |
There was a problem hiding this comment.
Agreed, and fixed in c8266c7. The /cdn-cgi/trace lookup is a real request made before the answer is known, and a file that argues for precise claims shouldn't need the reader to scroll to find that out. Rule 2 now reads:
- Nothing reaches Google until measurement is allowed: no tag, no request, no identifier. One request is made before the answer is known, and it goes to this site's own
/cdn-cgi/traceto find out whether the question has to be asked at all — see below.
Wording only; the behaviour was already the one described.
The rule read "Nothing loads until measurement is allowed. Not the tag, not a request." A request is made before the answer is known: the /cdn-cgi/trace lookup that decides whether the question has to be asked at all. It is same-origin and it is the mechanism the rest of this file describes, so the promise is about Google rather than about the network, and a file that argues for precise claims should not need the reader to scroll to find that out. Wording only; no behaviour changes. Claude-Session: https://claude.ai/code/session_012KwFfm2mZt7RhFip1PLpbU
There was a problem hiding this comment.
🔵 Needs a closer look
The DNT/GPC early-return path currently doesn’t revoke existing GA cookies (and related disable state), which undermines the stated “browser-level refusal wins” behavior and should be corrected before approval.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
site/src/components/Analytics.astro:320
- When Do Not Track / Global Privacy Control is active, the script returns early without calling stop(), so any previously-set GA cookies (and the ga-disable flag) are left as-is. Consider calling stop() on this path so enabling DNT/GPC also revokes existing analytics identifiers.
site/src/components/Analytics.astro:149 - consentRequirement() is cached only in-memory (requirementPromise), so readers with no stored preference will fetch /cdn-cgi/trace again on every full page load (including in “not_required” regions where preference typically stays null). If this becomes a measurable overhead, consider persisting the requirement result (e.g., in sessionStorage with a short TTL) to avoid repeated trace lookups during a browsing session.
site/src/components/AnalyticsConsent.astro:7 - The header comment says the decision to ask is made “before anything renders”, but Analytics.astro defers showing/hiding the prompt until DOMContentLoaded (and after the /cdn-cgi/trace lookup). Tightening this wording would avoid misleading future maintainers.
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
A reader who allowed measurement and later switched on Do Not Track or
Global Privacy Control was no longer measured -- the tag is never
inserted on that path -- but the identifier acceptance had already stored
stayed in the browser. Switching the signal off again resumed it, which
joins the sessions either side of the refusal into one returning reader,
and in the meantime the site was holding an analytics id for someone
whose browser was asking it not to.
The refusal path now calls the same stop() the /privacy control uses, so
the cookies go. The stored preference is deliberately left alone: consent
is still granted, so lifting the signal measures again -- under a new
identifier rather than the old one.
AnalyticsConsent.astro claimed the decision to ask was made "before
anything renders". It is made in the head, but the prompt is revealed
after DOMContentLoaded and after the location lookup answers. Says that
instead.
Driven in Chromium against the built site with a test measurement id,
googletagmanager and /cdn-cgi/trace stubbed:
granted, no signal _ga present, tag loaded, ga-disable false
signal switched on cookies gone, ga-disable true, 0 requests to
Google, banner hidden, preference still granted
signal switched off tag loads again, consent default denied then
update granted, config sent, no page errors
Claude-Session: https://claude.ai/code/session_012KwFfm2mZt7RhFip1PLpbU
|
Second Copilot pass raised three suppressed comments. Two taken, one declined.
A reader who allowed measurement and later switched the signal on stopped being measured, because the tag is never inserted on that path — but the identifier acceptance had already stored stayed in the browser. Switching the signal off again resumed it, which joins the sessions either side of the refusal into one returning reader, and in the meantime the site held an analytics id for someone whose browser was asking it not to. The path now calls the same Driven in Chromium against the built site with a test measurement id,
It is one same-origin request to the edge that terminates the connection anyway, on a static documentation site — Copilot conditions the suggestion on it becoming "a measurable overhead", and it is not. The cost is the wrong shape: caching it means writing another item into the browser of a reader who has not yet been asked whether anything may be stored, plus a TTL and its staleness window, to save a request that is cheaper than the entry. A file whose second rule is that nothing is stored before the answer is known should not store something to work out what the question is. All five checks green on |
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-tab consent changes (via localStorage) don’t currently trigger the analytics script to re-apply start/stop behavior, which can allow continued measurement after withdrawal until reload.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
site/src/components/Analytics.astro:346
- Consent preference changes made in another tab won't actually start/stop analytics in the current tab:
writePreference()callsapply()only in the tab where the click happened, and nothing in this script listens forstorageevents to re-runapply(). This contradicts the comment that the preference is mirrored across tabs and can allow continued measurement after withdrawal until a reload.
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
The file said the preference is mirrored across tabs. It was stored where
every tab can read it, which is not the same thing: only the tab that
wrote it ran apply(). A reader with a documentation page open and
/privacy in a second tab could decline, watch the privacy page agree that
analytics were not allowed, and leave the first tab still measuring --
`ga-disable` false and consent granted -- until something navigated it.
The withdrawing tab deletes the identifier cookies, and the tab still
running the tag writes them back on its next event, so the deletion did
not survive either.
A `storage` listener now re-runs apply() in the tabs that did not write.
A null key is the clear() case, where the preference is gone rather than
changed; subscribe() takes the same condition so the privacy page's own
display agrees.
Driven in Chromium, two tabs, no navigation between the click and the
reading, googletagmanager and /cdn-cgi/trace stubbed:
before docs tab measuring, ga-disable false
decline on /privacy docs tab ga-disable true, consent update denied,
cookies gone
allow on /privacy docs tab ga-disable false, consent update granted
Against the same build without this change, the third column of the
second row read `ga-disable` false and consent granted -- the defect,
reproduced before it was fixed.
Claude-Session: https://claude.ai/code/session_012KwFfm2mZt7RhFip1PLpbU
|
Third pass, one finding, and it was right. Fixed in 8146452. Cross-tab withdrawal did not reach the tab being measured. The file said the preference is "mirrored across tabs". It was stored where every tab can read it, which is not the same thing: only the tab that wrote it ran A Chromium, two tabs, nothing navigated between the click and the reading,
The right-hand column is the defect reproduced against the pre-fix build, so the listener is load-bearing rather than defensive. |
There was a problem hiding this comment.
🔵 Needs a closer look
The new inline browser script contains a trailing comma in a function call argument list that can break execution in older browsers and should be corrected before merging.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
site/src/components/AnalyticsPreference.astro:76
- The inline script uses a trailing comma in the
setAttributecall argument list. Since this script is shipped to browsers as-is (and the surrounding code intentionally avoids newer JS syntax), that trailing comma can cause a syntax error in older browsers and prevent the preference control from working.
site/src/components/Analytics.astro:51 - The header comment claims the preference “is mirrored across tabs”, but the implementation explicitly falls back to an in-memory preference when localStorage is unavailable (in which case it is not mirrored). Tighten the wording so it matches the actual behavior.
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
…the mirror `is:inline` ships these scripts verbatim -- that is the point of the attribute, and why the code is written in the ES5 dialect it is. The trailing comma after the last argument of a `setAttribute` call is ES2017, so a browser old enough to lack it fails to parse the whole script rather than that one line, and the standing opt-out on /privacy renders with its buttons wired to nothing while the status line still says it is checking. A control that appears and does nothing is the worst state this file has. The comma in the country array stays: a trailing comma in an array literal has been legal since ES5. The header also said the preference "is mirrored across tabs". That is now true where storage works, and the file has a deliberate path where it does not: a browser that refuses storage keeps the answer in memory, for the page it was given on and no other. Says both. Claude-Session: https://claude.ai/code/session_012KwFfm2mZt7RhFip1PLpbU
|
Fourth pass, two findings, both taken. df7e187.
Swept the other two the same pattern finds.
|
There was a problem hiding this comment.
🟢 Approval recommended
The updated analytics gating, consent UI integration, privacy controls, and documentation/export updates are consistent and don’t show any concrete correctness issues in the changed code.
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
What this changes
The site loaded gtag on every page and held it in Consent Mode's denied state.
Analytics.astroargued for that: a banner asking permission to do something the site does not need to do would be the failure this product refuses elsewhere.The argument was right about banners and wrong about what it bought. Denied mode still sent a hit for every page view — one with no stable client id, so every view arrived as a new user in a new session. The site shipped Google's tag to every reader and got numbers in which users, sessions, bounce rate and returning readers were all noise, and nothing in the reports said so. Refusing to ask is only principled when the answer is that nothing is collected, and something was.
Nothing loads now until measurement is allowed, and when it loads the consent it was previously denied is granted, so the identifier exists and the sessions are real.
The question is asked only of the readers who are owed it. The EEA, the UK and the Crown Dependencies require permission before an analytics identifier is stored; elsewhere measurement runs with a way to turn it off. Cloudflare reports the country at
/cdn-cgi/trace, so most readers are measured properly and never see a banner. A failed lookup asks.DNT and GPC keep the position they had and now sit above the site's own controls too. A new
/privacypage carries a standing control; declining actually revokes, deleting the cookies acceptance set. That page is linked from the row that ends every documentation page as well as from the site footer — outside the prompt regions no banner appears andSiteFooterrenders on splash pages alone, so an opt-out reachable only from the landing page is not reachable.The query string was already dropped from the recorded location. The referrer is now reduced to its origin for the same reason, and the injected tag states its own referrer policy rather than inheriting the browser's default.
Ported from labstack/fanout#219, including the four fixes that landed there after review.
Why this is correct
Built with a real measurement id and driven in Chrome against
astro preview, with/cdn-cgi/tracestubbed to fake locations:loc=DE, no stored preferenceloc=DE, Allowga-disablefalse,page_location=/start/installwith no query,page_referrer= origin onlyloc=US, no stored preferencega-disabletrue, consent update all-denied,_gacookies deletedga-disablefalse, consent update granted — the toggle is not one-wayThe last two are the review findings from the fanout PR, verified here rather than assumed to have ported: an early return on
startedmade in-page re-granting a no-op, and the privacy page contradicted itself in builds without analytics configured.No console errors on any path.
just checkpasses, which includessite-build—astro check, the build, and the tables, social-card and canonical gates across 42 pages.Effect on the safety envelope
None. This is
site/only; no Go source, no schema, no change to what the binary does to a running system.Checklist
just checkpasses locally.just checkverifies this).https://claude.ai/code/session_0188fRgp2DyMXzUcJgdMZSyu