feat(observability): instrument the checkout funnel with started/completed/failed events

[FagnerMartinsBrack]

Checkout funnel instrumentation (R2)

Hypothesis

The stage between the /account Subscribe click and revenue is completely dark. If we emit structured events at checkout start, checkout completion, and every checkout-return failure, we can see exactly where the 0% trial→paid conversion is leaking — and measure any future fix instead of guessing.

Evidence (30d, Jun 4 – Jul 4 2026)

• 35 signups, 28 trials reached end-of-trial: 28× charge_failed (no_card_on_file) → 28× cancelled. Trial→paid = 0%.
• Upgrade funnel: 54 header trial-countdown clicks / 40 visitors → 22 /account views / 11 visitors → 4 subscribe clicks / 4 users → 2 /auth/checkout/success returns → 0 recorded conversion events. Checkout success does not emit anything by design (upsertActive directly) — the two known conversions (3e9c7e5c Jun 21, af194ac4 Jun 23) are invisible to every dashboard.
• The only Stripe webhook in 30 days was dropped ("no subscription row found — skipping event emission"), so there is no backstop signal either. Only 3 of 45 subscription rows ever carried a Stripe subscriptionId.
• af194ac4 converted after their Jun 18 cancellation — the cancelled→resubscribe path is real revenue and equally dark.

What this changes

Three new events on the existing subscriptions stream, emitted by the hutch web app (not the subscription Lambdas):

Event	Where	Fields
checkout_started	POST /account/subscribe, after the Stripe Checkout session + pending-signup write	user_id, variant ∈ trial_checkout | cancelled_resubscribe | card_decline_fallback, checkout_session_id
checkout_completed	GET /auth/checkout/success paid path — the missing paid-conversion event	user_id, subscription_id, checkout_session_id
checkout_return_failed	each failure branch of GET /auth/checkout/success	reason ∈ invalid_query (400) | session_not_found (404) | not_paid (402) | replayed (409), optional user_id / checkout_session_id

Plumbing reuses the existing initEmitSubscriptionEvent factory and SUBSCRIPTION_EVENTS constants; the composition root supplies HutchLogger.fromJSON<SubscriptionLogEvent>() exactly like the conversion logger. One new dashboard widget "Checkout funnel per day" (timeSeries, sourced from the hutch handler log group) charts all three events; widget count 27 → 28.

Deliberately not covered: the one-click resubscribe success path (subscriptions.create with a saved card, no Checkout UI) — flagged as follow-up.

Estimated result

No behaviour change for users; pure instrumentation. Next trial-end cohort (~28 users/month at current volume) produces a complete started→completed/failed funnel, letting us attribute the 4-clicks→0-conversions gap to a concrete branch (abandoned Checkout, not-paid returns, replayed links, or a silent server error).

How to measure

CloudWatch Logs Insights against /aws/lambda/hutch-handler:

fields @timestamp, event, user_id, variant, reason, checkout_session_id
| filter stream = "subscriptions"
| filter event in ["checkout_started", "checkout_completed", "checkout_return_failed"]
| stats count(*) as n by event, coalesce(variant, reason, "-") as detail
| sort n desc

Funnel-per-day view is the new dashboard widget. Conversion rate = checkout_completed / distinct user_id on checkout_started.

Merge-conflict note

The dashboard drift test asserts an exact widget count (toHaveLength(28) after this PR; main was 27). Sibling PRs #916–#918 may bump the same assertion. Resolution is mechanical: take both widget additions, set the count to the actual widgets.length, merge the description-text category list, and re-run pnpm nx run hutch:check — the geometric no-overlap test catches any real collision (if two PRs used the same slot, move this widget to a full-width row at x:0, y:130).

https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: e8174d9

[claude]

Claude finished @FagnerMartinsBrack's task in 5m 28s —— View job

---

Claude Code Review — PR #935

Reviewed the full diff (git diff origin/main...HEAD), traced the checkout-success and subscribe control flow, checked widget geometry, and verified the user_id optionality has no downstream consumers. This is clean, well-scoped instrumentation with comprehensive tests.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. card_decline_fallback variant is emitted for any exception in the one-click resub try, not just card declines

In account.page.ts:448-473 the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DynamoDB write). If the card is charged successfully but the DB write throws, the funnel records variant: card_decline_fallback even though no card was declined — and (pre-existing, out of scope for this PR) a fresh Checkout is started on an already-charged customer. The instrumentation faithfully mirrors the existing control flow, so this is just a labeling nuance to be aware of when reading the dashboard. If you want the variant to mean what it says, the emit could be narrowed to only the createSubscriptionOnExistingCustomer failure. Fix this →

2. One-click resubscribe success path emits nothing (already flagged as follow-up)

The success branch at account.page.ts:454-459 redirects without emitting — the PR body documents this ("Deliberately not covered … flagged as follow-up") and account.route.test.ts asserts it. Just noting the funnel consequence: checkout_completed / distinct user_id on checkout_started will undercount revenue, because these conversions have neither a started nor a completed event. Worth prioritising the follow-up so the dashboard reads as "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

