fix(demo): probe timeout + demo-aware session-expired routing - #926
fix(demo): probe timeout + demo-aware session-expired routing#926njrini99-code wants to merge 1 commit into
Conversation
Both demo gates (/golf/demo, /baseball/demo) could hang forever on "Checking sign-in status" — supabase.auth.getUser() has no timeout of its own, so a stale/expired httpOnly auth cookie could leave its internal refresh-token exchange stuck indefinitely. Separately, a demo visitor whose session idle-timed-out was bounced to the password /login page — a dead end, since they never had a password. - Add src/lib/demo/gate-probe.ts: probeSignedIn() races supabase.auth.getUser() against a hard 4s deadline and treats ANY failure (timeout, thrown error, or a refresh-token error surfaced via getUser()'s own `error` field) as signed out. Wired into both demo gate pages' "already signed in" probe and baseball's isBaseballDemoSession/isBaseballDemoAvailable checks. - enterDemo / enterBaseballDemo now stamp user_metadata.is_demo = true right after sign-in (best-effort, never blocks entry) — a non-secret signal both client and server code can read, unlike the demo account's email. - middleware.ts's idle-timeout redirect and useSessionActivity's client-side idle logout both now check user_metadata.is_demo (with an email-match fallback for sessions that pre-date this fix) and route a demo session back to the sport's own /demo gate with ?message=demo_session_expired instead of /login. Both gate pages render a friendly "your demo session timed out" notice for that case. Investigated the ~30min demo session death separately: live queries against the golf demo account (demo@golfhelmdemo.com) showed zero rows in auth.sessions / auth.refresh_tokens despite a sign-in ~40min prior, and a second account showed a NEW session created every ~15-30s with every session's updated_at == created_at (never refreshed in place) — consistent with either a Supabase Auth dashboard session-limit/timebox setting or refresh-token-rotation reuse-detection revoking the shared account's sessions, not a code-level bug in the demo entry flow (its cookie-writing is identical to the regular login action). That's a dashboard-level Supabase Auth setting outside this repo/PR's scope — flagging for Nick. The demo_session_expired routing above is the correct code-level mitigation regardless of the exact cause: whatever kills the session, the visitor now lands back on a working re-entry point instead of a dead end. Gates: npx tsc --noEmit -p tsconfig.json (clean); npx eslint on all changed files (clean); npx vitest run across all new/changed test files (296 tests passed, 0 failed). Fixes #918 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Summary by CodeRabbit
WalkthroughThe PR adds bounded demo authentication probes, stamps shared demo users with ChangesDemo session recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Visitor
participant SessionActivity
participant SupabaseAuth
participant DemoGate
Visitor->>SessionActivity: idle session expires
SessionActivity->>SupabaseAuth: probe current user
SupabaseAuth-->>SessionActivity: demo metadata
SessionActivity->>DemoGate: redirect with demo_session_expired
DemoGate-->>Visitor: show timeout notice and demo form
Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
src/app/baseball/(auth)/demo/page.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/app/baseball/actions/__tests__/demo-access.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/app/baseball/actions/demo-access.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🤖 Mission Control — PR summary What it changes: Fixes #918. Both demo gates ( Risk / areas: golf + baseball demo gates, auth/session handling, session-expired routing. Watch: the 4s deadline value; that CI: ✅ green so far — 34 checks passing, 4 pending, 0 failing; mergeable state BLOCKED on required review (no CI failure). Awaiting review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/baseball/actions/__tests__/demo-access.test.ts (1)
14-60: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftUse the repository Supabase test client instead of direct module mocks.
src/app/baseball/actions/__tests__/demo-access.test.ts#L14-L60: migrate the server-client mock.src/app/golf/actions/__tests__/demo-access.test.ts#L16-L58: migrate the server-client mock.src/lib/auth/__tests__/session-activity.test.ts#L29-L44: provide auth behavior through the test client.src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts#L16-L27: configure middleware auth through the shared fixture.As per path instructions, “Mock Supabase via the test client in src/test/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/baseball/actions/__tests__/demo-access.test.ts` around lines 14 - 60, Replace direct Supabase module mocks with the repository test client fixture from src/test/ across the affected tests. In src/app/baseball/actions/__tests__/demo-access.test.ts (lines 14-60) and src/app/golf/actions/__tests__/demo-access.test.ts (lines 16-58), migrate the server-client mock; in src/lib/auth/__tests__/session-activity.test.ts (lines 29-44), configure auth behavior through the test client; and in src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts (lines 16-27), configure middleware auth through the shared fixture. Preserve each test’s existing mocked behavior while routing Supabase interactions through the repository client.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/auth/session-activity.ts`:
- Around line 104-109: Update the idle-timeout sign-out call in the session
activity flow to pass the local scope option to supabase.auth.signOut, matching
the local sign-out behavior used by the middleware. Keep the existing
best-effort error handling and redirect flow unchanged.
In `@src/lib/demo/gate-probe.ts`:
- Around line 79-104: Persist demo routing context in a sport-specific
first-party cookie independent of ProbedUser, with helpers in gate-probe.ts for
setting and reading the non-authoritative marker. Update middleware.ts and
session-activity.ts to consult the marker when getUser or the bounded probe
yields no user, while retaining authentication as the authorization signal.
Update the specified session-activity test to expect the demo gate for a hung
demo probe, and add expired-session user:null coverage to
middleware-demo-session-expired.test.ts.
---
Outside diff comments:
In `@src/app/baseball/actions/__tests__/demo-access.test.ts`:
- Around line 14-60: Replace direct Supabase module mocks with the repository
test client fixture from src/test/ across the affected tests. In
src/app/baseball/actions/__tests__/demo-access.test.ts (lines 14-60) and
src/app/golf/actions/__tests__/demo-access.test.ts (lines 16-58), migrate the
server-client mock; in src/lib/auth/__tests__/session-activity.test.ts (lines
29-44), configure auth behavior through the test client; and in
src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts (lines
16-27), configure middleware auth through the shared fixture. Preserve each
test’s existing mocked behavior while routing Supabase interactions through the
repository client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 234bba71-ded6-49b5-809b-a2afa55152e2
📒 Files selected for processing (12)
src/app/baseball/(auth)/demo/page.tsxsrc/app/baseball/actions/__tests__/demo-access.test.tssrc/app/baseball/actions/demo-access.tssrc/app/golf/(auth)/demo/page.tsxsrc/app/golf/actions/__tests__/demo-access.test.tssrc/app/golf/actions/demo-access.tssrc/lib/auth/__tests__/session-activity.test.tssrc/lib/auth/session-activity.tssrc/lib/demo/__tests__/gate-probe.test.tssrc/lib/demo/gate-probe.tssrc/lib/supabase/__tests__/middleware-demo-session-expired.test.tssrc/lib/supabase/middleware.ts
| clearLastActivity(); | ||
| try { | ||
| await supabase.auth.signOut(); | ||
| } catch { | ||
| /* best-effort — still redirect to the login screen below */ | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## registry\n'
git ls-files memory/registry.yml memory/features | sed 's#^`#-` #'
printf '\n## session activity file outline\n'
ast-grep outline src/lib/auth/session-activity.ts --view expanded || true
printf '\n## relevant lines\n'
cat -n src/lib/auth/session-activity.ts | sed -n '1,220p'
printf '\n## registry contents\n'
cat -n memory/registry.yml | sed -n '1,220p'
printf '\n## feature docs matched by registry mention session/auth/demo if any\n'
rg -n "session-activity|auth|demo|signOut|shared account" memory/features memory/registry.ymlRepository: njrini99-code/helmv3
Length of output: 23430
🌐 Web query:
Supabase JavaScript auth.signOut default scope global documentation
💡 Result:
In the Supabase JavaScript client, the default scope for the auth.signOut method is 'global' [1][2]. This means that calling supabase.auth.signOut without any arguments will sign the user out of every device and browser session where they are currently logged in, not just the session in the current environment [1][3]. To sign the user out of only the current session, which is the recommended practice for most applications, you must explicitly pass { scope: 'local' } to the method [1][2][3]: await supabase.auth.signOut({ scope: 'local' }) Supabase provides three distinct scopes for the sign-out operation [2][4]: 1. global (default): Terminates all sessions associated with the user account across all devices [2][4]. 2. local: Terminates only the current session, leaving sessions on other devices or browsers active [1][2][4]. 3. others: Terminates all sessions for the user except for the current session [1][4]. Note that when using 'others', no 'SIGNED_OUT' event is fired for the current session [1][4].
Citations:
- 1: https://supabase.com/docs/reference/javascript/auth-signout
- 2: https://supabase.com/docs/guides/auth/signout
- 3: docs(auth): surface global-scope warning on signOut JSDoc supabase/supabase-js#2269
- 4: https://supabase.github.io/auth-js/v2/types/SignOut.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## auth-onboarding-join excerpts\n'
cat -n memory/features/auth-onboarding-join.md | sed -n '1,140p'
printf '\n## team-access-control excerpts\n'
cat -n memory/features/team-access-control.md | sed -n '1,120p'
printf '\n## search for demo/shared session guidance in auth docs\n'
rg -n "demo|shared|signOut|scope|session|logout|login" memory/features/auth-onboarding-join.md memory/features/team-access-control.md memory/features/settings-preferences.mdRepository: njrini99-code/helmv3
Length of output: 9697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## signOut call sites\n'
rg -n "auth\.signOut\(" src memory -g '!**/node_modules/**'
printf '\n## session-related auth helpers\n'
rg -n "signOut|session_expired|demo_session_expired|probeSignedIn|isDemoMetadataUser" src/lib src/app memory/features -g '!**/node_modules/**'Repository: njrini99-code/helmv3
Length of output: 12345
src/lib/auth/session-activity.ts:106 should sign out locally, not globally.
supabase.auth.signOut() defaults to scope: 'global', so this idle-timeout path can revoke the shared demo account for other active sessions. Use supabase.auth.signOut({ scope: 'local' }) here, matching src/lib/supabase/middleware.ts:576.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/auth/session-activity.ts` around lines 104 - 109, Update the
idle-timeout sign-out call in the session activity flow to pass the local scope
option to supabase.auth.signOut, matching the local sign-out behavior used by
the middleware. Keep the existing best-effort error handling and redirect flow
unchanged.
| export async function probeSignedIn( | ||
| supabase: AuthProbeClient, | ||
| timeoutMs: number = DEMO_AUTH_PROBE_TIMEOUT_MS, | ||
| ): Promise<ProbedUser | null> { | ||
| try { | ||
| const { data, error } = await raceWithTimeout(supabase.auth.getUser(), timeoutMs); | ||
| if (error) return null; | ||
| return data.user ?? null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * True when `user`'s metadata marks it as a demo session. Both `enterDemo` | ||
| * (golf) and `enterBaseballDemo` (baseball) stamp `user_metadata.is_demo = | ||
| * true` onto the shared demo account right after sign-in specifically so | ||
| * this check works WITHOUT exposing the demo account's real email to client | ||
| * code — unlike the email, "this is a demo session" carries no secret. | ||
| * | ||
| * Pure/sync — safe to call from the client, the server, and Edge middleware | ||
| * alike on a `user` object already in hand (no extra network round-trip). | ||
| */ | ||
| export function isDemoMetadataUser(user: ProbedUser | null | undefined): boolean { | ||
| return user?.user_metadata?.is_demo === true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Persist demo context independently of the expiring auth session.
The recovery path reads is_demo only from getUser(). Once authentication has actually expired or the probe returns null, middleware cannot enter its demo branch and the client intentionally falls back to password login—reproducing #918.
src/lib/demo/gate-probe.ts#L79-L104: support a durable demo-context marker independent ofProbedUser.src/lib/supabase/middleware.ts#L595-L614: read that marker when no authenticated user is recoverable.src/lib/auth/session-activity.ts#L96-L124: use the marker when the bounded probe fails.src/lib/auth/__tests__/session-activity.test.ts#L138-L154: expect the demo gate, not/golf/login, for a hung demo probe.src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts#L54-L135: adduser: nullexpiry coverage.
A sport-specific first-party cookie set during demo entry would preserve routing context without becoming an authorization signal.
📍 Affects 5 files
src/lib/demo/gate-probe.ts#L79-L104(this comment)src/lib/supabase/middleware.ts#L595-L614src/lib/auth/session-activity.ts#L96-L124src/lib/auth/__tests__/session-activity.test.ts#L138-L154src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts#L54-L135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/demo/gate-probe.ts` around lines 79 - 104, Persist demo routing
context in a sport-specific first-party cookie independent of ProbedUser, with
helpers in gate-probe.ts for setting and reading the non-authoritative marker.
Update middleware.ts and session-activity.ts to consult the marker when getUser
or the bounded probe yields no user, while retaining authentication as the
authorization signal. Update the specified session-activity test to expect the
demo gate for a hung demo probe, and add expired-session user:null coverage to
middleware-demo-session-expired.test.ts.
1 participant
Problem
Both demo gates (
/golf/demoand/baseball/demo, shared page structure) render "Checking sign-in status" while probingsupabase.auth.getUser()to power the "you're already in the demo — continue" shortcut.getUser()has no timeout of its own; when the browser holds a stale/expired httpOnly Supabase auth cookie, its internal refresh-token exchange can hang indefinitely — verified live, the spinner survives reloads and localStorage clears, and the visitor has no way to reach the form.Separately, the demo session idle-times-out (app-level 5-minute idle window) and redirects to
/golf/login?message=session_expired— a dead end for a demo visitor, since they never had a password to sign back in with.Fix
1. Hard-timeout probe (
src/lib/demo/gate-probe.ts, new)probeSignedIn()racessupabase.auth.getUser()against a hard 4s deadline viaraceWithTimeout()and treats ANY failure — timeout, thrown error, or a refresh-token error surfaced throughgetUser()'s ownerrorfield — as signed out. Wired into:isBaseballDemoSession()/isBaseballDemoAvailable()server-action calls (same hang risk, same spinner).useSessionActivity's idle-logout demo-detection probe (item 3 below) — a stuck check there must not block the logout redirect itself.2. Demo-aware
session_expiredroutingenterDemo/enterBaseballDemonow stampuser_metadata.is_demo = trueright after sign-in (best-effort, never blocks entry) — a non-secret signal readable by both client and server, unlike the demo account's email (which stays a server-only secret by existing convention).middleware.ts's idle-timeout redirect now checksuser_metadata.is_demo(falling back to an email match against the server-only demo credentials, covering sessions established before this fix shipped) and routes a demo session to/{sport}/demo?message=demo_session_expiredinstead of/{sport}/login.useSessionActivity's client-side idle logout does the same (resolves demo-context via the hard-timeout probe before signing out, since the metadata only exists on the still-live session).3. Investigated the ~30min demo session death
Queried the live Supabase project directly (read-only):
auth.sessions/auth.refresh_tokensfor the golf demo account (demo@golfhelmdemo.com) show zero rows despitelast_sign_in_at~40 minutes prior — the session is genuinely gone, not just idle.updated_at == created_at(never refreshed in place, only replaced).enterDemo's cookie-writing is byte-for-byte the same pattern as the regular login action (createClient()→signInWithPassword()→redirect()) — no demo-specific "short-lived session" or "skipped refresh wiring" in the code itself.This points to a Supabase Auth dashboard setting (session timebox, or single-session-per-user interacting badly with a shared demo account signed into by many concurrent visitors, or refresh-token-rotation reuse-detection under concurrent shared-account sign-ins) rather than a code bug in the demo entry flow — outside this repo/PR's scope to fix (dashboard-only, not migration-controlled). Flagging for Nick to check Authentication → Sessions settings. Item 2 above is the correct code-level mitigation regardless of the exact cause: whatever kills the session, the visitor now lands back on a working re-entry point instead of a dead end.
Gates
npx tsc --noEmit -p tsconfig.json— cleannpx eslinton all 12 changed/added files — cleannpx vitest runacross all new/changed test files — 296 tests passed, 0 failed (includes the pre-existing admin-idle-timeout and baseball demo-access suites, run to confirm no regressions)New/extended test coverage:
src/lib/demo/__tests__/gate-probe.test.ts—raceWithTimeoutpass-through + timeout behavior,probeSignedInhappy path /errorfield / thrown rejection / never-settling promise (the actual hang failure mode),isDemoMetadataUser.src/lib/supabase/__tests__/middleware-demo-session-expired.test.ts— demo redirect via metadata, email-match fallback, baseball parity, non-demo regression,is_demo: falsenot misread as demo.src/lib/auth/__tests__/session-activity.test.ts— demo/non-demo/admin idle-logout routing, and a hung-getUser()case proving the logout itself can't be wedged.src/app/golf/actions/__tests__/demo-access.test.ts(new) + extendedsrc/app/baseball/actions/__tests__/demo-access.test.ts—is_demometadata stamp ordering + never-blocks-entry-on-failure.Fixes #918
🤖 Generated with Claude Code