fix(worker): make the Google Ads conversion tag env-driven instead of a dead placeholder - #136
fix(worker): make the Google Ads conversion tag env-driven instead of a dead placeholder#136nish3451 wants to merge 8 commits into
Conversation
… a dead placeholder The funnel's only Google Ads conversion measurement was dead by construction: brief-requested.html hardcoded the gtag loader with a placeholder conversion id, brief-requested.js fired the event to the same placeholder, and the production CSP blocked googletagmanager.com entirely — so even a real id pasted in would never load or record. The tag is now generated by the worker at request time from GOOGLE_ADS_CONVERSION_ID / GOOGLE_ADS_CONVERSION_LABEL, emitted only on /brief-requested when both are configured and well-formed, with gtag's CSP allowances scoped to that one noindex page's response. With either value missing or malformed, the page ships with no tag at all. CI now refuses any placeholder or hardcoded gtag in public/ or src/worker.js, so the dead-by-construction shape cannot return; the tracking spec documents the secret puts that activate the tag once the console-side conversion action exists. Re-applies PR #52's verified fix onto current main (favicon commits touched the same files); supersedes the stale conflicting PR #52.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe worker reads validated Google Ads conversion values from environment secrets and injects tracking only into ChangesGoogle Ads tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Worker
participant WorkerSecrets
participant GoogleAds
Browser->>Worker: Request /brief-requested
Worker->>WorkerSecrets: Read conversion ID and label
WorkerSecrets-->>Worker: Valid or unavailable configuration
Worker-->>Browser: HTML or generated JavaScript with page-scoped CSP
Browser->>GoogleAds: Load tag and send conversion event when configured
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-site.mjs`:
- Around line 637-655: Update the tracking validation around the
adsHtml/adsScript checks to enumerate every file under public/ rather than only
the two named assets. Scan each asset for the existing Google Ads placeholders,
Google Tag Manager references, and static tracking code using
whitespace-tolerant gtag call detection and dataLayer references, while
retaining the worker checks and failure reporting with the affected asset name.
In `@scripts/test-agent-worker.mjs`:
- Around line 1129-1140: Extend the malformed fixtures in the worker test to
include a non-empty invalid GOOGLE_ADS_CONVERSION_LABEL, such as a too-short or
quote-containing value. For that fixture, assert both /brief-requested and
/brief-requested.js responses contain no Google Ads tracking code, while
preserving the existing untouched HTML assertion for /brief-requested.
In `@specs/003-wellness-clinic-launch/tracking-setup.md`:
- Around line 74-75: Update the label validation description in the tracking
setup documentation to match GOOGLE_ADS_LABEL_PATTERN: document that labels are
10–50 characters and may contain letters, digits, underscores, and hyphens. Keep
the existing AW- plus digits rule unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb1414ac-0855-4e69-9204-776d0e50586e
📒 Files selected for processing (6)
public/brief-requested.htmlpublic/brief-requested.jsscripts/check-site.mjsscripts/test-agent-worker.mjsspecs/003-wellness-clinic-launch/tracking-setup.mdsrc/worker.js
| const adsHtml = read("public/brief-requested.html"); | ||
| const adsScript = read("public/brief-requested.js"); | ||
| for (const placeholder of ["AW-XXXXXXXXX", "YYYYYYYYYYYYYYYYYYY"]) { | ||
| for (const [label, content] of [ | ||
| ["public/brief-requested.html", adsHtml], | ||
| ["public/brief-requested.js", adsScript], | ||
| ["src/worker.js", worker] | ||
| ]) { | ||
| if (content.includes(placeholder)) { | ||
| failures.push(`Google Ads placeholder must never ship (dead conversion): ${placeholder} in ${label}.`); | ||
| } | ||
| } | ||
| } | ||
| if (adsHtml.includes("googletagmanager.com")) { | ||
| failures.push("public/brief-requested.html must not hardcode the Google Ads tag; the worker injects it from env at request time."); | ||
| } | ||
| if (adsScript.includes("gtag(") || adsScript.includes("dataLayer")) { | ||
| failures.push("public/brief-requested.js must not fire a conversion statically; the worker generates it from env when configured."); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scan all public assets for static tracking code.
Lines 637-643 inspect only two files under public/. A hardcoded tag in another public asset will pass CI. The exact gtag( check also misses whitespace variants such as gtag (.
Enumerate all files under public/. Use syntax-tolerant patterns for gtag calls and dataLayer references.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-site.mjs` around lines 637 - 655, Update the tracking
validation around the adsHtml/adsScript checks to enumerate every file under
public/ rather than only the two named assets. Scan each asset for the existing
Google Ads placeholders, Google Tag Manager references, and static tracking code
using whitespace-tolerant gtag call detection and dataLayer references, while
retaining the worker checks and failure reporting with the affected asset name.
| test("worker refuses to emit a Google Ads tag for a partial or malformed conversion config", async () => { | ||
| const malformed = [ | ||
| adsEnv({ GOOGLE_ADS_CONVERSION_LABEL: "" }), | ||
| adsEnv({ GOOGLE_ADS_CONVERSION_ID: "AW-123" }), | ||
| adsEnv({ GOOGLE_ADS_CONVERSION_ID: "javascript:alert(1)" }) | ||
| ]; | ||
| for (const env of malformed) { | ||
| const res = await worker.fetch(new Request("https://tinystudio.io/brief-requested"), env); | ||
| const body = await res.text(); | ||
| assert.ok(!body.includes("googletagmanager.com"), "no tag may emit for a partial or malformed config"); | ||
| assert.equal(body, BRIEF_REQUESTED_HTML, "unconfigured page must pass through untouched"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a malformed conversion-label fixture.
The fixtures test a missing label, but none tests a non-empty invalid GOOGLE_ADS_CONVERSION_LABEL. Add a malformed label such as a too-short value or a value containing a quote. Verify that both /brief-requested and /brief-requested.js emit no tracking code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-agent-worker.mjs` around lines 1129 - 1140, Extend the malformed
fixtures in the worker test to include a non-empty invalid
GOOGLE_ADS_CONVERSION_LABEL, such as a too-short or quote-containing value. For
that fixture, assert both /brief-requested and /brief-requested.js responses
contain no Google Ads tracking code, while preserving the existing untouched
HTML assertion for /brief-requested.
| The Worker validates both (`AW-` + digits; a 10+ character alphanumeric | ||
| label) and injects the gtag loader + conversion event into |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Match the documented label rule to GOOGLE_ADS_LABEL_PATTERN.
The text says the label is “10+ character alphanumeric.” src/worker.js accepts 10-50 letters, digits, _, and -. Document the implemented range so operators have an accurate configuration rule.
Proposed fix
- The Worker validates both (`AW-` + digits; a 10+ character alphanumeric
- label) and injects the gtag loader + conversion event into
+ The Worker validates both (`AW-` + 6-15 digits; a 10-50 character label
+ containing letters, digits, `_`, or `-`) and injects the gtag loader + conversion event into📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The Worker validates both (`AW-` + digits; a 10+ character alphanumeric | |
| label) and injects the gtag loader + conversion event into | |
| The Worker validates both (`AW-` + 6-15 digits; a 10-50 character label | |
| containing letters, digits, `_`, or `-`) and injects the gtag loader + conversion event into |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/003-wellness-clinic-launch/tracking-setup.md` around lines 74 - 75,
Update the label validation description in the tracking setup documentation to
match GOOGLE_ADS_LABEL_PATTERN: document that labels are 10–50 characters and
may contain letters, digits, underscores, and hyphens. Keep the existing AW-
plus digits rule unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5c87c5b32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (see googleAdsConversion below); when they are not, the page ships with | ||
| // the strict CSP and no tag at all. | ||
| const GOOGLE_ADS_CSP = | ||
| "default-src 'self'; img-src 'self' data: https://www.googleadservices.com; style-src 'self' https://fonts.googleapis.com; font-src 'self' https://fonts.gstatic.com; script-src 'self' https://static.cloudflareinsights.com https://www.googletagmanager.com; connect-src 'self' https://cloudflareinsights.com https://www.googletagmanager.com https://googleads.g.doubleclick.net https://www.googleadservices.com https://www.google-analytics.com https://stats.g.doubleclick.net; base-uri 'self'; frame-ancestors 'none'; form-action 'self'"; |
There was a problem hiding this comment.
When gtag sends the conversion or view-through pixel as an image request to googleads.g.doubleclick.net, this policy silently blocks it: that host appears only in connect-src, while img-src allows only www.googleadservices.com. CSP permissions do not carry across directives, so the loader can run while some conversion requests are still discarded; add the Google Ads pixel hosts to img-src and assert their placement rather than merely checking that the hostname occurs somewhere in the policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60379b7d26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
| const assetResponse = await env.ASSETS.fetch(request); | ||
| if (assetResponse.ok) { |
There was a problem hiding this comment.
When tracking is enabled after an unconfigured visit, the browser can revalidate its cached static /brief-requested HTML with If-None-Match; the Assets binding then returns 304, for which assetResponse.ok is false. This branch consequently skips loader injection and returns another 304, so the browser reuses the cached loader-free HTML while the generated brief-requested.js merely queues calls to a gtag library that was never loaded, losing that conversion. Fetch the source without the conditional headers or otherwise ensure the configured response is rewritten even during revalidation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
… a dead placeholder (#172) * fix(worker): make the Google Ads conversion tag env-driven instead of a dead placeholder The funnel's only Google Ads conversion measurement was dead by construction: brief-requested.html hardcoded the gtag loader with a placeholder conversion id, brief-requested.js fired the event to the same placeholder, and the production CSP blocked googletagmanager.com entirely — so even a real id pasted in would never load or record. The tag is now generated by the worker at request time from GOOGLE_ADS_CONVERSION_ID / GOOGLE_ADS_CONVERSION_LABEL, emitted only on /brief-requested when both are configured and well-formed, with gtag's CSP allowances scoped to that one noindex page's response. With either value missing or malformed, the page ships with no tag at all. CI now refuses any placeholder or hardcoded gtag in public/ or src/worker.js, so the dead-by-construction shape cannot return; the tracking spec documents the secret puts that activate the tag once the console-side conversion action exists. Co-authored-by: CommandCodeBot <noreply@commandcode.ai> * docs(evidence): record lane report for the env-driven Google Ads conversion tag fix Co-authored-by: CommandCodeBot <noreply@commandcode.ai> * docs(evidence): note superseded stale PR #136 in the lane report Co-authored-by: CommandCodeBot <noreply@commandcode.ai> --------- Co-authored-by: nish3451 <nish3451@users.noreply.github.com> Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
|
Closing: superseded by #172 (env-driven ads landed on main). |
1 participant
Why
The funnel's only Google Ads conversion measurement is dead by construction:
brief-requested.htmlhardcodes a gtag loader with theAW-XXXXXXXXXplaceholder ID,brief-requested.jsfires the event to the same placeholder, and the production CSP blocksgoogletagmanager.comentirely — so even a real ID pasted in would never load or record.What
The tag is generated by the worker at request time from
GOOGLE_ADS_CONVERSION_ID/GOOGLE_ADS_CONVERSION_LABELenv values:/brief-requested(and its generatedbrief-requested.js) when both values are configured and well-formed (AW-+ digits; 10+ alphanumeric label).check-site.mjs) now refuse any placeholder or hardcoded gtag inpublic/orsrc/worker.js, so the dead shape cannot return.tracking-setup.mddocuments the activation step: create theBrief requestedconversion action in Google Ads, thenwrangler secret putthe ID/label — no code change needed to go live.Verification
npm test— full suite green (95 tests, 0 failures), including 3 new worker tests: no tag while unconfigured; tag + scoped CSP on/brief-requestedonly; partial/malformed config emits nothing.npm run check:render-blocking— all six pages PASS.npm run deploy:dry-run— bundle OK.Supersedes PR #52 (same verified fix, re-applied onto current main after the favicon commits conflicted with that branch).
Summary by CodeRabbit