checkoutCompleted (subscription-events.ts) emits user_id / subscription_id / checkout_session_id but not the originating variant, so per-variant completion rates require a self-join on checkout_session_id back to checkout_started. Aggregate conversion (per the PR's query) works fine; this only matters if you later want "trial vs. resubscribe" completion split directly. Informational.

What I verified

• Widget geometry — "Checkout funnel per day" sits at x:12, y:122 beside "Homepage A/B" (x:0, y:122), both 12 wide, on the row under the full-width errors widget (y:114). No overlap; the no-overlap and event-reference drift tests still hold. Count comment arithmetic (7+3+3+3+2+1+3+1+2+1+1+1) = 28. ✓
• user_id → optional — no TypeScript consumer reads event.user_id; the .main.ts files only use SubscriptionLogEvent as a logger type param, and emitter param types still require userId for the events that always have it. Safe. ✓
• Emit placement — checkoutStarted fires after the Stripe session + pending-signup write (so a throw in either → route-level catch, no event; test confirms). checkoutCompleted uses pending.userId (correct — the returning visitor may have no session), not req.userId. Replay path emits exactly completed then return_failed, no double-emit. ✓
• Branch coverage — both ternaries in checkoutReturnFailed (user_id, checkout_session_id) have truthy+falsy outcomes exercised across the not_paid and invalid_query unit tests. ✓
• Test integrity — completeStripeSignup returns a real checkoutSessionId, so the new toBe(checkoutSessionId) assertions are genuine, not undefined === undefined. ✓
• Guideline compliance — compile-time union constants (CHECKOUT_VARIANTS, CHECKOUT_RETURN_FAILURE_REASONS) over runtime validation, branded ids, named-object params, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, Zod at the HTTP boundary, HutchLogger throughout, no as casts. ✓

Note: static review only — I did not run pnpm check for this pure-instrumentation change.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.

[FagnerMartinsBrack]

Claude Code Review — PR #935

Reviewed the full diff (git diff origin/main...HEAD), traced the checkout-success and subscribe control flow, checked widget geometry, and verified the user_id optionality has no downstream consumers. This is clean, well-scoped instrumentation with comprehensive tests.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. card_decline_fallback variant is emitted for any exception in the one-click resub try, not just card declines

In account.page.ts:448-473 the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DynamoDB write). If the card is charged successfully but the DB write throws, the funnel records variant: card_decline_fallback even though no card was declined — and (pre-existing, out of scope for this PR) a fresh Checkout is started on an already-charged customer. The instrumentation faithfully mirrors the existing control flow, so this is just a labeling nuance. If you want the variant to mean what it says, the emit could be narrowed to only the createSubscriptionOnExistingCustomer failure.

2. One-click resubscribe success path emits nothing (already flagged as follow-up)

The success branch at account.page.ts:454-459 redirects without emitting — documented in the PR body and asserted in tests. Funnel consequence: checkout_completed / distinct user_id on checkout_started will undercount revenue, because these conversions have neither event. Worth prioritising the follow-up.

3. checkout_completed carries no variant

checkoutCompleted emits user_id/subscription_id/checkout_session_id but not the originating variant, so per-variant completion rates require a self-join on checkout_session_id. Aggregate conversion works fine; informational.

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Branch: conversion/checkout-instrumentation

[claude]

Claude finished @FagnerMartinsBrack's task in 10m 14s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions & PR intent
• Rebase onto origin/main and identify conflicts
• Resolve conflicts (cross-referencing PR intent)
• Run hutch:check to verify
• Force-push with --force-with-lease

---

Rebased conversion/checkout-instrumentation onto current main (7302f6c2). The conflicts came from PR #939 "pre-expiry trial reminder email (R1)" landing in the same two files this PR instruments. Both conflicts were additive — the trial-reminder feature and this PR's checkout instrumentation are independent, so I kept both sides:

