Repository navigation
Conversation
- Added PostHog integration for user identification and event tracking in the application. - Created new documentation files for identifying users and integrating with TanStack Start. - Updated environment variables for PostHog project token and host. - Removed deprecated analytics event tracking code and replaced it with PostHog's capture methods. - Updated package dependencies to use the latest version of posthog-js. - Refactored components to utilize PostHog for analytics events instead of custom tracking.
The wizard's install called posthog.capture() with string literals directly in components. Every call site could invent an event name and attach arbitrary properties, which is the opposite of what issue #31 asks for. src/analytics/ now owns the integration: - events.ts declares AnalyticsEvent, a closed union. The assessment failure reason is derived from the endpoint's own error union, so an event cannot report a reason the API is incapable of returning. - useTrack.ts is the only file that calls usePostHog(). - AnalyticsProvider.tsx is the only file that knows PostHog exists; __root renders it and imports nothing from posthog-js. posthog-js imports are a lint error outside src/analytics/, so this holds at build time rather than by review. Restores the five page-engagement events dropped during the rewrite -- cta_clicked, nav_clicked, phone_clicked, faq_opened and form_started -- through the existing onActivate/onOpen seams, so base/ keeps no analytics dependency. Also: - deploy.yml passed VITE_POSTHOG_PROJECT_TOKEN while the code reads VITE_PUBLIC_POSTHOG_PROJECT_TOKEN, and never passed the host at all. A production build would have shipped without a token and reported nothing, with no failure anywhere. Both public variables are now passed. - window.posthog is assigned in dev only. The npm build never sets it, so the documented posthog.capture(...) console snippet threw ReferenceError. - .claude/skills/** is excluded from fmt and lint, on the same grounds as docs/**: vendor-authored primary sources, not code to be reformatted.
The brand copy said "Execile" while the legal entity, the domain and the contact address all said Execil: EXECIL PVT LTD, execil.net, contact@execil.net. The page was displaying a name its own privacy policy contradicted, so this is a correction rather than a rebrand. Covers all three kinds of occurrence: - Identifiers: package.json and the wrangler Worker name. Renaming the Worker is free today because nothing is deployed yet -- after a Cloudflare deploy this would create a second Worker rather than rename one. - User-facing copy: site name, page title, OG alt text, nav home label, the Why Us comparison table and the Results placeholder. - Docs: CLAUDE.md, CONTEXT.md, the provisioning runbook, the research notes and the design canvas. public/og.png renders the wordmark as pixels, so a text replace could not fix it; regenerated with scripts/og-image.py. The GitHub repository is renamed to Abuubkar/execil and the git remote now points at the new URL. The local working directory keeps its old name.
Abuubkar
changed the title
Owner
Author
|
Added a second, separate topic to this branch: the The brand copy said "Execile" while The GitHub repo is now Happy to split this into its own PR if you'd rather review the two topics separately. |
execil-dev-verify ran a duplicate Vite dev server on port 3100. Two Miniflare instances against the same project contend for the local SQLite state and crash workerd with SQLITE_BUSY, so a second config is a trap rather than a convenience.
Owner
Author
|
Two-axis review against One note first. The PR grew a third commit while this review ran: StandardsHard breaches of documented standards
Judgement calls on documented rules
Baseline smells (all judgement calls)
SpecBreaks #31, and the PR says so
Breaks #31 silently
Confirmed against the codeClient-only init, Unrequested additions
Follow-up outside the diffIssue #34's step 7 still names the old Summary. Standards: 2 hard, 3 judgement calls, 3 smells; worst is the broken "no accounts" constraint. Spec: 3 declared deviations, 5 silent ones, 6 unrequested additions; worst is PostHog moving into the eager bundle with no recorded cost. The rename commit is unreviewed. |
The serious one: .env.example shipped literal <ph_project_token> and <ph_client_api_host>. Those strings are truthy, so a fresh clone that copied the file past the guard in AnalyticsProvider and initialised PostHog against a host literally named "<ph_client_api_host>" -- under a header promising "every value below works with no account". The token is now blank and the host is the real US default. Blanking it alone would have traded one break for another, because the DEV-only guards threw and stopped the dev server. They now warn. Verified by blanking both variables: the page renders, PostHog never initialises, zero requests reach posthog.com, and two warnings explain why. Config values dropped when the wizard's options were carried across verbatim: - person_profiles: 'never' -- had silently reverted to identified_only - ui_host -- only consulted behind a reverse proxy, which is planned; setting it now means switching api_host is a one-variable change - defaults: '2026-05-30' -- had gone backwards to '2025-05-24' - capture_pageview: 'history_change' -- pinned rather than inherited Dropped tracing_headers: it stamped PostHog headers on the /api/assessment POST and was never asked for. assessment_details_copied fired only in the clipboard success branch, so a rejected write went uncounted -- and that event exists to answer whether the recovery panel actually recovers leads (issue #13), which makes the failures the interesting half. It now fires on the attempt. The no-restricted-imports boundary covered routes, sections and base only, leaving router.tsx, seo.ts, messages.ts, legal.ts and styles/ outside it. It now applies to all of src/ and is switched off for src/analytics/. Verified both ways: an import in seo.ts is caught, src/analytics/ stays exempt. Also removes 526 lines of vendored wizard reference docs -- one of which teaches the identify() call useTrack.ts forbids -- and with them the fmt and lint ignore rules that existed only to accommodate them. Fixes a stale event name in the provisioning runbook.
Workers Builds is connected to the repository and deploys on push to main, so deploy.yml was a second path to production that would have raced it the moment CLOUDFLARE_API_TOKEN existed. Removed. ci.yml already runs install, format, lint, types, build and the placeholder report on every push and pull request, so nothing is lost. Two things did not survive the move on their own: - The hard placeholder gate lived in deploy.yml. Workers Builds knows nothing about it, so it is now `build:deploy` in package.json, and the Cloudflare build command points at that. Verified it blocks: with a bracketed value present the script exits 1 and vite never runs. - The VITE_ build variables came from GitHub Actions variables. They are Cloudflare build variables now. The runbook's variable map says so, and step 1 documents where they go. Also drops the `deploy` script. A local `wrangler deploy` builds against .env, where VITE_SITE_URL is http://localhost:3000, and src/seo.ts falls back to that same value when unset -- which is why the first deployment shipped canonical and og:url pointing at localhost on every prerendered page. Deployment belongs to Workers Builds; there is no longer a one-command way to bypass it by accident. wrangler.jsonc states workers_dev and preview_urls explicitly rather than letting them default with a warning on every deploy.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
execil | 0b1bac1 | Commit Preview URL Branch Preview URL |
Sep 07 2026, 11:25 AM |
Workers Builds runs the Build command for preview versions as well as for production, so a gate there would fail every preview -- the copy ships with bracketed placeholders deliberately until launch, which is why deploy.yml gated the deploy step and not the build. The Deploy command is production-only, so the gate lives there: `deploy:production` is `placeholders && wrangler deploy`. Build and Version commands stay bare. Verified both halves with a placeholder present: `pnpm run build` succeeds, so previews keep working, and `pnpm run deploy:production` exits 1 before wrangler runs. The runbook now records all four dashboard fields, since that configuration lives only in Cloudflare and nothing in the repository would otherwise say what it should be.
Step 8 asked for a rule on POST /api/assessment. Rate limiting on the Free plan matches Path and Verified Bot only -- Method needs Business -- and allows one rule per zone with the period capped at 10 seconds. The route declares a POST handler and nothing else, so matching the path alone is equivalent here. Also finishes the GitHub-variable to Cloudflare-build-variable rename in the step bodies; the variable map was already updated.
SENDER_DOMAIN is the bare apex, execil.net, even though Email Routing goes on
the subdomain: the route composes assessment@forms.${SENDER_DOMAIN} itself.
The EMAIL binding is declared without destination_address. With one set the
binding is pinned to a single recipient and a `to` that differs is rejected,
which is the safer shape -- but it has to be an address already verified in the
Cloudflare account, and it must match ASSESSMENT_TO exactly. Both land together
once step 5 is done.
ASSESSMENT_TO is still placeholder@example.com, so submissions fail with reason
"delivery" until it is a verified destination. That is the designed failure
path, not a regression: the form's recovery panel covers it.
TURNSTILE_SECRET stays out of this file and out of vars -- it is a secret and
this file is committed.
Verified with `wrangler deploy --dry-run`, which now reports env.EMAIL,
env.ASSESSMENT_TO and env.SENDER_DOMAIN. Note the dry-run reads
dist/server/wrangler.json, which the Cloudflare vite plugin regenerates at
build time -- it reports the previous config until you rebuild.
Cloudflare Email Routing cannot be scoped to a subdomain without onboarding the
apex first: the dashboard reaches Settings > Subdomains only through the apex
domain, and its "Verify DNS records" screen replaces the apex MX with
route1/2/3.mx.cloudflare.net and the apex SPF with Cloudflare's include, offering
to remove the conflicting records "in place". Those are the records carrying the
Hostinger mailbox. Email Sending avoids the MX but writes an apex-wide _dmarc,
and needs a paid Workers plan.
So the endpoint POSTs to Resend and sends from a subdomain, which keeps every
verification record on forms.execil.net and the apex untouched.
The `!sent.ok` check matters: fetch only rejects on network failure, so without
it a 4xx from Resend would look like success and the visitor would be told their
request was delivered when it was not.
Verified locally against the real API with Cloudflare's always-passes Turnstile
test secret:
- no key -> {"ok":false,"error":"delivery"}, HTTP 502
- key rejected 401 -> {"ok":false,"error":"delivery"}, HTTP 502
and the failure log still carries field NAMES only -- no address, practice name
or message body -- so issue #12's guarantee survives the rewrite.
Removes the send_email binding; SENDER_DOMAIN stays, since the route still
composes assessment@forms.${SENDER_DOMAIN}. Runbook steps 4 and 5 rewritten, and
the standing DNS warning now names what actually threatens the mailbox.
Matches the address the site already publishes as its contact and uses for the form's mailto fallback (VITE_CONTACT_EMAIL, messages.json), so a visitor who hits the failure panel writes to the same mailbox the endpoint would have delivered to. Plain text rather than a secret on purpose: it is published on the site, and a reviewer should be able to see where leads go without opening a dashboard. Resend delivers to it like any other recipient -- there is no verification step for the RECIPIENT, unlike the Cloudflare binding this replaced, where the destination had to be verified in the account first. What still needs verifying is the SENDER domain, forms.execil.net, in Resend.
The subdomain existed to satisfy Cloudflare Email Routing, which needed a routing domain that was not the apex. Resend has no such requirement: it places its own MX and SPF on a send.execil.net subdomain and DKIM on resend._domainkey, and an MX record only affects the name it sits on, so the apex MX and SPF carrying the Hostinger mailbox are untouched either way. The subdomain was left-over scaffolding from a design that is gone. SENDER_DOMAIN is replaced by RESEND_FROM holding the whole From header rather than a domain the route reassembles, matching how the britnova project configures the same provider. The sender is now "Execil Assessment <assessment@execil.net>". Runbook step 4 rewritten to verify the apex in Resend, with an explicit warning not to add a Resend include: to the apex SPF -- it is not needed and a domain may hold only one SPF policy.
Same call britnova makes. Drops the manual Authorization header, the JSON
body and the !response.ok check, and removes the reply_to/replyTo ambiguity --
the SDK takes replyTo and maps it.
Kept the error check, in a different form: the SDK RESOLVES on an API error and
returns it in `error` rather than throwing, so without `if (error) throw` a
rejected send reads as success and the visitor is told their request was
delivered.
Verified the SDK runs in workerd, not just that it type-checks: an invalid key
through the local Worker returns {"ok":false,"error":"delivery"} with HTTP 502,
and the failure log still carries field names only.
CONTEXT.md still defined the Inbox as a Google Workspace mailbox. It is Hostinger, at contact@execil.net, and it is now a destination only -- sending is Resend's, and the separation is what keeps the apex MX from having to move. Two in the runbook: the launch check still said "the Workspace inbox", and step 6 still told you to run `wrangler secret put`, which needs a local wrangler login that the Workers Builds path does not create. Both secrets go in the dashboard now. Checked and deliberately left alone: - Email Routing and send_email appear in wrangler.jsonc and the runbook's DNS warning as the reason they are NOT used. That is rationale, not rot. - No dead code. The 22 exports nothing else imports are design-system prop unions plus getRouter, which the router entry and routeTree.gen.ts use. - react-dom is not an unused dependency; it is pulled in by the framework and appears 24 times in the built worker. - .env.example and the VITE_ variables the code reads are exactly in sync.
A real key in .dev.vars was one submit away from putting test mail in the client's inbox. Local runs now log the message and report success instead, so the form's success path stays exercisable offline without sending anything. The guard is `import.meta.env.DEV`, which is statically false in a production build, so the branch is removed at build time rather than shipped behind a runtime check that a spoofed header could reach. Confirmed: "DRY RUN" appears nowhere in dist/, and api.resend.com is still present in the server chunk. Proved it does not call out by putting an INVALID key in .dev.vars -- a real call would have returned error "delivery", and the endpoint returned ok:true with the message logged. Also marks two research documents as superseded. Both still describe designs that are no longer built, and worker-to-workspace-email.md describes one that would break the client's mail if followed: Cloudflare Email Routing has no subdomain-only path, so onboarding replaces the apex MX. Their findings stay accurate for their date and are kept as a record of what was checked.
It was written before any account existed, so much of it was reasoned rather than observed, and it has already been wrong about Email Routing subdomains, the mailbox provider, Free-plan rate-limiting fields and wrangler secret put. One of those would have replaced the apex MX and taken down the client's mailbox. Anyone following it should check the vendor's own documentation on any conflict and correct this file in the same change.
Cloudflare's own integration flow requires three checks. We did one. From https://developers.cloudflare.com/turnstile/spin/prompt.md: reject unless `success` is true, `action` matches, and `hostname` is in an approved list, where "production deployments must exclude localhost and 127.0.0.1". The hostname check is the one that mattered. Turnstile adds localhost and 127.0.0.1 to every widget automatically and they cannot be removed, and the sitekey is public in the page source -- so without it anyone could solve a real challenge on their own machine and spend the token against production. The field was already being read into the verdict type and then ignored. Also adds the `action` binding, an AbortSignal.timeout(10s) so a hung siteverify cannot hang the submission with it, and switches the request to urlencoded to match the documented idiom. remoteip stays CF-Connecting-IP, which Cloudflare sets itself and a client cannot forge. Both new checks are skipped in DEV, because they cannot be satisfied there: siteverify with Cloudflare's TEST keys answers hostname "example.com" and no `action` at all, whatever the widget was rendered with. Found that by asking siteverify directly after the strict version broke local submissions. `success` is still required, and import.meta.env.DEV is statically false in a production build so the relaxation cannot ship. Verified locally end to end: a real widget-minted token now returns ok:true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits: the wizard's PostHog install (
a96707e), then a rework that puts the integration behind one module (5e10f5f).What changed
src/analytics/owns PostHog.events.tsdeclares a closedAnalyticsEventunion,useTrack.tsis the only caller ofusePostHog(), andAnalyticsProvider.tsxis the only file that importsposthog-js.posthog-jsimports are a lint error everywhere else, so this is enforced by the build rather than by review.The five page-engagement events lost during the rewrite are back —
cta_clicked,nav_clicked,phone_clicked,faq_opened,form_started— wired through the existingonActivate/onOpenseams, sobase/still has no analytics dependency.The bug worth knowing about
deploy.ymlpassedVITE_POSTHOG_PROJECT_TOKENwhile the code readsVITE_PUBLIC_POSTHOG_PROJECT_TOKEN, and never passed the host. Production would have built with no token and reported nothing, silently. Fixed here; both repo variables now exist under the new names and the old one is deleted.Verified
pnpm checkgreen — 76 files formatted, 63 clean of lint and type errorspnpm buildsucceeds, prerenders/,/privacy,/terms,/hipaaposthog-jsimport in a section fails lint, an invalid failure reason fails typecheckwindow.posthog(dev only) confirmed absent from the production bundleKnown divergence, accepted
The integration runs on wizard defaults: autocapture, session replay, heatmaps and dead clicks are on.
cookieless_mode: 'always'is set, so no cookies and no persistentdistinct_id— but DOM selectors, element text and replay frames still flow./privacyand/hipaa, and issue #13's decision to ship without a consent banner, describe a quieter integration than the one running. This is a deliberate choice, not an oversight, and it is recorded indocs/provisioning.mdstep 7. It should be settled before counsel reviews the legal pages — see the legal-review section of #34.Follow-ups not in this PR
AnalyticsProviderthrows in dev when either variable is missing, so a fresh clone will not boot the dev server without them. Conflicts with the project's "runs with no account" constraint; one-line fix.cta_clicked,nav_clickedandphone_clickedeach duplicate a$autocaptureevent recording the same click.