Small updates - #39
Conversation
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
REQUEST_CHANGES — the consent-banner removal leaves analytics in a contradictory state and overrides an explicit stored decline.
Blocking finding
website/src/components/ui/CookieConsent.astro:98— preserve the consent gate or replace it with a consistent approved consent model.checkExistingConsent()now callsloadAnalytics()unconditionally, so the production GA script andgtag('config', ...)run for first-time visitors and even for visitors whose existingcookie-consentvalue isfalse. At the same time,website/src/scripts/shared/analytics.ts:25-31still requires that value to betruebefore any delegated/custom event is sent. Because the banner is never shown, a new visitor cannot set the value totrue: page-view tracking runs without consent while contact, signup, download, and other custom events silently remain disabled. Please either retain the consent UI/gating, or implement the intended no-banner/consent-mode policy consistently across both GA initialization andtrackEvent(); an existing decline must not be ignored.
Validation
- Exact diff reviewed at
73f2c49e91e5b3a4d4a847de2efd9a37918b1564against base69d40f8283c5a0651d9eb6739b3ab7ec87cdd235. git diff --checkpassed.PUPPETEER_SKIP_DOWNLOAD=true bun install --frozen-lockfilepassed (Bun 1.4.0 vianpx).SITE_BASE_URL=http://localhost:3000 bun run buildpassed; Astro generated 1,397 pages.- Focused Linkinator scan of
/community,/privacy-policy, and/launchpassed (41 links), and all nine locale files contain the relocatedcommunity.contactkeys. - The repository-wide Prettier and SEO-meta checks remain red on pre-existing baseline issues: among files changed by this PR, only unchanged lines in
zh-CN.jsonfail formatting; the 17 launch metadata length failures are outside the PR hunks. - GitHub reports no CI checks for this head.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
REQUEST_CHANGES — the analytics paths are now consistent, but the fix deliberately overrides visitors' existing stored opt-outs.
Blocking finding
website/src/scripts/shared/analytics.test.ts:54— continue honoring an explicit stored decline. The new regression test requirestrackEvent()to send whencookie-consentisfalse, whilewebsite/src/components/ui/Analytics.astro:12-13also initializes Google Analytics for every non-print visit without consulting that preference. Any returning visitor who previously clicked Decline will therefore begin sending page views and custom events after this deployment without changing their choice. Removing the banner need not block the PR: treat an unset value as the intended no-banner default, but keep both GA initialization andtrackEvent()disabled when the stored value is explicitlyfalse(and update this test to assert that behavior).
Validation
- Reviewed the complete 28-file diff at exact head
d0d0be2b7e3bfc112a61bc6b783fdb01e65573ecagainst base69d40f8283c5a0651d9eb6739b3ab7ec87cdd235, plus the incremental fix from73f2c49e. git diff --checkpassed.PUPPETEER_SKIP_DOWNLOAD=true npx --yes bun@1.4.0 install --frozen-lockfilepassed.npx --yes bun@1.4.0 testpassed (3/3 analytics tests); the passing "previously declined" case is the blocker above.SITE_BASE_URL=http://localhost:3000 npx --yes bun@1.4.0 run buildpassed and generated 1,397 pages; the built launch pages contain the globaloverscroll-behavior:nonerule.- Focused Linkinator scans passed for
/community(30 links),/privacy-policy(28), and/launch(7), including localized contact routes. - Repository-wide Prettier and SEO-meta checks remain red on baseline issues: changed-file formatting only reports unchanged lines in
zh-CN.json, and all 17 SEO failures are unchanged launch metadata values outside the PR hunks. - GitHub reports no CI checks for this head.
We intentionally want to remove cookie consent because we don't adhere to GDPR so regardless of the old choices, we will still track. |
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
REQUEST_CHANGES — unconditional Google Analytics loading remains a blocking privacy/compliance regression; the stated intent does not remove applicable consent and vendor requirements.
Blocking finding
website/src/components/ui/Analytics.astro:12— do not initialize GA before consent or after an explicit decline. This branch loads the productiongtag.jsand runsgtag('config', ...)on every non-print visit.website/src/scripts/shared/analytics.ts:68-77then sends custom events without consulting consent, and the new test atwebsite/src/scripts/shared/analytics.test.ts:54-60deliberately requires tracking when the stored choice isfalse. GA4 uses_gafirst-party cookies by default, while the published site policy explicitly recognizes EU/UK privacy rights. The EU ePrivacy Directive, Article 5(3) requires consent before non-essential storage/access, the ICO guidance specifically says analytics cookies require prior active consent, and Google's own Analytics guidance says customers must let users grant or deny consent. Restore and honor the consent gate, including existingfalsechoices, or implement an applicable-region Consent Mode/cookieless design that does not set or read analytics identifiers before consent.
Validation
- Re-reviewed the complete 28-file diff at exact head
d0d0be2b7e3bfc112a61bc6b783fdb01e65573ecagainst base69d40f8283c5a0651d9eb6739b3ab7ec87cdd235, including the author response; the live head remained unchanged before posting. git diff --checkpassed.PUPPETEER_SKIP_DOWNLOAD=true npx --yes bun@1.4.0 install --frozen-lockfilepassed.npx --yes bun@1.4.0 testpassed (3/3); the passing stored-decline case demonstrates the blocker above.SITE_BASE_URL=http://localhost:3000 ./node_modules/.bin/astro buildemitted a successful static build and sitemap for 1,397 pages; the local process remained open after completion and was stopped.- Focused Linkinator scans passed for
/community,/privacy-policy,/launch, and representative localized contact/privacy routes. - Repository-wide Prettier and SEO checks remain red on baseline issues: the changed-file formatting warning is on unchanged lines in
zh-CN.json, and all 17 SEO failures are unchanged launch metadata values outside this PR's hunks. - GitHub reports no CI checks for this head.
Uh oh!
There was an error while loading. Please reload this page.