From e820e7fecdace3fd9573b89be789f03978a97635 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 09:22:37 +0100 Subject: [PATCH] fix(analytics): the funnel source read a referrer that never changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The staging E2E gate has been red since #132 merged (run 1064, and 1066 after it): "a school reached from a location page is attributed to it, not to direct" expects `place`, receives `direct`. #132 fixed a real bug — `/schools/` had no case and fell through to `direct` — but the mechanism underneath it never worked. getNavigationSource read document.referrer, which the browser writes only when a *document* loads. Every internal navigation here is an App Router soft navigation: history.pushState, no new document, so document.referrer goes on naming whatever opened the tab for the whole session. Verified on staging: load /schools/brentwood, click a school, the URL becomes /school/… and document.referrer is still "". So `from` reported `direct` for essentially every in-app journey, not just the ones through the location layer — search, rankings, compare and detail were all being counted as "typed the URL". The unit suite passed throughout because every case set document.referrer directly, which only happens on a full page load. The fix is a module-level trail written by RouteTrail, a render-nothing client component in the root layout. Its lifetime is exactly right: it survives soft navigation, and it dies on a real document load — which is precisely when document.referrer becomes meaningful again, so the two cover each other with no overlap. Reading it skips entries equal to the current path rather than taking the second-to-last. That makes the answer independent of whether the layout effect or the page effect ran first — React orders those by tree position, which is not a contract worth resting a measurement on — and it gives the right answer both when the user returns to a page they came from and on a hard load of a school page. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM --- .../__tests__/components/RouteTrail.test.tsx | 38 ++++++++ nextjs-app/__tests__/lib/analytics.test.ts | 94 +++++++++++++++++++ nextjs-app/app/layout.tsx | 5 + nextjs-app/components/RouteTrail.tsx | 27 ++++++ nextjs-app/lib/analytics.ts | 77 ++++++++++++--- 5 files changed, 230 insertions(+), 11 deletions(-) create mode 100644 nextjs-app/__tests__/components/RouteTrail.test.tsx create mode 100644 nextjs-app/components/RouteTrail.tsx diff --git a/nextjs-app/__tests__/components/RouteTrail.test.tsx b/nextjs-app/__tests__/components/RouteTrail.test.tsx new file mode 100644 index 0000000..cc95c9e --- /dev/null +++ b/nextjs-app/__tests__/components/RouteTrail.test.tsx @@ -0,0 +1,38 @@ +/** + * The trail has to be written by something, and it has to be written on every + * route — not only the ones that happen to track an event. + */ +import { render } from '@testing-library/react'; + +const recordVisitedPath = jest.fn(); +let pathname = '/schools/brentwood'; + +jest.mock('next/navigation', () => ({ usePathname: () => pathname })); +jest.mock('@/lib/analytics', () => ({ + recordVisitedPath: (p: string) => recordVisitedPath(p), +})); + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { RouteTrail } = require('@/components/RouteTrail'); + +describe('RouteTrail', () => { + beforeEach(() => recordVisitedPath.mockClear()); + + it('records the page it is mounted on', () => { + render(); + expect(recordVisitedPath).toHaveBeenCalledWith('/schools/brentwood'); + }); + + it('records each new route as the user moves through the app', () => { + const { rerender } = render(); + pathname = '/school/115429-brentwood-school'; + rerender(); + expect(recordVisitedPath).toHaveBeenLastCalledWith( + '/school/115429-brentwood-school'); + }); + + it('renders nothing, so it can sit anywhere in the layout', () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); +}); diff --git a/nextjs-app/__tests__/lib/analytics.test.ts b/nextjs-app/__tests__/lib/analytics.test.ts index 22af2d7..96dd0fb 100644 --- a/nextjs-app/__tests__/lib/analytics.test.ts +++ b/nextjs-app/__tests__/lib/analytics.test.ts @@ -62,3 +62,97 @@ describe('getNavigationSource', () => { expect(getNavigationSource()).toBe('direct'); }); }); + +/* + * The defect the existing suite could not see. + * + * Every test above sets document.referrer, which the browser writes only when + * a *document* loads. Every internal navigation in this app is an App Router + * soft navigation — history.pushState, no new document — so document.referrer + * keeps naming whatever opened the tab for the whole session. Verified on + * staging: /schools/brentwood → click a school → URL changes to /school/… + * and document.referrer is still "". + * + * So `from` reported 'direct' for essentially every in-app journey, and the + * suite passed because it only ever exercised the full-page-load path. + */ +function freshAnalytics() { + let mod!: typeof import('@/lib/analytics'); + jest.isolateModules(() => { + mod = require('@/lib/analytics'); + }); + return mod; +} + +function at(path: string) { + window.history.pushState({}, '', path); +} + +describe('getNavigationSource across a soft navigation', () => { + afterEach(() => { + referrer(''); + at('/'); + }); + + it('attributes a school view to the place page the user actually came from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('place'); + }); + + it('does not depend on whether the new path was recorded first', () => { + // The trail is written by a layout-level effect and read by a page-level + // one. React orders those by tree position, which is not a contract worth + // resting a measurement on, so the answer must be the same either way. + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + recordVisitedPath('/rankings'); + at('/school/115429-brentwood-school'); + + expect(source()).toBe('rankings'); + }); + + it('names the previous page, not the current one, when both are schools', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/school/100010-brecknock-primary-school'); + recordVisitedPath('/school/100010-brecknock-primary-school'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('detail'); + }); + + it('looks past a return visit to the page the user came back from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + for (const p of ['/schools/brentwood', '/school/115429-brentwood-school', + '/schools/brentwood']) { + at(p); + recordVisitedPath(p); + } + expect(source()).toBe('detail'); + }); + + it('falls back to the referrer on a real document load, where it is true', () => { + // A fresh module is a fresh document: nothing has been recorded, and + // document.referrer is meaningful again. + const { getNavigationSource: source } = freshAnalytics(); + at('/school/115429-brentwood-school'); + referrer(`${ORIGIN}/schools/barnet`); + + expect(source()).toBe('place'); + }); + + it('still reads an arrival from outside as direct', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + referrer('https://www.google.com/search?q=schools+in+brentwood'); + + expect(source()).toBe('direct'); + }); +}); diff --git a/nextjs-app/app/layout.tsx b/nextjs-app/app/layout.tsx index 2f888b9..96a2be4 100644 --- a/nextjs-app/app/layout.tsx +++ b/nextjs-app/app/layout.tsx @@ -4,6 +4,7 @@ import Script from 'next/script'; import { Navigation } from '@/components/Navigation'; import { Footer } from '@/components/Footer'; import { ComparisonToast } from '@/components/ComparisonToast'; +import { RouteTrail } from '@/components/RouteTrail'; import { ComparisonProvider } from '@/context/ComparisonProvider'; import { SITE_URL } from '@/lib/site'; import './globals.css'; @@ -114,6 +115,10 @@ export default function RootLayout({ /> + {/* Records every route so funnel attribution has a previous page to + name. document.referrer cannot: a soft navigation creates no + document, so the browser never updates it. */} + Skip to main content diff --git a/nextjs-app/components/RouteTrail.tsx b/nextjs-app/components/RouteTrail.tsx new file mode 100644 index 0000000..4531693 --- /dev/null +++ b/nextjs-app/components/RouteTrail.tsx @@ -0,0 +1,27 @@ +/** + * Writes the in-app navigation trail that funnel attribution reads. + * + * Renders nothing. It exists because document.referrer cannot answer "which + * page did they come from" in an App Router app: a soft navigation creates no + * document, so the browser never updates it. See the trail comment in + * lib/analytics.ts. + * + * Mounted once in the root layout, so every route is recorded — including the + * ones that fire no event of their own, which are still somebody else's + * previous page. + */ +'use client'; + +import { useEffect } from 'react'; +import { usePathname } from 'next/navigation'; +import { recordVisitedPath } from '@/lib/analytics'; + +export function RouteTrail() { + const pathname = usePathname(); + + useEffect(() => { + recordVisitedPath(pathname); + }, [pathname]); + + return null; +} diff --git a/nextjs-app/lib/analytics.ts b/nextjs-app/lib/analytics.ts index bd05ba1..591ed7d 100644 --- a/nextjs-app/lib/analytics.ts +++ b/nextjs-app/lib/analytics.ts @@ -60,22 +60,77 @@ export function track(name: EventName, data?: Payload): void { export type NavigationSource = 'search' | 'rankings' | 'compare' | 'detail' | 'place' | 'direct'; +/* + * The in-app trail. + * + * document.referrer is written by the browser only when a *document* loads. + * Every internal navigation here is an App Router soft navigation — + * history.pushState, no new document — so document.referrer goes on naming + * whatever opened the tab (usually nothing, or a search engine) for the whole + * session. Reading it to answer "which page did they come from" therefore + * returned 'direct' for essentially every in-app journey, including the one + * the location layer exists to produce. + * + * Verified on staging: /schools/brentwood, click a school, the URL becomes + * /school/… and document.referrer is still "". + * + * A module-level trail is the counterpart with exactly the right lifetime. It + * survives soft navigation, and it dies on a real document load — which is + * precisely when document.referrer becomes meaningful again, so the two cover + * each other with no overlap. + */ +const TRAIL_LIMIT = 4; +const trail: string[] = []; + +/** Record a path the user is now on. Called by RouteTrail on every route. */ +export function recordVisitedPath(path: string): void { + if (trail[trail.length - 1] === path) return; + trail.push(path); + if (trail.length > TRAIL_LIMIT) trail.shift(); +} + +/** + * The most recent path that is not the one being viewed. + * + * Skipping the current path rather than taking trail[length - 2] is what + * makes the answer independent of ordering: the trail is written by a + * layout-level effect and read by a page-level one, and React orders those by + * tree position — not a contract worth resting a measurement on. It also + * gives the right answer when the user goes back to a page they came from. + */ +function previousInAppPath(): string | null { + if (typeof window === 'undefined') return null; + const current = window.location.pathname; + for (let i = trail.length - 1; i >= 0; i -= 1) { + if (trail[i] !== current) return trail[i]; + } + return null; +} + +function classifyPath(p: string): NavigationSource { + if (p === '/' || p === '') return 'search'; + if (p.startsWith('/rankings')) return 'rankings'; + if (p.startsWith('/compare')) return 'compare'; + // `/schools/` before `/school/`: they differ by one letter and mean + // different things — the location layer versus a single school. Checked + // first so the narrower-looking prefix cannot shadow it if either string + // is ever edited. + if (p.startsWith('/schools/')) return 'place'; + if (p.startsWith('/school/')) return 'detail'; + return 'direct'; +} + export function getNavigationSource(): NavigationSource { + const internal = previousInAppPath(); + if (internal) return classifyPath(internal); + + // No trail means this is the first page of the document, so the referrer is + // the only witness — and an honest one. if (typeof window === 'undefined' || !document.referrer) return 'direct'; try { const ref = new URL(document.referrer); if (ref.origin !== window.location.origin) return 'direct'; - const p = ref.pathname; - if (p === '/' || p === '') return 'search'; - if (p.startsWith('/rankings')) return 'rankings'; - if (p.startsWith('/compare')) return 'compare'; - // `/schools/` before `/school/`: they differ by one letter and mean - // different things — the location layer versus a single school. Checked - // first so the narrower-looking prefix cannot shadow it if either string - // is ever edited. - if (p.startsWith('/schools/')) return 'place'; - if (p.startsWith('/school/')) return 'detail'; - return 'direct'; + return classifyPath(ref.pathname); } catch { return 'direct'; }