Просмотр исходного кода

Keep people signed in while they use the app, and make Try again on the error page reload (#495)

A page render read the session through better-auth, which extends a day-old session by
writing a new expiry and a new cookie. A render cannot set cookies, so the expiry was saved
and the cookie lost. Every later read found the session already extended, and the cookie
still ran out a week after sign-in: someone using the app every day was signed out once a
week. getCachedSession now reads without extending, and the sign-in, sign-up, onboarding
and account pages use it instead of calling better-auth directly. The open tab's ping and
the browser's session lookup do the extending, and both can set the cookie. A test fails
if a page or layout calls better-auth's getSession directly again.

The error page's Try again called reset(), which drew the page again from what the tab
already had, so an error from the server or from the tab's own state came straight back
and people pressed it over and over. It now reloads the page, which also brings a tab left
open across a deploy onto the new build. Both error pages send the error to PostHog with
reportError, held like any event until PostHog has started: an error page is React
catching the error, so PostHog's own exception capture never sees it.
Bernt Christian Egeland 1 день назад
Родитель
Сommit
486286dcae

+ 57 - 0
src/__tests__/app/error-page.test.tsx

@@ -0,0 +1,57 @@
+import { fireEvent, render, screen } from '@testing-library/react'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+/**
+ * The error page's Try again reloads the page, and the error goes to PostHog.
+ *
+ * It used to call reset(), which drew the page again from what the tab
+ * already had: an error from the server or from the tab's own state came
+ * straight back, and a person pressed the button over and over to no effect.
+ */
+
+const reportError = vi.fn()
+vi.mock('@/lib/analytics', () => ({
+  reportError: (...args: unknown[]) => reportError(...args),
+}))
+
+import ErrorPage from '@/app/error'
+
+const originalLocation = window.location
+const reload = vi.fn()
+
+beforeEach(() => {
+  reportError.mockReset()
+  reload.mockReset()
+  vi.spyOn(console, 'error').mockImplementation(() => undefined)
+  Object.defineProperty(window, 'location', {
+    configurable: true,
+    value: { ...originalLocation, reload },
+  })
+})
+
+afterEach(() => {
+  vi.restoreAllMocks()
+  Object.defineProperty(window, 'location', { configurable: true, value: originalLocation })
+})
+
+describe('error page', () => {
+  it('reloads the whole page on Try again', () => {
+    render(<ErrorPage error={new Error('boom')} />)
+    fireEvent.click(screen.getByRole('button', { name: /try again/i }))
+    expect(reload).toHaveBeenCalledTimes(1)
+  })
+
+  it('reports the error with the digest that finds it in the server log', () => {
+    const error = Object.assign(new Error('An error occurred in the Server Components render.'), {
+      digest: '2338407428',
+    })
+    render(<ErrorPage error={error} />)
+    expect(reportError).toHaveBeenCalledWith(error, { error_page: 'page', digest: '2338407428' })
+  })
+
+  it('reports an error from the browser without a digest', () => {
+    const error = new Error("Cannot read properties of undefined (reading 'map')")
+    render(<ErrorPage error={error} />)
+    expect(reportError).toHaveBeenCalledWith(error, { error_page: 'page', digest: null })
+  })
+})

+ 20 - 0
src/__tests__/lib/analytics.test.ts

@@ -8,6 +8,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const posthog = vi.hoisted(() => ({
   capture: vi.fn(),
+  captureException: vi.fn(),
   register: vi.fn(),
   identify: vi.fn(),
   group: vi.fn(),
@@ -74,6 +75,25 @@ describe('analytics', () => {
     expect(posthog.capture).toHaveBeenCalledTimes(20)
   })
 
+  it('holds an error a page reports before PostHog starts, then sends it with its details', async () => {
+    const { reportError, analyticsStarted } = await freshAnalytics()
+    const error = new Error('Cannot read properties of undefined')
+    reportError(error, { error_page: 'page', digest: null })
+    expect(posthog.captureException).not.toHaveBeenCalled()
+
+    analyticsStarted()
+    expect(posthog.captureException).toHaveBeenCalledWith(error, {
+      error_page: 'page',
+      digest: null,
+    })
+
+    reportError(error, { error_page: 'root', digest: '123' })
+    expect(posthog.captureException).toHaveBeenLastCalledWith(error, {
+      error_page: 'root',
+      digest: '123',
+    })
+  })
+
   it('ties the browser to the account and workshop, held like any event', async () => {
     const { identifyUser, analyticsStarted } = await freshAnalytics()
     identifyUser('user-1', 'org-1')

+ 61 - 0
src/__tests__/lib/cached-session.test.ts

@@ -0,0 +1,61 @@
+import fs from 'node:fs'
+import path from 'node:path'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+/**
+ * A page render reads the session without extending it.
+ *
+ * better-auth extends a day-old session on the next read: a new expiry in the
+ * database and a new cookie. A render cannot set cookies, so a read there
+ * saved the expiry and lost the cookie, and the cookie ran out a week after
+ * sign-in however often the app was used. Extending is left to the open tab's
+ * ping and the browser's session lookup, which can set it.
+ */
+
+const getSession = vi.hoisted(() => vi.fn())
+vi.mock('@/lib/auth', () => ({ auth: { api: { getSession } } }))
+vi.mock('@/lib/db', () => ({ db: {} }))
+vi.mock('next/headers', () => ({
+  headers: async () => new Headers({ cookie: 'session=abc' }),
+  cookies: async () => ({ get: () => undefined }),
+}))
+
+import { getCachedSession } from '@/lib/cached-session'
+
+beforeEach(() => {
+  getSession.mockReset()
+})
+
+describe('getCachedSession', () => {
+  it('reads the session without extending it', async () => {
+    getSession.mockResolvedValue(null)
+    await getCachedSession()
+
+    expect(getSession).toHaveBeenCalledTimes(1)
+    expect(getSession.mock.calls[0][0]).toMatchObject({ query: { disableRefresh: true } })
+    expect(getSession.mock.calls[0][0].headers.get('cookie')).toBe('session=abc')
+  })
+})
+
+const API_ROUTES = path.join('src', 'app', 'api')
+
+/** Everything under src/app that renders on the server: all but the API routes. */
+function renderedFiles(dir: string): string[] {
+  return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => {
+    const full = path.join(dir, entry.name)
+    if (entry.isDirectory()) return full === API_ROUTES ? [] : renderedFiles(full)
+    return /\.tsx?$/.test(entry.name) ? [full] : []
+  })
+}
+
+describe('pages and layouts', () => {
+  it('read the session through getCachedSession, never better-auth directly', () => {
+    const offenders = renderedFiles(path.join('src', 'app')).filter((file) => {
+      const source = fs.readFileSync(file, 'utf8')
+      // A server action may set cookies, so it may extend the session
+      if (/^\s*['"]use server['"]/.test(source)) return false
+      return /auth\.api\.getSession\(/.test(source)
+    })
+    expect(offenders).toEqual([])
+  })
+})

+ 2 - 3
src/app/(authenticated)/settings/account/page.tsx

@@ -1,5 +1,4 @@
-import { headers } from 'next/headers'
-import { auth } from '@/lib/auth'
+import { getCachedSession } from '@/lib/cached-session'
 import { db } from '@/lib/db'
 import { redirect } from 'next/navigation'
 import { AccountSettings } from './account-settings'
@@ -7,7 +6,7 @@ import { listMyDevices } from '@/features/settings/Actions/sessionActions'
 import { getMySignature } from '@/features/signatures/Actions/signatureActions'
 
 export default async function AccountSettingsPage() {
-  const session = await auth.api.getSession({ headers: await headers() })
+  const session = await getCachedSession()
   if (!session) redirect('/auth/sign-in')
 
   const [user, verificationSetting, devices, signature] = await Promise.all([

+ 2 - 3
src/app/(public)/auth/sign-in/page.tsx

@@ -1,8 +1,7 @@
 import { redirect } from 'next/navigation'
-import { headers } from 'next/headers'
 import { safeRedirectPath } from '@/lib/safe-redirect'
 import { db } from '@/lib/db'
-import { auth } from '@/lib/auth'
+import { getCachedSession } from '@/lib/cached-session'
 import { SignInForm } from './sign-in-form'
 import { isDemoMode } from '@/lib/demo'
 import { isCloudMode } from '@/lib/features'
@@ -19,7 +18,7 @@ export default async function SignInPage({
   const redirectTo = params.redirect ? safeRedirectPath(params.redirect) : undefined
 
   const [session, regSetting] = await Promise.all([
-    auth.api.getSession({ headers: await headers() }),
+    getCachedSession(),
     isDemoMode
       ? null
       : db.systemSetting.findUnique({

+ 2 - 3
src/app/(public)/auth/sign-up/page.tsx

@@ -1,8 +1,7 @@
 import { redirect } from 'next/navigation'
 import { safeRedirectPath } from '@/lib/safe-redirect'
-import { headers } from 'next/headers'
 import { db } from '@/lib/db'
-import { auth } from '@/lib/auth'
+import { getCachedSession } from '@/lib/cached-session'
 import { SignUpForm } from './sign-up-form'
 import { isDemoMode } from '@/lib/demo'
 import { isCloudMode } from '@/lib/features'
@@ -24,7 +23,7 @@ export default async function SignUpPage({
   // The three lookups do not depend on each other. This is the first page a
   // new visitor sees, so it should not pay for three round-trips in a row.
   const [session, regSetting, verificationSetting] = await Promise.all([
-    auth.api.getSession({ headers: await headers() }),
+    getCachedSession(),
     inviteToken || isDemoMode
       ? null
       : db.systemSetting.findUnique({

+ 2 - 3
src/app/(public)/onboarding/page.tsx

@@ -1,7 +1,6 @@
-import { headers } from 'next/headers'
 import { redirect } from 'next/navigation'
 import { safeRedirectPath } from '@/lib/safe-redirect'
-import { auth } from '@/lib/auth'
+import { getCachedSession } from '@/lib/cached-session'
 import { db } from '@/lib/db'
 import { SetupWizard } from '@/features/onboarding/Components/setup/SetupWizard'
 import { SingleWorkshopNotice } from '@/features/onboarding/Components/SingleWorkshopNotice'
@@ -14,7 +13,7 @@ export default async function OnboardingPage({
 }) {
   const params = await searchParams
   const redirectTo = params.redirect ? safeRedirectPath(params.redirect) : undefined
-  const session = await auth.api.getSession({ headers: await headers() })
+  const session = await getCachedSession()
 
   if (!session?.user?.id) {
     redirect('/auth/sign-in')

+ 12 - 8
src/app/error.tsx

@@ -4,19 +4,16 @@ import { useEffect } from 'react'
 import { useTranslations } from 'next-intl'
 import { Wrench } from 'lucide-react'
 import { Button } from '@/components/ui/button'
+import { reportError } from '@/lib/analytics'
 
 // biome-ignore lint/suspicious/noShadowRestrictedNames: Next.js error boundary convention
-export default function Error({
-  error,
-  reset,
-}: {
-  error: Error & { digest?: string }
-  reset: () => void
-}) {
+export default function Error({ error }: { error: Error & { digest?: string } }) {
   const t = useTranslations('common')
 
   useEffect(() => {
     console.error(error)
+    // The digest finds the error in the server log when the server threw it
+    reportError(error, { error_page: 'page', digest: error.digest ?? null })
   }, [error])
 
   return (
@@ -36,7 +33,14 @@ export default function Error({
         <p className="mt-2 text-sm text-muted-foreground">{t('errorPage.description')}</p>
 
         <div className="mt-8 flex flex-col gap-3 sm:flex-row sm:justify-center">
-          <Button onClick={reset}>{t('errorPage.tryAgain')}</Button>
+          {/*
+            A full reload, not reset(). reset() only drew the page again from
+            what the tab already had, so an error from the server or from the
+            tab's own state came straight back and the button did nothing:
+            people pressed it over and over. A tab left open for days may also
+            be on an old build, and only a reload brings the new one.
+          */}
+          <Button onClick={() => window.location.reload()}>{t('errorPage.tryAgain')}</Button>
           <Button variant="outline" asChild>
             <a href="/">{t('errorPage.backToDashboard')}</a>
           </Button>

+ 7 - 8
src/app/global-error.tsx

@@ -1,16 +1,13 @@
 'use client'
 
 import { useEffect } from 'react'
+import { reportError } from '@/lib/analytics'
 
-export default function GlobalError({
-  error,
-  reset,
-}: {
-  error: Error & { digest?: string }
-  reset: () => void
-}) {
+export default function GlobalError({ error }: { error: Error & { digest?: string } }) {
   useEffect(() => {
     console.error(error)
+    // Sent only if PostHog started before the root layout broke
+    reportError(error, { error_page: 'root', digest: error.digest ?? null })
   }, [error])
 
   return (
@@ -69,7 +66,9 @@ export default function GlobalError({
             The application encountered a fatal error. Please try refreshing the page.
           </p>
           <button
-            onClick={reset}
+            // A full reload: the root layout itself broke, and reset() would
+            // only draw it again from the same state
+            onClick={() => window.location.reload()}
             style={{
               padding: '10px 24px',
               fontSize: 14,

+ 15 - 0
src/lib/analytics.ts

@@ -61,6 +61,7 @@ type Properties = Record<string, string | number | boolean | null>
 
 type Pending =
   | { kind: 'capture'; event: AnalyticsEvent; properties?: Properties }
+  | { kind: 'exception'; error: unknown; properties?: Properties }
   | { kind: 'register'; properties: Properties }
   | { kind: 'unregister'; names: string[] }
   | { kind: 'identify'; userId: string; organizationId: string }
@@ -82,6 +83,19 @@ export function track(event: AnalyticsEvent, properties?: Properties): void {
   else hold({ kind: 'capture', event, properties })
 }
 
+/**
+ * Sends an error a person was shown, with its stack, or holds it until
+ * PostHog has started.
+ *
+ * For errors the app catches itself, the error pages above all. PostHog's own
+ * exception capture listens for uncaught errors, and an error page is React
+ * catching one, so without this nobody learns which page broke or why.
+ */
+export function reportError(error: unknown, properties?: Properties): void {
+  if (ready) posthog.captureException(error, properties)
+  else hold({ kind: 'exception', error, properties })
+}
+
 /**
  * Attaches properties to every later event from this browser, autocaptured
  * clicks and page views included (PostHog's super properties). For facts
@@ -132,6 +146,7 @@ export function analyticsStarted(): void {
     else if (item.kind === 'unregister') for (const name of item.names) posthog.unregister(name)
     else if (item.kind === 'identify') sendIdentify(item.userId, item.organizationId)
     else if (item.kind === 'reset') posthog.reset()
+    else if (item.kind === 'exception') posthog.captureException(item.error, item.properties)
     else posthog.capture(item.event, item.properties)
   }
 }

+ 13 - 1
src/lib/cached-session.ts

@@ -4,8 +4,20 @@ import { auth } from './auth'
 import { db } from './db'
 import { SCOPED_SESSION_MESSAGE, sessionOrganizationScope } from './session-scope'
 
+/**
+ * The session, read without extending it.
+ *
+ * Once a session is a day old, better-auth extends it on the next read: a new
+ * expiry in the database and a new cookie. A page render cannot set cookies,
+ * so a read here saved the new expiry and quietly lost the cookie. Every later
+ * read then found the session already extended, and the cookie still ran out
+ * a week after sign-in: someone using the app every day was signed out once a
+ * week. Extending is left to requests that can set the cookie, the open tab's
+ * ping (/api/protected/ping, on load and every minute) and the session lookup
+ * in the browser.
+ */
 export const getCachedSession = cache(async () => {
-  return auth.api.getSession({ headers: await headers() })
+  return auth.api.getSession({ headers: await headers(), query: { disableRefresh: true } })
 })
 
 export const getCachedMembership = cache(async (userId: string) => {