File	main side (PR #939)	This PR	Resolution
web/auth/auth.page.ts — checkout-success paid path	await deps.trialScheduler.deleteTrialReminderSchedule({ userId })	deps.emitSubscriptionEvent.checkoutCompleted({ … })	Kept both, in order, before the redirect
web/pages/account/account.page.ts — import block	import { trialReminderFiresAt }	import { CHECKOUT_VARIANTS, … } + EmitSubscriptionEvent	Kept both imports (both used)

Verification

• Post-rebase diff vs main is still exactly +347 / −31 across 12 files — the instrumentation is fully intact, nothing dropped or duplicated.
• Confirmed the interleaved region is coherent: main's trial-reminder scheduling (account.page.ts:366-369) and this PR's checkoutStarted emit (:408-412) both survive.
• The dashboard files auto-merged; main never touched analytics-dashboard.ts, so the toHaveLength(28) drift assertion still holds. The PR-body warning was about sibling PRs fix(save): route anonymous save-intent to signup, not the sign-in page #916–feat(hutch): emit signup_attempted to make the signup form measurable #918, which are not yet merged, so no widget-count reconciliation was needed.
• pnpm nx run hutch:check passes — lint, type-check, all unit/integration/E2E tests (incl. checkout-success.route.test.ts, account.route.test.ts, subscription-events.test.ts), knip, and coverage (functions 100%, branches 97.01%, statements/lines 99.48% — all above threshold).

Force-pushed with --force-with-lease (e8174d90 → ef7b2f4d); the lease verified the remote hadn't moved before rewriting. The PR is now conflict-free and ready.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: ef7b2f4

[claude]

Claude finished @FagnerMartinsBrack's task in 4m 47s —— View job

---

Claude Code Review — PR #935 (re-review @ ef7b2f4d, post-rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the rebased head, re-traced the checkout-success and subscribe control flow in their merged-with-#939 state, re-checked widget geometry, and closed the one funnel-symmetry question the earlier passes left implicit. Clean, well-scoped, pure instrumentation.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier reviews and remain accurate at ef7b2f4d. Repeating them tersely for the record — none are blocking.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:455-480 — the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write). A successful charge followed by a failing upsert would emit variant: card_decline_fallback despite no decline. The instrumentation faithfully mirrors the existing control flow, so this is a dashboard-reading nuance, not a bug. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

account.page.ts:461-466 redirects on success without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:441 asserts it. Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

What I verified independently

• Funnel symmetry (new this pass) — I traced every producer of a pending signup. PendingSignup.method is a single literal "existing-user-subscribe" (src/packages/provider-contracts/src/pending-signup.ts:8) and the only storePendingSignup caller in the web runtime is startCheckout (account.page.ts:397). The email/google/apple method values are emitUserCreated conversion-event fields, not pending-signup methods, and those signup paths never touch Stripe Checkout. So every checkout_completed on /auth/checkout/success is necessarily preceded by a checkout_started — no unpaired signup-path completions leak into the funnel. The started→completed relationship is sound; the only gap is the saved-card resub (item 2). ✓
• No double-emit — each branch of GET /auth/checkout/success returns after exactly one emit; the replay path emits checkout_completed on the first visit and checkout_return_failed(replayed) on the second, never both in one request. checkout-success.route.test.ts:90-101 asserts the 2-event total. ✓
• Emit placement — checkoutStarted fires after the Checkout session + pending-signup write, so a throw in either routes to the payment_method catch with no event (account.route.test.ts:541 asserts 0). checkoutCompleted uses pending.userId (the returning visitor may be anonymous), not req.userId. ✓
• req.userId optionality — /auth/checkout/success is not behind requireAuth, so renderFailure correctly treats user_id as optional; SubscriptionLogEvent.user_id is now ? and no consumer dereferences it. ✓
• Widget geometry (post-rebase) — "Checkout funnel per day" at x:12,y:122,w:12,h:8 sits beside "Homepage A/B" (x:0,y:122) on a fresh row below the full-width errors widget (y:114, h:8, ending at y:122). No overlap; feat(conversion): pre-expiry trial reminder email (R1) #939 didn't touch analytics-dashboard.ts, so toHaveLength(28) holds and the count comment sums to 28 (7+3+3+3+2+1+3+1+2+1+1+1). ✓
• Branch coverage of the two spread-ternaries in checkoutReturnFailed — both truthy (not_paid unit test) and falsy (invalid_query unit test) outcomes for user_id and checkout_session_id are exercised. ✓
• Guideline compliance — compile-time union constants over runtime validation, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, HutchLogger.fromJSON at the composition root, Zod at the HTTP boundary, branded ids, named-object params, no as casts. ✓

Static review only — this commit is the conflict-fixer's rebase head, which reported hutch:check green (coverage functions 100% / branches 97.01%, above threshold). I did not re-run pnpm check for this pure-instrumentation change.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Branch: conversion/checkout-instrumentation

[claude]

Claude finished @FagnerMartinsBrack's task in 13m 46s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions & PR intent
• Rebase onto origin/main and identify conflicts
• Resolve conflicts (cross-referencing PR intent)
• Run hutch:check to verify
• Force-push with --force-with-lease

---

Rebased conversion/checkout-instrumentation onto current main (6ebaa1f0). Since the last rebase, main advanced by 3 commits — the collision came from PR #940 "self-serve account deletion" (a77a0f30) landing in the same files this PR instruments.

One real conflict, in account.page.ts (3 hunks). The other four overlapping files (app.ts, server.ts, test-app.ts, account.route.test.ts) auto-merged — main's additions and this PR's are in disjoint regions.

The conflict was two independent, intentional changes hitting the same lines:

Side	Change to the three checkout-fallback call sites
main (#940)	renamed redirectToCheckout → redirectFullPage (generalized the helper + reused it for the account-deletion logout redirect, redirectFullPage(req, res, "/"))
this PR (R2)	added the variant argument to startCheckout(...) + the checkoutStarted emit

Resolution — kept both: each site is now startCheckout(req, { variant: … }) calling redirectFullPage(...). The PR branched before the rename, so its side still referenced the now-removed redirectToCheckout; keeping that name would not compile.

Verification

• No conflict markers remain repo-wide; redirectToCheckout is gone everywhere (rename fully adopted). The PR's account.page.ts diff vs main shows only its instrumentation additions — the redirectFullPage lines appear as unchanged context, so main's rename was adopted, not reverted.
• Post-rebase diff vs main is still exactly +347 / −31 across 12 files — nothing dropped or duplicated. The PR's imports, the Pick<EmitSubscriptionEvent, "checkoutStarted"> dep, all three variants, and the emit all survived.
• Dashboard drift test: none of the 3 new main commits touched analytics-dashboard.ts, so the toHaveLength(28) assertion still holds — no widget-count reconciliation needed.
• pnpm nx run hutch:check passes — lint, type-check, all unit/integration/E2E tests (incl. checkout-success.route.test.ts, account.route.test.ts, subscription-events.test.ts), knip, and coverage (functions 100%, branches 97.06%, statements/lines 99.49% — all above threshold).

Force-pushed with --force-with-lease (ef7b2f4d → 3319453b); the lease was pinned to the expected remote SHA and verified the branch hadn't moved before rewriting. The PR is now conflict-free and green.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: 3319453

[claude]

Claude encountered an error —— View job

---

Claude Code Review — PR #935 (re-review @ 3319453b)

Working…

• Read reviewer instructions & PR intent
• Pull full diff at 3319453b (post-feat(hutch): self-serve account deletion (App Store 5.1.1(v)) #940 rebase)
• Trace checkout-success + subscribe control flow
• Verify emit placement, event fields, branch coverage
• Check widget geometry & dashboard drift assertions
• Verify guideline compliance (CLAUDE.md)
• Post review

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: b0cb4f7

[claude]

Claude finished @FagnerMartinsBrack's task in 4m 14s —— View job

---

Claude Code Review — PR #935 (re-review @ b0cb4f7c)

Re-reviewed the full diff (git diff origin/main...HEAD) at the squashed single-commit head, re-traced the checkout-success + subscribe control flow, and independently re-verified emit placement, the user_id optionality blast radius, composition-root wiring, and widget geometry. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at b0cb4f7c. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:498-511 — the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write). A successful charge followed by a failing upsert would emit variant: card_decline_fallback despite no decline. The instrumentation faithfully mirrors the existing control flow, so this is a dashboard-reading nuance, not a bug. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

account.page.ts:486-497 redirects on success without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:439-441 asserts it. Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts checkoutCompleted emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

What I verified independently

• user_id optionality blast radius — widening SubscriptionLogEvent.user_id to ? is type-safe: the only reads of .user_id on a subscription event are in the two new test files (which correctly assert undefined); the subscription Lambda *.main.ts files use SubscriptionLogEvent only as a HutchLogger.fromJSON<> / HutchLogger.Typed<> type param, never dereferencing fields. The chargeSucceeded/chargeFailed/cancelled emitters still require userId: UserId, so their events keep user_id. ✓
• Composition-root wiring — the only two createApp callers (app.ts prod root → subscriptionLogger: HutchLogger.fromJSON<…>(); test-app.ts → capturing subscriptionBundle.logger) both supply the now-required subscriptionLogger; initAuthRoutes + initAccountRoutes are each wired once in server.ts with a Pick<>-narrowed emitSubscriptionEvent. No caller left unsatisfied. ✓
• Emit placement — checkoutStarted fires after the Checkout session + pending-signup write, so a throw in either routes to the payment_method catch with no event (account.route.test.ts:538 asserts 0). checkoutCompleted uses pending.userId (the returning visitor may differ from req.userId) after the upsert + schedule deletes. checkoutReturnFailed runs in the hoisted renderFailure for all four failure branches, using optional req.userId. ✓
• No double-emit — each /auth/checkout/success branch returns after exactly one emit; the replay path emits checkout_completed on the first visit and checkout_return_failed(replayed) on the second (checkout-success.route.test.ts asserts the 2-event total). ✓
• Widget geometry — "Checkout funnel per day" at x:12,y:122,w:12,h:8 sits beside "Homepage A/B" (x:0,y:122) on a fresh row below the full-width errors table (x:0,y:114,w:24,h:8, ending at y:122). No overlap; the geometric no-overlap test still holds. Count comment sums to 28 (7+3+3+3+2+1+3+1+2+1+1+1) and both drift assertions were updated; the new test pins the checkout widget to the hutch handler log group. ✓
• Guideline compliance — compile-time union constants (CHECKOUT_VARIANTS, CHECKOUT_RETURN_FAILURE_REASONS) over runtime validation, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, HutchLogger.fromJSON at the composition root, Zod at the HTTP boundary, branded ids, named-object params, assert for invariants, no as casts. ✓

Static review only — I did not re-run pnpm check for this pure-instrumentation change. The conflict-fixer reported hutch:check green (functions 100% / branches 97.06%) on the pre-squash head, and this commit is that content squashed.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

Claude Code Review — PR #935 (re-review @ b0cb4f7c)

Re-reviewed the full diff (git diff origin/main...HEAD) at the squashed single-commit head, re-traced the checkout-success + subscribe control flow, and independently re-verified emit placement, the user_id optionality blast radius, composition-root wiring, and widget geometry. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at b0cb4f7c. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:498-511 — the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write). A successful charge followed by a failing upsert would emit variant: card_decline_fallback despite no decline. The instrumentation faithfully mirrors the existing control flow, so this is a dashboard-reading nuance, not a bug. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

account.page.ts:486-497 redirects on success without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:439-441 asserts it. Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts checkoutCompleted emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Branch: conversion/checkout-instrumentation

[claude]

Claude finished @FagnerMartinsBrack's task in 15m 56s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions & PR intent
• Rebase onto origin/main and identify conflicts
• Resolve conflicts (cross-referencing PR intent)
• Run hutch:check to verify
• Force-push with --force-with-lease

---

Rebased conversion/checkout-instrumentation onto current main (3c527309). Since the last rebase, main advanced by several commits; two of them collided with this PR:

• PR feat(blog-analytics): blog pageviews, visitor ids, and first-touch attribution via @packages/web-analytics #941 "blog pageviews … via @packages/web-analytics" — (a) extracted the analytics middleware + AnalyticsEvent type out of hutch into a new workspace package @packages/web-analytics, and (b) added a "Blog pageviews by path" dashboard widget into the exact slot (x:12, y:122) this PR's "Checkout funnel per day" widget used.
• PR feat(hutch): require typing the confirmation phrase to delete an account #945 "confirm-phrase account deletion" — introduced a new workspace package @packages/escape-regexp (a @packages/domain dependency), so a fresh pnpm install was needed to wire up the symlink after rebasing.

5 conflicted files — all resolved additively (the two features are independent; nothing dropped):

File	main side	This PR	Resolution
app.ts	AnalyticsEvent from @packages/web-analytics	SubscriptionLogEvent + AnalyticsEvent from old local path	Kept PR's SubscriptionLogEvent; adopted main's relocated AnalyticsEvent import
test-app.ts	same relocation	same	Kept main's package import + PR's SubscriptionLogEvent
server.ts	middleware imported from @packages/web-analytics	initEmitSubscriptionEvent + local middleware imports	Kept PR's subscription-events import; dropped the now-moved local middleware imports (main supplies them from the package)
analytics-dashboard.ts	"Blog pageviews by path" widget at x:12, y:122	"Checkout funnel per day" at x:12, y:122	Kept both; moved checkout-funnel to a fresh row x:0, y:130 to clear the collision (per the PR body's slot-collision instruction)
analytics-dashboard.test.ts	count → 28 (+1 blog-traffic)	count → 28 (+1 checkout-funnel)	Merged: count → 29, category list carries both 1 blog-traffic + 1 checkout-funnel

The PR branched before #941's @packages/web-analytics extraction, so its side still imported the now-deleted local ./web/middleware/analytics, ./web/click-attribution.middleware, and ./web/visitor-id.middleware files — keeping those would not compile. Main's package-based imports were adopted, not reverted; the two dropped server.ts middleware imports are still satisfied by main's @packages/web-analytics import block above.

Verification

• No conflict markers remain repo-wide. Post-rebase diff vs main is still exactly +347 / −31 across 12 files — the instrumentation (all three events, the Pick<>-narrowed emitSubscriptionEvent deps, both union constants) is fully intact.
• Dashboard geometry: bottom three rows are y:114 (full-width errors, w:24) → y:122 (Homepage A/B x:0 + Blog pageviews x:12, both w:12) → y:130 (Checkout funnel x:0, w:12, alone). The geometric no-overlap test and the toHaveLength(29) drift test both hold; the checkout-widget test asserts only its query/log-group, not position, so the move is safe.
• pnpm nx run hutch:check passes — lint, type-check, all unit/integration/E2E tests (incl. checkout-success.route.test.ts, account.route.test.ts, subscription-events.test.ts, analytics-dashboard.test.ts), knip, and coverage (functions 100%, branches 97.04%, statements/lines 99.47% — all above threshold). A pnpm install was required first to wire up main's new @packages/escape-regexp workspace package.

Force-pushed with --force-with-lease (b0cb4f7c → 64568b50); the lease was pinned to the expected remote SHA and verified the branch hadn't moved before rewriting. The PR is now conflict-free and green.

· branch conversion/checkout-instrumentation ·

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: 64568b5

[claude]

Claude finished @FagnerMartinsBrack's task in 3m 30s —— View job

---

Claude Code Review — PR #935 (re-review @ 64568b50, post-#941/#945 rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the current head, re-traced the checkout-success + subscribe control flow, and independently re-verified the two areas this latest rebase actually moved: the dashboard widget geometry (now x:0,y:130, count 28 → 29) and the @packages/web-analytics import relocation across the three composition roots. The emit logic in auth.page.ts / account.page.ts is unchanged from the previously-reviewed head. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at 64568b50. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:507-532 — the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write). A successful charge followed by a failing upsert would emit variant: card_decline_fallback despite no decline. The instrumentation faithfully mirrors the existing control flow (which already falls back to Checkout on any throw here), so this is a dashboard-reading nuance, not a bug. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

The saved-card success branch (account.page.ts:518) redirects without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:513 asserts it (zero checkout_started on that path). Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising the follow-up so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

What I verified independently at this head

• Widget geometry & count (the thing this rebase moved) — counted 29 positioned widgets, exactly matching toHaveLength(29) and the updated count comment (… 1 blog-traffic, 1 checkout-funnel). Bottom three rows: y:114 full-width errors (x:0,w:24) → y:122 Homepage A/B (x:0,w:12) + Blog pageviews (x:12,w:12) → y:130 Checkout funnel (x:0,w:12, alone). Only one widget sits at y:130, so the geometric no-overlap test holds; the #941 collision at the old x:12,y:122 slot was correctly cleared by moving to a fresh row. ✓
• Checkout widget sources the right log group — the new widget uses logGroupNames: [hutchLogGroupName] (checkout events come from the web app, not the subscription Lambdas). The pre-existing "one SOURCE per subscription group" drift test was correctly narrowed to exclude exactly the hutch-prefixed query (!q.startsWith("SOURCE '${LOG_GROUPS.hutchHandler}' | ")), and a new dedicated test pins the checkout widget to the hutch handler group + asserts its three events and the stats … by bin(1d), event shape. The exclusion filters only this one widget; genuine subscription-Lambda widgets still start with the SUBSCRIPTION_DASHBOARD_LOG_GROUPS prefix. ✓
• web-analytics relocation resolved correctly — app.ts/test-app.ts/server.ts adopt main's AnalyticsEvent from @packages/web-analytics (appears as unchanged context, i.e. not reverted) while adding this PR's locally-defined SubscriptionLogEvent; the now-required subscriptionLogger is supplied at both createApp call sites (HutchLogger.fromJSON in prod, capturing bundle in tests). ✓
• Emit placement unchanged & sound — checkoutStarted fires after the Checkout session + pending-signup write (a throw in either routes to the payment_method catch with no event; account.route.test.ts:610 asserts 0). checkoutCompleted uses pending.userId after the upsert + schedule deletes. checkoutReturnFailed runs in the hoisted renderFailure for all four failure branches with optional req.userId (route is not behind requireAuth). No double-emit — each branch returns after one emit; the replay path emits completed then return_failed across two requests (checkout-success.route.test.ts asserts the 2-event total). ✓
• Branch coverage of the two spread-ternaries in checkoutReturnFailed — truthy (not_paid unit test: both ids present) and falsy (invalid_query unit test: neither) outcomes for user_id and checkout_session_id are both exercised, so no uncovered V8 branch. ✓
• Comment policy — the one added code comment (dashboard, explaining why the widget sources the hutch group rather than the subscription Lambdas) is a justified why comment; the added test comments explain why each event-count assertion holds. No what/how narration. ✓
• Guideline compliance — compile-time union constants (CHECKOUT_VARIANTS, CHECKOUT_RETURN_FAILURE_REASONS) over runtime validation, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, HutchLogger.fromJSON at the composition root, Zod at the HTTP boundary, branded ids, named-object params, assert for invariants, no as casts. ✓

Static review only — I did not run pnpm check for this pure-instrumentation change. The conflict-fixer reported hutch:check green at this exact head (64568b50): functions 100%, branches 97.04%, statements/lines 99.47% — all above threshold.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

Claude Code Review — PR #935 (re-review @ 64568b50, post-#941/#945 rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the current head, re-traced the checkout-success + subscribe control flow, and independently re-verified the two areas this latest rebase actually moved: the dashboard widget geometry (now x:0,y:130, count 28 → 29) and the @packages/web-analytics import relocation across the three composition roots. The emit logic in auth.page.ts / account.page.ts is unchanged from the previously-reviewed head. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at 64568b50. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:507-532 — the catch wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write). A successful charge followed by a failing upsert would emit variant: card_decline_fallback despite no decline. The instrumentation faithfully mirrors the existing control flow (which already falls back to Checkout on any throw here), so this is a dashboard-reading nuance, not a bug. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal.

2. One-click-resubscribe success path still emits nothing (documented follow-up)

The saved-card success branch (account.page.ts:518) redirects without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:513 asserts it. Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising the follow-up so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Branch: conversion/checkout-instrumentation

[claude]

Claude finished @FagnerMartinsBrack's task in 11m 8s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions & PR intent
• Rebase onto origin/main and identify conflicts
• Resolve conflicts (cross-referencing PR intent)
• Run hutch:check to verify
• Force-push with --force-with-lease

---

Rebased conversion/checkout-instrumentation onto current main (f1c6149e). Since the last rebase, main advanced by 15 commits; one collided with this PR: 58c287c6 "rename stripe contracts to provider-agnostic ports", which renamed the checkout-success test helper completeStripeSignup (./test-helpers/complete-stripe-signup) → completeCheckoutSignup (./test-helpers/complete-checkout-signup).

One conflicted file — web/auth/checkout-success.route.test.ts (2 hunks). The other 11 files auto-merged (main's rename and this PR's instrumentation live in disjoint regions). Both sides had edited the same two lines:

Line	main (58c287c6 rename)	This PR (R2)	Resolution — kept both
helper import	completeCheckoutSignup from ./test-helpers/complete-checkout-signup	added CHECKOUT_RETURN_FAILURE_REASONS from ../../observability/events (+ old helper name)	main's renamed import + this PR's new CHECKOUT_RETURN_FAILURE_REASONS
"marks active" test call	const { successResponse }	const { successResponse, checkoutSessionId } (old name)	const { successResponse, checkoutSessionId } = await completeCheckoutSignup({

The PR branched before the rename, so its side still referenced the now-deleted completeStripeSignup / complete-stripe-signup — keeping that would not compile. Main's rename was adopted, not reverted: the other two call sites (:78 replay test, :143 active-row test) already carried main's renamed name via 3-way merge, and I verified main's completeCheckoutSignup returns { successResponse, checkoutSessionId }, so this PR's checkout_session_id assertion (:134) still binds.

Verification

• No conflict markers remain repo-wide; no completeStripeSignup / complete-stripe-signup references left anywhere. No stale old-contract (stripe-contracts) imports in the changed runtime files.
• Post-rebase diff vs main is still exactly +347 / −31 across 12 files — the instrumentation (all three events, the Pick<>-narrowed emitSubscriptionEvent deps, both union constants, the dashboard widget) is fully intact, nothing dropped or duplicated.
• Dashboard drift test: none of main's 15 new commits touched the checkout widget's slot, so analytics-dashboard.ts auto-merged and the widget-count drift assertion still holds — no widget reconciliation needed.
• pnpm nx run hutch:check passes — lint, type-check, all unit/integration/E2E tests (incl. checkout-success.route.test.ts, account.route.test.ts, subscription-events.test.ts, analytics-dashboard.test.ts; E2E 9 passed), knip, and coverage (functions 100%, branches 97.08%, statements/lines 99.47% — all above threshold).

Force-pushed with --force-with-lease (64568b50 → bf078bf4); the lease was pinned to the fetched remote SHA and verified the branch hadn't moved before rewriting. The PR is now conflict-free and green.

· branch conversion/checkout-instrumentation ·

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: bf078bf

[claude]

Claude finished @FagnerMartinsBrack's task in 3m 3s —— View job

---

Claude Code Review — PR #935 (re-review @ bf078bf4, post-#58c287c6 rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the current head, re-traced the checkout-success + subscribe control flow, and independently re-verified the widget geometry, the test-helper rename from this rebase, and the emit placement/branch coverage. The runtime instrumentation is byte-identical to the previously-reviewed 64568b50 head — the only delta this rebase introduced is the completeStripeSignup → completeCheckoutSignup test-helper rename (from #58c287c6), confined to checkout-success.route.test.ts. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at bf078bf4. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:535-560 — the try wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write), and the single catch emits variant: card_decline_fallback for any throw. A successful charge followed by a failing upsert would be labelled a decline despite none. The emit faithfully mirrors the pre-existing control flow (which already falls back to Checkout on any throw — and, pre-existing/out-of-scope, could start a fresh Checkout on an already-charged customer), so this is a dashboard-reading nuance, not a bug this PR introduces. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

The saved-card success branch (account.page.ts:546) redirects without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:513 asserts it (zero checkout_started on that path). Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising the follow-up so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

What I verified independently at this head

• Test-helper rename (the only thing this rebase moved) — confirmed no completeStripeSignup / complete-stripe-signup references remain anywhere under projects/hutch/src; all four call sites use completeCheckoutSignup, and the helper returns { successResponse, checkoutSessionId: checkout.id } — a real id — so the new expect(evt.checkout_session_id).toBe(checkoutSessionId) assertion (:121) is genuine, not undefined === undefined. The runtime files (auth.page.ts, account.page.ts, subscription-events.ts, events.ts, analytics-dashboard.ts) are unchanged from 64568b50. ✓
• Widget geometry & count — counted 29 positioned widgets, exactly matching toHaveLength(29) and the updated count comment (… 1 blog-traffic, 1 checkout-funnel). Bottom three rows: y:114 full-width errors (x:0,w:24) → y:122 Homepage A/B (x:0,w:12) + Blog pageviews (x:12,w:12) → y:130 Checkout funnel (x:0,w:12, alone on the row). Only one widget at y:130, so the geometric no-overlap test holds. ✓
• Checkout widget sources the right log group — logGroupNames: [hutchLogGroupName] (checkout events come from the web app, not the subscription Lambdas). The pre-existing "one SOURCE per subscription group" drift test was correctly narrowed to exclude exactly the hutch-prefixed query, and a dedicated new test pins the widget to the hutch handler group + its three events + the stats … by bin(1d), event shape. ✓
• Emit placement & no double-emit — checkoutStarted fires after the Checkout session + pending-signup write, so a throw in either routes to the payment_method catch with no event (account.route.test.ts:610 asserts 0). checkoutCompleted uses pending.userId after the upsert + schedule deletes. checkoutReturnFailed runs in the hoisted renderFailure for all four failure branches with optional req.userId (route is not behind requireAuth). Each /auth/checkout/success branch returns after exactly one emit; the replay path emits completed then return_failed across two requests (checkout-success.route.test.ts asserts the 2-event total). ✓
• Branch coverage of the two spread-ternaries in checkoutReturnFailed — truthy (not_paid unit test: both ids present) and falsy (invalid_query unit test: neither) outcomes for user_id and checkout_session_id are both exercised, so no uncovered V8 branch. ✓
• Composition-root wiring — both createApp callers supply the now-required subscriptionLogger (HutchLogger.fromJSON<SubscriptionLogEvent>() in app.ts; capturing bundle in test-app.ts); initAuthRoutes + initAccountRoutes each receive a Pick<>-narrowed emitSubscriptionEvent in server.ts. Widening SubscriptionLogEvent.user_id to optional is type-safe — the always-present emitters (chargeSucceeded/chargeFailed/cancelled) still require userId: UserId, and no consumer dereferences .user_id off a subscription event outside the two new test files. ✓
• Guideline compliance — compile-time union constants (CHECKOUT_VARIANTS, CHECKOUT_RETURN_FAILURE_REASONS) over runtime validation, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, HutchLogger.fromJSON at the composition root, Zod at the HTTP boundary, branded ids, named-object params, assert for invariants, no as casts. ✓

Static review only — I did not re-run pnpm check for this pure-instrumentation change. The conflict-fixer reported hutch:check green at this exact head (bf078bf4): functions 100%, branches 97.08%, statements/lines 99.47% — all above threshold.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:535-560 — the try wraps both createSubscriptionOnExistingCustomer (charge) and upsertActiveSubscription (DB write); the single catch emits variant: card_decline_fallback for any throw. Faithfully mirrors pre-existing control flow, so a dashboard-reading nuance, not a bug this PR introduces.

2. One-click-resubscribe success path still emits nothing (documented follow-up)

account.page.ts:546 redirects without a checkout_completed. Flagged as out-of-scope follow-up and asserted in tests. Funnel undercounts saved-card resub revenue.

3. checkout_completed carries no variant

subscription-events.ts:79-88 omits the originating variant, so a per-variant completion split needs a self-join on checkout_session_id. Aggregate conversion unaffected. Informational.

[FagnerMartinsBrack]

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Branch: conversion/checkout-instrumentation

[claude]

Claude finished @FagnerMartinsBrack's task in 12m 42s —— View job

---

Conflicts resolved ✅

• Read conflict-fixer instructions & PR intent
• Rebase onto origin/main and identify conflicts
• Resolve conflicts (cross-referencing PR intent)
• Run hutch:check to verify
• Force-push with --force-with-lease

---

Rebased conversion/checkout-instrumentation onto current main (78dfa44b). Since the last rebase, main advanced by 9 commits; the collision came from the @packages/web-analytics line of work (#941 + 78dfa44b "suppress click-counting on bot fake-success redirects"), which now applies the analytics middleware via an express layer (createAnalyticsMiddleware) instead of inside createApp.

One conflicted file — test-app.ts (2 hunks). The other 11 files auto-merged (main's changes and this PR's instrumentation live in disjoint regions). Both hunks were two independent, intentional changes hitting the same lines, resolved additively:

Hunk	main side	This PR (R2)	Resolution — kept both
imports	import { type AnalyticsEvent, createAnalyticsMiddleware } from @packages/web-analytics	added import type { SubscriptionLogEvent }	main's value import of createAnalyticsMiddleware + this PR's SubscriptionLogEvent
createTestApp body	express().use(createAnalyticsMiddleware({…})).use(createApp(…))	subscription-events capture bundle + flatten(fixture, analyticsBundle, subscriptionBundle)	main's express+middleware wrapping wrapping createApp, with this PR's subscriptionBundle set up and passed as the 3rd flatten arg

The PR branched before the middleware relocation, so its side still passed analytics into createApp the old way; keeping main's express layer means the relocation was adopted, not reverted. The auto-merged flattenFixtureToAppDependencies already carried the correct 3-arg signature and still passes main's analytics/salt and this PR's subscriptionLogger — I verified app.ts/server.ts diffs vs main show only this PR's additive subscriptionLogger + emitSubscriptionEvent wiring, with main's analytics deps intact as unchanged context.

One environment step: main's #950 "extract the inbox into its own deployable" introduced a new @packages/inbox-store workspace package, so a pnpm install was needed after rebasing to wire up the symlink — without it, four unrelated packages (inbox-store, article-store, web-shell, subscription-access) failed to compile against stale workspace state. Not a code conflict; purely stale local wiring.

Verification

• No conflict markers remain repo-wide. Post-rebase diff vs main is still exactly +347 / −31 across 12 files — every event, the Pick<>-narrowed emitSubscriptionEvent deps, both union constants, and the dashboard widget are fully intact, nothing dropped or duplicated.
• Dashboard drift test: none of main's 9 new commits touched the checkout widget's slot or analytics-dashboard.ts, so the widget-count drift assertion still holds — no widget reconciliation needed.
• pnpm nx run hutch:check passes — lint, type-check, all unit/integration/E2E tests (incl. checkout-success.route.test.ts, account.route.test.ts, subscription-events.test.ts, analytics-dashboard.test.ts; E2E 9 passed), knip, and coverage (functions 100%, branches 97.01%, statements/lines 99.46% — all above threshold).

Force-pushed with --force-with-lease (bf078bf4 → 31767483); the lease was pinned to the expected remote SHA and verified the branch hadn't moved before rewriting. The PR is now conflict-free and green.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(observability): instrument the checkout funnel with started/completed/failed events #935
• Commit: 3176748

[claude]

Claude finished @FagnerMartinsBrack's task in 5m 4s —— View job

---

Claude Code Review — PR #935 (re-review @ 31767483, post-#941/#950 rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the current head, re-traced the checkout-success + subscribe control flow, and independently re-verified the areas this latest rebase actually moved — the @packages/web-analytics middleware relocation in test-app.ts (createAnalyticsMiddleware express layer) — plus the funnel-symmetry, composition-root, and user_id-optionality invariants from scratch. The runtime emit logic is unchanged from the previously-reviewed head. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at 31767483. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:535-560 — the try wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write), and the single catch emits checkout_started with variant: card_decline_fallback for any throw. A successful charge followed by a failing upsert would be labelled a decline despite none. The emit faithfully mirrors the pre-existing control flow (which already falls back to Checkout on any throw), so this is a dashboard-reading nuance, not a bug this PR introduces. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal. Fix this →

2. One-click-resubscribe success path still emits nothing (documented follow-up)

The saved-card success branch (account.page.ts:546) redirects without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:513 asserts it (zero checkout_started on that path). Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising the follow-up so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.

What I verified independently at this head

• Funnel symmetry (started always precedes completed) — re-derived from the contract: PendingSignup.method is a single literal "existing-user-subscribe" (@packages/provider-contracts/pending-signup.ts), and the only production storePendingSignup caller is startCheckout (account.page.ts:476), which emits checkout_started immediately after (:487). /auth/checkout/success reaches checkout_completed only by consuming that pending row. So every checkout_completed is necessarily preceded by a checkout_started — no unpaired signup-path completions leak into the funnel; the only gap is the saved-card resub (item 2). ✓
• web-analytics relocation (the delta this rebase moved) — test-app.ts now wraps createApp in an express().use(createAnalyticsMiddleware({…})) layer (adopting main's feat(blog-analytics): blog pageviews, visitor ids, and first-touch attribution via @packages/web-analytics #941/feat(inbox): extract the inbox into its own deployable (phases A–C: pre-refactors, seam, additive project) #950 extraction, not reverting it) while adding this PR's SubscriptionEventsBundle capture and threading it as the 3rd flattenFixtureToAppDependencies arg. The capturing logger mirrors the existing AnalyticsBundle pattern exactly. ✓
• Composition-root wiring — all four createApp/createHutchApp call sites satisfy the now-required subscriptionLogger: app.ts:738 (HutchLogger.fromJSON<SubscriptionLogEvent>()), test-app.ts:262 (capturing bundle), and the two arg-less createHutchApp() roots (lambda.main.ts, server.main.ts) inherit app.ts's default. server.ts builds emitSubscriptionEvent once and hands each route a Pick<>-narrowed slice (checkoutStarted to account, checkoutCompleted/checkoutReturnFailed to auth). ✓
• user_id → optional is type-safe — grepped every .user_id read: all are in test files; no production consumer dereferences it. The subscription-Lambda *.main.ts files use SubscriptionLogEvent only as a HutchLogger.fromJSON<> type param. The always-present emitters (chargeSucceeded/chargeFailed/cancelled) still require userId: UserId, so their events keep user_id. ✓
• Emit placement & no double-emit — checkoutStarted fires after the Checkout session + pending-signup write, so a throw in either routes to the payment_method catch with no event (account.route.test.ts:610 asserts 0). checkoutCompleted uses pending.userId (not req.userId) after the upsert + both schedule deletes, with a validated subscriptionId (asserted non-null). checkoutReturnFailed runs in the hoisted renderFailure for all four failure branches with optional req.userId. Each /auth/checkout/success branch returns after exactly one emit; the replay path emits completed then return_failed across two requests (checkout-success.route.test.ts asserts the 2-event total). ✓
• Widget geometry & count — bottom three rows: y:114 full-width errors (x:0,w:24) → y:122 Homepage A/B (x:0,w:12) + Blog pageviews (x:12,w:12, the feat(blog-analytics): blog pageviews, visitor ids, and first-touch attribution via @packages/web-analytics #941 addition) → y:130 Checkout funnel (x:0,w:12, alone on the row). The checkout widget was correctly moved off the collided x:12,y:122 slot to a fresh row; only one widget sits at y:130, so the geometric no-overlap test holds and the count comment sums to 29 matching toHaveLength(29). The new dedicated test pins the widget to the hutch handler log group (checkout events come from the web app, not the subscription Lambdas) and the "one SOURCE per subscription group" drift test was narrowed to exclude exactly that hutch-prefixed query. ✓
• Branch coverage of the two spread-ternaries in checkoutReturnFailed — truthy (not_paid unit test: both ids present) and falsy (invalid_query unit test: neither) outcomes for user_id and checkout_session_id are both exercised, so no uncovered V8 branch. ✓
• Guideline compliance — compile-time union constants (CHECKOUT_VARIANTS, CHECKOUT_RETURN_FAILURE_REASONS) over runtime validation, Pick<> interface segregation per route, required (non-defaulted) emitSubscriptionEvent dep, HutchLogger.fromJSON at the composition root, Zod at the HTTP boundary, branded ids, named-object params, assert for invariants, no as casts. The two added code comments (dashboard: why the widget sources the hutch group; account: why the catch falls through to Checkout) are justified why comments. ✓

Static review only — I did not run pnpm check for this pure-instrumentation change. The conflict-fixer reported hutch:check green at this exact head (31767483): functions 100%, branches 97.01%, statements/lines 99.46% — all above threshold.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· branch conversion/checkout-instrumentation

[FagnerMartinsBrack]

Claude Code Review — PR #935 (re-review @ 31767483, post-#941/#950 rebase)

Re-reviewed the full diff (git diff origin/main...HEAD) at the current head, re-traced the checkout-success + subscribe control flow, and independently re-verified the areas this latest rebase actually moved — the @packages/web-analytics middleware relocation in test-app.ts (createAnalyticsMiddleware express layer) — plus the funnel-symmetry, composition-root, and user_id-optionality invariants from scratch. The runtime emit logic is unchanged from the previously-reviewed head. Clean, well-scoped, pure instrumentation — no behaviour change for users.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

(All three carry over from the earlier passes and remain accurate at 31767483. None blocking — repeated tersely for the record.)

1. card_decline_fallback labels any throw in the one-click-resub try, not just card declines

account.page.ts:535-560 — the try wraps both createSubscriptionOnExistingCustomer (the charge) and upsertActiveSubscription (the DB write), and the single catch emits checkout_started with variant: card_decline_fallback for any throw. A successful charge followed by a failing upsert would be labelled a decline despite none. The emit faithfully mirrors the pre-existing control flow (which already falls back to Checkout on any throw), so this is a dashboard-reading nuance, not a bug this PR introduces. Narrow the emit to only the createSubscriptionOnExistingCustomer failure if you want the label to be literal.

2. One-click-resubscribe success path still emits nothing (documented follow-up)

The saved-card success branch (account.page.ts:546) redirects without a checkout_completed. The PR body flags this as out-of-scope follow-up and account.route.test.ts:513 asserts it (zero checkout_started on that path). Funnel consequence: checkout_completed / distinct user_id on checkout_started undercounts revenue, because a saved-card resub has neither event. Worth prioritising the follow-up so the dashboard reads "0 leak" rather than "invisible conversions."

3. checkout_completed carries no variant

subscription-events.ts:79-88 emits user_id/subscription_id/checkout_session_id but not the originating variant, so a per-variant completion split needs a self-join on checkout_session_id back to checkout_started. Aggregate conversion (the PR's query) is unaffected. Informational.