fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion by FagnerMartinsBrack · Pull Request #934 · Readplace/readplace.com

FagnerMartinsBrack · 2026-07-05T13:55:25Z

fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion

Hypothesis

Four measurement defects make the funnel unreadable: (1) static-asset fetches are counted as pageviews, (2) the same article's traffic splits across scheme-variant reader paths, (3) every automatic trial expiration is labelled as a user-initiated cancel, and (4) the owner's own reading dominates readership metrics. Fixing measurement changes no user-facing behavior but makes every subsequent funnel PR (#916–#918) evaluable.

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

~57% of anonymous "pageviews" are asset requests: /toast.client.js 6,526 pv, /extension-suggestion-banner.client.js 6,510, /progress-bar.client.js 4,947, /view-paywall.client.js 3,936 pv / 2,923 visitors; one signup's landing_path is /client-dist/toast.client.js.
Root cause (reproduced locally on express@5.2.1): the analytics middleware wraps the whole app and reads req.path at res.on("finish"), but express.static and res.redirect respond terminally, so the router never restores the mount-trimmed req.url. /client-dist/toast.client.js therefore logs as /toast.client.js, and /view/https:/host/slug logs as /https:/host/slug. Click-attribution runs pre-mount, which is why landing_path shows the untrimmed variants — two different corruptions of the same requests.
Same article split: /fagnerbrack.com/learn-sql-… 1,173 pv/878 v vs /https:/fagnerbrack.com/learn-sql-… 756 pv/715 v (~40% of top-article traffic); landing_path /view/https:/… vs /view/… splits signup attribution 7 vs 5. (The routing-level 301 with query preservation already exists — PR feat(hutch): canonicalize /view URLs to drop https:// and url encoding #439, view.page.ts:141-148; the residual split is entirely analytics-side.)
All 28 end-of-trial expirations emitted cancelled reason=user_initiated_trial ~60 min after charge_failed reason=no_card_on_file — the reason is derived from row.subscriptionId alone at the final conversion, so "never added a card" is indistinguishable from "chose to cancel". Trial→paid is 0%; this label mislead is why churn looked voluntary.
Owner account c6f587a1… = 360 of 422 article_reads and is NOT excluded; production query (this PR's evidence) recovered its 22 device visitor_hashes covering 361 reads.

What this changes

Asset pageviews (a) — new isStaticAssetRequestPath derived from the actual static routes in server.ts (/client-dist mount, apple-touch-icon redirect, root-level trimmed bundle names). Applied in shouldLog and in click-attribution's landing capture. Explicitly not an extension regex: reader paths mirror arbitrary source URLs and may end .png/.html — covered by tests.
Path fidelity + scheme-variant merge (b) — snapshot req.path at middleware entry (kills both trimmed shapes for every route), stop counting 3xx responses as pageviews (the 301 canonicalization leg no longer double-counts), and canonicalize /view scheme-variant landing_path at cookie time via canonicalizeViewLandingPath.
Cancellation reason (c) — new reason trial_ended_no_payment_method. SubscriptionChargeFailed handling now publishes CancelSubscriptionCommand with trigger:"charge_failed", round-tripped through the deferred-cancellation EventBridge schedule Input, and resolved at the final conversion. The churned-trial feedback email filter accepts both trial reasons, so the only churn email keeps firing.
Owner exclusion (d) — 22 production-confirmed owner device hashes appended to hutch:excludedVisitorHashes (Pulumi prod config; dashboard-query-side only).

Estimated result

Anonymous pageview volume drops ~50–57% to human page renders only; per-page dashboards become trustworthy.
Top-article reader traffic consolidates onto one canonical path (~+65% on the canonical row, variants → 0); landing_path attribution merges (the 7-vs-5 split becomes a single 12).
cancelled events split into trial_ended_no_payment_method (expected ~28/mo today) vs true user_initiated_trial (expected ≈0) — the actual conversion-blocking problem (no card on file) becomes visible in one dashboard widget.
Readership metrics drop from 422 to ~60 reads/30d — the honest ~2-weekly-actives baseline the funnel PRs must move.

How to measure

CloudWatch Logs Insights on /aws/lambda/hutch-handler after deploy:

fields path
| filter stream="analytics" and event="pageview"
  and (strcontains(path, "/client-dist/") or path like /\.client\.js/ or path like /^\/(view\/)?https?:\//)
| stats count(*) as polluted by bin(1d)

Expect polluted = 0 from deploy day onward. For (c):

filter stream="subscriptions" and event="cancelled" | stats count(*) by reason

Expect trial_ended_no_payment_method at the next trial expiry, each followed by a [schedule-trial-feedback-email] schedule created line. For (d): the readership widget baseline steps down; raw logs still contain owner reads (exclusion is query-side, reversible).

Coordination note

analytics-dashboard.test.ts pins the widget count (toHaveLength(27) on current main). This PR adds no events or widgets and does not touch that assertion, but sibling PRs #916–#918 may bump it; on conflict, keep the sibling's count — R6 contributes +0. The four sub-fixes are grouped in separate commits ((a) asset exclusion, (b) path fidelity/3xx/landing canonicalization, (c) cancellation reason + feedback-email filter [revert together], (d) Pulumi hash list) so each is independently revertable.

https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9

FagnerMartinsBrack · 2026-07-05T14:02:32Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Commit: 272d323

claude · 2026-07-05T14:03:07Z

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

Claude PR Review — complete ✅

Read review instructions (.github/workflows/claude-PR-code-reviewer.md)
Pull full diff vs origin/main
Review (a) asset-pageview exclusion
Review (b) path fidelity / 3xx / landing canonicalization
Review (c) cancellation-reason plumbing + feedback-email filter
Review (d) Pulumi owner-hash exclusion
Check tests + CLAUDE.md guideline adherence
Emit final review block

Verdict: 0 high / 0 medium priority issues. A tight, well-tested measurement-hygiene PR. I traced each of the four sub-fixes end-to-end:

(a) Asset exclusion is guarded primarily by the robust /client-dist/ prefix check + the entry-time req.path snapshot; the trimmed-bundle/apple-touch regexes are correct defensive fallbacks.
(b) Snapshot-at-entry + 3xx exclusion + landing_path canonicalization coherently kill the scheme-variant split. Verified this does not regress the homepage A/B split (its redirect is client-side location.replace, so the / pageview is a 200, not a 3xx) nor the POST→303 click counting (shouldCountClick still allows 3xx).
(c) trigger round-trips charge-failed → command → trialing-branch → deferred schedule → pending_cancellation, and the paid discriminator (subscriptionId) correctly wins. zod-validated at the boundary; optional field keeps pre-deploy payloads backward-compatible.
(d) 22 hashes in Pulumi.prod.yaml (correct per-stage location), validated as lowercase-hex by the existing infra assertion, query-side only.

Full review is emitted as my final run message; the auto-apply workflow posts it as the review comment (per claude-PR-code-reviewer.md), so I'm not duplicating the CLAUDE_REVIEW block here.

FagnerMartinsBrack · 2026-07-05T14:12:10Z

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. `TRIMMED_BUNDLE_PATH` charset couples to the current bundle-naming convention

static-asset-paths.ts:10 — /^\/[a-z0-9-]+\.client\.js(\.map)?$/ only matches lowercase-hyphen bundle names. If a future client bundle is ever named with an uppercase letter, underscore, or camelCase (e.g. myWidget.client.js), the mount-trimmed shape would silently fall back to being counted as a pageview again. Real-world blast radius is small — the primary guard is the /client-dist/ prefix check combined with the entry-time req.path snapshot (analytics is the outermost wrapper in lambda.main.ts, and click-attribution runs pre-mount, so both consumers normally see the untrimmed /client-dist/... path). This regex is the defensive fallback for the trimmed shape, so the coupling is low-severity, but a broader [^/]+ (still anchored on the .client.js suffix, which no hostname carries) would make the fallback naming-agnostic. Fix this →

2. `canonicalizeViewLandingPath` diverges from `parseViewPath`'s canonical encoding

view-path.ts:53 — the routing canonical (parseViewPath) runs the tail through encodeArticlePathInfo (re-encoding %25/?/#), but canonicalizeViewLandingPath does not. For a /view/... URL whose article path contains a literal ?, #, or %, the stored landing_path and the actual canonical route path would differ slightly. This does not cause an attribution split (the first-touch cookie is written once, and both scheme-variant-first and canonical-first requests collapse to the same value), and such characters are rare in reader paths — so this is cosmetic. Worth a one-line comment noting the intentional divergence if it's deliberate (attribution grouping doesn't need routing-grade encoding fidelity).

3. `user_cancel` trigger value is a documented placeholder, never explicitly emitted

hutch-infra-components/src/events.ts:837 defines trigger: z.enum(["user_cancel", "charge_failed"]), but no caller passes "user_cancel" — the account-page cancel (account.page.ts:313) omits trigger entirely, and only the charge-failed handler sets "charge_failed". undefined and "user_cancel" both resolve to user_initiated_trial, so this is correct and the enum value usefully documents the default semantics; just noting it's semantically load-bearing only via the "absent ⇒ user_cancel" convention described in the event's doc comment.

FagnerMartinsBrack · 2026-07-09T09:02:02Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Commit: efa6d1d

claude · 2026-07-09T09:02:45Z

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

Claude PR Review — complete ✅

Read review instructions (.github/workflows/claude-PR-code-reviewer.md)
Pull full diff vs origin/main
Review (a) asset-pageview exclusion
Review (b) path fidelity / 3xx / landing canonicalization
Review (c) cancellation-reason plumbing + feedback-email filter
Review (d) Pulumi owner-hash exclusion
Check tests + CLAUDE.md guideline adherence
Emit final review block

Verdict: 0 high / 0 medium. Re-reviewed at efa6d1d0; traced all four sub-fixes end-to-end, including the production seams that fixture tests don't exercise:

(a) isStaticAssetRequestPath is anchored on the robust /client-dist/ prefix + entry-time req.path snapshot; the apple-touch classifier regex is identical to the real route regex (server.ts:493), and the trimmed-bundle fallback matches all 19 current *.client.ts bundles. Reader paths ending .png correctly stay pageviews (tested).
(b) Snapshot-at-entry + 3xx-drop + landing_path canonicalization are internally coherent: the /view scheme-variant returns a 301 (view.page.ts:145) that's now dropped, so only the canonical 200 destination is counted; the cookie path canonicalizes because it's written pre-redirect. Verified 303 POST clicks still count (shouldCountClick unchanged) and no existing test asserted a 3xx pageview.
(c) Confirmed the trigger round-trips through the real initEventBridgeCancelSubscriptionCommand (forwards full params) → Scheduler Input → pending_cancellation, and the paid discriminator (subscriptionId) wins. Only two real callers; both label correctly. Optional field keeps in-flight payloads backward-compatible.
(d) 22 hashes, no duplicates (verified), all lowercase-hex (infra assertion at index.ts:1497), query-side only.

Two low-priority maintainability nits (non-blocking) are in the review block. Full review is emitted as my final run message; the auto-apply workflow posts it as the review comment (per claude-PR-code-reviewer.md), so I'm not duplicating the CLAUDE_REVIEW block here.
• fix/analytics-data-hygiene

FagnerMartinsBrack · 2026-07-09T09:10:25Z

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. Apple-touch-icon regex is duplicated between the route and the classifier

static-asset-paths.ts:3 and server.ts:493 both hold the identical literal /^\/apple-touch-icon(?:-\d+x\d+)?(?:-precomposed)?\.png$/ — one as the actual redirect route, one as the analytics/click classifier. They must stay byte-for-byte in sync: if the route ever grows a new apple-touch shape, the classifier silently drifts and those requests get counted as pageviews again. This PR already established the single-source-of-truth pattern by extracting CLIENT_DIST_MOUNT_PATH and importing it into server.ts; the apple-touch regex is the one static-route constant that didn't get the same treatment. Low severity because the redirect is a 301 (already dropped by the new 3xx rule) and the classifier branch is defensive, but exporting the regex from static-asset-paths.ts and consuming it in server.ts would close the drift window and match the pattern the PR itself introduced. Fix this →

2. `TRIMMED_BUNDLE_PATH` charset couples to the current bundle-naming convention

static-asset-paths.ts:10 — /^\/[a-z0-9-]+\.client\.js(\.map)?$/ matches only lowercase-hyphen names. I confirmed all 19 current *.client.ts bundles are lowercase-hyphen, so it classifies every existing bundle today; the concern is purely forward-looking — a future myWidget.client.js/my_widget.client.js mount-trimmed shape would fall back to being counted as a pageview. The primary guard (/client-dist/ prefix + the entry-time req.path snapshot) still covers the normal path, so this fallback regex is low-severity, but anchoring on the .client.js suffix with a broader [^/]+ (no hostname carries that suffix) would make it naming-agnostic. (Same observation as the prior review — still valid at this commit.) Fix this →

(The two remaining items from the prior review — canonicalizeViewLandingPath not re-running encodeArticlePathInfo, and the user_cancel enum value never being explicitly emitted — remain accurate and are by-design/cosmetic; no action needed. The first doesn't cause an attribution split because the first-touch cookie is written once; the second is a documented default whose only live check is trigger === "charge_failed".)

FagnerMartinsBrack · 2026-07-09T15:42:40Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Branch: fix/analytics-data-hygiene

claude · 2026-07-09T15:43:20Z

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

Conflicts resolved ✅

Rebased fix/analytics-data-hygiene onto origin/main and force-pushed (efa6d1d0 → d145c453). pnpm check passes across all 40 projects.

Root cause of the conflicts: main (PR #941, blog analytics) extracted the analytics + click-attribution middleware into a shared @packages/web-analytics package now consumed by both hutch and blog-site. This PR had edited the old hutch-local copies. Because the shared package cannot import hutch-specific modules (isStaticAssetRequestPath, canonicalizeViewLandingPath — a package can't depend on a project), I preserved the PR's intent via an injection seam rather than reverting either side.

3 conflicted files:

File	Resolution
`web-analytics/src/analytics.ts`	Kept main's package-local imports + path-snapshot-at-entry (main independently landed the same fix (b) snapshot). Added injected `isStaticAssetPath` dep for fix (a); kept the general 3xx-drop (`statusCode < 200 \|\| >= 300`) in the shared `shouldLog` since it applies to both apps.
`web-analytics/src/click-attribution.middleware.ts`	Added injected `isStaticAssetPath` + `canonicalizeLandingPath` deps. Both main's `SKIP_PATHS` skip and the PR's static-asset skip now coexist; `landing_path` runs through the injected canonicalizer.
`hutch/src/runtime/server.ts`	Kept main's `@packages/web-analytics` imports, dropped the PR's now-obsolete local-middleware imports, kept `CLIENT_DIST_MOUNT_PATH`, added the two hutch fns for injection.

Composition-root wiring (preserves the PR's four fixes exactly):

hutch lambda.main.ts → injects isStaticAssetRequestPath into analytics.
hutch server.ts → injects isStaticAssetRequestPath + canonicalizeViewLandingPath into click-attribution.
blog app.ts → injects () => false + identity (blog serves shell assets from a separate origin, never through Express, and has no /view route).

Tests: the auto-merged shared-package tests were adapted to assert the injection contract (a flagged path is dropped / the canonicalizer's output is stored). The hutch path-shape specifics (/client-dist, trimmed bundles, apple-touch, /view/https:/ collapse) remain fully covered by hutch's static-asset-paths.test.ts and view-path.test.ts. The cancellation (c) and Pulumi owner-hash (d) fixes had no conflicts. No behavior change — the four measurement fixes land intact on top of main's refactor.

CI note: verification required a pnpm install first — the rebase pulled in main's newly-added @packages/escape-regexp workspace package, which the pre-rebase node_modules hadn't linked (unrelated to this PR's code).

Read conflict-fixer instructions
Understand PR intent + map main's refactor
Resolve all 3 conflicts via the injection seam
Wire injections in hutch + blog composition roots
Adapt shared tests to the injection contract
pnpm check — all 40 projects pass (hutch: 2957 unit + 11 integration + 9 E2E; functions coverage 100%)
Force-push with --force-with-lease
• fix/analytics-data-hygiene

FagnerMartinsBrack · 2026-07-09T16:20:10Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Commit: d145c45

claude · 2026-07-09T16:20:46Z

FagnerMartinsBrack · 2026-07-09T16:27:48Z

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. `shouldLog`'s `statusCode >= 300` cutoff also drops `304 Not Modified`, which is a cached page render, not a redirect leg

web-analytics/src/analytics.ts:239 — if (params.statusCode < 200 || params.statusCode >= 300) return false; correctly kills the 301/302/303 redirect legs that were double-counting (the core of fix (b)), and the tests pin 301/302/199. But the same cutoff silently drops 304 Not Modified, which — unlike a redirect — represents a real page the human re-viewed from cache.

I traced the blast radius and it's genuinely small, which is why this is low-priority:

Every explicit 304 emitter is CacheableComponent (conditional-get.ts:35), used only on HTMX poll fragments — queue cards (queue.page.ts:1139,1165) and the reader/summary polls (view.page.ts:327,348). All carry hx-request: true and are already dropped by shouldLog's hx-request guard, so the 3xx change has zero incremental effect there.
The only residual is Express's default weak ETag on full-page res.send() (no app.disable('etag') exists), which could 304 a full-page navigation on reload/back-forward revalidation. That was counted before this PR and is now uncounted — a modest, low-rate undercount.

This is arguably acceptable for the dashboard's "human page renders" intent, but it's an untested behavior the PR's "stop counting 3xx" framing glosses over. If you want to preserve cached full-page renders, exclude only redirect statuses (e.g. treat >= 300 && !== 304 as the drop, or gate on a Location header) and add a 304 test case alongside the existing 301/302/199 ones. Fix this →

2. Apple-touch-icon regex is duplicated between the route and the classifier (carried over from prior review)

static-asset-paths.ts:3 and server.ts:503 both hold the identical literal /^\/apple-touch-icon(?:-\d+x\d+)?(?:-precomposed)?\.png$/ — one as the redirect route, one as the analytics/click classifier. They must stay byte-for-byte in sync or the classifier silently drifts. This PR already established the single-source pattern with CLIENT_DIST_MOUNT_PATH; exporting the apple-touch regex from static-asset-paths.ts and importing it into server.ts would close the drift window the same way. Still low severity (the redirect is a 301, now dropped by the new 3xx rule; the classifier branch is defensive). Unchanged at this commit. Fix this →

3. `TRIMMED_BUNDLE_PATH` charset couples to the current bundle-naming convention (carried over from prior review)

static-asset-paths.ts:10 — /^\/[a-z0-9-]+\.client\.js(\.map)?$/ matches only lowercase-hyphen names. All current *.client.ts bundles are lowercase-hyphen so every existing bundle is classified today; the concern is purely forward-looking — a future myWidget.client.js/my_widget.client.js mount-trimmed shape would fall back to being counted as a pageview. The primary guard (/client-dist/ prefix + entry-time req.path snapshot) still covers the normal path, so anchoring on the .client.js suffix with a broader [^/]+ (no hostname carries that suffix) would make the fallback naming-agnostic. Unchanged at this commit. Fix this →

FagnerMartinsBrack · 2026-07-10T07:35:27Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Branch: fix/analytics-data-hygiene

claude · 2026-07-10T07:36:18Z

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

Conflicts resolved ✅

Rebased fix/analytics-data-hygiene onto origin/main (d145c453 → e6e2922b) and force-pushed with --force-with-lease. pnpm check passes across all 40 projects.

1 conflict — src/packages/web-analytics/src/analytics.ts. Main's cff18fab feat(analytics): slice the device-mix pie by device class / browser family hoisted a const userAgent = req.get("user-agent") local (feeding new device_class/browser pageview fields) on the line immediately after the same shouldLog(...) call this PR extended with isStaticAssetPath. The two edits are orthogonal, so I kept both:

if (!shouldLog({ req, path, statusCode: res.statusCode, isStaticAssetPath: deps.isStaticAssetPath })) return;
const userAgent = req.get("user-agent");

Everything else auto-merged cleanly, and I verified the merged results are internally coherent rather than just marker-free:

shouldLog retains this PR's signature — the injected isStaticAssetPath guard and the 3xx-drop (statusCode < 200 || >= 300) — unchanged by main.
The isStaticAssetPath dep is a required field on createAnalyticsMiddleware; main's new device-mix tests route through this PR's captureEvents helper, which already threads the dep (defaulting to () => false), so main's classifyDeviceClass/classifyBrowser cases and this PR's asset/path-hygiene cases coexist.
Diff vs origin/main is still the full +466 / −34 across 26 files — no PR content lost to the rebase.

The cancellation (c) and Pulumi owner-hash (d) fixes had no conflicts. No behavior change — the four measurement fixes land intact on top of main's device-mix analytics work.

Verification (pnpm check, 40 projects / 55 tasks): hutch 2990 unit + 11 integration + 9 E2E passing; coverage thresholds met (functions 100%, statements 99.47%, branches 97.06%).

Read conflict-fixer instructions
Understand PR intent (commit history origin/main..HEAD)
Rebase onto origin/main and resolve the analytics.ts conflict
Verify auto-merged files are coherent (shouldLog signature, required dep, test helper)
pnpm check — all 40 projects pass
Force-push with --force-with-lease
• fix/analytics-data-hygiene

…ancellation mislabel, owner exclusion Four independently revertable measurement fixes: (a) Static-asset fetches were counted as pageviews. New isStaticAssetRequestPath, derived from the actual static routes in server.ts (the /client-dist mount and the apple-touch-icon redirect), excludes them in analytics shouldLog and in click-attribution's landing capture. (b) Path fidelity: snapshot req.path at analytics-middleware entry so a finish-time express mount-trim can no longer corrupt the logged path; stop counting 3xx responses as pageviews (a redirect leg is not a pageview); and canonicalize /view scheme-variant landing_path at cookie time via canonicalizeViewLandingPath so the same article stops splitting attribution. (c) Cancellation reason: thread a trigger through CancelSubscriptionCommand → deferred-cancellation schedule → final conversion so a trial that lapsed for no card on file emits trial_ended_no_payment_method instead of user_initiated_trial. The churned-trial feedback-email filter accepts both trial reasons so the churn email keeps firing. (d) Owner exclusion: 22 production-confirmed owner device hashes appended to hutch:excludedVisitorHashes (dashboard-query-side only). Claude-Session: https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9

FagnerMartinsBrack · 2026-07-10T07:54:40Z

@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: fix(analytics): data hygiene — asset pageviews, reader-path splits, cancellation mislabel, owner exclusion #934
Commit: e6e2922

claude · 2026-07-10T07:55:27Z

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

Claude PR Review — complete ✅

Read review instructions (.github/workflows/claude-PR-code-reviewer.md)
Pull full diff vs origin/main (rebased single commit e6e2922b)
Review (a) asset-pageview exclusion
Review (b) path fidelity / 3xx / landing canonicalization
Review (c) cancellation-reason plumbing + feedback-email filter
Review (d) Pulumi owner-hash exclusion
Re-verify the @packages/web-analytics injection seam + device-mix rebase merge
Confirm CI green on this commit (run 29077698705 ✅)
Emit final review block

Verdict: 0 high / 0 medium. Re-reviewed at the rebased e6e2922b (CI ✅). The one rebase conflict (main's device-mix feat hoisting const userAgent = req.get("user-agent") next to the shouldLog call this PR extended) merged coherently: userAgent is computed after shouldLog's early-return and feeds classifyDeviceClass/classifyBrowser (same-file fns), while shouldLog keeps this PR's injected isStaticAssetPath guard and the 3xx-drop. All four fixes land intact:

(a) isStaticAssetRequestPath injected into both shared middlewares; anchored on the robust /client-dist/ prefix + entry-time req.path snapshot. Reader paths ending .png stay pageviews (tested). Blog injects () => false / identity at its own composition root (not a package default) — consistent with the no-default-impl rules.
(b) Snapshot-at-entry + 3xx-drop + landing_path canonicalization are mutually reinforcing: the /view/https:/… 301 leg is dropped, the 200 destination logs the snapshot path, and the cookie canonicalizes pre-redirect — so both the pageview split and the attribution split collapse. 303 POST-clicks still count (shouldCountClick unchanged, tested).
(c) Traced the charge_failed trigger end-to-end: charge-failed handler → CancelSubscriptionCommand → trialing branch → deferred Scheduler Input → pending_cancellation → trial_ended_no_payment_method; the paid discriminator (subscriptionId) wins; optional field keeps in-flight payloads backward-compatible; feedback-email filter accepts both trial reasons. Every leg tested.
(d) excludedVisitorHashes = 26 entries, all unique (verified), all lowercase-hex-16 (infra assertion at index.ts:1501), query-side only.

Three low-priority notes (all carried over from prior reviews, unchanged at this commit — 304-cutoff, apple-touch regex duplication, TRIMMED_BUNDLE_PATH charset) are in the review block. Full review is emitted as my final run message; the auto-apply workflow posts it as the review comment (per claude-PR-code-reviewer.md), so I'm not duplicating the CLAUDE_REVIEW block here.
• fix/analytics-data-hygiene

FagnerMartinsBrack · 2026-07-10T08:00:19Z

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. `shouldLog`'s `>= 300` cutoff also drops `304 Not Modified`, a cached page render (carried over)

web-analytics/src/analytics.ts:239 — if (params.statusCode < 200 || params.statusCode >= 300) return false; correctly kills the 301/302/303 redirect legs (the core of fix (b), pinned by tests), but the same cutoff also drops 304 Not Modified, which — unlike a redirect — is a real page the human re-viewed from cache. Blast radius is small: every explicit 304 emitter is CacheableComponent (HTMX poll fragments) already dropped by the hx-request guard, so the incremental effect is only Express's default weak-ETag 304 on a full-page res.send() reload/back-forward revalidation — a modest undercount that was counted before this PR. Arguably acceptable for the "human page renders" intent, but untested; if you want to preserve cached full-page renders, treat >= 300 && !== 304 as the drop (or gate on a Location header) and add a 304 case next to the 301/302/199 ones. Fix this →

2. Apple-touch-icon regex is duplicated between the route and the classifier (carried over)

static-asset-paths.ts:3 and server.ts:503 both hold the identical literal /^\/apple-touch-icon(?:-\d+x\d+)?(?:-precomposed)?\.png$/ — one as the redirect route, one as the analytics/click classifier. I confirmed they're still byte-for-byte identical at this commit. They must stay in sync or the classifier silently drifts and those requests get counted again. This PR already established the single-source pattern with CLIENT_DIST_MOUNT_PATH; exporting the apple-touch regex from static-asset-paths.ts and importing it into server.ts would close the drift window the same way. Still low severity (the redirect is a 301, now dropped by the new 3xx rule; the classifier branch is defensive). Fix this →

3. `TRIMMED_BUNDLE_PATH` charset couples to the current bundle-naming convention (carried over)

static-asset-paths.ts:10 — /^\/[a-z0-9-]+\.client\.js(\.map)?$/ matches only lowercase-hyphen names. All current *.client.ts bundles are lowercase-hyphen, so every existing bundle is classified today; the concern is purely forward-looking — a future myWidget.client.js/my_widget.client.js mount-trimmed shape would fall back to being counted as a pageview. The primary guard (/client-dist/ prefix + entry-time req.path snapshot) still covers the normal path, so anchoring on the .client.js suffix with a broader [^/]+ (no hostname carries that suffix) would make the fallback naming-agnostic. Fix this →

(The prior-review note that canonicalizeViewLandingPath doesn't re-run encodeArticlePathInfo remains accurate and by-design: the first-touch cookie is written once, so scheme-variant-first and canonical-first requests still collapse to one value — no attribution split. No action needed.)

FagnerMartinsBrack temporarily deployed to staging July 5, 2026 13:55 — with GitHub Actions Inactive

FagnerMartinsBrack force-pushed the fix/analytics-data-hygiene branch from 272d323 to efa6d1d Compare July 9, 2026 08:53

FagnerMartinsBrack temporarily deployed to staging July 9, 2026 08:53 — with GitHub Actions Inactive

FagnerMartinsBrack force-pushed the fix/analytics-data-hygiene branch from efa6d1d to d145c45 Compare July 9, 2026 16:12

FagnerMartinsBrack temporarily deployed to staging July 9, 2026 16:12 — with GitHub Actions Inactive

FagnerMartinsBrack force-pushed the fix/analytics-data-hygiene branch from d145c45 to e6e2922 Compare July 10, 2026 07:46

FagnerMartinsBrack temporarily deployed to staging July 10, 2026 07:46 — with GitHub Actions Inactive

FagnerMartinsBrack mentioned this pull request Jul 10, 2026

feat(inbox): extract the inbox into its own deployable (phases A–C: pre-refactors, seam, additive project) #950

Merged

2 tasks

Uh oh!

Conversation

FagnerMartinsBrack commented Jul 5, 2026